[PATCH v6 0/4] Add support for debugfs based RAS DES feature in PCIe DW

[PATCH v6 0/4] Add support for debugfs based RAS DES feature in PCIe DW

[Shradha Todi]

DesignWare controller provides a vendor specific extended capability
called RASDES as an IP feature. This extended capability  provides
hardware information like:
 - Debug registers to know the state of the link or controller. 
 - Error injection mechanisms to inject various PCIe errors including
   sequence number, CRC
 - Statistical counters to know how many times a particular event
   occurred

However, in Linux we do not have any generic or custom support to be
able to use this feature in an efficient manner. This is the reason we
are proposing this framework. Debug and bring up time of high-speed IPs
are highly dependent on costlier hardware analyzers and this solution
will in some ways help to reduce the HW analyzer usage.

The debugfs entries can be used to get information about underlying
hardware and can be shared with user space. Separate debugfs entries has
been created to cater to all the DES hooks provided by the controller.
The debugfs entries interacts with the RASDES registers in the required
sequence and provides the meaningful data to the user. This eases the
effort to understand and use the register information for debugging.

This series creates a generic debugfs framework for DesignWare PCIe
controllers where other debug features apart from RASDES can also be
added as and when required.

v6:
    - Addressed Niklas's comment to make vsec ID finding similar to perf
    - Minor changes in the driver to make the debugfs file common and
      not specefic to RASDES so that other developers can add debug
      related features to this file.

v5: https://lore.kernel.org/all/20250121111421.35437-1-shradha.t@samsung.com/
    - Addressed Fan's comment to split the patches for easier review
    - Addressed Bjorn's comment to fix vendor specific cap search
    - Addressed style related change requests from v4
    - Added rasdes debugfs init call to common designware files for host
      and EP.

v4: https://lore.kernel.org/lkml/20241206074456.17401-1-shradha.t@samsung.com/
    - Addressed comments from Manivannan, Bjorn and Jonathan
    - Addressed style related change requests from v3
    - Added Documentation under Documentation/ABI/testing and kdoc stype
      comments wherever required for better understanding
    - Enhanced error injection to include all possible error groups
    - Removed debugfs init call from common designware file and left it
      up to individual platform drivers to init/deinit as required.

v3: https://lore.kernel.org/all/20240625093813.112555-1-shradha.t@samsung.com/
    - v2 had suggestions about moving this framework to perf/EDAC instead of a
      controller specific debugfs but after discussions we decided to go ahead
      with the same. Rebased and posted v3 with minor style changes.

v2: https://lore.kernel.org/lkml/20231130115044.53512-1-shradha.t@samsung.com/
    - Addressed comments from Krzysztof Wilczyński, Bjorn Helgaas and
      posted v2 with a changed implementation for a better code design

v1: https://lore.kernel.org/all/20210518174618.42089-1-shradha.t@samsung.com/T/

Shradha Todi (4):
  PCI: dwc: Add support for vendor specific capability search
  Add debugfs based silicon debug support in DWC
  Add debugfs based error injection support in DWC
  Add debugfs based statistical counter support in DWC

 Documentation/ABI/testing/debugfs-dwc-pcie    | 144 +++++
 drivers/pci/controller/dwc/Kconfig            |  10 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 .../controller/dwc/pcie-designware-debugfs.c  | 595 ++++++++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |   5 +
 .../pci/controller/dwc/pcie-designware-host.c |   6 +
 drivers/pci/controller/dwc/pcie-designware.c  |  19 +
 drivers/pci/controller/dwc/pcie-designware.h  |  21 +
 8 files changed, 801 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-dwc-pcie
 create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c

-- 
2.17.1

[PATCH v6 1/4] PCI: dwc: Add support for vendor specific capability search

[Shradha Todi]

Add vendor specific extended configuration space capability search API
using struct dw_pcie pointer for DW controllers.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 19 +++++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 6d6cbc8b5b2c..3588197ba2d7 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -277,6 +277,25 @@ static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
 	return 0;
 }
 
+u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id, u16 vsec_cap)
+{
+	u16 vsec = 0;
+	u32 header;
+
+	if (vendor_id != dw_pcie_readw_dbi(pci, PCI_VENDOR_ID))
+		return 0;
+
+	while ((vsec = dw_pcie_find_next_ext_capability(pci, vsec,
+					PCI_EXT_CAP_ID_VNDR))) {
+		header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
+		if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
+			return vsec;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
+
 u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
 {
 	return dw_pcie_find_next_ext_capability(pci, 0, cap);
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35a..02e94bd9b042 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -476,6 +476,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
 
 u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
 u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
+u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id, u16 vsec_cap);
 
 int dw_pcie_read(void __iomem *addr, int size, u32 *val);
 int dw_pcie_write(void __iomem *addr, int size, u32 val);
-- 
2.17.1

Re: [PATCH v6 1/4] PCI: dwc: Add support for vendor specific capability search

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 04:20:04PM +0530, Shradha Todi wrote:
> Add vendor specific extended configuration space capability search API
> using struct dw_pcie pointer for DW controllers.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>

I took this patch and modified to pass the 'struct dwc_pcie_vsec_id' to the API
to simplify the callers:
https://lore.kernel.org/linux-pci/20250218-pcie-qcom-ptm-v1-2-16d7e480d73e@linaro.org

- Mani

> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 19 +++++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 6d6cbc8b5b2c..3588197ba2d7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -277,6 +277,25 @@ static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
>  	return 0;
>  }
>  
> +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id, u16 vsec_cap)
> +{
> +	u16 vsec = 0;
> +	u32 header;
> +
> +	if (vendor_id != dw_pcie_readw_dbi(pci, PCI_VENDOR_ID))
> +		return 0;
> +
> +	while ((vsec = dw_pcie_find_next_ext_capability(pci, vsec,
> +					PCI_EXT_CAP_ID_VNDR))) {
> +		header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
> +		if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
> +			return vsec;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
> +
>  u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
>  {
>  	return dw_pcie_find_next_ext_capability(pci, 0, cap);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 347ab74ac35a..02e94bd9b042 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -476,6 +476,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
>  
>  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
>  u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
> +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id, u16 vsec_cap);
>  
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val);
>  int dw_pcie_write(void __iomem *addr, int size, u32 val);
> -- 
> 2.17.1
> 

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

RE: [PATCH v6 1/4] PCI: dwc: Add support for vendor specific capability search

[Shradha Todi]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 18 February 2025 20:11
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@Huawei.com; fan.ni@samsung.com; nifan.cxl@gmail.com;
> a.manzanares@samsung.com; pankaj.dubey@samsung.com; cassel@kernel.org; 18255117159@163.com;
> quic_nitegupt@quicinc.com; quic_krichai@quicinc.com; gost.dev@samsung.com
> Subject: Re: [PATCH v6 1/4] PCI: dwc: Add support for vendor specific capability search
> 
> On Fri, Feb 14, 2025 at 04:20:04PM +0530, Shradha Todi wrote:
> > Add vendor specific extended configuration space capability search API
> > using struct dw_pcie pointer for DW controllers.
> >
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> 
> I took this patch and modified to pass the 'struct dwc_pcie_vsec_id' to the API to simplify the callers:
> https://lore.kernel.org/linux-pci/20250218-pcie-qcom-ptm-v1-2-16d7e480d73e@linaro.org
> 
> - Mani
> 

I saw the series. I'm okay to move that to a common header file to avoid duplication but I feel that this movement will
cause my patch to become dependent on your PTM series. Since that series in still in v1 stage and mine is already in v6,
I feel the debugfs patchset will get further delayed. If you are okay, I could take in the changes as part of v7 after waiting
for some more reviews. Or, the debugfs patches could be reviewed as it is, and the changes or movement could go on top
of that?

> > ---
> >  drivers/pci/controller/dwc/pcie-designware.c | 19 +++++++++++++++++++
> > drivers/pci/controller/dwc/pcie-designware.h |  1 +
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c
> > b/drivers/pci/controller/dwc/pcie-designware.c
> > index 6d6cbc8b5b2c..3588197ba2d7 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -277,6 +277,25 @@ static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
> >  	return 0;
> >  }
> >
> > +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id,
> > +u16 vsec_cap) {
> > +	u16 vsec = 0;
> > +	u32 header;
> > +
> > +	if (vendor_id != dw_pcie_readw_dbi(pci, PCI_VENDOR_ID))
> > +		return 0;
> > +
> > +	while ((vsec = dw_pcie_find_next_ext_capability(pci, vsec,
> > +					PCI_EXT_CAP_ID_VNDR))) {
> > +		header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
> > +		if (PCI_VNDR_HEADER_ID(header) == vsec_cap)
> > +			return vsec;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dw_pcie_find_vsec_capability);
> > +
> >  u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)  {
> >  	return dw_pcie_find_next_ext_capability(pci, 0, cap); diff --git
> > a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 347ab74ac35a..02e94bd9b042 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -476,6 +476,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);
> >
> >  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap);
> >  u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap);
> > +u16 dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id,
> > +u16 vsec_cap);
> >
> >  int dw_pcie_read(void __iomem *addr, int size, u32 *val);  int
> > dw_pcie_write(void __iomem *addr, int size, u32 val);
> > --
> > 2.17.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v6 1/4] PCI: dwc: Add support for vendor specific capability search

['Manivannan Sadhasivam']

On Thu, Feb 20, 2025 at 11:44:36AM +0530, Shradha Todi wrote:
> 
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 18 February 2025 20:11
> > To: Shradha Todi <shradha.t@samsung.com>
> > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> > bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@Huawei.com; fan.ni@samsung.com; nifan.cxl@gmail.com;
> > a.manzanares@samsung.com; pankaj.dubey@samsung.com; cassel@kernel.org; 18255117159@163.com;
> > quic_nitegupt@quicinc.com; quic_krichai@quicinc.com; gost.dev@samsung.com
> > Subject: Re: [PATCH v6 1/4] PCI: dwc: Add support for vendor specific capability search
> > 
> > On Fri, Feb 14, 2025 at 04:20:04PM +0530, Shradha Todi wrote:
> > > Add vendor specific extended configuration space capability search API
> > > using struct dw_pcie pointer for DW controllers.
> > >
> > > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > 
> > I took this patch and modified to pass the 'struct dwc_pcie_vsec_id' to the API to simplify the callers:
> > https://lore.kernel.org/linux-pci/20250218-pcie-qcom-ptm-v1-2-16d7e480d73e@linaro.org
> > 
> > - Mani
> > 
> 
> I saw the series. I'm okay to move that to a common header file to avoid duplication but I feel that this movement will
> cause my patch to become dependent on your PTM series. Since that series in still in v1 stage and mine is already in v6,
> I feel the debugfs patchset will get further delayed. If you are okay, I could take in the changes as part of v7 after waiting
> for some more reviews. Or, the debugfs patches could be reviewed as it is, and the changes or movement could go on top
> of that?
> 

You can take my first 2 patches. I'll post my v2 on top of your next version.

- Mani

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

[PATCH v6 2/4] Add debugfs based silicon debug support in DWC

[Shradha Todi]

Add support to provide silicon debug interface to userspace. This set
of debug registers are part of the RASDES feature present in DesignWare
PCIe controllers.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 Documentation/ABI/testing/debugfs-dwc-pcie    |  13 ++
 drivers/pci/controller/dwc/Kconfig            |  10 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 .../controller/dwc/pcie-designware-debugfs.c  | 207 ++++++++++++++++++
 .../pci/controller/dwc/pcie-designware-ep.c   |   5 +
 .../pci/controller/dwc/pcie-designware-host.c |   6 +
 drivers/pci/controller/dwc/pcie-designware.h  |  20 ++
 7 files changed, 262 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-dwc-pcie
 create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c

diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie b/Documentation/ABI/testing/debugfs-dwc-pcie
new file mode 100644
index 000000000000..e8ed34e988ef
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-dwc-pcie
@@ -0,0 +1,13 @@
+What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/lane_detect
+Date:		Feburary 2025
+Contact:	Shradha Todi <shradha.t@samsung.com>
+Description:	(RW) Write the lane number to be checked for detection.	Read
+		will return whether PHY indicates receiver detection on the
+		selected lane. The default selected lane is Lane0.
+
+What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/rx_valid
+Date:		Feburary 2025
+Contact:	Shradha Todi <shradha.t@samsung.com>
+Description:	(RW) Write the lane number to be checked as valid or invalid. Read
+		will return the status of PIPE RXVALID signal of the selected lane.
+		The default selected lane is Lane0.
diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index b6d6778b0698..48a10428a492 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -6,6 +6,16 @@ menu "DesignWare-based PCIe controllers"
 config PCIE_DW
 	bool
 
+config PCIE_DW_DEBUGFS
+	default y
+	depends on DEBUG_FS
+	depends on PCIE_DW_HOST || PCIE_DW_EP
+	bool "DWC PCIe debugfs entries"
+	help
+	  Enables debugfs entries for the DW PCIe Controller. These entries
+	  provide all debug features related to DW controller including the RAS
+	  DES features to help in debug, error injection and statistical counters.
+
 config PCIE_DW_HOST
 	bool
 	select PCIE_DW
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a8308d9ea986..54565eedc52c 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_PCIE_DW) += pcie-designware.o
+obj-$(CONFIG_PCIE_DW_DEBUGFS) += pcie-designware-debugfs.o
 obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
 obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
