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

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

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

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

 drivers/pci/controller/dwc/Kconfig            |   9 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 .../controller/dwc/pcie-designware-debugfs.c  | 544 ++++++++++++++++++
 .../controller/dwc/pcie-designware-debugfs.h  | 133 +++++
 drivers/pci/controller/dwc/pcie-designware.c  |  21 +
 drivers/pci/controller/dwc/pcie-designware.h  |   5 +
 6 files changed, 713 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 |  4 ++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index a945f0c0e73d..348f6f696976 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -90,6 +90,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 7d6e9b7576be..307525aaa063 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -155,6 +155,9 @@
 #define MAX_IATU_IN			256
 #define MAX_IATU_OUT			256
 
+/* Synopsys Vendor specific data */
+#define DW_PCIE_RAS_CAP_ID		0x2
+
 struct pcie_port;
 struct dw_pcie;
 struct dw_pcie_ep;
@@ -284,6 +287,7 @@ struct dw_pcie {
 
 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 RAS framework in DWC

[Shradha Todi]

RAS is a vendor specific extended PCIe capability which which helps to read
hardware internal state.

This framework support provides debugfs entries to use these features for
DesignWare controller. Following primary features of DesignWare controllers
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            |   9 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 .../controller/dwc/pcie-designware-debugfs.c  | 544 ++++++++++++++++++
 .../controller/dwc/pcie-designware-debugfs.h  | 133 +++++
 drivers/pci/controller/dwc/pcie-designware.h  |   1 +
 5 files changed, 688 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 423d35872ce4..2d0a18cb9418 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -6,6 +6,15 @@ menu "DesignWare PCI Core Support"
 config PCIE_DW
 	bool
 
+config PCIE_DW_DEBUGFS
+	bool "DWC PCIe debugfs entries"
+	default n
+	help
+	  Enables debugfs entries for the DWC PCIe Controller.
+	  These entries make use of the RAS DES features in the DW
+	  controller to help in debug, error injection and statistical
+	  counters
+
 config PCIE_DW_HOST
 	bool
 	depends on PCI_MSI_IRQ_DOMAIN
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index eca805c1a023..6ad12638f793 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..84bf196df240
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
@@ -0,0 +1,544 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Synopsys DesignWare PCIe controller debugfs driver
+ *
+ * Copyright (C) 2021 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * Author: Shradha Todi <shradha.t@samsung.com>
+ */
+
+#include <linux/fs.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+#include "pcie-designware.h"
+#include "pcie-designware-debugfs.h"
+
+#define DEBUGFS_BUF_MAX_SIZE	100
+
+static char debugfs_buf[DEBUGFS_BUF_MAX_SIZE];
+
+static struct dentry *dir;
+static struct dentry *sub_dir;
+
+static struct event_counters ras_events[] = {
+	{"ebuf_overflow",		ebuf_overflow},
+	{"ebuf_underrun",		ebuf_underrun},
+	{"decode_error",		decode_error},
+	{"sync_header_error",		sync_header_error},
+	{"receiver_error",		receiver_error},
+	{"framing_error",		framing_error},
+	{"lcrc_error",			lcrc_error},
+	{"ecrc_error",			ecrc_error},
+	{"unsupp_req_error",		unsupp_req_error},
+	{"cmpltr_abort_error",		cmpltr_abort_error},
+	{"cmpltn_timeout_error",	cmpltn_timeout_error},
+	{"tx_l0s_entry",		tx_l0s_entry},
+	{"rx_l0s_entry",		rx_l0s_entry},
+	{"l1_entry",			l1_entry},
+	{"l1_1_entry",			l1_1_entry},
+	{"l1_2_entry",			l1_2_entry},
+	{"l2_entry",			l2_entry},
+	{"speed_change",		speed_change},
+	{"width_chage",			width_chage},
+	{0, 0},
+};
+
+static struct error_injections error_list[] = {
+	{"tx_lcrc",		tx_lcrc},
+	{"tx_ecrc",		tx_ecrc},
+	{"rx_lcrc",		rx_lcrc},
+	{"rx_ecrc",		rx_ecrc},
+	{0, 0},
+};
+
+static int open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode->i_private;
+
+	return 0;
+}
+
+/*
+ * set_event_number: Function to set event number based on filename
+ *
+ * This function is called from the common read and write function
+ * written for all event counters. Using the debugfs filname, the
+ * group number and event number for the counter is extracted and
+ * then programmed into the control register.
+ *
+ * @file: file pointer to the debugfs entry
+ *
+ * Return: void
+ */
+static void set_event_number(struct file *file)
+{
+	int i;
+	u32 val;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+	u32 max_size = sizeof(ras_events) / sizeof(struct event_counters);
+
+	for (i = 0; i < max_size; i++) {
+		if (strcmp(ras_events[i].name,
+			   file->f_path.dentry->d_parent->d_iname) == 0) {
+			val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+					RAS_DES_EVENT_COUNTER_CTRL_REG);
+			val &= ~(EVENT_COUNTER_ENABLE);
+			val &= ~(0xFFF << 16);
+			val |= (ras_events[i].event_num << 16);
+			dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
+					RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+			break;
+		}
+	}
+}
+/*
+ * get_error_inj_number: Function to get error number based on filename
+ *
+ * This function is called from the common read and write function
+ * written for all error injection debugfs entries. Using the debugfs
+ * filname, the error group and type of error to be injected is extracted.
+ *
+ * @file: file pointer to the debugfs entry
+ *
+ * Return: u32
+ * [31:8]: Type of error to be injected
+ * [7:0]: Group of error it belongs to
+ */
+
+static u32 get_error_inj_number(struct file *file)
+{
+	int i;
+	u32 max_size = sizeof(error_list) / sizeof(struct error_injections);
+
+	for (i = 0; i < max_size; i++) {
+		if (strcmp(error_list[i].name,
+			   file->f_path.dentry->d_iname) == 0) {
+			return error_list[i].type_of_err;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * ras_event_counter_en_read: Function to get if counter is enable
+ *
+ * This function is invoked when the following command is made:
+ * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_enable
+ * It returns whether the counter is enabled or not
+ */
+static ssize_t ras_event_counter_en_read(struct file *file, char __user *buf,
+					 size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	set_event_number(file);
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	val = (val >> EVENT_COUNTER_STATUS_SHIFT) & EVENT_COUNTER_STATUS_MASK;
+	if (val)
+		sprintf(debugfs_buf, "Enabled\n");
+	else
+		sprintf(debugfs_buf, "Disabled\n");
+
+	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
+				      strlen(debugfs_buf));
+
+	return ret;
+}
+
+/*
+ * ras_event_counter_lane_sel_read: Function to get lane number selected
+ *
+ * This function is invoked when the following command is made:
+ * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
+ * It returns for which lane the counter configurations are done
+ */
+static ssize_t ras_event_counter_lane_sel_read(struct file *file,
+					       char __user *buf, size_t count,
+					       loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	set_event_number(file);
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+			RAS_DES_EVENT_COUNTER_CTRL_REG);
+	val = (val >> LANE_SELECT_SHIFT) & LANE_SELECT_MASK;
+	sprintf(debugfs_buf, "0x%x\n", val);
+	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
+				      strlen(debugfs_buf));
+
+	return ret;
+}
+
+/*
+ * ras_event_counter_value_read: Function to get counter value
+ *
+ * This function is invoked when the following command is made:
+ * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_value
+ * It returns the number of time the selected event has happened if enabled
+ */
+
+static ssize_t ras_event_counter_value_read(struct file *file, char __user *buf,
+					    size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	set_event_number(file);
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+				RAS_DES_EVENT_COUNTER_DATA_REG);
+	sprintf(debugfs_buf, "0x%x\n", val);
+	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
+				      strlen(debugfs_buf));
+
+	return ret;
+}
+
+/*
+ * ras_event_counter_en_write: Function to set if counter is enable
+ *
+ * This function is invoked when the following command is made:
+ * echo n > /sys/kernel/debug/dwc_pcie_plat/
+ *		ras_des_counter/<name>/counter_enable
+ * Here n can be 1 to enable and 0 to disable
+ */
+static ssize_t ras_event_counter_en_write(struct file *file,
+					  const char __user *buf,
+					  size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	u32 enable;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	ret = kstrtou32_from_user(buf, count, 0, &enable);
+	if (ret)
+		return ret;
+
+	set_event_number(file);
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	if (enable)
+		val |= PER_EVENT_ON;
+	else
+		val |= PER_EVENT_OFF;
+
+	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
+			   RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+
+	return count;
+}
+
+/*
+ * ras_event_counter_lane_sel_write: Function to set lane number
+ *
+ * This function is invoked when the following command is made:
+ * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
+ * Here n is the lane that we want to select for counter configuration
+ */
+static ssize_t ras_event_counter_lane_sel_write(struct file *file,
+						const char __user *buf,
+						size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	u32 lane;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	ret = kstrtou32_from_user(buf, count, 0, &lane);
+	if (ret)
+		return ret;
+
+	set_event_number(file);
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+				RAS_DES_EVENT_COUNTER_CTRL_REG);
+	val &= ~(LANE_SELECT_MASK << LANE_SELECT_SHIFT);
+	val |= (lane << LANE_SELECT_SHIFT);
+	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
+			   RAS_DES_EVENT_COUNTER_CTRL_REG, val);
+
+	return count;
+}
+
+/*
+ * ras_error_inj_read: Function to read number of errors left to be injected
+ *
+ * This function is invoked when the following command is made:
+ * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
+ * This returns the number of errors left to be injected which will
+ * keep reducing as we make pcie transactions to inject error.
+ */
+static ssize_t ras_error_inj_read(struct file *file, char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val, err_num, inj_num;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	err_num = get_error_inj_number(file);
+	inj_num = (err_num & 0xFF);
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
+				(0x4 * inj_num));
+	sprintf(debugfs_buf, "0x%x\n", (val & EINJ_COUNT_MASK));
+	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
+				      strlen(debugfs_buf));
+
+	return ret;
+}
+
+/*
+ * ras_error_inj_write: Function to set number of errors to be injected
+ *
+ * This function is invoked when the following command is made:
+ * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
+ * Here n is the number of errors we want to inject
+ */
+static ssize_t ras_error_inj_write(struct file *file, const char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val, err_num, inj_num;
+	u32 counter;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	if (count > DEBUGFS_BUF_MAX_SIZE)
+		return -EINVAL;
+
+	ret = kstrtou32_from_user(buf, count, 0, &counter);
+	if (ret)
+		return ret;
+
+	err_num = get_error_inj_number(file);
+	inj_num = (err_num & 0xFF);
+	err_num = (err_num >> 8);
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
+				(0x4 * inj_num));
+	val &= ~(EINJ_TYPE_MASK << EINJ_TYPE_SHIFT);
+	val |= (err_num << EINJ_TYPE_SHIFT);
+	val &= ~(EINJ_COUNT_MASK);
+	val |= counter;
+	dw_pcie_writel_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
+			   (0x4 * inj_num), val);
+	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
+			   ERR_INJ_ENABLE_REG, (0x1 << inj_num));
+
+	return count;
+}
+
+static ssize_t lane_detection_read(struct file *file, char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+				SD_STATUS_L1LANE_REG);
+	val = (val >> LANE_DETECT_SHIFT) & LANE_DETECT_MASK;
+	sprintf(debugfs_buf, "0x%x\n", val);
+
+	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
+				      strlen(debugfs_buf));
+
+	return ret;
+}
+
+static ssize_t rx_valid_read(struct file *file, char __user *buf,
+			     size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+				SD_STATUS_L1LANE_REG);
+	val = (val >> PIPE_RXVALID_SHIFT) & PIPE_RXVALID_MASK;
+	sprintf(debugfs_buf, "0x%x\n", val);
+
+	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
+				      strlen(debugfs_buf));
+
+	return ret;
+}
+
+static ssize_t lane_selection_write(struct file *file, const char __user *buf,
+				    size_t count, loff_t *ppos)
+{
+	u32 ret;
+	u32 val;
+	u32 lane;
+	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
+
+	if (count > DEBUGFS_BUF_MAX_SIZE)
+		return -EINVAL;
+
+	ret = kstrtou32_from_user(buf, count, 0, &lane);
+	if (ret)
+		return ret;
+
+	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
+				SD_STATUS_L1LANE_REG);
+	val &= ~(LANE_SELECT_MASK);
+	val |= lane;
+	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
+			   SD_STATUS_L1LANE_REG, val);
+
+	return count;
+}
+
+static const struct file_operations lane_detection_fops = {
+	.open = open,
+	.read = lane_detection_read,
+	.write = lane_selection_write
+};
+
+static const struct file_operations rx_valid_fops = {
+	.open = open,
+	.read = rx_valid_read,
+	.write = lane_selection_write
+};
+
+static const struct file_operations cnt_en_ops = {
+	.read = ras_event_counter_en_read,
+	.write = ras_event_counter_en_write,
+	.open = simple_open,
+};
+
+static const struct file_operations lane_sel_ops = {
+	.read = ras_event_counter_lane_sel_read,
+	.write = ras_event_counter_lane_sel_write,
+	.open = simple_open,
+};
+
+static const struct file_operations cnt_val_ops = {
+	.read = ras_event_counter_value_read,
+	.open = simple_open,
+};
+
+static const struct file_operations inj_ops = {
+	.read = ras_error_inj_read,
+	.write = ras_error_inj_write,
+	.open = simple_open,
+};
+
+int create_debugfs_files(struct dw_pcie *pci)
+{
+	int ret = 0;
+	char dirname[32];
+	struct device *dev;
+
+	struct dentry *ras_des_debug_regs;
+	struct dentry *ras_des_error_inj;
+	struct dentry *ras_des_event_counter;
+	struct dentry *lane_detection;
+	struct dentry *rx_valid;
+
+	if (!pci) {
+		pr_err("pcie struct is NULL\n");
+		return -ENODEV;
+	}
+
+	dev = pci->dev;
+	sprintf(dirname, "pcie_dwc_%s", dev_name(dev));
+
+	pci->ras_cap_offset = dw_pcie_find_vsec_capability(pci,
+							   DW_PCIE_RAS_CAP_ID);
+	if (!pci->ras_cap_offset) {
+		pr_err("No RAS capability available\n");
+		return -ENODEV;
+	}
+
+	/* Create main directory for each platform driver */
+	dir = debugfs_create_dir(dirname, NULL);
+	if (dir == NULL) {
+		pr_err("error creating directory: %s\n", dirname);
+		return -ENODEV;
+	}
+
+	/* Create sub dirs for Debug, Error injection, Statistics */
+	ras_des_debug_regs = debugfs_create_dir("ras_des_debug_regs", dir);
+	if (ras_des_debug_regs == NULL) {
+		pr_err("error creating directory: %s\n", dirname);
+		ret = -ENODEV;
+		goto remove_debug_file;
+	}
+
+	ras_des_error_inj = debugfs_create_dir("ras_des_error_inj", dir);
+	if (ras_des_error_inj == NULL) {
+		pr_err("error creating directory: %s\n", dirname);
+		ret = -ENODEV;
+		goto remove_debug_file;
+	}
+
+	ras_des_event_counter = debugfs_create_dir("ras_des_counter", dir);
+	if (ras_des_event_counter == NULL) {
+		pr_err("error creating directory: %s\n", dirname);
+		ret = -ENODEV;
+		goto remove_debug_file;
+	}
+
+	/* Create debugfs files for Debug subdirectory */
+	lane_detection = debugfs_create_file("lane_detection", 0644,
+					     ras_des_debug_regs, pci,
+					     &lane_detection_fops);
+
+	rx_valid = debugfs_create_file("rx_valid", 0644,
+					     ras_des_debug_regs, pci,
+					     &lane_detection_fops);
+
+	/* Create debugfs files for Error injection sub dir */
+	CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_ecrc);
+	CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_ecrc);
+	CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_lcrc);
+	CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_lcrc);
+
+	/* Create debugfs files for Statistical counter sub dir */
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_overflow);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_underrun);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(decode_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(receiver_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(framing_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(lcrc_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(ecrc_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(unsupp_req_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltr_abort_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltn_timeout_error);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(tx_l0s_entry);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(rx_l0s_entry);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_entry);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_1_entry);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_2_entry);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l2_entry);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(speed_change);
+	CREATE_RAS_EVENT_COUNTER_DEBUGFS(width_chage);
+
+	return ret;
+
+remove_debug_file:
+	remove_debugfs_files();
+	return ret;
+}
+
+void remove_debugfs_files(void)
+{
+	debugfs_remove_recursive(dir);
+}
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..3dc3cf696a04
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
@@ -0,0 +1,133 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Synopsys DesignWare PCIe controller debugfs driver
+ *
+ * Copyright (C) 2021 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * Author: Shradha Todi <shradha.t@samsung.com>
+ */
+
+#ifndef _PCIE_DESIGNWARE_DEBUGFS_H
+#define _PCIE_DESIGNWARE_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			0xFFFFFF
+#define EINJ_TYPE_SHIFT			8
+
+enum event_numbers {
+	ebuf_overflow		= 0x0,
+	ebuf_underrun		= 0x1,
+	decode_error		= 0x2,
+	sync_header_error	= 0x5,
+	receiver_error		= 0x106,
+	framing_error		= 0x109,
+	lcrc_error		= 0x201,
+	ecrc_error		= 0x302,
+	unsupp_req_error	= 0x303,
+	cmpltr_abort_error	= 0x304,
+	cmpltn_timeout_error	= 0x305,
+	tx_l0s_entry		= 0x502,
+	rx_l0s_entry		= 0x503,
+	l1_entry		= 0x505,
+	l1_1_entry		= 0x507,
+	l1_2_entry		= 0x508,
+	l2_entry		= 0x50B,
+	speed_change		= 0x50C,
+	width_chage		= 0x50D,
+};
+
+/*
+ * struct event_counters: Struct to store event number
+ *
+ * @name: name of the event counter
+ *        eg: ecrc_err count, l1 entry count
+ * @event_num: Event number and group number
+ * [16:8]: Group number
+ * [7:0]: Event number
+ */
+struct event_counters {
+	const char *name;
+	u32 event_num;
+};
+
+enum error_inj_code {
+	tx_lcrc			= 0x000,
+	tx_ecrc			= 0x300,
+	rx_lcrc			= 0x800,
+	rx_ecrc			= 0xB00,
+};
+
+/*
+ * struct error_injectionss: Struct to store error numbers
+ *
+ * @name: name of the error to be injected
+ *        eg: ecrc_err, lcrc_err
+ * @event_num: Error number and group number
+ * [31:8]: Error type. This should be same as bits [31:8]
+ *         in the EINJn_REG where n is group number
+ * [7:0]: Error injection group
+ *        0 - CRC
+ *        1 - seq num
+ *        2 - DLLP error
+ *        3 - symbol error
+ *        4 - FC error
+ */
+struct error_injections {
+	const char *name;
+	u32 type_of_err;
+};
+
+#define CREATE_RAS_EVENT_COUNTER_DEBUGFS(name)				\
+do {									\
+	sub_dir = debugfs_create_dir(#name, ras_des_event_counter);	\
+	debugfs_create_file("counter_enable", 0644, sub_dir, pci,	\
+				&cnt_en_ops);				\
+	debugfs_create_file("lane_select", 0644, sub_dir, pci,		\
+				&lane_sel_ops);				\
+	debugfs_create_file("counter_value", 0444, sub_dir, pci,	\
+				&cnt_val_ops);				\
+} while (0)
+
+#define CREATE_RAS_ERROR_INJECTION_DEBUGFS(name)			\
+	debugfs_create_file(#name, 0644, ras_des_error_inj, pci,	\
+				&inj_ops);
+
+#ifdef CONFIG_PCIE_DW_DEBUGFS
+int create_debugfs_files(struct dw_pcie *pci);
+void remove_debugfs_files(void);
+#else
+int create_debugfs_files(struct dw_pcie *pci)
+{
+	/* No OP */
+	return 0;
+}
+
+void remove_debugfs_files(void)
+{
+	/* No OP */
+}
+#endif
+
+#endif
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 307525aaa063..031fdafe0a3e 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -278,6 +278,7 @@ struct dw_pcie {
 	u8			n_fts[2];
 	bool			iatu_unroll_enabled: 1;
 	bool			io_cfg_atu_shared: 1;
+	u32			ras_cap_offset;
 };
 
 #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
-- 
2.17.1

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

[Shradha Todi]

Add call to create_debugfs_files() from DWC driver to create the RASDES
debugfs structure for each platform driver. Since it can be used for both
DW HOST controller as well as DW EP controller, let's add it in the common
setup function.

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

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 348f6f696976..c054f8ba1cf4 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -15,6 +15,7 @@
 
 #include "../../pci.h"
 #include "pcie-designware.h"
+#include "pcie-designware-debugfs.h"
 
 /*
  * These interfaces resemble the pci_find_*capability() interfaces, but these
@@ -793,4 +794,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
 		       PCIE_PL_CHK_REG_CHK_REG_START;
 		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
 	}
+
+	ret = create_debugfs_files(pci);
+	if (ret)
+		dev_err(pci->dev, "Couldn't create debugfs files\n");
 }
-- 
2.17.1

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

[Bjorn Helgaas]

On Tue, May 18, 2021 at 11:16:17PM +0530, Shradha Todi wrote:
> RAS is a vendor specific extended PCIe capability which which helps to read
> hardware internal state.
> 
> This framework support provides debugfs entries to use these features for
> DesignWare controller. Following primary features of DesignWare controllers
> 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            |   9 +
>  drivers/pci/controller/dwc/Makefile           |   1 +
>  .../controller/dwc/pcie-designware-debugfs.c  | 544 ++++++++++++++++++
>  .../controller/dwc/pcie-designware-debugfs.h  | 133 +++++
>  drivers/pci/controller/dwc/pcie-designware.h  |   1 +
>  5 files changed, 688 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 423d35872ce4..2d0a18cb9418 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -6,6 +6,15 @@ menu "DesignWare PCI Core Support"
>  config PCIE_DW
>  	bool
>  
> +config PCIE_DW_DEBUGFS
> +	bool "DWC PCIe debugfs entries"
> +	default n
> +	help
> +	  Enables debugfs entries for the DWC PCIe Controller.
> +	  These entries make use of the RAS DES features in the DW
> +	  controller to help in debug, error injection and statistical
> +	  counters

Maybe omit "DES"?  The rest of the sentence spells it out, but the
connection isn't completely obvious, and I don't think the first "DES"
adds anything.

> +
>  config PCIE_DW_HOST
>  	bool
>  	depends on PCI_MSI_IRQ_DOMAIN
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index eca805c1a023..6ad12638f793 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..84bf196df240
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -0,0 +1,544 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@samsung.com>
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "pcie-designware.h"
> +#include "pcie-designware-debugfs.h"
> +
> +#define DEBUGFS_BUF_MAX_SIZE	100
> +
> +static char debugfs_buf[DEBUGFS_BUF_MAX_SIZE];

Should not be static, file scope.  More comments below.

> +static struct dentry *dir;
> +static struct dentry *sub_dir;

These should not be static, file scope either.  I think "dir" should
be returned by create_debugfs_files(), and the caller should hang onto
it until it needs it for remove_debugfs_files().

"sub_dir" looks like it's only needed inside create_debugfs_files(),
so it should be a local variable there.

> +static struct event_counters ras_events[] = {
> +	{"ebuf_overflow",		ebuf_overflow},
> +	{"ebuf_underrun",		ebuf_underrun},
> +	{"decode_error",		decode_error},
> +	{"sync_header_error",		sync_header_error},
> +	{"receiver_error",		receiver_error},
> +	{"framing_error",		framing_error},
> +	{"lcrc_error",			lcrc_error},
> +	{"ecrc_error",			ecrc_error},
> +	{"unsupp_req_error",		unsupp_req_error},
> +	{"cmpltr_abort_error",		cmpltr_abort_error},
> +	{"cmpltn_timeout_error",	cmpltn_timeout_error},
> +	{"tx_l0s_entry",		tx_l0s_entry},
> +	{"rx_l0s_entry",		rx_l0s_entry},
> +	{"l1_entry",			l1_entry},
> +	{"l1_1_entry",			l1_1_entry},
> +	{"l1_2_entry",			l1_2_entry},
> +	{"l2_entry",			l2_entry},
> +	{"speed_change",		speed_change},
> +	{"width_chage",			width_chage},
> +	{0, 0},
> +};
> +
> +static struct error_injections error_list[] = {
> +	{"tx_lcrc",		tx_lcrc},
> +	{"tx_ecrc",		tx_ecrc},
> +	{"rx_lcrc",		rx_lcrc},
> +	{"rx_ecrc",		rx_ecrc},
> +	{0, 0},
> +};
> +
> +static int open(struct inode *inode, struct file *file)
> +{
> +	file->private_data = inode->i_private;
> +
> +	return 0;
> +}
> +
> +/*
> + * set_event_number: Function to set event number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all event counters. Using the debugfs filname, the
> + * group number and event number for the counter is extracted and
> + * then programmed into the control register.

You can omit the "Function to" text here (more occurrences below).

You can also omit the "This function is " text.  It's obvious that
this comment applies to this function.

> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: void
> + */
> +static void set_event_number(struct file *file)
> +{
> +	int i;
> +	u32 val;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +	u32 max_size = sizeof(ras_events) / sizeof(struct event_counters);

Reorder the local decls here and below like this:

	struct dw_pcie *pci = file->private_data;
	int i;
	u32 val;

so things are in the order they're used.  It's fairly conventional to
look up things based on incoming parameters first, e.g., the
"struct dw_pcie *pci = file->private_data;" (no cast needed there).

> +	for (i = 0; i < max_size; i++) {

Drop "max_size" and use this instead:

	for (i = 0; i < ARRAY_SIZE(ras_events); i++) {

> +		if (strcmp(ras_events[i].name,
> +			   file->f_path.dentry->d_parent->d_iname) == 0) {

There are *very* few uses of d_iname in the tree, which makes me think
you're doing something wrong here.  I don't have a suggestion yet, but
maybe take a look at other debugfs users to see if there's a common
pattern you can copy?

> +			val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +					RAS_DES_EVENT_COUNTER_CTRL_REG);
> +			val &= ~(EVENT_COUNTER_ENABLE);

Unnecessary parentheses.

> +			val &= ~(0xFFF << 16);
> +			val |= (ras_events[i].event_num << 16);
> +			dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +					RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +			break;
> +		}
> +	}
> +}

Add blank line here.

> +/*
> + * get_error_inj_number: Function to get error number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all error injection debugfs entries. Using the debugfs
> + * filname, the error group and type of error to be injected is extracted.
> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: u32
> + * [31:8]: Type of error to be injected
> + * [7:0]: Group of error it belongs to
> + */
> +

Remove spurious blank line above.

> +static u32 get_error_inj_number(struct file *file)
> +{
> +	int i;
> +	u32 max_size = sizeof(error_list) / sizeof(struct error_injections);

Drop "max_size" and use ARRAY_SIZE() again.

> +	for (i = 0; i < max_size; i++) {
> +		if (strcmp(error_list[i].name,
> +			   file->f_path.dentry->d_iname) == 0) {
> +			return error_list[i].type_of_err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * ras_event_counter_en_read: Function to get if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_enable
> + * It returns whether the counter is enabled or not
> + */
> +static ssize_t ras_event_counter_en_read(struct file *file, char __user *buf,
> +					 size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;

Unnecessary cast, reorder decls (more occurrences below).

> +	set_event_number(file);
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +				RAS_DES_EVENT_COUNTER_CTRL_REG);

This *looks* like it might require mutual exclusion around the
"set_event_number(); dw_pcie_readl_dbi();" pair.  Does
set_event_number() write a register that determines which counter
dw_pcie_readl_dbi() will read?  If so, that should be made atomic
somehow.

> +	val = (val >> EVENT_COUNTER_STATUS_SHIFT) & EVENT_COUNTER_STATUS_MASK;
> +	if (val)
> +		sprintf(debugfs_buf, "Enabled\n");
> +	else
> +		sprintf(debugfs_buf, "Disabled\n");
> +
> +	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +				      strlen(debugfs_buf));

I don't feel confident about this because:

  - "debugfs_buf" is static and shared among all these users without
    any obvious mutual exclusion, and

  - sprintf() is a notorious problem for buffer overflows.  This isn't
    a problem here, but it does require extra effort to validate.  I
    think it would be better to use scnprintf(), which also has the
    benefit of returning the value you need to pass to
    simple_read_from_buffer(), so you don't need the strlen().
    drivers/ntb/test/ntb_tool.c has lots of examples you could copy.

I don't *think* you have any buffer overflow issues in this file, but
you also don't seem to ever print more than a dozen or so characters.
Using buffers on the stack would be simple and make it obvious that
there are no mutual exclusion issues.

> +
> +	return ret;

	return simple_read_from_buffer(...);

and drop "ret" (also more occurrences below).

> +}
> +
> +/*
> + * ras_event_counter_lane_sel_read: Function to get lane number selected
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
> + * It returns for which lane the counter configurations are done
> + */
> +static ssize_t ras_event_counter_lane_sel_read(struct file *file,
> +					       char __user *buf, size_t count,
> +					       loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	set_event_number(file);
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +			RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	val = (val >> LANE_SELECT_SHIFT) & LANE_SELECT_MASK;
> +	sprintf(debugfs_buf, "0x%x\n", val);

I always use "%#x" because it's a tiny bit shorter and avoids the
possibility of typos like "0X%x" or "0x%X", which would produce
"0Xabcd" or "0xABCD".  Weird personal idiosyncracy :)

> +	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +				      strlen(debugfs_buf));
> +
> +	return ret;
> +}
> +
> +/*
> + * ras_event_counter_value_read: Function to get counter value
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_value
> + * It returns the number of time the selected event has happened if enabled
> + */
> +

