[patch v3 0/4] Add edac driver for i.MX8MP based on synopsys edac driver

[patch v3 0/4] Add edac driver for i.MX8MP based on synopsys edac driver

[Sherry Sun]

This patchset add edac driver support for i.MX8MP, since i.MX8MP use the same
synopsys memory controller with ZynqMP, so the driver is based on synopsys_edac
driver.

Considering that i.MX8MP dts file is still in kernel/git/shawnguo/linux.git and
isn't in mainline now, I will add EDAC node in i.MX8MP dts after this file is
pushed to mainline. 

And the edac driver for i.MX8MP has been tested in kernel/git/shawnguo/linux.git
where i.MX8MP is supported, it turns out that this driver works well to detect
corrected and uncorrected errors for LPDDR4 in i.MX8MP.

Changes since v2:
=================
- Use u64 data instead u32 data_low and u32 data_high as robert suggested in
patch 4/4.
- Add edac_dbg() for the != DEV_X8 cases in zynqmp_get_error_info() in patch
4/4.
- Change the format of the output data to avoid build warnings in patch 4/4.

Changes since v1:
=================
- Reorganized the patchset, no content changes. 

Sherry Sun (4):
  dt-bindings: memory-controllers: Add i.MX8MP DDRC binding doc
  EDAC: Add synopsys edac driver support for i.MX8MP
  EDAC: synopsys: Add edac driver support for i.MX8MP
  EDAC: synopsys: Add useful debug and output information for 64bit
    systems

 .../bindings/memory-controllers/synopsys.txt  |   8 +-
 drivers/edac/Kconfig                          |   2 +-
 drivers/edac/synopsys_edac.c                  | 248 ++++++++++++------
 3 files changed, 179 insertions(+), 79 deletions(-)

-- 
2.17.1

[patch v3 1/4] dt-bindings: memory-controllers: Add i.MX8MP DDRC binding doc

[Sherry Sun]

Add documentation for i.MX8MP DDRC binding based on synopsys_edac doc,
which use the same memory-controller IP.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/memory-controllers/synopsys.txt   | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/synopsys.txt b/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
index 9d32762c47e1..4fd14ba61474 100644
--- a/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/synopsys.txt
@@ -6,16 +6,20 @@ bus width configurations.
 The Zynq DDR ECC controller has an optional ECC support in half-bus width
 (16-bit) configuration.
 
-These both ECC controllers correct single bit ECC errors and detect double bit
+The i.MX8MP DDR ECC controller has an ECC support in 64-bit bus width
+configurations.
+
+All the ECC controllers correct single bit ECC errors and detect double bit
 ECC errors.
 
 Required properties:
  - compatible: One of:
 	- 'xlnx,zynq-ddrc-a05' : Zynq DDR ECC controller
 	- 'xlnx,zynqmp-ddrc-2.40a' : ZynqMP DDR ECC controller
+	- 'fsl,imx8mp-ddrc' : i.MX8MP DDR ECC controller
  - reg: Should contain DDR controller registers location and length.
 
-Required properties for "xlnx,zynqmp-ddrc-2.40a":
+Required properties for "xlnx,zynqmp-ddrc-2.40a" and "fsl,imx8mp-ddrc":
  - interrupts: Property with a value describing the interrupt number.
 
 Example:
-- 
2.17.1

[patch v3 2/4] EDAC: Add synopsys edac driver support for i.MX8MP

[Sherry Sun]

The i.MX8MP has a memory controller supported by this driver. So here
add edac driver for i.MX8MP based on synopsys edac driver.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/edac/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index fe2eb892a1bd..58a2d67d5513 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -475,7 +475,7 @@ config EDAC_ARMADA_XP
 
 config EDAC_SYNOPSYS
 	tristate "Synopsys DDR Memory Controller"
-	depends on ARCH_ZYNQ || ARCH_ZYNQMP
+	depends on ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_MXC
 	help
 	  Support for error detection and correction on the Synopsys DDR
 	  memory controller.
-- 
2.17.1

[patch v3 3/4] EDAC: synopsys: Add edac driver support for i.MX8MP

[Sherry Sun]

Since i.MX8MP use synopsys ddr controller IP, so add edac support
for i.MX8MP based on synopsys edac driver. i.MX8MP use LPDDR4 and
support interrupts for corrected and uncorrected errors. The main
difference between ZynqMP and i.MX8MP ddr controller is the interrupt
registers. So add another interrupt handler function, enable/disable
interrupt function to distinguish with ZynqMP.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/edac/synopsys_edac.c | 77 +++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index 12211dc040e8..bf4202a24683 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -101,6 +101,7 @@
 /* DDR ECC Quirks */
 #define DDR_ECC_INTR_SUPPORT		BIT(0)
 #define DDR_ECC_DATA_POISON_SUPPORT	BIT(1)
