[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

Added EDAC support for reporting the ecc errors of synopsys ddr controller.
The ddr ecc controller corrects single bit errors and detects double bit
errors.

Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
---
Changes for v3:
- Updated maintainer information
- Driver cleanup as per the review comments
- Shortened the prefix "sysnopsys" to "synps"
Changes for v2:
- Updated the commit header and message
- Renamed the filenames to synopsys_edac
- Corrected the compatilble string, commnets
- Renamed the macros,fucntions and data structures
---

 .../devicetree/bindings/edac/synps_edac.txt        |   18 +
 MAINTAINERS                                        |    1 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/Makefile                              |    1 +
 drivers/edac/synps_edac.c                          |  554 ++++++++++++++++++++
 5 files changed, 581 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/synps_edac.txt
 create mode 100644 drivers/edac/synps_edac.c

diff --git a/Documentation/devicetree/bindings/edac/synps_edac.txt b/Documentation/devicetree/bindings/edac/synps_edac.txt
new file mode 100644
index 0000000..c4a559b
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/synps_edac.txt
@@ -0,0 +1,18 @@
+Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are
+corrected and double bit ecc errors that are detected by the DDR ECC controller.
+ECC support for DDR is available in half-bus width(16 bit) configuration only.
+
+Required properties:
+- compatible: Should be "xlnx,zynq-ddrc-1.04"
+- reg: Should contain DDR controller registers location and length.
+
+Example:
+++++++++
+
+ddrc0: ddrc@f8006000 {
+	compatible = "xlnx,zynq-ddrc-1.04";
+	reg = <0xf8006000 0x1000>;
+};
+
+Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate
+control register.
diff --git a/MAINTAINERS b/MAINTAINERS
index d76e077..984b5a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1479,6 +1479,7 @@ N:	xilinx
 F:	drivers/clocksource/cadence_ttc_timer.c
 F:	drivers/i2c/busses/i2c-cadence.c
 F:	drivers/mmc/host/sdhci-of-arasan.c
+F:	drivers/edac/synps_edac.c
 
 ARM SMMU DRIVER
 M:	Will Deacon <will.deacon@arm.com>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..f628a2b 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
 	  Support for error detection and correction on the
 	  Cavium Octeon family of SOCs.
 
+config EDAC_SYNPS
+	tristate "Synopsys DDR Memory Controller"
+	depends on EDAC_MM_EDAC && ARCH_ZYNQ
+	help
+	  Support for EDAC on the ECC memory used with the Synopsys DDR
+	  memory controller.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 4154ed6..0af900f 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -64,3 +64,4 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
 obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
 obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
 obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
+obj-$(CONFIG_EDAC_SYNPS)		+= synps_edac.o
diff --git a/drivers/edac/synps_edac.c b/drivers/edac/synps_edac.c
new file mode 100644
index 0000000..a9417fe
--- /dev/null
+++ b/drivers/edac/synps_edac.c
@@ -0,0 +1,554 @@
+/*
+ * Synopsys DDR ECC Driver
+ * This driver is based on ppc4xx_edac.c drivers
+ *
+ * Copyright (C) 2012 - 2014 Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details
+ */
+
+#include <linux/edac.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "edac_core.h"
+
+/* Number of cs_rows needed per memory controller */
+#define SYNPS_EDAC_NR_CSROWS	1
+
+/* Number of channels per memory controller */
+#define SYNPS_EDAC_NR_CHANS	1
+
+/* Granularity of reported error in bytes */
+#define SYNPS_EDAC_ERR_GRAIN	1
+
+#define SYNPS_EDAC_MSG_SIZE	256
+
+#define SYNPS_EDAC_MOD_STRING	"synps_edac"
+#define SYNPS_EDAC_MOD_VER	"1"
+
+/* Synopsys DDR memory controller registers that are relevant to ECC */
+#define SYNPS_DDRC_CTRL_REG_OFST	0x0
+#define SYNPS_DDRC_T_ZQ_REG_OFST	0xA4
+
+/* ECC control register */
+#define SYNPS_DDRC_ECC_CTRL_REG_OFST		0xC4
+/* ECC log register */
+#define SYNPS_DDRC_ECC_CE_LOG_REG_OFST		0xC8
+/* ECC address register */
+#define SYNPS_DDRC_ECC_CE_ADDR_REG_OFST		0xCC
+/* ECC data[31:0] register */
+#define SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST	0xD0
+
+/* Uncorrectable error info regsisters */
+#define SYNPS_DDRC_ECC_UE_LOG_REG_OFST		0xDC
+#define SYNPS_DDRC_ECC_UE_ADDR_REG_OFST		0xE0
+#define SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST	0xE4
+
+#define SYNPS_DDRC_ECC_STAT_REG_OFST		0xF0
+#define SYNPS_DDRC_ECC_SCRUB_REG_OFST		0xF4
+
+/* Control regsiter bitfield definitions */
+#define SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK	0xC
+#define SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT	2
+
+#define SYNPS_DDRCTL_WDTH_16	1
+#define SYNPS_DDRCTL_WDTH_32	0
+
+/* ZQ register bitfield definitions */
+#define SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK	0x2
+
+/* ECC control register bitfield definitions */
+#define SYNPS_DDRC_ECCCTRL_CLR_CE_ERR		0x2
+#define SYNPS_DDRC_ECCCTRL_CLR_UE_ERR		0x1
+
+/* ECC correctable/uncorrectable error log register definitions */
+#define SYNPS_DDRC_ECC_CE_LOGREG_VALID		0x1
+#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK	0xFE
+#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT	1
+
+/* ECC correctable/uncorrectable error address register definitions */
+#define SYNPS_DDRC_ECC_ADDRREG_COL_MASK		0xFFF
+#define SYNPS_DDRC_ECC_ADDRREG_ROW_MASK		0xFFFF000
+#define SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT	12
+#define SYNPS_DDRC_ECC_ADDRREG_BANK_MASK	0x70000000
+#define SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT	28
+
+/* ECC statistic regsiter definitions */
+#define SYNPS_DDRC_ECC_STATREG_UECNT_MASK	0xFF
+#define SYNPS_DDRC_ECC_STATREG_CECNT_MASK	0xFF00
+#define SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT	8
+
+/* ECC scrub regsiter definitions */
+#define SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK	0x7
+#define SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED	0x4
+
+/**
+ * struct ecc_error_info - ECC error log information
+ * @row:	Row number
+ * @col:	Column number
+ * @bank:	Bank number
+ * @bitpos:	Bit position
+ * @data:	Data causing the error
+ */
+struct ecc_error_info {
+	u32 row;
+	u32 col;
+	u32 bank;
+	u32 bitpos;
+	u32 data;
+};
+
+/**
+ * struct synps_ecc_status - ECC status information to report
+ * @ce_count:	Correctable error count
+ * @ue_count:	Uncorrectable error count
+ * @ceinfo:	Correctable error log information
+ * @ueinfo:	Uncorrectable error log information
+ */
+struct synps_ecc_status {
+	u32 ce_cnt;
+	u32 ue_cnt;
+	struct ecc_error_info ceinfo;
+	struct ecc_error_info ueinfo;
+};
+
+/**
+ * struct synps_edac_priv - DDR memory controller private instance data
+ * @baseaddr:		Base address of the DDR controller
+ * @ce_count:		Correctable Error count
+ * @ue_count:		Uncorrectable Error count
+ */
+struct synps_edac_priv {
+	void __iomem *baseaddr;
+	u32 ce_cnt;
+	u32 ue_cnt;
+};
+
+/**
+ * synps_edac_geterror_info - Get the current ecc error info
+ * @base:	Pointer to the base address of the ddr memory controller
+ * @perrstatus:	Pointer to the synopsys ecc status structure
+ *
+ * This routine determines there is any ecc error or not
+ *
+ * Return: zero if there is no error otherwise returns 1
+ */
+static int synps_edac_geterror_info(void __iomem *base,
+				    struct synps_ecc_status *p)
+{
+	u32 regval;
+	u32 clearval = 0;
+
+	regval = readl(base + SYNPS_DDRC_ECC_STAT_REG_OFST) &
+			(SYNPS_DDRC_ECC_STATREG_UECNT_MASK |
+			SYNPS_DDRC_ECC_STATREG_CECNT_MASK);
+
+	if (!regval)
+		return 0;
+
+	memset(p, 0, sizeof(*p));
+
+	p->ce_cnt = (regval & SYNPS_DDRC_ECC_STATREG_CECNT_MASK) >>
+				SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT;
+	p->ue_cnt = regval & SYNPS_DDRC_ECC_STATREG_UECNT_MASK;
+
+	regval = readl(base + SYNPS_DDRC_ECC_CE_LOG_REG_OFST);
+	if (!(p->ce_cnt && (regval & SYNPS_DDRC_ECC_CE_LOGREG_VALID)))
+		goto ue_err;
+
+	p->ceinfo.bitpos = (regval & SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK) >>
+					SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT;
+	regval = readl(base + SYNPS_DDRC_ECC_CE_ADDR_REG_OFST);
+	p->ceinfo.row = (regval & SYNPS_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT;
+	p->ceinfo.col = regval & SYNPS_DDRC_ECC_ADDRREG_COL_MASK;
+	p->ceinfo.bank = (regval & SYNPS_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT;
+	p->ceinfo.data = readl(base + SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST);
+	edac_dbg(3, "ce bitposition: %d data: %d\n", p->ceinfo.bitpos,
+						p->ceinfo.data);
+	clearval = SYNPS_DDRC_ECCCTRL_CLR_CE_ERR;
+
+ue_err:
+	regval = readl(base + SYNPS_DDRC_ECC_UE_LOG_REG_OFST);
+	if (!(p->ue_cnt && (regval & SYNPS_DDRC_ECC_CE_LOGREG_VALID)))
+		goto out;
+
+	regval = readl(base + SYNPS_DDRC_ECC_UE_ADDR_REG_OFST);
+	p->ueinfo.row = (regval & SYNPS_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT;
+	p->ueinfo.col = regval & SYNPS_DDRC_ECC_ADDRREG_COL_MASK;
+	p->ueinfo.bank = (regval & SYNPS_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT;
+	p->ueinfo.data = readl(base + SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST);
+	clearval |= SYNPS_DDRC_ECCCTRL_CLR_UE_ERR;
+
+out:
+	writel(clearval, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+	writel(0x0, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+
+	return 1;
+}
+
+/**
+ * synps_edac_handle_error - Handle controller error types CE and UE
+ * @mci:	Pointer to the edac memory controller instance
+ * @perrstatus:	Pointer to the synopsys ecc status structure
+ *
+ * This routine handles the controller ECC correctable and un correctable
+ * error.
+ */
+static void synps_edac_handle_error(struct mem_ctl_info *mci,
+				    struct synps_ecc_status *p)
+{
+	char message[SYNPS_EDAC_MSG_SIZE];
+	struct ecc_error_info *pinf;
+
+	if (p->ce_cnt) {
+		pinf = &p->ceinfo;
+		snprintf(message, SYNPS_EDAC_MSG_SIZE,
+			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+			 "CE", pinf->row, pinf->bank, pinf->col);
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
+				     message, "");
+	}
+
+	if (p->ue_cnt) {
+		pinf = &p->ueinfo;
+		snprintf(message, SYNPS_EDAC_MSG_SIZE,
+			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+			 "UE", pinf->row, pinf->bank, pinf->col);
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
+				     message, "");
+	}
+}
+
+/**
+ * synps_edac_check - Check controller for ECC errors
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine is used to check and post ECC errors and is called by
+ * the EDAC polling thread
+ */
+static void synps_edac_check(struct mem_ctl_info *mci)
+{
+	struct synps_edac_priv *priv = mci->pvt_info;
+	struct synps_ecc_status stat;
+	int status;
+
+	status = synps_edac_geterror_info(priv->baseaddr, &stat);
+	if (!status)
+		return;
+
+	priv->ce_cnt += stat.ce_cnt;
+	priv->ue_cnt += stat.ue_cnt;
+	synps_edac_handle_error(mci, &stat);
+
+	edac_dbg(3, "Total error count ce %d ue %d\n",
+			 priv->ce_cnt, priv->ue_cnt);
+}
+
+/**
+ * synps_edac_get_dtype - Return the controller memory width
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the EDAC device type width appropriate for the
+ * current controller configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type synps_edac_get_dtype(const void __iomem *base)
+{
+	enum dev_type dt;
+	u32 width;
+
+	width = readl(base + SYNPS_DDRC_CTRL_REG_OFST);
+	width = (width & SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK) >>
+			SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT;
+
+	switch (width) {
+	case SYNPS_DDRCTL_WDTH_16:
+		dt = DEV_X2;
+		break;
+	case SYNPS_DDRCTL_WDTH_32:
+		dt = DEV_X4;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
+
+	return dt;
+}
+
+/**
+ * synps_edac_get_eccstate - Return the controller ecc enable/disable status
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the ECC enable/diable status for the controller
+ *
+ * Return: a ecc status boolean i.e true/false - enabled/disabled.
+ */
+static bool synps_edac_get_eccstate(void __iomem *base)
+{
+	enum dev_type dt;
+	u32 ecctype;
+	bool state = false;
+
+	dt = synps_edac_get_dtype(base);
+	if (dt == DEV_UNKNOWN)
+		return state;
+
+	ecctype = (readl(base + SYNPS_DDRC_ECC_SCRUB_REG_OFST) &
+			SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK);
+
+	if ((ecctype == SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED)
+			&& (dt == DEV_X2)) {
+		state = true;
+		writel(0x0, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+	}
+
+	return state;
+}
+
+/**
+ * synps_edac_get_memsize - reads the size of the attached memory device
+ *
+ * Return: the memory size in bytes
+ */
+static u32 synps_edac_get_memsize(void)
+{
+	struct sysinfo inf;
+
+	si_meminfo(&inf);
+
+	return inf.totalram * inf.mem_unit;
+}
+
+/**
+ * synps_edac_get_mtype - Returns controller memory type
+ * @base:	pointer to the synopsys ecc status structure
+ *
+ * This routine returns the EDAC memory type appropriate for the
+ * current controller configuration.
+ *
+ * Return: a memory type enumeration.
+ */
+static enum mem_type synps_edac_get_mtype(const void __iomem *base)
+{
+	enum mem_type mt;
+	u32 memtype;
+
+	memtype = readl(base + SYNPS_DDRC_T_ZQ_REG_OFST);
+
+	if (memtype & SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK)
+		mt = MEM_DDR3;
+	else
+		mt = MEM_DDR2;
+
+	return mt;
+}
+
+/**
+ * synps_edac_init_csrows - Initialize the cs row data
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine initializes the chip select rows associated
+ * with the EDAC memory controller instance
+ *
+ * Return: Unconditionally 0.
+ */
+static int synps_edac_init_csrows(struct mem_ctl_info *mci)
+{
+	struct csrow_info *csi;
+	struct dimm_info *dimm;
+	struct synps_edac_priv *priv = mci->pvt_info;
+	u32 size;
+	int row, j;
+
+	for (row = 0; row < mci->nr_csrows; row++) {
+		csi = mci->csrows[row];
+		size = synps_edac_get_memsize();
+
+		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->nr_pages =
+			    (size >> PAGE_SHIFT) / csi->nr_channels;
+			dimm->grain = SYNPS_EDAC_ERR_GRAIN;
+			dimm->dtype = synps_edac_get_dtype(priv->baseaddr);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * synps_edac_mc_init - Initialize driver instance
+ * @mci:	Pointer to the edac memory controller instance
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine performs initialization of the EDAC memory controller
+ * instance and related driver-private data associated with the
+ * memory controller the instance is bound to.
+ *
+ * Return: 0 if OK; otherwise, < 0 on error.
+ */
+static int synps_edac_mc_init(struct mem_ctl_info *mci,
+				 struct platform_device *pdev)
+{
+	int status;
+	struct synps_edac_priv *priv;
+
+	mci->pdev = &pdev->dev;
+	priv = mci->pvt_info;
+	platform_set_drvdata(pdev, mci);
+
+	/* Initialize controller capabilities and configuration */
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->scrub_cap = SCRUB_HW_SRC;
+	mci->scrub_mode = SCRUB_NONE;
+
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	mci->ctl_name = "synps_ddr_controller";
+	mci->dev_name = SYNPS_EDAC_MOD_STRING;
+	mci->mod_name = SYNPS_EDAC_MOD_VER;
+	mci->mod_ver = "1";
+
+	edac_op_state = EDAC_OPSTATE_POLL;
+	mci->edac_check = synps_edac_check;
+	mci->ctl_page_to_phys = NULL;
+
+	/*
+	 * Initialize the MC control structure 'csrows' table
+	 * with the mapping and control information.
+	 */
+	status = synps_edac_init_csrows(mci);
+
+	return status;
+}
+
+/**
+ * synps_edac_mc_probe - Check controller and bind driver
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine probes a specific controller
+ * instance for binding with the driver.
+ *
+ * Return: 0 if the controller instance was successfully bound to the
+ * driver; otherwise, < 0 on error.
+ */
+static int synps_edac_mc_probe(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[2];
+	struct synps_edac_priv *priv;
+	int rc;
+	struct resource *res;
+	void __iomem *baseaddr;
+
+	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) == false) {
+		edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
+		return -ENXIO;
+	}
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = SYNPS_EDAC_NR_CSROWS;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = SYNPS_EDAC_NR_CHANS;
+	layers[1].is_virt_csrow = false;
+
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
+			    sizeof(struct synps_edac_priv));
+	if (!mci) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed memory allocation for mc instance\n");
+		return -ENOMEM;
+	}
+
+	priv = mci->pvt_info;
+	priv->baseaddr = baseaddr;
+	rc = synps_edac_mc_init(mci, pdev);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed to initialize instance\n");
+		goto free_edac_mc;
+	}
+
+	rc = edac_mc_add_mc(mci);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed to register with EDAC core\n");
+		goto del_edac_mc;
+	}
+
+	return rc;
+
+del_edac_mc:
+	edac_mc_del_mc(&pdev->dev);
+free_edac_mc:
+	edac_mc_free(mci);
+
+	return rc;
+}
+
+/**
+ * synps_edac_mc_remove - Unbind driver from controller
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Return: Unconditionally 0
+ */
+static int synps_edac_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+
+	return 0;
+}
+
+static struct of_device_id synps_edac_match[] = {
+	{ .compatible = "xlnx,zynq-ddrc-1.04", },
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, synps_edac_match);
+
+static struct platform_driver synps_edac_mc_driver = {
+	.driver = {
+		   .name = "synps-edac",
+		   .owner = THIS_MODULE,
+		   .of_match_table = synps_edac_match,
+		   },
+	.probe = synps_edac_mc_probe,
+	.remove = synps_edac_mc_remove,
+};
+
+module_platform_driver(synps_edac_mc_driver);
+
+MODULE_AUTHOR("Xilinx Inc");
+MODULE_DESCRIPTION("Synopsys DDR ECC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.4

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

Added EDAC support for reporting the ecc errors of synopsys ddr controller.
The ddr ecc controller corrects single bit errors and detects double bit
errors.

Signed-off-by: Punnaiah Choudary Kalluri <punnaia-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
Changes for v3:
- Updated maintainer information
- Driver cleanup as per the review comments
- Shortened the prefix "sysnopsys" to "synps"
Changes for v2:
- Updated the commit header and message
- Renamed the filenames to synopsys_edac
- Corrected the compatilble string, commnets
- Renamed the macros,fucntions and data structures
---

 .../devicetree/bindings/edac/synps_edac.txt        |   18 +
 MAINTAINERS                                        |    1 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/Makefile                              |    1 +
 drivers/edac/synps_edac.c                          |  554 ++++++++++++++++++++
 5 files changed, 581 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/synps_edac.txt
 create mode 100644 drivers/edac/synps_edac.c

diff --git a/Documentation/devicetree/bindings/edac/synps_edac.txt b/Documentation/devicetree/bindings/edac/synps_edac.txt
new file mode 100644
index 0000000..c4a559b
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/synps_edac.txt
@@ -0,0 +1,18 @@
+Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are
+corrected and double bit ecc errors that are detected by the DDR ECC controller.
+ECC support for DDR is available in half-bus width(16 bit) configuration only.
+
+Required properties:
+- compatible: Should be "xlnx,zynq-ddrc-1.04"
+- reg: Should contain DDR controller registers location and length.
+
+Example:
+++++++++
+
+ddrc0: ddrc@f8006000 {
+	compatible = "xlnx,zynq-ddrc-1.04";
+	reg = <0xf8006000 0x1000>;
+};
+
+Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate
+control register.
diff --git a/MAINTAINERS b/MAINTAINERS
index d76e077..984b5a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1479,6 +1479,7 @@ N:	xilinx
 F:	drivers/clocksource/cadence_ttc_timer.c
 F:	drivers/i2c/busses/i2c-cadence.c
 F:	drivers/mmc/host/sdhci-of-arasan.c
+F:	drivers/edac/synps_edac.c
 
 ARM SMMU DRIVER
 M:	Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..f628a2b 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
 	  Support for error detection and correction on the
 	  Cavium Octeon family of SOCs.
 
+config EDAC_SYNPS
+	tristate "Synopsys DDR Memory Controller"
+	depends on EDAC_MM_EDAC && ARCH_ZYNQ
+	help
+	  Support for EDAC on the ECC memory used with the Synopsys DDR
+	  memory controller.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 4154ed6..0af900f 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -64,3 +64,4 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
 obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
 obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
 obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
+obj-$(CONFIG_EDAC_SYNPS)		+= synps_edac.o
diff --git a/drivers/edac/synps_edac.c b/drivers/edac/synps_edac.c
new file mode 100644
index 0000000..a9417fe
--- /dev/null
+++ b/drivers/edac/synps_edac.c
@@ -0,0 +1,554 @@
+/*
+ * Synopsys DDR ECC Driver
+ * This driver is based on ppc4xx_edac.c drivers
+ *
+ * Copyright (C) 2012 - 2014 Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details
+ */
+
+#include <linux/edac.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "edac_core.h"
+
+/* Number of cs_rows needed per memory controller */
+#define SYNPS_EDAC_NR_CSROWS	1
+
+/* Number of channels per memory controller */
+#define SYNPS_EDAC_NR_CHANS	1
+
+/* Granularity of reported error in bytes */
+#define SYNPS_EDAC_ERR_GRAIN	1
+
+#define SYNPS_EDAC_MSG_SIZE	256
+
+#define SYNPS_EDAC_MOD_STRING	"synps_edac"
+#define SYNPS_EDAC_MOD_VER	"1"
+
+/* Synopsys DDR memory controller registers that are relevant to ECC */
+#define SYNPS_DDRC_CTRL_REG_OFST	0x0
+#define SYNPS_DDRC_T_ZQ_REG_OFST	0xA4
+
+/* ECC control register */
+#define SYNPS_DDRC_ECC_CTRL_REG_OFST		0xC4
+/* ECC log register */
+#define SYNPS_DDRC_ECC_CE_LOG_REG_OFST		0xC8
+/* ECC address register */
+#define SYNPS_DDRC_ECC_CE_ADDR_REG_OFST		0xCC
+/* ECC data[31:0] register */
+#define SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST	0xD0
+
+/* Uncorrectable error info regsisters */
+#define SYNPS_DDRC_ECC_UE_LOG_REG_OFST		0xDC
+#define SYNPS_DDRC_ECC_UE_ADDR_REG_OFST		0xE0
+#define SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST	0xE4
+
+#define SYNPS_DDRC_ECC_STAT_REG_OFST		0xF0
+#define SYNPS_DDRC_ECC_SCRUB_REG_OFST		0xF4
+
+/* Control regsiter bitfield definitions */
+#define SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK	0xC
+#define SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT	2
+
+#define SYNPS_DDRCTL_WDTH_16	1
+#define SYNPS_DDRCTL_WDTH_32	0
+
+/* ZQ register bitfield definitions */
+#define SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK	0x2
+
+/* ECC control register bitfield definitions */
+#define SYNPS_DDRC_ECCCTRL_CLR_CE_ERR		0x2
+#define SYNPS_DDRC_ECCCTRL_CLR_UE_ERR		0x1
+
+/* ECC correctable/uncorrectable error log register definitions */
+#define SYNPS_DDRC_ECC_CE_LOGREG_VALID		0x1
+#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK	0xFE
+#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT	1
+
+/* ECC correctable/uncorrectable error address register definitions */
+#define SYNPS_DDRC_ECC_ADDRREG_COL_MASK		0xFFF
+#define SYNPS_DDRC_ECC_ADDRREG_ROW_MASK		0xFFFF000
+#define SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT	12
+#define SYNPS_DDRC_ECC_ADDRREG_BANK_MASK	0x70000000
+#define SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT	28
+
+/* ECC statistic regsiter definitions */
+#define SYNPS_DDRC_ECC_STATREG_UECNT_MASK	0xFF
+#define SYNPS_DDRC_ECC_STATREG_CECNT_MASK	0xFF00
+#define SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT	8
+
+/* ECC scrub regsiter definitions */
+#define SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK	0x7
+#define SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED	0x4
+
+/**
+ * struct ecc_error_info - ECC error log information
+ * @row:	Row number
+ * @col:	Column number
+ * @bank:	Bank number
+ * @bitpos:	Bit position
+ * @data:	Data causing the error
+ */
+struct ecc_error_info {
+	u32 row;
+	u32 col;
+	u32 bank;
+	u32 bitpos;
+	u32 data;
+};
+
+/**
+ * struct synps_ecc_status - ECC status information to report
+ * @ce_count:	Correctable error count
+ * @ue_count:	Uncorrectable error count
+ * @ceinfo:	Correctable error log information
+ * @ueinfo:	Uncorrectable error log information
+ */
+struct synps_ecc_status {
+	u32 ce_cnt;
+	u32 ue_cnt;
+	struct ecc_error_info ceinfo;
+	struct ecc_error_info ueinfo;
+};
+
+/**
+ * struct synps_edac_priv - DDR memory controller private instance data
+ * @baseaddr:		Base address of the DDR controller
+ * @ce_count:		Correctable Error count
+ * @ue_count:		Uncorrectable Error count
+ */
+struct synps_edac_priv {
+	void __iomem *baseaddr;
+	u32 ce_cnt;
+	u32 ue_cnt;
+};
+
+/**
+ * synps_edac_geterror_info - Get the current ecc error info
+ * @base:	Pointer to the base address of the ddr memory controller
+ * @perrstatus:	Pointer to the synopsys ecc status structure
+ *
+ * This routine determines there is any ecc error or not
+ *
+ * Return: zero if there is no error otherwise returns 1
+ */
+static int synps_edac_geterror_info(void __iomem *base,
+				    struct synps_ecc_status *p)
+{
+	u32 regval;
+	u32 clearval = 0;
+
+	regval = readl(base + SYNPS_DDRC_ECC_STAT_REG_OFST) &
+			(SYNPS_DDRC_ECC_STATREG_UECNT_MASK |
+			SYNPS_DDRC_ECC_STATREG_CECNT_MASK);
+
+	if (!regval)
+		return 0;
+
+	memset(p, 0, sizeof(*p));
+
+	p->ce_cnt = (regval & SYNPS_DDRC_ECC_STATREG_CECNT_MASK) >>
+				SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT;
+	p->ue_cnt = regval & SYNPS_DDRC_ECC_STATREG_UECNT_MASK;
+
+	regval = readl(base + SYNPS_DDRC_ECC_CE_LOG_REG_OFST);
+	if (!(p->ce_cnt && (regval & SYNPS_DDRC_ECC_CE_LOGREG_VALID)))
+		goto ue_err;
+
+	p->ceinfo.bitpos = (regval & SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK) >>
+					SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT;
+	regval = readl(base + SYNPS_DDRC_ECC_CE_ADDR_REG_OFST);
+	p->ceinfo.row = (regval & SYNPS_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT;
+	p->ceinfo.col = regval & SYNPS_DDRC_ECC_ADDRREG_COL_MASK;
+	p->ceinfo.bank = (regval & SYNPS_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT;
+	p->ceinfo.data = readl(base + SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST);
+	edac_dbg(3, "ce bitposition: %d data: %d\n", p->ceinfo.bitpos,
+						p->ceinfo.data);
+	clearval = SYNPS_DDRC_ECCCTRL_CLR_CE_ERR;
+
+ue_err:
+	regval = readl(base + SYNPS_DDRC_ECC_UE_LOG_REG_OFST);
+	if (!(p->ue_cnt && (regval & SYNPS_DDRC_ECC_CE_LOGREG_VALID)))
+		goto out;
+
+	regval = readl(base + SYNPS_DDRC_ECC_UE_ADDR_REG_OFST);
+	p->ueinfo.row = (regval & SYNPS_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT;
+	p->ueinfo.col = regval & SYNPS_DDRC_ECC_ADDRREG_COL_MASK;
+	p->ueinfo.bank = (regval & SYNPS_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT;
+	p->ueinfo.data = readl(base + SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST);
+	clearval |= SYNPS_DDRC_ECCCTRL_CLR_UE_ERR;
+
+out:
+	writel(clearval, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+	writel(0x0, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+
+	return 1;
+}
+
+/**
+ * synps_edac_handle_error - Handle controller error types CE and UE
+ * @mci:	Pointer to the edac memory controller instance
+ * @perrstatus:	Pointer to the synopsys ecc status structure
+ *
+ * This routine handles the controller ECC correctable and un correctable
+ * error.
+ */
+static void synps_edac_handle_error(struct mem_ctl_info *mci,
+				    struct synps_ecc_status *p)
+{
+	char message[SYNPS_EDAC_MSG_SIZE];
+	struct ecc_error_info *pinf;
+
+	if (p->ce_cnt) {
+		pinf = &p->ceinfo;
+		snprintf(message, SYNPS_EDAC_MSG_SIZE,
+			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+			 "CE", pinf->row, pinf->bank, pinf->col);
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
+				     message, "");
+	}
+
+	if (p->ue_cnt) {
+		pinf = &p->ueinfo;
+		snprintf(message, SYNPS_EDAC_MSG_SIZE,
+			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+			 "UE", pinf->row, pinf->bank, pinf->col);
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
+				     message, "");
+	}
+}
+
+/**
+ * synps_edac_check - Check controller for ECC errors
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine is used to check and post ECC errors and is called by
+ * the EDAC polling thread
+ */
+static void synps_edac_check(struct mem_ctl_info *mci)
+{
+	struct synps_edac_priv *priv = mci->pvt_info;
+	struct synps_ecc_status stat;
+	int status;
+
+	status = synps_edac_geterror_info(priv->baseaddr, &stat);
+	if (!status)
+		return;
+
+	priv->ce_cnt += stat.ce_cnt;
+	priv->ue_cnt += stat.ue_cnt;
+	synps_edac_handle_error(mci, &stat);
+
+	edac_dbg(3, "Total error count ce %d ue %d\n",
+			 priv->ce_cnt, priv->ue_cnt);
+}
+
+/**
+ * synps_edac_get_dtype - Return the controller memory width
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the EDAC device type width appropriate for the
+ * current controller configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type synps_edac_get_dtype(const void __iomem *base)
+{
+	enum dev_type dt;
+	u32 width;
+
+	width = readl(base + SYNPS_DDRC_CTRL_REG_OFST);
+	width = (width & SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK) >>
+			SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT;
+
+	switch (width) {
+	case SYNPS_DDRCTL_WDTH_16:
+		dt = DEV_X2;
+		break;
+	case SYNPS_DDRCTL_WDTH_32:
+		dt = DEV_X4;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
+
+	return dt;
+}
+
+/**
+ * synps_edac_get_eccstate - Return the controller ecc enable/disable status
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the ECC enable/diable status for the controller
+ *
+ * Return: a ecc status boolean i.e true/false - enabled/disabled.
+ */
+static bool synps_edac_get_eccstate(void __iomem *base)
+{
+	enum dev_type dt;
+	u32 ecctype;
+	bool state = false;
+
+	dt = synps_edac_get_dtype(base);
+	if (dt == DEV_UNKNOWN)
+		return state;
+
+	ecctype = (readl(base + SYNPS_DDRC_ECC_SCRUB_REG_OFST) &
+			SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK);
+
+	if ((ecctype == SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED)
+			&& (dt == DEV_X2)) {
+		state = true;
+		writel(0x0, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+	}
+
+	return state;
+}
+
+/**
+ * synps_edac_get_memsize - reads the size of the attached memory device
+ *
+ * Return: the memory size in bytes
+ */
+static u32 synps_edac_get_memsize(void)
+{
+	struct sysinfo inf;
+
+	si_meminfo(&inf);
+
+	return inf.totalram * inf.mem_unit;
+}
+
+/**
+ * synps_edac_get_mtype - Returns controller memory type
+ * @base:	pointer to the synopsys ecc status structure
+ *
+ * This routine returns the EDAC memory type appropriate for the
+ * current controller configuration.
+ *
+ * Return: a memory type enumeration.
+ */
+static enum mem_type synps_edac_get_mtype(const void __iomem *base)
+{
+	enum mem_type mt;
+	u32 memtype;
+
+	memtype = readl(base + SYNPS_DDRC_T_ZQ_REG_OFST);
+
+	if (memtype & SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK)
+		mt = MEM_DDR3;
+	else
+		mt = MEM_DDR2;
+
+	return mt;
+}
+
+/**
+ * synps_edac_init_csrows - Initialize the cs row data
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine initializes the chip select rows associated
+ * with the EDAC memory controller instance
+ *
+ * Return: Unconditionally 0.
+ */
+static int synps_edac_init_csrows(struct mem_ctl_info *mci)
+{
+	struct csrow_info *csi;
+	struct dimm_info *dimm;
+	struct synps_edac_priv *priv = mci->pvt_info;
+	u32 size;
+	int row, j;
+
+	for (row = 0; row < mci->nr_csrows; row++) {
+		csi = mci->csrows[row];
+		size = synps_edac_get_memsize();
+
+		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->nr_pages =
+			    (size >> PAGE_SHIFT) / csi->nr_channels;
+			dimm->grain = SYNPS_EDAC_ERR_GRAIN;
+			dimm->dtype = synps_edac_get_dtype(priv->baseaddr);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * synps_edac_mc_init - Initialize driver instance
+ * @mci:	Pointer to the edac memory controller instance
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine performs initialization of the EDAC memory controller
+ * instance and related driver-private data associated with the
+ * memory controller the instance is bound to.
+ *
+ * Return: 0 if OK; otherwise, < 0 on error.
+ */
+static int synps_edac_mc_init(struct mem_ctl_info *mci,
+				 struct platform_device *pdev)
+{
+	int status;
+	struct synps_edac_priv *priv;
+
+	mci->pdev = &pdev->dev;
+	priv = mci->pvt_info;
+	platform_set_drvdata(pdev, mci);
+
+	/* Initialize controller capabilities and configuration */
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->scrub_cap = SCRUB_HW_SRC;
+	mci->scrub_mode = SCRUB_NONE;
+
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	mci->ctl_name = "synps_ddr_controller";
+	mci->dev_name = SYNPS_EDAC_MOD_STRING;
+	mci->mod_name = SYNPS_EDAC_MOD_VER;
+	mci->mod_ver = "1";
+
+	edac_op_state = EDAC_OPSTATE_POLL;
+	mci->edac_check = synps_edac_check;
+	mci->ctl_page_to_phys = NULL;
+
+	/*
+	 * Initialize the MC control structure 'csrows' table
+	 * with the mapping and control information.
+	 */
+	status = synps_edac_init_csrows(mci);
+
+	return status;
+}
+
+/**
+ * synps_edac_mc_probe - Check controller and bind driver
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine probes a specific controller
+ * instance for binding with the driver.
+ *
+ * Return: 0 if the controller instance was successfully bound to the
+ * driver; otherwise, < 0 on error.
+ */
+static int synps_edac_mc_probe(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[2];
+	struct synps_edac_priv *priv;
+	int rc;
+	struct resource *res;
+	void __iomem *baseaddr;
+
+	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) == false) {
+		edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
+		return -ENXIO;
+	}
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = SYNPS_EDAC_NR_CSROWS;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = SYNPS_EDAC_NR_CHANS;
+	layers[1].is_virt_csrow = false;
+
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
+			    sizeof(struct synps_edac_priv));
+	if (!mci) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed memory allocation for mc instance\n");
+		return -ENOMEM;
+	}
+
+	priv = mci->pvt_info;
+	priv->baseaddr = baseaddr;
+	rc = synps_edac_mc_init(mci, pdev);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed to initialize instance\n");
+		goto free_edac_mc;
+	}
+
+	rc = edac_mc_add_mc(mci);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed to register with EDAC core\n");
+		goto del_edac_mc;
+	}
+
+	return rc;
+
+del_edac_mc:
+	edac_mc_del_mc(&pdev->dev);
+free_edac_mc:
+	edac_mc_free(mci);
+
+	return rc;
+}
+
+/**
+ * synps_edac_mc_remove - Unbind driver from controller
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Return: Unconditionally 0
+ */
+static int synps_edac_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+
+	return 0;
+}
+
+static struct of_device_id synps_edac_match[] = {
+	{ .compatible = "xlnx,zynq-ddrc-1.04", },
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, synps_edac_match);
+
+static struct platform_driver synps_edac_mc_driver = {
+	.driver = {
+		   .name = "synps-edac",
+		   .owner = THIS_MODULE,
+		   .of_match_table = synps_edac_match,
+		   },
+	.probe = synps_edac_mc_probe,
+	.remove = synps_edac_mc_remove,
+};
+
+module_platform_driver(synps_edac_mc_driver);
+
+MODULE_AUTHOR("Xilinx Inc");
+MODULE_DESCRIPTION("Synopsys DDR ECC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.4


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

Added EDAC support for reporting the ecc errors of synopsys ddr controller.
The ddr ecc controller corrects single bit errors and detects double bit
errors.

Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
---
Changes for v3:
- Updated maintainer information
- Driver cleanup as per the review comments
- Shortened the prefix "sysnopsys" to "synps"
Changes for v2:
- Updated the commit header and message
- Renamed the filenames to synopsys_edac
- Corrected the compatilble string, commnets
- Renamed the macros,fucntions and data structures
---

 .../devicetree/bindings/edac/synps_edac.txt        |   18 +
 MAINTAINERS                                        |    1 +
 drivers/edac/Kconfig                               |    7 +
 drivers/edac/Makefile                              |    1 +
 drivers/edac/synps_edac.c                          |  554 ++++++++++++++++++++
 5 files changed, 581 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/edac/synps_edac.txt
 create mode 100644 drivers/edac/synps_edac.c

diff --git a/Documentation/devicetree/bindings/edac/synps_edac.txt b/Documentation/devicetree/bindings/edac/synps_edac.txt
new file mode 100644
index 0000000..c4a559b
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/synps_edac.txt
@@ -0,0 +1,18 @@
+Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are
+corrected and double bit ecc errors that are detected by the DDR ECC controller.
+ECC support for DDR is available in half-bus width(16 bit) configuration only.
+
+Required properties:
+- compatible: Should be "xlnx,zynq-ddrc-1.04"
+- reg: Should contain DDR controller registers location and length.
+
+Example:
+++++++++
+
+ddrc0: ddrc at f8006000 {
+	compatible = "xlnx,zynq-ddrc-1.04";
+	reg = <0xf8006000 0x1000>;
+};
+
+Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate
+control register.
diff --git a/MAINTAINERS b/MAINTAINERS
index d76e077..984b5a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1479,6 +1479,7 @@ N:	xilinx
 F:	drivers/clocksource/cadence_ttc_timer.c
 F:	drivers/i2c/busses/i2c-cadence.c
 F:	drivers/mmc/host/sdhci-of-arasan.c
+F:	drivers/edac/synps_edac.c
 
 ARM SMMU DRIVER
 M:	Will Deacon <will.deacon@arm.com>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..f628a2b 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
 	  Support for error detection and correction on the
 	  Cavium Octeon family of SOCs.
 
+config EDAC_SYNPS
+	tristate "Synopsys DDR Memory Controller"
+	depends on EDAC_MM_EDAC && ARCH_ZYNQ
+	help
+	  Support for EDAC on the ECC memory used with the Synopsys DDR
+	  memory controller.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 4154ed6..0af900f 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -64,3 +64,4 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
 obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
 obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
 obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
+obj-$(CONFIG_EDAC_SYNPS)		+= synps_edac.o
diff --git a/drivers/edac/synps_edac.c b/drivers/edac/synps_edac.c
new file mode 100644
index 0000000..a9417fe
--- /dev/null
+++ b/drivers/edac/synps_edac.c
@@ -0,0 +1,554 @@
+/*
+ * Synopsys DDR ECC Driver
+ * This driver is based on ppc4xx_edac.c drivers
+ *
+ * Copyright (C) 2012 - 2014 Xilinx, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details
+ */
+
+#include <linux/edac.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include "edac_core.h"
+
+/* Number of cs_rows needed per memory controller */
+#define SYNPS_EDAC_NR_CSROWS	1
+
+/* Number of channels per memory controller */
+#define SYNPS_EDAC_NR_CHANS	1
+
+/* Granularity of reported error in bytes */
+#define SYNPS_EDAC_ERR_GRAIN	1
+
+#define SYNPS_EDAC_MSG_SIZE	256
+
+#define SYNPS_EDAC_MOD_STRING	"synps_edac"
+#define SYNPS_EDAC_MOD_VER	"1"
+
+/* Synopsys DDR memory controller registers that are relevant to ECC */
+#define SYNPS_DDRC_CTRL_REG_OFST	0x0
+#define SYNPS_DDRC_T_ZQ_REG_OFST	0xA4
+
+/* ECC control register */
+#define SYNPS_DDRC_ECC_CTRL_REG_OFST		0xC4
+/* ECC log register */
+#define SYNPS_DDRC_ECC_CE_LOG_REG_OFST		0xC8
+/* ECC address register */
+#define SYNPS_DDRC_ECC_CE_ADDR_REG_OFST		0xCC
+/* ECC data[31:0] register */
+#define SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST	0xD0
+
+/* Uncorrectable error info regsisters */
+#define SYNPS_DDRC_ECC_UE_LOG_REG_OFST		0xDC
+#define SYNPS_DDRC_ECC_UE_ADDR_REG_OFST		0xE0
+#define SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST	0xE4
+
+#define SYNPS_DDRC_ECC_STAT_REG_OFST		0xF0
+#define SYNPS_DDRC_ECC_SCRUB_REG_OFST		0xF4
+
+/* Control regsiter bitfield definitions */
+#define SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK	0xC
+#define SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT	2
+
+#define SYNPS_DDRCTL_WDTH_16	1
+#define SYNPS_DDRCTL_WDTH_32	0
+
+/* ZQ register bitfield definitions */
+#define SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK	0x2
+
+/* ECC control register bitfield definitions */
+#define SYNPS_DDRC_ECCCTRL_CLR_CE_ERR		0x2
+#define SYNPS_DDRC_ECCCTRL_CLR_UE_ERR		0x1
+
+/* ECC correctable/uncorrectable error log register definitions */
+#define SYNPS_DDRC_ECC_CE_LOGREG_VALID		0x1
+#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK	0xFE
+#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT	1
+
+/* ECC correctable/uncorrectable error address register definitions */
+#define SYNPS_DDRC_ECC_ADDRREG_COL_MASK		0xFFF
+#define SYNPS_DDRC_ECC_ADDRREG_ROW_MASK		0xFFFF000
+#define SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT	12
+#define SYNPS_DDRC_ECC_ADDRREG_BANK_MASK	0x70000000
+#define SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT	28
+
+/* ECC statistic regsiter definitions */
+#define SYNPS_DDRC_ECC_STATREG_UECNT_MASK	0xFF
+#define SYNPS_DDRC_ECC_STATREG_CECNT_MASK	0xFF00
+#define SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT	8
+
+/* ECC scrub regsiter definitions */
+#define SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK	0x7
+#define SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED	0x4
+
+/**
+ * struct ecc_error_info - ECC error log information
+ * @row:	Row number
+ * @col:	Column number
+ * @bank:	Bank number
+ * @bitpos:	Bit position
+ * @data:	Data causing the error
+ */
+struct ecc_error_info {
+	u32 row;
+	u32 col;
+	u32 bank;
+	u32 bitpos;
+	u32 data;
+};
+
+/**
+ * struct synps_ecc_status - ECC status information to report
+ * @ce_count:	Correctable error count
+ * @ue_count:	Uncorrectable error count
+ * @ceinfo:	Correctable error log information
+ * @ueinfo:	Uncorrectable error log information
+ */
+struct synps_ecc_status {
+	u32 ce_cnt;
+	u32 ue_cnt;
+	struct ecc_error_info ceinfo;
+	struct ecc_error_info ueinfo;
+};
+
+/**
+ * struct synps_edac_priv - DDR memory controller private instance data
+ * @baseaddr:		Base address of the DDR controller
+ * @ce_count:		Correctable Error count
+ * @ue_count:		Uncorrectable Error count
+ */
+struct synps_edac_priv {
+	void __iomem *baseaddr;
+	u32 ce_cnt;
+	u32 ue_cnt;
+};
+
+/**
+ * synps_edac_geterror_info - Get the current ecc error info
+ * @base:	Pointer to the base address of the ddr memory controller
+ * @perrstatus:	Pointer to the synopsys ecc status structure
+ *
+ * This routine determines there is any ecc error or not
+ *
+ * Return: zero if there is no error otherwise returns 1
+ */
+static int synps_edac_geterror_info(void __iomem *base,
+				    struct synps_ecc_status *p)
+{
+	u32 regval;
+	u32 clearval = 0;
+
+	regval = readl(base + SYNPS_DDRC_ECC_STAT_REG_OFST) &
+			(SYNPS_DDRC_ECC_STATREG_UECNT_MASK |
+			SYNPS_DDRC_ECC_STATREG_CECNT_MASK);
+
+	if (!regval)
+		return 0;
+
+	memset(p, 0, sizeof(*p));
+
+	p->ce_cnt = (regval & SYNPS_DDRC_ECC_STATREG_CECNT_MASK) >>
+				SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT;
+	p->ue_cnt = regval & SYNPS_DDRC_ECC_STATREG_UECNT_MASK;
+
+	regval = readl(base + SYNPS_DDRC_ECC_CE_LOG_REG_OFST);
+	if (!(p->ce_cnt && (regval & SYNPS_DDRC_ECC_CE_LOGREG_VALID)))
+		goto ue_err;
+
+	p->ceinfo.bitpos = (regval & SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK) >>
+					SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT;
+	regval = readl(base + SYNPS_DDRC_ECC_CE_ADDR_REG_OFST);
+	p->ceinfo.row = (regval & SYNPS_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT;
+	p->ceinfo.col = regval & SYNPS_DDRC_ECC_ADDRREG_COL_MASK;
+	p->ceinfo.bank = (regval & SYNPS_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT;
+	p->ceinfo.data = readl(base + SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST);
+	edac_dbg(3, "ce bitposition: %d data: %d\n", p->ceinfo.bitpos,
+						p->ceinfo.data);
+	clearval = SYNPS_DDRC_ECCCTRL_CLR_CE_ERR;
+
+ue_err:
+	regval = readl(base + SYNPS_DDRC_ECC_UE_LOG_REG_OFST);
+	if (!(p->ue_cnt && (regval & SYNPS_DDRC_ECC_CE_LOGREG_VALID)))
+		goto out;
+
+	regval = readl(base + SYNPS_DDRC_ECC_UE_ADDR_REG_OFST);
+	p->ueinfo.row = (regval & SYNPS_DDRC_ECC_ADDRREG_ROW_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT;
+	p->ueinfo.col = regval & SYNPS_DDRC_ECC_ADDRREG_COL_MASK;
+	p->ueinfo.bank = (regval & SYNPS_DDRC_ECC_ADDRREG_BANK_MASK) >>
+					SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT;
+	p->ueinfo.data = readl(base + SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST);
+	clearval |= SYNPS_DDRC_ECCCTRL_CLR_UE_ERR;
+
+out:
+	writel(clearval, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+	writel(0x0, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+
+	return 1;
+}
+
+/**
+ * synps_edac_handle_error - Handle controller error types CE and UE
+ * @mci:	Pointer to the edac memory controller instance
+ * @perrstatus:	Pointer to the synopsys ecc status structure
+ *
+ * This routine handles the controller ECC correctable and un correctable
+ * error.
+ */
+static void synps_edac_handle_error(struct mem_ctl_info *mci,
+				    struct synps_ecc_status *p)
+{
+	char message[SYNPS_EDAC_MSG_SIZE];
+	struct ecc_error_info *pinf;
+
+	if (p->ce_cnt) {
+		pinf = &p->ceinfo;
+		snprintf(message, SYNPS_EDAC_MSG_SIZE,
+			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+			 "CE", pinf->row, pinf->bank, pinf->col);
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+				     p->ce_cnt, 0, 0, 0, 0, 0, -1,
+				     message, "");
+	}
+
+	if (p->ue_cnt) {
+		pinf = &p->ueinfo;
+		snprintf(message, SYNPS_EDAC_MSG_SIZE,
+			 "DDR ECC error type :%s Row %d Bank %d Col %d ",
+			 "UE", pinf->row, pinf->bank, pinf->col);
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+				     p->ue_cnt, 0, 0, 0, 0, 0, -1,
+				     message, "");
+	}
+}
+
+/**
+ * synps_edac_check - Check controller for ECC errors
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine is used to check and post ECC errors and is called by
+ * the EDAC polling thread
+ */
+static void synps_edac_check(struct mem_ctl_info *mci)
+{
+	struct synps_edac_priv *priv = mci->pvt_info;
+	struct synps_ecc_status stat;
+	int status;
+
+	status = synps_edac_geterror_info(priv->baseaddr, &stat);
+	if (!status)
+		return;
+
+	priv->ce_cnt += stat.ce_cnt;
+	priv->ue_cnt += stat.ue_cnt;
+	synps_edac_handle_error(mci, &stat);
+
+	edac_dbg(3, "Total error count ce %d ue %d\n",
+			 priv->ce_cnt, priv->ue_cnt);
+}
+
+/**
+ * synps_edac_get_dtype - Return the controller memory width
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the EDAC device type width appropriate for the
+ * current controller configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type synps_edac_get_dtype(const void __iomem *base)
+{
+	enum dev_type dt;
+	u32 width;
+
+	width = readl(base + SYNPS_DDRC_CTRL_REG_OFST);
+	width = (width & SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK) >>
+			SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT;
+
+	switch (width) {
+	case SYNPS_DDRCTL_WDTH_16:
+		dt = DEV_X2;
+		break;
+	case SYNPS_DDRCTL_WDTH_32:
+		dt = DEV_X4;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
+
+	return dt;
+}
+
+/**
+ * synps_edac_get_eccstate - Return the controller ecc enable/disable status
+ * @base:	Pointer to the ddr memory contoller base address
+ *
+ * This routine returns the ECC enable/diable status for the controller
+ *
+ * Return: a ecc status boolean i.e true/false - enabled/disabled.
+ */
+static bool synps_edac_get_eccstate(void __iomem *base)
+{
+	enum dev_type dt;
+	u32 ecctype;
+	bool state = false;
+
+	dt = synps_edac_get_dtype(base);
+	if (dt == DEV_UNKNOWN)
+		return state;
+
+	ecctype = (readl(base + SYNPS_DDRC_ECC_SCRUB_REG_OFST) &
+			SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK);
+
+	if ((ecctype == SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED)
+			&& (dt == DEV_X2)) {
+		state = true;
+		writel(0x0, base + SYNPS_DDRC_ECC_CTRL_REG_OFST);
+	}
+
+	return state;
+}
+
+/**
+ * synps_edac_get_memsize - reads the size of the attached memory device
+ *
+ * Return: the memory size in bytes
+ */
+static u32 synps_edac_get_memsize(void)
+{
+	struct sysinfo inf;
+
+	si_meminfo(&inf);
+
+	return inf.totalram * inf.mem_unit;
+}
+
+/**
+ * synps_edac_get_mtype - Returns controller memory type
+ * @base:	pointer to the synopsys ecc status structure
+ *
+ * This routine returns the EDAC memory type appropriate for the
+ * current controller configuration.
+ *
+ * Return: a memory type enumeration.
+ */
+static enum mem_type synps_edac_get_mtype(const void __iomem *base)
+{
+	enum mem_type mt;
+	u32 memtype;
+
+	memtype = readl(base + SYNPS_DDRC_T_ZQ_REG_OFST);
+
+	if (memtype & SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK)
+		mt = MEM_DDR3;
+	else
+		mt = MEM_DDR2;
+
+	return mt;
+}
+
+/**
+ * synps_edac_init_csrows - Initialize the cs row data
+ * @mci:	Pointer to the edac memory controller instance
+ *
+ * This routine initializes the chip select rows associated
+ * with the EDAC memory controller instance
+ *
+ * Return: Unconditionally 0.
+ */
+static int synps_edac_init_csrows(struct mem_ctl_info *mci)
+{
+	struct csrow_info *csi;
+	struct dimm_info *dimm;
+	struct synps_edac_priv *priv = mci->pvt_info;
+	u32 size;
+	int row, j;
+
+	for (row = 0; row < mci->nr_csrows; row++) {
+		csi = mci->csrows[row];
+		size = synps_edac_get_memsize();
+
+		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->nr_pages =
+			    (size >> PAGE_SHIFT) / csi->nr_channels;
+			dimm->grain = SYNPS_EDAC_ERR_GRAIN;
+			dimm->dtype = synps_edac_get_dtype(priv->baseaddr);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * synps_edac_mc_init - Initialize driver instance
+ * @mci:	Pointer to the edac memory controller instance
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine performs initialization of the EDAC memory controller
+ * instance and related driver-private data associated with the
+ * memory controller the instance is bound to.
+ *
+ * Return: 0 if OK; otherwise, < 0 on error.
+ */
+static int synps_edac_mc_init(struct mem_ctl_info *mci,
+				 struct platform_device *pdev)
+{
+	int status;
+	struct synps_edac_priv *priv;
+
+	mci->pdev = &pdev->dev;
+	priv = mci->pvt_info;
+	platform_set_drvdata(pdev, mci);
+
+	/* Initialize controller capabilities and configuration */
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->scrub_cap = SCRUB_HW_SRC;
+	mci->scrub_mode = SCRUB_NONE;
+
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	mci->ctl_name = "synps_ddr_controller";
+	mci->dev_name = SYNPS_EDAC_MOD_STRING;
+	mci->mod_name = SYNPS_EDAC_MOD_VER;
+	mci->mod_ver = "1";
+
+	edac_op_state = EDAC_OPSTATE_POLL;
+	mci->edac_check = synps_edac_check;
+	mci->ctl_page_to_phys = NULL;
+
+	/*
+	 * Initialize the MC control structure 'csrows' table
+	 * with the mapping and control information.
+	 */
+	status = synps_edac_init_csrows(mci);
+
+	return status;
+}
+
+/**
+ * synps_edac_mc_probe - Check controller and bind driver
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * This routine probes a specific controller
+ * instance for binding with the driver.
+ *
+ * Return: 0 if the controller instance was successfully bound to the
+ * driver; otherwise, < 0 on error.
+ */
+static int synps_edac_mc_probe(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[2];
+	struct synps_edac_priv *priv;
+	int rc;
+	struct resource *res;
+	void __iomem *baseaddr;
+
+	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) == false) {
+		edac_printk(KERN_INFO, EDAC_MC, "ECC not enabled\n");
+		return -ENXIO;
+	}
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = SYNPS_EDAC_NR_CSROWS;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = SYNPS_EDAC_NR_CHANS;
+	layers[1].is_virt_csrow = false;
+
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
+			    sizeof(struct synps_edac_priv));
+	if (!mci) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed memory allocation for mc instance\n");
+		return -ENOMEM;
+	}
+
+	priv = mci->pvt_info;
+	priv->baseaddr = baseaddr;
+	rc = synps_edac_mc_init(mci, pdev);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed to initialize instance\n");
+		goto free_edac_mc;
+	}
+
+	rc = edac_mc_add_mc(mci);
+	if (rc) {
+		edac_printk(KERN_ERR, EDAC_MC,
+			"Failed to register with EDAC core\n");
+		goto del_edac_mc;
+	}
+
+	return rc;
+
+del_edac_mc:
+	edac_mc_del_mc(&pdev->dev);
+free_edac_mc:
+	edac_mc_free(mci);
+
+	return rc;
+}
+
+/**
+ * synps_edac_mc_remove - Unbind driver from controller
+ * @pdev:	Pointer to the platform_device struct
+ *
+ * Return: Unconditionally 0
+ */
+static int synps_edac_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+
+	return 0;
+}
+
+static struct of_device_id synps_edac_match[] = {
+	{ .compatible = "xlnx,zynq-ddrc-1.04", },
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, synps_edac_match);
+
+static struct platform_driver synps_edac_mc_driver = {
+	.driver = {
+		   .name = "synps-edac",
+		   .owner = THIS_MODULE,
+		   .of_match_table = synps_edac_match,
+		   },
+	.probe = synps_edac_mc_probe,
+	.remove = synps_edac_mc_remove,
+};
+
+module_platform_driver(synps_edac_mc_driver);
+
+MODULE_AUTHOR("Xilinx Inc");
+MODULE_DESCRIPTION("Synopsys DDR ECC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.4

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Sun, Jul 27, 2014 at 12:10:52AM +0530, Punnaiah Choudary Kalluri wrote:
> Added EDAC support for reporting the ecc errors of synopsys ddr controller.
> The ddr ecc controller corrects single bit errors and detects double bit
> errors.
> 
> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> ---
> Changes for v3:
> - Updated maintainer information
> - Driver cleanup as per the review comments
> - Shortened the prefix "sysnopsys" to "synps"

This is not a good idea, IMO. "synopsys" is much more understandable
instead of "synps". And synopsys-edac is just fine.

> Changes for v2:
> - Updated the commit header and message
> - Renamed the filenames to synopsys_edac
> - Corrected the compatilble string, commnets
> - Renamed the macros,fucntions and data structures
> ---
> 
>  .../devicetree/bindings/edac/synps_edac.txt        |   18 +
>  MAINTAINERS                                        |    1 +
>  drivers/edac/Kconfig                               |    7 +
>  drivers/edac/Makefile                              |    1 +
>  drivers/edac/synps_edac.c                          |  554 ++++++++++++++++++++
>  5 files changed, 581 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/edac/synps_edac.txt
>  create mode 100644 drivers/edac/synps_edac.c
> 
> diff --git a/Documentation/devicetree/bindings/edac/synps_edac.txt b/Documentation/devicetree/bindings/edac/synps_edac.txt
> new file mode 100644
> index 0000000..c4a559b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/synps_edac.txt
> @@ -0,0 +1,18 @@
> +Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are
> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
> +
> +Required properties:
> +- compatible: Should be "xlnx,zynq-ddrc-1.04"
> +- reg: Should contain DDR controller registers location and length.
> +
> +Example:
> +++++++++
> +
> +ddrc0: ddrc@f8006000 {
> +	compatible = "xlnx,zynq-ddrc-1.04";
> +	reg = <0xf8006000 0x1000>;
> +};

I'd need an ack from device tree people about the DT bits.

> +
> +Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate
> +control register.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d76e077..984b5a7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1479,6 +1479,7 @@ N:	xilinx
>  F:	drivers/clocksource/cadence_ttc_timer.c
>  F:	drivers/i2c/busses/i2c-cadence.c
>  F:	drivers/mmc/host/sdhci-of-arasan.c
> +F:	drivers/edac/synps_edac.c
>  
>  ARM SMMU DRIVER
>  M:	Will Deacon <will.deacon@arm.com>
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 878f090..f628a2b 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
>  	  Support for error detection and correction on the
>  	  Cavium Octeon family of SOCs.
>  
> +config EDAC_SYNPS
> +	tristate "Synopsys DDR Memory Controller"
> +	depends on EDAC_MM_EDAC && ARCH_ZYNQ
> +	help
> +	  Support for EDAC on the ECC memory used with the Synopsys DDR
> +	  memory controller.
> +
>  endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 4154ed6..0af900f 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -64,3 +64,4 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
>  obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
>  obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
>  obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
> +obj-$(CONFIG_EDAC_SYNPS)		+= synps_edac.o
> diff --git a/drivers/edac/synps_edac.c b/drivers/edac/synps_edac.c
> new file mode 100644
> index 0000000..a9417fe
> --- /dev/null
> +++ b/drivers/edac/synps_edac.c
> @@ -0,0 +1,554 @@
> +/*
> + * Synopsys DDR ECC Driver
> + * This driver is based on ppc4xx_edac.c drivers
> + *
> + * Copyright (C) 2012 - 2014 Xilinx, Inc.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details
> + */
> +
> +#include <linux/edac.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "edac_core.h"
> +
> +/* Number of cs_rows needed per memory controller */
> +#define SYNPS_EDAC_NR_CSROWS	1
> +
> +/* Number of channels per memory controller */
> +#define SYNPS_EDAC_NR_CHANS	1
> +
> +/* Granularity of reported error in bytes */
> +#define SYNPS_EDAC_ERR_GRAIN	1
> +
> +#define SYNPS_EDAC_MSG_SIZE	256
> +
> +#define SYNPS_EDAC_MOD_STRING	"synps_edac"
> +#define SYNPS_EDAC_MOD_VER	"1"
> +
> +/* Synopsys DDR memory controller registers that are relevant to ECC */
> +#define SYNPS_DDRC_CTRL_REG_OFST	0x0
> +#define SYNPS_DDRC_T_ZQ_REG_OFST	0xA4
> +
> +/* ECC control register */
> +#define SYNPS_DDRC_ECC_CTRL_REG_OFST		0xC4
> +/* ECC log register */
> +#define SYNPS_DDRC_ECC_CE_LOG_REG_OFST		0xC8
> +/* ECC address register */
> +#define SYNPS_DDRC_ECC_CE_ADDR_REG_OFST		0xCC
> +/* ECC data[31:0] register */
> +#define SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST	0xD0
> +
> +/* Uncorrectable error info regsisters */
> +#define SYNPS_DDRC_ECC_UE_LOG_REG_OFST		0xDC
> +#define SYNPS_DDRC_ECC_UE_ADDR_REG_OFST		0xE0
> +#define SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST	0xE4
> +
> +#define SYNPS_DDRC_ECC_STAT_REG_OFST		0xF0
> +#define SYNPS_DDRC_ECC_SCRUB_REG_OFST		0xF4
> +
> +/* Control regsiter bitfield definitions */
> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK	0xC
> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT	2
> +
> +#define SYNPS_DDRCTL_WDTH_16	1
> +#define SYNPS_DDRCTL_WDTH_32	0
> +
> +/* ZQ register bitfield definitions */
> +#define SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK	0x2
> +
> +/* ECC control register bitfield definitions */
> +#define SYNPS_DDRC_ECCCTRL_CLR_CE_ERR		0x2
> +#define SYNPS_DDRC_ECCCTRL_CLR_UE_ERR		0x1
> +
> +/* ECC correctable/uncorrectable error log register definitions */
> +#define SYNPS_DDRC_ECC_CE_LOGREG_VALID		0x1
> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK	0xFE
> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT	1
> +
> +/* ECC correctable/uncorrectable error address register definitions */
> +#define SYNPS_DDRC_ECC_ADDRREG_COL_MASK		0xFFF
> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_MASK		0xFFFF000
> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT	12
> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_MASK	0x70000000
> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT	28
> +
> +/* ECC statistic regsiter definitions */
> +#define SYNPS_DDRC_ECC_STATREG_UECNT_MASK	0xFF
> +#define SYNPS_DDRC_ECC_STATREG_CECNT_MASK	0xFF00
> +#define SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT	8
> +
> +/* ECC scrub regsiter definitions */
> +#define SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK	0x7
> +#define SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED	0x4

Now those are certainly too long: having the
SYNPS_EDAC/SYNPS_DDRC{,_ECC}* prefix for defines used only in this file
is certainly making the code unreadable. Here's what it could look like,
for example:

     regval = readl(base + STAT_OFST) & (STAT_UECNT_MASK | STAT_CECNT_MASK);

so you have STAT which is the register name, followed by a name of the
bit or bits. I think this is perfectly fine instead of those heavy names
which all start with the same prefix and you have to go look at the end
to find out what they actually are.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Sun, Jul 27, 2014 at 12:10:52AM +0530, Punnaiah Choudary Kalluri wrote:
> Added EDAC support for reporting the ecc errors of synopsys ddr controller.
> The ddr ecc controller corrects single bit errors and detects double bit
> errors.
> 
> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
> ---
> Changes for v3:
> - Updated maintainer information
> - Driver cleanup as per the review comments
> - Shortened the prefix "sysnopsys" to "synps"

This is not a good idea, IMO. "synopsys" is much more understandable
instead of "synps". And synopsys-edac is just fine.

> Changes for v2:
> - Updated the commit header and message
> - Renamed the filenames to synopsys_edac
> - Corrected the compatilble string, commnets
> - Renamed the macros,fucntions and data structures
> ---
> 
>  .../devicetree/bindings/edac/synps_edac.txt        |   18 +
>  MAINTAINERS                                        |    1 +
>  drivers/edac/Kconfig                               |    7 +
>  drivers/edac/Makefile                              |    1 +
>  drivers/edac/synps_edac.c                          |  554 ++++++++++++++++++++
>  5 files changed, 581 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/edac/synps_edac.txt
>  create mode 100644 drivers/edac/synps_edac.c
> 
> diff --git a/Documentation/devicetree/bindings/edac/synps_edac.txt b/Documentation/devicetree/bindings/edac/synps_edac.txt
> new file mode 100644
> index 0000000..c4a559b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/edac/synps_edac.txt
> @@ -0,0 +1,18 @@
> +Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are
> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
> +
> +Required properties:
> +- compatible: Should be "xlnx,zynq-ddrc-1.04"
> +- reg: Should contain DDR controller registers location and length.
> +
> +Example:
> +++++++++
> +
> +ddrc0: ddrc at f8006000 {
> +	compatible = "xlnx,zynq-ddrc-1.04";
> +	reg = <0xf8006000 0x1000>;
> +};

I'd need an ack from device tree people about the DT bits.

> +
> +Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate
> +control register.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d76e077..984b5a7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1479,6 +1479,7 @@ N:	xilinx
>  F:	drivers/clocksource/cadence_ttc_timer.c
>  F:	drivers/i2c/busses/i2c-cadence.c
>  F:	drivers/mmc/host/sdhci-of-arasan.c
> +F:	drivers/edac/synps_edac.c
>  
>  ARM SMMU DRIVER
>  M:	Will Deacon <will.deacon@arm.com>
> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
> index 878f090..f628a2b 100644
> --- a/drivers/edac/Kconfig
> +++ b/drivers/edac/Kconfig
> @@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
>  	  Support for error detection and correction on the
>  	  Cavium Octeon family of SOCs.
>  
> +config EDAC_SYNPS
> +	tristate "Synopsys DDR Memory Controller"
> +	depends on EDAC_MM_EDAC && ARCH_ZYNQ
> +	help
> +	  Support for EDAC on the ECC memory used with the Synopsys DDR
> +	  memory controller.
> +
>  endif # EDAC
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index 4154ed6..0af900f 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -64,3 +64,4 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
>  obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
>  obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
>  obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
> +obj-$(CONFIG_EDAC_SYNPS)		+= synps_edac.o
> diff --git a/drivers/edac/synps_edac.c b/drivers/edac/synps_edac.c
> new file mode 100644
> index 0000000..a9417fe
> --- /dev/null
> +++ b/drivers/edac/synps_edac.c
> @@ -0,0 +1,554 @@
> +/*
> + * Synopsys DDR ECC Driver
> + * This driver is based on ppc4xx_edac.c drivers
> + *
> + * Copyright (C) 2012 - 2014 Xilinx, Inc.
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details
> + */
> +
> +#include <linux/edac.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include "edac_core.h"
> +
> +/* Number of cs_rows needed per memory controller */
> +#define SYNPS_EDAC_NR_CSROWS	1
> +
> +/* Number of channels per memory controller */
> +#define SYNPS_EDAC_NR_CHANS	1
> +
> +/* Granularity of reported error in bytes */
> +#define SYNPS_EDAC_ERR_GRAIN	1
> +
> +#define SYNPS_EDAC_MSG_SIZE	256
> +
> +#define SYNPS_EDAC_MOD_STRING	"synps_edac"
> +#define SYNPS_EDAC_MOD_VER	"1"
> +
> +/* Synopsys DDR memory controller registers that are relevant to ECC */
> +#define SYNPS_DDRC_CTRL_REG_OFST	0x0
> +#define SYNPS_DDRC_T_ZQ_REG_OFST	0xA4
> +
> +/* ECC control register */
> +#define SYNPS_DDRC_ECC_CTRL_REG_OFST		0xC4
> +/* ECC log register */
> +#define SYNPS_DDRC_ECC_CE_LOG_REG_OFST		0xC8
> +/* ECC address register */
> +#define SYNPS_DDRC_ECC_CE_ADDR_REG_OFST		0xCC
> +/* ECC data[31:0] register */
> +#define SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST	0xD0
> +
> +/* Uncorrectable error info regsisters */
> +#define SYNPS_DDRC_ECC_UE_LOG_REG_OFST		0xDC
> +#define SYNPS_DDRC_ECC_UE_ADDR_REG_OFST		0xE0
> +#define SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST	0xE4
> +
> +#define SYNPS_DDRC_ECC_STAT_REG_OFST		0xF0
> +#define SYNPS_DDRC_ECC_SCRUB_REG_OFST		0xF4
> +
> +/* Control regsiter bitfield definitions */
> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK	0xC
> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT	2
> +
> +#define SYNPS_DDRCTL_WDTH_16	1
> +#define SYNPS_DDRCTL_WDTH_32	0
> +
> +/* ZQ register bitfield definitions */
> +#define SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK	0x2
> +
> +/* ECC control register bitfield definitions */
> +#define SYNPS_DDRC_ECCCTRL_CLR_CE_ERR		0x2
> +#define SYNPS_DDRC_ECCCTRL_CLR_UE_ERR		0x1
> +
> +/* ECC correctable/uncorrectable error log register definitions */
> +#define SYNPS_DDRC_ECC_CE_LOGREG_VALID		0x1
> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK	0xFE
> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT	1
> +
> +/* ECC correctable/uncorrectable error address register definitions */
> +#define SYNPS_DDRC_ECC_ADDRREG_COL_MASK		0xFFF
> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_MASK		0xFFFF000
> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT	12
> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_MASK	0x70000000
> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT	28
> +
> +/* ECC statistic regsiter definitions */
> +#define SYNPS_DDRC_ECC_STATREG_UECNT_MASK	0xFF
> +#define SYNPS_DDRC_ECC_STATREG_CECNT_MASK	0xFF00
> +#define SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT	8
> +
> +/* ECC scrub regsiter definitions */
> +#define SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK	0x7
> +#define SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED	0x4

Now those are certainly too long: having the
SYNPS_EDAC/SYNPS_DDRC{,_ECC}* prefix for defines used only in this file
is certainly making the code unreadable. Here's what it could look like,
for example:

     regval = readl(base + STAT_OFST) & (STAT_UECNT_MASK | STAT_CECNT_MASK);

so you have STAT which is the register name, followed by a name of the
bit or bits. I think this is perfectly fine instead of those heavy names
which all start with the same prefix and you have to go look at the end
to find out what they actually are.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[punnaiah choudary kalluri]

Hi Boris,

On Mon, Jul 28, 2014 at 5:04 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Jul 27, 2014 at 12:10:52AM +0530, Punnaiah Choudary Kalluri wrote:
>> Added EDAC support for reporting the ecc errors of synopsys ddr controller.
>> The ddr ecc controller corrects single bit errors and detects double bit
>> errors.
>>
>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>> ---
>> Changes for v3:
>> - Updated maintainer information
>> - Driver cleanup as per the review comments
>> - Shortened the prefix "sysnopsys" to "synps"
>
> This is not a good idea, IMO. "synopsys" is much more understandable
> instead of "synps". And synopsys-edac is just fine.

Ok. I thought of keeping file name and function name prefixes in sync
will be good and shortening
the prefix would save alignment issues at some places.

As you said i will revert the file name and still i will use "synps"
as prefix for functions.
Hope this will be ok for you.

>
>> Changes for v2:
>> - Updated the commit header and message
>> - Renamed the filenames to synopsys_edac
>> - Corrected the compatilble string, commnets
>> - Renamed the macros,fucntions and data structures
>> ---
>>
>>  .../devicetree/bindings/edac/synps_edac.txt        |   18 +
>>  MAINTAINERS                                        |    1 +
>>  drivers/edac/Kconfig                               |    7 +
>>  drivers/edac/Makefile                              |    1 +
>>  drivers/edac/synps_edac.c                          |  554 ++++++++++++++++++++
>>  5 files changed, 581 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/edac/synps_edac.txt
>>  create mode 100644 drivers/edac/synps_edac.c
>>
>> diff --git a/Documentation/devicetree/bindings/edac/synps_edac.txt b/Documentation/devicetree/bindings/edac/synps_edac.txt
>> new file mode 100644
>> index 0000000..c4a559b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/edac/synps_edac.txt
>> @@ -0,0 +1,18 @@
>> +Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are
>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
>> +
>> +Required properties:
>> +- compatible: Should be "xlnx,zynq-ddrc-1.04"
>> +- reg: Should contain DDR controller registers location and length.
>> +
>> +Example:
>> +++++++++
>> +
>> +ddrc0: ddrc@f8006000 {
>> +     compatible = "xlnx,zynq-ddrc-1.04";
>> +     reg = <0xf8006000 0x1000>;
>> +};
>
> I'd need an ack from device tree people about the DT bits.
>
>> +
>> +Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate
>> +control register.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d76e077..984b5a7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1479,6 +1479,7 @@ N:      xilinx
>>  F:   drivers/clocksource/cadence_ttc_timer.c
>>  F:   drivers/i2c/busses/i2c-cadence.c
>>  F:   drivers/mmc/host/sdhci-of-arasan.c
>> +F:   drivers/edac/synps_edac.c
>>
>>  ARM SMMU DRIVER
>>  M:   Will Deacon <will.deacon@arm.com>
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index 878f090..f628a2b 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
>>         Support for error detection and correction on the
>>         Cavium Octeon family of SOCs.
>>
>> +config EDAC_SYNPS
>> +     tristate "Synopsys DDR Memory Controller"
>> +     depends on EDAC_MM_EDAC && ARCH_ZYNQ
>> +     help
>> +       Support for EDAC on the ECC memory used with the Synopsys DDR
>> +       memory controller.
>> +
>>  endif # EDAC
>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
>> index 4154ed6..0af900f 100644
>> --- a/drivers/edac/Makefile
>> +++ b/drivers/edac/Makefile
>> @@ -64,3 +64,4 @@ obj-$(CONFIG_EDAC_OCTEON_PC)                += octeon_edac-pc.o
>>  obj-$(CONFIG_EDAC_OCTEON_L2C)                += octeon_edac-l2c.o
>>  obj-$(CONFIG_EDAC_OCTEON_LMC)                += octeon_edac-lmc.o
>>  obj-$(CONFIG_EDAC_OCTEON_PCI)                += octeon_edac-pci.o
>> +obj-$(CONFIG_EDAC_SYNPS)             += synps_edac.o
>> diff --git a/drivers/edac/synps_edac.c b/drivers/edac/synps_edac.c
>> new file mode 100644
>> index 0000000..a9417fe
>> --- /dev/null
>> +++ b/drivers/edac/synps_edac.c
>> @@ -0,0 +1,554 @@
>> +/*
>> + * Synopsys DDR ECC Driver
>> + * This driver is based on ppc4xx_edac.c drivers
>> + *
>> + * Copyright (C) 2012 - 2014 Xilinx, Inc.
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License.  See the file "COPYING" in the main directory of this archive
>> + * for more details
>> + */
>> +
>> +#include <linux/edac.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "edac_core.h"
>> +
>> +/* Number of cs_rows needed per memory controller */
>> +#define SYNPS_EDAC_NR_CSROWS 1
>> +
>> +/* Number of channels per memory controller */
>> +#define SYNPS_EDAC_NR_CHANS  1
>> +
>> +/* Granularity of reported error in bytes */
>> +#define SYNPS_EDAC_ERR_GRAIN 1
>> +
>> +#define SYNPS_EDAC_MSG_SIZE  256
>> +
>> +#define SYNPS_EDAC_MOD_STRING        "synps_edac"
>> +#define SYNPS_EDAC_MOD_VER   "1"
>> +
>> +/* Synopsys DDR memory controller registers that are relevant to ECC */
>> +#define SYNPS_DDRC_CTRL_REG_OFST     0x0
>> +#define SYNPS_DDRC_T_ZQ_REG_OFST     0xA4
>> +
>> +/* ECC control register */
>> +#define SYNPS_DDRC_ECC_CTRL_REG_OFST         0xC4
>> +/* ECC log register */
>> +#define SYNPS_DDRC_ECC_CE_LOG_REG_OFST               0xC8
>> +/* ECC address register */
>> +#define SYNPS_DDRC_ECC_CE_ADDR_REG_OFST              0xCC
>> +/* ECC data[31:0] register */
>> +#define SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST 0xD0
>> +
>> +/* Uncorrectable error info regsisters */
>> +#define SYNPS_DDRC_ECC_UE_LOG_REG_OFST               0xDC
>> +#define SYNPS_DDRC_ECC_UE_ADDR_REG_OFST              0xE0
>> +#define SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST 0xE4
>> +
>> +#define SYNPS_DDRC_ECC_STAT_REG_OFST         0xF0
>> +#define SYNPS_DDRC_ECC_SCRUB_REG_OFST                0xF4
>> +
>> +/* Control regsiter bitfield definitions */
>> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK     0xC
>> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT    2
>> +
>> +#define SYNPS_DDRCTL_WDTH_16 1
>> +#define SYNPS_DDRCTL_WDTH_32 0
>> +
>> +/* ZQ register bitfield definitions */
>> +#define SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK     0x2
>> +
>> +/* ECC control register bitfield definitions */
>> +#define SYNPS_DDRC_ECCCTRL_CLR_CE_ERR                0x2
>> +#define SYNPS_DDRC_ECCCTRL_CLR_UE_ERR                0x1
>> +
>> +/* ECC correctable/uncorrectable error log register definitions */
>> +#define SYNPS_DDRC_ECC_CE_LOGREG_VALID               0x1
>> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK 0xFE
>> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT        1
>> +
>> +/* ECC correctable/uncorrectable error address register definitions */
>> +#define SYNPS_DDRC_ECC_ADDRREG_COL_MASK              0xFFF
>> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_MASK              0xFFFF000
>> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT     12
>> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_MASK     0x70000000
>> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT    28
>> +
>> +/* ECC statistic regsiter definitions */
>> +#define SYNPS_DDRC_ECC_STATREG_UECNT_MASK    0xFF
>> +#define SYNPS_DDRC_ECC_STATREG_CECNT_MASK    0xFF00
>> +#define SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT   8
>> +
>> +/* ECC scrub regsiter definitions */
>> +#define SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK        0x7
>> +#define SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED       0x4
>
> Now those are certainly too long: having the
> SYNPS_EDAC/SYNPS_DDRC{,_ECC}* prefix for defines used only in this file
> is certainly making the code unreadable. Here's what it could look like,
> for example:
>
>      regval = readl(base + STAT_OFST) & (STAT_UECNT_MASK | STAT_CECNT_MASK);
>
> so you have STAT which is the register name, followed by a name of the
> bit or bits. I think this is perfectly fine instead of those heavy names
> which all start with the same prefix and you have to go look at the end
> to find out what they actually are.

I can agree with you that we can use shorter names.But ZYNQ has
programmable logic next to
processing system where one can add soft memory controller in the same
system and may use
different driver. So, the edac driver for zynq may include multiple
drivers for different
memory controllers in the same file. In this case it is difficult to
maintain generic macros as you
suggested above.

Also the current edac framework for edac memory controllers is
expecting the mc_num from the
driver while allocating the memory controller instance using the
edac_mc_alloc function. This
requirement mandates that if the system contains two different memory
controllers then the
corresponding  edac drivers should be in single file.

Regards,
Punnaiah
>
> --
> Regards/Gruss,
>     Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[punnaiah choudary kalluri]

Hi Boris,

On Mon, Jul 28, 2014 at 5:04 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Sun, Jul 27, 2014 at 12:10:52AM +0530, Punnaiah Choudary Kalluri wrote:
>> Added EDAC support for reporting the ecc errors of synopsys ddr controller.
>> The ddr ecc controller corrects single bit errors and detects double bit
>> errors.
>>
>> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>> ---
>> Changes for v3:
>> - Updated maintainer information
>> - Driver cleanup as per the review comments
>> - Shortened the prefix "sysnopsys" to "synps"
>
> This is not a good idea, IMO. "synopsys" is much more understandable
> instead of "synps". And synopsys-edac is just fine.

Ok. I thought of keeping file name and function name prefixes in sync
will be good and shortening
the prefix would save alignment issues at some places.

As you said i will revert the file name and still i will use "synps"
as prefix for functions.
Hope this will be ok for you.

>
>> Changes for v2:
>> - Updated the commit header and message
>> - Renamed the filenames to synopsys_edac
>> - Corrected the compatilble string, commnets
>> - Renamed the macros,fucntions and data structures
>> ---
>>
>>  .../devicetree/bindings/edac/synps_edac.txt        |   18 +
>>  MAINTAINERS                                        |    1 +
>>  drivers/edac/Kconfig                               |    7 +
>>  drivers/edac/Makefile                              |    1 +
>>  drivers/edac/synps_edac.c                          |  554 ++++++++++++++++++++
>>  5 files changed, 581 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/edac/synps_edac.txt
>>  create mode 100644 drivers/edac/synps_edac.c
>>
>> diff --git a/Documentation/devicetree/bindings/edac/synps_edac.txt b/Documentation/devicetree/bindings/edac/synps_edac.txt
>> new file mode 100644
>> index 0000000..c4a559b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/edac/synps_edac.txt
>> @@ -0,0 +1,18 @@
>> +Synopsys EDAC driver, it does reports the DDR ECC single bit errors that are
>> +corrected and double bit ecc errors that are detected by the DDR ECC controller.
>> +ECC support for DDR is available in half-bus width(16 bit) configuration only.
>> +
>> +Required properties:
>> +- compatible: Should be "xlnx,zynq-ddrc-1.04"
>> +- reg: Should contain DDR controller registers location and length.
>> +
>> +Example:
>> +++++++++
>> +
>> +ddrc0: ddrc at f8006000 {
>> +     compatible = "xlnx,zynq-ddrc-1.04";
>> +     reg = <0xf8006000 0x1000>;
>> +};
>
> I'd need an ack from device tree people about the DT bits.
>
>> +
>> +Synopsys EDAC driver detects the DDR ECC enable state by reading the appropriate
>> +control register.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index d76e077..984b5a7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1479,6 +1479,7 @@ N:      xilinx
>>  F:   drivers/clocksource/cadence_ttc_timer.c
>>  F:   drivers/i2c/busses/i2c-cadence.c
>>  F:   drivers/mmc/host/sdhci-of-arasan.c
>> +F:   drivers/edac/synps_edac.c
>>
>>  ARM SMMU DRIVER
>>  M:   Will Deacon <will.deacon@arm.com>
>> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
>> index 878f090..f628a2b 100644
>> --- a/drivers/edac/Kconfig
>> +++ b/drivers/edac/Kconfig
>> @@ -368,4 +368,11 @@ config EDAC_OCTEON_PCI
>>         Support for error detection and correction on the
>>         Cavium Octeon family of SOCs.
>>
>> +config EDAC_SYNPS
>> +     tristate "Synopsys DDR Memory Controller"
>> +     depends on EDAC_MM_EDAC && ARCH_ZYNQ
>> +     help
>> +       Support for EDAC on the ECC memory used with the Synopsys DDR
>> +       memory controller.
>> +
>>  endif # EDAC
>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
>> index 4154ed6..0af900f 100644
>> --- a/drivers/edac/Makefile
>> +++ b/drivers/edac/Makefile
>> @@ -64,3 +64,4 @@ obj-$(CONFIG_EDAC_OCTEON_PC)                += octeon_edac-pc.o
>>  obj-$(CONFIG_EDAC_OCTEON_L2C)                += octeon_edac-l2c.o
>>  obj-$(CONFIG_EDAC_OCTEON_LMC)                += octeon_edac-lmc.o
>>  obj-$(CONFIG_EDAC_OCTEON_PCI)                += octeon_edac-pci.o
>> +obj-$(CONFIG_EDAC_SYNPS)             += synps_edac.o
>> diff --git a/drivers/edac/synps_edac.c b/drivers/edac/synps_edac.c
>> new file mode 100644
>> index 0000000..a9417fe
>> --- /dev/null
>> +++ b/drivers/edac/synps_edac.c
>> @@ -0,0 +1,554 @@
>> +/*
>> + * Synopsys DDR ECC Driver
>> + * This driver is based on ppc4xx_edac.c drivers
>> + *
>> + * Copyright (C) 2012 - 2014 Xilinx, Inc.
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License.  See the file "COPYING" in the main directory of this archive
>> + * for more details
>> + */
>> +
>> +#include <linux/edac.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include "edac_core.h"
>> +
>> +/* Number of cs_rows needed per memory controller */
>> +#define SYNPS_EDAC_NR_CSROWS 1
>> +
>> +/* Number of channels per memory controller */
>> +#define SYNPS_EDAC_NR_CHANS  1
>> +
>> +/* Granularity of reported error in bytes */
>> +#define SYNPS_EDAC_ERR_GRAIN 1
>> +
>> +#define SYNPS_EDAC_MSG_SIZE  256
>> +
>> +#define SYNPS_EDAC_MOD_STRING        "synps_edac"
>> +#define SYNPS_EDAC_MOD_VER   "1"
>> +
>> +/* Synopsys DDR memory controller registers that are relevant to ECC */
>> +#define SYNPS_DDRC_CTRL_REG_OFST     0x0
>> +#define SYNPS_DDRC_T_ZQ_REG_OFST     0xA4
>> +
>> +/* ECC control register */
>> +#define SYNPS_DDRC_ECC_CTRL_REG_OFST         0xC4
>> +/* ECC log register */
>> +#define SYNPS_DDRC_ECC_CE_LOG_REG_OFST               0xC8
>> +/* ECC address register */
>> +#define SYNPS_DDRC_ECC_CE_ADDR_REG_OFST              0xCC
>> +/* ECC data[31:0] register */
>> +#define SYNPS_DDRC_ECC_CE_DATA_31_0_REG_OFST 0xD0
>> +
>> +/* Uncorrectable error info regsisters */
>> +#define SYNPS_DDRC_ECC_UE_LOG_REG_OFST               0xDC
>> +#define SYNPS_DDRC_ECC_UE_ADDR_REG_OFST              0xE0
>> +#define SYNPS_DDRC_ECC_UE_DATA_31_0_REG_OFST 0xE4
>> +
>> +#define SYNPS_DDRC_ECC_STAT_REG_OFST         0xF0
>> +#define SYNPS_DDRC_ECC_SCRUB_REG_OFST                0xF4
>> +
>> +/* Control regsiter bitfield definitions */
>> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_MASK     0xC
>> +#define SYNPS_DDRC_CTRLREG_BUSWIDTH_SHIFT    2
>> +
>> +#define SYNPS_DDRCTL_WDTH_16 1
>> +#define SYNPS_DDRCTL_WDTH_32 0
>> +
>> +/* ZQ register bitfield definitions */
>> +#define SYNPS_DDRC_T_ZQ_REG_DDRMODE_MASK     0x2
>> +
>> +/* ECC control register bitfield definitions */
>> +#define SYNPS_DDRC_ECCCTRL_CLR_CE_ERR                0x2
>> +#define SYNPS_DDRC_ECCCTRL_CLR_UE_ERR                0x1
>> +
>> +/* ECC correctable/uncorrectable error log register definitions */
>> +#define SYNPS_DDRC_ECC_CE_LOGREG_VALID               0x1
>> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_MASK 0xFE
>> +#define SYNPS_DDRC_ECC_CE_LOGREG_BITPOS_SHIFT        1
>> +
>> +/* ECC correctable/uncorrectable error address register definitions */
>> +#define SYNPS_DDRC_ECC_ADDRREG_COL_MASK              0xFFF
>> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_MASK              0xFFFF000
>> +#define SYNPS_DDRC_ECC_ADDRREG_ROW_SHIFT     12
>> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_MASK     0x70000000
>> +#define SYNPS_DDRC_ECC_ADDRREG_BANK_SHIFT    28
>> +
>> +/* ECC statistic regsiter definitions */
>> +#define SYNPS_DDRC_ECC_STATREG_UECNT_MASK    0xFF
>> +#define SYNPS_DDRC_ECC_STATREG_CECNT_MASK    0xFF00
>> +#define SYNPS_DDRC_ECC_STATREG_CECNT_SHIFT   8
>> +
>> +/* ECC scrub regsiter definitions */
>> +#define SYNPS_DDRC_ECC_SCRUBREG_ECC_MODE_MASK        0x7
>> +#define SYNPS_DDRC_ECC_SCRUBREG_ECCMODE_SECDED       0x4
>
> Now those are certainly too long: having the
> SYNPS_EDAC/SYNPS_DDRC{,_ECC}* prefix for defines used only in this file
> is certainly making the code unreadable. Here's what it could look like,
> for example:
>
>      regval = readl(base + STAT_OFST) & (STAT_UECNT_MASK | STAT_CECNT_MASK);
>
> so you have STAT which is the register name, followed by a name of the
> bit or bits. I think this is perfectly fine instead of those heavy names
> which all start with the same prefix and you have to go look at the end
> to find out what they actually are.

I can agree with you that we can use shorter names.But ZYNQ has
programmable logic next to
processing system where one can add soft memory controller in the same
system and may use
different driver. So, the edac driver for zynq may include multiple
drivers for different
memory controllers in the same file. In this case it is difficult to
maintain generic macros as you
suggested above.

Also the current edac framework for edac memory controllers is
expecting the mc_num from the
driver while allocating the memory controller instance using the
edac_mc_alloc function. This
requirement mandates that if the system contains two different memory
controllers then the
corresponding  edac drivers should be in single file.

Regards,
Punnaiah
>
> --
> Regards/Gruss,
>     Boris.
>
> Sent from a fat crate under my desk. Formatting is fine.
> --

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote:
> I can agree with you that we can use shorter names.But ZYNQ has
> programmable logic next to processing system where one can add soft
> memory controller in the same system and may use different driver. So,
> the edac driver for zynq may include multiple drivers for different
> memory controllers in the same file. In this case it is difficult to
> maintain generic macros as you suggested above.

So?

You can still shorten them a bit - the current names are awfully long.
SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR
and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever
you wanna call it prefix and then the rest. I.e.,

    SYNPS_<reg>_<bits*>
    ZYNQ_<reg>_bits*>

You can even use single letter prefixes like S_ and Z_ and add a comment
explaining what those mean. Still much more readable than the long
repeating prefixes currently.

> Also the current edac framework for edac memory controllers is
> expecting the mc_num from the driver while allocating the memory
> controller instance using the edac_mc_alloc function. This requirement
> mandates that if the system contains two different memory controllers
> then the corresponding edac drivers should be in single file.

So you're telling me that you want one edac driver for *two* memory
controllers which can be present on a single system *at* *the* *same*
*time*? Is that it?

How exactly is that topology supposed to look like, work, etc, etc? What
kind of error reporting do you imagine you want to do with EDAC?

IOW, more info would be appreciated.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote:
> I can agree with you that we can use shorter names.But ZYNQ has
> programmable logic next to processing system where one can add soft
> memory controller in the same system and may use different driver. So,
> the edac driver for zynq may include multiple drivers for different
> memory controllers in the same file. In this case it is difficult to
> maintain generic macros as you suggested above.

So?

You can still shorten them a bit - the current names are awfully long.
SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR
and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever
you wanna call it prefix and then the rest. I.e.,

    SYNPS_<reg>_<bits*>
    ZYNQ_<reg>_bits*>

You can even use single letter prefixes like S_ and Z_ and add a comment
explaining what those mean. Still much more readable than the long
repeating prefixes currently.

> Also the current edac framework for edac memory controllers is
> expecting the mc_num from the driver while allocating the memory
> controller instance using the edac_mc_alloc function. This requirement
> mandates that if the system contains two different memory controllers
> then the corresponding edac drivers should be in single file.

So you're telling me that you want one edac driver for *two* memory
controllers which can be present on a single system *at* *the* *same*
*time*? Is that it?

How exactly is that topology supposed to look like, work, etc, etc? What
kind of error reporting do you imagine you want to do with EDAC?

IOW, more info would be appreciated.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

RE: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3603 bytes --]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Thursday, July 31, 2014 7:27 PM
>To: Michal Simek
>Cc: Punnaiah Choudary Kalluri; dougthompson@xmission.com;
>robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
>ijc+devicetree@hellion.org.uk; Kumar Gala; Rob Landley;
>devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>edac@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>Vemula
>Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>ecc controller
>
>On Thu, Jul 31, 2014 at 03:36:35PM +0200, Michal Simek wrote:
>> Mixing functions for two/more different controller seems to me
>> really messy.
>
>Just stating that something is "really messy" without giving at least
>one technical reason for it is not going to get you any further. So let
>me give you mine:
>
>I think it is not messy because one file contains all the EDAC code for
>that system. Like every other system out there uses one EDAC driver.
>Everything is nicely contained in one place, any communication between
>the two memory controllers is done internally in that driver.
>
>Having two drivers would make that much more complicated. But hey, if
>you want to make your life more complicated because it is not enough
>complicated, feel free to do so. Just make sure you don't impose any
>restrictions on the rest of the subsystem.

Here we are also considering reusability of the driver. Let's say if some other
SOC contains Synopsys ddr controller then this driver can be reused. In this case,
Having separate driver would be good. But as you said in your previous comment,
We will talk about this later. 

Thanks,
Punnaiah
>
>> > -ENOPARSE for this sentence.
>>
>> I meant by this this scenario - please correct me if I am completely wrong
>> because I am not playing with this subsystem. My expectation is that
>> memory controller number should be same all the time.
>>
>> two memory controllers with the same edac driver.
>>
>> Normal calling sequence:
>> X probe - number 0
>> Y probe - number 1
>>
>> Different order/name (I think order is taken at the first place)
>> in DTS should caused that X will have number 1 and Y number 0.
>>
>> Another scenario:
>> two controllers and one with external gpio reset for example.
>>
>> X probe is called but gpio controller is not ready and gpio driver returns
>> -EPROBE_DEFER then Y probe is called and get number 0. Then X probe is
>called
>> again and get number 1.
>
>Right, since you have a heterogeneous design, you'd need to account
>for all that in your driver and deal with probing order. Like having a
>way to ask a controller what kind of a controller it is and so on. I'm
>afraid I don't know anything about your design to be of more help there.
>
>However, doing this in one <name>_edac.c file is almost trivial than
>doing it with two separate drivers. Unless you don't need to do any
>meaningful communication between the PS and PL. Then it doesn't matter.
>
>If I were you, I'd spend a more time on a proper design now so that all
>my work later is easy. Adding two drivers just because it seems messy is
>not a very good idea. What I would do is analyze all my use cases and
>come up with a proper design then. But this is just me.
>
>HTH.
>
>--
>Regards/Gruss,
>    Boris.
>
>Sent from a fat crate under my desk. Formatting is fine.
>--
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Thursday, July 31, 2014 7:27 PM
>To: Michal Simek
>Cc: Punnaiah Choudary Kalluri; dougthompson@xmission.com;
>robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
>ijc+devicetree@hellion.org.uk; Kumar Gala; Rob Landley;
>devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>edac@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>Vemula
>Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>ecc controller
>
>On Thu, Jul 31, 2014 at 03:36:35PM +0200, Michal Simek wrote:
>> Mixing functions for two/more different controller seems to me
>> really messy.
>
>Just stating that something is "really messy" without giving at least
>one technical reason for it is not going to get you any further. So let
>me give you mine:
>
>I think it is not messy because one file contains all the EDAC code for
>that system. Like every other system out there uses one EDAC driver.
>Everything is nicely contained in one place, any communication between
>the two memory controllers is done internally in that driver.
>
>Having two drivers would make that much more complicated. But hey, if
>you want to make your life more complicated because it is not enough
>complicated, feel free to do so. Just make sure you don't impose any
>restrictions on the rest of the subsystem.

Here we are also considering reusability of the driver. Let's say if some other
SOC contains Synopsys ddr controller then this driver can be reused. In this case,
Having separate driver would be good. But as you said in your previous comment,
We will talk about this later. 

Thanks,
Punnaiah
>
>> > -ENOPARSE for this sentence.
>>
>> I meant by this this scenario - please correct me if I am completely wrong
>> because I am not playing with this subsystem. My expectation is that
>> memory controller number should be same all the time.
>>
>> two memory controllers with the same edac driver.
>>
>> Normal calling sequence:
>> X probe - number 0
>> Y probe - number 1
>>
>> Different order/name (I think order is taken at the first place)
>> in DTS should caused that X will have number 1 and Y number 0.
>>
>> Another scenario:
>> two controllers and one with external gpio reset for example.
>>
>> X probe is called but gpio controller is not ready and gpio driver returns
>> -EPROBE_DEFER then Y probe is called and get number 0. Then X probe is
>called
>> again and get number 1.
>
>Right, since you have a heterogeneous design, you'd need to account
>for all that in your driver and deal with probing order. Like having a
>way to ask a controller what kind of a controller it is and so on. I'm
>afraid I don't know anything about your design to be of more help there.
>
>However, doing this in one <name>_edac.c file is almost trivial than
>doing it with two separate drivers. Unless you don't need to do any
>meaningful communication between the PS and PL. Then it doesn't matter.
>
>If I were you, I'd spend a more time on a proper design now so that all
>my work later is easy. Adding two drivers just because it seems messy is
>not a very good idea. What I would do is analyze all my use cases and
>come up with a proper design then. But this is just me.
>
>HTH.
>
>--
>Regards/Gruss,
>    Boris.
>
>Sent from a fat crate under my desk. Formatting is fine.
>--

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp at alien8.de]
>Sent: Thursday, July 31, 2014 7:27 PM
>To: Michal Simek
>Cc: Punnaiah Choudary Kalluri; dougthompson at xmission.com;
>robh+dt at kernel.org; pawel.moll at arm.com; mark.rutland at arm.com;
>ijc+devicetree at hellion.org.uk; Kumar Gala; Rob Landley;
>devicetree at vger.kernel.org; linux-doc at vger.kernel.org; linux-
>edac at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
>kernel at lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>Vemula
>Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>ecc controller
>
>On Thu, Jul 31, 2014 at 03:36:35PM +0200, Michal Simek wrote:
>> Mixing functions for two/more different controller seems to me
>> really messy.
>
>Just stating that something is "really messy" without giving at least
>one technical reason for it is not going to get you any further. So let
>me give you mine:
>
>I think it is not messy because one file contains all the EDAC code for
>that system. Like every other system out there uses one EDAC driver.
>Everything is nicely contained in one place, any communication between
>the two memory controllers is done internally in that driver.
>
>Having two drivers would make that much more complicated. But hey, if
>you want to make your life more complicated because it is not enough
>complicated, feel free to do so. Just make sure you don't impose any
>restrictions on the rest of the subsystem.

Here we are also considering reusability of the driver. Let's say if some other
SOC contains Synopsys ddr controller then this driver can be reused. In this case,
Having separate driver would be good. But as you said in your previous comment,
We will talk about this later. 

Thanks,
Punnaiah
>
>> > -ENOPARSE for this sentence.
>>
>> I meant by this this scenario - please correct me if I am completely wrong
>> because I am not playing with this subsystem. My expectation is that
>> memory controller number should be same all the time.
>>
>> two memory controllers with the same edac driver.
>>
>> Normal calling sequence:
>> X probe - number 0
>> Y probe - number 1
>>
>> Different order/name (I think order is taken at the first place)
>> in DTS should caused that X will have number 1 and Y number 0.
>>
>> Another scenario:
>> two controllers and one with external gpio reset for example.
>>
>> X probe is called but gpio controller is not ready and gpio driver returns
>> -EPROBE_DEFER then Y probe is called and get number 0. Then X probe is
>called
>> again and get number 1.
>
>Right, since you have a heterogeneous design, you'd need to account
>for all that in your driver and deal with probing order. Like having a
>way to ask a controller what kind of a controller it is and so on. I'm
>afraid I don't know anything about your design to be of more help there.
>
>However, doing this in one <name>_edac.c file is almost trivial than
>doing it with two separate drivers. Unless you don't need to do any
>meaningful communication between the PS and PL. Then it doesn't matter.
>
>If I were you, I'd spend a more time on a proper design now so that all
>my work later is easy. Adding two drivers just because it seems messy is
>not a very good idea. What I would do is analyze all my use cases and
>come up with a proper design then. But this is just me.
>
>HTH.
>
>--
>Regards/Gruss,
>    Boris.
>
>Sent from a fat crate under my desk. Formatting is fine.
>--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Thu, Jul 31, 2014 at 03:36:35PM +0200, Michal Simek wrote:
> Mixing functions for two/more different controller seems to me
> really messy.

Just stating that something is "really messy" without giving at least
one technical reason for it is not going to get you any further. So let
me give you mine:

I think it is not messy because one file contains all the EDAC code for
that system. Like every other system out there uses one EDAC driver.
Everything is nicely contained in one place, any communication between
the two memory controllers is done internally in that driver.

Having two drivers would make that much more complicated. But hey, if
you want to make your life more complicated because it is not enough
complicated, feel free to do so. Just make sure you don't impose any
restrictions on the rest of the subsystem.

> > -ENOPARSE for this sentence.
> 
> I meant by this this scenario - please correct me if I am completely wrong
> because I am not playing with this subsystem. My expectation is that
> memory controller number should be same all the time.
> 
> two memory controllers with the same edac driver.
> 
> Normal calling sequence:
> X probe - number 0
> Y probe - number 1
> 
> Different order/name (I think order is taken at the first place)
> in DTS should caused that X will have number 1 and Y number 0.
> 
> Another scenario:
> two controllers and one with external gpio reset for example.
> 
> X probe is called but gpio controller is not ready and gpio driver returns
> -EPROBE_DEFER then Y probe is called and get number 0. Then X probe is called
> again and get number 1.

Right, since you have a heterogeneous design, you'd need to account
for all that in your driver and deal with probing order. Like having a
way to ask a controller what kind of a controller it is and so on. I'm
afraid I don't know anything about your design to be of more help there.

However, doing this in one <name>_edac.c file is almost trivial than
doing it with two separate drivers. Unless you don't need to do any
meaningful communication between the PS and PL. Then it doesn't matter.

If I were you, I'd spend a more time on a proper design now so that all
my work later is easy. Adding two drivers just because it seems messy is
not a very good idea. What I would do is analyze all my use cases and
come up with a proper design then. But this is just me.

HTH.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Thu, Jul 31, 2014 at 03:36:35PM +0200, Michal Simek wrote:
> Mixing functions for two/more different controller seems to me
> really messy.

Just stating that something is "really messy" without giving at least
one technical reason for it is not going to get you any further. So let
me give you mine:

I think it is not messy because one file contains all the EDAC code for
that system. Like every other system out there uses one EDAC driver.
Everything is nicely contained in one place, any communication between
the two memory controllers is done internally in that driver.

Having two drivers would make that much more complicated. But hey, if
you want to make your life more complicated because it is not enough
complicated, feel free to do so. Just make sure you don't impose any
restrictions on the rest of the subsystem.

> > -ENOPARSE for this sentence.
> 
> I meant by this this scenario - please correct me if I am completely wrong
> because I am not playing with this subsystem. My expectation is that
> memory controller number should be same all the time.
> 
> two memory controllers with the same edac driver.
> 
> Normal calling sequence:
> X probe - number 0
> Y probe - number 1
> 
> Different order/name (I think order is taken at the first place)
> in DTS should caused that X will have number 1 and Y number 0.
> 
> Another scenario:
> two controllers and one with external gpio reset for example.
> 
> X probe is called but gpio controller is not ready and gpio driver returns
> -EPROBE_DEFER then Y probe is called and get number 0. Then X probe is called
> again and get number 1.

Right, since you have a heterogeneous design, you'd need to account
for all that in your driver and deal with probing order. Like having a
way to ask a controller what kind of a controller it is and so on. I'm
afraid I don't know anything about your design to be of more help there.

However, doing this in one <name>_edac.c file is almost trivial than
doing it with two separate drivers. Unless you don't need to do any
meaningful communication between the PS and PL. Then it doesn't matter.

If I were you, I'd spend a more time on a proper design now so that all
my work later is easy. Adding two drivers just because it seems messy is
not a very good idea. What I would do is analyze all my use cases and
come up with a proper design then. But this is just me.

HTH.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Thu, Jul 31, 2014 at 03:36:35PM +0200, Michal Simek wrote:
> Mixing functions for two/more different controller seems to me
> really messy.

Just stating that something is "really messy" without giving at least
one technical reason for it is not going to get you any further. So let
me give you mine:

I think it is not messy because one file contains all the EDAC code for
that system. Like every other system out there uses one EDAC driver.
Everything is nicely contained in one place, any communication between
the two memory controllers is done internally in that driver.

Having two drivers would make that much more complicated. But hey, if
you want to make your life more complicated because it is not enough
complicated, feel free to do so. Just make sure you don't impose any
restrictions on the rest of the subsystem.

> > -ENOPARSE for this sentence.
> 
> I meant by this this scenario - please correct me if I am completely wrong
> because I am not playing with this subsystem. My expectation is that
> memory controller number should be same all the time.
> 
> two memory controllers with the same edac driver.
> 
> Normal calling sequence:
> X probe - number 0
> Y probe - number 1
> 
> Different order/name (I think order is taken at the first place)
> in DTS should caused that X will have number 1 and Y number 0.
> 
> Another scenario:
> two controllers and one with external gpio reset for example.
> 
> X probe is called but gpio controller is not ready and gpio driver returns
> -EPROBE_DEFER then Y probe is called and get number 0. Then X probe is called
> again and get number 1.

Right, since you have a heterogeneous design, you'd need to account
for all that in your driver and deal with probing order. Like having a
way to ask a controller what kind of a controller it is and so on. I'm
afraid I don't know anything about your design to be of more help there.

However, doing this in one <name>_edac.c file is almost trivial than
doing it with two separate drivers. Unless you don't need to do any
meaningful communication between the PS and PL. Then it doesn't matter.

If I were you, I'd spend a more time on a proper design now so that all
my work later is easy. Adding two drivers just because it seems messy is
not a very good idea. What I would do is analyze all my use cases and
come up with a proper design then. But this is just me.

HTH.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/31/2014 03:17 PM, Borislav Petkov wrote:
> On Thu, Jul 31, 2014 at 02:13:48PM +0200, Michal Simek wrote:
>> Mixing two drivers in the one file is not a good idea because with
>> more memory controllers it is just a mess and you are not able to
>> cover all cases.
> 
> Why is it a mess?

Mixing functions for two/more different controller seems to me
really messy. Also having that drivers separated is useful if
driver probe fails for one controller (you can write the code for it
but it won't be nice).

>> If this is just about providing uniq number we can easily extend
>> binding and provide that uniq value. That's remind me solution with
>> edac_mc_get_id() can caused that you won't have exact number all the
>> time - depends on driver loading (deferred probing too).
> 
> -ENOPARSE for this sentence.

I meant by this this scenario - please correct me if I am completely wrong
because I am not playing with this subsystem. My expectation is that
memory controller number should be same all the time.

two memory controllers with the same edac driver.

Normal calling sequence:
X probe - number 0
Y probe - number 1

Different order/name (I think order is taken at the first place)
in DTS should caused that X will have number 1 and Y number 0.

Another scenario:
two controllers and one with external gpio reset for example.

X probe is called but gpio controller is not ready and gpio driver returns
-EPROBE_DEFER then Y probe is called and get number 0. Then X probe is called
again and get number 1.


>> One option via DT can be via aliases where you can easily specify
>> order. But all of these issues can be solved in follow-up patch.
> 
> Whatever you do, it should be designed cleanly and not introduce some
> homegrown solution.

Definitely I 100% agree with you. That's why I think this should
be solved separately.

Thanks,
Michal

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/31/2014 03:17 PM, Borislav Petkov wrote:
> On Thu, Jul 31, 2014 at 02:13:48PM +0200, Michal Simek wrote:
>> Mixing two drivers in the one file is not a good idea because with
>> more memory controllers it is just a mess and you are not able to
>> cover all cases.
> 
> Why is it a mess?

Mixing functions for two/more different controller seems to me
really messy. Also having that drivers separated is useful if
driver probe fails for one controller (you can write the code for it
but it won't be nice).

>> If this is just about providing uniq number we can easily extend
>> binding and provide that uniq value. That's remind me solution with
>> edac_mc_get_id() can caused that you won't have exact number all the
>> time - depends on driver loading (deferred probing too).
> 
> -ENOPARSE for this sentence.

I meant by this this scenario - please correct me if I am completely wrong
because I am not playing with this subsystem. My expectation is that
memory controller number should be same all the time.

two memory controllers with the same edac driver.

Normal calling sequence:
X probe - number 0
Y probe - number 1

Different order/name (I think order is taken at the first place)
in DTS should caused that X will have number 1 and Y number 0.

Another scenario:
two controllers and one with external gpio reset for example.

X probe is called but gpio controller is not ready and gpio driver returns
-EPROBE_DEFER then Y probe is called and get number 0. Then X probe is called
again and get number 1.


>> One option via DT can be via aliases where you can easily specify
>> order. But all of these issues can be solved in follow-up patch.
> 
> Whatever you do, it should be designed cleanly and not introduce some
> homegrown solution.

Definitely I 100% agree with you. That's why I think this should
be solved separately.

Thanks,
Michal


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/31/2014 03:17 PM, Borislav Petkov wrote:
> On Thu, Jul 31, 2014 at 02:13:48PM +0200, Michal Simek wrote:
>> Mixing two drivers in the one file is not a good idea because with
>> more memory controllers it is just a mess and you are not able to
>> cover all cases.
> 
> Why is it a mess?

Mixing functions for two/more different controller seems to me
really messy. Also having that drivers separated is useful if
driver probe fails for one controller (you can write the code for it
but it won't be nice).

>> If this is just about providing uniq number we can easily extend
>> binding and provide that uniq value. That's remind me solution with
>> edac_mc_get_id() can caused that you won't have exact number all the
>> time - depends on driver loading (deferred probing too).
> 
> -ENOPARSE for this sentence.

I meant by this this scenario - please correct me if I am completely wrong
because I am not playing with this subsystem. My expectation is that
memory controller number should be same all the time.

two memory controllers with the same edac driver.

Normal calling sequence:
X probe - number 0
Y probe - number 1

Different order/name (I think order is taken at the first place)
in DTS should caused that X will have number 1 and Y number 0.

Another scenario:
two controllers and one with external gpio reset for example.

X probe is called but gpio controller is not ready and gpio driver returns
-EPROBE_DEFER then Y probe is called and get number 0. Then X probe is called
again and get number 1.


>> One option via DT can be via aliases where you can easily specify
>> order. But all of these issues can be solved in follow-up patch.
> 
> Whatever you do, it should be designed cleanly and not introduce some
> homegrown solution.

Definitely I 100% agree with you. That's why I think this should
be solved separately.

Thanks,
Michal

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Thu, Jul 31, 2014 at 02:13:48PM +0200, Michal Simek wrote:
> Mixing two drivers in the one file is not a good idea because with
> more memory controllers it is just a mess and you are not able to
> cover all cases.

Why is it a mess?

> If this is just about providing uniq number we can easily extend
> binding and provide that uniq value. That's remind me solution with
> edac_mc_get_id() can caused that you won't have exact number all the
> time - depends on driver loading (deferred probing too).

-ENOPARSE for this sentence.

> One option via DT can be via aliases where you can easily specify
> order. But all of these issues can be solved in follow-up patch.

Whatever you do, it should be designed cleanly and not introduce some
homegrown solution.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Thu, Jul 31, 2014 at 02:13:48PM +0200, Michal Simek wrote:
> Mixing two drivers in the one file is not a good idea because with
> more memory controllers it is just a mess and you are not able to
> cover all cases.

Why is it a mess?

> If this is just about providing uniq number we can easily extend
> binding and provide that uniq value. That's remind me solution with
> edac_mc_get_id() can caused that you won't have exact number all the
> time - depends on driver loading (deferred probing too).

-ENOPARSE for this sentence.

> One option via DT can be via aliases where you can easily specify
> order. But all of these issues can be solved in follow-up patch.

Whatever you do, it should be designed cleanly and not introduce some
homegrown solution.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Thu, Jul 31, 2014 at 02:13:48PM +0200, Michal Simek wrote:
> Mixing two drivers in the one file is not a good idea because with
> more memory controllers it is just a mess and you are not able to
> cover all cases.

Why is it a mess?

> If this is just about providing uniq number we can easily extend
> binding and provide that uniq value. That's remind me solution with
> edac_mc_get_id() can caused that you won't have exact number all the
> time - depends on driver loading (deferred probing too).

-ENOPARSE for this sentence.

> One option via DT can be via aliases where you can easily specify
> order. But all of these issues can be solved in follow-up patch.

Whatever you do, it should be designed cleanly and not introduce some
homegrown solution.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/31/2014 01:33 PM, Borislav Petkov wrote:
> On Wed, Jul 30, 2014 at 03:41:47PM +0000, Punnaiah Choudary Kalluri wrote:
>>> So you're telling me that you want one edac driver for *two* memory
>>> controllers which can be present on a single system *at* *the* *same*
>>> *time*? Is that it?
>>
>> Yes.
> 
> Oh, this'll be fun. :-P

just to give me one more possible and more crazy example.
You can start system with one memory controller - then use partial
reconfiguration and add others at run time till you have enough pins
on your board. You can also remove it when you decide to do so.
And every boot can be different on the same board.


>>>
>>> How exactly is that topology supposed to look like, work, etc, etc? What
>>> kind of error reporting do you imagine you want to do with EDAC?
>>
>> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
>> System(PS) and Xilinx programmable logic(PL) in a single device.
>>
>> Assume the application is a broadcast camera. The design for this system use PS as
>> Control plane and use PL as data plane for processing the video data. So, the design 
>> may have two different memory controllers one in PS and another one in PL.   
>> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
>> OS running on PS.
> 
> Ok.
> 
>>  Now the requirement is to develop a monitoring system for all the
>> hardware failures Including ddr ecc errors. Since the broadcast camera
>> involves more data processing and DDR memory access, there is a high
>> probability of getting ddr ecc errors over the period.
> 
> So you're saying the camera has memory with ECC? People really pay
> attention on getting error free video frames? :-)
> 
> Or this is just an example? You're saying the PL will need ECC for some
> applications?

I believe that this is real example.
The another example can also be that Linux uses this PL memory with ECC
and PS memory for application because of whatever reason - speed for example.

But that configurations are real.

> 
>> So, the user should be intimated with these errors when they occur and if the error rate
>>  is high, then the user can consider the preventive methods. Without this error reporting
>> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>>  may occur due to ddr ecc failures.
>>
>> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
>> errors and also maintaining the statistics of that particular memory controller. With the current
>> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
>> Assume the code for the two drivers looks like below
>>
>> Driver 1:
>> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> 			    sizeof(struct  ctrl1_drvdata));
>>
>> Driver 2:
>> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> 			    sizeof(struct ctrl2_drvdata));
>>
>> Issue:
>>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>>  both the drivers claiming the same number.
>>
>> Solution 1:
>>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
>> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
>> reuse these drivers
> 
> Ok, I think I know what you mean. And this architecture works just fine.
> On x86 we have one EDAC instance per memory controller so on a multinode
> machine with multiple memory controllers, we do edac_mc_alloc() per
> memory controller.
> 
> For examples, see amd64_edac::init_one_instance() and sb_edac should
> have this too. So you basically have a local array of instances which
> you allocate and setup.
> 
> If someone wants to reuse the driver, then we can talk about this later,
> when the time comes.
>
> For now I think you should put both PS and PL stuff in one file,
> zynq_edac or so.
> 
> Any issues with this design?

Mixing two drivers in the one file is not a good idea because with more
memory controllers it is just a mess and you are not able to cover all cases.

If this is just about providing uniq number we can easily extend binding
and provide that uniq value.
That's remind me solution with edac_mc_get_id() can caused that you won't have
exact number all the time - depends on driver loading (deferred probing too).

One option via DT can be via aliases where you can easily specify order.
But all of these issues can be solved in follow-up patch.

Thanks,
Michal

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/31/2014 01:33 PM, Borislav Petkov wrote:
> On Wed, Jul 30, 2014 at 03:41:47PM +0000, Punnaiah Choudary Kalluri wrote:
>>> So you're telling me that you want one edac driver for *two* memory
>>> controllers which can be present on a single system *at* *the* *same*
>>> *time*? Is that it?
>>
>> Yes.
> 
> Oh, this'll be fun. :-P

just to give me one more possible and more crazy example.
You can start system with one memory controller - then use partial
reconfiguration and add others at run time till you have enough pins
on your board. You can also remove it when you decide to do so.
And every boot can be different on the same board.


>>>
>>> How exactly is that topology supposed to look like, work, etc, etc? What
>>> kind of error reporting do you imagine you want to do with EDAC?
>>
>> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
>> System(PS) and Xilinx programmable logic(PL) in a single device.
>>
>> Assume the application is a broadcast camera. The design for this system use PS as
>> Control plane and use PL as data plane for processing the video data. So, the design 
>> may have two different memory controllers one in PS and another one in PL.   
>> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
>> OS running on PS.
> 
> Ok.
> 
>>  Now the requirement is to develop a monitoring system for all the
>> hardware failures Including ddr ecc errors. Since the broadcast camera
>> involves more data processing and DDR memory access, there is a high
>> probability of getting ddr ecc errors over the period.
> 
> So you're saying the camera has memory with ECC? People really pay
> attention on getting error free video frames? :-)
> 
> Or this is just an example? You're saying the PL will need ECC for some
> applications?

