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

  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.

Signed-off-by: Sai Krishna <saikrishnag@marvell.com>
Signed-off-by: Naveen Mamindlapalli <naveenm@marvell.com>
Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
---
v2:
    - Addressed review comments given by Vadim Fedorenko, Vinicius, Simon Horman
	1. Driver build restricted to ARM64 and COMPILE_TEST+64BIT
        2. Fixed Sparse complaints by using readq/writeq like else where
        3. Modified error conditions, clode cleanup
        4. Forwarding to linux-pci maintainers as suggested by Jakub 

 drivers/ptp/Kconfig          |  11 ++
 drivers/ptp/Makefile         |   1 +
 drivers/ptp/ptp_octeon_ptm.c | 243 +++++++++++++++++++++++++++++++++++
 3 files changed, 255 insertions(+)
 create mode 100644 drivers/ptp/ptp_octeon_ptm.c
  

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.

s/PTM(/PTM (/   # space before open paren is conventional as in file comment
s/ptp/PTP/      # not sure if you intend "ptp" to be different from "PTP"?

Perhaps expand "PTP"?  I guess it must be "Precision Time Protocol",
which obviously would be well-known to all clock people since it's in
"drivers/ptp/" :)

> Signed-off-by: Sai Krishna <saikrishnag@marvell.com>
> Signed-off-by: Naveen Mamindlapalli <naveenm@marvell.com>
> Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>

Strictly speaking, I think the sender's Signed-off-by should be last
here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.6#n396

> ---
> v2:
>     - Addressed review comments given by Vadim Fedorenko, Vinicius, Simon Horman
> 	1. Driver build restricted to ARM64 and COMPILE_TEST+64BIT
>         2. Fixed Sparse complaints by using readq/writeq like else where
>         3. Modified error conditions, clode cleanup
>         4. Forwarding to linux-pci maintainers as suggested by Jakub 
> 
>  drivers/ptp/Kconfig          |  11 ++
>  drivers/ptp/Makefile         |   1 +
>  drivers/ptp/ptp_octeon_ptm.c | 243 +++++++++++++++++++++++++++++++++++
>  3 files changed, 255 insertions(+)
>  create mode 100644 drivers/ptp/ptp_octeon_ptm.c
> 
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 604541dcb320..3256b12842a6 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -224,4 +224,15 @@ config PTP_DFL_TOD
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called ptp_dfl_tod.
>  
> +config PTP_CLOCK_OCTEON
> +	tristate "OCTEON PTM PTP clock"
> +	depends on PTP_1588_CLOCK

I guess this can't even be compile-tested without PTP_1588_CLOCK?
Some subsystems supply stubs to allow compile testing even when the
subsystem is not enabled, but maybe ptp does not.

> +	depends on (64BIT && COMPILE_TEST) || ARM64

Why the 64BIT dependency?  Is it not even compile-testable without it?

> +	default n
> +	help
> +	  This driver adds support for using Octeon PTM device clock as
> +	  a PTP clock.

Another possible place to expand PTM and/or PTP.

> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ptp_octeon_ptm.
>  endmenu
> diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
> index 68bf02078053..19e2ab4c7f1b 100644
> --- a/drivers/ptp/Makefile
> +++ b/drivers/ptp/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_PTP_1588_CLOCK_MOCK)	+= ptp_mock.o
>  obj-$(CONFIG_PTP_1588_CLOCK_VMW)	+= ptp_vmw.o
>  obj-$(CONFIG_PTP_1588_CLOCK_OCP)	+= ptp_ocp.o
>  obj-$(CONFIG_PTP_DFL_TOD)		+= ptp_dfl_tod.o
> +obj-$(CONFIG_PTP_CLOCK_OCTEON)		+= ptp_octeon_ptm.o
> diff --git a/drivers/ptp/ptp_octeon_ptm.c b/drivers/ptp/ptp_octeon_ptm.c
> new file mode 100644
> index 000000000000..6867c1d28f17
> --- /dev/null
> +++ b/drivers/ptp/ptp_octeon_ptm.c
> @@ -0,0 +1,243 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell PTP PHC clock driver for PCIe PTM (Precision Time Measurement) EP
> + *
> + * Copyright (c) 2024 Marvell.
> + *

