[PATCH v3 0/3] Add support for RAS DES feature in PCIe DW

[PATCH v3 0/3] Add support for 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.

v2: https://lore.kernel.org/lkml/20240319163315.GD3297@thinkpad/T/

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

Shradha Todi (3):
  PCI: dwc: Add support for vendor specific capability search
  PCI: debugfs: Add support for RASDES framework in DWC
  PCI: dwc: Create debugfs files in DWC driver

 drivers/pci/controller/dwc/Kconfig            |   8 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 .../controller/dwc/pcie-designware-debugfs.c  | 474 ++++++++++++++++++
 .../controller/dwc/pcie-designware-debugfs.h  |   0
 .../pci/controller/dwc/pcie-designware-host.c |   2 +
 drivers/pci/controller/dwc/pcie-designware.c  |  20 +
 drivers/pci/controller/dwc/pcie-designware.h  |  18 +
 7 files changed, 523 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
 create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.h

-- 
2.17.1

[PATCH 1/3] 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 | 16 ++++++++++++++++
 drivers/pci/controller/dwc/pcie-designware.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 250cf7f40b85..b74e4a97558e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -275,6 +275,22 @@ 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, u8 vsec_cap)
+{
+	u16 vsec = 0;
+	u32 header;
+
+	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 f8e5431a207b..77686957a30d 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -423,6 +423,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, u8 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

[PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC

[Shradha Todi]

Add support to use the RASDES feature of DesignWare PCIe controller
using debugfs entries.

RASDES is a vendor specific extended PCIe capability which reads the
current hardware internal state of PCIe device. Following primary
features are provided to userspace via debugfs:
- Debug registers
- Error injection
- Statistical counters

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/pci/controller/dwc/Kconfig            |   8 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 .../controller/dwc/pcie-designware-debugfs.c  | 474 ++++++++++++++++++
 .../controller/dwc/pcie-designware-debugfs.h  |   0
 drivers/pci/controller/dwc/pcie-designware.h  |  17 +
 5 files changed, 500 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
 create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.h

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 8afacc90c63b..e8e920470412 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -6,6 +6,14 @@ menu "DesignWare-based PCIe controllers"
 config PCIE_DW
 	bool
 
+config PCIE_DW_DEBUGFS
+	bool "DWC PCIe debugfs entries"
+	help
+	  Enables debugfs entries for the DWC PCIe Controller.
+	  These entries make use of the RAS features in the DW
+	  controller 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 bac103faa523..77bd4f7a2f75 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..af5e4ad53fcb
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
@@ -0,0 +1,474 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Synopsys DesignWare PCIe controller debugfs driver
+ *
+ * Copyright (C) 2023 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * Author: Shradha Todi <shradha.t@samsung.com>
+ */
+
+#include <linux/debugfs.h>
+
+#include "pcie-designware.h"
+
+#define RAS_DES_EVENT_COUNTER_CTRL_REG	0x8
+#define RAS_DES_EVENT_COUNTER_DATA_REG	0xc
+#define SD_STATUS_L1LANE_REG		0xb0
+#define ERR_INJ_ENABLE_REG		0x30
+#define ERR_INJ0_OFF			0x34
+
+#define LANE_DETECT_SHIFT		17
+#define LANE_DETECT_MASK		0x1
+#define PIPE_RXVALID_SHIFT		18
+#define PIPE_RXVALID_MASK		0x1
+
+#define LANE_SELECT_SHIFT		8
+#define LANE_SELECT_MASK		0xf
+#define EVENT_COUNTER_STATUS_SHIFT	7
+#define EVENT_COUNTER_STATUS_MASK	0x1
+#define EVENT_COUNTER_ENABLE		(0x7 << 2)
+#define PER_EVENT_OFF			(0x1 << 2)
+#define PER_EVENT_ON			(0x3 << 2)
+
+#define EINJ_COUNT_MASK			0xff
+#define EINJ_TYPE_MASK			0xf
+#define EINJ_TYPE_SHIFT			8
+#define EINJ_INFO_MASK			0xfffff
+#define EINJ_INFO_SHIFT			12
+
+#define DWC_DEBUGFS_MAX			128
+
+struct rasdes_info {
+	/* to store rasdes capability offset */
+	u32 ras_cap;
+	struct mutex dbg_mutex;
+	struct dentry *rasdes;
+};
+
+struct rasdes_priv {
+	struct dw_pcie *pci;
+	int idx;
+};
+
+struct event_counter {
+	const char *name;
+	/* values can be between 0-15 */
+	u32 group_no;
+	/* values can be between 0-32 */
+	u32 event_no;
+};
+
+static const struct event_counter event_counters[] = {
+	{"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},
+	{"detect_ei_infer", 0x1, 0x5},
+	{"receiver_err", 0x1, 0x6},
+	{"rx_recovery_req", 0x1, 0x7},
+	{"framing_err", 0x1, 0x9},
+	{"deskew_err", 0x1, 0xa},
+	{"bad_tlp", 0x2, 0x0},
+	{"lcrc_err", 0x2, 0x1},
+	{"bad_dllp", 0x2, 0x2},
+};
+
+struct err_inj {
+	const char *name;
+	/* values can be from group 0 - 6 */
+	u32 err_inj_group;
+	/* within each group there can be types */
+	u32 err_inj_type;
+	/* More details about the error */
+	u32 err_inj_12_31;
+};
+
+static const struct err_inj err_inj_list[] = {
+	{"tx_lcrc", 0x0, 0x0, 0x0},
+	{"tx_ecrc", 0x0, 0x3, 0x0},
+	{"rx_lcrc", 0x0, 0x8, 0x0},
+	{"rx_ecrc", 0x0, 0xb, 0x0},
+	{"b16_crc_dllp", 0x0, 0x1, 0x0},
+	{"b16_crc_upd_fc", 0x0, 0x2, 0x0},
+	{"fcrc_tlp", 0x0, 0x4, 0x0},
+	{"parity_tsos", 0x0, 0x5, 0x0},
+	{"parity_skpos", 0x0, 0x6, 0x0},
+	{"ack_nak_dllp", 0x2, 0x0, 0x0},
+	{"upd_fc_dllp", 0x2, 0x1, 0x0},
+	{"nak_dllp", 0x2, 0x2, 0x0},
+	{"inv_sync_hdr_sym", 0x3, 0x0, 0x0},
+	{"com_pad_ts1", 0x3, 0x1, 0x0},
+	{"com_pad_ts2", 0x3, 0x2, 0x0},
+	{"com_fts", 0x3, 0x3, 0x0},
+	{"com_idl", 0x3, 0x4, 0x0},
+	{"end_edb", 0x3, 0x5, 0x0},
+	{"stp_sdp", 0x3, 0x6, 0x0},
+	{"com_skp", 0x3, 0x7, 0x0},
+	{"duplicate_dllp", 0x5, 0x0, 0x0},
+	{"nullified_tlp", 0x5, 0x1, 0x0},
+};
+
+static ssize_t dbg_lane_detect_read(struct file *file, char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	struct dw_pcie *pci = file->private_data;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	ssize_t off = 0;
+	char debugfs_buf[DWC_DEBUGFS_MAX];
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap + SD_STATUS_L1LANE_REG);
+	val = (val >> LANE_DETECT_SHIFT) & LANE_DETECT_MASK;
+	if (val)
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Detected\n");
+	else
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Undetected\n");
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t dbg_lane_detect_write(struct file *file, const char __user *buf,
+				     size_t count, loff_t *ppos)
+{
+	struct dw_pcie *pci = file->private_data;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	u32 lane;
+
+	val = kstrtou32_from_user(buf, count, 0, &lane);
+	if (val)
+		return val;
+
+	if (lane > 15)
+		return -EINVAL;
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap + SD_STATUS_L1LANE_REG);
+	val &= ~LANE_SELECT_MASK;
+	val |= lane;
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap + SD_STATUS_L1LANE_REG, val);
+
+	return count;
+}
+
+static ssize_t dbg_rx_valid_read(struct file *file, char __user *buf,
+				 size_t count, loff_t *ppos)
+{
+	struct dw_pcie *pci = file->private_data;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	ssize_t off = 0;
+	char debugfs_buf[DWC_DEBUGFS_MAX];
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap + SD_STATUS_L1LANE_REG);
+	val = (val >> PIPE_RXVALID_SHIFT) & PIPE_RXVALID_MASK;
+	if (val)
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Valid\n");
+	else
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Invalid\n");
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t dbg_rx_valid_write(struct file *file, const char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	return dbg_lane_detect_write(file, buf, count, ppos);
+}
+
+static void set_event_number(struct rasdes_priv *pdata, struct dw_pcie *pci,
+			     struct rasdes_info *rinfo)
+{
+	u32 val;
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	val &= ~EVENT_COUNTER_ENABLE;
+	val &= ~(0xFFF << 16);
+	val |= (event_counters[pdata->idx].group_no << 24);
+	val |= (event_counters[pdata->idx].event_no << 16);
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap +
+			   RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+}
+
+static ssize_t cnt_en_read(struct file *file, char __user *buf, size_t count,
+			   loff_t *ppos)
+{
+	struct rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	ssize_t off = 0;
+	char debugfs_buf[DWC_DEBUGFS_MAX];
+
+	mutex_lock(&rinfo->dbg_mutex);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	mutex_unlock(&rinfo->dbg_mutex);
+	val = (val >> EVENT_COUNTER_STATUS_SHIFT) & EVENT_COUNTER_STATUS_MASK;
+	if (val)
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Enabled\n");
+	else
+		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Disabled\n");
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t cnt_en_write(struct file *file, const char __user *buf,
+			    size_t count, loff_t *ppos)
+{
+	struct rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	u32 enable;
+
+	val = kstrtou32_from_user(buf, count, 0, &enable);
+	if (val)
+		return val;
+
+	mutex_lock(&rinfo->dbg_mutex);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	if (enable)
+		val |= PER_EVENT_ON;
+	else
+		val |= PER_EVENT_OFF;
+
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap +
+			   RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+	mutex_unlock(&rinfo->dbg_mutex);
+
+	return count;
+}
+
+static ssize_t cnt_lane_read(struct file *file, char __user *buf, size_t count,
+			     loff_t *ppos)
+{
+	struct rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	ssize_t off = 0;
+	char debugfs_buf[DWC_DEBUGFS_MAX];
+
+	mutex_lock(&rinfo->dbg_mutex);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	mutex_unlock(&rinfo->dbg_mutex);
+	val = (val >> LANE_SELECT_SHIFT) & LANE_SELECT_MASK;
+	off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Lane: %d\n", val);
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t cnt_lane_write(struct file *file, const char __user *buf,
+			      size_t count, loff_t *ppos)
+{
+	struct rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	u32 lane;
+
+	val = kstrtou32_from_user(buf, count, 0, &lane);
+	if (val)
+		return val;
+
+	if (lane > 15)
+		return -EINVAL;
+
+	mutex_lock(&rinfo->dbg_mutex);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	val &= ~(LANE_SELECT_MASK << LANE_SELECT_SHIFT);
+	val |= (lane << LANE_SELECT_SHIFT);
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap +
+			   RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+	mutex_unlock(&rinfo->dbg_mutex);
+
+	return count;
+}
+
+static ssize_t cnt_val_read(struct file *file, char __user *buf, size_t count,
+			    loff_t *ppos)
+{
+	struct rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	ssize_t off = 0;
+	char debugfs_buf[DWC_DEBUGFS_MAX];
+
+	mutex_lock(&rinfo->dbg_mutex);
+	set_event_number(pdata, pci, rinfo);
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap +
+				RAS_DES_EVENT_COUNTER_DATA_REG);
+	mutex_unlock(&rinfo->dbg_mutex);
+	off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
+				 "Value: %d\n", val);
+
+	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
+}
+
+static ssize_t err_inj_write(struct file *file, const char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	struct rasdes_priv *pdata = file->private_data;
+	struct dw_pcie *pci = pdata->pci;
+	struct rasdes_info *rinfo = pci->dump_info;
+	u32 val;
+	u32 counter;
+
+	val = kstrtou32_from_user(buf, count, 0, &counter);
+	if (val)
+		return val;
+
+	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap + ERR_INJ0_OFF +
+			(0x4 * err_inj_list[pdata->idx].err_inj_group));
+	val &= ~(EINJ_TYPE_MASK << EINJ_TYPE_SHIFT);
+	val |= err_inj_list[pdata->idx].err_inj_type << EINJ_TYPE_SHIFT;
+	val &= ~(EINJ_INFO_MASK << EINJ_INFO_SHIFT);
+	val |= err_inj_list[pdata->idx].err_inj_12_31 << EINJ_INFO_SHIFT;
+	val &= ~EINJ_COUNT_MASK;
+	val |= counter;
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap + ERR_INJ0_OFF +
+			(0x4 * err_inj_list[pdata->idx].err_inj_group), val);
+	dw_pcie_writel_dbi(pci, rinfo->ras_cap + ERR_INJ_ENABLE_REG,
+			   (0x1 << err_inj_list[pdata->idx].err_inj_group));
+
+	return count;
+}
+
+#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 = {	\
+	.read = dbg_ ## name ## _read,				\
+	.write = dbg_ ## name ## _write				\
+}
+
+DWC_DEBUGFS_FOPS(lane_detect);
+DWC_DEBUGFS_FOPS(rx_valid);
+
+static const struct file_operations cnt_en_ops = {
+	.open = simple_open,
+	.read = cnt_en_read,
+	.write = cnt_en_write,
+};
+
+static const struct file_operations cnt_lane_ops = {
+	.open = simple_open,
+	.read = cnt_lane_read,
+	.write = cnt_lane_write,
+};
+
+static const struct file_operations cnt_val_ops = {
+	.open = simple_open,
+	.read = cnt_val_read,
+};
+
+static const struct file_operations err_inj_ops = {
+	.open = simple_open,
+	.write = err_inj_write,
+};
+
+void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
+{
+	struct rasdes_info *rinfo = pci->dump_info;
+
+	debugfs_remove_recursive(rinfo->rasdes);
+	mutex_destroy(&rinfo->dbg_mutex);
+}
+
+int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci)
+{
+	struct device *dev = pci->dev;
+	int ras_cap;
+	struct rasdes_info *dump_info;
+	char dirname[DWC_DEBUGFS_MAX];
+	struct dentry *dir, *rasdes_debug, *rasdes_err_inj;
+	struct dentry *rasdes_event_counter, *rasdes_events;
+	int i;
+	struct rasdes_priv *priv_tmp;
+
+	ras_cap = dw_pcie_find_vsec_capability(pci, DW_PCIE_RAS_DES_CAP);
+	if (!ras_cap) {
+		dev_err(dev, "No RASDES capability available\n");
+		return -ENODEV;
+	}
+
+	dump_info = devm_kzalloc(dev, sizeof(*dump_info), GFP_KERNEL);
+	if (!dump_info)
+		return -ENOMEM;
+
+	/* Create main directory for each platform driver */
+	sprintf(dirname, "pcie_dwc_%s", dev_name(dev));
+	dir = debugfs_create_dir(dirname, NULL);
+
+	/* 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(&dump_info->dbg_mutex);
+	dump_info->ras_cap = ras_cap;
+	dump_info->rasdes = dir;
+	pci->dump_info = dump_info;
+
+	/* Create debugfs files for Debug subdirectory */
+	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)
+			goto err;
+
+		priv_tmp->idx = i;
+		priv_tmp->pci = pci;
+		debugfs_create_file(err_inj_list[i].name, 0200,
+				    rasdes_err_inj, priv_tmp, &err_inj_ops);
+	}
+
+	/* Create debugfs files for Statistical counter subdirectory */
+	for (i = 0; i < ARRAY_SIZE(event_counters); i++) {
+		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
+		if (!priv_tmp)
+			goto err;
+
+		priv_tmp->idx = i;
+		priv_tmp->pci = pci;
+		rasdes_events = debugfs_create_dir(event_counters[i].name,
+						   rasdes_event_counter);
+		if (event_counters[i].group_no == 0) {
+			debugfs_create_file("lane_select", 0644, rasdes_events,
+					    priv_tmp, &cnt_lane_ops);
+		}
+		debugfs_create_file("counter_value", 0444, rasdes_events, priv_tmp,
+				    &cnt_val_ops);
+		debugfs_create_file("counter_enable", 0644, rasdes_events, priv_tmp,
+				    &cnt_en_ops);
+	}
+
+	return 0;
+err:
+	dwc_pcie_rasdes_debugfs_deinit(pci);
+	return -ENOMEM;
+}
diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.h b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
new file mode 100644
index 000000000000..e69de29bb2d1
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 77686957a30d..9fa9f33e4ddb 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -223,6 +223,8 @@
 
 #define PCIE_RAS_DES_EVENT_COUNTER_DATA		0xc
 