+#define DDR_ECC_IMX8MP			BIT(2)
 
 /* ZynqMP Enhanced DDR memory controller registers that are relevant to ECC */
 /* ECC Configuration Registers */
@@ -266,6 +267,11 @@
 
 #define RANK_B0_BASE			6
 
+/* ECCCTL UE/CE Interrupt enable/disable for IMX8MP*/
+#define DDR_CE_INTR_EN_MASK			0x100
+#define DDR_UE_INTR_EN_MASK			0x200
+#define ECC_INTR_MASK				0x10100
+
 /**
  * struct ecc_error_info - ECC error log information.
  * @row:	Row number.
@@ -516,6 +522,54 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
 	memset(p, 0, sizeof(*p));
 }
 
+static void enable_intr_imx8mp(struct synps_edac_priv *priv)
+{
+	int regval;
+
+	regval = readl(priv->baseaddr + ECC_CLR_OFST);
+	regval |= (DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
+	writel(regval, priv->baseaddr + ECC_CLR_OFST);
+}
+
+static void disable_intr_imx8mp(struct synps_edac_priv *priv)
+{
+	int regval;
+
+	regval = readl(priv->baseaddr + ECC_CLR_OFST);
+	regval &= ~(DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
+	writel(regval, priv->baseaddr + ECC_CLR_OFST);
+}
+
+/* Interrupt Handler for ECC interrupts on imx8mp platform. */
+static irqreturn_t intr_handler_imx8mp(int irq, void *dev_id)
+{
+	const struct synps_platform_data *p_data;
+	struct mem_ctl_info *mci = dev_id;
+	struct synps_edac_priv *priv;
+	int status, regval;
+
+	priv = mci->pvt_info;
+	p_data = priv->p_data;
+
+	regval = readl(priv->baseaddr + ECC_STAT_OFST);
+	if (!(regval & ECC_INTR_MASK))
+		return IRQ_NONE;
+
+	status = p_data->get_error_info(priv);
+	if (status)
+		return IRQ_NONE;
+
+	priv->ce_cnt += priv->stat.ce_cnt;
+	priv->ue_cnt += priv->stat.ue_cnt;
+	handle_error(mci, &priv->stat);
+
+	edac_dbg(3, "Total error count CE %d UE %d\n",
+		 priv->ce_cnt, priv->ue_cnt);
+	enable_intr_imx8mp(priv);
+
+	return IRQ_HANDLED;
+}
+
 /**
  * intr_handler - Interrupt Handler for ECC interrupts.
  * @irq:        IRQ number.
@@ -533,6 +587,9 @@ static irqreturn_t intr_handler(int irq, void *dev_id)
 	priv = mci->pvt_info;
 	p_data = priv->p_data;
 
+	if (p_data->quirks & DDR_ECC_IMX8MP)
+		return intr_handler_imx8mp(irq, dev_id);
+
 	regval = readl(priv->baseaddr + DDR_QOS_IRQ_STAT_OFST);
 	regval &= (DDR_QOSCE_MASK | DDR_QOSUE_MASK);
 	if (!(regval & ECC_CE_UE_INTR_MASK))
@@ -809,7 +866,7 @@ static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
 	platform_set_drvdata(pdev, mci);
 
 	/* Initialize controller capabilities and configuration */
-	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->mtype_cap = MEM_FLAG_LRDDR4 | 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;
@@ -834,6 +891,9 @@ static void mc_init(struct mem_ctl_info *mci, struct platform_device *pdev)
 static void enable_intr(struct synps_edac_priv *priv)
 {
 	/* Enable UE/CE Interrupts */
+	if (priv->p_data->quirks & DDR_ECC_IMX8MP)
+		return enable_intr_imx8mp(priv);
+
 	writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 			priv->baseaddr + DDR_QOS_IRQ_EN_OFST);
 }
@@ -841,6 +901,9 @@ static void enable_intr(struct synps_edac_priv *priv)
 static void disable_intr(struct synps_edac_priv *priv)
 {
 	/* Disable UE/CE Interrupts */
+	if (priv->p_data->quirks & DDR_ECC_IMX8MP)
+		return disable_intr_imx8mp(priv);
+
 	writel(DDR_QOSUE_MASK | DDR_QOSCE_MASK,
 			priv->baseaddr + DDR_QOS_IRQ_DB_OFST);
 }
@@ -890,6 +953,14 @@ static const struct synps_platform_data zynqmp_edac_def = {
 			  ),
 };
 