I believe that this is real example.
The another example can also be that Linux uses this PL memory with ECC
and PS memory for application because of whatever reason - speed for example.

But that configurations are real.

> 
>> So, the user should be intimated with these errors when they occur and if the error rate
>>  is high, then the user can consider the preventive methods. Without this error reporting
>> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>>  may occur due to ddr ecc failures.
>>
>> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
>> errors and also maintaining the statistics of that particular memory controller. With the current
>> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
>> Assume the code for the two drivers looks like below
>>
>> Driver 1:
>> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> 			    sizeof(struct  ctrl1_drvdata));
>>
>> Driver 2:
>> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> 			    sizeof(struct ctrl2_drvdata));
>>
>> Issue:
>>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>>  both the drivers claiming the same number.
>>
>> Solution 1:
>>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
>> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
>> reuse these drivers
> 
> Ok, I think I know what you mean. And this architecture works just fine.
> On x86 we have one EDAC instance per memory controller so on a multinode
> machine with multiple memory controllers, we do edac_mc_alloc() per
> memory controller.
> 
> For examples, see amd64_edac::init_one_instance() and sb_edac should
> have this too. So you basically have a local array of instances which
> you allocate and setup.
> 
> If someone wants to reuse the driver, then we can talk about this later,
> when the time comes.
>
> For now I think you should put both PS and PL stuff in one file,
> zynq_edac or so.
> 
> Any issues with this design?