+#define DW_PCIE_RAS_DES_CAP			0x2
+
 /*
  * The default address offset between dbi_base and atu_base. Root controller
  * drivers are not required to initialize atu_base if the offset matches this
@@ -410,6 +412,7 @@ struct dw_pcie {
 	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
 	struct gpio_desc		*pe_rst;
 	bool			suspended;
+	void			*dump_info;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
@@ -745,4 +748,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_rasdes_debugfs_init(struct dw_pcie *pci);
+void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci);
+#else
+static inline int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci)
+{
+	return 0;
+}
+static inline void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
+{
+}
+#endif
+
 #endif /* _PCIE_DESIGNWARE_H */
-- 
2.17.1

[PATCH 3/3] PCI: dwc: Create debugfs files in DWC driver

[Shradha Todi]

Add call to initialize debugfs from DWC driver and create the RASDES
debugfs hierarchy for each platform driver. Since it can be used for
both DW HOST controller as well as DW EP controller, add it in the
common setup function.

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

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d15a5c2d5b48..c2e6f8484000 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -537,6 +537,8 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
 	pci_stop_root_bus(pp->bridge->bus);
 	pci_remove_root_bus(pp->bridge->bus);
 
+	dwc_pcie_rasdes_debugfs_deinit(pci);
+
 	dw_pcie_stop_link(pci);
 
 	dw_pcie_edma_remove(pci);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index b74e4a97558e..ebb21ba75388 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -1084,4 +1084,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
 	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
 
 	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