Remove spurious blank line above.

> +static ssize_t ras_event_counter_value_read(struct file *file, char __user *buf,
> +					    size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	set_event_number(file);
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +				RAS_DES_EVENT_COUNTER_DATA_REG);
> +	sprintf(debugfs_buf, "0x%x\n", val);
> +	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +				      strlen(debugfs_buf));
> +
> +	return ret;
> +}
> +
> +/*
> + * ras_event_counter_en_write: Function to set if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/
> + *		ras_des_counter/<name>/counter_enable
> + * Here n can be 1 to enable and 0 to disable
> + */
> +static ssize_t ras_event_counter_en_write(struct file *file,
> +					  const char __user *buf,
> +					  size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	u32 enable;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	ret = kstrtou32_from_user(buf, count, 0, &enable);
> +	if (ret)
> +		return ret;
> +
> +	set_event_number(file);
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +				RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	if (enable)
> +		val |= PER_EVENT_ON;
> +	else
> +		val |= PER_EVENT_OFF;
> +
> +	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +			   RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +
> +	return count;
> +}
> +
> +/*
> + * ras_event_counter_lane_sel_write: Function to set lane number
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
> + * Here n is the lane that we want to select for counter configuration
> + */
> +static ssize_t ras_event_counter_lane_sel_write(struct file *file,
> +						const char __user *buf,
> +						size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	u32 lane;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	ret = kstrtou32_from_user(buf, count, 0, &lane);
> +	if (ret)
> +		return ret;
> +
> +	set_event_number(file);
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +				RAS_DES_EVENT_COUNTER_CTRL_REG);
> +	val &= ~(LANE_SELECT_MASK << LANE_SELECT_SHIFT);
> +	val |= (lane << LANE_SELECT_SHIFT);
> +	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +			   RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +
> +	return count;
> +}
> +
> +/*
> + * ras_error_inj_read: Function to read number of errors left to be injected
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
> + * This returns the number of errors left to be injected which will
> + * keep reducing as we make pcie transactions to inject error.

Use "PCIe" in text.

> + */
> +static ssize_t ras_error_inj_read(struct file *file, char __user *buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val, err_num, inj_num;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	err_num = get_error_inj_number(file);
> +	inj_num = (err_num & 0xFF);
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> +				(0x4 * inj_num));
> +	sprintf(debugfs_buf, "0x%x\n", (val & EINJ_COUNT_MASK));
> +	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +				      strlen(debugfs_buf));
> +
> +	return ret;
> +}
> +
> +/*
> + * ras_error_inj_write: Function to set number of errors to be injected
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
> + * Here n is the number of errors we want to inject
> + */
> +static ssize_t ras_error_inj_write(struct file *file, const char __user *buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val, err_num, inj_num;
> +	u32 counter;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	if (count > DEBUGFS_BUF_MAX_SIZE)
> +		return -EINVAL;

What is the point of this check?  "debugfs_buf" isn't involved here.
Same question for the similar test below.

> +	ret = kstrtou32_from_user(buf, count, 0, &counter);
> +	if (ret)
> +		return ret;
> +
> +	err_num = get_error_inj_number(file);
> +	inj_num = (err_num & 0xFF);
> +	err_num = (err_num >> 8);
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> +				(0x4 * inj_num));
> +	val &= ~(EINJ_TYPE_MASK << EINJ_TYPE_SHIFT);
> +	val |= (err_num << EINJ_TYPE_SHIFT);
> +	val &= ~(EINJ_COUNT_MASK);
> +	val |= counter;
> +	dw_pcie_writel_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> +			   (0x4 * inj_num), val);
> +	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +			   ERR_INJ_ENABLE_REG, (0x1 << inj_num));
> +
> +	return count;
> +}
> +
> +static ssize_t lane_detection_read(struct file *file, char __user *buf,
> +				   size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +				SD_STATUS_L1LANE_REG);
> +	val = (val >> LANE_DETECT_SHIFT) & LANE_DETECT_MASK;
> +	sprintf(debugfs_buf, "0x%x\n", val);
> +
> +	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +				      strlen(debugfs_buf));
> +
> +	return ret;
> +}
> +
> +static ssize_t rx_valid_read(struct file *file, char __user *buf,
> +			     size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +				SD_STATUS_L1LANE_REG);
> +	val = (val >> PIPE_RXVALID_SHIFT) & PIPE_RXVALID_MASK;
> +	sprintf(debugfs_buf, "0x%x\n", val);
> +
> +	ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +				      strlen(debugfs_buf));
> +
> +	return ret;
> +}
> +
> +static ssize_t lane_selection_write(struct file *file, const char __user *buf,
> +				    size_t count, loff_t *ppos)
> +{
> +	u32 ret;
> +	u32 val;
> +	u32 lane;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	if (count > DEBUGFS_BUF_MAX_SIZE)
> +		return -EINVAL;
> +
> +	ret = kstrtou32_from_user(buf, count, 0, &lane);
> +	if (ret)
> +		return ret;
> +
> +	val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +				SD_STATUS_L1LANE_REG);
> +	val &= ~(LANE_SELECT_MASK);