Mixing two drivers in the one file is not a good idea because with more
memory controllers it is just a mess and you are not able to cover all cases.

If this is just about providing uniq number we can easily extend binding
and provide that uniq value.
That's remind me solution with edac_mc_get_id() can caused that you won't have
exact number all the time - depends on driver loading (deferred probing too).

One option via DT can be via aliases where you can easily specify order.
But all of these issues can be solved in follow-up patch.

Thanks,
Michal

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/31/2014 01:33 PM, Borislav Petkov wrote:
> On Wed, Jul 30, 2014 at 03:41:47PM +0000, Punnaiah Choudary Kalluri wrote:
>>> So you're telling me that you want one edac driver for *two* memory
>>> controllers which can be present on a single system *at* *the* *same*
>>> *time*? Is that it?
>>
>> Yes.
> 
> Oh, this'll be fun. :-P

just to give me one more possible and more crazy example.
You can start system with one memory controller - then use partial
reconfiguration and add others at run time till you have enough pins
on your board. You can also remove it when you decide to do so.
And every boot can be different on the same board.


>>>
>>> How exactly is that topology supposed to look like, work, etc, etc? What
>>> kind of error reporting do you imagine you want to do with EDAC?
>>
>> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
>> System(PS) and Xilinx programmable logic(PL) in a single device.
>>
>> Assume the application is a broadcast camera. The design for this system use PS as
>> Control plane and use PL as data plane for processing the video data. So, the design 
>> may have two different memory controllers one in PS and another one in PL.   
>> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
>> OS running on PS.
> 
> Ok.
> 
>>  Now the requirement is to develop a monitoring system for all the
>> hardware failures Including ddr ecc errors. Since the broadcast camera
>> involves more data processing and DDR memory access, there is a high
>> probability of getting ddr ecc errors over the period.
> 
> So you're saying the camera has memory with ECC? People really pay
> attention on getting error free video frames? :-)
> 
> Or this is just an example? You're saying the PL will need ECC for some
> applications?