new file mode 100644
index 000000000000..fe799d36fa7f
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Synopsys DesignWare PCIe controller debugfs driver
+ *
+ * Copyright (C) 2025 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * Author: Shradha Todi <shradha.t@samsung.com>
+ */
+
+#include <linux/debugfs.h>
+
+#include "pcie-designware.h"
+
+#define SD_STATUS_L1LANE_REG		0xb0
+#define PIPE_RXVALID			BIT(18)
+#define PIPE_DETECT_LANE		BIT(17)
+#define LANE_SELECT			GENMASK(3, 0)
+
+#define DWC_DEBUGFS_BUF_MAX		128
+
+struct dwc_pcie_vsec_id {
+	u16 vendor_id;
+	u16 vsec_id;
+	u8 vsec_rev;
+};
+
+/*
+ * VSEC IDs are allocated by the vendor, so a given ID may mean different
+ * things to different vendors. See PCIe r6.0, sec 7.9.5.2.
+ */
+static const struct dwc_pcie_vsec_id dwc_pcie_vsec_ids[] = {
+	{ .vendor_id = PCI_VENDOR_ID_ALIBABA,
+		.vsec_id = 0x02, .vsec_rev = 0x4 },
+	{ .vendor_id = PCI_VENDOR_ID_AMPERE,
+		.vsec_id = 0x02, .vsec_rev = 0x4 },
+	{ .vendor_id = PCI_VENDOR_ID_QCOM,
+		.vsec_id = 0x02, .vsec_rev = 0x4 },
+	{ .vendor_id = PCI_VENDOR_ID_SAMSUNG,
+		.vsec_id = 0x02, .vsec_rev = 0x4 },
+	{} /* terminator */
+};
+
+/**
+ * struct dwc_pcie_rasdes_info - Stores controller common information
+ * @ras_cap_offset: RAS DES vendor specific extended capability offset
+ * @reg_lock: Mutex used for RASDES shadow event registers
+ *
+ * Any parameter constant to all files of the debugfs hierarchy for a single controller
+ * will be stored in this struct. It is allocated and assigned to controller specific
+ * struct dw_pcie during initialization.
+ */
+struct dwc_pcie_rasdes_info {
+	u32 ras_cap_offset;
+	struct mutex reg_lock;
+};
+
+static ssize_t lane_detect_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct dw_pcie *pci = file->private_data;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
+	ssize_t off = 0;
+	u32 val;
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
+	val = FIELD_GET(PIPE_DETECT_LANE, val);
+	if (val)
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane Detected\n");
+	else
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane Undetected\n");
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t lane_detect_write(struct file *file, const char __user *buf,
+				 size_t count, loff_t *ppos)
+{
+	struct dw_pcie *pci = file->private_data;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	u32 lane, val;
+
+	val = kstrtou32_from_user(buf, count, 0, &lane);
+	if (val)
+		return val;
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
+	val &= ~(LANE_SELECT);
+	val |= FIELD_PREP(LANE_SELECT, lane);
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG, val);
+
+	return count;
+}
+
+static ssize_t rx_valid_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct dw_pcie *pci = file->private_data;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
+	ssize_t off = 0;
+	u32 val;
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
+	val = FIELD_GET(PIPE_RXVALID, val);
+	if (val)
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "RX Valid\n");
+	else
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "RX Invalid\n");
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t rx_valid_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	return lane_detect_write(file, buf, count, ppos);
+}
+
+#define dwc_debugfs_create(name)			\
+debugfs_create_file(#name, 0644, rasdes_debug, pci,	\
+			&dbg_ ## name ## _fops)
+
+#define DWC_DEBUGFS_FOPS(name)					\
+static const struct file_operations dbg_ ## name ## _fops = {	\
+	.open = simple_open,				\
+	.read = name ## _read,				\
+	.write = name ## _write				\
+}
+
+DWC_DEBUGFS_FOPS(lane_detect);
+DWC_DEBUGFS_FOPS(rx_valid);
+
+static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
+{
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+
+	mutex_destroy(&rinfo->reg_lock);
+}
+
+static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
+{
+	struct dentry *rasdes_debug;
+	struct dwc_pcie_rasdes_info *rasdes_info;
+	const struct dwc_pcie_vsec_id *vid;
+	struct device *dev = pci->dev;
+	int ras_cap;
+
+	for (vid = dwc_pcie_vsec_ids; vid->vendor_id; vid++) {
+		ras_cap = dw_pcie_find_vsec_capability(pci, vid->vendor_id,
+							vid->vsec_id);
+		if (ras_cap)
+			break;
+	}
+	if (!ras_cap) {
+		dev_dbg(dev, "no rasdes capability available\n");
+		return -ENODEV;
+	}
+
+	rasdes_info = devm_kzalloc(dev, sizeof(*rasdes_info), GFP_KERNEL);
+	if (!rasdes_info)
+		return -ENOMEM;
+
+	/* Create subdirectories for Debug, Error injection, Statistics */
+	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
+
+	mutex_init(&rasdes_info->reg_lock);
+	rasdes_info->ras_cap_offset = ras_cap;
+	pci->debugfs->rasdes_info = rasdes_info;
+
+	/* Create debugfs files for Debug subdirectory */
+	dwc_debugfs_create(lane_detect);
+	dwc_debugfs_create(rx_valid);
+
+	return 0;
+}
+
+void dwc_pcie_debugfs_deinit(struct dw_pcie *pci)
+{
+	dwc_pcie_rasdes_debugfs_deinit(pci);
+	debugfs_remove_recursive(pci->debugfs->debug_dir);
+}
+
+int dwc_pcie_debugfs_init(struct dw_pcie *pci)
+{
+	char dirname[DWC_DEBUGFS_BUF_MAX];
+	struct device *dev = pci->dev;
+	struct debugfs_info *debugfs;
+	struct dentry *dir;
+	int ret;
+
+	/* Create main directory for each platform driver */
+	snprintf(dirname, DWC_DEBUGFS_BUF_MAX, "dwc_pcie_%s", dev_name(dev));
+	dir = debugfs_create_dir(dirname, NULL);
+	if (IS_ERR(dir))
+		return PTR_ERR(dir);
+
+	debugfs = devm_kzalloc(dev, sizeof(*debugfs), GFP_KERNEL);
+	if (!debugfs)
+		return -ENOMEM;
+
+	debugfs->debug_dir = dir;
+	pci->debugfs = debugfs;
+	ret = dwc_pcie_rasdes_debugfs_init(pci, dir);
+	if (ret)
+		dev_dbg(dev, "rasdes debugfs init failed\n");
+
+	return 0;
+}
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index f3ac7d46a855..a87a714bb472 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -642,6 +642,7 @@ void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
+	dwc_pcie_debugfs_deinit(pci);
 	dw_pcie_edma_remove(pci);
 }
 EXPORT_SYMBOL_GPL(dw_pcie_ep_cleanup);
@@ -813,6 +814,10 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
 
 	dw_pcie_ep_init_non_sticky_registers(pci);
 
+	ret = dwc_pcie_debugfs_init(pci);
+	if (ret)
+		goto err_remove_edma;
+
 	return 0;
 
 err_remove_edma:
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d2291c3ceb8b..6b03ef7fd014 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -524,6 +524,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	if (ret)
 		goto err_remove_edma;
 
+	ret = dwc_pcie_debugfs_init(pci);
+	if (ret)
+		goto err_remove_edma;
+
 	if (!dw_pcie_link_up(pci)) {
 		ret = dw_pcie_start_link(pci);
 		if (ret)
@@ -571,6 +575,8 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
 
 	dw_pcie_stop_link(pci);
 
+	dwc_pcie_debugfs_deinit(pci);
+
 	dw_pcie_edma_remove(pci);
 
 	if (pp->has_msi_ctrl)
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 02e94bd9b042..8543b4f38d24 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -435,6 +435,11 @@ struct dw_pcie_ops {
 	void	(*stop_link)(struct dw_pcie *pcie);
 };
 
+struct debugfs_info {
+	struct dentry		*debug_dir;
+	void			*rasdes_info;
+};
+
 struct dw_pcie {
 	struct device		*dev;
 	void __iomem		*dbi_base;
@@ -463,6 +468,7 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+	struct debugfs_info	*debugfs;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
@@ -796,4 +802,18 @@ dw_pcie_ep_get_func_from_ep(struct dw_pcie_ep *ep, u8 func_no)
 	return NULL;
 }
 #endif
+
+#ifdef CONFIG_PCIE_DW_DEBUGFS
+int dwc_pcie_debugfs_init(struct dw_pcie *pci);
+void dwc_pcie_debugfs_deinit(struct dw_pcie *pci);
+#else
+static inline int dwc_pcie_debugfs_init(struct dw_pcie *pci)
+{
+	return 0;
+}
+static inline void dwc_pcie_debugfs_deinit(struct dw_pcie *pci)
+{
+}
+#endif
+
 #endif /* _PCIE_DESIGNWARE_H */
-- 
2.17.1

Re: [PATCH v6 2/4] Add debugfs based silicon debug support in DWC

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 04:20:05PM +0530, Shradha Todi wrote:
> Add support to provide silicon debug interface to userspace. This set
> of debug registers are part of the RASDES feature present in DesignWare
> PCIe controllers.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  Documentation/ABI/testing/debugfs-dwc-pcie    |  13 ++
>  drivers/pci/controller/dwc/Kconfig            |  10 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  .../controller/dwc/pcie-designware-debugfs.c  | 207 ++++++++++++++++++
>  .../pci/controller/dwc/pcie-designware-ep.c   |   5 +
>  .../pci/controller/dwc/pcie-designware-host.c |   6 +
>  drivers/pci/controller/dwc/pcie-designware.h  |  20 ++
>  7 files changed, 262 insertions(+)
>  create mode 100644 Documentation/ABI/testing/debugfs-dwc-pcie
>  create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
> 
> diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie b/Documentation/ABI/testing/debugfs-dwc-pcie
> new file mode 100644
> index 000000000000..e8ed34e988ef
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-dwc-pcie
> @@ -0,0 +1,13 @@
> +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/lane_detect
> +Date:		Feburary 2025

Please align these fields

> +Contact:	Shradha Todi <shradha.t@samsung.com>
> +Description:	(RW) Write the lane number to be checked for detection.	Read
> +		will return whether PHY indicates receiver detection on the
> +		selected lane. The default selected lane is Lane0.
> +
> +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/rx_valid
> +Date:		Feburary 2025
> +Contact:	Shradha Todi <shradha.t@samsung.com>
> +Description:	(RW) Write the lane number to be checked as valid or invalid. Read
> +		will return the status of PIPE RXVALID signal of the selected lane.
> +		The default selected lane is Lane0.
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index b6d6778b0698..48a10428a492 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -6,6 +6,16 @@ menu "DesignWare-based PCIe controllers"
>  config PCIE_DW
>  	bool
>  
> +config PCIE_DW_DEBUGFS
> +	default y
> +	depends on DEBUG_FS
> +	depends on PCIE_DW_HOST || PCIE_DW_EP
> +	bool "DWC PCIe debugfs entries"
> +	help
> +	  Enables debugfs entries for the DW PCIe Controller. These entries
> +	  provide all debug features related to DW controller including the RAS
> +	  DES features to help in debug, error injection and statistical counters.
> +
>  config PCIE_DW_HOST
>  	bool
>  	select PCIE_DW
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index a8308d9ea986..54565eedc52c 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_PCIE_DW) += pcie-designware.o
> +obj-$(CONFIG_PCIE_DW_DEBUGFS) += pcie-designware-debugfs.o
>  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
>  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> new file mode 100644
> index 000000000000..fe799d36fa7f
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2025 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@samsung.com>
> + */
> +
> +#include <linux/debugfs.h>
> +
> +#include "pcie-designware.h"
> +
> +#define SD_STATUS_L1LANE_REG		0xb0
> +#define PIPE_RXVALID			BIT(18)
> +#define PIPE_DETECT_LANE		BIT(17)
> +#define LANE_SELECT			GENMASK(3, 0)
> +
> +#define DWC_DEBUGFS_BUF_MAX		128
> +
> +struct dwc_pcie_vsec_id {
> +	u16 vendor_id;
> +	u16 vsec_id;
> +	u8 vsec_rev;
> +};
> +
> +/*
> + * VSEC IDs are allocated by the vendor, so a given ID may mean different
> + * things to different vendors. See PCIe r6.0, sec 7.9.5.2.
> + */
> +static const struct dwc_pcie_vsec_id dwc_pcie_vsec_ids[] = {
> +	{ .vendor_id = PCI_VENDOR_ID_ALIBABA,
> +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> +	{ .vendor_id = PCI_VENDOR_ID_AMPERE,
> +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> +	{ .vendor_id = PCI_VENDOR_ID_QCOM,
> +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> +	{ .vendor_id = PCI_VENDOR_ID_SAMSUNG,
> +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> +	{} /* terminator */

This should go into the common include file as I proposed in my series:
https://lore.kernel.org/linux-pci/20250218-pcie-qcom-ptm-v1-1-16d7e480d73e@linaro.org/

> +};
> +
> +/**
> + * struct dwc_pcie_rasdes_info - Stores controller common information
> + * @ras_cap_offset: RAS DES vendor specific extended capability offset
> + * @reg_lock: Mutex used for RASDES shadow event registers

If this is not used by all register accesses, please rename it as such. Like,
reg_event_lock.

> + *
> + * Any parameter constant to all files of the debugfs hierarchy for a single controller
> + * will be stored in this struct. It is allocated and assigned to controller specific
> + * struct dw_pcie during initialization.
> + */
> +struct dwc_pcie_rasdes_info {
> +	u32 ras_cap_offset;
> +	struct mutex reg_lock;
> +};
> +
> +static ssize_t lane_detect_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct dw_pcie *pci = file->private_data;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
> +	ssize_t off = 0;

Not required. See below.

> +	u32 val;
> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
> +	val = FIELD_GET(PIPE_DETECT_LANE, val);
> +	if (val)
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane Detected\n");

I don't understand why you want to add 'off' which was initialized to 0. Also
this just prints single string.

> +	else
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane Undetected\n");
> +
> +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> +}
> +
> +static ssize_t lane_detect_write(struct file *file, const char __user *buf,
> +				 size_t count, loff_t *ppos)
> +{
> +	struct dw_pcie *pci = file->private_data;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	u32 lane, val;
> +
> +	val = kstrtou32_from_user(buf, count, 0, &lane);
> +	if (val)
> +		return val;
> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
> +	val &= ~(LANE_SELECT);
> +	val |= FIELD_PREP(LANE_SELECT, lane);
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG, val);
> +
> +	return count;
> +}
> +
> +static ssize_t rx_valid_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct dw_pcie *pci = file->private_data;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
> +	ssize_t off = 0;
> +	u32 val;
> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
> +	val = FIELD_GET(PIPE_RXVALID, val);
> +	if (val)
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "RX Valid\n");

Same here.

> +	else
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "RX Invalid\n");
> +
> +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> +}
> +
> +static ssize_t rx_valid_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> +{
> +	return lane_detect_write(file, buf, count, ppos);
> +}
> +
> +#define dwc_debugfs_create(name)			\
> +debugfs_create_file(#name, 0644, rasdes_debug, pci,	\
> +			&dbg_ ## name ## _fops)
> +
> +#define DWC_DEBUGFS_FOPS(name)					\
> +static const struct file_operations dbg_ ## name ## _fops = {	\
> +	.open = simple_open,				\
> +	.read = name ## _read,				\
> +	.write = name ## _write				\
> +}
> +
> +DWC_DEBUGFS_FOPS(lane_detect);
> +DWC_DEBUGFS_FOPS(rx_valid);
> +
> +static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
> +{
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +
> +	mutex_destroy(&rinfo->reg_lock);
> +}
> +
> +static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
> +{
> +	struct dentry *rasdes_debug;
> +	struct dwc_pcie_rasdes_info *rasdes_info;
> +	const struct dwc_pcie_vsec_id *vid;
> +	struct device *dev = pci->dev;
> +	int ras_cap;
> +
> +	for (vid = dwc_pcie_vsec_ids; vid->vendor_id; vid++) {
> +		ras_cap = dw_pcie_find_vsec_capability(pci, vid->vendor_id,
> +							vid->vsec_id);
> +		if (ras_cap)
> +			break;
> +	}
> +	if (!ras_cap) {
> +		dev_dbg(dev, "no rasdes capability available\n");
> +		return -ENODEV;
> +	}

This will also go inside a new API, dw_pcie_find_rasdes_capability(pci).

> +
> +	rasdes_info = devm_kzalloc(dev, sizeof(*rasdes_info), GFP_KERNEL);
> +	if (!rasdes_info)
> +		return -ENOMEM;
> +
> +	/* Create subdirectories for Debug, Error injection, Statistics */
> +	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);