Unnecessary parentheses.  If needed, they should be in the
LANE_SELECT_MASK definition.

> +	val |= lane;
> +	dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +			   SD_STATUS_L1LANE_REG, val);
> +
> +	return count;
> +}
> +
> +static const struct file_operations lane_detection_fops = {
> +	.open = open,
> +	.read = lane_detection_read,
> +	.write = lane_selection_write
> +};
> +
> +static const struct file_operations rx_valid_fops = {
> +	.open = open,
> +	.read = rx_valid_read,
> +	.write = lane_selection_write
> +};
> +
> +static const struct file_operations cnt_en_ops = {
> +	.read = ras_event_counter_en_read,
> +	.write = ras_event_counter_en_write,
> +	.open = simple_open,
> +};
> +
> +static const struct file_operations lane_sel_ops = {
> +	.read = ras_event_counter_lane_sel_read,
> +	.write = ras_event_counter_lane_sel_write,
> +	.open = simple_open,
> +};
> +
> +static const struct file_operations cnt_val_ops = {
> +	.read = ras_event_counter_value_read,
> +	.open = simple_open,
> +};
> +
> +static const struct file_operations inj_ops = {
> +	.read = ras_error_inj_read,
> +	.write = ras_error_inj_write,
> +	.open = simple_open,
> +};
> +
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> +	int ret = 0;
> +	char dirname[32];
> +	struct device *dev;
> +