I believe that this is real example.
The another example can also be that Linux uses this PL memory with ECC
and PS memory for application because of whatever reason - speed for example.

But that configurations are real.

> 
>> So, the user should be intimated with these errors when they occur and if the error rate
>>  is high, then the user can consider the preventive methods. Without this error reporting
>> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>>  may occur due to ddr ecc failures.
>>
>> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
>> errors and also maintaining the statistics of that particular memory controller. With the current
>> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
>> Assume the code for the two drivers looks like below
>>
>> Driver 1:
>> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> 			    sizeof(struct  ctrl1_drvdata));
>>
>> Driver 2:
>> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
>> 			    sizeof(struct ctrl2_drvdata));
>>
>> Issue:
>>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>>  both the drivers claiming the same number.
>>
>> Solution 1:
>>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
>> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
>> reuse these drivers
> 
> Ok, I think I know what you mean. And this architecture works just fine.
> On x86 we have one EDAC instance per memory controller so on a multinode
> machine with multiple memory controllers, we do edac_mc_alloc() per
> memory controller.
> 
> For examples, see amd64_edac::init_one_instance() and sb_edac should
> have this too. So you basically have a local array of instances which
> you allocate and setup.
> 
> If someone wants to reuse the driver, then we can talk about this later,
> when the time comes.
>
> For now I think you should put both PS and PL stuff in one file,
> zynq_edac or so.
> 
> Any issues with this design?

