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

[Bjorn Helgaas <helgaas@kernel.org>]

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.

Bjorn