Remove spurious blank line above.

> +	struct dentry *ras_des_debug_regs;
> +	struct dentry *ras_des_error_inj;
> +	struct dentry *ras_des_event_counter;
> +	struct dentry *lane_detection;
> +	struct dentry *rx_valid;
> +
> +	if (!pci) {
> +		pr_err("pcie struct is NULL\n");
> +		return -ENODEV;
> +	}
> +
> +	dev = pci->dev;
> +	sprintf(dirname, "pcie_dwc_%s", dev_name(dev));

scnprintf() to prevent buffer overflow.

> +	pci->ras_cap_offset = dw_pcie_find_vsec_capability(pci,
> +							   DW_PCIE_RAS_CAP_ID);
> +	if (!pci->ras_cap_offset) {
> +		pr_err("No RAS capability available\n");

Use dev_err() here and below to connect the message with the device.

> +		return -ENODEV;
> +	}
> +
> +	/* Create main directory for each platform driver */
> +	dir = debugfs_create_dir(dirname, NULL);
> +	if (dir == NULL) {

"if (!dir)" is typical style for NULL pointer checking.

> +		pr_err("error creating directory: %s\n", dirname);
> +		return -ENODEV;
> +	}
> +
> +	/* Create sub dirs for Debug, Error injection, Statistics */
> +	ras_des_debug_regs = debugfs_create_dir("ras_des_debug_regs", dir);
> +	if (ras_des_debug_regs == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}
> +
> +	ras_des_error_inj = debugfs_create_dir("ras_des_error_inj", dir);
> +	if (ras_des_error_inj == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}
> +
> +	ras_des_event_counter = debugfs_create_dir("ras_des_counter", dir);
> +	if (ras_des_event_counter == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}
> +
> +	/* Create debugfs files for Debug subdirectory */
> +	lane_detection = debugfs_create_file("lane_detection", 0644,
> +					     ras_des_debug_regs, pci,
> +					     &lane_detection_fops);
> +
> +	rx_valid = debugfs_create_file("rx_valid", 0644,
> +					     ras_des_debug_regs, pci,
> +					     &lane_detection_fops);
> +
> +	/* Create debugfs files for Error injection sub dir */
> +	CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_ecrc);
> +	CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_ecrc);
> +	CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_lcrc);
> +	CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_lcrc);
> +
> +	/* Create debugfs files for Statistical counter sub dir */
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_overflow);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_underrun);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(decode_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(receiver_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(framing_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(lcrc_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(ecrc_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(unsupp_req_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltr_abort_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltn_timeout_error);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(tx_l0s_entry);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(rx_l0s_entry);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_entry);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_1_entry);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_2_entry);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(l2_entry);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(speed_change);
> +	CREATE_RAS_EVENT_COUNTER_DEBUGFS(width_chage);
> +
> +	return ret;
> +
> +remove_debug_file:
> +	remove_debugfs_files();
> +	return ret;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> +	debugfs_remove_recursive(dir);
> +}
> diff --git a/drivers/pci/controller/dwc/pcie-designware-debugfs.h b/drivers/pci/controller/dwc/pcie-designware-debugfs.h

This new file is only included from one place.  Why not just put the
contents there?

> new file mode 100644
> index 000000000000..3dc3cf696a04
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> @@ -0,0 +1,133 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@samsung.com>
> + */
> +
> +#ifndef _PCIE_DESIGNWARE_DEBUGFS_H
> +#define _PCIE_DESIGNWARE_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			0xFFFFFF
> +#define EINJ_TYPE_SHIFT			8
> +
> +enum event_numbers {
> +	ebuf_overflow		= 0x0,
> +	ebuf_underrun		= 0x1,
> +	decode_error		= 0x2,
> +	sync_header_error	= 0x5,
> +	receiver_error		= 0x106,
> +	framing_error		= 0x109,
> +	lcrc_error		= 0x201,
> +	ecrc_error		= 0x302,
> +	unsupp_req_error	= 0x303,
> +	cmpltr_abort_error	= 0x304,
> +	cmpltn_timeout_error	= 0x305,
> +	tx_l0s_entry		= 0x502,
> +	rx_l0s_entry		= 0x503,
> +	l1_entry		= 0x505,
> +	l1_1_entry		= 0x507,
> +	l1_2_entry		= 0x508,
> +	l2_entry		= 0x50B,
> +	speed_change		= 0x50C,
> +	width_chage		= 0x50D,
> +};
> +
> +/*
> + * struct event_counters: Struct to store event number
> + *
> + * @name: name of the event counter
> + *        eg: ecrc_err count, l1 entry count
> + * @event_num: Event number and group number
> + * [16:8]: Group number
> + * [7:0]: Event number
> + */
> +struct event_counters {
> +	const char *name;
> +	u32 event_num;
> +};
> +
> +enum error_inj_code {
> +	tx_lcrc			= 0x000,
> +	tx_ecrc			= 0x300,
> +	rx_lcrc			= 0x800,
> +	rx_ecrc			= 0xB00,
> +};
> +
> +/*
> + * struct error_injectionss: Struct to store error numbers

s/error_injectionss/error_injections/

> + *
> + * @name: name of the error to be injected
> + *        eg: ecrc_err, lcrc_err
> + * @event_num: Error number and group number
> + * [31:8]: Error type. This should be same as bits [31:8]
> + *         in the EINJn_REG where n is group number
> + * [7:0]: Error injection group
> + *        0 - CRC
> + *        1 - seq num
> + *        2 - DLLP error
> + *        3 - symbol error
> + *        4 - FC error
> + */
> +struct error_injections {
> +	const char *name;
> +	u32 type_of_err;
> +};
> +
> +#define CREATE_RAS_EVENT_COUNTER_DEBUGFS(name)				\
> +do {									\
> +	sub_dir = debugfs_create_dir(#name, ras_des_event_counter);	\
> +	debugfs_create_file("counter_enable", 0644, sub_dir, pci,	\
> +				&cnt_en_ops);				\
> +	debugfs_create_file("lane_select", 0644, sub_dir, pci,		\
> +				&lane_sel_ops);				\
> +	debugfs_create_file("counter_value", 0444, sub_dir, pci,	\
> +				&cnt_val_ops);				\
> +} while (0)
> +
> +#define CREATE_RAS_ERROR_INJECTION_DEBUGFS(name)			\
> +	debugfs_create_file(#name, 0644, ras_des_error_inj, pci,	\
> +				&inj_ops);
> +
> +#ifdef CONFIG_PCIE_DW_DEBUGFS
> +int create_debugfs_files(struct dw_pcie *pci);
> +void remove_debugfs_files(void);
> +#else
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> +	/* No OP */
> +	return 0;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> +	/* No OP */
> +}
> +#endif
> +
> +#endif
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 307525aaa063..031fdafe0a3e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -278,6 +278,7 @@ struct dw_pcie {
>  	u8			n_fts[2];
>  	bool			iatu_unroll_enabled: 1;
>  	bool			io_cfg_atu_shared: 1;
> +	u32			ras_cap_offset;

u16 should be sufficient, since that's what
dw_pcie_find_vsec_capability() returns.

I would just call it "ras" with a comment here about it being a "VSEC
RAS capability".  But that's just a personal preference.

>  };
>  
>  #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> -- 
> 2.17.1
> 

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

[Bjorn Helgaas]

On Tue, May 18, 2021 at 11:16:18PM +0530, Shradha Todi wrote:
> Add call to create_debugfs_files() from DWC driver to create the RASDES
> debugfs structure for each platform driver. Since it can be used for both
> DW HOST controller as well as DW EP controller, let's add it in the common
> setup function.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 348f6f696976..c054f8ba1cf4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -15,6 +15,7 @@
>  
>  #include "../../pci.h"
>  #include "pcie-designware.h"
> +#include "pcie-designware-debugfs.h"
>  
>  /*
>   * These interfaces resemble the pci_find_*capability() interfaces, but these
> @@ -793,4 +794,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  		       PCIE_PL_CHK_REG_CHK_REG_START;
>  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
>  	}
> +
> +	ret = create_debugfs_files(pci);
> +	if (ret)
> +		dev_err(pci->dev, "Couldn't create debugfs files\n");

Was there supposed to be a corresponding remove_debugfs_files() call?
On module unload?

>  }
> -- 
> 2.17.1
> 

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

[Krzysztof Wilczyński]

Hi Shradha,

Thank you for this working on this!  Looks very nice!

A few suggestions below.

> +config PCIE_DW_DEBUGFS
> +	bool "DWC PCIe debugfs entries"
> +	default n

No need to add this default for "no" answer, and this is the Kconfig's
default, so you can omit it here.

> +static int open(struct inode *inode, struct file *file)
> +{
> +	file->private_data = inode->i_private;
> +
> +	return 0;
> +}

Why custom callback?  This seem to almost copy what simple_open() does,
as per:

  int simple_open(struct inode *inode, struct file *file)
  {
  	if (inode->i_private)
  		file->private_data = inode->i_private;
  	return 0;
  }

You seem to be using simple_open() everywhere else.

> + * set_event_number: Function to set event number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all event counters. Using the debugfs filname, the
> + * group number and event number for the counter is extracted and
> + * then programmed into the control register.
> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: void
> + */

About this and other functions comments - did you intend to format this
and the others as kernel-doc compliant? At the first glance it does look
very similar, as per:

  https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

[...]
> +/*
> + * ras_event_counter_en_read: Function to get if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_enable

You don't really need to explain when the read() function evokes.  This
is also true for other such comments.

> + * It returns whether the counter is enabled or not

Nit pick: missing period at the end of the sentence (also true in other
code comments and sentences throughout).

[...]
> +	u32 ret;
> +	u32 val;
> +	u32 lane;
> +	struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +	ret = kstrtou32_from_user(buf, count, 0, &lane);
> +	if (ret)
> +		return ret;
> +
> +	set_event_number(file);

You could add a newline here so that this is easier to read and also
consistent with other functions.

[...]
> +	err_num = get_error_inj_number(file);
> +	inj_num = (err_num & 0xFF);

Surplus parenthesis above.  This could also be moved to the top where
you declare all the variables.

[...]
> +	err_num = get_error_inj_number(file);
> +	inj_num = (err_num & 0xFF);
> +	err_num = (err_num >> 8);

Surplus parenthesis here too.  Also, you could probably move some of
these to the top, if possible.

[...]
> +static const struct file_operations rx_valid_fops = {
> +	.open = open,
> +	.read = rx_valid_read,
> +	.write = lane_selection_write

Just to make sure - this "lane_selection_write" here is intended?

[...]
> +	if (!pci) {
> +		pr_err("pcie struct is NULL\n");
> +		return -ENODEV;
> +	}

I think, given this particular case should the "pci" be a NULL pointer,
then we ought to have a much worse problems, if you think about this.

Thus, I am not sure if returning here would be better over letting
things crash properly (which might not be ideal either), as if at this
point "pci" is invalid and we still use it here, then something is
potentially has gone really bad somewhere.

Regardless of the approach here, the pr_err() message is not very
helpful in its current wording.

> +
> +	dev = pci->dev;

You can move this to the top where you declare your variable, so that
you would define it at the same time, for example:

  struct device *dev = pci->dev;

[...]
> +	/* Create main directory for each platform driver */
> +	dir = debugfs_create_dir(dirname, NULL);
> +	if (dir == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		return -ENODEV;
> +	}

A small suggestion about the above: you could perhaps rely on the
following approach:

  if (IS_ERR(dir)) {
  	dev_err(dev, ...);
  	return PTR_ERR(dir);
  }

Unless you want to return -ENODEV for everything, regardless of what the
underlying error code might have been (such as i.e., -EPERM, for
example).  If so, then perhaps the IS_ERR_OR_NULL() macro could be
useful?

Note that debugfs_create_dir() and many other debugfs functions
correctly return -ENODEV if debugfs is not enabled.

Having said that, perhaps this approach would be an overkill.

> +	/* Create sub dirs for Debug, Error injection, Statistics */
> +	ras_des_debug_regs = debugfs_create_dir("ras_des_debug_regs", dir);
> +	if (ras_des_debug_regs == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}
> +
> +	ras_des_error_inj = debugfs_create_dir("ras_des_error_inj", dir);
> +	if (ras_des_error_inj == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}
> +
> +	ras_des_event_counter = debugfs_create_dir("ras_des_counter", dir);
> +	if (ras_des_event_counter == NULL) {
> +		pr_err("error creating directory: %s\n", dirname);
> +		ret = -ENODEV;
> +		goto remove_debug_file;
> +	}

I believe you don't necessary need to print an error message for each
sub-directory created under the root directory "dir".  Why? Technically,
if you can create the root, then you should be able to create all the
sub-directories without issues.

Also, each of the subsequent error messages would print the same name
being the root directory "dir" regardless of which one has failed to be
created.

[...]
> +	/* Create debugfs files for Debug subdirectory */
> +	lane_detection = debugfs_create_file("lane_detection", 0644,
> +					     ras_des_debug_regs, pci,
> +					     &lane_detection_fops);
> +
> +	rx_valid = debugfs_create_file("rx_valid", 0644,
> +					     ras_des_debug_regs, pci,
> +					     &lane_detection_fops);
> +
> +	/* Create debugfs files for Error injection sub dir */

Nit pick: to be consistent, if you could keep using "subdirectory"
everywhere where you use "sub dir".

[...]
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> +	/* No OP */
> +	return 0;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> +	/* No OP */
> +}
[...]