Mixing two drivers in the one file is not a good idea because with more
memory controllers it is just a mess and you are not able to cover all cases.

If this is just about providing uniq number we can easily extend binding
and provide that uniq value.
That's remind me solution with edac_mc_get_id() can caused that you won't have
exact number all the time - depends on driver loading (deferred probing too).

One option via DT can be via aliases where you can easily specify order.
But all of these issues can be solved in follow-up patch.

Thanks,
Michal

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Wed, Jul 30, 2014 at 03:41:47PM +0000, Punnaiah Choudary Kalluri wrote:
> >So you're telling me that you want one edac driver for *two* memory
> >controllers which can be present on a single system *at* *the* *same*
> >*time*? Is that it?
> 
> Yes.

Oh, this'll be fun. :-P

> >
> >How exactly is that topology supposed to look like, work, etc, etc? What
> >kind of error reporting do you imagine you want to do with EDAC?
> 
> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
> System(PS) and Xilinx programmable logic(PL) in a single device.
> 
> Assume the application is a broadcast camera. The design for this system use PS as
> Control plane and use PL as data plane for processing the video data. So, the design 
> may have two different memory controllers one in PS and another one in PL.   
> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
> OS running on PS.

Ok.

>  Now the requirement is to develop a monitoring system for all the
> hardware failures Including ddr ecc errors. Since the broadcast camera
> involves more data processing and DDR memory access, there is a high
> probability of getting ddr ecc errors over the period.

