Re: [PATCH 3/4] PCI: dwc: Add sysfs support for PTM

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Feb 18, 2025 at 07:54:24PM +0200, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 08:06:42PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > Precision Time Management (PTM) mechanism defined in PCIe spec r6.0,
> > sec 6.22 allows precise coordination of timing information across multiple
> > components in a PCIe hierarchy with independent local time clocks.
> > 
> > While the PTM support itself is indicated by the presence of PTM Extended
> > Capability structure, Synopsys Designware IPs expose the PTM context
> > (timing information) through Vendor Specific Extended Capability (VSEC)
> > registers.
> > 
> > Hence, add the sysfs support to expose the PTM context information to
> > userspace from both PCIe RC and EP controllers. Below PTM context are
> > exposed through sysfs:
> > 
> > PCIe RC
> > =======
> > 
> > 1. PTM Local clock
> > 2. PTM T2 timestamp
> > 3. PTM T3 timestamp
> > 4. PTM Context valid
> > 
> > PCIe EP
> > =======
> > 
> > 1. PTM Local clock
> > 2. PTM T1 timestamp
> > 3. PTM T4 timestamp
> > 4. PTM Master clock
> > 5. PTM Context update
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  Documentation/ABI/testing/sysfs-platform-dwc-pcie  |  70 ++++++
> >  MAINTAINERS                                        |   1 +
> >  drivers/pci/controller/dwc/Makefile                |   2 +-
> >  drivers/pci/controller/dwc/pcie-designware-ep.c    |   3 +
> >  drivers/pci/controller/dwc/pcie-designware-host.c  |   4 +
> >  drivers/pci/controller/dwc/pcie-designware-sysfs.c | 278 +++++++++++++++++++++
> >  drivers/pci/controller/dwc/pcie-designware.c       |   6 +
> >  drivers/pci/controller/dwc/pcie-designware.h       |  22 ++
> >  include/linux/pcie-dwc.h                           |   8 +
> >  9 files changed, 393 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-dwc-pcie b/Documentation/ABI/testing/sysfs-platform-dwc-pcie
> > new file mode 100644
> > index 000000000000..6b429108cd09
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-platform-dwc-pcie
> 
> Should be a class or just a ptm group in the PCIe controller device? How
> generic are those attributes?
> 

Even though these are generic attributes, the way PTM support is exposed in
kernel right now makes it harder to make these as generic attributes. These
attributes are specific to RC/EP controllers and the generic PTM driver is for
endpoint devices. Maybe I could think of exposing it for RC/EP controller
drivers (not just DWC). But still then these would be exposed as a group under
each platform device.

> > @@ -0,0 +1,70 @@
> > +What:		/sys/devices/platform/*/dwc/ptm/ptm_local_clock
> > +Date:		February 2025
> > +Contact:	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > +Description:
> > +		(RO) PTM local clock in nanoseconds. Applicable for both Root
> > +		Complex and Endpoint mode.

[...]

> > +static umode_t ptm_attr_visible(struct kobject *kobj, struct attribute *attr,
> > +				int n)
> > +{
> > +	struct device *dev = container_of(kobj, struct device, kobj);
> > +	struct dw_pcie *pci = dev_get_drvdata(dev);
> > +
> > +	/* RC only needs local, t2 and t3 clocks and context_valid */
> > +	if ((attr == &dev_attr_ptm_t1.attr && pci->mode == DW_PCIE_RC_TYPE) ||
> > +	    (attr == &dev_attr_ptm_t4.attr && pci->mode == DW_PCIE_RC_TYPE) ||
> > +	    (attr == &dev_attr_ptm_master_clock.attr && pci->mode == DW_PCIE_RC_TYPE) ||
> > +	    (attr == &dev_attr_ptm_context_update.attr && pci->mode == DW_PCIE_RC_TYPE))
> > +		return 0;
> 
> The pci->mode checks definitely can be refactored to a top-level instead
> of being repeated on each line.
> 

Ok.