Spurious blank line.

> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +
> +#include <linux/ptp_clock_kernel.h>
> +
> +#include "ptp_private.h"
> +
> +#define PEMX_PFX_CSX_PFCFGX(pem, pf, _offset)	({typeof(_offset) (offset) = (_offset); \
> +						((0x8e0000008000 | (u64)(pem) << 36 \
> +						| (pf) << 18 \
> +						| (((offset) >> 16) & 1) << 16 \
> +						| ((offset) >> 3) << 3) \
> +						+ ((((offset) >> 2) & 1) << 2)); })
> +
> +#define PEMX_CFG_WR(a)			(0x8E0000000018ull | (u64)(a) << 36)
> +#define PEMX_CFG_RD(a)			(0x8E0000000020ull | (u64)(a) << 36)
> +
> +/* Octeon CSRs   */
> +#define PEMX_CFG                        0x8e00000000d8ULL
> +#define PEMX_PTM_CTL			0x8e0000000098ULL
> +#define PEMX_PTM_CTL_CAP		BIT_ULL(10)
> +#define PEMX_PTM_LCL_TIME		0x8e00000000a0ULL /* PTM time */
> +#define PEMX_PTM_MAS_TIME		0x8e00000000a8ULL /* PTP time */
> +
> +struct oct_ptp_clock {
> +	struct ptp_clock *ptp_clock;
> +	struct ptp_clock_info caps;
> +	bool cn10k_variant;
> +};
> +
> +static struct oct_ptp_clock oct_ptp_clock;
> +static void __iomem *ptm_ctl_addr;
> +static void __iomem *ptm_lcl_addr;
> +
> +/* Config space registers   */

Spurious spaces at end of comment.

> +#define PCIEEPX_PTM_REQ_STAT		(oct_ptp_clock.cn10k_variant ? 0x3a8 : 0x474)
> +#define PCIEEPX_PTM_REQ_T1L		(oct_ptp_clock.cn10k_variant ? 0x3b4 : 0x480)
> +#define PCIEEPX_PTM_REQ_T1M		(oct_ptp_clock.cn10k_variant ? 0x3b8 : 0x484)
> +#define PCIEEPX_PTM_REQ_T4L		(oct_ptp_clock.cn10k_variant ? 0x3c4 : 0x490)
> +#define PCIEEPX_PTM_REQ_T4M		(oct_ptp_clock.cn10k_variant ? 0x3c8 : 0x494)
> +
> +#define PCI_VENDOR_ID_CAVIUM			0x177d
> +#define PCI_DEVID_OCTEONTX2_PTP			0xA00C
> +#define PCI_SUBSYS_DEVID_95XX			0xB300
> +#define PCI_SUBSYS_DEVID_95XXN			0xB400
> +#define PCI_SUBSYS_DEVID_95XXMM			0xB500
> +#define PCI_SUBSYS_DEVID_96XX			0xB200
> +#define PCI_SUBSYS_DEVID_98XX			0xB100
> +#define PCI_SUBSYS_DEVID_CN10K_A		0xB900
> +#define PCI_SUBSYS_DEVID_CN10K_B		0xBD00
> +#define PCI_SUBSYS_DEVID_CNF10K_A		0xBA00
> +#define PCI_SUBSYS_DEVID_CNF10K_B		0xBC00

Random mixture of upper-case and lower-case hex above.  Also random
usage of "ull" vs "ULL".

