Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1703921 > unrolled thread

[PATCH 1/5] edac: synopsys: Add platform specific structures ddrc controller

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/5] edac: synopsys: Add platform specific structures ddrc controller Michal Simek <michal.simek@xilinx.com> - 2017-08-04 14:10 +0200
    [PATCH 3/5] edac: synopsys: Add ecc error injection support Michal Simek <michal.simek@xilinx.com> - 2017-08-04 14:10 +0200
    Re: [PATCH 1/5] edac: synopsys: Add platform specific structures  ddrc controller Borislav Petkov <bp@alien8.de> - 2017-08-06 07:20 +0200
      Re: [PATCH 1/5] edac: synopsys: Add platform specific structures  ddrc controller Borislav Petkov <bp@alien8.de> - 2017-08-07 11:30 +0200

#1703921 — [PATCH 1/5] edac: synopsys: Add platform specific structures ddrc controller

From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

This patch adds platform specific structures, so that we can add
different IP support later using quirks.

Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 drivers/edac/synopsys_edac.c | 70 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 12 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 1c01dec78ec3..65f3b04d5a87 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -22,6 +22,7 @@
 #include <linux/edac.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 #include "edac_module.h"
 
@@ -95,6 +96,9 @@
 #define SCRUB_MODE_MASK		0x7
 #define SCRUB_MODE_SECDED	0x4
 
+/* DDR ECC Quirks */
+#define DDR_ECC_INTR_SUPPORT    BIT(0)
+
 /**
  * struct ecc_error_info - ECC error log information
  * @row:	Row number
@@ -130,6 +134,7 @@ struct synps_ecc_status {
  * @baseaddr:	Base address of the DDR controller
  * @message:	Buffer for framing the event specific info
  * @stat:	ECC status information
+ * @p_data:	Pointer to platform data
  * @ce_cnt:	Correctable Error count
  * @ue_cnt:	Uncorrectable Error count
  */
