Re: [net-next PATCH v2] octeontx2: Add PTP clock driver for Octeon PTM clock.

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Feb 29, 2024 at 04:57:26AM +0000, Sai Krishna Gajula wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Wednesday, February 28, 2024 9:39 PM
> > To: Sai Krishna Gajula <saikrishnag@marvell.com>
> > Cc: bhelgaas@google.com; linux-pci@vger.kernel.org;
> > richardcochran@gmail.com; horms@kernel.org; vinicius.gomes@intel.com;
> > vadim.fedorenko@linux.dev; davem@davemloft.net; kuba@kernel.org;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> > Goutham <sgoutham@marvell.com>; Geethasowjanya Akula
> > <gakula@marvell.com>; Linu Cherian <lcherian@marvell.com>; Hariprasad
> > Kelam <hkelam@marvell.com>; Subbaraya Sundeep Bhatta
> > <sbhatta@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
> > 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@kernel.org>
> > > > 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@kernel.org>
> > > > > > 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