> +static bool is_otx2_support_ptm(struct pci_dev *pdev)
> +{
> +	return (pdev->subsystem_device == PCI_SUBSYS_DEVID_96XX ||
> +		pdev->subsystem_device == PCI_SUBSYS_DEVID_95XX ||
> +		pdev->subsystem_device == PCI_SUBSYS_DEVID_95XXN ||
> +		pdev->subsystem_device == PCI_SUBSYS_DEVID_98XX ||
> +		pdev->subsystem_device == PCI_SUBSYS_DEVID_95XXMM);
> +}
> +
> +static bool is_cn10k_support_ptm(struct pci_dev *pdev)
> +{
> +	return (pdev->subsystem_device == PCI_SUBSYS_DEVID_CN10K_A ||
> +		pdev->subsystem_device == PCI_SUBSYS_DEVID_CNF10K_A ||
> +		pdev->subsystem_device == PCI_SUBSYS_DEVID_CN10K_B ||
> +		pdev->subsystem_device == PCI_SUBSYS_DEVID_CNF10K_B);
> +}
> +
> +static int ptp_oct_ptm_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ptp_oct_ptm_settime(struct ptp_clock_info *ptp,
> +			       const struct timespec64 *ts)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static u32 read_pcie_config32(int ep_pem, int cfg_addr)
> +{
> +	void __iomem *addr;
> +	u64 val;
> +
> +	if (oct_ptp_clock.cn10k_variant) {
> +		addr = ioremap(PEMX_PFX_CSX_PFCFGX(ep_pem, 0, cfg_addr), 8);

These ioremap()s look like things that should be done at probe-time
and retained for the life of the module since (I assume) this will be
called many times.

> +		if (!addr) {
> +			pr_err("PTM_EP: Failed to ioremap Octeon CSR space\n");
> +			return -1U;
> +		}
> +		val = readl(addr);
> +		iounmap(addr);
> +	} else {
> +		addr = ioremap(PEMX_CFG_RD(ep_pem), 8);
> +		if (!addr) {
> +			pr_err("PTM_EP: Failed to ioremap Octeon CSR space\n");
> +			return -1U;
> +		}
> +		val = ((1 << 15) | (cfg_addr & 0xfff));
> +		writeq(val, addr);
> +		val = readq(addr) >> 32;
> +		iounmap(addr);
> +	}
> +	return (val & 0xffffffff);
> +}
> +
> +static uint64_t octeon_csr_read(u64 csr_addr)
> +{
> +	void __iomem *addr;
> +	u64 val;
> +
> +	addr = ioremap(csr_addr, 8);
> +	if (!addr) {
> +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> +		return -1UL;
> +	}
> +	val = readq(addr);
> +	iounmap(addr);
> +	return val;
> +}
> +
> +static int ptp_oct_ptm_gettime(struct ptp_clock_info *ptp, struct timespec64 *ts)
> +{
> +	u64 ptp_time, val64;
> +	u32 val32;
> +
> +	/* Check for valid PTM context */
> +	val32 = read_pcie_config32(0, PCIEEPX_PTM_REQ_STAT);
> +	if (!(val32 & 0x1)) {
> +		pr_err("PTM_EP: ERROR: PTM context not valid: 0x%x\n", val32);
> +
> +		ts->tv_sec = 0;
> +		ts->tv_nsec = 0;
> +
> +		return -EINVAL;
> +	}
> +
> +	/* Trigger PTM/PTP capture */
> +	val64 = readq(ptm_ctl_addr);
> +	val64 |= PEMX_PTM_CTL_CAP;
> +	writeq(val64, ptm_ctl_addr);
> +	/* Read PTM/PTP clocks  */
> +	ptp_time = readq(ptm_lcl_addr);
> +
> +	*ts = ns_to_timespec64(ptp_time);
> +
> +	return 0;
> +}
> +
> +static int ptp_oct_ptm_enable(struct ptp_clock_info *ptp,
> +			      struct ptp_clock_request *rq, int on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static const struct ptp_clock_info ptp_oct_caps = {
> +	.owner		= THIS_MODULE,
> +	.name		= "OCTEON PTM PHC",
> +	.max_adj	= 0,
> +	.n_ext_ts	= 0,
> +	.n_pins		= 0,
> +	.pps		= 0,

Initialization to zero is unnecessary, but maybe it's the local
drivers/ptp/ style.

> +	.adjtime	= ptp_oct_ptm_adjtime,
> +	.gettime64	= ptp_oct_ptm_gettime,
> +	.settime64	= ptp_oct_ptm_settime,
> +	.enable		= ptp_oct_ptm_enable,
> +};
> +
> +static void __exit ptp_oct_ptm_exit(void)
> +{
> +	iounmap(ptm_ctl_addr);
> +	iounmap(ptm_lcl_addr);
> +	ptp_clock_unregister(oct_ptp_clock.ptp_clock);
> +}
> +
> +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.

> +	if (!pdev)
> +		return 0;
> +
> +	if (octeon_csr_read(PEMX_CFG) & 0x1ULL) {
> +		pr_err("PEM0 is configured as RC\n");

pci_err() or dev_err() etc. when possible.  Maybe #define dev_fmt or
pr_fmt as appropriate.

> +		return 0;
> +	}
> +
> +	if (is_otx2_support_ptm(pdev)) {
> +		oct_ptp_clock.cn10k_variant = 0;
> +	} else if (is_cn10k_support_ptm(pdev)) {
> +		oct_ptp_clock.cn10k_variant = 1;
> +	} else {
> +		/* PTM_EP: unsupported processor */
> +		return 0;
> +	}
> +
> +	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?

> +	if (!ptm_ctl_addr) {
> +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> +		return 0;
> +	}
> +
> +	ptm_lcl_addr = ioremap(PEMX_PTM_LCL_TIME, 8);
> +	if (!ptm_lcl_addr) {
> +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> +		return 0;
> +	}
> +
> +	oct_ptp_clock.caps = ptp_oct_caps;
> +
> +	oct_ptp_clock.ptp_clock = ptp_clock_register(&oct_ptp_clock.caps, NULL);
> +	if (IS_ERR(oct_ptp_clock.ptp_clock))
> +		return PTR_ERR(oct_ptp_clock.ptp_clock);
> +
> +	pr_info("PTP device index for PTM clock:%d\n", oct_ptp_clock.ptp_clock->index);
> +	pr_info("cn10k_variant %d\n", oct_ptp_clock.cn10k_variant);

Combine into single line; otherwise there's no hint in the dmesg log
of what "cn10k_variant" is connected to (though dev_fmt/pr_fmt would
also help with this).

> +	return 0;
> +}
> +
> +module_init(ptp_oct_ptm_init);
> +module_exit(ptp_oct_ptm_exit);
> +
> +MODULE_AUTHOR("Marvell Inc.");
> +MODULE_DESCRIPTION("PTP PHC clock using PTM");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
>
  

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, February 14, 2024 10:59 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 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.
> 
> s/PTM(/PTM (/   # space before open paren is conventional as in file comment
> s/ptp/PTP/      # not sure if you intend "ptp" to be different from "PTP"?
> 
> Perhaps expand "PTP"?  I guess it must be "Precision Time Protocol", which
> obviously would be well-known to all clock people since it's in "drivers/ptp/"
> :)
Ack, Will submit patch V3 with the suggestions/changes

> 
> > Signed-off-by: Sai Krishna <saikrishnag@marvell.com>
> > Signed-off-by: Naveen Mamindlapalli <naveenm@marvell.com>
> > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
> 
> Strictly speaking, I think the sender's Signed-off-by should be last
> here:
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__git.kernel.org_pub_scm_linux_kernel_git_torvalds_linux.git_tree_Docum
> entation_process_submitting-2Dpatches.rst-3Fid-3Dv6.6-
> 23n396&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=c3MsgrR-U-
> HFhmFd6R4MWRZG-8QeikJn5PkjqMTpBSg&m=-
> hUWeOgCxq0JK2uXUtjKrhTZxpTXRF4VzG5fgtC2LX1KB1FOV9PkK5E_fsjNvncM&
> s=O3K3uhTGzhVQbSkfb_MSDRhdqcoqyqjLbVASMs7ouEw&e=
> 
Ack, Will submit patch V3 with the sign-off re-order change

> > ---
> > v2:
> >     - Addressed review comments given by Vadim Fedorenko, Vinicius, Simon
> Horman
> > 	1. Driver build restricted to ARM64 and COMPILE_TEST+64BIT
> >         2. Fixed Sparse complaints by using readq/writeq like else where
> >         3. Modified error conditions, clode cleanup
> >         4. Forwarding to linux-pci maintainers as suggested by Jakub
> >
> >  drivers/ptp/Kconfig          |  11 ++
> >  drivers/ptp/Makefile         |   1 +
> >  drivers/ptp/ptp_octeon_ptm.c | 243
> > +++++++++++++++++++++++++++++++++++
> >  3 files changed, 255 insertions(+)
> >  create mode 100644 drivers/ptp/ptp_octeon_ptm.c
> >
> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index
> > 604541dcb320..3256b12842a6 100644
> > --- a/drivers/ptp/Kconfig
> > +++ b/drivers/ptp/Kconfig
> > @@ -224,4 +224,15 @@ config PTP_DFL_TOD
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called ptp_dfl_tod.
> >
> > +config PTP_CLOCK_OCTEON
> > +	tristate "OCTEON PTM PTP clock"
> > +	depends on PTP_1588_CLOCK
> 
> I guess this can't even be compile-tested without PTP_1588_CLOCK?
> Some subsystems supply stubs to allow compile testing even when the
> subsystem is not enabled, but maybe ptp does not.

Yes

> 
> > +	depends on (64BIT && COMPILE_TEST) || ARM64
> 
> Why the 64BIT dependency?  Is it not even compile-testable without it?

readq/writeq calls in the driver did not compile for x86 32bit systems. Hence added 64BIT dependency.

> 
> > +	default n
> > +	help
> > +	  This driver adds support for using Octeon PTM device clock as
> > +	  a PTP clock.
> 
> Another possible place to expand PTM and/or PTP.

Ack, Will submit patch V3 with the suggestions/changes

> 
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called ptp_octeon_ptm.
> >  endmenu
> > diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile index
> > 68bf02078053..19e2ab4c7f1b 100644
> > --- a/drivers/ptp/Makefile
> > +++ b/drivers/ptp/Makefile
> > @@ -21,3 +21,4 @@ obj-$(CONFIG_PTP_1588_CLOCK_MOCK)	+=
> ptp_mock.o
> >  obj-$(CONFIG_PTP_1588_CLOCK_VMW)	+= ptp_vmw.o
> >  obj-$(CONFIG_PTP_1588_CLOCK_OCP)	+= ptp_ocp.o
> >  obj-$(CONFIG_PTP_DFL_TOD)		+= ptp_dfl_tod.o
> > +obj-$(CONFIG_PTP_CLOCK_OCTEON)		+= ptp_octeon_ptm.o
> > diff --git a/drivers/ptp/ptp_octeon_ptm.c
> > b/drivers/ptp/ptp_octeon_ptm.c new file mode 100644 index
> > 000000000000..6867c1d28f17
> > --- /dev/null
> > +++ b/drivers/ptp/ptp_octeon_ptm.c
> > @@ -0,0 +1,243 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell PTP PHC clock driver for PCIe PTM (Precision Time
> > +Measurement) EP
> > + *
> > + * Copyright (c) 2024 Marvell.
> > + *
> 
> Spurious blank line.

Ack, Will submit patch V3 with the suggestions/changes
> 
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/pci.h>
> > +#include <linux/slab.h>
> > +#include <linux/module.h>
> > +
> > +#include <linux/ptp_clock_kernel.h>
> > +
> > +#include "ptp_private.h"
> > +
> > +#define PEMX_PFX_CSX_PFCFGX(pem, pf, _offset)	({typeof(_offset)
> (offset) = (_offset); \
> > +						((0x8e0000008000 |
> (u64)(pem) << 36 \
> > +						| (pf) << 18 \
> > +						| (((offset) >> 16) & 1) << 16 \
> > +						| ((offset) >> 3) << 3) \
> > +						+ ((((offset) >> 2) & 1) << 2));
> })
> > +
> > +#define PEMX_CFG_WR(a)			(0x8E0000000018ull |
> (u64)(a) << 36)
> > +#define PEMX_CFG_RD(a)			(0x8E0000000020ull |
> (u64)(a) << 36)
> > +
> > +/* Octeon CSRs   */
> > +#define PEMX_CFG                        0x8e00000000d8ULL
> > +#define PEMX_PTM_CTL			0x8e0000000098ULL
> > +#define PEMX_PTM_CTL_CAP		BIT_ULL(10)
> > +#define PEMX_PTM_LCL_TIME		0x8e00000000a0ULL /* PTM
> time */
> > +#define PEMX_PTM_MAS_TIME		0x8e00000000a8ULL /* PTP
> time */
> > +
> > +struct oct_ptp_clock {
> > +	struct ptp_clock *ptp_clock;
> > +	struct ptp_clock_info caps;
> > +	bool cn10k_variant;
> > +};
> > +
> > +static struct oct_ptp_clock oct_ptp_clock; static void __iomem
> > +*ptm_ctl_addr; static void __iomem *ptm_lcl_addr;
> > +
> > +/* Config space registers   */
> 
> Spurious spaces at end of comment.

