On Thu, Feb 29, 2024 at 04:57:26AM +0000, Sai Krishna Gajula wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > Sent: Wednesday, February 28, 2024 9:39 PM
> > To: Sai Krishna Gajula <saikrishnag@xxxxxxxxxxx>
> > Cc: bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> > richardcochran@xxxxxxxxx; horms@xxxxxxxxxx; vinicius.gomes@xxxxxxxxx;
> > vadim.fedorenko@xxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sunil Kovvuri
> > Goutham <sgoutham@xxxxxxxxxxx>; Geethasowjanya Akula
> > <gakula@xxxxxxxxxxx>; Linu Cherian <lcherian@xxxxxxxxxxx>; Hariprasad
> > Kelam <hkelam@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
> > <sbhatta@xxxxxxxxxxx>; Naveen Mamindlapalli <naveenm@xxxxxxxxxxx>
> > Subject: Re: [net-next PATCH v2] octeontx2: Add PTP clock driver for
> > Octeon PTM clock.
> >
> > On Wed, Feb 28, 2024 at 12:37:02PM +0000, Sai Krishna Gajula wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > > Sent: Monday, February 26, 2024 10:31 PM
> > > ...
> > > > On Mon, Feb 26, 2024 at 03:40:25PM +0000, Sai Krishna Gajula wrote:
> > > > > > -----Original Message-----
> > > > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > > > > Sent: Wednesday, February 14, 2024 10:59 PM ...
> > > > > > On Wed, Feb 14, 2024 at 06:38:53PM +0530, Sai Krishna wrote:
> > > > > > > The PCIe PTM(Precision time measurement) protocol provides
> > > > > > > precise coordination of events across multiple components like
> > > > > > > PCIe host clock, PCIe EP PHC local clocks of PCIe devices.
> > > > > > > This patch adds support for ptp clock based PTM clock. We can
> > > > > > > use this PTP device to sync the PTM time with CLOCK_REALTIME
> > > > > > > or other PTP PHC devices using phc2sys.
> >
> > > > > > > +static int __init ptp_oct_ptm_init(void) {
> > > > > > > + struct pci_dev *pdev = NULL;
> > > > > > > +
> > > > > > > + pdev = pci_get_device(PCI_VENDOR_ID_CAVIUM,
> > > > > > > + PCI_DEVID_OCTEONTX2_PTP, pdev);
> > > > > >
> > > > > > pci_get_device() is a sub-optimal method for a driver to claim a
> > device.
> > > > > > pci_register_driver() is the preferred method. If you can't use
> > > > > > that, a comment here explaining why not would be helpful.
> > > > >
> > > > > We just want to check the PTP device availability in the system as
> > > > > one of the use case is to sync PTM time to PTP.
> > > >
> > > > This doesn't explain why you can't use pci_register_driver(). Can
> > > > you clarify that?
> > >
> > > This is not a PCI endpoint driver. This piece of code is used to
> > > identify the silicon version. We will update the code by reading the
> > > silicon version from Endpoint internal BAR register offsets.
> >
> > > > I assume the PCI_DEVID_OCTEONTX2_PTP device is a PCIe Endpoint, and
> > > > this driver runs on the host? I.e., this driver does not run as
> > > > firmware on the Endpoint itself? So if you run lspci on the host,
> > > > you would see this device as one of the PCI devices?
> > > >
> > > > If that's the case, a driver would normally operate the device via
> > > > MMIO accesses to regions described by PCI BARs. "lspci -v" would
> > > > show those addresses.
> > >
> > > This driver don't run on Host but runs on the EP firmware itself.
> >
> > The "endpoint driver" terminology is a bit confusing here. See
> > Documentation/PCI/endpoint/pci-endpoint.rst for details.
> >
> > If this driver actually runs as part of the Endpoint firmware, it would not
> > normally see a hierarchy of pci_devs, and I don't think
> > pci_get_device() would work.
> >
> > So I suspect this driver actually runs on the host, and it looks like it wants to
> > use the same device (0x177d:0xa00c) as these two drivers:
> >
> > drivers/net/ethernet/cavium/common/cavium_ptp.c:#define
> > PCI_DEVICE_ID_CAVIUM_PTP 0xA00C
> > drivers/net/ethernet/marvell/octeontx2/af/ptp.c:#define
> > PCI_DEVID_OCTEONTX2_PTP 0xA00C
> >
> > It seems like maybe it should be integrated into them? Otherwise you have
> > multiple drivers thinking they are controlling a single device.
>
> Though this device does not appear as a PCI device on EP firmware,
> but there are some other internal PCI devices that will be
> enumerated.
>
> We will remove the dependency of reading the PTP device to check the
> SoC versions, instead we will read the config space of this PCI
> device itself.
>
> I hope this clears your doubt whether this driver is running on Host
> or EP device.
It does not. But I don't maintain this area and I'm not making any
progress on understanding how this works, so I don't think I can
give any useful advice here.
Bjorn