So you're saying the camera has memory with ECC? People really pay
attention on getting error free video frames? :-)

Or this is just an example? You're saying the PL will need ECC for some
applications?

> So, the user should be intimated with these errors when they occur and if the error rate
>  is high, then the user can consider the preventive methods. Without this error reporting
> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>  may occur due to ddr ecc failures.
> 
> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
> errors and also maintaining the statistics of that particular memory controller. With the current
> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
> Assume the code for the two drivers looks like below
> 
> Driver 1:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
> 
> Driver 2:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
> 
> Issue:
>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>  both the drivers claiming the same number.
> 
> Solution 1:
>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
> reuse these drivers

Ok, I think I know what you mean. And this architecture works just fine.
On x86 we have one EDAC instance per memory controller so on a multinode
machine with multiple memory controllers, we do edac_mc_alloc() per
memory controller.

For examples, see amd64_edac::init_one_instance() and sb_edac should
have this too. So you basically have a local array of instances which
you allocate and setup.

If someone wants to reuse the driver, then we can talk about this later,
when the time comes.

For now I think you should put both PS and PL stuff in one file,
zynq_edac or so.

Any issues with this design?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Wed, Jul 30, 2014 at 03:41:47PM +0000, Punnaiah Choudary Kalluri wrote:
> >So you're telling me that you want one edac driver for *two* memory
> >controllers which can be present on a single system *at* *the* *same*
> >*time*? Is that it?
> 
> Yes.