Ack, Will submit patch V3 with the suggestions/changes

> 
> > +#define PCIEEPX_PTM_REQ_STAT		(oct_ptp_clock.cn10k_variant
> ? 0x3a8 : 0x474)
> > +#define PCIEEPX_PTM_REQ_T1L		(oct_ptp_clock.cn10k_variant
> ? 0x3b4 : 0x480)
> > +#define PCIEEPX_PTM_REQ_T1M		(oct_ptp_clock.cn10k_variant
> ? 0x3b8 : 0x484)
> > +#define PCIEEPX_PTM_REQ_T4L		(oct_ptp_clock.cn10k_variant
> ? 0x3c4 : 0x490)
> > +#define PCIEEPX_PTM_REQ_T4M		(oct_ptp_clock.cn10k_variant
> ? 0x3c8 : 0x494)
> > +
> > +#define PCI_VENDOR_ID_CAVIUM			0x177d
> > +#define PCI_DEVID_OCTEONTX2_PTP			0xA00C
> > +#define PCI_SUBSYS_DEVID_95XX			0xB300
> > +#define PCI_SUBSYS_DEVID_95XXN			0xB400
> > +#define PCI_SUBSYS_DEVID_95XXMM			0xB500
> > +#define PCI_SUBSYS_DEVID_96XX			0xB200
> > +#define PCI_SUBSYS_DEVID_98XX			0xB100
> > +#define PCI_SUBSYS_DEVID_CN10K_A		0xB900
> > +#define PCI_SUBSYS_DEVID_CN10K_B		0xBD00
> > +#define PCI_SUBSYS_DEVID_CNF10K_A		0xBA00
> > +#define PCI_SUBSYS_DEVID_CNF10K_B		0xBC00
> 
> Random mixture of upper-case and lower-case hex above.  Also random
> usage of "ull" vs "ULL".
> 