_debug prefix is not needed since the directory itself belongs to debugfs.

> +
> +	mutex_init(&rasdes_info->reg_lock);
> +	rasdes_info->ras_cap_offset = ras_cap;
> +	pci->debugfs->rasdes_info = rasdes_info;
> +
> +	/* Create debugfs files for Debug subdirectory */
> +	dwc_debugfs_create(lane_detect);
> +	dwc_debugfs_create(rx_valid);
> +
> +	return 0;
> +}
> +
> +void dwc_pcie_debugfs_deinit(struct dw_pcie *pci)
> +{
> +	dwc_pcie_rasdes_debugfs_deinit(pci);
> +	debugfs_remove_recursive(pci->debugfs->debug_dir);
> +}
> +
> +int dwc_pcie_debugfs_init(struct dw_pcie *pci)
> +{
> +	char dirname[DWC_DEBUGFS_BUF_MAX];
> +	struct device *dev = pci->dev;
> +	struct debugfs_info *debugfs;
> +	struct dentry *dir;
> +	int ret;
> +
> +	/* Create main directory for each platform driver */
> +	snprintf(dirname, DWC_DEBUGFS_BUF_MAX, "dwc_pcie_%s", dev_name(dev));
> +	dir = debugfs_create_dir(dirname, NULL);
> +	if (IS_ERR(dir))
> +		return PTR_ERR(dir);

debugfs creation is not supposed to fail. So you should remove the error check.

> +
> +	debugfs = devm_kzalloc(dev, sizeof(*debugfs), GFP_KERNEL);
> +	if (!debugfs)
> +		return -ENOMEM;
> +
> +	debugfs->debug_dir = dir;
> +	pci->debugfs = debugfs;
> +	ret = dwc_pcie_rasdes_debugfs_init(pci, dir);
> +	if (ret)
> +		dev_dbg(dev, "rasdes debugfs init failed\n");

RASDES

> +
> +	return 0;
> +}
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index f3ac7d46a855..a87a714bb472 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -642,6 +642,7 @@ void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  
> +	dwc_pcie_debugfs_deinit(pci);
>  	dw_pcie_edma_remove(pci);
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_ep_cleanup);
> @@ -813,6 +814,10 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep *ep)
>  
>  	dw_pcie_ep_init_non_sticky_registers(pci);
>  
> +	ret = dwc_pcie_debugfs_init(pci);
> +	if (ret)
> +		goto err_remove_edma;
> +
>  	return 0;
>  
>  err_remove_edma:
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d2291c3ceb8b..6b03ef7fd014 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -524,6 +524,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret)
>  		goto err_remove_edma;
>  
> +	ret = dwc_pcie_debugfs_init(pci);
> +	if (ret)
> +		goto err_remove_edma;
> +

Why can't you move it to the end of the function?

>  	if (!dw_pcie_link_up(pci)) {
>  		ret = dw_pcie_start_link(pci);
>  		if (ret)
> @@ -571,6 +575,8 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
>  
>  	dw_pcie_stop_link(pci);
>  
> +	dwc_pcie_debugfs_deinit(pci);
> +

This should be moved to the start of the function.

- Mani

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

RE: [PATCH v6 2/4] Add debugfs based silicon debug support in DWC

[Shradha Todi]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 18 February 2025 20:33
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@Huawei.com; fan.ni@samsung.com; nifan.cxl@gmail.com;
> a.manzanares@samsung.com; pankaj.dubey@samsung.com; cassel@kernel.org; 18255117159@163.com;
> quic_nitegupt@quicinc.com; quic_krichai@quicinc.com; gost.dev@samsung.com
> Subject: Re: [PATCH v6 2/4] Add debugfs based silicon debug support in DWC
> 
> On Fri, Feb 14, 2025 at 04:20:05PM +0530, Shradha Todi wrote:
> > Add support to provide silicon debug interface to userspace. This set
> > of debug registers are part of the RASDES feature present in
> > DesignWare PCIe controllers.
> >
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > ---
> >  Documentation/ABI/testing/debugfs-dwc-pcie    |  13 ++
> >  drivers/pci/controller/dwc/Kconfig            |  10 +
> >  drivers/pci/controller/dwc/Makefile           |   1 +
> >  .../controller/dwc/pcie-designware-debugfs.c  | 207 ++++++++++++++++++
> >  .../pci/controller/dwc/pcie-designware-ep.c   |   5 +
> >  .../pci/controller/dwc/pcie-designware-host.c |   6 +
> >  drivers/pci/controller/dwc/pcie-designware.h  |  20 ++
> >  7 files changed, 262 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/debugfs-dwc-pcie
> >  create mode 100644
> > drivers/pci/controller/dwc/pcie-designware-debugfs.c
> >
> > diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie
> > b/Documentation/ABI/testing/debugfs-dwc-pcie
> > new file mode 100644
> > index 000000000000..e8ed34e988ef
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/debugfs-dwc-pcie
> > @@ -0,0 +1,13 @@
> > +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/lane_detect
> > +Date:		Feburary 2025
> 
> Please align these fields

The fields are already aligned in my patch (when I check in git), not sure why the mail misaligns it. Can you suggest how
should I fix this?

> 
> > +Contact:	Shradha Todi <shradha.t@samsung.com>
> > +Description:	(RW) Write the lane number to be checked for detection.	Read
> > +		will return whether PHY indicates receiver detection on the
> > +		selected lane. The default selected lane is Lane0.
> > +
> > +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/rx_valid
> > +Date:		Feburary 2025
> > +Contact:	Shradha Todi <shradha.t@samsung.com>
> > +Description:	(RW) Write the lane number to be checked as valid or invalid. Read
> > +		will return the status of PIPE RXVALID signal of the selected lane.
> > +		The default selected lane is Lane0.
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index b6d6778b0698..48a10428a492 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -6,6 +6,16 @@ menu "DesignWare-based PCIe controllers"
> >  config PCIE_DW
> >  	bool
> >
> > +config PCIE_DW_DEBUGFS
> > +	default y
> > +	depends on DEBUG_FS
> > +	depends on PCIE_DW_HOST || PCIE_DW_EP
> > +	bool "DWC PCIe debugfs entries"
> > +	help
> > +	  Enables debugfs entries for the DW PCIe Controller. These entries
> > +	  provide all debug features related to DW controller including the RAS
> > +	  DES features to help in debug, error injection and statistical counters.
> > +
> >  config PCIE_DW_HOST
> >  	bool
> >  	select PCIE_DW
> > diff --git a/drivers/pci/controller/dwc/Makefile
> > b/drivers/pci/controller/dwc/Makefile
> > index a8308d9ea986..54565eedc52c 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -1,5 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_PCIE_DW) += pcie-designware.o
> > +obj-$(CONFIG_PCIE_DW_DEBUGFS) += pcie-designware-debugfs.o
> >  obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
> >  obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
> >  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o diff --git
> > a/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> > b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> > new file mode 100644
> > index 000000000000..fe799d36fa7f
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Synopsys DesignWare PCIe controller debugfs driver
> > + *
> > + * Copyright (C) 2025 Samsung Electronics Co., Ltd.
> > + *		http://www.samsung.com
> > + *
> > + * Author: Shradha Todi <shradha.t@samsung.com>  */
> > +
> > +#include <linux/debugfs.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +#define SD_STATUS_L1LANE_REG		0xb0
> > +#define PIPE_RXVALID			BIT(18)
> > +#define PIPE_DETECT_LANE		BIT(17)
> > +#define LANE_SELECT			GENMASK(3, 0)
> > +
> > +#define DWC_DEBUGFS_BUF_MAX		128
> > +
> > +struct dwc_pcie_vsec_id {
> > +	u16 vendor_id;
> > +	u16 vsec_id;
> > +	u8 vsec_rev;
> > +};
> > +
> > +/*
> > + * VSEC IDs are allocated by the vendor, so a given ID may mean
> > +different
> > + * things to different vendors. See PCIe r6.0, sec 7.9.5.2.
> > + */
> > +static const struct dwc_pcie_vsec_id dwc_pcie_vsec_ids[] = {
> > +	{ .vendor_id = PCI_VENDOR_ID_ALIBABA,
> > +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> > +	{ .vendor_id = PCI_VENDOR_ID_AMPERE,
> > +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> > +	{ .vendor_id = PCI_VENDOR_ID_QCOM,
> > +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> > +	{ .vendor_id = PCI_VENDOR_ID_SAMSUNG,
> > +		.vsec_id = 0x02, .vsec_rev = 0x4 },
> > +	{} /* terminator */
> 
> This should go into the common include file as I proposed in my series:
> https://lore.kernel.org/linux-pci/20250218-pcie-qcom-ptm-v1-1-16d7e480d73e@linaro.org/
> 
> > +};
> > +
> > +/**
> > + * struct dwc_pcie_rasdes_info - Stores controller common information
> > + * @ras_cap_offset: RAS DES vendor specific extended capability
> > +offset
> > + * @reg_lock: Mutex used for RASDES shadow event registers
> 
> If this is not used by all register accesses, please rename it as such. Like, reg_event_lock.
>

Will fix in next version
 
> > + *
> > + * Any parameter constant to all files of the debugfs hierarchy for a
> > +single controller
> > + * will be stored in this struct. It is allocated and assigned to
> > +controller specific
> > + * struct dw_pcie during initialization.
> > + */
> > +struct dwc_pcie_rasdes_info {
> > +	u32 ras_cap_offset;
> > +	struct mutex reg_lock;
> > +};
> > +
> > +static ssize_t lane_detect_read(struct file *file, char __user *buf,
> > +size_t count, loff_t *ppos) {
> > +	struct dw_pcie *pci = file->private_data;
> > +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> > +	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
> > +	ssize_t off = 0;
> 
> Not required. See below.
> 
> > +	u32 val;
> > +
> > +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
> > +	val = FIELD_GET(PIPE_DETECT_LANE, val);
> > +	if (val)
> > +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane
> > +Detected\n");
> 
> I don't understand why you want to add 'off' which was initialized to 0. Also this just prints single string.
> 

Okay will change it everywhere.

> > +	else
> > +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane
> > +Undetected\n");
> > +
> > +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> > +}
> > +
> > +static ssize_t lane_detect_write(struct file *file, const char __user *buf,
> > +				 size_t count, loff_t *ppos)
> > +{
> > +	struct dw_pcie *pci = file->private_data;
> > +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> > +	u32 lane, val;
> > +
> > +	val = kstrtou32_from_user(buf, count, 0, &lane);
> > +	if (val)
> > +		return val;
> > +
> > +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
> > +	val &= ~(LANE_SELECT);
> > +	val |= FIELD_PREP(LANE_SELECT, lane);
> > +	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset +
> > +SD_STATUS_L1LANE_REG, val);
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t rx_valid_read(struct file *file, char __user *buf,
> > +size_t count, loff_t *ppos) {
> > +	struct dw_pcie *pci = file->private_data;
> > +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> > +	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
> > +	ssize_t off = 0;
> > +	u32 val;
> > +
> > +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + SD_STATUS_L1LANE_REG);
> > +	val = FIELD_GET(PIPE_RXVALID, val);
> > +	if (val)
> > +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "RX
> > +Valid\n");
> 
> Same here.
> 
> > +	else
> > +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "RX
> > +Invalid\n");
> > +
> > +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> > +}
> > +
> > +static ssize_t rx_valid_write(struct file *file, const char __user
> > +*buf, size_t count, loff_t *ppos) {
> > +	return lane_detect_write(file, buf, count, ppos); }
> > +
> > +#define dwc_debugfs_create(name)			\
> > +debugfs_create_file(#name, 0644, rasdes_debug, pci,	\
> > +			&dbg_ ## name ## _fops)
> > +
> > +#define DWC_DEBUGFS_FOPS(name)					\
> > +static const struct file_operations dbg_ ## name ## _fops = {	\
> > +	.open = simple_open,				\
> > +	.read = name ## _read,				\
> > +	.write = name ## _write				\
> > +}
> > +
> > +DWC_DEBUGFS_FOPS(lane_detect);
> > +DWC_DEBUGFS_FOPS(rx_valid);
> > +
> > +static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci) {
> > +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> > +
> > +	mutex_destroy(&rinfo->reg_lock);
> > +}
> > +
> > +static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct
> > +dentry *dir) {
> > +	struct dentry *rasdes_debug;
> > +	struct dwc_pcie_rasdes_info *rasdes_info;
> > +	const struct dwc_pcie_vsec_id *vid;
> > +	struct device *dev = pci->dev;
> > +	int ras_cap;
> > +
> > +	for (vid = dwc_pcie_vsec_ids; vid->vendor_id; vid++) {
> > +		ras_cap = dw_pcie_find_vsec_capability(pci, vid->vendor_id,
> > +							vid->vsec_id);
> > +		if (ras_cap)
> > +			break;
> > +	}
> > +	if (!ras_cap) {
> > +		dev_dbg(dev, "no rasdes capability available\n");
> > +		return -ENODEV;
> > +	}
> 
> This will also go inside a new API, dw_pcie_find_rasdes_capability(pci).
> 

Okay, are we planning to make a function for each VSEC? Or should we just pass the rasdes_vids to the
dw_pcie_find_vsec_capability?

> > +
> > +	rasdes_info = devm_kzalloc(dev, sizeof(*rasdes_info), GFP_KERNEL);
> > +	if (!rasdes_info)
> > +		return -ENOMEM;
> > +
> > +	/* Create subdirectories for Debug, Error injection, Statistics */
> > +	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
> 
> _debug prefix is not needed since the directory itself belongs to debugfs.
> 

It's not for the debug in debugfs. So the DES features are
Debug
Error Injection
Statistics.
The debug here is for the "D" in DES.

> > +
> > +	mutex_init(&rasdes_info->reg_lock);
> > +	rasdes_info->ras_cap_offset = ras_cap;
> > +	pci->debugfs->rasdes_info = rasdes_info;
> > +
> > +	/* Create debugfs files for Debug subdirectory */
> > +	dwc_debugfs_create(lane_detect);
> > +	dwc_debugfs_create(rx_valid);
> > +
> > +	return 0;
> > +}
> > +
> > +void dwc_pcie_debugfs_deinit(struct dw_pcie *pci) {
> > +	dwc_pcie_rasdes_debugfs_deinit(pci);
> > +	debugfs_remove_recursive(pci->debugfs->debug_dir);
> > +}
> > +
> > +int dwc_pcie_debugfs_init(struct dw_pcie *pci) {
> > +	char dirname[DWC_DEBUGFS_BUF_MAX];
> > +	struct device *dev = pci->dev;
> > +	struct debugfs_info *debugfs;
> > +	struct dentry *dir;
> > +	int ret;
> > +
> > +	/* Create main directory for each platform driver */
> > +	snprintf(dirname, DWC_DEBUGFS_BUF_MAX, "dwc_pcie_%s", dev_name(dev));
> > +	dir = debugfs_create_dir(dirname, NULL);
> > +	if (IS_ERR(dir))
> > +		return PTR_ERR(dir);
> 
> debugfs creation is not supposed to fail. So you should remove the error check.
> 

There was no error check until v3. Got a comment from Jonathan in v3:
	"Check for errors in all these."
I think he wanted to add in all the debugfs creations but I just added in the topmost directory.
I checked that error will be returned in case someone turns off debugfs mounting as early param.
So, if the first directory gets made, there would be no issues in subsequent subdirectories.

> > +
> > +	debugfs = devm_kzalloc(dev, sizeof(*debugfs), GFP_KERNEL);
> > +	if (!debugfs)
> > +		return -ENOMEM;
> > +
> > +	debugfs->debug_dir = dir;
> > +	pci->debugfs = debugfs;
> > +	ret = dwc_pcie_rasdes_debugfs_init(pci, dir);
> > +	if (ret)
> > +		dev_dbg(dev, "rasdes debugfs init failed\n");
> 
> RASDES
> 
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f3ac7d46a855..a87a714bb472 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -642,6 +642,7 @@ void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)  {
> >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >
> > +	dwc_pcie_debugfs_deinit(pci);
> >  	dw_pcie_edma_remove(pci);
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_ep_cleanup);
> > @@ -813,6 +814,10 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep
> > *ep)
> >
> >  	dw_pcie_ep_init_non_sticky_registers(pci);
> >
> > +	ret = dwc_pcie_debugfs_init(pci);
> > +	if (ret)
> > +		goto err_remove_edma;
> > +
> >  	return 0;
> >
> >  err_remove_edma:
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index d2291c3ceb8b..6b03ef7fd014 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -524,6 +524,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  	if (ret)
> >  		goto err_remove_edma;
> >
> > +	ret = dwc_pcie_debugfs_init(pci);
> > +	if (ret)
> > +		goto err_remove_edma;
> > +
> 
> Why can't you move it to the end of the function?