No need for the "No OP" comment.  Also, you could potentially fit
everything on a single line, for example:

  int create_debugfs_files(struct dw_pcie *pci) { return 0; }
  void remove_debugfs_files(void) {}

But this is a matter of style, so I leave it up to you.

Having said that, both of these functions could use less generic names,
so that any potential current or future symbol name clash would be
avoided, especially since these have global scope.  Thus, adding
a prefix such as e.g., "ras_", or "dw_", etc., I am not sure which one
would be more appropriate.

	Krzysztof

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

[Krzysztof Wilczyński]

Hi Shradha,

[...]
> +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);

A small question as I am curious: why not use pci_find_vsec_capability()
here?  The implementation looks very similar, which is why I am asking,
but it might be that I am missing something, and for that I apologise in
advance.

	Krzysztof

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

[Vidya Sagar]

On 5/22/2021 5:01 AM, Krzysztof Wilczyński wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hi Shradha,
> 
> [...]
>> +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);
> 
> A small question as I am curious: why not use pci_find_vsec_capability()
> here?  The implementation looks very similar, which is why I am asking,
> but it might be that I am missing something, and for that I apologise in
> advance.
pci_find_vsec_capability() expects struct pci_dev * which we get only 
after enumeration is done. In the current scenario, we are still in 
pre-link up phase and don't have struct pci_dev * yet, hence this 
implementation, right Shradha??