> > +
> > +	/* EP only needs local, master, t1, and t4 clocks and context_update */
> > +	if ((attr == &dev_attr_ptm_t2.attr && pci->mode == DW_PCIE_EP_TYPE) ||
> > +	    (attr == &dev_attr_ptm_t3.attr && pci->mode == DW_PCIE_EP_TYPE) ||
> > +	    (attr == &dev_attr_ptm_context_valid.attr && pci->mode == DW_PCIE_EP_TYPE))
> > +		return 0;
> > +
> > +	return attr->mode;
> 
> I think it might be better to register two separate groups, one for RC,
> one for EP and use presense of the corresponding capability in the
> .is_visible callback to check if the PTM attributes should be visible at
> all.
> 

What benefit does it provide? I did thought about this idea, but then I didn't
find useful since the top level platform device (RC/EP) should itself
distinguish between PTM requester and responder. So one more differentiation
seemed overkill to me.

> > +}
> > +
> > +static const struct attribute_group ptm_attr_group = {
> > +	.name = "ptm",
> > +	.attrs = ptm_attrs,
> > +	.is_visible = ptm_attr_visible,
> > +};
> > +
> > +static const struct attribute_group *dwc_pcie_attr_groups[] = {
> > +	&ptm_attr_group,
> > +	NULL,
> > +};
> > +
> > +static void pcie_designware_sysfs_release(struct device *dev)
> > +{
> > +	kfree(dev);
> > +}
> > +
> > +void pcie_designware_sysfs_init(struct dw_pcie *pci,
> > +				    enum dw_pcie_device_mode mode)
> > +{
> > +	struct device *dev;
> > +	int ret;
> > +
> > +	/* Check for capabilities before creating sysfs attrbutes */
> > +	ret = dw_pcie_find_ptm_capability(pci);
> > +	if (!ret) {
> > +		dev_dbg(pci->dev, "PTM capability not present\n");
> > +		return;
> > +	}
> > +
> > +	pci->ptm_vsec_offset = ret;
> > +	pci->mode = mode;
> > +
> > +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +	if (!dev)
> > +		return;
> > +
> > +	device_initialize(dev);
> > +	dev->groups = dwc_pcie_attr_groups;
> > +	dev->release = pcie_designware_sysfs_release;
> > +	dev->parent = pci->dev;
> > +	dev_set_drvdata(dev, pci);
> > +
> > +	ret = dev_set_name(dev, "dwc");
> > +	if (ret)
> > +		goto err_free;
> > +
> > +	ret = device_add(dev);
> > +	if (ret)
> > +		goto err_free;
> > +
> > +	pci->sysfs_dev = dev;
> 
> Why do you need to add a new device under the PCIe controller?
> 

Just because we cannot reference the 'struct dw_pcie' from the 'struct device'
belonging to the platform device. All the controller drivers are already setting
their own private structure as drvdata.

> > +
> > +	return;
> > +
> > +err_free:
> > +	put_device(dev);
> > +}
> > +
> > +void pcie_designware_sysfs_exit(struct dw_pcie *pci)
> > +{
> > +	if (pci->sysfs_dev)
> > +		device_unregister(pci->sysfs_dev);
> > +}
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index a7c0671c6715..30825ec0648e 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -323,6 +323,12 @@ static u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci,
> >  	return 0;
> >  }
> >  
> > +u16 dw_pcie_find_ptm_capability(struct dw_pcie *pci)
> > +{
> > +	return dw_pcie_find_vsec_capability(pci, dwc_pcie_ptm_vsec_ids);
> > +}
> > +EXPORT_SYMBOL_GPL(dw_pcie_find_ptm_capability);
> 
> This API should go into the previous patch. Otherwise it will result in
> unused function warnings.
> 

Yes, but that should be fine. Unused warnings are generally acceptable if the
function is defined in subsequent patch. Only rule is that the build should not
be broken when using defconfig.

Moreover, the previous patch just adds the VSEC helpers and I inherited them
from Shradha's patch. Clubbing PTM API would make it look like two separate
changes in a single patch.

- Mani

-- 
மணிவண்ணன் சதாசிவம்