@@ -137,11 +142,29 @@ struct synps_edac_priv {
 	void __iomem *baseaddr;
 	char message[SYNPS_EDAC_MSG_SIZE];
 	struct synps_ecc_status stat;
+	const struct synps_platform_data *p_data;
 	u32 ce_cnt;
 	u32 ue_cnt;
 };
 
 /**
+ * struct synps_platform_data -  synps platform data structure
+ * @synps_edac_geterror_info:	function pointer to synps edac error info
+ * @synps_edac_get_mtype:	function pointer to synps edac mtype
+ * @synps_edac_get_dtype:	function pointer to synps edac dtype
+ * @synps_edac_get_eccstate:	function pointer to synps edac eccstate
+ * @quirks:			to differentiate IPs
+ */
+struct synps_platform_data {
+	int (*synps_edac_geterror_info)(void __iomem *base,
+					 struct synps_ecc_status *p);
+	enum mem_type (*synps_edac_get_mtype)(const void __iomem *base);
+	enum dev_type (*synps_edac_get_dtype)(const void __iomem *base);
+	bool (*synps_edac_get_eccstate)(void __iomem *base);
+	int quirks;
+};
+
+/**
  * synps_edac_geterror_info - Get the current ecc error info
  * @base:	Pointer to the base address of the ddr memory controller
  * @p:		Pointer to the synopsys ecc status structure
@@ -242,7 +265,8 @@ static void synps_edac_check(struct mem_ctl_info *mci)
 	struct synps_edac_priv *priv = mci->pvt_info;
 	int status;
 
-	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
+	status = priv->p_data->synps_edac_geterror_info(priv->baseaddr,
+							&priv->stat);
 	if (status)
 		return;
 
@@ -372,10 +396,12 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
 		for (j = 0; j < csi->nr_channels; j++) {
 			dimm            = csi->channels[j]->dimm;
 			dimm->edac_mode = EDAC_FLAG_SECDED;
-			dimm->mtype     = synps_edac_get_mtype(priv->baseaddr);
+			dimm->mtype     = priv->p_data->synps_edac_get_mtype(
+						priv->baseaddr);
 			dimm->nr_pages  = (size >> PAGE_SHIFT) / csi->nr_channels;
 			dimm->grain     = SYNPS_EDAC_ERR_GRAIN;
-			dimm->dtype     = synps_edac_get_dtype(priv->baseaddr);
+			dimm->dtype     = priv->p_data->synps_edac_get_dtype(
+						priv->baseaddr);
 		}
 	}
 
@@ -424,6 +450,21 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 	return status;
 }
 
+static const struct synps_platform_data zynq_edac_def = {
+	.synps_edac_geterror_info	= synps_edac_geterror_info,
+	.synps_edac_get_mtype		= synps_edac_get_mtype,
+	.synps_edac_get_dtype		= synps_edac_get_dtype,
+	.synps_edac_get_eccstate	= synps_edac_get_eccstate,
+	.quirks				= 0,
+};
+
+static const struct of_device_id synps_edac_match[] = {
+	{ .compatible = "xlnx,zynq-ddrc-a05", .data = (void *)&zynq_edac_def },
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, synps_edac_match);
+
 /**
  * synps_edac_mc_probe - Check controller and bind driver
  * @pdev:	Pointer to the platform_device struct
@@ -441,13 +482,22 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 	int rc;
 	struct resource *res;
 	void __iomem *baseaddr;
+	const struct of_device_id *match;
+	const struct synps_platform_data *p_data;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	baseaddr = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(baseaddr))
 		return PTR_ERR(baseaddr);
 
-	if (!synps_edac_get_eccstate(baseaddr)) {
+	match = of_match_node(synps_edac_match, pdev->dev.of_node);
+	if (!match && !match->data) {
+		dev_err(&pdev->dev, "of_match_node() failed\n");
+		return -EINVAL;
+	}
+
+	p_data = (struct synps_platform_data *)match->data;
+	if (!(p_data->synps_edac_get_eccstate(baseaddr))) {
 		edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
 		return -ENXIO;
 	}
@@ -469,6 +519,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 
 	priv = mci->pvt_info;
 	priv->baseaddr = baseaddr;
+	priv->p_data = match->data;
+
 	rc = synps_edac_mc_init(mci, pdev);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC,
@@ -487,7 +539,8 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 	 * Start capturing the correctable and uncorrectable errors. A write of
 	 * 0 starts the counters.
 	 */
-	writel(0x0, baseaddr + ECC_CTRL_OFST);
+	if (!(priv->p_data->quirks & DDR_ECC_INTR_SUPPORT))
+		writel(0x0, baseaddr + ECC_CTRL_OFST);
 	return rc;
 
 free_edac_mc:
