> -----Original Message----- > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Sent: Wednesday, February 14, 2024 10:59 PM > To: Sai Krishna Gajula <saikrishnag@xxxxxxxxxxx> > Cc: bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; > richardcochran@xxxxxxxxx; horms@xxxxxxxxxx; vinicius.gomes@xxxxxxxxx; > vadim.fedorenko@xxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Sunil Kovvuri > Goutham <sgoutham@xxxxxxxxxxx>; Geethasowjanya Akula > <gakula@xxxxxxxxxxx>; Linu Cherian <lcherian@xxxxxxxxxxx>; Hariprasad > Kelam <hkelam@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta > <sbhatta@xxxxxxxxxxx>; Naveen Mamindlapalli <naveenm@xxxxxxxxxxx> > 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@xxxxxxxxxxx> > > Signed-off-by: Naveen Mamindlapalli <naveenm@xxxxxxxxxxx> > > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx> > > 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 > >