So the debugfs entries record certain debug values that might be useful to read
in case link does not come up. Therefore, I'm adding it before starting link up so that users can 
read it in case some failure occurs in further steps.

> 
> >  	if (!dw_pcie_link_up(pci)) {
> >  		ret = dw_pcie_start_link(pci);
> >  		if (ret)
> > @@ -571,6 +575,8 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
> >
> >  	dw_pcie_stop_link(pci);
> >
> > +	dwc_pcie_debugfs_deinit(pci);
> > +
> 
> This should be moved to the start of the function.
> 
> - Mani
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v6 2/4] Add debugfs based silicon debug support in DWC

['Manivannan Sadhasivam']

On Thu, Feb 20, 2025 at 02:48:12PM +0530, Shradha Todi wrote:
> 
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 18 February 2025 20:33
> > To: Shradha Todi <shradha.t@samsung.com>
> > Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> > bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@Huawei.com; fan.ni@samsung.com; nifan.cxl@gmail.com;
> > a.manzanares@samsung.com; pankaj.dubey@samsung.com; cassel@kernel.org; 18255117159@163.com;
> > quic_nitegupt@quicinc.com; quic_krichai@quicinc.com; gost.dev@samsung.com
> > Subject: Re: [PATCH v6 2/4] Add debugfs based silicon debug support in DWC
> > 
> > On Fri, Feb 14, 2025 at 04:20:05PM +0530, Shradha Todi wrote:
> > > Add support to provide silicon debug interface to userspace. This set
> > > of debug registers are part of the RASDES feature present in
> > > DesignWare PCIe controllers.
> > >
> > > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > > ---
> > >  Documentation/ABI/testing/debugfs-dwc-pcie    |  13 ++
> > >  drivers/pci/controller/dwc/Kconfig            |  10 +
> > >  drivers/pci/controller/dwc/Makefile           |   1 +
> > >  .../controller/dwc/pcie-designware-debugfs.c  | 207 ++++++++++++++++++
> > >  .../pci/controller/dwc/pcie-designware-ep.c   |   5 +
> > >  .../pci/controller/dwc/pcie-designware-host.c |   6 +
> > >  drivers/pci/controller/dwc/pcie-designware.h  |  20 ++
> > >  7 files changed, 262 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/debugfs-dwc-pcie
> > >  create mode 100644
> > > drivers/pci/controller/dwc/pcie-designware-debugfs.c
> > >
> > > diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie
> > > b/Documentation/ABI/testing/debugfs-dwc-pcie
> > > new file mode 100644
> > > index 000000000000..e8ed34e988ef
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/debugfs-dwc-pcie
> > > @@ -0,0 +1,13 @@
> > > +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/lane_detect
> > > +Date:		Feburary 2025
> > 
> > Please align these fields
> 
> The fields are already aligned in my patch (when I check in git), not sure why the mail misaligns it. Can you suggest how
> should I fix this?
> 

You should consistently use tabs or space after colon throughout the file. Don't
mix them.

> > 
> > > +Contact:	Shradha Todi <shradha.t@samsung.com>
> > > +Description:	(RW) Write the lane number to be checked for detection.	Read
> > > +		will return whether PHY indicates receiver detection on the
> > > +		selected lane. The default selected lane is Lane0.
> > > +
> > > +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_debug/rx_valid
> > > +Date:		Feburary 2025
> > > +Contact:	Shradha Todi <shradha.t@samsung.com>
> > > +Description:	(RW) Write the lane number to be checked as valid or invalid. Read
> > > +		will return the status of PIPE RXVALID signal of the selected lane.
> > > +		The default selected lane is Lane0.

[...]