@@ -512,13 +565,6 @@ static int synps_edac_mc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id synps_edac_match[] = {
-	{ .compatible = "xlnx,zynq-ddrc-a05", },
-	{ /* end of table */ }
-};
-
-MODULE_DEVICE_TABLE(of, synps_edac_match);
-
 static struct platform_driver synps_edac_mc_driver = {
 	.driver = {
 		   .name = "synopsys-edac",
-- 
1.9.1

[toc] | [next] | [standalone]

#1703923 — [PATCH 3/5] edac: synopsys: Add ecc error injection support

From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

The ZynqMP DDRC controller has data poisoning support
to inject CE or UE errors. this patch adds this support
using sysfs attributes.

created the following sysfs entries to support this.
-> /sys/devices/system/edac/mc/mc0/inject_data_poison
-> /sys/devices/system/edac/mc/mc0/inject_data_error

Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 drivers/edac/synopsys_edac.c | 291 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 290 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index fdf1186151c1..546adc243bca 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -99,6 +99,7 @@
 
 /* DDR ECC Quirks */
 #define DDR_ECC_INTR_SUPPORT    BIT(0)
+#define DDR_ECC_DATA_POISON_SUPPORT BIT(1)
 
 /* ZynqMP Enhanced DDR memory controller registers that are relevant to ECC */
 /* ECC Configuration Registers */
@@ -174,6 +175,11 @@
 #define ECC_CEADDR1_BNKGRP_SHIFT	24
 #define ECC_CEADDR1_BNKNR_SHIFT	16
 
+/* ECC Poison register shifts */
+#define ECC_POISON0_RANK_SHIFT 24
+#define ECC_POISON1_BANKGRP_SHIFT 28
+#define ECC_POISON1_BANKNR_SHIFT 24
+
 /* DDR Memory type defines */
 #define MEM_TYPE_DDR3 0x1
 #define MEM_TYPE_LPDDR3 0x1
@@ -181,6 +187,38 @@
 #define MEM_TYPE_DDR4 0x10
 #define MEM_TYPE_LPDDR4 0x10
 
+/* DDRC Software control register */
+#define DDRC_SWCTL 0x320
+
+/* DDRC ECC CE & UE poison mask */
+#define ECC_CEPOISON_MASK 0x3
+#define ECC_UEPOISON_MASK 0x1
+
+/* DDRC Device config masks */
+#define DDRC_MSTR_DEV_CONFIG_MASK 0xC0000000
+#define DDRC_MSTR_DEV_CONFIG_SHIFT	30
+#define DDRC_MSTR_DEV_CONFIG_X4_MASK	0
+#define DDRC_MSTR_DEV_CONFIG_X8_MASK	1
+#define DDRC_MSTR_DEV_CONFIG_X16_MASK	0x10
+#define DDRC_MSTR_DEV_CONFIG_X32_MASK	0X11
+
+/* DDR4 and DDR3 device Row,Column,Bank Mapping */
+#define DDR4_COL_SHIFT		3
+#define DDR4_BANKGRP_SHIFT	13
+#define DDR4_BANK_SHIFT	15
+#define DDR4_ROW_SHIFT		17
+#define DDR4_COL_MASK		0x3FF
+#define DDR4_BANKGRP_MASK	0x3
+#define DDR4_BANK_MASK		0x3
+#define DDR4_ROW_MASK		0x7FFF
+
+#define DDR3_COL_SHIFT	3
+#define DDR3_BANK_SHIFT 13
+#define DDR3_ROW_SHIFT	16
+#define DDR3_COL_MASK	0x3FF
+#define DDR3_BANK_MASK	0x7
+#define DDR3_ROW_MASK	0x3FFF
+
 /**
  * struct ecc_error_info - ECC error log information
  * @row:	Row number
@@ -223,6 +261,7 @@ struct synps_ecc_status {
  * @p_data:	Pointer to platform data
  * @ce_cnt:	Correctable Error count
  * @ue_cnt:	Uncorrectable Error count
+ * @poison_addr:Data poison address
  */
 struct synps_edac_priv {
 	void __iomem *baseaddr;
@@ -231,6 +270,7 @@ struct synps_edac_priv {
 	const struct synps_platform_data *p_data;
 	u32 ce_cnt;
 	u32 ue_cnt;
+	ulong poison_addr;
 };
 
 /**
@@ -628,6 +668,7 @@ static enum mem_type synps_enh_edac_get_mtype(const void __iomem *base)
 
 	memtype = readl(base + CTRL_OFST);
 
+	mt = MEM_UNKNOWN;
 	if ((memtype & MEM_TYPE_DDR3) || (memtype & MEM_TYPE_LPDDR3))
 		mt = MEM_DDR3;
 	else if (memtype & MEM_TYPE_DDR2)
@@ -732,7 +773,8 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 	.synps_edac_get_mtype		= synps_enh_edac_get_mtype,
 	.synps_edac_get_dtype		= synps_enh_edac_get_dtype,
 	.synps_edac_get_eccstate	= synps_enh_edac_get_eccstate,
-	.quirks				= DDR_ECC_INTR_SUPPORT,
+	.quirks				= (DDR_ECC_INTR_SUPPORT |
+					   DDR_ECC_DATA_POISON_SUPPORT),
 };
 
 static const struct of_device_id synps_edac_match[] = {
@@ -744,6 +786,242 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
 
 MODULE_DEVICE_TABLE(of, synps_edac_match);
 
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
+/**
+ * ddr4_poison_setup - update poison registers
+ * @dttype:		Device structure variable
+ * @device_config:	Device configuration
+ * @priv:		Pointer to synps_edac_priv struct
+ *
+ * Update poison registers as per ddr4 mapping
+ * Return: none.
+ */
+static void ddr4_poison_setup(enum dev_type dttype, int device_config,
+				struct synps_edac_priv *priv)
+{
+	int col, row, bank, bankgrp, regval, shift_val = 0, col_shift;
+
+	/* Check the Configuration of the device */
+	if (device_config & DDRC_MSTR_DEV_CONFIG_X8_MASK) {
+		/* For Full Dq bus */
+		if (dttype == DEV_X8)
+			shift_val = 0;
+		/* For Half Dq bus */
+		else if (dttype == DEV_X4)
+			shift_val = 1;
+		col_shift = 0;
+	} else if (device_config & DDRC_MSTR_DEV_CONFIG_X16_MASK) {
+		if (dttype == DEV_X8)
+			shift_val = 1;
+		else if (dttype == DEV_X4)
+			shift_val = 2;
+		col_shift = 1;
+	}
+
+	col = (priv->poison_addr >> (DDR4_COL_SHIFT -
+				(shift_val - col_shift))) &
+				DDR4_COL_MASK;
+	row = priv->poison_addr >> (DDR4_ROW_SHIFT - shift_val);
+	row &= DDR4_ROW_MASK;
+	bank = priv->poison_addr >> (DDR4_BANK_SHIFT - shift_val);
+	bank &= DDR4_BANK_MASK;
+	bankgrp = (priv->poison_addr >> (DDR4_BANKGRP_SHIFT -
+				(shift_val - col_shift))) &
+				DDR4_BANKGRP_MASK;
+
+	writel(col, priv->baseaddr + ECC_POISON0_OFST);
+	regval = (bankgrp << ECC_POISON1_BANKGRP_SHIFT) |
+		 (bank << ECC_POISON1_BANKNR_SHIFT) | row;
+	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
+}
+
+/**
+ * ddr3_poison_setup - update poison registers
+ * @dttype:		Device structure variable
+ * @device_config:	Device configuration
+ * @priv:		Pointer to synps_edac_priv struct
+ *
+ * Update poison registers as per ddr3 mapping
+ * Return: none.
+ */
+static void ddr3_poison_setup(enum dev_type dttype, int device_config,
+				struct synps_edac_priv *priv)
+{
+	int col, row, bank, bankgrp, regval, shift_val = 0;
+
+	if (dttype == DEV_X8)
+		/* For Full Dq bus */
+		shift_val = 0;
+	else if (dttype == DEV_X4)
+		/* For Half Dq bus */
+		shift_val = 1;
+
+	col = (priv->poison_addr >> (DDR3_COL_SHIFT - shift_val)) &
+		DDR3_COL_MASK;
+	row = priv->poison_addr >> (DDR3_ROW_SHIFT - shift_val);
+	row &= DDR3_ROW_MASK;
+	bank = priv->poison_addr >> (DDR3_BANK_SHIFT - shift_val);
+	bank &= DDR3_BANK_MASK;
+	bankgrp = 0;
+	writel(col, priv->baseaddr + ECC_POISON0_OFST);
+	regval = (bankgrp << ECC_POISON1_BANKGRP_SHIFT) |
+			 (bank << ECC_POISON1_BANKNR_SHIFT) | row;
+	writel(regval, priv->baseaddr + ECC_POISON1_OFST);
+}
+
+/**
+ * synps_edac_mc_inject_data_error_show - Get Poison0 & 1 register contents
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ *
+ * Get the Poison0 and Poison1 register contents
+ * Return: Number of bytes copied.
+ */
+static ssize_t synps_edac_mc_inject_data_error_show(struct device *dev,
+					      struct device_attribute *mattr,
+					      char *data)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+
+	return sprintf(data, "Poison0 Addr: 0x%08x\n\rPoison1 Addr: 0x%08x\n\r"
+			"Error injection Address: 0x%lx\n\r",
+			readl(priv->baseaddr + ECC_POISON0_OFST),
+			readl(priv->baseaddr + ECC_POISON1_OFST),
+			priv->poison_addr);
+}
+
+/**
+ * synps_edac_mc_inject_data_error_store - Configure Poison0 Poison1 registers
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Configures the Poison0 and Poison1 register contents as per user given
+ * address
+ * Return: Number of bytes copied.
+ */
+static ssize_t synps_edac_mc_inject_data_error_store(struct device *dev,
+					       struct device_attribute *mattr,
+					       const char *data, size_t count)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+	int device_config;
+	enum mem_type mttype;
+	enum dev_type dttype;
+
+	mttype = priv->p_data->synps_edac_get_mtype(
+						priv->baseaddr);
+	dttype = priv->p_data->synps_edac_get_dtype(
+						priv->baseaddr);
+	if (kstrtoul(data, 0, &priv->poison_addr))
+		return -EINVAL;
+
+	device_config = readl(priv->baseaddr + CTRL_OFST);
+	device_config = (device_config & DDRC_MSTR_DEV_CONFIG_MASK) >>
+					DDRC_MSTR_DEV_CONFIG_SHIFT;
+	if (mttype == MEM_DDR4)
+		ddr4_poison_setup(dttype, device_config, priv);
+	else if (mttype == MEM_DDR3)
+		ddr3_poison_setup(dttype, device_config, priv);
+
+	return count;
+}
+
+/**
+ * synps_edac_mc_inject_data_poison_show - Shows type of Data poison
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ *
+ * Shows the type of Error injection enabled, either UE or CE
+ * Return: Number of bytes copied.
+ */
+static ssize_t synps_edac_mc_inject_data_poison_show(struct device *dev,
+					      struct device_attribute *mattr,
+					      char *data)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+
+	return sprintf(data, "Data Poisoning: %s\n\r",
+			((readl(priv->baseaddr + ECC_CFG1_OFST)) & 0x3) ?
+			("Correctable Error"):("UnCorrectable Error"));
+}
+
+/**
+ * synps_edac_mc_inject_data_poison_store - Enbles Data poison CE/UE
+ * @dev:	Pointer to the device struct
+ * @mattr:	Pointer to device attributes
+ * @data:	Pointer to user data
+ * @count:	read the size bytes from buffer
+ *
+ * Enables the CE or UE Data poison
+ * Return: Number of bytes copied.
+ */
+static ssize_t synps_edac_mc_inject_data_poison_store(struct device *dev,
+					       struct device_attribute *mattr,
+					       const char *data, size_t count)
+{
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct synps_edac_priv *priv = mci->pvt_info;
+
+	writel(0, priv->baseaddr + DDRC_SWCTL);
+	if (strncmp(data, "CE", 2) == 0)
+		writel(ECC_CEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
+	else
+		writel(ECC_UEPOISON_MASK, priv->baseaddr + ECC_CFG1_OFST);
+	writel(1, priv->baseaddr + DDRC_SWCTL);
+
+	return count;
+}
+
+static DEVICE_ATTR(inject_data_error, 0644,
+	    synps_edac_mc_inject_data_error_show,
+	    synps_edac_mc_inject_data_error_store);
+static DEVICE_ATTR(inject_data_poison, 0644,
+	    synps_edac_mc_inject_data_poison_show,
+	    synps_edac_mc_inject_data_poison_store);
+
+/**
+ * synps_edac_create_sysfs_attributes - Create sysfs entries
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * Create sysfs attributes for injecting ECC errors using data poison.
+ *
+ * Return: 0 if sysfs creation was successful, else return negative error code.
+ */
+static int synps_edac_create_sysfs_attributes(struct mem_ctl_info *mci)
+{
+	int rc;
+
+	rc = device_create_file(&mci->dev, &dev_attr_inject_data_error);
+	if (rc < 0)
+		return rc;
+	rc = device_create_file(&mci->dev, &dev_attr_inject_data_poison);
+	if (rc < 0)
+		return rc;
+	return 0;
+}
+
+/**
+ * synps_edac_remove_sysfs_attributes - Removes sysfs entries
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * Removes sysfs attributes.
+ *
+ * Return: none.
+ */
+static void synps_edac_remove_sysfs_attributes(struct mem_ctl_info *mci)
+{
+	device_remove_file(&mci->dev, &dev_attr_inject_data_error);
+	device_remove_file(&mci->dev, &dev_attr_inject_data_poison);
+}
+
 /**
  * synps_edac_mc_probe - Check controller and bind driver
  * @pdev:	Pointer to the platform_device struct
@@ -831,6 +1109,13 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 		goto free_edac_mc;
 	}
 
+	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT) {
+		if (synps_edac_create_sysfs_attributes(mci)) {
+			edac_printk(KERN_ERR, EDAC_MC,
+					"Failed to create sysfs entries\n");
+			goto free_edac_mc;
+		}
+	}
 	/*
 	 * Start capturing the correctable and uncorrectable errors. A write of
 	 * 0 starts the counters.
@@ -854,8 +1139,12 @@ static int synps_edac_mc_probe(struct platform_device *pdev)
 static int synps_edac_mc_remove(struct platform_device *pdev)
 {
 	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+	struct synps_edac_priv *priv;
 
+	priv = mci->pvt_info;
 	edac_mc_del_mc(&pdev->dev);
+	if (priv->p_data->quirks & DDR_ECC_DATA_POISON_SUPPORT)
+		synps_edac_remove_sysfs_attributes(mci);
 	edac_mc_free(mci);
 
 	return 0;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1704745 — Re: [PATCH 1/5] edac: synopsys: Add platform specific structures ddrc controller

On Fri, Aug 04, 2017 at 02:00:23PM +0200, Michal Simek wrote:
> From: Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>

That subject:
 Subject: [PATCH 1/5] edac: synopsys: Add platform specific structures ddrc controller

doesn't read like a proper sentence to me.

> This patch adds platform specific structures, so that we can add

"This patch" in a commit message is tautologically redundant.

> different IP support later using quirks.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>  drivers/edac/synopsys_edac.c | 70 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
> index 1c01dec78ec3..65f3b04d5a87 100644
> --- a/drivers/edac/synopsys_edac.c
> +++ b/drivers/edac/synopsys_edac.c
> @@ -22,6 +22,7 @@
>  #include <linux/edac.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/of.h>
>  
>  #include "edac_module.h"
>  
> @@ -95,6 +96,9 @@
>  #define SCRUB_MODE_MASK		0x7
>  #define SCRUB_MODE_SECDED	0x4
>  
> +/* DDR ECC Quirks */
> +#define DDR_ECC_INTR_SUPPORT    BIT(0)
> +
>  /**
>   * struct ecc_error_info - ECC error log information
>   * @row:	Row number
> @@ -130,6 +134,7 @@ struct synps_ecc_status {
>   * @baseaddr:	Base address of the DDR controller
>   * @message:	Buffer for framing the event specific info
>   * @stat:	ECC status information
> + * @p_data:	Pointer to platform data
>   * @ce_cnt:	Correctable Error count
>   * @ue_cnt:	Uncorrectable Error count
>   */
> @@ -137,11 +142,29 @@ struct synps_edac_priv {
>  	void __iomem *baseaddr;
>  	char message[SYNPS_EDAC_MSG_SIZE];
>  	struct synps_ecc_status stat;
> +	const struct synps_platform_data *p_data;
>  	u32 ce_cnt;
>  	u32 ue_cnt;
>  };
>  
>  /**
> + * struct synps_platform_data -  synps platform data structure
> + * @synps_edac_geterror_info:	function pointer to synps edac error info
> + * @synps_edac_get_mtype:	function pointer to synps edac mtype
> + * @synps_edac_get_dtype:	function pointer to synps edac dtype
> + * @synps_edac_get_eccstate:	function pointer to synps edac eccstate

"function pointer to" and then something, doesn't look like an optimal
explanation to me. How about:

"Function which returns the DIMM type"

and so on.

> + * @quirks:			to differentiate IPs
> + */
> +struct synps_platform_data {
> +	int (*synps_edac_geterror_info)(void __iomem *base,
> +					 struct synps_ecc_status *p);
> +	enum mem_type (*synps_edac_get_mtype)(const void __iomem *base);
> +	enum dev_type (*synps_edac_get_dtype)(const void __iomem *base);
> +	bool (*synps_edac_get_eccstate)(void __iomem *base);
> +	int quirks;
> +};
> +
> +/**
>   * synps_edac_geterror_info - Get the current ecc error info
>   * @base:	Pointer to the base address of the ddr memory controller
>   * @p:		Pointer to the synopsys ecc status structure
> @@ -242,7 +265,8 @@ static void synps_edac_check(struct mem_ctl_info *mci)
>  	struct synps_edac_priv *priv = mci->pvt_info;
>  	int status;
>  
> -	status = synps_edac_geterror_info(priv->baseaddr, &priv->stat);
> +	status = priv->p_data->synps_edac_geterror_info(priv->baseaddr,
> +							&priv->stat);
>  	if (status)
>  		return;
>  
> @@ -372,10 +396,12 @@ static int synps_edac_init_csrows(struct mem_ctl_info *mci)
>  		for (j = 0; j < csi->nr_channels; j++) {
>  			dimm            = csi->channels[j]->dimm;
>  			dimm->edac_mode = EDAC_FLAG_SECDED;
> -			dimm->mtype     = synps_edac_get_mtype(priv->baseaddr);
> +			dimm->mtype     = priv->p_data->synps_edac_get_mtype(
> +						priv->baseaddr);
>  			dimm->nr_pages  = (size >> PAGE_SHIFT) / csi->nr_channels;
>  			dimm->grain     = SYNPS_EDAC_ERR_GRAIN;
> -			dimm->dtype     = synps_edac_get_dtype(priv->baseaddr);
> +			dimm->dtype     = priv->p_data->synps_edac_get_dtype(
> +						priv->baseaddr);
>  		}
>  	}
>  
> @@ -424,6 +450,21 @@ static int synps_edac_mc_init(struct mem_ctl_info *mci,
>  	return status;
>  }
>  
> +static const struct synps_platform_data zynq_edac_def = {
> +	.synps_edac_geterror_info	= synps_edac_geterror_info,
> +	.synps_edac_get_mtype		= synps_edac_get_mtype,
> +	.synps_edac_get_dtype		= synps_edac_get_dtype,
> +	.synps_edac_get_eccstate	= synps_edac_get_eccstate,
> +	.quirks				= 0,
> +};

Please make the actual function names and function pointer names
different. For example, the function pointer names don't need to have
the "synpc_" prefix as they're used all locally.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1705293 — Re: [PATCH 1/5] edac: synopsys: Add platform specific structures ddrc controller

On Mon, Aug 07, 2017 at 09:39:57AM +0200, Michal Simek wrote:
> > "This patch" in a commit message is tautologically redundant.
> 
> Fixed in v2.

You forgot to remove "This patch" in the remaining patches. But please
wait with sending v3 until I go through all of them.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web