On Thu, Feb 29, 2024 at 04:57:26AM +0000, Sai Krishna Gajula wrote:

quoted

-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: Wednesday, February 28, 2024 9:39 PM
To: Sai Krishna Gajula <redacted>
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 [off-list ref]; Geethasowjanya Akula
[off-list ref]; Linu Cherian [off-list ref]; Hariprasad
Kelam [off-list ref]; Subbaraya Sundeep Bhatta
[off-list ref]; Naveen Mamindlapalli [off-list ref]
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:

quoted

quoted

-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: Monday, February 26, 2024 10:31 PM

...

quoted

On Mon, Feb 26, 2024 at 03:40:25PM +0000, Sai Krishna Gajula wrote:

quoted

quoted

-----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:

quoted

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.

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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.

quoted

quoted

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