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

[Bjorn Helgaas <helgaas@kernel.org>]

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.

> > > +#define PCI_VENDOR_ID_CAVIUM			0x177d

Already defined in pci_ids.h.

> > > +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?

> > > +	ptm_ctl_addr = ioremap(PEMX_PTM_CTL, 8);
> > 
> > Hard-coded register addresses?  That can't be right.  Shouldn't
> > this be discoverable either as a PCI BAR or via DT or similar
> > firmware interface?
> 
> Ack, will explore the DT implementation for register addresses
> access and submit patch V3. Thanks for the review.

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.

Bjorn