+
+	val = dwc_pcie_rasdes_debugfs_init(pci);
+	if (val)
+		dev_err(pci->dev, "Couldn't create debugfs files\n");
 }
-- 
2.17.1

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

[Jonathan Cameron]

On Tue, 25 Jun 2024 15:08:11 +0530
Shradha Todi <shradha.t@samsung.com> 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>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h |  1 +
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 250cf7f40b85..b74e4a97558e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -275,6 +275,22 @@ 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, u8 vsec_cap)
> +{
> +	u16 vsec = 0;
> +	u32 header;
> +
> +	while ((vsec = dw_pcie_find_next_ext_capability(pci, vsec,
> +					PCI_EXT_CAP_ID_VNDR))) {

Trivial but one more set of outer brackets than makes sense here.

> +		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 f8e5431a207b..77686957a30d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -423,6 +423,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, u8 vsec_cap);
>  
>  int dw_pcie_read(void __iomem *addr, int size, u32 *val);
>  int dw_pcie_write(void __iomem *addr, int size, u32 val);

Re: [PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC

[Jonathan Cameron]

On Tue, 25 Jun 2024 15:08:12 +0530
Shradha Todi <shradha.t@samsung.com> wrote:

> Add support to use the RASDES feature of DesignWare PCIe controller
> using debugfs entries.
> 
> RASDES is a vendor specific extended PCIe capability which reads the
> current hardware internal state of PCIe device. Following primary
> features are provided to userspace via debugfs:
> - Debug registers
> - Error injection
> - Statistical counters
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>

A few minor things inline.

> +
> +struct rasdes_info {
> +	/* to store rasdes capability offset */
> +	u32 ras_cap;
> +	struct mutex dbg_mutex;

Add a comment on what data this mutex protects.

> +	struct dentry *rasdes;
> +};

> +struct err_inj {
Very generic name is likely to bite in future if similar
gets defined in a header. I'd at least prefix with dw_

> +	const char *name;
> +	/* values can be from group 0 - 6 */
> +	u32 err_inj_group;
> +	/* within each group there can be types */
> +	u32 err_inj_type;
> +	/* More details about the error */
> +	u32 err_inj_12_31;
> +};


> +
> +int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +	int ras_cap;
> +	struct rasdes_info *dump_info;
> +	char dirname[DWC_DEBUGFS_MAX];
> +	struct dentry *dir, *rasdes_debug, *rasdes_err_inj;
> +	struct dentry *rasdes_event_counter, *rasdes_events;
> +	int i;

Perhaps combine with int ras_cap above.

From a quick look, I think this can get called from resume paths
as well as initial setup which doesn't look like a safe thing to do.
imx6_pcie_resume_irq()
dw_pcie_setup_rc()
dw_pcie_setup()
dwc_pcie_rasdes_debugfs_init()


> +	struct rasdes_priv *priv_tmp;
> +
> +	ras_cap = dw_pcie_find_vsec_capability(pci, DW_PCIE_RAS_DES_CAP);
> +	if (!ras_cap) {
> +		dev_err(dev, "No RASDES capability available\n");
> +		return -ENODEV;
> +	}
> +
> +	dump_info = devm_kzalloc(dev, sizeof(*dump_info), GFP_KERNEL);
> +	if (!dump_info)
> +		return -ENOMEM;
> +
> +	/* Create main directory for each platform driver */
> +	sprintf(dirname, "pcie_dwc_%s", dev_name(dev));

dev_name could in theory be huge.  I'd use snprintf() and check the
result as more obviously correct.

> +	dir = debugfs_create_dir(dirname, NULL);

Check for errors in all these.

> +
> +	/* 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(&dump_info->dbg_mutex);
> +	dump_info->ras_cap = ras_cap;
> +	dump_info->rasdes = dir;
> +	pci->dump_info = dump_info;
> +
> +	/* Create debugfs files for Debug subdirectory */
> +	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)
> +			goto err;
> +
> +		priv_tmp->idx = i;
> +		priv_tmp->pci = pci;
> +		debugfs_create_file(err_inj_list[i].name, 0200,
> +				    rasdes_err_inj, priv_tmp, &err_inj_ops);
> +	}
> +
> +	/* Create debugfs files for Statistical counter subdirectory */
> +	for (i = 0; i < ARRAY_SIZE(event_counters); i++) {
> +		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
> +		if (!priv_tmp)
> +			goto err;
> +
> +		priv_tmp->idx = i;
> +		priv_tmp->pci = pci;
> +		rasdes_events = debugfs_create_dir(event_counters[i].name,
> +						   rasdes_event_counter);
> +		if (event_counters[i].group_no == 0) {
> +			debugfs_create_file("lane_select", 0644, rasdes_events,
> +					    priv_tmp, &cnt_lane_ops);
> +		}
> +		debugfs_create_file("counter_value", 0444, rasdes_events, priv_tmp,
> +				    &cnt_val_ops);
> +		debugfs_create_file("counter_enable", 0644, rasdes_events, priv_tmp,
> +				    &cnt_en_ops);
> +	}
> +
> +	return 0;
> +err:
> +	dwc_pcie_rasdes_debugfs_deinit(pci);
> +	return -ENOMEM;
> +}
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.h b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> new file mode 100644
> index 000000000000..e69de29bb2d1
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 77686957a30d..9fa9f33e4ddb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -223,6 +223,8 @@
>  
>  #define PCIE_RAS_DES_EVENT_COUNTER_DATA		0xc
>  
> +#define DW_PCIE_RAS_DES_CAP			0x2