Oh, this'll be fun. :-P

> >
> >How exactly is that topology supposed to look like, work, etc, etc? What
> >kind of error reporting do you imagine you want to do with EDAC?
> 
> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
> System(PS) and Xilinx programmable logic(PL) in a single device.
> 
> Assume the application is a broadcast camera. The design for this system use PS as
> Control plane and use PL as data plane for processing the video data. So, the design 
> may have two different memory controllers one in PS and another one in PL.   
> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
> OS running on PS.

Ok.

>  Now the requirement is to develop a monitoring system for all the
> hardware failures Including ddr ecc errors. Since the broadcast camera
> involves more data processing and DDR memory access, there is a high
> probability of getting ddr ecc errors over the period.

So you're saying the camera has memory with ECC? People really pay
attention on getting error free video frames? :-)

Or this is just an example? You're saying the PL will need ECC for some
applications?

> So, the user should be intimated with these errors when they occur and if the error rate
>  is high, then the user can consider the preventive methods. Without this error reporting
> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>  may occur due to ddr ecc failures.
> 
> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
> errors and also maintaining the statistics of that particular memory controller. With the current
> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
> Assume the code for the two drivers looks like below
> 
> Driver 1:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
> 
> Driver 2:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
> 
> Issue:
>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>  both the drivers claiming the same number.
> 
> Solution 1:
>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
> reuse these drivers

Ok, I think I know what you mean. And this architecture works just fine.
On x86 we have one EDAC instance per memory controller so on a multinode
machine with multiple memory controllers, we do edac_mc_alloc() per
memory controller.

For examples, see amd64_edac::init_one_instance() and sb_edac should
have this too. So you basically have a local array of instances which
you allocate and setup.

If someone wants to reuse the driver, then we can talk about this later,
when the time comes.

For now I think you should put both PS and PL stuff in one file,
zynq_edac or so.

Any issues with this design?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Borislav Petkov]

On Wed, Jul 30, 2014 at 03:41:47PM +0000, Punnaiah Choudary Kalluri wrote:
> >So you're telling me that you want one edac driver for *two* memory
> >controllers which can be present on a single system *at* *the* *same*
> >*time*? Is that it?
> 
> Yes.

Oh, this'll be fun. :-P

> >
> >How exactly is that topology supposed to look like, work, etc, etc? What
> >kind of error reporting do you imagine you want to do with EDAC?
> 
> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
> System(PS) and Xilinx programmable logic(PL) in a single device.
> 
> Assume the application is a broadcast camera. The design for this system use PS as
> Control plane and use PL as data plane for processing the video data. So, the design 
> may have two different memory controllers one in PS and another one in PL.   
> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
> OS running on PS.

Ok.

>  Now the requirement is to develop a monitoring system for all the
> hardware failures Including ddr ecc errors. Since the broadcast camera
> involves more data processing and DDR memory access, there is a high
> probability of getting ddr ecc errors over the period.

So you're saying the camera has memory with ECC? People really pay
attention on getting error free video frames? :-)

Or this is just an example? You're saying the PL will need ECC for some
applications?

> So, the user should be intimated with these errors when they occur and if the error rate
>  is high, then the user can consider the preventive methods. Without this error reporting
> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>  may occur due to ddr ecc failures.
> 
> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
> errors and also maintaining the statistics of that particular memory controller. With the current
> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
> Assume the code for the two drivers looks like below
> 
> Driver 1:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
> 
> Driver 2:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
> 
> Issue:
>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>  both the drivers claiming the same number.
> 
> Solution 1:
>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
> reuse these drivers

Ok, I think I know what you mean. And this architecture works just fine.
On x86 we have one EDAC instance per memory controller so on a multinode
machine with multiple memory controllers, we do edac_mc_alloc() per
memory controller.

For examples, see amd64_edac::init_one_instance() and sb_edac should
have this too. So you basically have a local array of instances which
you allocate and setup.

If someone wants to reuse the driver, then we can talk about this later,
when the time comes.

For now I think you should put both PS and PL stuff in one file,
zynq_edac or so.