> > > +static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct
> > > +dentry *dir) {
> > > +	struct dentry *rasdes_debug;
> > > +	struct dwc_pcie_rasdes_info *rasdes_info;
> > > +	const struct dwc_pcie_vsec_id *vid;
> > > +	struct device *dev = pci->dev;
> > > +	int ras_cap;
> > > +
> > > +	for (vid = dwc_pcie_vsec_ids; vid->vendor_id; vid++) {
> > > +		ras_cap = dw_pcie_find_vsec_capability(pci, vid->vendor_id,
> > > +							vid->vsec_id);
> > > +		if (ras_cap)
> > > +			break;
> > > +	}
> > > +	if (!ras_cap) {
> > > +		dev_dbg(dev, "no rasdes capability available\n");
> > > +		return -ENODEV;
> > > +	}
> > 
> > This will also go inside a new API, dw_pcie_find_rasdes_capability(pci).
> > 
> 
> Okay, are we planning to make a function for each VSEC? Or should we just pass the rasdes_vids to the
> dw_pcie_find_vsec_capability?
> 

dw_pcie_find_vsec_capability() is a static function in my series. We should
add separate function for each VSEC.

> > > +
> > > +	rasdes_info = devm_kzalloc(dev, sizeof(*rasdes_info), GFP_KERNEL);
> > > +	if (!rasdes_info)
> > > +		return -ENOMEM;
> > > +
> > > +	/* Create subdirectories for Debug, Error injection, Statistics */
> > > +	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
> > 
> > _debug prefix is not needed since the directory itself belongs to debugfs.
> > 
> 
> It's not for the debug in debugfs. So the DES features are
> Debug
> Error Injection
> Statistics.
> The debug here is for the "D" in DES.
> 

Sorry, I missed it.

> > > +
> > > +	mutex_init(&rasdes_info->reg_lock);
> > > +	rasdes_info->ras_cap_offset = ras_cap;
> > > +	pci->debugfs->rasdes_info = rasdes_info;
> > > +
> > > +	/* Create debugfs files for Debug subdirectory */
> > > +	dwc_debugfs_create(lane_detect);
> > > +	dwc_debugfs_create(rx_valid);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +void dwc_pcie_debugfs_deinit(struct dw_pcie *pci) {
> > > +	dwc_pcie_rasdes_debugfs_deinit(pci);
> > > +	debugfs_remove_recursive(pci->debugfs->debug_dir);
> > > +}
> > > +
> > > +int dwc_pcie_debugfs_init(struct dw_pcie *pci) {
> > > +	char dirname[DWC_DEBUGFS_BUF_MAX];
> > > +	struct device *dev = pci->dev;
> > > +	struct debugfs_info *debugfs;
> > > +	struct dentry *dir;
> > > +	int ret;
> > > +
> > > +	/* Create main directory for each platform driver */
> > > +	snprintf(dirname, DWC_DEBUGFS_BUF_MAX, "dwc_pcie_%s", dev_name(dev));
> > > +	dir = debugfs_create_dir(dirname, NULL);
> > > +	if (IS_ERR(dir))
> > > +		return PTR_ERR(dir);
> > 
> > debugfs creation is not supposed to fail. So you should remove the error check.
> > 
> 
> There was no error check until v3. Got a comment from Jonathan in v3:
> 	"Check for errors in all these."
> I think he wanted to add in all the debugfs creations but I just added in the topmost directory.
> I checked that error will be returned in case someone turns off debugfs mounting as early param.
> So, if the first directory gets made, there would be no issues in subsequent subdirectories.
> 

Please see the Kdoc comments for this API:

 * NOTE: it's expected that most callers should _ignore_ the errors returned
 * by this function. Other debugfs functions handle the fact that the "dentry"
 * passed to them could be an error and they don't crash in that case.
 * Drivers should generally work fine even if debugfs fails to init anyway.

> > > +
> > > +	debugfs = devm_kzalloc(dev, sizeof(*debugfs), GFP_KERNEL);
> > > +	if (!debugfs)
> > > +		return -ENOMEM;
> > > +
> > > +	debugfs->debug_dir = dir;
> > > +	pci->debugfs = debugfs;
> > > +	ret = dwc_pcie_rasdes_debugfs_init(pci, dir);
> > > +	if (ret)
> > > +		dev_dbg(dev, "rasdes debugfs init failed\n");
> > 
> > RASDES
> > 
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > index f3ac7d46a855..a87a714bb472 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > > @@ -642,6 +642,7 @@ void dw_pcie_ep_cleanup(struct dw_pcie_ep *ep)  {
> > >  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > >
> > > +	dwc_pcie_debugfs_deinit(pci);
> > >  	dw_pcie_edma_remove(pci);
> > >  }
> > >  EXPORT_SYMBOL_GPL(dw_pcie_ep_cleanup);
> > > @@ -813,6 +814,10 @@ int dw_pcie_ep_init_registers(struct dw_pcie_ep
> > > *ep)
> > >
> > >  	dw_pcie_ep_init_non_sticky_registers(pci);
> > >
> > > +	ret = dwc_pcie_debugfs_init(pci);
> > > +	if (ret)
> > > +		goto err_remove_edma;
> > > +
> > >  	return 0;
> > >
> > >  err_remove_edma:
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index d2291c3ceb8b..6b03ef7fd014 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -524,6 +524,10 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  	if (ret)
> > >  		goto err_remove_edma;
> > >
> > > +	ret = dwc_pcie_debugfs_init(pci);
> > > +	if (ret)
> > > +		goto err_remove_edma;
> > > +
> > 
> > Why can't you move it to the end of the function?
> 
> So the debugfs entries record certain debug values that might be useful to read
> in case link does not come up. Therefore, I'm adding it before starting link up so that users can 
> read it in case some failure occurs in further steps.
> 

I don't understand this. Even if you move it to the end of the function, users
can still read the attributes and learn what happended with the link. We don't
fail if the link doesn't come up.

- Mani

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

[PATCH v6 3/4] Add debugfs based error injection support in DWC

[Shradha Todi]

Add support to provide error injection interface to userspace. This set
of debug registers are part of the RASDES feature present in DesignWare
PCIe controllers.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 Documentation/ABI/testing/debugfs-dwc-pcie    |  70 ++++++++
 .../controller/dwc/pcie-designware-debugfs.c  | 165 +++++++++++++++++-
 2 files changed, 233 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie b/Documentation/ABI/testing/debugfs-dwc-pcie
index e8ed34e988ef..9eae0ab1dbea 100644
--- a/Documentation/ABI/testing/debugfs-dwc-pcie
+++ b/Documentation/ABI/testing/debugfs-dwc-pcie
@@ -11,3 +11,73 @@ Contact:	Shradha Todi <shradha.t@samsung.com>
 Description:	(RW) Write the lane number to be checked as valid or invalid. Read
 		will return the status of PIPE RXVALID signal of the selected lane.
 		The default selected lane is Lane0.
+
+What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
+Date:		Feburary 2025
+Contact:	Shradha Todi <shradha.t@samsung.com>
+Description:	rasdes_err_inj is the directory which can be used to inject errors in the
+		system. The possible errors that can be injected are:
+
+		1) TLP LCRC error injection TX Path - tx_lcrc
+		2) 16b CRC error injection of ACK/NAK DLLP - b16_crc_dllp
+		3) 16b CRC error injection of Update-FC DLLP - b16_crc_upd_fc
+		4) TLP ECRC error injection TX Path - tx_ecrc
+		5) TLP's FCRC error injection TX Path - fcrc_tlp
+		6) Parity error of TSOS - parity_tsos
+		7) Parity error on SKPOS - parity_skpos
+		8) LCRC error injection RX Path - rx_lcrc
+		9) ECRC error injection RX Path - rx_ecrc
+		10) TLPs SEQ# error - tlp_err_seq
+		11) DLLPS ACK/NAK SEQ# error - ack_nak_dllp_seq
+		12) ACK/NAK DLLPs transmission block - ack_nak_dllp
+		13) UpdateFC DLLPs transmission block - upd_fc_dllp
+		14) Always transmission for NAK DLLP - nak_dllp
+		15) Invert SYNC header - inv_sync_hdr_sym
+		16) COM/PAD TS1 order set - com_pad_ts1
+		17) COM/PAD TS2 order set - com_pad_ts2
+		18) COM/FTS FTS order set - com_fts
+		19) COM/IDL E-idle order set - com_idl
+		20) END/EDB symbol - end_edb
+		21) STP/SDP symbol - stp_sdp
+		22) COM/SKP SKP order set - com_skp
+		23) Posted TLP Header credit value control - posted_tlp_hdr
+		24) Non-Posted TLP Header credit value control - non_post_tlp_hdr
+		25) Completion TLP Header credit value control - cmpl_tlp_hdr
+		26) Posted TLP Data credit value control - posted_tlp_data
+		27) Non-Posted TLP Data credit value control - non_post_tlp_data
+		28) Completion TLP Data credit value control - cmpl_tlp_data
+		29) Generates duplicate TLPs - duplicate_dllp
+		30) Generates Nullified TLPs - nullified_tlp
+
+		Each of the possible errors are WO attributes. Write to the attribute will
+		prepare controller to inject the respective error in the next transmission
+		of data. Parameter required to write will change in the following ways:
+
+		i) Errors 9) - 10) are sequence errors. The write command for these will be
+
+			echo <count> <diff> > /sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
+
+			<count>
+				Number of errors to be injected
+			<diff>
+				The difference to add or subtract from natural sequence number to
+				generate sequence error. Range (-4095 : 4095)
+
+		ii) Errors 23) - 28) are credit value error insertions. Write command:
+
+			echo <count> <diff> <vc> > /sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
+
+			<count>
+				Number of errors to be injected
+			<diff>
+				The difference to add or subtract from UpdateFC credit value.
+				Range (-4095 : 4095)
+			<vc>
+				Target VC number
+
+		iii) All other errors. Write command:
+
+			echo <count> > /sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
+
+			<count>
+				Number of errors to be injected
diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
index fe799d36fa7f..dfb0840390d3 100644
--- a/drivers/pci/controller/dwc/pcie-designware-debugfs.c
+++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
@@ -17,6 +17,20 @@
 #define PIPE_DETECT_LANE		BIT(17)
 #define LANE_SELECT			GENMASK(3, 0)
 