I'd be tempted to try and name that in a fashion that makes it clear it
is a vsec capability ID.  Currently it sounds like a top level
capability ID.

> +
>  /*
>   * The default address offset between dbi_base and atu_base. Root controller
>   * drivers are not required to initialize atu_base if the offset matches this
> @@ -410,6 +412,7 @@ struct dw_pcie {
>  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
>  	struct gpio_desc		*pe_rst;
>  	bool			suspended;
> +	void			*dump_info;
>  };

Re: [PATCH 3/3] PCI: dwc: Create debugfs files in DWC driver

[Jonathan Cameron]

On Tue, 25 Jun 2024 15:08:13 +0530
Shradha Todi <shradha.t@samsung.com> wrote:

> Add call to initialize debugfs from DWC driver and create the RASDES
> debugfs hierarchy for each platform driver. Since it can be used for
> both DW HOST controller as well as DW EP controller, add it in the
> common setup function.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>

Squash this with the previous patch given it's so trivial.

> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 2 ++
>  drivers/pci/controller/dwc/pcie-designware.c      | 4 ++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index d15a5c2d5b48..c2e6f8484000 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -537,6 +537,8 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
>  	pci_stop_root_bus(pp->bridge->bus);
>  	pci_remove_root_bus(pp->bridge->bus);
>  
> +	dwc_pcie_rasdes_debugfs_deinit(pci);
> +
>  	dw_pcie_stop_link(pci);
>  
>  	dw_pcie_edma_remove(pci);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index b74e4a97558e..ebb21ba75388 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1084,4 +1084,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
>  
>  	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
> +
> +	val = dwc_pcie_rasdes_debugfs_init(pci);
> +	if (val)
> +		dev_err(pci->dev, "Couldn't create debugfs files\n");
>  }

Re: [PATCH v3 0/3] Add support for RAS DES feature in PCIe DW

[Jonathan Cameron]

On Tue, 25 Jun 2024 15:08:10 +0530
Shradha Todi <shradha.t@samsung.com> 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.

To consider this properly I think some documentation is needed.

Maybe just ABI in Documentation/ABI/testing/debugfs-*
or maybe a more freestanding document.


> 
> v2: https://lore.kernel.org/lkml/20240319163315.GD3297@thinkpad/T/
> 
> v1: https://lore.kernel.org/all/20210518174618.42089-1-shradha.t@samsung.com/T/
> 
> Shradha Todi (3):
>   PCI: dwc: Add support for vendor specific capability search
>   PCI: debugfs: Add support for RASDES framework in DWC
>   PCI: dwc: Create debugfs files in DWC driver
> 
>  drivers/pci/controller/dwc/Kconfig            |   8 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  .../controller/dwc/pcie-designware-debugfs.c  | 474 ++++++++++++++++++
>  .../controller/dwc/pcie-designware-debugfs.h  |   0
>  .../pci/controller/dwc/pcie-designware-host.c |   2 +
>  drivers/pci/controller/dwc/pcie-designware.c  |  20 +
>  drivers/pci/controller/dwc/pcie-designware.h  |  18 +
>  7 files changed, 523 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
>  create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.h
> 

RE: [PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC

[Shradha Todi]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: 01 July 2024 16:40
> 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;
> fancer.lancer@gmail.com; yoshihiro.shimoda.uh@renesas.com;
> conor.dooley@microchip.com; pankaj.dubey@samsung.com;
> gost.dev@samsung.com
> Subject: Re: [PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC
> 
> On Tue, 25 Jun 2024 15:08:12 +0530
> Shradha Todi <shradha.t@samsung.com> wrote:
> 
> > Add support to use the RASDES feature of DesignWare PCIe controller
> > using debugfs entries.
> >
> > RASDES is a vendor specific extended PCIe capability which reads the
> > current hardware internal state of PCIe device. Following primary
> > features are provided to userspace via debugfs:
> > - Debug registers
> > - Error injection
> > - Statistical counters
> >
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> 
> A few minor things inline.
> 
> > +
> > +struct rasdes_info {
> > +	/* to store rasdes capability offset */
> > +	u32 ras_cap;
> > +	struct mutex dbg_mutex;
> 
> Add a comment on what data this mutex protects.
> 
> > +	struct dentry *rasdes;
> > +};
> 
> > +struct err_inj {
> Very generic name is likely to bite in future if similar gets defined in a
header. I'd at
> least prefix with dw_
> 
> > +	const char *name;
> > +	/* values can be from group 0 - 6 */
> > +	u32 err_inj_group;
> > +	/* within each group there can be types */
> > +	u32 err_inj_type;
> > +	/* More details about the error */
> > +	u32 err_inj_12_31;
> > +};
> 
> 
> > +
> > +int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci) {
> > +	struct device *dev = pci->dev;
> > +	int ras_cap;
> > +	struct rasdes_info *dump_info;
> > +	char dirname[DWC_DEBUGFS_MAX];
> > +	struct dentry *dir, *rasdes_debug, *rasdes_err_inj;
> > +	struct dentry *rasdes_event_counter, *rasdes_events;
> > +	int i;
> 
> Perhaps combine with int ras_cap above.
> 
> From a quick look, I think this can get called from resume paths as well as
initial
> setup which doesn't look like a safe thing to do.
> imx6_pcie_resume_irq()
> dw_pcie_setup_rc()
> dw_pcie_setup()
> dwc_pcie_rasdes_debugfs_init()
> 
> 

Thanks for review. I'm going to post the next version soon with all your
comments addressed.
Yes, I guess this is a fair point that calling it during resume path + setup is
not a safe thing to do.
I was thinking we leave it up to  platform drivers to call it and drop patch 3
altogether as I realised
that there is also no ep_deinit to call the
"dwc_pcie_rasdes_debugfs_deinit(pci)" from.
Would love to know your opinion on this before I proceed to post the changes.

> > +	struct rasdes_priv *priv_tmp;
> > +
> > +	ras_cap = dw_pcie_find_vsec_capability(pci, DW_PCIE_RAS_DES_CAP);
> > +	if (!ras_cap) {
> > +		dev_err(dev, "No RASDES capability available\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	dump_info = devm_kzalloc(dev, sizeof(*dump_info), GFP_KERNEL);
> > +	if (!dump_info)
> > +		return -ENOMEM;
> > +
> > +	/* Create main directory for each platform driver */
> > +	sprintf(dirname, "pcie_dwc_%s", dev_name(dev));
> 
> dev_name could in theory be huge.  I'd use snprintf() and check the result as
more
> obviously correct.
> 
> > +	dir = debugfs_create_dir(dirname, NULL);
> 
> Check for errors in all these.
> 
> > +
> > +	/* 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(&dump_info->dbg_mutex);
> > +	dump_info->ras_cap = ras_cap;
> > +	dump_info->rasdes = dir;
> > +	pci->dump_info = dump_info;
> > +
> > +	/* Create debugfs files for Debug subdirectory */
> > +	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)
> > +			goto err;
> > +
> > +		priv_tmp->idx = i;
> > +		priv_tmp->pci = pci;
> > +		debugfs_create_file(err_inj_list[i].name, 0200,
> > +				    rasdes_err_inj, priv_tmp, &err_inj_ops);
> > +	}
> > +
> > +	/* Create debugfs files for Statistical counter subdirectory */
> > +	for (i = 0; i < ARRAY_SIZE(event_counters); i++) {
> > +		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
> > +		if (!priv_tmp)
> > +			goto err;
> > +
> > +		priv_tmp->idx = i;
> > +		priv_tmp->pci = pci;
> > +		rasdes_events = debugfs_create_dir(event_counters[i].name,
> > +						   rasdes_event_counter);
> > +		if (event_counters[i].group_no == 0) {
> > +			debugfs_create_file("lane_select", 0644, rasdes_events,
> > +					    priv_tmp, &cnt_lane_ops);
> > +		}
> > +		debugfs_create_file("counter_value", 0444, rasdes_events,
> priv_tmp,
> > +				    &cnt_val_ops);
> > +		debugfs_create_file("counter_enable", 0644, rasdes_events,
> priv_tmp,
> > +				    &cnt_en_ops);
> > +	}
> > +
> > +	return 0;
> > +err:
> > +	dwc_pcie_rasdes_debugfs_deinit(pci);
> > +	return -ENOMEM;
> > +}
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> > b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> > new file mode 100644
> > index 000000000000..e69de29bb2d1
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> > b/drivers/pci/controller/dwc/pcie-designware.h
> > index 77686957a30d..9fa9f33e4ddb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -223,6 +223,8 @@
> >
> >  #define PCIE_RAS_DES_EVENT_COUNTER_DATA		0xc
> >
> > +#define DW_PCIE_RAS_DES_CAP			0x2
> 
> I'd be tempted to try and name that in a fashion that makes it clear it is a
vsec
> capability ID.  Currently it sounds like a top level capability ID.
> 
> > +
> >  /*
> >   * The default address offset between dbi_base and atu_base. Root
controller
> >   * drivers are not required to initialize atu_base if the offset
> > matches this @@ -410,6 +412,7 @@ struct dw_pcie {
> >  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
> >  	struct gpio_desc		*pe_rst;
> >  	bool			suspended;
> > +	void			*dump_info;
> >  };
> 

Re: [PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC

[Manivannan Sadhasivam]

On Tue, Jun 25, 2024 at 03:08:12PM +0530, Shradha Todi wrote:
> Add support to use the RASDES feature of DesignWare PCIe controller
> using debugfs entries.
> 
> RASDES is a vendor specific extended PCIe capability which reads the
> current hardware internal state of PCIe device. Following primary
> features are provided to userspace via debugfs:
> - Debug registers
> - Error injection
> - Statistical counters
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  drivers/pci/controller/dwc/Kconfig            |   8 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  .../controller/dwc/pcie-designware-debugfs.c  | 474 ++++++++++++++++++
>  .../controller/dwc/pcie-designware-debugfs.h  |   0
>  drivers/pci/controller/dwc/pcie-designware.h  |  17 +
>  5 files changed, 500 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
>  create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.h
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 8afacc90c63b..e8e920470412 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -6,6 +6,14 @@ menu "DesignWare-based PCIe controllers"
>  config PCIE_DW
>  	bool
>  
> +config PCIE_DW_DEBUGFS
> +	bool "DWC PCIe debugfs entries"

What is the benefit of making this driver selectable? If debugfs is enabled in
the kernel, then exposing this interface for all platforms is of no harm (ofc
you should not spam the log as I mentioned below).

> +	help
> +	  Enables debugfs entries for the DWC PCIe Controller.
> +	  These entries make use of the RAS features in the DW
> +	  controller 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 bac103faa523..77bd4f7a2f75 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..af5e4ad53fcb
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -0,0 +1,474 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2023 Samsung Electronics Co., Ltd.

2024

> + *		http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@samsung.com>
> + */
> +
> +#include <linux/debugfs.h>
> +
> +#include "pcie-designware.h"
> +
> +#define RAS_DES_EVENT_COUNTER_CTRL_REG	0x8
> +#define RAS_DES_EVENT_COUNTER_DATA_REG	0xc
> +#define SD_STATUS_L1LANE_REG		0xb0
> +#define ERR_INJ_ENABLE_REG		0x30
> +#define ERR_INJ0_OFF			0x34
> +
> +#define LANE_DETECT_SHIFT		17
> +#define LANE_DETECT_MASK		0x1
> +#define PIPE_RXVALID_SHIFT		18
> +#define PIPE_RXVALID_MASK		0x1
> +
> +#define LANE_SELECT_SHIFT		8
> +#define LANE_SELECT_MASK		0xf
> +#define EVENT_COUNTER_STATUS_SHIFT	7
> +#define EVENT_COUNTER_STATUS_MASK	0x1
> +#define EVENT_COUNTER_ENABLE		(0x7 << 2)
> +#define PER_EVENT_OFF			(0x1 << 2)
> +#define PER_EVENT_ON			(0x3 << 2)
> +
> +#define EINJ_COUNT_MASK			0xff
> +#define EINJ_TYPE_MASK			0xf
> +#define EINJ_TYPE_SHIFT			8
> +#define EINJ_INFO_MASK			0xfffff
> +#define EINJ_INFO_SHIFT			12
> +
> +#define DWC_DEBUGFS_MAX			128