Any issues with this design?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/30/2014 05:41 PM, Punnaiah Choudary Kalluri wrote:
> Hi Boris,
> 
>> -----Original Message-----
>> From: Borislav Petkov [mailto:bp@alien8.de]
>> Sent: Monday, July 28, 2014 11:32 PM
>> To: Punnaiah Choudary Kalluri
>> Cc: Punnaiah Choudary Kalluri; dougthompson@xmission.com;
>> robh+dt@kernel.org; pawel.moll@arm.com; Michal Simek;
>> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; Kumar Gala; Rob
>> Landley; devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>> edac@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>> Vemula
>> Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>> ecc controller
>>
>> On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote:
>>> I can agree with you that we can use shorter names.But ZYNQ has
>>> programmable logic next to processing system where one can add soft
>>> memory controller in the same system and may use different driver. So,
>>> the edac driver for zynq may include multiple drivers for different
>>> memory controllers in the same file. In this case it is difficult to
>>> maintain generic macros as you suggested above.
>>
>> So?
>>
>> You can still shorten them a bit - the current names are awfully long.
>> SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR
>> and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever
>> you wanna call it prefix and then the rest. I.e.,
>>
>>    SYNPS_<reg>_<bits*>
>>    ZYNQ_<reg>_bits*>
>>
>> You can even use single letter prefixes like S_ and Z_ and add a comment
>> explaining what those mean. Still much more readable than the long
>> repeating prefixes currently.
> 
> Ok.
>>
>>> Also the current edac framework for edac memory controllers is
>>> expecting the mc_num from the driver while allocating the memory
>>> controller instance using the edac_mc_alloc function. This requirement
>>> mandates that if the system contains two different memory controllers
>>> then the corresponding edac drivers should be in single file.
>>
>> So you're telling me that you want one edac driver for *two* memory
>> controllers which can be present on a single system *at* *the* *same*
>> *time*? Is that it?
> 
> Yes.
>>
>> How exactly is that topology supposed to look like, work, etc, etc? What
>> kind of error reporting do you imagine you want to do with EDAC?
> 
> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
> System(PS) and Xilinx programmable logic(PL) in a single device.
> 
> Assume the application is a broadcast camera. The design for this system use PS as
> Control plane and use PL as data plane for processing the video data. So, the design 
> may have two different memory controllers one in PS and another one in PL.   
> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
> OS running on PS.
>  Now the requirement is to develop a monitoring system for all the hardware failures 
> Including ddr ecc errors. Since the broadcast camera involves more data processing and
> DDR memory access, there is a high probability of getting ddr ecc errors over the period.
> So, the user should be intimated with these errors when they occur and if the error rate
>  is high, then the user can consider the preventive methods. Without this error reporting
> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>  may occur due to ddr ecc failures.
> 
> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
> errors and also maintaining the statistics of that particular memory controller. With the current
> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
> Assume the code for the two drivers looks like below
> 
> Driver 1:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
> 
> Driver 2:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
> 
> Issue:
>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>  both the drivers claiming the same number.
> 
> Solution 1:
>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
> reuse these drivers
> static int mc_num = 0;
> 
> Driver 1:
> 	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
>               mc_num++;
> 
> Driver 2:
> 	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
>                  mc_num++;
> 
> Solution 2:
>   Let the framework assign the mc_num to the edac driver
> 
>      mc_num = edac_mc_get_id(); /* returns the next available mci slot */
>      mci = edac_mc_alloc(mc_num,...);
> 
> 
> In my opinion solution 2  looks neat and it eliminates the dependency on tracking
>  the mc_num.

Isn't there also 3rd option which is provide mc_num via DTS?

Thanks,
Michal

Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/30/2014 05:41 PM, Punnaiah Choudary Kalluri wrote:
> Hi Boris,
> 
>> -----Original Message-----
>> From: Borislav Petkov [mailto:bp@alien8.de]
>> Sent: Monday, July 28, 2014 11:32 PM
>> To: Punnaiah Choudary Kalluri
>> Cc: Punnaiah Choudary Kalluri; dougthompson@xmission.com;
>> robh+dt@kernel.org; pawel.moll@arm.com; Michal Simek;
>> mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; Kumar Gala; Rob
>> Landley; devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>> edac@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>> Vemula
>> Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>> ecc controller
>>
>> On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote:
>>> I can agree with you that we can use shorter names.But ZYNQ has
>>> programmable logic next to processing system where one can add soft
>>> memory controller in the same system and may use different driver. So,
>>> the edac driver for zynq may include multiple drivers for different
>>> memory controllers in the same file. In this case it is difficult to
>>> maintain generic macros as you suggested above.
>>
>> So?
>>
>> You can still shorten them a bit - the current names are awfully long.
>> SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR
>> and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever
>> you wanna call it prefix and then the rest. I.e.,
>>
>>    SYNPS_<reg>_<bits*>
>>    ZYNQ_<reg>_bits*>
>>
>> You can even use single letter prefixes like S_ and Z_ and add a comment
>> explaining what those mean. Still much more readable than the long
>> repeating prefixes currently.
> 
> Ok.
>>
>>> Also the current edac framework for edac memory controllers is
>>> expecting the mc_num from the driver while allocating the memory
>>> controller instance using the edac_mc_alloc function. This requirement
>>> mandates that if the system contains two different memory controllers
>>> then the corresponding edac drivers should be in single file.
>>
>> So you're telling me that you want one edac driver for *two* memory
>> controllers which can be present on a single system *at* *the* *same*
>> *time*? Is that it?
> 
> Yes.
>>
>> How exactly is that topology supposed to look like, work, etc, etc? What
>> kind of error reporting do you imagine you want to do with EDAC?
> 
> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
> System(PS) and Xilinx programmable logic(PL) in a single device.
> 
> Assume the application is a broadcast camera. The design for this system use PS as
> Control plane and use PL as data plane for processing the video data. So, the design 
> may have two different memory controllers one in PS and another one in PL.   
> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
> OS running on PS.
>  Now the requirement is to develop a monitoring system for all the hardware failures 
> Including ddr ecc errors. Since the broadcast camera involves more data processing and
> DDR memory access, there is a high probability of getting ddr ecc errors over the period.
> So, the user should be intimated with these errors when they occur and if the error rate
>  is high, then the user can consider the preventive methods. Without this error reporting
> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>  may occur due to ddr ecc failures.
> 
> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
> errors and also maintaining the statistics of that particular memory controller. With the current
> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
> Assume the code for the two drivers looks like below
> 
> Driver 1:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
> 
> Driver 2:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
> 
> Issue:
>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>  both the drivers claiming the same number.
> 
> Solution 1:
>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
> reuse these drivers
> static int mc_num = 0;
> 
> Driver 1:
> 	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
>               mc_num++;
> 
> Driver 2:
> 	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
>                  mc_num++;
> 
> Solution 2:
>   Let the framework assign the mc_num to the edac driver
> 
>      mc_num = edac_mc_get_id(); /* returns the next available mci slot */
>      mci = edac_mc_alloc(mc_num,...);
> 
> 
> In my opinion solution 2  looks neat and it eliminates the dependency on tracking
>  the mc_num.

Isn't there also 3rd option which is provide mc_num via DTS?

Thanks,
Michal

[RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Michal Simek]

On 07/30/2014 05:41 PM, Punnaiah Choudary Kalluri wrote:
> Hi Boris,
> 
>> -----Original Message-----
>> From: Borislav Petkov [mailto:bp at alien8.de]
>> Sent: Monday, July 28, 2014 11:32 PM
>> To: Punnaiah Choudary Kalluri
>> Cc: Punnaiah Choudary Kalluri; dougthompson at xmission.com;
>> robh+dt at kernel.org; pawel.moll at arm.com; Michal Simek;
>> mark.rutland at arm.com; ijc+devicetree at hellion.org.uk; Kumar Gala; Rob
>> Landley; devicetree at vger.kernel.org; linux-doc at vger.kernel.org; linux-
>> edac at vger.kernel.org; linux-kernel at vger.kernel.org; linux-arm-
>> kernel at lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>> Vemula
>> Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>> ecc controller
>>
>> On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote:
>>> I can agree with you that we can use shorter names.But ZYNQ has
>>> programmable logic next to processing system where one can add soft
>>> memory controller in the same system and may use different driver. So,
>>> the edac driver for zynq may include multiple drivers for different
>>> memory controllers in the same file. In this case it is difficult to
>>> maintain generic macros as you suggested above.
>>
>> So?
>>
>> You can still shorten them a bit - the current names are awfully long.
>> SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR
>> and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever
>> you wanna call it prefix and then the rest. I.e.,
>>
>>    SYNPS_<reg>_<bits*>
>>    ZYNQ_<reg>_bits*>
>>
>> You can even use single letter prefixes like S_ and Z_ and add a comment
>> explaining what those mean. Still much more readable than the long
>> repeating prefixes currently.
> 
> Ok.
>>
>>> Also the current edac framework for edac memory controllers is
>>> expecting the mc_num from the driver while allocating the memory
>>> controller instance using the edac_mc_alloc function. This requirement
>>> mandates that if the system contains two different memory controllers
>>> then the corresponding edac drivers should be in single file.
>>
>> So you're telling me that you want one edac driver for *two* memory
>> controllers which can be present on a single system *at* *the* *same*
>> *time*? Is that it?
> 
> Yes.
>>
>> How exactly is that topology supposed to look like, work, etc, etc? What
>> kind of error reporting do you imagine you want to do with EDAC?
> 
> Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
> System(PS) and Xilinx programmable logic(PL) in a single device.
> 
> Assume the application is a broadcast camera. The design for this system use PS as
> Control plane and use PL as data plane for processing the video data. So, the design 
> may have two different memory controllers one in PS and another one in PL.   
> PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
> OS running on PS.
>  Now the requirement is to develop a monitoring system for all the hardware failures 
> Including ddr ecc errors. Since the broadcast camera involves more data processing and
> DDR memory access, there is a high probability of getting ddr ecc errors over the period.
> So, the user should be intimated with these errors when they occur and if the error rate
>  is high, then the user can consider the preventive methods. Without this error reporting
> mechanism it is difficult to debug the issues like memory corruption, kernel oops which
>  may occur due to ddr ecc failures.
> 
> Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
> errors and also maintaining the statistics of that particular memory controller. With the current
> framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
> Assume the code for the two drivers looks like below
> 
> Driver 1:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
> 
> Driver 2:
> 	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
> 
> Issue:
>   Since driver is providing the mc_num to framework, now there is chance that only one device active as
>  both the drivers claiming the same number.
> 
> Solution 1:
>  Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
> good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
> reuse these drivers
> static int mc_num = 0;
> 
> Driver 1:
> 	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct  ctrl1_drvdata));
>               mc_num++;
> 
> Driver 2:
> 	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
> 			    sizeof(struct ctrl2_drvdata));
>                  mc_num++;
> 
> Solution 2:
>   Let the framework assign the mc_num to the edac driver
> 
>      mc_num = edac_mc_get_id(); /* returns the next available mci slot */
>      mci = edac_mc_alloc(mc_num,...);
> 
> 
> In my opinion solution 2  looks neat and it eliminates the dependency on tracking
>  the mc_num.

Isn't there also 3rd option which is provide mc_num via DTS?

Thanks,
Michal

RE: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5561 bytes --]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Monday, July 28, 2014 11:32 PM
>To: Punnaiah Choudary Kalluri
>Cc: Punnaiah Choudary Kalluri; dougthompson@xmission.com;
>robh+dt@kernel.org; pawel.moll@arm.com; Michal Simek;
>mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; Kumar Gala; Rob
>Landley; devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>edac@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>Vemula
>Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>ecc controller
>
>On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote:
>> I can agree with you that we can use shorter names.But ZYNQ has
>> programmable logic next to processing system where one can add soft
>> memory controller in the same system and may use different driver. So,
>> the edac driver for zynq may include multiple drivers for different
>> memory controllers in the same file. In this case it is difficult to
>> maintain generic macros as you suggested above.
>
>So?
>
>You can still shorten them a bit - the current names are awfully long.
>SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR
>and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever
>you wanna call it prefix and then the rest. I.e.,
>
>    SYNPS_<reg>_<bits*>
>    ZYNQ_<reg>_bits*>
>
>You can even use single letter prefixes like S_ and Z_ and add a comment
>explaining what those mean. Still much more readable than the long
>repeating prefixes currently.

Ok.
>
>> Also the current edac framework for edac memory controllers is
>> expecting the mc_num from the driver while allocating the memory
>> controller instance using the edac_mc_alloc function. This requirement
>> mandates that if the system contains two different memory controllers
>> then the corresponding edac drivers should be in single file.
>
>So you're telling me that you want one edac driver for *two* memory
>controllers which can be present on a single system *at* *the* *same*
>*time*? Is that it?

Yes.
>
>How exactly is that topology supposed to look like, work, etc, etc? What
>kind of error reporting do you imagine you want to do with EDAC?

Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
System(PS) and Xilinx programmable logic(PL) in a single device.

Assume the application is a broadcast camera. The design for this system use PS as
Control plane and use PL as data plane for processing the video data. So, the design 
may have two different memory controllers one in PS and another one in PL.   
PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
OS running on PS.
 Now the requirement is to develop a monitoring system for all the hardware failures 
Including ddr ecc errors. Since the broadcast camera involves more data processing and
DDR memory access, there is a high probability of getting ddr ecc errors over the period.
So, the user should be intimated with these errors when they occur and if the error rate
 is high, then the user can consider the preventive methods. Without this error reporting
mechanism it is difficult to debug the issues like memory corruption, kernel oops which
 may occur due to ddr ecc failures.

Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
errors and also maintaining the statistics of that particular memory controller. With the current
framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
Assume the code for the two drivers looks like below

Driver 1:
	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
			    sizeof(struct  ctrl1_drvdata));

Driver 2:
	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
			    sizeof(struct ctrl2_drvdata));

Issue:
  Since driver is providing the mc_num to framework, now there is chance that only one device active as
 both the drivers claiming the same number.

Solution 1:
 Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
reuse these drivers
static int mc_num = 0;

Driver 1:
	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
			    sizeof(struct  ctrl1_drvdata));
              mc_num++;

Driver 2:
	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
			    sizeof(struct ctrl2_drvdata));
                 mc_num++;

Solution 2:
  Let the framework assign the mc_num to the edac driver

     mc_num = edac_mc_get_id(); /* returns the next available mci slot */
     mci = edac_mc_alloc(mc_num,...);


In my opinion solution 2  looks neat and it eliminates the dependency on tracking
 the mc_num.

Please suggest your view on this solution.
 
At this point of time, my requirement is reporting the ecc correctable and uncorrectable
 errors and also maintaining the error stats for future decision making. Since the linux edac
Framework is ready, we decided to use this framework.
 
Meanwhile, I will  submit the next revision of the patch for the above cooments. 

Regards,
Punnaiah
>
>IOW, more info would be appreciated.
>
>Thanks.
>
>--
>Regards/Gruss,
>    Boris.
>
>Sent from a fat crate under my desk. Formatting is fine.
>--
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@alien8.de]
>Sent: Monday, July 28, 2014 11:32 PM
>To: Punnaiah Choudary Kalluri
>Cc: Punnaiah Choudary Kalluri; dougthompson@xmission.com;
>robh+dt@kernel.org; pawel.moll@arm.com; Michal Simek;
>mark.rutland@arm.com; ijc+devicetree@hellion.org.uk; Kumar Gala; Rob
>Landley; devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
>edac@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; Punnaiah Choudary; Anirudha Sarangi; Srikanth
>Vemula
>Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr
>ecc controller
>
>On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote:
>> I can agree with you that we can use shorter names.But ZYNQ has
>> programmable logic next to processing system where one can add soft
>> memory controller in the same system and may use different driver. So,
>> the edac driver for zynq may include multiple drivers for different
>> memory controllers in the same file. In this case it is difficult to
>> maintain generic macros as you suggested above.
>
>So?
>
>You can still shorten them a bit - the current names are awfully long.
>SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR
>and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever
>you wanna call it prefix and then the rest. I.e.,
>
>    SYNPS_<reg>_<bits*>
>    ZYNQ_<reg>_bits*>
>
>You can even use single letter prefixes like S_ and Z_ and add a comment
>explaining what those mean. Still much more readable than the long
>repeating prefixes currently.

Ok.
>
>> Also the current edac framework for edac memory controllers is
>> expecting the mc_num from the driver while allocating the memory
>> controller instance using the edac_mc_alloc function. This requirement
>> mandates that if the system contains two different memory controllers
>> then the corresponding edac drivers should be in single file.
>
>So you're telling me that you want one edac driver for *two* memory
>controllers which can be present on a single system *at* *the* *same*
>*time*? Is that it?

Yes.
>
>How exactly is that topology supposed to look like, work, etc, etc? What
>kind of error reporting do you imagine you want to do with EDAC?

Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing
System(PS) and Xilinx programmable logic(PL) in a single device.

Assume the application is a broadcast camera. The design for this system use PS as
Control plane and use PL as data plane for processing the video data. So, the design 
may have two different memory controllers one in PS and another one in PL.   
PS is running with Linux OS and PL doesn't have the OS and it is  controlled by the 
OS running on PS.
 Now the requirement is to develop a monitoring system for all the hardware failures 
Including ddr ecc errors. Since the broadcast camera involves more data processing and
DDR memory access, there is a high probability of getting ddr ecc errors over the period.
So, the user should be intimated with these errors when they occur and if the error rate
 is high, then the user can consider the preventive methods. Without this error reporting
mechanism it is difficult to debug the issues like memory corruption, kernel oops which
 may occur due to ddr ecc failures.

Since, the memory controllers are different, it need two edac drivers for reporting the ecc 
errors and also maintaining the statistics of that particular memory controller. With the current
framework, there is a chance that both the drivers get mc_num as zero and malfunction. 
Assume the code for the two drivers looks like below

Driver 1:
	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
			    sizeof(struct  ctrl1_drvdata));

Driver 2:
	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
			    sizeof(struct ctrl2_drvdata));

Issue:
  Since driver is providing the mc_num to framework, now there is chance that only one device active as
 both the drivers claiming the same number.

Solution 1:
 Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks
good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not
reuse these drivers
static int mc_num = 0;

Driver 1:
	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
			    sizeof(struct  ctrl1_drvdata));
              mc_num++;

Driver 2:
	mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers,
			    sizeof(struct ctrl2_drvdata));
                 mc_num++;

Solution 2:
  Let the framework assign the mc_num to the edac driver

     mc_num = edac_mc_get_id(); /* returns the next available mci slot */
     mci = edac_mc_alloc(mc_num,...);


In my opinion solution 2  looks neat and it eliminates the dependency on tracking
 the mc_num.

Please suggest your view on this solution.
 
At this point of time, my requirement is reporting the ecc correctable and uncorrectable
 errors and also maintaining the error stats for future decision making. Since the linux edac
Framework is ready, we decided to use this framework.
 
Meanwhile, I will  submit the next revision of the patch for the above cooments. 

Regards,
Punnaiah
>
>IOW, more info would be appreciated.
>
>Thanks.
>
>--
>Regards/Gruss,
>    Boris.
>
>Sent from a fat crate under my desk. Formatting is fine.
>--