Ack, Will submit patch V3 with the suggestions/changes

> > +static bool is_otx2_support_ptm(struct pci_dev *pdev) {
> > +	return (pdev->subsystem_device == PCI_SUBSYS_DEVID_96XX ||
> > +		pdev->subsystem_device == PCI_SUBSYS_DEVID_95XX ||
> > +		pdev->subsystem_device == PCI_SUBSYS_DEVID_95XXN ||
> > +		pdev->subsystem_device == PCI_SUBSYS_DEVID_98XX ||
> > +		pdev->subsystem_device == PCI_SUBSYS_DEVID_95XXMM); }
> > +
> > +static bool is_cn10k_support_ptm(struct pci_dev *pdev) {
> > +	return (pdev->subsystem_device == PCI_SUBSYS_DEVID_CN10K_A ||
> > +		pdev->subsystem_device == PCI_SUBSYS_DEVID_CNF10K_A
> ||
> > +		pdev->subsystem_device == PCI_SUBSYS_DEVID_CN10K_B ||
> > +		pdev->subsystem_device == PCI_SUBSYS_DEVID_CNF10K_B); }
> > +
> > +static int ptp_oct_ptm_adjtime(struct ptp_clock_info *ptp, s64 delta)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static int ptp_oct_ptm_settime(struct ptp_clock_info *ptp,
> > +			       const struct timespec64 *ts) {
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static u32 read_pcie_config32(int ep_pem, int cfg_addr) {
> > +	void __iomem *addr;
> > +	u64 val;
> > +
> > +	if (oct_ptp_clock.cn10k_variant) {
> > +		addr = ioremap(PEMX_PFX_CSX_PFCFGX(ep_pem, 0,
> cfg_addr), 8);
> 
> These ioremap()s look like things that should be done at probe-time and
> retained for the life of the module since (I assume) this will be called many
> times.
> 

Ack, Will explore the suggestion and submit patch V3 with the changes

> > +		if (!addr) {
> > +			pr_err("PTM_EP: Failed to ioremap Octeon CSR
> space\n");
> > +			return -1U;
> > +		}
> > +		val = readl(addr);
> > +		iounmap(addr);
> > +	} else {
> > +		addr = ioremap(PEMX_CFG_RD(ep_pem), 8);
> > +		if (!addr) {
> > +			pr_err("PTM_EP: Failed to ioremap Octeon CSR
> space\n");
> > +			return -1U;
> > +		}
> > +		val = ((1 << 15) | (cfg_addr & 0xfff));
> > +		writeq(val, addr);
> > +		val = readq(addr) >> 32;
> > +		iounmap(addr);
> > +	}
> > +	return (val & 0xffffffff);
> > +}
> > +
> > +static uint64_t octeon_csr_read(u64 csr_addr) {
> > +	void __iomem *addr;
> > +	u64 val;
> > +
> > +	addr = ioremap(csr_addr, 8);
> > +	if (!addr) {
> > +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> > +		return -1UL;
> > +	}
> > +	val = readq(addr);
> > +	iounmap(addr);
> > +	return val;
> > +}
> > +
> > +static int ptp_oct_ptm_gettime(struct ptp_clock_info *ptp, struct
> > +timespec64 *ts) {
> > +	u64 ptp_time, val64;
> > +	u32 val32;
> > +
> > +	/* Check for valid PTM context */
> > +	val32 = read_pcie_config32(0, PCIEEPX_PTM_REQ_STAT);
> > +	if (!(val32 & 0x1)) {
> > +		pr_err("PTM_EP: ERROR: PTM context not valid: 0x%x\n",
> val32);
> > +
> > +		ts->tv_sec = 0;
> > +		ts->tv_nsec = 0;
> > +
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Trigger PTM/PTP capture */
> > +	val64 = readq(ptm_ctl_addr);
> > +	val64 |= PEMX_PTM_CTL_CAP;
> > +	writeq(val64, ptm_ctl_addr);
> > +	/* Read PTM/PTP clocks  */
> > +	ptp_time = readq(ptm_lcl_addr);
> > +
> > +	*ts = ns_to_timespec64(ptp_time);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ptp_oct_ptm_enable(struct ptp_clock_info *ptp,
> > +			      struct ptp_clock_request *rq, int on) {
> > +	return -EOPNOTSUPP;
> > +}
> > +
> > +static const struct ptp_clock_info ptp_oct_caps = {
> > +	.owner		= THIS_MODULE,
> > +	.name		= "OCTEON PTM PHC",
> > +	.max_adj	= 0,
> > +	.n_ext_ts	= 0,
> > +	.n_pins		= 0,
> > +	.pps		= 0,
> 
> Initialization to zero is unnecessary, but maybe it's the local drivers/ptp/ style.

Yes

> 
> > +	.adjtime	= ptp_oct_ptm_adjtime,
> > +	.gettime64	= ptp_oct_ptm_gettime,
> > +	.settime64	= ptp_oct_ptm_settime,
> > +	.enable		= ptp_oct_ptm_enable,
> > +};
> > +
> > +static void __exit ptp_oct_ptm_exit(void) {
> > +	iounmap(ptm_ctl_addr);
> > +	iounmap(ptm_lcl_addr);
> > +	ptp_clock_unregister(oct_ptp_clock.ptp_clock);
> > +}
> > +
> > +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.

> > +	if (!pdev)
> > +		return 0;
> > +
> > +	if (octeon_csr_read(PEMX_CFG) & 0x1ULL) {
> > +		pr_err("PEM0 is configured as RC\n");
> 
> pci_err() or dev_err() etc. when possible.  Maybe #define dev_fmt or pr_fmt
> as appropriate.
> 

Ack, Will submit patch V3 with the suggestions/changes

> > +		return 0;
> > +	}
> > +
> > +	if (is_otx2_support_ptm(pdev)) {
> > +		oct_ptp_clock.cn10k_variant = 0;
> > +	} else if (is_cn10k_support_ptm(pdev)) {
> > +		oct_ptp_clock.cn10k_variant = 1;
> > +	} else {
> > +		/* PTM_EP: unsupported processor */
> > +		return 0;
> > +	}
> > +
> > +	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.

> > +	if (!ptm_ctl_addr) {
> > +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> > +		return 0;
> > +	}
> > +
> > +	ptm_lcl_addr = ioremap(PEMX_PTM_LCL_TIME, 8);
> > +	if (!ptm_lcl_addr) {
> > +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> > +		return 0;
> > +	}
> > +
> > +	oct_ptp_clock.caps = ptp_oct_caps;
> > +
> > +	oct_ptp_clock.ptp_clock = ptp_clock_register(&oct_ptp_clock.caps,
> NULL);
> > +	if (IS_ERR(oct_ptp_clock.ptp_clock))
> > +		return PTR_ERR(oct_ptp_clock.ptp_clock);
> > +
> > +	pr_info("PTP device index for PTM clock:%d\n",
> oct_ptp_clock.ptp_clock->index);
> > +	pr_info("cn10k_variant %d\n", oct_ptp_clock.cn10k_variant);
> 
> Combine into single line; otherwise there's no hint in the dmesg log of what
> "cn10k_variant" is connected to (though dev_fmt/pr_fmt would also help
> with this).

Ack, Will submit patch V3 with the suggestions/changes

> 
> > +	return 0;
> > +}
> > +
> > +module_init(ptp_oct_ptm_init);
> > +module_exit(ptp_oct_ptm_exit);
> > +
> > +MODULE_AUTHOR("Marvell Inc.");
> > +MODULE_DESCRIPTION("PTP PHC clock using PTM");
> MODULE_LICENSE("GPL");
> > --
> > 2.25.1
> >
  

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
  

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Monday, February 26, 2024 10:31 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 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.

Ack, will use this V3 patch

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

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

This driver don't run on Host but runs on the EP firmware itself.

> 
> Bjorn

Thanks,
Sai
  

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
  

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

> 
> Bjorn
  

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