+#define ERR_INJ0_OFF			0x34
+#define EINJ_VAL_DIFF			GENMASK(28, 16)
+#define EINJ_VC_NUM			GENMASK(14, 12)
+#define EINJ_TYPE_SHIFT			8
+#define EINJ0_TYPE			GENMASK(11, 8)
+#define EINJ1_TYPE			BIT(8)
+#define EINJ2_TYPE			GENMASK(9, 8)
+#define EINJ3_TYPE			GENMASK(10, 8)
+#define EINJ4_TYPE			GENMASK(10, 8)
+#define EINJ5_TYPE			BIT(8)
+#define EINJ_COUNT			GENMASK(7, 0)
+
+#define ERR_INJ_ENABLE_REG		0x30
+
 #define DWC_DEBUGFS_BUF_MAX		128
 
 struct dwc_pcie_vsec_id {
@@ -55,6 +69,72 @@ struct dwc_pcie_rasdes_info {
 	struct mutex reg_lock;
 };
 
+/**
+ * struct dwc_pcie_rasdes_priv - Stores file specific private data information
+ * @pci: Reference to the dw_pcie structure
+ * @idx: Index to point to specific file related information in array of structs
+ *
+ * All debugfs files will have this struct as its private data.
+ */
+struct dwc_pcie_rasdes_priv {
+	struct dw_pcie *pci;
+	int idx;
+};
+
+/**
+ * struct dwc_pcie_err_inj - Store details about each error injection supported by DWC RASDES
+ * @name: Name of the error that can be injected
+ * @err_inj_group: Group number to which the error belongs to. Value can range from 0 - 5
+ * @err_inj_type: Each group can have multiple types of error
+ */
+struct dwc_pcie_err_inj {
+	const char *name;
+	u32 err_inj_group;
+	u32 err_inj_type;
+};
+
+static const struct dwc_pcie_err_inj err_inj_list[] = {
+	{"tx_lcrc", 0x0, 0x0},
+	{"b16_crc_dllp", 0x0, 0x1},
+	{"b16_crc_upd_fc", 0x0, 0x2},
+	{"tx_ecrc", 0x0, 0x3},
+	{"fcrc_tlp", 0x0, 0x4},
+	{"parity_tsos", 0x0, 0x5},
+	{"parity_skpos", 0x0, 0x6},
+	{"rx_lcrc", 0x0, 0x8},
+	{"rx_ecrc", 0x0, 0xb},
+	{"tlp_err_seq", 0x1, 0x0},
+	{"ack_nak_dllp_seq", 0x1, 0x1},
+	{"ack_nak_dllp", 0x2, 0x0},
+	{"upd_fc_dllp", 0x2, 0x1},
+	{"nak_dllp", 0x2, 0x2},
+	{"inv_sync_hdr_sym", 0x3, 0x0},
+	{"com_pad_ts1", 0x3, 0x1},
+	{"com_pad_ts2", 0x3, 0x2},
+	{"com_fts", 0x3, 0x3},
+	{"com_idl", 0x3, 0x4},
+	{"end_edb", 0x3, 0x5},
+	{"stp_sdp", 0x3, 0x6},
+	{"com_skp", 0x3, 0x7},
+	{"posted_tlp_hdr", 0x4, 0x0},
+	{"non_post_tlp_hdr", 0x4, 0x1},
+	{"cmpl_tlp_hdr", 0x4, 0x2},
+	{"posted_tlp_data", 0x4, 0x4},
+	{"non_post_tlp_data", 0x4, 0x5},
+	{"cmpl_tlp_data", 0x4, 0x6},
+	{"duplicate_dllp", 0x5, 0x0},
+	{"nullified_tlp", 0x5, 0x1},
+};
+
+static const u32 err_inj_type_mask[] = {
+	EINJ0_TYPE,
+	EINJ1_TYPE,
+	EINJ2_TYPE,
+	EINJ3_TYPE,
+	EINJ4_TYPE,
+	EINJ5_TYPE,
+};
+
 static ssize_t lane_detect_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
 	struct dw_pcie *pci = file->private_data;
@@ -115,6 +195,63 @@ static ssize_t rx_valid_write(struct file *file, const char __user *buf, size_t
 	return lane_detect_write(file, buf, count, ppos);
 }
 
+static ssize_t err_inj_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
+{
+	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	u32 val, counter, vc_num, err_group, type_mask;
+	int val_diff = 0;
+	char *kern_buf;
+
+	err_group = err_inj_list[pdata->idx].err_inj_group;
+	type_mask = err_inj_type_mask[err_group];
+
+	kern_buf = memdup_user_nul(buf, count);
+	if (IS_ERR(kern_buf))
+		return PTR_ERR(kern_buf);
+
+	if (err_group == 4) {
+		val = sscanf(kern_buf, "%u %d %u", &counter, &val_diff, &vc_num);
+		if ((val != 3) || (val_diff < -4095 || val_diff > 4095)) {
+			kfree(kern_buf);
+			return -EINVAL;
+		}
+	} else if (err_group == 1) {
+		val = sscanf(kern_buf, "%u %d", &counter, &val_diff);
+		if ((val != 2) || (val_diff < -4095 || val_diff > 4095)) {
+			kfree(kern_buf);
+			return -EINVAL;
+		}
+	} else {
+		val = kstrtou32(kern_buf, 0, &counter);
+		if (val) {
+			kfree(kern_buf);
+			return val;
+		}
+	}
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + ERR_INJ0_OFF + (0x4 * err_group));
+	val &= ~(type_mask | EINJ_COUNT);
+	val |= ((err_inj_list[pdata->idx].err_inj_type << EINJ_TYPE_SHIFT) & type_mask);
+	val |= FIELD_PREP(EINJ_COUNT, counter);
+
+	if (err_group == 1 || err_group == 4) {
+		val &= ~(EINJ_VAL_DIFF);
+		val |= FIELD_PREP(EINJ_VAL_DIFF, val_diff);
+	}
+	if (err_group == 4) {
+		val &= ~(EINJ_VC_NUM);
+		val |= FIELD_PREP(EINJ_VC_NUM, vc_num);
+	}
+
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + ERR_INJ0_OFF + (0x4 * err_group), val);
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + ERR_INJ_ENABLE_REG, (0x1 << err_group));
+
+	kfree(kern_buf);
+	return count;
+}
+
 #define dwc_debugfs_create(name)			\
 debugfs_create_file(#name, 0644, rasdes_debug, pci,	\
 			&dbg_ ## name ## _fops)
@@ -129,6 +266,11 @@ static const struct file_operations dbg_ ## name ## _fops = {	\
 DWC_DEBUGFS_FOPS(lane_detect);
 DWC_DEBUGFS_FOPS(rx_valid);
 
+static const struct file_operations dwc_pcie_err_inj_ops = {
+	.open = simple_open,
+	.write = err_inj_write,
+};
+
 static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
 {
 	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
@@ -138,11 +280,12 @@ static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
 
 static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
 {
-	struct dentry *rasdes_debug;
+	struct dentry *rasdes_debug, *rasdes_err_inj;
 	struct dwc_pcie_rasdes_info *rasdes_info;
+	struct dwc_pcie_rasdes_priv *priv_tmp;
 	const struct dwc_pcie_vsec_id *vid;
 	struct device *dev = pci->dev;
-	int ras_cap;
+	int ras_cap, i, ret;
 
 	for (vid = dwc_pcie_vsec_ids; vid->vendor_id; vid++) {
 		ras_cap = dw_pcie_find_vsec_capability(pci, vid->vendor_id,
@@ -161,6 +304,7 @@ static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
 
 	/* Create subdirectories for Debug, Error injection, Statistics */
 	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
+	rasdes_err_inj = debugfs_create_dir("rasdes_err_inj", dir);
 
 	mutex_init(&rasdes_info->reg_lock);
 	rasdes_info->ras_cap_offset = ras_cap;
@@ -170,7 +314,24 @@ static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
 	dwc_debugfs_create(lane_detect);
 	dwc_debugfs_create(rx_valid);
 
+	/* Create debugfs files for Error injection subdirectory */
+	for (i = 0; i < ARRAY_SIZE(err_inj_list); i++) {
+		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
+		if (!priv_tmp) {
+			ret = -ENOMEM;
+			goto err_deinit;
+		}
+
+		priv_tmp->idx = i;
+		priv_tmp->pci = pci;
+		debugfs_create_file(err_inj_list[i].name, 0200, rasdes_err_inj, priv_tmp,
+				    &dwc_pcie_err_inj_ops);
+	}
 	return 0;
+
+err_deinit:
+	dwc_pcie_rasdes_debugfs_deinit(pci);
+	return ret;
 }
 
 void dwc_pcie_debugfs_deinit(struct dw_pcie *pci)
-- 
2.17.1

Re: [PATCH v6 3/4] Add debugfs based error injection support in DWC

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 04:20:06PM +0530, Shradha Todi wrote:
> Add support to provide error injection interface to userspace. This set
> of debug registers are part of the RASDES feature present in DesignWare
> PCIe controllers.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  Documentation/ABI/testing/debugfs-dwc-pcie    |  70 ++++++++
>  .../controller/dwc/pcie-designware-debugfs.c  | 165 +++++++++++++++++-
>  2 files changed, 233 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie b/Documentation/ABI/testing/debugfs-dwc-pcie
> index e8ed34e988ef..9eae0ab1dbea 100644
> --- a/Documentation/ABI/testing/debugfs-dwc-pcie
> +++ b/Documentation/ABI/testing/debugfs-dwc-pcie
> @@ -11,3 +11,73 @@ Contact:	Shradha Todi <shradha.t@samsung.com>
>  Description:	(RW) Write the lane number to be checked as valid or invalid. Read
>  		will return the status of PIPE RXVALID signal of the selected lane.
>  		The default selected lane is Lane0.
> +
> +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
> +Date:		Feburary 2025
> +Contact:	Shradha Todi <shradha.t@samsung.com>
> +Description:	rasdes_err_inj is the directory which can be used to inject errors in the
> +		system. The possible errors that can be injected are:
> +
> +		1) TLP LCRC error injection TX Path - tx_lcrc
> +		2) 16b CRC error injection of ACK/NAK DLLP - b16_crc_dllp
> +		3) 16b CRC error injection of Update-FC DLLP - b16_crc_upd_fc
> +		4) TLP ECRC error injection TX Path - tx_ecrc
> +		5) TLP's FCRC error injection TX Path - fcrc_tlp
> +		6) Parity error of TSOS - parity_tsos
> +		7) Parity error on SKPOS - parity_skpos
> +		8) LCRC error injection RX Path - rx_lcrc
> +		9) ECRC error injection RX Path - rx_ecrc
> +		10) TLPs SEQ# error - tlp_err_seq
> +		11) DLLPS ACK/NAK SEQ# error - ack_nak_dllp_seq
> +		12) ACK/NAK DLLPs transmission block - ack_nak_dllp
> +		13) UpdateFC DLLPs transmission block - upd_fc_dllp
> +		14) Always transmission for NAK DLLP - nak_dllp
> +		15) Invert SYNC header - inv_sync_hdr_sym
> +		16) COM/PAD TS1 order set - com_pad_ts1
> +		17) COM/PAD TS2 order set - com_pad_ts2
> +		18) COM/FTS FTS order set - com_fts
> +		19) COM/IDL E-idle order set - com_idl
> +		20) END/EDB symbol - end_edb
> +		21) STP/SDP symbol - stp_sdp
> +		22) COM/SKP SKP order set - com_skp
> +		23) Posted TLP Header credit value control - posted_tlp_hdr
> +		24) Non-Posted TLP Header credit value control - non_post_tlp_hdr
> +		25) Completion TLP Header credit value control - cmpl_tlp_hdr
> +		26) Posted TLP Data credit value control - posted_tlp_data
> +		27) Non-Posted TLP Data credit value control - non_post_tlp_data
> +		28) Completion TLP Data credit value control - cmpl_tlp_data
> +		29) Generates duplicate TLPs - duplicate_dllp
> +		30) Generates Nullified TLPs - nullified_tlp
> +
> +		Each of the possible errors are WO attributes. Write to the attribute will
> +		prepare controller to inject the respective error in the next transmission
> +		of data. Parameter required to write will change in the following ways:
> +
> +		i) Errors 9) - 10) are sequence errors. The write command for these will be
> +
> +			echo <count> <diff> > /sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
> +
> +			<count>
> +				Number of errors to be injected
> +			<diff>
> +				The difference to add or subtract from natural sequence number to
> +				generate sequence error. Range (-4095 : 4095)
> +
> +		ii) Errors 23) - 28) are credit value error insertions. Write command:
> +
> +			echo <count> <diff> <vc> > /sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
> +
> +			<count>
> +				Number of errors to be injected
> +			<diff>
> +				The difference to add or subtract from UpdateFC credit value.
> +				Range (-4095 : 4095)
> +			<vc>
> +				Target VC number
> +
> +		iii) All other errors. Write command:
> +
> +			echo <count> > /sys/kernel/debug/dwc_pcie_<dev>/rasdes_err_inj/<error>
> +
> +			<count>
> +				Number of errors to be injected
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> index fe799d36fa7f..dfb0840390d3 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -17,6 +17,20 @@
>  #define PIPE_DETECT_LANE		BIT(17)
>  #define LANE_SELECT			GENMASK(3, 0)
>  
> +#define ERR_INJ0_OFF			0x34
> +#define EINJ_VAL_DIFF			GENMASK(28, 16)
> +#define EINJ_VC_NUM			GENMASK(14, 12)
> +#define EINJ_TYPE_SHIFT			8
> +#define EINJ0_TYPE			GENMASK(11, 8)
> +#define EINJ1_TYPE			BIT(8)
> +#define EINJ2_TYPE			GENMASK(9, 8)
> +#define EINJ3_TYPE			GENMASK(10, 8)
> +#define EINJ4_TYPE			GENMASK(10, 8)
> +#define EINJ5_TYPE			BIT(8)
> +#define EINJ_COUNT			GENMASK(7, 0)
> +
> +#define ERR_INJ_ENABLE_REG		0x30
> +
>  #define DWC_DEBUGFS_BUF_MAX		128
>  
>  struct dwc_pcie_vsec_id {
> @@ -55,6 +69,72 @@ struct dwc_pcie_rasdes_info {
>  	struct mutex reg_lock;
>  };
>  
> +/**
> + * struct dwc_pcie_rasdes_priv - Stores file specific private data information
> + * @pci: Reference to the dw_pcie structure
> + * @idx: Index to point to specific file related information in array of structs
> + *
> + * All debugfs files will have this struct as its private data.
> + */
> +struct dwc_pcie_rasdes_priv {
> +	struct dw_pcie *pci;
> +	int idx;
> +};
> +
> +/**
> + * struct dwc_pcie_err_inj - Store details about each error injection supported by DWC RASDES
> + * @name: Name of the error that can be injected
> + * @err_inj_group: Group number to which the error belongs to. Value can range from 0 - 5
> + * @err_inj_type: Each group can have multiple types of error
> + */
> +struct dwc_pcie_err_inj {
> +	const char *name;
> +	u32 err_inj_group;
> +	u32 err_inj_type;
> +};
> +
> +static const struct dwc_pcie_err_inj err_inj_list[] = {
> +	{"tx_lcrc", 0x0, 0x0},
> +	{"b16_crc_dllp", 0x0, 0x1},
> +	{"b16_crc_upd_fc", 0x0, 0x2},
> +	{"tx_ecrc", 0x0, 0x3},
> +	{"fcrc_tlp", 0x0, 0x4},
> +	{"parity_tsos", 0x0, 0x5},
> +	{"parity_skpos", 0x0, 0x6},
> +	{"rx_lcrc", 0x0, 0x8},
> +	{"rx_ecrc", 0x0, 0xb},
> +	{"tlp_err_seq", 0x1, 0x0},
> +	{"ack_nak_dllp_seq", 0x1, 0x1},
> +	{"ack_nak_dllp", 0x2, 0x0},
> +	{"upd_fc_dllp", 0x2, 0x1},
> +	{"nak_dllp", 0x2, 0x2},
> +	{"inv_sync_hdr_sym", 0x3, 0x0},
> +	{"com_pad_ts1", 0x3, 0x1},
> +	{"com_pad_ts2", 0x3, 0x2},
> +	{"com_fts", 0x3, 0x3},
> +	{"com_idl", 0x3, 0x4},
> +	{"end_edb", 0x3, 0x5},
> +	{"stp_sdp", 0x3, 0x6},
> +	{"com_skp", 0x3, 0x7},
> +	{"posted_tlp_hdr", 0x4, 0x0},
> +	{"non_post_tlp_hdr", 0x4, 0x1},
> +	{"cmpl_tlp_hdr", 0x4, 0x2},
> +	{"posted_tlp_data", 0x4, 0x4},
> +	{"non_post_tlp_data", 0x4, 0x5},
> +	{"cmpl_tlp_data", 0x4, 0x6},
> +	{"duplicate_dllp", 0x5, 0x0},
> +	{"nullified_tlp", 0x5, 0x1},
> +};
> +
> +static const u32 err_inj_type_mask[] = {
> +	EINJ0_TYPE,
> +	EINJ1_TYPE,
> +	EINJ2_TYPE,
> +	EINJ3_TYPE,
> +	EINJ4_TYPE,
> +	EINJ5_TYPE,
> +};
> +
>  static ssize_t lane_detect_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  {
>  	struct dw_pcie *pci = file->private_data;
> @@ -115,6 +195,63 @@ static ssize_t rx_valid_write(struct file *file, const char __user *buf, size_t
>  	return lane_detect_write(file, buf, count, ppos);
>  }
>  
> +static ssize_t err_inj_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	u32 val, counter, vc_num, err_group, type_mask;
> +	int val_diff = 0;
> +	char *kern_buf;
> +
> +	err_group = err_inj_list[pdata->idx].err_inj_group;
> +	type_mask = err_inj_type_mask[err_group];
> +
> +	kern_buf = memdup_user_nul(buf, count);
> +	if (IS_ERR(kern_buf))
> +		return PTR_ERR(kern_buf);
> +
> +	if (err_group == 4) {
> +		val = sscanf(kern_buf, "%u %d %u", &counter, &val_diff, &vc_num);
> +		if ((val != 3) || (val_diff < -4095 || val_diff > 4095)) {
> +			kfree(kern_buf);
> +			return -EINVAL;
> +		}
> +	} else if (err_group == 1) {
> +		val = sscanf(kern_buf, "%u %d", &counter, &val_diff);
> +		if ((val != 2) || (val_diff < -4095 || val_diff > 4095)) {
> +			kfree(kern_buf);
> +			return -EINVAL;
> +		}
> +	} else {
> +		val = kstrtou32(kern_buf, 0, &counter);
> +		if (val) {
> +			kfree(kern_buf);
> +			return val;
> +		}
> +	}
> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + ERR_INJ0_OFF + (0x4 * err_group));
> +	val &= ~(type_mask | EINJ_COUNT);
> +	val |= ((err_inj_list[pdata->idx].err_inj_type << EINJ_TYPE_SHIFT) & type_mask);
> +	val |= FIELD_PREP(EINJ_COUNT, counter);
> +
> +	if (err_group == 1 || err_group == 4) {
> +		val &= ~(EINJ_VAL_DIFF);
> +		val |= FIELD_PREP(EINJ_VAL_DIFF, val_diff);
> +	}
> +	if (err_group == 4) {
> +		val &= ~(EINJ_VC_NUM);
> +		val |= FIELD_PREP(EINJ_VC_NUM, vc_num);
> +	}
> +
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + ERR_INJ0_OFF + (0x4 * err_group), val);
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + ERR_INJ_ENABLE_REG, (0x1 << err_group));
> +
> +	kfree(kern_buf);
> +	return count;
> +}
> +
>  #define dwc_debugfs_create(name)			\
>  debugfs_create_file(#name, 0644, rasdes_debug, pci,	\
>  			&dbg_ ## name ## _fops)
> @@ -129,6 +266,11 @@ static const struct file_operations dbg_ ## name ## _fops = {	\
>  DWC_DEBUGFS_FOPS(lane_detect);
>  DWC_DEBUGFS_FOPS(rx_valid);
>  
> +static const struct file_operations dwc_pcie_err_inj_ops = {
> +	.open = simple_open,
> +	.write = err_inj_write,
> +};
> +
>  static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
>  {
>  	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> @@ -138,11 +280,12 @@ static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
>  
>  static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
>  {
> -	struct dentry *rasdes_debug;
> +	struct dentry *rasdes_debug, *rasdes_err_inj;
>  	struct dwc_pcie_rasdes_info *rasdes_info;
> +	struct dwc_pcie_rasdes_priv *priv_tmp;
>  	const struct dwc_pcie_vsec_id *vid;
>  	struct device *dev = pci->dev;
> -	int ras_cap;
> +	int ras_cap, i, ret;
>  
>  	for (vid = dwc_pcie_vsec_ids; vid->vendor_id; vid++) {
>  		ras_cap = dw_pcie_find_vsec_capability(pci, vid->vendor_id,
> @@ -161,6 +304,7 @@ static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
>  
>  	/* Create subdirectories for Debug, Error injection, Statistics */
>  	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
> +	rasdes_err_inj = debugfs_create_dir("rasdes_err_inj", dir);
>  
>  	mutex_init(&rasdes_info->reg_lock);
>  	rasdes_info->ras_cap_offset = ras_cap;
> @@ -170,7 +314,24 @@ static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
>  	dwc_debugfs_create(lane_detect);
>  	dwc_debugfs_create(rx_valid);
>  
> +	/* Create debugfs files for Error injection subdirectory */
> +	for (i = 0; i < ARRAY_SIZE(err_inj_list); i++) {
> +		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
> +		if (!priv_tmp) {
> +			ret = -ENOMEM;
> +			goto err_deinit;
> +		}
> +
> +		priv_tmp->idx = i;
> +		priv_tmp->pci = pci;
> +		debugfs_create_file(err_inj_list[i].name, 0200, rasdes_err_inj, priv_tmp,
> +				    &dwc_pcie_err_inj_ops);
> +	}
>  	return 0;
> +
> +err_deinit:
> +	dwc_pcie_rasdes_debugfs_deinit(pci);
> +	return ret;
>  }
>  
>  void dwc_pcie_debugfs_deinit(struct dw_pcie *pci)
> -- 
> 2.17.1
> 

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

[PATCH v6 4/4] Add debugfs based statistical counter support in DWC

[Shradha Todi]

Add support to provide statistical counter interface to userspace. This set
of debug registers are part of the RASDES feature present in DesignWare
PCIe controllers.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 Documentation/ABI/testing/debugfs-dwc-pcie    |  61 +++++
 .../controller/dwc/pcie-designware-debugfs.c  | 229 +++++++++++++++++-
 2 files changed, 289 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie b/Documentation/ABI/testing/debugfs-dwc-pcie
index 9eae0ab1dbea..01aa9d3a00c6 100644
--- a/Documentation/ABI/testing/debugfs-dwc-pcie
+++ b/Documentation/ABI/testing/debugfs-dwc-pcie
@@ -81,3 +81,64 @@ Description:	rasdes_err_inj is the directory which can be used to inject errors
 
 			<count>
 				Number of errors to be injected
+
+What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_event_counters/<event>/counter_enable
+Date:		Feburary 2025
+Contact:	Shradha Todi <shradha.t@samsung.com>
+Description:	rasdes_event_counters is the directory which can be used to collect
+		statistical data about the number of times a certain event has occurred
+		in the controller. The list of possible events are:
+
+		1) EBUF Overflow
+		2) EBUF Underrun
+		3) Decode Error
+		4) Running Disparity Error
+		5) SKP OS Parity Error
+		6) SYNC Header Error
+		7) Rx Valid De-assertion
+		8) CTL SKP OS Parity Error
+		9) 1st Retimer Parity Error
+		10) 2nd Retimer Parity Error
+		11) Margin CRC and Parity Error
+		12) Detect EI Infer
+		13) Receiver Error
+		14) RX Recovery Req
+		15) N_FTS Timeout
+		16) Framing Error
+		17) Deskew Error
+		18) Framing Error In L0
+		19) Deskew Uncompleted Error
+		20) Bad TLP
+		21) LCRC Error
+		22) Bad DLLP
+		23) Replay Number Rollover
+		24) Replay Timeout
+		25) Rx Nak DLLP
+		26) Tx Nak DLLP
+		27) Retry TLP
+		28) FC Timeout
+		29) Poisoned TLP
+		30) ECRC Error
+		31) Unsupported Request
+		32) Completer Abort
+		33) Completion Timeout
+		34) EBUF SKP Add
+		35) EBUF SKP Del
+
+		counter_enable is RW. Write 1 to enable the event counter and write 0 to
+		disable the event counter. Read will return whether the counter is currently
+		enabled	or disabled. Counter is disabled by default.
+
+What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_event_counters/<event>/counter_value
+Date:		Feburary 2025
+Contact:	Shradha Todi <shradha.t@samsung.com>
+Description:	(RO) Read will return the current value of the event counter. To reset the counter,
+		counter should be disabled and enabled back using the 'counter_enable' attribute.
+
+What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_event_counters/<event>/lane_select
+Date:		Feburary 2025
+Contact:	Shradha Todi <shradha.t@samsung.com>
+Description:	(RW) Some lanes in the event list are lane specific events. These include
+		events 1) - 11) and 34) - 35).
+		Write lane number for which counter needs to be enabled/disabled/dumped.
+		Read will return the current selected lane number. Lane0 is selected by default.
diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
index dfb0840390d3..2087185a1968 100644
--- a/drivers/pci/controller/dwc/pcie-designware-debugfs.c
+++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
@@ -31,6 +31,17 @@
 
 #define ERR_INJ_ENABLE_REG		0x30
 