> 
>          Krzysztof
> 

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

[Vidya Sagar]

On 5/18/2021 11:16 PM, Shradha Todi wrote:
> External email: Use caution opening links or attachments
> 
> 
> Add call to create_debugfs_files() from DWC driver to create the RASDES
> debugfs structure for each platform driver. Since it can be used for both
> DW HOST controller as well as DW EP controller, let's add it in the common
> setup function.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>   drivers/pci/controller/dwc/pcie-designware.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 348f6f696976..c054f8ba1cf4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -15,6 +15,7 @@
> 
>   #include "../../pci.h"
>   #include "pcie-designware.h"
> +#include "pcie-designware-debugfs.h"
> 
>   /*
>    * These interfaces resemble the pci_find_*capability() interfaces, but these
> @@ -793,4 +794,8 @@ void dw_pcie_setup(struct dw_pcie *pci)
>                         PCIE_PL_CHK_REG_CHK_REG_START;
>                  dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
>          }
> +
> +       ret = create_debugfs_files(pci);
> +       if (ret)
> +               dev_err(pci->dev, "Couldn't create debugfs files\n");
'ret' is undeclared in this function . (for reference, I applied this 
patch on top of 'next-20210526')
>   }
> --
> 2.17.1
> 

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

[Vidya Sagar]

On 5/18/2021 11:16 PM, Shradha Todi wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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 |  4 ++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index a945f0c0e73d..348f6f696976 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -90,6 +90,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 7d6e9b7576be..307525aaa063 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -155,6 +155,9 @@
>   #define MAX_IATU_IN                    256
>   #define MAX_IATU_OUT                   256
> 
> +/* Synopsys Vendor specific data */
> +#define DW_PCIE_RAS_CAP_ID             0x2
This is not used in this patch. Better to move it to the second patch in 
this series where it is used.