DWC_DEBUGFS_BUF_MAX?

> +
> +struct rasdes_info {
> +	/* to store rasdes capability offset */

Not needed. If you really want to provide comment, please add kdoc for the whole
struct.

> +	u32 ras_cap;
> +	struct mutex dbg_mutex;

dbg_mutex? Use a better name like reg_lock or something else.

> +	struct dentry *rasdes;
> +};
> +
> +struct rasdes_priv {
> +	struct dw_pcie *pci;
> +	int idx;
> +};
> +
> +struct event_counter {
> +	const char *name;
> +	/* values can be between 0-15 */

Use Kdoc.

> +	u32 group_no;
> +	/* values can be between 0-32 */
> +	u32 event_no;
> +};
> +
> +static const struct event_counter event_counters[] = {
> +	{"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},
> +	{"detect_ei_infer", 0x1, 0x5},
> +	{"receiver_err", 0x1, 0x6},
> +	{"rx_recovery_req", 0x1, 0x7},
> +	{"framing_err", 0x1, 0x9},
> +	{"deskew_err", 0x1, 0xa},
> +	{"bad_tlp", 0x2, 0x0},
> +	{"lcrc_err", 0x2, 0x1},
> +	{"bad_dllp", 0x2, 0x2},
> +};
> +
> +struct err_inj {
> +	const char *name;
> +	/* values can be from group 0 - 6 */

Same here.

> +	u32 err_inj_group;
> +	/* within each group there can be types */
> +	u32 err_inj_type;
> +	/* More details about the error */
> +	u32 err_inj_12_31;
> +};
> +
> +static const struct err_inj err_inj_list[] = {
> +	{"tx_lcrc", 0x0, 0x0, 0x0},
> +	{"tx_ecrc", 0x0, 0x3, 0x0},
> +	{"rx_lcrc", 0x0, 0x8, 0x0},
> +	{"rx_ecrc", 0x0, 0xb, 0x0},
> +	{"b16_crc_dllp", 0x0, 0x1, 0x0},
> +	{"b16_crc_upd_fc", 0x0, 0x2, 0x0},
> +	{"fcrc_tlp", 0x0, 0x4, 0x0},
> +	{"parity_tsos", 0x0, 0x5, 0x0},
> +	{"parity_skpos", 0x0, 0x6, 0x0},
> +	{"ack_nak_dllp", 0x2, 0x0, 0x0},
> +	{"upd_fc_dllp", 0x2, 0x1, 0x0},
> +	{"nak_dllp", 0x2, 0x2, 0x0},
> +	{"inv_sync_hdr_sym", 0x3, 0x0, 0x0},
> +	{"com_pad_ts1", 0x3, 0x1, 0x0},
> +	{"com_pad_ts2", 0x3, 0x2, 0x0},
> +	{"com_fts", 0x3, 0x3, 0x0},
> +	{"com_idl", 0x3, 0x4, 0x0},
> +	{"end_edb", 0x3, 0x5, 0x0},
> +	{"stp_sdp", 0x3, 0x6, 0x0},
> +	{"com_skp", 0x3, 0x7, 0x0},
> +	{"duplicate_dllp", 0x5, 0x0, 0x0},
> +	{"nullified_tlp", 0x5, 0x1, 0x0},
> +};
> +

You can add a Kdoc comment for each helper to document the debugfs entries.

> +static ssize_t dbg_lane_detect_read(struct file *file, char __user *buf,
> +				    size_t count, loff_t *ppos)

Either use 'dwc_debugfs' prefix or no prefix (for static helpers). 'dbg' doesn't
convey much info.

> +{
> +	struct dw_pcie *pci = file->private_data;
> +	struct rasdes_info *rinfo = pci->dump_info;
> +	u32 val;
> +	ssize_t off = 0;
> +	char debugfs_buf[DWC_DEBUGFS_MAX];

Use reverse XMAS tree order.

> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap + SD_STATUS_L1LANE_REG);
> +	val = (val >> LANE_DETECT_SHIFT) & LANE_DETECT_MASK;

Use FIELD_* macros whereever applicable.

> +	if (val)
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
> +				 "Detected\n");
> +	else
> +		off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
> +				 "Undetected\n");
> +
> +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> +}
> +
> +static ssize_t dbg_lane_detect_write(struct file *file, const char __user *buf,
> +				     size_t count, loff_t *ppos)
> +{
> +	struct dw_pcie *pci = file->private_data;
> +	struct rasdes_info *rinfo = pci->dump_info;
> +	u32 val;
> +	u32 lane;
> +
> +	val = kstrtou32_from_user(buf, count, 0, &lane);
> +	if (val)
> +		return val;
> +
> +	if (lane > 15)

What does 15 corresponds to? Can you add a macro?

> +		return -EINVAL;
> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap + SD_STATUS_L1LANE_REG);
> +	val &= ~LANE_SELECT_MASK;
> +	val |= lane;
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap + SD_STATUS_L1LANE_REG, val);
> +
> +	return count;
> +}