+#define RAS_DES_EVENT_COUNTER_DATA_REG	0xc
+
+#define RAS_DES_EVENT_COUNTER_CTRL_REG	0x8
+#define EVENT_COUNTER_GROUP_SELECT	GENMASK(27, 24)
+#define EVENT_COUNTER_EVENT_SELECT	GENMASK(23, 16)
+#define EVENT_COUNTER_LANE_SELECT	GENMASK(11, 8)
+#define EVENT_COUNTER_STATUS		BIT(7)
+#define EVENT_COUNTER_ENABLE		GENMASK(4, 2)
+#define PER_EVENT_ON			0x3
+#define PER_EVENT_OFF			0x1
+
 #define DWC_DEBUGFS_BUF_MAX		128
 
 struct dwc_pcie_vsec_id {
@@ -135,6 +146,61 @@ static const u32 err_inj_type_mask[] = {
 	EINJ5_TYPE,
 };
 
+/**
+ * struct dwc_pcie_event_counter - Store details about each event counter supported in DWC RASDES
+ * @name: Name of the error counter
+ * @group_no: Group number that the event belongs to. Value ranges from 0 - 4
+ * @event_no: Event number of the particular event. Value ranges from -
+ *		Group 0: 0 - 10
+ *		Group 1: 5 - 13
+ *		Group 2: 0 - 7
+ *		Group 3: 0 - 5
+ *		Group 4: 0 - 1
+ */
+struct dwc_pcie_event_counter {
+	const char *name;
+	u32 group_no;
+	u32 event_no;
+};
+
+static const struct dwc_pcie_event_counter event_list[] = {
+	{"ebuf_overflow", 0x0, 0x0},
+	{"ebuf_underrun", 0x0, 0x1},
+	{"decode_err", 0x0, 0x2},
+	{"running_disparity_err", 0x0, 0x3},
+	{"skp_os_parity_err", 0x0, 0x4},
+	{"sync_header_err", 0x0, 0x5},
+	{"rx_valid_deassertion", 0x0, 0x6},
+	{"ctl_skp_os_parity_err", 0x0, 0x7},
+	{"retimer_parity_err_1st", 0x0, 0x8},
+	{"retimer_parity_err_2nd", 0x0, 0x9},
+	{"margin_crc_parity_err", 0x0, 0xA},
+	{"detect_ei_infer", 0x1, 0x5},
+	{"receiver_err", 0x1, 0x6},
+	{"rx_recovery_req", 0x1, 0x7},
+	{"n_fts_timeout", 0x1, 0x8},
+	{"framing_err", 0x1, 0x9},
+	{"deskew_err", 0x1, 0xa},
+	{"framing_err_in_l0", 0x1, 0xc},
+	{"deskew_uncompleted_err", 0x1, 0xd},
+	{"bad_tlp", 0x2, 0x0},
+	{"lcrc_err", 0x2, 0x1},
+	{"bad_dllp", 0x2, 0x2},
+	{"replay_num_rollover", 0x2, 0x3},
+	{"replay_timeout", 0x2, 0x4},
+	{"rx_nak_dllp", 0x2, 0x5},
+	{"tx_nak_dllp", 0x2, 0x6},
+	{"retry_tlp", 0x2, 0x7},
+	{"fc_timeout", 0x3, 0x0},
+	{"poisoned_tlp", 0x3, 0x1},
+	{"ecrc_error", 0x3, 0x2},
+	{"unsupported_request", 0x3, 0x3},
+	{"completer_abort", 0x3, 0x4},
+	{"completion_timeout", 0x3, 0x5},
+	{"ebuf_skp_add", 0x4, 0x0},
+	{"ebuf_skp_del", 0x4, 0x1},
+};
+
 static ssize_t lane_detect_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
 	struct dw_pcie *pci = file->private_data;
@@ -252,6 +318,127 @@ static ssize_t err_inj_write(struct file *file, const char __user *buf, size_t c
 	return count;
 }
 
+static void set_event_number(struct dwc_pcie_rasdes_priv *pdata, struct dw_pcie *pci,
+			     struct dwc_pcie_rasdes_info *rinfo)
+{
+	u32 val;
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
+	val &= ~EVENT_COUNTER_ENABLE;
+	val &= ~(EVENT_COUNTER_GROUP_SELECT | EVENT_COUNTER_EVENT_SELECT);
+	val |= FIELD_PREP(EVENT_COUNTER_GROUP_SELECT, event_list[pdata->idx].group_no);
+	val |= FIELD_PREP(EVENT_COUNTER_EVENT_SELECT, event_list[pdata->idx].event_no);
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+}
+
+static ssize_t counter_enable_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
+	ssize_t off = 0;
+	u32 val;
+
+	mutex_lock(&rinfo->reg_lock);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
+	mutex_unlock(&rinfo->reg_lock);
+	val = FIELD_GET(EVENT_COUNTER_STATUS, val);
+	if (val)
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Counter Enabled\n");
+	else
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Counter Disabled\n");
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t counter_enable_write(struct file *file, const char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	u32 val, enable;
+
+	val = kstrtou32_from_user(buf, count, 0, &enable);
+	if (val)
+		return val;
+
+	mutex_lock(&rinfo->reg_lock);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
+	if (enable)
+		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_ON);
+	else
+		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_OFF);
+
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+	mutex_unlock(&rinfo->reg_lock);
+
+	return count;
+}
+
+static ssize_t counter_lane_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
+	ssize_t off = 0;
+	u32 val;
+
+	mutex_lock(&rinfo->reg_lock);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
+	mutex_unlock(&rinfo->reg_lock);
+	val = FIELD_GET(EVENT_COUNTER_LANE_SELECT, val);
+	off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane: %d\n", val);
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t counter_lane_write(struct file *file, const char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	u32 val, lane;
+
+	val = kstrtou32_from_user(buf, count, 0, &lane);
+	if (val)
+		return val;
+
+	mutex_lock(&rinfo->reg_lock);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
+	val &= ~(EVENT_COUNTER_LANE_SELECT);
+	val |= FIELD_PREP(EVENT_COUNTER_LANE_SELECT, lane);
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+	mutex_unlock(&rinfo->reg_lock);
+
+	return count;
+}
+
+static ssize_t counter_value_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
+{
+	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
+	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
+	ssize_t off = 0;
+	u32 val;
+
+	mutex_lock(&rinfo->reg_lock);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_DATA_REG);
+	mutex_unlock(&rinfo->reg_lock);
+	off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Counter value: %d\n", val);
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
 #define dwc_debugfs_create(name)			\
 debugfs_create_file(#name, 0644, rasdes_debug, pci,	\
 			&dbg_ ## name ## _fops)
@@ -271,6 +458,23 @@ static const struct file_operations dwc_pcie_err_inj_ops = {
 	.write = err_inj_write,
 };
 
+static const struct file_operations dwc_pcie_counter_enable_ops = {
+	.open = simple_open,
+	.read = counter_enable_read,
+	.write = counter_enable_write,
+};
+
+static const struct file_operations dwc_pcie_counter_lane_ops = {
+	.open = simple_open,
+	.read = counter_lane_read,
+	.write = counter_lane_write,
+};
+
+static const struct file_operations dwc_pcie_counter_value_ops = {
+	.open = simple_open,
+	.read = counter_value_read,
+};
+
 static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
 {
 	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
@@ -280,7 +484,7 @@ static void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
 
 static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
 {
-	struct dentry *rasdes_debug, *rasdes_err_inj;
+	struct dentry *rasdes_debug, *rasdes_err_inj, *rasdes_event_counter, *rasdes_events;
 	struct dwc_pcie_rasdes_info *rasdes_info;
 	struct dwc_pcie_rasdes_priv *priv_tmp;
 	const struct dwc_pcie_vsec_id *vid;
@@ -305,6 +509,7 @@ static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
 	/* Create subdirectories for Debug, Error injection, Statistics */
 	rasdes_debug = debugfs_create_dir("rasdes_debug", dir);
 	rasdes_err_inj = debugfs_create_dir("rasdes_err_inj", dir);
+	rasdes_event_counter = debugfs_create_dir("rasdes_event_counter", dir);
 
 	mutex_init(&rasdes_info->reg_lock);
 	rasdes_info->ras_cap_offset = ras_cap;
@@ -327,6 +532,28 @@ static int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci, struct dentry *dir)
 		debugfs_create_file(err_inj_list[i].name, 0200, rasdes_err_inj, priv_tmp,
 				    &dwc_pcie_err_inj_ops);
 	}
+
+	/* Create debugfs files for Statistical counter subdirectory */
+	for (i = 0; i < ARRAY_SIZE(event_list); i++) {
+		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
+		if (!priv_tmp) {
+			ret = -ENOMEM;
+			goto err_deinit;
+		}
+
+		priv_tmp->idx = i;
+		priv_tmp->pci = pci;
+		rasdes_events = debugfs_create_dir(event_list[i].name, rasdes_event_counter);
+		if (event_list[i].group_no == 0 || event_list[i].group_no == 4) {
+			debugfs_create_file("lane_select", 0644, rasdes_events,
+					    priv_tmp, &dwc_pcie_counter_lane_ops);
+		}
+		debugfs_create_file("counter_value", 0444, rasdes_events, priv_tmp,
+				    &dwc_pcie_counter_value_ops);
+		debugfs_create_file("counter_enable", 0644, rasdes_events, priv_tmp,
+				    &dwc_pcie_counter_enable_ops);
+	}
+
 	return 0;
 
 err_deinit:
-- 
2.17.1

Re: [PATCH v6 4/4] Add debugfs based statistical counter support in DWC

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 04:20:07PM +0530, Shradha Todi wrote:
> Add support to provide statistical counter interface to userspace. This set
> of debug registers are part of the RASDES feature present in DesignWare
> PCIe controllers.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  Documentation/ABI/testing/debugfs-dwc-pcie    |  61 +++++
>  .../controller/dwc/pcie-designware-debugfs.c  | 229 +++++++++++++++++-
>  2 files changed, 289 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/debugfs-dwc-pcie b/Documentation/ABI/testing/debugfs-dwc-pcie
> index 9eae0ab1dbea..01aa9d3a00c6 100644
> --- a/Documentation/ABI/testing/debugfs-dwc-pcie
> +++ b/Documentation/ABI/testing/debugfs-dwc-pcie
> @@ -81,3 +81,64 @@ Description:	rasdes_err_inj is the directory which can be used to inject errors
>  
>  			<count>
>  				Number of errors to be injected
> +
> +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_event_counters/<event>/counter_enable
> +Date:		Feburary 2025
> +Contact:	Shradha Todi <shradha.t@samsung.com>
> +Description:	rasdes_event_counters is the directory which can be used to collect
> +		statistical data about the number of times a certain event has occurred
> +		in the controller. The list of possible events are:
> +
> +		1) EBUF Overflow
> +		2) EBUF Underrun
> +		3) Decode Error
> +		4) Running Disparity Error
> +		5) SKP OS Parity Error
> +		6) SYNC Header Error
> +		7) Rx Valid De-assertion
> +		8) CTL SKP OS Parity Error
> +		9) 1st Retimer Parity Error
> +		10) 2nd Retimer Parity Error
> +		11) Margin CRC and Parity Error
> +		12) Detect EI Infer
> +		13) Receiver Error
> +		14) RX Recovery Req
> +		15) N_FTS Timeout
> +		16) Framing Error
> +		17) Deskew Error
> +		18) Framing Error In L0
> +		19) Deskew Uncompleted Error
> +		20) Bad TLP
> +		21) LCRC Error
> +		22) Bad DLLP
> +		23) Replay Number Rollover
> +		24) Replay Timeout
> +		25) Rx Nak DLLP
> +		26) Tx Nak DLLP
> +		27) Retry TLP
> +		28) FC Timeout
> +		29) Poisoned TLP
> +		30) ECRC Error
> +		31) Unsupported Request
> +		32) Completer Abort
> +		33) Completion Timeout
> +		34) EBUF SKP Add
> +		35) EBUF SKP Del
> +
> +		counter_enable is RW. Write 1 to enable the event counter and write 0 to