+static const struct synps_platform_data imx8mp_edac_def = {
+	.get_error_info	= zynqmp_get_error_info,
+	.get_mtype	= zynqmp_get_mtype,
+	.get_dtype	= zynqmp_get_dtype,
+	.get_ecc_state	= zynqmp_get_ecc_state,
+	.quirks         = (DDR_ECC_INTR_SUPPORT | DDR_ECC_IMX8MP),
+};
+
 static const struct of_device_id synps_edac_match[] = {
 	{
 		.compatible = "xlnx,zynq-ddrc-a05",
@@ -899,6 +970,10 @@ static const struct of_device_id synps_edac_match[] = {
 		.compatible = "xlnx,zynqmp-ddrc-2.40a",
 		.data = (void *)&zynqmp_edac_def
 	},
+	{
+		.compatible = "fsl,imx8mp-ddrc",
+		.data = (void *)&imx8mp_edac_def
+	},
 	{
 		/* end of table */
 	}
-- 
2.17.1

[patch v3 4/4] EDAC: synopsys: Add useful debug and output information for 64bit systems

[Sherry Sun]

Now the synopsys_edac driver only support to output the 32-bit error
data, but for 64 bit systems, such as i.MX8MP, 64 bit error data is
needed. At the same time, when CE/UE happens, syndrome data is also
useful to showed to user. So here add data_high and syndrome data for
64-bit systems.

And in order to distinguish 64-bit systems and other systems, here
adjust the position of the zynqmp_get_dtype(), so we can called
this function to distinguish it. To ensure that functions of the same
function are in the same position, here adjust the position of the
zynq_get_dtype() too.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>

---
Changes since v2:
- Use u64 data instead u32 data_low and u32 data_high as robert suggested.
- Add edac_dbg() for the != DEV_X8 cases in zynqmp_get_error_info()
- Change the format of the output data to avoid build warnings.

---
 drivers/edac/synopsys_edac.c | 171 ++++++++++++++++++++---------------
 1 file changed, 96 insertions(+), 75 deletions(-)

diff --git a/drivers/edac/synopsys_edac.c b/drivers/edac/synopsys_edac.c
index bf4202a24683..2bfab62bce1b 100644
--- a/drivers/edac/synopsys_edac.c
+++ b/drivers/edac/synopsys_edac.c
@@ -281,15 +281,17 @@
  * @data:	Data causing the error.
  * @bankgrpnr:	Bank group number.
  * @blknr:	Block number.
+ * @syndrome:	Syndrome of the error.
  */
 struct ecc_error_info {
 	u32 row;
 	u32 col;
 	u32 bank;
 	u32 bitpos;
-	u32 data;
+	u64 data;
 	u32 bankgrpnr;
 	u32 blknr;
+	u32 syndrome;
 };
 
 /**
@@ -354,6 +356,70 @@ struct synps_platform_data {
 	int quirks;
 };
 
+/**
+ * zynq_get_dtype - Return the controller memory width.
+ * @base:	DDR memory controller base address.
+ *
+ * Get the EDAC device type width appropriate for the current controller
+ * configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type zynq_get_dtype(const void __iomem *base)
+{
+	enum dev_type dt;
+	u32 width;
+
+	width = readl(base + CTRL_OFST);
+	width = (width & CTRL_BW_MASK) >> CTRL_BW_SHIFT;
+
+	switch (width) {
+	case DDRCTL_WDTH_16:
+		dt = DEV_X2;
+		break;
+	case DDRCTL_WDTH_32:
+		dt = DEV_X4;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
+
+	return dt;
+}
+
+/**
+ * zynqmp_get_dtype - Return the controller memory width.
+ * @base:	DDR memory controller base address.
+ *
+ * Get the EDAC device type width appropriate for the current controller
+ * configuration.
+ *
+ * Return: a device type width enumeration.
+ */
+static enum dev_type zynqmp_get_dtype(const void __iomem *base)
+{
+	enum dev_type dt;
+	u32 width;
+
+	width = readl(base + CTRL_OFST);
+	width = (width & ECC_CTRL_BUSWIDTH_MASK) >> ECC_CTRL_BUSWIDTH_SHIFT;
+	switch (width) {
+	case DDRCTL_EWDTH_16:
+		dt = DEV_X2;
+		break;
+	case DDRCTL_EWDTH_32:
+		dt = DEV_X4;
+		break;
+	case DDRCTL_EWDTH_64:
+		dt = DEV_X8;
+		break;
+	default:
+		dt = DEV_UNKNOWN;
+	}
+
+	return dt;
+}
+
 /**
  * zynq_get_error_info - Get the current ECC error info.
  * @priv:	DDR memory controller private instance data.
@@ -386,7 +452,7 @@ static int zynq_get_error_info(struct synps_edac_priv *priv)
 	p->ceinfo.col = regval & ADDR_COL_MASK;
 	p->ceinfo.bank = (regval & ADDR_BANK_MASK) >> ADDR_BANK_SHIFT;
 	p->ceinfo.data = readl(base + CE_DATA_31_0_OFST);
-	edac_dbg(3, "CE bit position: %d data: %d\n", p->ceinfo.bitpos,
+	edac_dbg(3, "CE bit position: %d data: %lld\n", p->ceinfo.bitpos,
 		 p->ceinfo.data);
 	clearval = ECC_CTRL_CLR_CE_ERR;
 
@@ -444,9 +510,13 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
 					ECC_CEADDR1_BNKGRP_SHIFT;
 	p->ceinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
 	p->ceinfo.data = readl(base + ECC_CSYND0_OFST);
-	edac_dbg(2, "ECCCSYN0: 0x%08X ECCCSYN1: 0x%08X ECCCSYN2: 0x%08X\n",
-		 readl(base + ECC_CSYND0_OFST), readl(base + ECC_CSYND1_OFST),
-		 readl(base + ECC_CSYND2_OFST));
+	edac_dbg(2, "CE data_low: 0x%08X \n", (u32)(p->ceinfo.data));
+	if (zynqmp_get_dtype(base) == DEV_X8) {
+		p->ceinfo.data |= ((u64)readl(base + ECC_CSYND1_OFST)) << 32;
+		p->ceinfo.syndrome = readl(base + ECC_CSYND2_OFST);
+		edac_dbg(2, "data_high: 0x%08X syndrome: 0x%08X\n",
+			 (u32)(p->ceinfo.data >> 32), p->ceinfo.syndrome);
+	}
 ue_err:
 	if (!p->ue_cnt)
 		goto out;
@@ -460,6 +530,13 @@ static int zynqmp_get_error_info(struct synps_edac_priv *priv)
 					ECC_CEADDR1_BNKNR_SHIFT;
 	p->ueinfo.blknr = (regval & ECC_CEADDR1_BLKNR_MASK);
 	p->ueinfo.data = readl(base + ECC_UESYND0_OFST);
+	edac_dbg(2, "UE data_low: 0x%08X \n", (u32)(p->ueinfo.data));
+	if (zynqmp_get_dtype(base) == DEV_X8) {
+		p->ueinfo.data |= ((u64)readl(base + ECC_UESYND1_OFST)) << 32;
+		p->ueinfo.syndrome = readl(base + ECC_UESYND2_OFST);
+		edac_dbg(2, "data_high: 0x%08X syndrome: 0x%08X\n",
+			 (u32)(p->ueinfo.data >> 32), p->ueinfo.syndrome);
+	}
 out:
 	clearval = ECC_CTRL_CLR_CE_ERR | ECC_CTRL_CLR_CE_ERRCNT;
 	clearval |= ECC_CTRL_CLR_UE_ERR | ECC_CTRL_CLR_UE_ERRCNT;
@@ -480,20 +557,28 @@ static void handle_error(struct mem_ctl_info *mci, struct synps_ecc_status *p)
 {
 	struct synps_edac_priv *priv = mci->pvt_info;
 	struct ecc_error_info *pinf;
+	int n;
 
 	if (p->ce_cnt) {
 		pinf = &p->ceinfo;
 		if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) {
-			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
-				 "DDR ECC error type:%s Row %d Bank %d BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x",
-				 "CE", pinf->row, pinf->bank,
-				 pinf->bankgrpnr, pinf->blknr,
-				 pinf->bitpos, pinf->data);
+			n = snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
+				     "DDR ECC error type:%s Row %d Bank %d BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x",
+				     "CE", pinf->row, pinf->bank,
+				     pinf->bankgrpnr, pinf->blknr,
+				     pinf->bitpos, (u32)(pinf->data));
+
+			if (zynqmp_get_dtype(priv->baseaddr) == DEV_X8)
+				snprintf(priv->message + n,
+					 SYNPS_EDAC_MSG_SIZE - n,
+					 " Data_high: 0x%08x Syndrome: 0x%08x",
+					 (u32)(pinf->data >> 32),
+					 pinf->syndrome);
 		} else {
 			snprintf(priv->message, SYNPS_EDAC_MSG_SIZE,
 				 "DDR ECC error type:%s Row %d Bank %d Col %d Bit Position: %d Data: 0x%08x",
 				 "CE", pinf->row, pinf->bank, pinf->col,
-				 pinf->bitpos, pinf->data);
+				 pinf->bitpos, (u32)(pinf->data));
 		}
 
 		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
@@ -636,70 +721,6 @@ static void check_errors(struct mem_ctl_info *mci)
 		 priv->ce_cnt, priv->ue_cnt);
 }
 
-/**
- * zynq_get_dtype - Return the controller memory width.
- * @base:	DDR memory controller base address.
- *
- * Get the EDAC device type width appropriate for the current controller
- * configuration.
- *
- * Return: a device type width enumeration.
- */
-static enum dev_type zynq_get_dtype(const void __iomem *base)
-{
-	enum dev_type dt;
-	u32 width;
-
-	width = readl(base + CTRL_OFST);
-	width = (width & CTRL_BW_MASK) >> CTRL_BW_SHIFT;
-
-	switch (width) {
-	case DDRCTL_WDTH_16:
-		dt = DEV_X2;
-		break;
-	case DDRCTL_WDTH_32:
-		dt = DEV_X4;
-		break;
-	default:
-		dt = DEV_UNKNOWN;
-	}
-
-	return dt;
-}
-
-/**
- * zynqmp_get_dtype - Return the controller memory width.
- * @base:	DDR memory controller base address.
- *
- * Get the EDAC device type width appropriate for the current controller
- * configuration.
- *
- * Return: a device type width enumeration.
- */
-static enum dev_type zynqmp_get_dtype(const void __iomem *base)
-{
-	enum dev_type dt;
-	u32 width;
-
-	width = readl(base + CTRL_OFST);
-	width = (width & ECC_CTRL_BUSWIDTH_MASK) >> ECC_CTRL_BUSWIDTH_SHIFT;
-	switch (width) {
-	case DDRCTL_EWDTH_16:
-		dt = DEV_X2;
-		break;
-	case DDRCTL_EWDTH_32:
-		dt = DEV_X4;
-		break;
-	case DDRCTL_EWDTH_64:
-		dt = DEV_X8;
-		break;
-	default:
-		dt = DEV_UNKNOWN;
-	}
-
-	return dt;
-}
-
 /**
  * zynq_get_ecc_state - Return the controller ECC enable/disable status.
  * @base:	DDR memory controller base address.
-- 
2.17.1

Re: [patch v3 3/4] EDAC: synopsys: Add edac driver support for i.MX8MP

[Robert Richter]

On 02.04.20 09:20:32, Sherry Sun wrote:
> Since i.MX8MP use synopsys ddr controller IP, so add edac support
> for i.MX8MP based on synopsys edac driver. i.MX8MP use LPDDR4 and
> support interrupts for corrected and uncorrected errors. The main
> difference between ZynqMP and i.MX8MP ddr controller is the interrupt
> registers. So add another interrupt handler function, enable/disable
> interrupt function to distinguish with ZynqMP.
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>  drivers/edac/synopsys_edac.c | 77 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)

> +static void enable_intr_imx8mp(struct synps_edac_priv *priv)
> +{
> +	int regval;
> +
> +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> +	regval |= (DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> +	writel(regval, priv->baseaddr + ECC_CLR_OFST);
> +}
> +
> +static void disable_intr_imx8mp(struct synps_edac_priv *priv)
> +{
> +	int regval;
> +
> +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> +	regval &= ~(DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> +	writel(regval, priv->baseaddr + ECC_CLR_OFST);
> +}
> +
> +/* Interrupt Handler for ECC interrupts on imx8mp platform. */
> +static irqreturn_t intr_handler_imx8mp(int irq, void *dev_id)
> +{
> +	const struct synps_platform_data *p_data;
> +	struct mem_ctl_info *mci = dev_id;
> +	struct synps_edac_priv *priv;
> +	int status, regval;
> +
> +	priv = mci->pvt_info;
> +	p_data = priv->p_data;
> +
> +	regval = readl(priv->baseaddr + ECC_STAT_OFST);
> +	if (!(regval & ECC_INTR_MASK))
> +		return IRQ_NONE;
> +
> +	status = p_data->get_error_info(priv);
> +	if (status)
> +		return IRQ_NONE;
> +
> +	priv->ce_cnt += priv->stat.ce_cnt;
> +	priv->ue_cnt += priv->stat.ue_cnt;
> +	handle_error(mci, &priv->stat);
> +
> +	edac_dbg(3, "Total error count CE %d UE %d\n",
> +		 priv->ce_cnt, priv->ue_cnt);
> +	enable_intr_imx8mp(priv);

Why do you enable interrupts here?

-Robert

> +
> +	return IRQ_HANDLED;
> +}

RE: [patch v3 3/4] EDAC: synopsys: Add edac driver support for i.MX8MP

[Sherry Sun]

Hi Robert,

> -----Original Message-----
> From: Robert Richter <rrichter@marvell.com>
> Sent: 2020年4月2日 15:22
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: bp@alien8.de; mchehab@kernel.org; tony.luck@intel.com;
> james.morse@arm.com; michal.simek@xilinx.com;
> manish.narani@xilinx.com; linux-edac@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Frank Li
> <frank.li@nxp.com>
> Subject: Re: [patch v3 3/4] EDAC: synopsys: Add edac driver support for
> i.MX8MP
> 
> On 02.04.20 09:20:32, Sherry Sun wrote:
> > Since i.MX8MP use synopsys ddr controller IP, so add edac support for
> > i.MX8MP based on synopsys edac driver. i.MX8MP use LPDDR4 and support
> > interrupts for corrected and uncorrected errors. The main difference
> > between ZynqMP and i.MX8MP ddr controller is the interrupt registers.
> > So add another interrupt handler function, enable/disable interrupt
> > function to distinguish with ZynqMP.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> >  drivers/edac/synopsys_edac.c | 77
> > +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 76 insertions(+), 1 deletion(-)
> 
> > +static void enable_intr_imx8mp(struct synps_edac_priv *priv) {
> > +	int regval;
> > +
> > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > +	regval |= (DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > +
> > +static void disable_intr_imx8mp(struct synps_edac_priv *priv) {
> > +	int regval;
> > +
> > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > +	regval &= ~(DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > +
> > +/* Interrupt Handler for ECC interrupts on imx8mp platform. */ static
> > +irqreturn_t intr_handler_imx8mp(int irq, void *dev_id) {
> > +	const struct synps_platform_data *p_data;
> > +	struct mem_ctl_info *mci = dev_id;
> > +	struct synps_edac_priv *priv;
> > +	int status, regval;
> > +
> > +	priv = mci->pvt_info;
> > +	p_data = priv->p_data;
> > +
> > +	regval = readl(priv->baseaddr + ECC_STAT_OFST);
> > +	if (!(regval & ECC_INTR_MASK))
> > +		return IRQ_NONE;
> > +
> > +	status = p_data->get_error_info(priv);
> > +	if (status)
> > +		return IRQ_NONE;
> > +
> > +	priv->ce_cnt += priv->stat.ce_cnt;
> > +	priv->ue_cnt += priv->stat.ue_cnt;
> > +	handle_error(mci, &priv->stat);
> > +
> > +	edac_dbg(3, "Total error count CE %d UE %d\n",
> > +		 priv->ce_cnt, priv->ue_cnt);
> > +	enable_intr_imx8mp(priv);
> 
> Why do you enable interrupts here?

Because zynqmp_get_error_info() wrote 0 to ECC_CLR_OFST, so here have to re-enable the interrupts.

As said in the commit, the main difference between ZynqMP and i.MX8MP ddr controller is the interrupt registers.
ZynqMP use DDR QOS Interrupt registers,  but i.MX8MP use ECC_CLR_OFST Register(bit8 and bit9) to enable/disable the ce/ue interrupts. 

In zynqmp_get_error_info(), Zynqmp wrote 0 to ECC_CLR_OFST register to clear CE/UE error flags and counts, it has no effect on Zynqmp interrupts. 
But for i.MX8MP, wirte 0 to ECC_CLR_OFST will disable i.MX8MP CE/UE interrupt, so need re-enable the interrupts.

Best regards
Sherry Sun

> 
> -Robert
> 
> > +
> > +	return IRQ_HANDLED;
> > +}

Re: [patch v3 3/4] EDAC: synopsys: Add edac driver support for i.MX8MP

[Robert Richter]

On 02.04.20 09:06:27, Sherry Sun wrote:
> > From: Robert Richter <rrichter@marvell.com>
> > On 02.04.20 09:20:32, Sherry Sun wrote:

> > > +static void enable_intr_imx8mp(struct synps_edac_priv *priv) {
> > > +	int regval;
> > > +
> > > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > > +	regval |= (DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > > +
> > > +static void disable_intr_imx8mp(struct synps_edac_priv *priv) {
> > > +	int regval;
> > > +
> > > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > > +	regval &= ~(DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > > +
> > > +/* Interrupt Handler for ECC interrupts on imx8mp platform. */ static
> > > +irqreturn_t intr_handler_imx8mp(int irq, void *dev_id) {
> > > +	const struct synps_platform_data *p_data;
> > > +	struct mem_ctl_info *mci = dev_id;
> > > +	struct synps_edac_priv *priv;
> > > +	int status, regval;
> > > +
> > > +	priv = mci->pvt_info;
> > > +	p_data = priv->p_data;
> > > +
> > > +	regval = readl(priv->baseaddr + ECC_STAT_OFST);
> > > +	if (!(regval & ECC_INTR_MASK))
> > > +		return IRQ_NONE;
> > > +
> > > +	status = p_data->get_error_info(priv);
> > > +	if (status)
> > > +		return IRQ_NONE;
> > > +
> > > +	priv->ce_cnt += priv->stat.ce_cnt;
> > > +	priv->ue_cnt += priv->stat.ue_cnt;
> > > +	handle_error(mci, &priv->stat);
> > > +
> > > +	edac_dbg(3, "Total error count CE %d UE %d\n",
> > > +		 priv->ce_cnt, priv->ue_cnt);
> > > +	enable_intr_imx8mp(priv);
> > 
> > Why do you enable interrupts here?
> 
> Because zynqmp_get_error_info() wrote 0 to ECC_CLR_OFST, so here have to re-enable the interrupts.

This does not seem to be the right place for it.

> As said in the commit, the main difference between ZynqMP and i.MX8MP ddr controller is the interrupt registers.
> ZynqMP use DDR QOS Interrupt registers,  but i.MX8MP use ECC_CLR_OFST Register(bit8 and bit9) to enable/disable the ce/ue interrupts. 
> 
> In zynqmp_get_error_info(), Zynqmp wrote 0 to ECC_CLR_OFST register to clear CE/UE error flags and counts, it has no effect on Zynqmp interrupts. 
> But for i.MX8MP, wirte 0 to ECC_CLR_OFST will disable i.MX8MP CE/UE interrupt, so need re-enable the interrupts.

All this shows one more time there should be separate handlers. You
should get rid most callbacks in struct synps_platform_data and
instead have separate probe functions for both flavors that share
common code.

-Robert

RE: [patch v3 3/4] EDAC: synopsys: Add edac driver support for i.MX8MP

[Sherry Sun]

Hi Robert,

> -----Original Message-----
> From: Robert Richter <rrichter@marvell.com>
> Sent: 2020年4月2日 19:17
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: bp@alien8.de; mchehab@kernel.org; tony.luck@intel.com;
> james.morse@arm.com; michal.simek@xilinx.com;
> manish.narani@xilinx.com; linux-edac@vger.kernel.org; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Frank Li
> <frank.li@nxp.com>
> Subject: Re: [patch v3 3/4] EDAC: synopsys: Add edac driver support for
> i.MX8MP
> 
> On 02.04.20 09:06:27, Sherry Sun wrote:
> > > From: Robert Richter <rrichter@marvell.com> On 02.04.20 09:20:32,
> > > Sherry Sun wrote:
> 
> > > > +static void enable_intr_imx8mp(struct synps_edac_priv *priv) {
> > > > +	int regval;
> > > > +
> > > > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > > > +	regval |= (DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > > > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > > > +
> > > > +static void disable_intr_imx8mp(struct synps_edac_priv *priv) {
> > > > +	int regval;
> > > > +
> > > > +	regval = readl(priv->baseaddr + ECC_CLR_OFST);
> > > > +	regval &= ~(DDR_CE_INTR_EN_MASK | DDR_UE_INTR_EN_MASK);
> > > > +	writel(regval, priv->baseaddr + ECC_CLR_OFST); }
> > > > +
> > > > +/* Interrupt Handler for ECC interrupts on imx8mp platform. */
> > > > +static irqreturn_t intr_handler_imx8mp(int irq, void *dev_id) {
> > > > +	const struct synps_platform_data *p_data;
> > > > +	struct mem_ctl_info *mci = dev_id;
> > > > +	struct synps_edac_priv *priv;
> > > > +	int status, regval;
> > > > +
> > > > +	priv = mci->pvt_info;
> > > > +	p_data = priv->p_data;
> > > > +
> > > > +	regval = readl(priv->baseaddr + ECC_STAT_OFST);
> > > > +	if (!(regval & ECC_INTR_MASK))
> > > > +		return IRQ_NONE;
> > > > +
> > > > +	status = p_data->get_error_info(priv);
> > > > +	if (status)
> > > > +		return IRQ_NONE;
> > > > +
> > > > +	priv->ce_cnt += priv->stat.ce_cnt;
> > > > +	priv->ue_cnt += priv->stat.ue_cnt;
> > > > +	handle_error(mci, &priv->stat);
> > > > +
> > > > +	edac_dbg(3, "Total error count CE %d UE %d\n",
> > > > +		 priv->ce_cnt, priv->ue_cnt);
> > > > +	enable_intr_imx8mp(priv);
> > >
> > > Why do you enable interrupts here?
> >
> > Because zynqmp_get_error_info() wrote 0 to ECC_CLR_OFST, so here have
> to re-enable the interrupts.
> 
> This does not seem to be the right place for it.
> 
> > As said in the commit, the main difference between ZynqMP and i.MX8MP
> ddr controller is the interrupt registers.
> > ZynqMP use DDR QOS Interrupt registers,  but i.MX8MP use ECC_CLR_OFST
> Register(bit8 and bit9) to enable/disable the ce/ue interrupts.
> >
> > In zynqmp_get_error_info(), Zynqmp wrote 0 to ECC_CLR_OFST register to
> clear CE/UE error flags and counts, it has no effect on Zynqmp interrupts.
> > But for i.MX8MP, wirte 0 to ECC_CLR_OFST will disable i.MX8MP CE/UE
> interrupt, so need re-enable the interrupts.
> 
> All this shows one more time there should be separate handlers. You should
> get rid most callbacks in struct synps_platform_data and instead have
> separate probe functions for both flavors that share common code.

Do you mean that I should create specific callbacks in struct synps_platform_data for i.MX8MP instead reuse the ZynqMP functions?
But except this operation of write 0 to ECC_CLR_OFST in zynqmp_get_error_info(), 
the other callbacks in struct synps_platform_data are all the same between ZynqMP and i.MX8MP.

Best regards
Sherry Sun

Re: [patch v3 3/4] EDAC: synopsys: Add edac driver support for i.MX8MP

[Robert Richter]

On 02.04.20 13:09:57, Sherry Sun wrote:
> > From: Robert Richter <rrichter@marvell.com>
> > On 02.04.20 09:06:27, Sherry Sun wrote:
> > > > From: Robert Richter <rrichter@marvell.com> On 02.04.20 09:20:32,

> > > > > +/* Interrupt Handler for ECC interrupts on imx8mp platform. */
> > > > > +static irqreturn_t intr_handler_imx8mp(int irq, void *dev_id) {
> > > > > +	const struct synps_platform_data *p_data;
> > > > > +	struct mem_ctl_info *mci = dev_id;
> > > > > +	struct synps_edac_priv *priv;
> > > > > +	int status, regval;
> > > > > +
> > > > > +	priv = mci->pvt_info;
> > > > > +	p_data = priv->p_data;
> > > > > +
> > > > > +	regval = readl(priv->baseaddr + ECC_STAT_OFST);
> > > > > +	if (!(regval & ECC_INTR_MASK))
> > > > > +		return IRQ_NONE;
> > > > > +
> > > > > +	status = p_data->get_error_info(priv);
> > > > > +	if (status)
> > > > > +		return IRQ_NONE;
> > > > > +
> > > > > +	priv->ce_cnt += priv->stat.ce_cnt;
> > > > > +	priv->ue_cnt += priv->stat.ue_cnt;
> > > > > +	handle_error(mci, &priv->stat);
> > > > > +
> > > > > +	edac_dbg(3, "Total error count CE %d UE %d\n",
> > > > > +		 priv->ce_cnt, priv->ue_cnt);
 > > > > > +	enable_intr_imx8mp(priv);
> > > >
> > > > Why do you enable interrupts here?
> > >
> > > Because zynqmp_get_error_info() wrote 0 to ECC_CLR_OFST, so here have
> > to re-enable the interrupts.
> > 
> > This does not seem to be the right place for it.
> > 
> > > As said in the commit, the main difference between ZynqMP and i.MX8MP
> > ddr controller is the interrupt registers.
> > > ZynqMP use DDR QOS Interrupt registers,  but i.MX8MP use ECC_CLR_OFST
> > Register(bit8 and bit9) to enable/disable the ce/ue interrupts.
> > >
> > > In zynqmp_get_error_info(), Zynqmp wrote 0 to ECC_CLR_OFST register to
> > clear CE/UE error flags and counts, it has no effect on Zynqmp interrupts.
> > > But for i.MX8MP, wirte 0 to ECC_CLR_OFST will disable i.MX8MP CE/UE
> > interrupt, so need re-enable the interrupts.
> > 
> > All this shows one more time there should be separate handlers. You should
> > get rid most callbacks in struct synps_platform_data and instead have
> > separate probe functions for both flavors that share common code.
> 
> Do you mean that I should create specific callbacks in struct synps_platform_data for i.MX8MP instead reuse the ZynqMP functions?
> But except this operation of write 0 to ECC_CLR_OFST in zynqmp_get_error_info(), 
> the other callbacks in struct synps_platform_data are all the same between ZynqMP and i.MX8MP.

Most callbacks in struct synps_platform_data will become obsolete if
there are either device tailored functions or if the width parameter
would be stored in private data and used by a common function.

-Robert