[...]

> +static ssize_t cnt_val_read(struct file *file, char __user *buf, size_t count,
> +			    loff_t *ppos)
> +{
> +	struct rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct rasdes_info *rinfo = pci->dump_info;
> +	u32 val;
> +	ssize_t off = 0;
> +	char debugfs_buf[DWC_DEBUGFS_MAX];
> +
> +	mutex_lock(&rinfo->dbg_mutex);
> +	set_event_number(pdata, pci, rinfo);
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap +
> +				RAS_DES_EVENT_COUNTER_DATA_REG);
> +	mutex_unlock(&rinfo->dbg_mutex);
> +	off += scnprintf(debugfs_buf, DWC_DEBUGFS_MAX - off,
> +				 "Value: %d\n", val);

Use elaborative text instead of just 'Value'.

> +
> +	return simple_read_from_buffer(buf, count, ppos, debugfs_buf, off);
> +}
> +
> +static ssize_t err_inj_write(struct file *file, const char __user *buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	struct rasdes_priv *pdata = file->private_data;
> +	struct dw_pcie *pci = pdata->pci;
> +	struct rasdes_info *rinfo = pci->dump_info;
> +	u32 val;
> +	u32 counter;
> +
> +	val = kstrtou32_from_user(buf, count, 0, &counter);
> +	if (val)
> +		return val;
> +
> +	val = dw_pcie_readl_dbi(pci, rinfo->ras_cap + ERR_INJ0_OFF +
> +			(0x4 * err_inj_list[pdata->idx].err_inj_group));
> +	val &= ~(EINJ_TYPE_MASK << EINJ_TYPE_SHIFT);
> +	val |= err_inj_list[pdata->idx].err_inj_type << EINJ_TYPE_SHIFT;
> +	val &= ~(EINJ_INFO_MASK << EINJ_INFO_SHIFT);
> +	val |= err_inj_list[pdata->idx].err_inj_12_31 << EINJ_INFO_SHIFT;
> +	val &= ~EINJ_COUNT_MASK;
> +	val |= counter;
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap + ERR_INJ0_OFF +
> +			(0x4 * err_inj_list[pdata->idx].err_inj_group), val);
> +	dw_pcie_writel_dbi(pci, rinfo->ras_cap + ERR_INJ_ENABLE_REG,
> +			   (0x1 << err_inj_list[pdata->idx].err_inj_group));
> +
> +	return count;
> +}
> +
> +#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 = {	\
> +	.read = dbg_ ## name ## _read,				\
> +	.write = dbg_ ## name ## _write				\
> +}
> +
> +DWC_DEBUGFS_FOPS(lane_detect);
> +DWC_DEBUGFS_FOPS(rx_valid);
> +
> +static const struct file_operations cnt_en_ops = {

'cnt_en_ops' is not descriptive enough. Pick a better name that reflects what
this ops is meant for, like 'counter_enable_ops'. This applies to all other ops
as well.

> +	.open = simple_open,
> +	.read = cnt_en_read,
> +	.write = cnt_en_write,
> +};
> +
> +static const struct file_operations cnt_lane_ops = {
> +	.open = simple_open,
> +	.read = cnt_lane_read,
> +	.write = cnt_lane_write,
> +};
> +
> +static const struct file_operations cnt_val_ops = {
> +	.open = simple_open,
> +	.read = cnt_val_read,
> +};
> +
> +static const struct file_operations err_inj_ops = {
> +	.open = simple_open,
> +	.write = err_inj_write,
> +};
> +
> +void dwc_pcie_rasdes_debugfs_deinit(struct dw_pcie *pci)
> +{
> +	struct rasdes_info *rinfo = pci->dump_info;
> +
> +	debugfs_remove_recursive(rinfo->rasdes);
> +	mutex_destroy(&rinfo->dbg_mutex);
> +}
> +
> +int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +	int ras_cap;
> +	struct rasdes_info *dump_info;
> +	char dirname[DWC_DEBUGFS_MAX];
> +	struct dentry *dir, *rasdes_debug, *rasdes_err_inj;
> +	struct dentry *rasdes_event_counter, *rasdes_events;
> +	int i;
> +	struct rasdes_priv *priv_tmp;
> +
> +	ras_cap = dw_pcie_find_vsec_capability(pci, DW_PCIE_RAS_DES_CAP);
> +	if (!ras_cap) {
> +		dev_err(dev, "No RASDES capability available\n");

If all controllers start calling this API, then this will become a noise. Can
you move it to dev_dbg()?

> +		return -ENODEV;
> +	}
> +
> +	dump_info = devm_kzalloc(dev, sizeof(*dump_info), GFP_KERNEL);
> +	if (!dump_info)
> +		return -ENOMEM;
> +
> +	/* Create main directory for each platform driver */
> +	sprintf(dirname, "pcie_dwc_%s", dev_name(dev));
> +	dir = debugfs_create_dir(dirname, NULL);
> +
> +	/* 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(&dump_info->dbg_mutex);
> +	dump_info->ras_cap = ras_cap;
> +	dump_info->rasdes = dir;
> +	pci->dump_info = dump_info;
> +
> +	/* Create debugfs files for Debug subdirectory */
> +	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)
> +			goto err;
> +
> +		priv_tmp->idx = i;
> +		priv_tmp->pci = pci;
> +		debugfs_create_file(err_inj_list[i].name, 0200,

Use macros for file attributes.

> +				    rasdes_err_inj, priv_tmp, &err_inj_ops);
> +	}
> +
> +	/* Create debugfs files for Statistical counter subdirectory */
> +	for (i = 0; i < ARRAY_SIZE(event_counters); i++) {
> +		priv_tmp = devm_kzalloc(dev, sizeof(*priv_tmp), GFP_KERNEL);
> +		if (!priv_tmp)
> +			goto err;
> +
> +		priv_tmp->idx = i;
> +		priv_tmp->pci = pci;
> +		rasdes_events = debugfs_create_dir(event_counters[i].name,
> +						   rasdes_event_counter);
> +		if (event_counters[i].group_no == 0) {
> +			debugfs_create_file("lane_select", 0644, rasdes_events,
> +					    priv_tmp, &cnt_lane_ops);
> +		}
> +		debugfs_create_file("counter_value", 0444, rasdes_events, priv_tmp,
> +				    &cnt_val_ops);

What does the 'counter value' represent?

> +		debugfs_create_file("counter_enable", 0644, rasdes_events, priv_tmp,
> +				    &cnt_en_ops);
> +	}
> +
> +	return 0;
> +err:

err_deinit?

> +	dwc_pcie_rasdes_debugfs_deinit(pci);
> +	return -ENOMEM;

It is really a bad design to return a fixed error no in the error path. Pass the
error no and return it.

> +}
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.h b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> new file mode 100644
> index 000000000000..e69de29bb2d1
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 77686957a30d..9fa9f33e4ddb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -223,6 +223,8 @@
>  
>  #define PCIE_RAS_DES_EVENT_COUNTER_DATA		0xc
>  
> +#define DW_PCIE_RAS_DES_CAP			0x2
> +
>  /*
>   * The default address offset between dbi_base and atu_base. Root controller
>   * drivers are not required to initialize atu_base if the offset matches this
> @@ -410,6 +412,7 @@ struct dw_pcie {
>  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
>  	struct gpio_desc		*pe_rst;
>  	bool			suspended;
> +	void			*dump_info;

Why an opaque pointer? Also, the name is not elaborative.

- Mani

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

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

[Bjorn Helgaas]

On Tue, Jun 25, 2024 at 03:08:11PM +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>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++++++++
>  drivers/pci/controller/dwc/pcie-designware.h |  1 +
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 250cf7f40b85..b74e4a97558e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -275,6 +275,22 @@ 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, u8 vsec_cap)
> +{
> +	u16 vsec = 0;
> +	u32 header;

IIUC, any use of PCI_EXT_CAP_ID_VNDR should check dev->vendor, as
pci_find_vsec_capability() does.

You only know how "vsec" works if you also know the vendor who defined
vsec.

Do you expect DW_PCIE_RAS_DES_CAP to be present only in devices with
Vendor ID of PCI_VENDOR_ID_SYNOPSYS?

I was hoping that this was generic DesignWare functionality that might
be present in devices from other vendors that incorporated the DWC IP.
Those devices would likely have other Vendor IDs.  In that case, a
DVSEC (not VSEC) capability might be more appropriate.

> +	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);

Re: [PATCH 2/3] PCI: debugfs: Add support for RASDES framework in DWC

[Bjorn Helgaas]

Based on the files touched, this looks DWC-specific, so the subject
prefix should be "PCI: dwc: ", not the very generic "debugfs".
The "debugfs" part could go later, e.g.,

  PCI: dwc: Add RASDES debugfs support

On Tue, Jun 25, 2024 at 03:08:12PM +0530, Shradha Todi wrote:
> Add support to use the RASDES feature of DesignWare PCIe controller
> using debugfs entries.
> 
> RASDES is a vendor specific extended PCIe capability which reads the
> current hardware internal state of PCIe device. Following primary
> features are provided to userspace via debugfs:
> - Debug registers
> - Error injection
> - Statistical counters

This looks like great stuff, thanks a lot for implementing this!

I think this debugfs structure and functionality should be documented
somewhere like Documentation/ABI/testing/.  This functionality is
likely to be used by userspace tools like perf that will depend on
this ABI.  (Oh, sorry, I just saw Jonathan's similar comment, didn't
mean to duplicate it.)

I don't expect other vendors to implement exactly the same
functionality, but we can at least try to use similar structure if
they do.

> +config PCIE_DW_DEBUGFS
> +	bool "DWC PCIe debugfs entries"
> +	help
> +	  Enables debugfs entries for the DWC PCIe Controller.
> +	  These entries make use of the RAS features in the DW
> +	  controller to help in debug, error injection and statistical
> +	  counters

> +int dwc_pcie_rasdes_debugfs_init(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +	int ras_cap;
> +	struct rasdes_info *dump_info;
> +	char dirname[DWC_DEBUGFS_MAX];
> +	struct dentry *dir, *rasdes_debug, *rasdes_err_inj;
> +	struct dentry *rasdes_event_counter, *rasdes_events;
> +	int i;
> +	struct rasdes_priv *priv_tmp;
> +
> +	ras_cap = dw_pcie_find_vsec_capability(pci, DW_PCIE_RAS_DES_CAP);

Does this look at config space of a DWC Root Port, or is this in a
RCRB or similar?

Bjorn

Re: [PATCH v3 0/3] Add support for RAS DES feature in PCIe DW

[Krishna Chaitanya Chundru]

+cc nitesh

On 6/25/2024 3:08 PM, 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.
> 
> v2: https://lore.kernel.org/lkml/20240319163315.GD3297@thinkpad/T/
> 
> v1: https://lore.kernel.org/all/20210518174618.42089-1-shradha.t@samsung.com/T/
> 
> Shradha Todi (3):
>    PCI: dwc: Add support for vendor specific capability search
>    PCI: debugfs: Add support for RASDES framework in DWC
>    PCI: dwc: Create debugfs files in DWC driver
> 
>   drivers/pci/controller/dwc/Kconfig            |   8 +
>   drivers/pci/controller/dwc/Makefile           |   1 +
>   .../controller/dwc/pcie-designware-debugfs.c  | 474 ++++++++++++++++++
>   .../controller/dwc/pcie-designware-debugfs.h  |   0
>   .../pci/controller/dwc/pcie-designware-host.c |   2 +
>   drivers/pci/controller/dwc/pcie-designware.c  |  20 +
>   drivers/pci/controller/dwc/pcie-designware.h  |  18 +
>   7 files changed, 523 insertions(+)
>   create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
>   create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.h
> 

Re: [PATCH v3 0/3] Add support for RAS DES feature in PCIe DW

[Krishna Chaitanya Chundru]

forgot to add the email in the previous mail.

- Krishna chaitanya.
On 6/25/2024 3:08 PM, 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.
> 
> v2: https://lore.kernel.org/lkml/20240319163315.GD3297@thinkpad/T/
> 
> v1: https://lore.kernel.org/all/20210518174618.42089-1-shradha.t@samsung.com/T/
> 
> Shradha Todi (3):
>    PCI: dwc: Add support for vendor specific capability search
>    PCI: debugfs: Add support for RASDES framework in DWC
>    PCI: dwc: Create debugfs files in DWC driver
> 
>   drivers/pci/controller/dwc/Kconfig            |   8 +
>   drivers/pci/controller/dwc/Makefile           |   1 +
>   .../controller/dwc/pcie-designware-debugfs.c  | 474 ++++++++++++++++++
>   .../controller/dwc/pcie-designware-debugfs.h  |   0
>   .../pci/controller/dwc/pcie-designware-host.c |   2 +
>   drivers/pci/controller/dwc/pcie-designware.c  |  20 +
>   drivers/pci/controller/dwc/pcie-designware.h  |  18 +
>   7 files changed, 523 insertions(+)
>   create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.c
>   create mode 100644 drivers/pci/controller/dwc/pcie-designware-debugfs.h
> 

Re: [PATCH v3 0/3] Add support for RAS DES feature in PCIe DW

[Nitesh Gupta]

Hi Shradha,

Can you please update on status of this Patch?

Are you going to take it up or is it fine for us to take it up?

-Nitesh Gupta

On 11/26/2024 10:47 AM, Krishna Chaitanya Chundru wrote:
>
> forgot to add the email in the previous mail.
>
> - Krishna chaitanya.
> On 6/25/2024 3:08 PM, 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.
>>
>> v2: https://lore.kernel.org/lkml/20240319163315.GD3297@thinkpad/T/
>>
>> v1: 
>> https://lore.kernel.org/all/20210518174618.42089-1-shradha.t@samsung.com/T/
>>
>> Shradha Todi (3):
>>    PCI: dwc: Add support for vendor specific capability search
>>    PCI: debugfs: Add support for RASDES framework in DWC
>>    PCI: dwc: Create debugfs files in DWC driver
>>
>>   drivers/pci/controller/dwc/Kconfig            |   8 +
>>   drivers/pci/controller/dwc/Makefile           |   1 +
>>   .../controller/dwc/pcie-designware-debugfs.c  | 474 ++++++++++++++++++
>>   .../controller/dwc/pcie-designware-debugfs.h  |   0
>>   .../pci/controller/dwc/pcie-designware-host.c |   2 +
>>   drivers/pci/controller/dwc/pcie-designware.c  |  20 +
>>   drivers/pci/controller/dwc/pcie-designware.h  |  18 +
>>   7 files changed, 523 insertions(+)
>>   create mode 100644 
>> drivers/pci/controller/dwc/pcie-designware-debugfs.c
>>   create mode 100644 
>> drivers/pci/controller/dwc/pcie-designware-debugfs.h
>>

RE: [PATCH v3 0/3] Add support for RAS DES feature in PCIe DW

[Shradha Todi]

Hey Nitish,

Due to some discussions about including this in the EDAC framework etc, the patches got
delayed. Sorry about that. I am already working on the next version and will post it by this
Friday! Feel free to add review comments to the previous version if there are any so that
I can include them in my next version.

> -----Original Message-----
> From: Nitesh Gupta <quic_nitegupt@quicinc.com>
> Sent: 26 November 2024 12:46
> To: Krishna Chaitanya Chundru <quic_krichai@quicinc.com>; Shradha Todi
> <shradha.t@samsung.com>; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org
> Cc: manivannan.sadhasivam@linaro.org; lpieralisi@kernel.org;
> kw@linux.com; robh@kernel.org; bhelgaas@google.com;
> jingoohan1@gmail.com; fancer.lancer@gmail.com;
> yoshihiro.shimoda.uh@renesas.com; conor.dooley@microchip.com;
> pankaj.dubey@samsung.com; gost.dev@samsung.com
> Subject: Re: [PATCH v3 0/3] Add support for RAS DES feature in PCIe DW
> 
> Hi Shradha,
> 
> Can you please update on status of this Patch?
> 
> Are you going to take it up or is it fine for us to take it up?
> 
> -Nitesh Gupta
> 
> On 11/26/2024 10:47 AM, Krishna Chaitanya Chundru wrote:
> >
> > forgot to add the email in the previous mail.
> >
> > - Krishna chaitanya.
> > On 6/25/2024 3:08 PM, 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.
> >>
> >> v2: https://lore.kernel.org/lkml/20240319163315.GD3297@thinkpad/T/
> >>
> >> v1:
> >> https://lore.kernel.org/all/20210518174618.42089-1-shradha.t@samsung.
> >> com/T/
> >>
> >> Shradha Todi (3):
> >>    PCI: dwc: Add support for vendor specific capability search
> >>    PCI: debugfs: Add support for RASDES framework in DWC
> >>    PCI: dwc: Create debugfs files in DWC driver
> >>
> >>   drivers/pci/controller/dwc/Kconfig            |   8 +
> >>   drivers/pci/controller/dwc/Makefile           |   1 +
> >>   .../controller/dwc/pcie-designware-debugfs.c  | 474
> >> ++++++++++++++++++
> >>   .../controller/dwc/pcie-designware-debugfs.h  |   0
> >>   .../pci/controller/dwc/pcie-designware-host.c |   2 +
> >>   drivers/pci/controller/dwc/pcie-designware.c  |  20 +
> >>   drivers/pci/controller/dwc/pcie-designware.h  |  18 +
> >>   7 files changed, 523 insertions(+)
> >>   create mode 100644
> >> drivers/pci/controller/dwc/pcie-designware-debugfs.c
> >>   create mode 100644
> >> drivers/pci/controller/dwc/pcie-designware-debugfs.h
> >>