> +
>   struct pcie_port;
>   struct dw_pcie;
>   struct dw_pcie_ep;
> @@ -284,6 +287,7 @@ struct dw_pcie {
> 
>   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
> 

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

[Vidya Sagar]

On 5/18/2021 11:16 PM, Shradha Todi wrote:
> External email: Use caution opening links or attachments
> 
> 
> RAS is a vendor specific extended PCIe capability which which helps to read
> hardware internal state.
> 
> This framework support provides debugfs entries to use these features for
> DesignWare controller. Following primary features of DesignWare controllers
> 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            |   9 +
>   drivers/pci/controller/dwc/Makefile           |   1 +
>   .../controller/dwc/pcie-designware-debugfs.c  | 544 ++++++++++++++++++
>   .../controller/dwc/pcie-designware-debugfs.h  | 133 +++++
>   drivers/pci/controller/dwc/pcie-designware.h  |   1 +
>   5 files changed, 688 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 423d35872ce4..2d0a18cb9418 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -6,6 +6,15 @@ menu "DesignWare PCI Core Support"
>   config PCIE_DW
>          bool
> 
> +config PCIE_DW_DEBUGFS
> +       bool "DWC PCIe debugfs entries"
> +       default n
> +       help
> +         Enables debugfs entries for the DWC PCIe Controller.
> +         These entries make use of the RAS DES features in the DW
> +         controller to help in debug, error injection and statistical
> +         counters
> +
>   config PCIE_DW_HOST
>          bool
>          depends on PCI_MSI_IRQ_DOMAIN
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index eca805c1a023..6ad12638f793 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..84bf196df240
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.c
> @@ -0,0 +1,544 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + *             http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@samsung.com>
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +#include "pcie-designware.h"
> +#include "pcie-designware-debugfs.h"
> +
> +#define DEBUGFS_BUF_MAX_SIZE   100
> +
> +static char debugfs_buf[DEBUGFS_BUF_MAX_SIZE];
> +
> +static struct dentry *dir;
Why can't this be moved inside struct dw_pcie?
> +static struct dentry *sub_dir;
> +
> +static struct event_counters ras_events[] = {
> +       {"ebuf_overflow",               ebuf_overflow},
> +       {"ebuf_underrun",               ebuf_underrun},
> +       {"decode_error",                decode_error},
> +       {"sync_header_error",           sync_header_error},
> +       {"receiver_error",              receiver_error},
> +       {"framing_error",               framing_error},
> +       {"lcrc_error",                  lcrc_error},
> +       {"ecrc_error",                  ecrc_error},
> +       {"unsupp_req_error",            unsupp_req_error},
> +       {"cmpltr_abort_error",          cmpltr_abort_error},
> +       {"cmpltn_timeout_error",        cmpltn_timeout_error},
> +       {"tx_l0s_entry",                tx_l0s_entry},
> +       {"rx_l0s_entry",                rx_l0s_entry},
> +       {"l1_entry",                    l1_entry},
> +       {"l1_1_entry",                  l1_1_entry},
> +       {"l1_2_entry",                  l1_2_entry},
> +       {"l2_entry",                    l2_entry},
> +       {"speed_change",                speed_change},
> +       {"width_chage",                 width_chage},
> +       {0, 0},
> +};
> +
> +static struct error_injections error_list[] = {
> +       {"tx_lcrc",             tx_lcrc},
> +       {"tx_ecrc",             tx_ecrc},
> +       {"rx_lcrc",             rx_lcrc},
> +       {"rx_ecrc",             rx_ecrc},
> +       {0, 0},
> +};
> +
> +static int open(struct inode *inode, struct file *file)
> +{
> +       file->private_data = inode->i_private;
> +
> +       return 0;
> +}
> +
> +/*
> + * set_event_number: Function to set event number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all event counters. Using the debugfs filname, the
> + * group number and event number for the counter is extracted and
> + * then programmed into the control register.
> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: void
> + */
> +static void set_event_number(struct file *file)
> +{
> +       int i;
> +       u32 val;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +       u32 max_size = sizeof(ras_events) / sizeof(struct event_counters);
> +
> +       for (i = 0; i < max_size; i++) {
> +               if (strcmp(ras_events[i].name,
> +                          file->f_path.dentry->d_parent->d_iname) == 0) {
> +                       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                                       RAS_DES_EVENT_COUNTER_CTRL_REG);
> +                       val &= ~(EVENT_COUNTER_ENABLE);
> +                       val &= ~(0xFFF << 16);
> +                       val |= (ras_events[i].event_num << 16);
> +                       dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +                                       RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +                       break;
> +               }
> +       }
> +}
> +/*
> + * get_error_inj_number: Function to get error number based on filename
> + *
> + * This function is called from the common read and write function
> + * written for all error injection debugfs entries. Using the debugfs
> + * filname, the error group and type of error to be injected is extracted.
> + *
> + * @file: file pointer to the debugfs entry
> + *
> + * Return: u32
> + * [31:8]: Type of error to be injected
> + * [7:0]: Group of error it belongs to
> + */
> +
> +static u32 get_error_inj_number(struct file *file)
> +{
> +       int i;
> +       u32 max_size = sizeof(error_list) / sizeof(struct error_injections);
> +
> +       for (i = 0; i < max_size; i++) {
> +               if (strcmp(error_list[i].name,
> +                          file->f_path.dentry->d_iname) == 0) {
> +                       return error_list[i].type_of_err;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * ras_event_counter_en_read: Function to get if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_enable
> + * It returns whether the counter is enabled or not
> + */
> +static ssize_t ras_event_counter_en_read(struct file *file, char __user *buf,
> +                                        size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       set_event_number(file);
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                               RAS_DES_EVENT_COUNTER_CTRL_REG);
> +       val = (val >> EVENT_COUNTER_STATUS_SHIFT) & EVENT_COUNTER_STATUS_MASK;
> +       if (val)
> +               sprintf(debugfs_buf, "Enabled\n");
> +       else
> +               sprintf(debugfs_buf, "Disabled\n");
> +
> +       ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +                                     strlen(debugfs_buf));
> +
> +       return ret;
> +}
> +
> +/*
> + * ras_event_counter_lane_sel_read: Function to get lane number selected
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
> + * It returns for which lane the counter configurations are done
> + */
> +static ssize_t ras_event_counter_lane_sel_read(struct file *file,
> +                                              char __user *buf, size_t count,
> +                                              loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       set_event_number(file);
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                       RAS_DES_EVENT_COUNTER_CTRL_REG);
> +       val = (val >> LANE_SELECT_SHIFT) & LANE_SELECT_MASK;
> +       sprintf(debugfs_buf, "0x%x\n", val);
> +       ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +                                     strlen(debugfs_buf));
> +
> +       return ret;
> +}
> +
> +/*
> + * ras_event_counter_value_read: Function to get counter value
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/counter_value
> + * It returns the number of time the selected event has happened if enabled
> + */
> +
> +static ssize_t ras_event_counter_value_read(struct file *file, char __user *buf,
> +                                           size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       set_event_number(file);
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                               RAS_DES_EVENT_COUNTER_DATA_REG);
> +       sprintf(debugfs_buf, "0x%x\n", val);
> +       ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +                                     strlen(debugfs_buf));
> +
> +       return ret;
> +}
> +
> +/*
> + * ras_event_counter_en_write: Function to set if counter is enable
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/
> + *             ras_des_counter/<name>/counter_enable
> + * Here n can be 1 to enable and 0 to disable
> + */
> +static ssize_t ras_event_counter_en_write(struct file *file,
> +                                         const char __user *buf,
> +                                         size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       u32 enable;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       ret = kstrtou32_from_user(buf, count, 0, &enable);
> +       if (ret)
> +               return ret;
> +
> +       set_event_number(file);
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                               RAS_DES_EVENT_COUNTER_CTRL_REG);
> +       if (enable)
> +               val |= PER_EVENT_ON;
> +       else
> +               val |= PER_EVENT_OFF;
> +
> +       dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +                          RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +
> +       return count;
> +}
> +
> +/*
> + * ras_event_counter_lane_sel_write: Function to set lane number
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_counter/<name>/lane_select
> + * Here n is the lane that we want to select for counter configuration
> + */
> +static ssize_t ras_event_counter_lane_sel_write(struct file *file,
> +                                               const char __user *buf,
> +                                               size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       u32 lane;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       ret = kstrtou32_from_user(buf, count, 0, &lane);
> +       if (ret)
> +               return ret;
> +
> +       set_event_number(file);
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                               RAS_DES_EVENT_COUNTER_CTRL_REG);
> +       val &= ~(LANE_SELECT_MASK << LANE_SELECT_SHIFT);
> +       val |= (lane << LANE_SELECT_SHIFT);
> +       dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +                          RAS_DES_EVENT_COUNTER_CTRL_REG, val);
> +
> +       return count;
> +}
> +
> +/*
> + * ras_error_inj_read: Function to read number of errors left to be injected
> + *
> + * This function is invoked when the following command is made:
> + * cat /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
> + * This returns the number of errors left to be injected which will
> + * keep reducing as we make pcie transactions to inject error.
> + */
> +static ssize_t ras_error_inj_read(struct file *file, char __user *buf,
> +                                 size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val, err_num, inj_num;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       err_num = get_error_inj_number(file);
> +       inj_num = (err_num & 0xFF);
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> +                               (0x4 * inj_num));
> +       sprintf(debugfs_buf, "0x%x\n", (val & EINJ_COUNT_MASK));
> +       ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +                                     strlen(debugfs_buf));
> +
> +       return ret;
> +}
> +
> +/*
> + * ras_error_inj_write: Function to set number of errors to be injected
> + *
> + * This function is invoked when the following command is made:
> + * echo n > /sys/kernel/debug/dwc_pcie_plat/ras_des_error_inj/<name of error>
> + * Here n is the number of errors we want to inject
> + */
> +static ssize_t ras_error_inj_write(struct file *file, const char __user *buf,
> +                                  size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val, err_num, inj_num;
> +       u32 counter;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       if (count > DEBUGFS_BUF_MAX_SIZE)
> +               return -EINVAL;
> +
> +       ret = kstrtou32_from_user(buf, count, 0, &counter);
> +       if (ret)
> +               return ret;
> +
> +       err_num = get_error_inj_number(file);
> +       inj_num = (err_num & 0xFF);
> +       err_num = (err_num >> 8);
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> +                               (0x4 * inj_num));
> +       val &= ~(EINJ_TYPE_MASK << EINJ_TYPE_SHIFT);
> +       val |= (err_num << EINJ_TYPE_SHIFT);
> +       val &= ~(EINJ_COUNT_MASK);
> +       val |= counter;
> +       dw_pcie_writel_dbi(pci, pci->ras_cap_offset + ERR_INJ0_OFF +
> +                          (0x4 * inj_num), val);
> +       dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +                          ERR_INJ_ENABLE_REG, (0x1 << inj_num));
> +
> +       return count;
> +}
> +
> +static ssize_t lane_detection_read(struct file *file, char __user *buf,
> +                                  size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                               SD_STATUS_L1LANE_REG);
> +       val = (val >> LANE_DETECT_SHIFT) & LANE_DETECT_MASK;
> +       sprintf(debugfs_buf, "0x%x\n", val);
> +
> +       ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +                                     strlen(debugfs_buf));
> +
> +       return ret;
> +}
> +
> +static ssize_t rx_valid_read(struct file *file, char __user *buf,
> +                            size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                               SD_STATUS_L1LANE_REG);
> +       val = (val >> PIPE_RXVALID_SHIFT) & PIPE_RXVALID_MASK;
> +       sprintf(debugfs_buf, "0x%x\n", val);
> +
> +       ret = simple_read_from_buffer(buf, count, ppos, debugfs_buf,
> +                                     strlen(debugfs_buf));
> +
> +       return ret;
> +}
> +
> +static ssize_t lane_selection_write(struct file *file, const char __user *buf,
> +                                   size_t count, loff_t *ppos)
> +{
> +       u32 ret;
> +       u32 val;
> +       u32 lane;
> +       struct dw_pcie *pci = (struct dw_pcie *) file->private_data;
> +
> +       if (count > DEBUGFS_BUF_MAX_SIZE)
> +               return -EINVAL;
> +
> +       ret = kstrtou32_from_user(buf, count, 0, &lane);
> +       if (ret)
> +               return ret;
> +
> +       val = dw_pcie_readl_dbi(pci, pci->ras_cap_offset +
> +                               SD_STATUS_L1LANE_REG);
> +       val &= ~(LANE_SELECT_MASK);
> +       val |= lane;
> +       dw_pcie_writel_dbi(pci, pci->ras_cap_offset +
> +                          SD_STATUS_L1LANE_REG, val);
> +
> +       return count;
> +}
> +
> +static const struct file_operations lane_detection_fops = {
> +       .open = open,
> +       .read = lane_detection_read,
> +       .write = lane_selection_write
> +};
> +
> +static const struct file_operations rx_valid_fops = {
> +       .open = open,
> +       .read = rx_valid_read,
> +       .write = lane_selection_write
> +};
> +
> +static const struct file_operations cnt_en_ops = {
> +       .read = ras_event_counter_en_read,
> +       .write = ras_event_counter_en_write,
> +       .open = simple_open,
> +};
> +
> +static const struct file_operations lane_sel_ops = {
> +       .read = ras_event_counter_lane_sel_read,
> +       .write = ras_event_counter_lane_sel_write,
> +       .open = simple_open,
> +};
> +
> +static const struct file_operations cnt_val_ops = {
> +       .read = ras_event_counter_value_read,
> +       .open = simple_open,
> +};
> +
> +static const struct file_operations inj_ops = {
> +       .read = ras_error_inj_read,
> +       .write = ras_error_inj_write,
> +       .open = simple_open,
> +};
> +
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> +       int ret = 0;
> +       char dirname[32];
> +       struct device *dev;
> +
> +       struct dentry *ras_des_debug_regs;
> +       struct dentry *ras_des_error_inj;
> +       struct dentry *ras_des_event_counter;
> +       struct dentry *lane_detection;
> +       struct dentry *rx_valid;
> +
> +       if (!pci) {
> +               pr_err("pcie struct is NULL\n");
I think just returning from here should be fine without print.
Also, it is better to use dev_* instead of pr_* for prints.
> +               return -ENODEV;
> +       }
> +
> +       dev = pci->dev;
> +       sprintf(dirname, "pcie_dwc_%s", dev_name(dev));
> +
> +       pci->ras_cap_offset = dw_pcie_find_vsec_capability(pci,
> +                                                          DW_PCIE_RAS_CAP_ID);
> +       if (!pci->ras_cap_offset) {
> +               pr_err("No RAS capability available\n");
> +               return -ENODEV;
> +       }
> +
> +       /* Create main directory for each platform driver */
> +       dir = debugfs_create_dir(dirname, NULL);
> +       if (dir == NULL) {
> +               pr_err("error creating directory: %s\n", dirname);
> +               return -ENODEV;
> +       }
> +
> +       /* Create sub dirs for Debug, Error injection, Statistics */
> +       ras_des_debug_regs = debugfs_create_dir("ras_des_debug_regs", dir);
> +       if (ras_des_debug_regs == NULL) {
> +               pr_err("error creating directory: %s\n", dirname);
> +               ret = -ENODEV;
> +               goto remove_debug_file;
> +       }
> +
> +       ras_des_error_inj = debugfs_create_dir("ras_des_error_inj", dir);
> +       if (ras_des_error_inj == NULL) {
> +               pr_err("error creating directory: %s\n", dirname);
> +               ret = -ENODEV;
> +               goto remove_debug_file;
> +       }
> +
> +       ras_des_event_counter = debugfs_create_dir("ras_des_counter", dir);
> +       if (ras_des_event_counter == NULL) {
> +               pr_err("error creating directory: %s\n", dirname);
> +               ret = -ENODEV;
> +               goto remove_debug_file;
> +       }
> +
> +       /* Create debugfs files for Debug subdirectory */
> +       lane_detection = debugfs_create_file("lane_detection", 0644,
> +                                            ras_des_debug_regs, pci,
> +                                            &lane_detection_fops);
> +
> +       rx_valid = debugfs_create_file("rx_valid", 0644,
> +                                            ras_des_debug_regs, pci,
> +                                            &lane_detection_fops);
Shouldn't it be 'rx_valid_fops' instead of 'lane_detection_fops', 
otherwise I don't see any references for 'rx_valid_fops'.
> +
> +       /* Create debugfs files for Error injection sub dir */
> +       CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_ecrc);
> +       CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_ecrc);
> +       CREATE_RAS_ERROR_INJECTION_DEBUGFS(tx_lcrc);
> +       CREATE_RAS_ERROR_INJECTION_DEBUGFS(rx_lcrc);
> +
> +       /* Create debugfs files for Statistical counter sub dir */
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_overflow);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(ebuf_underrun);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(decode_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(receiver_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(framing_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(lcrc_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(ecrc_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(unsupp_req_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltr_abort_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(cmpltn_timeout_error);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(tx_l0s_entry);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(rx_l0s_entry);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_entry);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_1_entry);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(l1_2_entry);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(l2_entry);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(speed_change);
> +       CREATE_RAS_EVENT_COUNTER_DEBUGFS(width_chage);
> +
> +       return ret;
> +
> +remove_debug_file:
> +       remove_debugfs_files();
> +       return ret;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> +       debugfs_remove_recursive(dir);
> +}
> 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..3dc3cf696a04
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-designware-debugfs.h
> @@ -0,0 +1,133 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Synopsys DesignWare PCIe controller debugfs driver
> + *
> + * Copyright (C) 2021 Samsung Electronics Co., Ltd.
> + *             http://www.samsung.com
> + *
> + * Author: Shradha Todi <shradha.t@samsung.com>
> + */
> +
> +#ifndef _PCIE_DESIGNWARE_DEBUGFS_H
> +#define _PCIE_DESIGNWARE_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                 0xFFFFFF
> +#define EINJ_TYPE_SHIFT                        8
> +
> +enum event_numbers {
> +       ebuf_overflow           = 0x0,
> +       ebuf_underrun           = 0x1,
> +       decode_error            = 0x2,
> +       sync_header_error       = 0x5,
> +       receiver_error          = 0x106,
> +       framing_error           = 0x109,
> +       lcrc_error              = 0x201,
> +       ecrc_error              = 0x302,
> +       unsupp_req_error        = 0x303,
> +       cmpltr_abort_error      = 0x304,
> +       cmpltn_timeout_error    = 0x305,
Is there a plan to add entries for Group-4?
> +       tx_l0s_entry            = 0x502,
> +       rx_l0s_entry            = 0x503,
> +       l1_entry                = 0x505,
> +       l1_1_entry              = 0x507,
> +       l1_2_entry              = 0x508,
> +       l2_entry                = 0x50B,
> +       speed_change            = 0x50C,
> +       width_chage             = 0x50D,
Also wondering about the plan for Groups-6,7
> +};
> +
> +/*
> + * struct event_counters: Struct to store event number
> + *
> + * @name: name of the event counter
> + *        eg: ecrc_err count, l1 entry count
> + * @event_num: Event number and group number
> + * [16:8]: Group number
> + * [7:0]: Event number
> + */
> +struct event_counters {
> +       const char *name;
> +       u32 event_num;
> +};
> +
> +enum error_inj_code {
> +       tx_lcrc                 = 0x000,
> +       tx_ecrc                 = 0x300,
> +       rx_lcrc                 = 0x800,
> +       rx_ecrc                 = 0xB00,
> +};
This is a generic query. DesignWare cores have 'Time-based Analysis 
Support' as well as part of DES. Is it in your pipeline to upstream the 
support for it as well?
> +
> +/*
> + * struct error_injectionss: Struct to store error numbers
> + *
> + * @name: name of the error to be injected
> + *        eg: ecrc_err, lcrc_err
> + * @event_num: Error number and group number
> + * [31:8]: Error type. This should be same as bits [31:8]
> + *         in the EINJn_REG where n is group number
> + * [7:0]: Error injection group
> + *        0 - CRC
> + *        1 - seq num
> + *        2 - DLLP error
> + *        3 - symbol error
> + *        4 - FC error
> + */
> +struct error_injections {
> +       const char *name;
> +       u32 type_of_err;
> +};
> +
> +#define CREATE_RAS_EVENT_COUNTER_DEBUGFS(name)                         \
> +do {                                                                   \
> +       sub_dir = debugfs_create_dir(#name, ras_des_event_counter);     \
> +       debugfs_create_file("counter_enable", 0644, sub_dir, pci,       \
> +                               &cnt_en_ops);                           \
> +       debugfs_create_file("lane_select", 0644, sub_dir, pci,          \
> +                               &lane_sel_ops);                         \
lane selection is required only for group-0 & 4 and other group counters 
are defined for all common lanes. Since group-4 is anyway not 
implemented in the current patch series, it would be good to create 
'lane_select' file only for group-0?
> +       debugfs_create_file("counter_value", 0444, sub_dir, pci,        \
> +                               &cnt_val_ops);                          \
> +} while (0)
> +
> +#define CREATE_RAS_ERROR_INJECTION_DEBUGFS(name)                       \
> +       debugfs_create_file(#name, 0644, ras_des_error_inj, pci,        \
> +                               &inj_ops);
> +
> +#ifdef CONFIG_PCIE_DW_DEBUGFS
> +int create_debugfs_files(struct dw_pcie *pci);
> +void remove_debugfs_files(void);
> +#else
> +int create_debugfs_files(struct dw_pcie *pci)
> +{
> +       /* No OP */
> +       return 0;
> +}
> +
> +void remove_debugfs_files(void)
> +{
> +       /* No OP */
> +}
> +#endif
> +
> +#endif
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 307525aaa063..031fdafe0a3e 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -278,6 +278,7 @@ struct dw_pcie {
>          u8                      n_fts[2];
>          bool                    iatu_unroll_enabled: 1;
>          bool                    io_cfg_atu_shared: 1;
> +       u32                     ras_cap_offset;
>   };
> 
>   #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> --
> 2.17.1
>