Please use (RW) pattern at the start as you did for other attributes.

> +		disable the event counter. Read will return whether the counter is currently
> +		enabled	or disabled. Counter is disabled by default.
> +
> +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_event_counters/<event>/counter_value
> +Date:		Feburary 2025
> +Contact:	Shradha Todi <shradha.t@samsung.com>
> +Description:	(RO) Read will return the current value of the event counter. To reset the counter,
> +		counter should be disabled and enabled back using the 'counter_enable' attribute.
> +
> +What:		/sys/kernel/debug/dwc_pcie_<dev>/rasdes_event_counters/<event>/lane_select
> +Date:		Feburary 2025
> +Contact:	Shradha Todi <shradha.t@samsung.com>
> +Description:	(RW) Some lanes in the event list are lane specific events. These include
> +		events 1) - 11) and 34) - 35).
> +		Write lane number for which counter needs to be enabled/disabled/dumped.
> +		Read will return the current selected lane number. Lane0 is selected by default.
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.c b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> index dfb0840390d3..2087185a1968 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -31,6 +31,17 @@
>  
>  #define ERR_INJ_ENABLE_REG		0x30
>  
> +#define RAS_DES_EVENT_COUNTER_DATA_REG	0xc
> +
> +#define RAS_DES_EVENT_COUNTER_CTRL_REG	0x8
> +#define EVENT_COUNTER_GROUP_SELECT	GENMASK(27, 24)
> +#define EVENT_COUNTER_EVENT_SELECT	GENMASK(23, 16)
> +#define EVENT_COUNTER_LANE_SELECT	GENMASK(11, 8)
> +#define EVENT_COUNTER_STATUS		BIT(7)
> +#define EVENT_COUNTER_ENABLE		GENMASK(4, 2)
> +#define PER_EVENT_ON			0x3
> +#define PER_EVENT_OFF			0x1
> +
>  #define DWC_DEBUGFS_BUF_MAX		128
>  
>  struct dwc_pcie_vsec_id {
> @@ -135,6 +146,61 @@ static const u32 err_inj_type_mask[] = {
>  	EINJ5_TYPE,
>  };
>  
> +/**
> + * struct dwc_pcie_event_counter - Store details about each event counter supported in DWC RASDES
> + * @name: Name of the error counter
> + * @group_no: Group number that the event belongs to. Value ranges from 0 - 4
> + * @event_no: Event number of the particular event. Value ranges from -
> + *		Group 0: 0 - 10
> + *		Group 1: 5 - 13
> + *		Group 2: 0 - 7
> + *		Group 3: 0 - 5
> + *		Group 4: 0 - 1
> + */
> +struct dwc_pcie_event_counter {
> +	const char *name;
> +	u32 group_no;
> +	u32 event_no;
> +};
> +
> +static const struct dwc_pcie_event_counter event_list[] = {
> +	{"ebuf_overflow", 0x0, 0x0},
> +	{"ebuf_underrun", 0x0, 0x1},
> +	{"decode_err", 0x0, 0x2},
> +	{"running_disparity_err", 0x0, 0x3},
> +	{"skp_os_parity_err", 0x0, 0x4},
> +	{"sync_header_err", 0x0, 0x5},
> +	{"rx_valid_deassertion", 0x0, 0x6},
> +	{"ctl_skp_os_parity_err", 0x0, 0x7},
> +	{"retimer_parity_err_1st", 0x0, 0x8},
> +	{"retimer_parity_err_2nd", 0x0, 0x9},
> +	{"margin_crc_parity_err", 0x0, 0xA},
> +	{"detect_ei_infer", 0x1, 0x5},
> +	{"receiver_err", 0x1, 0x6},
> +	{"rx_recovery_req", 0x1, 0x7},
> +	{"n_fts_timeout", 0x1, 0x8},
> +	{"framing_err", 0x1, 0x9},
> +	{"deskew_err", 0x1, 0xa},
> +	{"framing_err_in_l0", 0x1, 0xc},
> +	{"deskew_uncompleted_err", 0x1, 0xd},
> +	{"bad_tlp", 0x2, 0x0},
> +	{"lcrc_err", 0x2, 0x1},
> +	{"bad_dllp", 0x2, 0x2},
> +	{"replay_num_rollover", 0x2, 0x3},
> +	{"replay_timeout", 0x2, 0x4},
> +	{"rx_nak_dllp", 0x2, 0x5},
> +	{"tx_nak_dllp", 0x2, 0x6},
> +	{"retry_tlp", 0x2, 0x7},
> +	{"fc_timeout", 0x3, 0x0},
> +	{"poisoned_tlp", 0x3, 0x1},
> +	{"ecrc_error", 0x3, 0x2},
> +	{"unsupported_request", 0x3, 0x3},
> +	{"completer_abort", 0x3, 0x4},
> +	{"completion_timeout", 0x3, 0x5},
> +	{"ebuf_skp_add", 0x4, 0x0},
> +	{"ebuf_skp_del", 0x4, 0x1},
> +};
> +
>  static ssize_t lane_detect_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  {
>  	struct dw_pcie *pci = file->private_data;
> @@ -252,6 +318,127 @@ static ssize_t err_inj_write(struct file *file, const char __user *buf, size_t c
>  	return count;
>  }
>  
> +static void set_event_number(struct dwc_pcie_rasdes_priv *pdata, struct dw_pcie *pci,
> +			     struct dwc_pcie_rasdes_info *rinfo)
> +{
> +	u32 val;
> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	val &= ~EVENT_COUNTER_ENABLE;
> +	val &= ~(EVENT_COUNTER_GROUP_SELECT | EVENT_COUNTER_EVENT_SELECT);
> +	val |= FIELD_PREP(EVENT_COUNTER_GROUP_SELECT, event_list[pdata->idx].group_no);
> +	val |= FIELD_PREP(EVENT_COUNTER_EVENT_SELECT, event_list[pdata->idx].event_no);
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +}
> +
> +static ssize_t counter_enable_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
> +	ssize_t off = 0;
> +	u32 val;
> +
> +	mutex_lock(&rinfo->reg_lock);
> +	set_event_number(pdata, pci, rinfo);
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	mutex_unlock(&rinfo->reg_lock);
> +	val = FIELD_GET(EVENT_COUNTER_STATUS, val);
> +	if (val)
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Counter Enabled\n");

Here also, adding 'off' doesn't make sense.

> +	else
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Counter Disabled\n");
> +
> +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> +}
> +
> +static ssize_t counter_enable_write(struct file *file, const char __user *buf,
> +				    size_t count, loff_t *ppos)
> +{
> +	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	u32 val, enable;
> +
> +	val = kstrtou32_from_user(buf, count, 0, &enable);
> +	if (val)
> +		return val;
> +
> +	mutex_lock(&rinfo->reg_lock);
> +	set_event_number(pdata, pci, rinfo);
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	if (enable)
> +		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_ON);
> +	else
> +		val |= FIELD_PREP(EVENT_COUNTER_ENABLE, PER_EVENT_OFF);
> +
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +	mutex_unlock(&rinfo->reg_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t counter_lane_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
> +	ssize_t off = 0;
> +	u32 val;
> +
> +	mutex_lock(&rinfo->reg_lock);
> +	set_event_number(pdata, pci, rinfo);
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	mutex_unlock(&rinfo->reg_lock);
> +	val = FIELD_GET(EVENT_COUNTER_LANE_SELECT, val);
> +	off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Lane: %d\n", val);

Same here.

> +
> +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> +}
> +
> +static ssize_t counter_lane_write(struct file *file, const char __user *buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	u32 val, lane;
> +
> +	val = kstrtou32_from_user(buf, count, 0, &lane);
> +	if (val)
> +		return val;
> +
> +	mutex_lock(&rinfo->reg_lock);
> +	set_event_number(pdata, pci, rinfo);
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	val &= ~(EVENT_COUNTER_LANE_SELECT);
> +	val |= FIELD_PREP(EVENT_COUNTER_LANE_SELECT, lane);
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +	mutex_unlock(&rinfo->reg_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t counter_value_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct dwc_pcie_rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct dwc_pcie_rasdes_info *rinfo = pci->debugfs->rasdes_info;
> +	char debugfs_buf[DWC_DEBUGFS_BUF_MAX];
> +	ssize_t off = 0;
> +	u32 val;
> +
> +	mutex_lock(&rinfo->reg_lock);
> +	set_event_number(pdata, pci, rinfo);
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap_offset + RAS_DES_EVENT_COUNTER_DATA_REG);
> +	mutex_unlock(&rinfo->reg_lock);
> +	off += scnprintf(debugfs_buf, DWC_DEBUGFS_BUF_MAX - off, "Counter value: %d\n", val);

Here also.

- Mani

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

Re: [PATCH v6 0/4] Add support for debugfs based RAS DES feature in PCIe DW

[Niklas Cassel]

On Fri, Feb 14, 2025 at 04:20:03PM +0530, Shradha Todi wrote:
> DesignWare controller provides a vendor specific extended capability
> called RASDES as an IP feature. This extended capability  provides
> hardware information like:
>  - Debug registers to know the state of the link or controller. 
>  - Error injection mechanisms to inject various PCIe errors including
>    sequence number, CRC
>  - Statistical counters to know how many times a particular event
>    occurred
> 
> However, in Linux we do not have any generic or custom support to be
> able to use this feature in an efficient manner. This is the reason we
> are proposing this framework. Debug and bring up time of high-speed IPs
> are highly dependent on costlier hardware analyzers and this solution
> will in some ways help to reduce the HW analyzer usage.
> 
> The debugfs entries can be used to get information about underlying
> hardware and can be shared with user space. Separate debugfs entries has
> been created to cater to all the DES hooks provided by the controller.
> The debugfs entries interacts with the RASDES registers in the required
> sequence and provides the meaningful data to the user. This eases the
> effort to understand and use the register information for debugging.
> 
> This series creates a generic debugfs framework for DesignWare PCIe
> controllers where other debug features apart from RASDES can also be
> added as and when required.

FWIW, for the series:
Tested-by: Niklas Cassel <cassel@kernel.org>

RE: [PATCH v6 0/4] Add support for debugfs based RAS DES feature in PCIe DW

[Shradha Todi]

> -----Original Message-----
> From: Niklas Cassel <cassel@kernel.org>
> Sent: 19 February 2025 23:16
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com; jingoohan1@gmail.com; Jonathan.Cameron@huawei.com;
> fan.ni@samsung.com; nifan.cxl@gmail.com; a.manzanares@samsung.com; pankaj.dubey@samsung.com; 18255117159@163.com;
> quic_nitegupt@quicinc.com; quic_krichai@quicinc.com; gost.dev@samsung.com
> Subject: Re: [PATCH v6 0/4] Add support for debugfs based RAS DES feature in PCIe DW
> 
> On Fri, Feb 14, 2025 at 04:20:03PM +0530, Shradha Todi wrote:
> > DesignWare controller provides a vendor specific extended capability
> > called RASDES as an IP feature. This extended capability  provides
> > hardware information like:
> >  - Debug registers to know the state of the link or controller.
> >  - Error injection mechanisms to inject various PCIe errors including
> >    sequence number, CRC
> >  - Statistical counters to know how many times a particular event
> >    occurred
> >
> > However, in Linux we do not have any generic or custom support to be
> > able to use this feature in an efficient manner. This is the reason we
> > are proposing this framework. Debug and bring up time of high-speed
> > IPs are highly dependent on costlier hardware analyzers and this
> > solution will in some ways help to reduce the HW analyzer usage.
> >
> > The debugfs entries can be used to get information about underlying
> > hardware and can be shared with user space. Separate debugfs entries
> > has been created to cater to all the DES hooks provided by the controller.
> > The debugfs entries interacts with the RASDES registers in the
> > required sequence and provides the meaningful data to the user. This
> > eases the effort to understand and use the register information for debugging.
> >
> > This series creates a generic debugfs framework for DesignWare PCIe
> > controllers where other debug features apart from RASDES can also be
> > added as and when required.
> 
> FWIW, for the series:
> Tested-by: Niklas Cassel <cassel@kernel.org>

Hey Niklas, sent out v7 with a few minor modifications. Would be great if you could test that :)