[PATCH v2 0/6] EDAC/fsl-ddr: Add imx9 support

[PATCH v2 0/6] EDAC/fsl-ddr: Add imx9 support

[Frank Li]

Add imx9 support for fsl-ddr.

Patch 1-2 is prepare patch, no function chagne
Patch 3 is small fix for bit shift
Patch 4 is dt binding patch.
Patch 5 is driver change to support imx9
Patch 6 is imx93 dts change

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v2:
- Fix first patch build error.
- Rewrite commit message with AI help.
- Add Krzysztof'r review tag for binding doc.
- Link to v1: https://lore.kernel.org/r/20240709-imx95_edac-v1-0-3e9c146c1b01@nxp.com

---
Frank Li (4):
      EDAC/fsl_ddr: Pass down fsl_mc_pdata in ddr_in32() and ddr_out32()
      EDAC/fsl_ddr: Move global variables into struct fsl_mc_pdata
      dt-bindings: memory: fsl: Add compatible string nxp,imx9-memory-controller
      arm64: dts: imx93: add ddr edac support

Priyanka Singh (1):
      EDAC/fsl_ddr: Fix bad bit shift operations

Ye Li (1):
      EDAC/fsl_ddr: Add support for i.MX9 DDR controller

 .../bindings/memory-controllers/fsl/fsl,ddr.yaml   |  31 ++++-
 arch/arm64/boot/dts/freescale/imx93.dtsi           |   8 ++
 drivers/edac/fsl_ddr_edac.c                        | 136 ++++++++++++++-------
 drivers/edac/fsl_ddr_edac.h                        |  13 ++
 drivers/edac/layerscape_edac.c                     |   1 +
 5 files changed, 142 insertions(+), 47 deletions(-)
---
base-commit: 0cca97bf23640ff68a6e8a74e9b6659fdc27f48c
change-id: 20240704-imx95_edac-209cec208446

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v2 1/6] EDAC/fsl_ddr: Pass down fsl_mc_pdata in ddr_in32() and ddr_out32()

[Frank Li]

Pass down `fsl_mc_pdata` in helper functions `ddr_in32()` and `ddr_out32()`
to prepare for adding iMX9 support. The iMX9 has a slightly different
register layout.

No functional change.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2
- update commit message.
- fix a build error.
---
 drivers/edac/fsl_ddr_edac.c | 64 ++++++++++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index d148d262d0d4d..9924c6b816648 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -35,13 +35,17 @@ static u32 orig_ddr_err_disable;
 static u32 orig_ddr_err_sbe;
 static bool little_endian;
 
-static inline u32 ddr_in32(void __iomem *addr)
+static inline u32 ddr_in32(struct fsl_mc_pdata *pdata, unsigned int off)
 {
+	void __iomem *addr = pdata->mc_vbase + off;
+
 	return little_endian ? ioread32(addr) : ioread32be(addr);
 }
 
-static inline void ddr_out32(void __iomem *addr, u32 value)
+static inline void ddr_out32(struct fsl_mc_pdata *pdata, unsigned int off, u32 value)
 {
+	void __iomem *addr = pdata->mc_vbase + off;
+
 	if (little_endian)
 		iowrite32(value, addr);
 	else
@@ -60,7 +64,7 @@ static ssize_t fsl_mc_inject_data_hi_show(struct device *dev,
 	struct mem_ctl_info *mci = to_mci(dev);
 	struct fsl_mc_pdata *pdata = mci->pvt_info;
 	return sprintf(data, "0x%08x",
-		       ddr_in32(pdata->mc_vbase + FSL_MC_DATA_ERR_INJECT_HI));
+		       ddr_in32(pdata, FSL_MC_DATA_ERR_INJECT_HI));
 }
 
 static ssize_t fsl_mc_inject_data_lo_show(struct device *dev,
@@ -70,7 +74,7 @@ static ssize_t fsl_mc_inject_data_lo_show(struct device *dev,
 	struct mem_ctl_info *mci = to_mci(dev);
 	struct fsl_mc_pdata *pdata = mci->pvt_info;
 	return sprintf(data, "0x%08x",
-		       ddr_in32(pdata->mc_vbase + FSL_MC_DATA_ERR_INJECT_LO));
+		       ddr_in32(pdata, FSL_MC_DATA_ERR_INJECT_LO));
 }
 
 static ssize_t fsl_mc_inject_ctrl_show(struct device *dev,
@@ -80,7 +84,7 @@ static ssize_t fsl_mc_inject_ctrl_show(struct device *dev,
 	struct mem_ctl_info *mci = to_mci(dev);
 	struct fsl_mc_pdata *pdata = mci->pvt_info;
 	return sprintf(data, "0x%08x",
-		       ddr_in32(pdata->mc_vbase + FSL_MC_ECC_ERR_INJECT));
+		       ddr_in32(pdata, FSL_MC_ECC_ERR_INJECT));
 }
 
 static ssize_t fsl_mc_inject_data_hi_store(struct device *dev,
@@ -97,7 +101,7 @@ static ssize_t fsl_mc_inject_data_hi_store(struct device *dev,
 		if (rc)
 			return rc;
 
-		ddr_out32(pdata->mc_vbase + FSL_MC_DATA_ERR_INJECT_HI, val);
+		ddr_out32(pdata, FSL_MC_DATA_ERR_INJECT_HI, val);
 		return count;
 	}
 	return 0;
@@ -117,7 +121,7 @@ static ssize_t fsl_mc_inject_data_lo_store(struct device *dev,
 		if (rc)
 			return rc;
 
-		ddr_out32(pdata->mc_vbase + FSL_MC_DATA_ERR_INJECT_LO, val);
+		ddr_out32(pdata, FSL_MC_DATA_ERR_INJECT_LO, val);
 		return count;
 	}
 	return 0;
@@ -137,7 +141,7 @@ static ssize_t fsl_mc_inject_ctrl_store(struct device *dev,
 		if (rc)
 			return rc;
 
-		ddr_out32(pdata->mc_vbase + FSL_MC_ECC_ERR_INJECT, val);
+		ddr_out32(pdata, FSL_MC_ECC_ERR_INJECT, val);
 		return count;
 	}
 	return 0;
@@ -286,7 +290,7 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 	int bad_data_bit;
 	int bad_ecc_bit;
 
-	err_detect = ddr_in32(pdata->mc_vbase + FSL_MC_ERR_DETECT);
+	err_detect = ddr_in32(pdata, FSL_MC_ERR_DETECT);
 	if (!err_detect)
 		return;
 
@@ -295,14 +299,14 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 
 	/* no more processing if not ECC bit errors */
 	if (!(err_detect & (DDR_EDE_SBE | DDR_EDE_MBE))) {
-		ddr_out32(pdata->mc_vbase + FSL_MC_ERR_DETECT, err_detect);
+		ddr_out32(pdata, FSL_MC_ERR_DETECT, err_detect);
 		return;
 	}
 
-	syndrome = ddr_in32(pdata->mc_vbase + FSL_MC_CAPTURE_ECC);
+	syndrome = ddr_in32(pdata, FSL_MC_CAPTURE_ECC);
 
 	/* Mask off appropriate bits of syndrome based on bus width */
-	bus_width = (ddr_in32(pdata->mc_vbase + FSL_MC_DDR_SDRAM_CFG) &
+	bus_width = (ddr_in32(pdata, FSL_MC_DDR_SDRAM_CFG) &
 		     DSC_DBW_MASK) ? 32 : 64;
 	if (bus_width == 64)
 		syndrome &= 0xff;
@@ -310,8 +314,8 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 		syndrome &= 0xffff;
 
 	err_addr = make64(
-		ddr_in32(pdata->mc_vbase + FSL_MC_CAPTURE_EXT_ADDRESS),
-		ddr_in32(pdata->mc_vbase + FSL_MC_CAPTURE_ADDRESS));
+		ddr_in32(pdata, FSL_MC_CAPTURE_EXT_ADDRESS),
+		ddr_in32(pdata, FSL_MC_CAPTURE_ADDRESS));
 	pfn = err_addr >> PAGE_SHIFT;
 
 	for (row_index = 0; row_index < mci->nr_csrows; row_index++) {
@@ -320,8 +324,8 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 			break;
 	}
 
-	cap_high = ddr_in32(pdata->mc_vbase + FSL_MC_CAPTURE_DATA_HI);
-	cap_low = ddr_in32(pdata->mc_vbase + FSL_MC_CAPTURE_DATA_LO);
+	cap_high = ddr_in32(pdata, FSL_MC_CAPTURE_DATA_HI);
+	cap_low = ddr_in32(pdata, FSL_MC_CAPTURE_DATA_LO);
 
 	/*
 	 * Analyze single-bit errors on 64-bit wide buses
@@ -367,7 +371,7 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 				     row_index, 0, -1,
 				     mci->ctl_name, "");
 
-	ddr_out32(pdata->mc_vbase + FSL_MC_ERR_DETECT, err_detect);
+	ddr_out32(pdata, FSL_MC_ERR_DETECT, err_detect);
 }
 
 static irqreturn_t fsl_mc_isr(int irq, void *dev_id)
@@ -376,7 +380,7 @@ static irqreturn_t fsl_mc_isr(int irq, void *dev_id)
 	struct fsl_mc_pdata *pdata = mci->pvt_info;
 	u32 err_detect;
 
-	err_detect = ddr_in32(pdata->mc_vbase + FSL_MC_ERR_DETECT);
+	err_detect = ddr_in32(pdata, FSL_MC_ERR_DETECT);
 	if (!err_detect)
 		return IRQ_NONE;
 
@@ -396,7 +400,7 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 	u32 cs_bnds;
 	int index;
 
-	sdram_ctl = ddr_in32(pdata->mc_vbase + FSL_MC_DDR_SDRAM_CFG);
+	sdram_ctl = ddr_in32(pdata, FSL_MC_DDR_SDRAM_CFG);
 
 	sdtype = sdram_ctl & DSC_SDTYPE_MASK;
 	if (sdram_ctl & DSC_RD_EN) {
@@ -444,7 +448,7 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 		csrow = mci->csrows[index];
 		dimm = csrow->channels[0]->dimm;
 
-		cs_bnds = ddr_in32(pdata->mc_vbase + FSL_MC_CS_BNDS_0 +
+		cs_bnds = ddr_in32(pdata, FSL_MC_CS_BNDS_0 +
 				   (index * FSL_MC_CS_BNDS_OFS));
 
 		start = (cs_bnds & 0xffff0000) >> 16;
@@ -531,7 +535,7 @@ int fsl_mc_err_probe(struct platform_device *op)
 		goto err;
 	}
 
-	sdram_ctl = ddr_in32(pdata->mc_vbase + FSL_MC_DDR_SDRAM_CFG);
+	sdram_ctl = ddr_in32(pdata, FSL_MC_DDR_SDRAM_CFG);
 	if (!(sdram_ctl & DSC_ECC_EN)) {
 		/* no ECC */
 		pr_warn("%s: No ECC DIMMs discovered\n", __func__);
@@ -558,11 +562,11 @@ int fsl_mc_err_probe(struct platform_device *op)
 	fsl_ddr_init_csrows(mci);
 
 	/* store the original error disable bits */
-	orig_ddr_err_disable = ddr_in32(pdata->mc_vbase + FSL_MC_ERR_DISABLE);
-	ddr_out32(pdata->mc_vbase + FSL_MC_ERR_DISABLE, 0);
+	orig_ddr_err_disable = ddr_in32(pdata, FSL_MC_ERR_DISABLE);
+	ddr_out32(pdata, FSL_MC_ERR_DISABLE, 0);
 
 	/* clear all error bits */
-	ddr_out32(pdata->mc_vbase + FSL_MC_ERR_DETECT, ~0);
+	ddr_out32(pdata, FSL_MC_ERR_DETECT, ~0);
 
 	res = edac_mc_add_mc_with_groups(mci, fsl_ddr_dev_groups);
 	if (res) {
@@ -571,15 +575,15 @@ int fsl_mc_err_probe(struct platform_device *op)
 	}
 
 	if (edac_op_state == EDAC_OPSTATE_INT) {
-		ddr_out32(pdata->mc_vbase + FSL_MC_ERR_INT_EN,
+		ddr_out32(pdata, FSL_MC_ERR_INT_EN,
 			  DDR_EIE_MBEE | DDR_EIE_SBEE);
 
 		/* store the original error management threshold */
-		orig_ddr_err_sbe = ddr_in32(pdata->mc_vbase +
+		orig_ddr_err_sbe = ddr_in32(pdata,
 					    FSL_MC_ERR_SBE) & 0xff0000;
 
 		/* set threshold to 1 error per interrupt */
-		ddr_out32(pdata->mc_vbase + FSL_MC_ERR_SBE, 0x10000);
+		ddr_out32(pdata, FSL_MC_ERR_SBE, 0x10000);
 
 		/* register interrupts */
 		pdata->irq = platform_get_irq(op, 0);
@@ -620,12 +624,12 @@ void fsl_mc_err_remove(struct platform_device *op)
 	edac_dbg(0, "\n");
 
 	if (edac_op_state == EDAC_OPSTATE_INT) {
-		ddr_out32(pdata->mc_vbase + FSL_MC_ERR_INT_EN, 0);
+		ddr_out32(pdata, FSL_MC_ERR_INT_EN, 0);
 	}
 
-	ddr_out32(pdata->mc_vbase + FSL_MC_ERR_DISABLE,
+	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
 		  orig_ddr_err_disable);
-	ddr_out32(pdata->mc_vbase + FSL_MC_ERR_SBE, orig_ddr_err_sbe);
+	ddr_out32(pdata, FSL_MC_ERR_SBE, orig_ddr_err_sbe);
 
 	edac_mc_del_mc(&op->dev);
 	edac_mc_free(mci);

-- 
2.34.1

[PATCH v2 2/6] EDAC/fsl_ddr: Move global variables into struct fsl_mc_pdata

[Frank Li]

Move global variables into the struct `fsl_mc_pdata` to handle systems with
multiple DDR controllers.

No functional change.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
- rework commit message.
---
 drivers/edac/fsl_ddr_edac.c | 21 +++++++++------------
 drivers/edac/fsl_ddr_edac.h |  3 +++
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index 9924c6b816648..7a9fb1202f1a0 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -31,22 +31,18 @@
 
 static int edac_mc_idx;
 
-static u32 orig_ddr_err_disable;
-static u32 orig_ddr_err_sbe;
-static bool little_endian;
-
 static inline u32 ddr_in32(struct fsl_mc_pdata *pdata, unsigned int off)
 {
 	void __iomem *addr = pdata->mc_vbase + off;
 
-	return little_endian ? ioread32(addr) : ioread32be(addr);
+	return pdata->little_endian ? ioread32(addr) : ioread32be(addr);
 }
 
 static inline void ddr_out32(struct fsl_mc_pdata *pdata, unsigned int off, u32 value)
 {
 	void __iomem *addr = pdata->mc_vbase + off;
 
-	if (little_endian)
+	if (pdata->little_endian)
 		iowrite32(value, addr);
 	else
 		iowrite32be(value, addr);
@@ -511,7 +507,7 @@ int fsl_mc_err_probe(struct platform_device *op)
 	 * Get the endianness of DDR controller registers.
 	 * Default is big endian.
 	 */
-	little_endian = of_property_read_bool(op->dev.of_node, "little-endian");
+	pdata->little_endian = of_property_read_bool(op->dev.of_node, "little-endian");
 
 	res = of_address_to_resource(op->dev.of_node, 0, &r);
 	if (res) {
@@ -562,7 +558,7 @@ int fsl_mc_err_probe(struct platform_device *op)
 	fsl_ddr_init_csrows(mci);
 
 	/* store the original error disable bits */
-	orig_ddr_err_disable = ddr_in32(pdata, FSL_MC_ERR_DISABLE);
+	pdata->orig_ddr_err_disable = ddr_in32(pdata, FSL_MC_ERR_DISABLE);
 	ddr_out32(pdata, FSL_MC_ERR_DISABLE, 0);
 
 	/* clear all error bits */
@@ -579,8 +575,8 @@ int fsl_mc_err_probe(struct platform_device *op)
 			  DDR_EIE_MBEE | DDR_EIE_SBEE);
 
 		/* store the original error management threshold */
-		orig_ddr_err_sbe = ddr_in32(pdata,
-					    FSL_MC_ERR_SBE) & 0xff0000;
+		pdata->orig_ddr_err_sbe = ddr_in32(pdata,
+						   FSL_MC_ERR_SBE) & 0xff0000;
 
 		/* set threshold to 1 error per interrupt */
 		ddr_out32(pdata, FSL_MC_ERR_SBE, 0x10000);
@@ -628,8 +624,9 @@ void fsl_mc_err_remove(struct platform_device *op)
 	}
 
 	ddr_out32(pdata, FSL_MC_ERR_DISABLE,
-		  orig_ddr_err_disable);
-	ddr_out32(pdata, FSL_MC_ERR_SBE, orig_ddr_err_sbe);
+		  pdata->orig_ddr_err_disable);
+	ddr_out32(pdata, FSL_MC_ERR_SBE, pdata->orig_ddr_err_sbe);
+
 
 	edac_mc_del_mc(&op->dev);
 	edac_mc_free(mci);
diff --git a/drivers/edac/fsl_ddr_edac.h b/drivers/edac/fsl_ddr_edac.h
index c0994a2a003c2..de66f9822fba1 100644
--- a/drivers/edac/fsl_ddr_edac.h
+++ b/drivers/edac/fsl_ddr_edac.h
@@ -70,6 +70,9 @@ struct fsl_mc_pdata {
 	int edac_idx;
 	void __iomem *mc_vbase;
 	int irq;
+	u32 orig_ddr_err_disable;
+	u32 orig_ddr_err_sbe;
+	bool little_endian;
 };
 int fsl_mc_err_probe(struct platform_device *op);
 void fsl_mc_err_remove(struct platform_device *op);

-- 
2.34.1

[PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

[Frank Li]

From: Priyanka Singh <priyanka.singh@nxp.com>

Fix undefined behavior caused by left-shifting a negative value in the
expression:

    cap_high ^ (1 << (bad_data_bit - 32))

The variable `bad_data_bit` ranges from 0 to 63. When `bad_data_bit` is
less than 32, `bad_data_bit - 32` becomes negative, and left-shifting by a
negative value in C is undefined behavior.

Fix this by checking the range of `bad_data_bit` before performing the
shift.

Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Li Yang <leoyang.li@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/edac/fsl_ddr_edac.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index 7a9fb1202f1a0..ccc13c2adfd6f 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -338,11 +338,18 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
 			fsl_mc_printk(mci, KERN_ERR,
 				"Faulty ECC bit: %d\n", bad_ecc_bit);
 
-		fsl_mc_printk(mci, KERN_ERR,
-			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
-			cap_high ^ (1 << (bad_data_bit - 32)),
-			cap_low ^ (1 << bad_data_bit),
-			syndrome ^ (1 << bad_ecc_bit));
+		if ((bad_data_bit > 0 && bad_data_bit < 32) && bad_ecc_bit > 0) {
+			fsl_mc_printk(mci, KERN_ERR,
+				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
+				      cap_high, cap_low ^ (1 << bad_data_bit),
+				      syndrome ^ (1 << bad_ecc_bit));
+		}
+		if (bad_data_bit >= 32 && bad_ecc_bit > 0) {
+			fsl_mc_printk(mci, KERN_ERR,
+				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
+				      cap_high ^ (1 << (bad_data_bit - 32)),
+				      cap_low, syndrome ^ (1 << bad_ecc_bit));
+		}
 	}
 
 	fsl_mc_printk(mci, KERN_ERR,

-- 
2.34.1

[PATCH v2 4/6] dt-bindings: memory: fsl: Add compatible string nxp,imx9-memory-controller

[Frank Li]

iMX9 memory controller is similar with other layerscape chips. But some
register layout has a little bit difference, so add new compatible string
'nxp,imx9-memory-controller' for it.

imx9 need two 'reg', one for DDR controller and the other is ECC inject
engine register space. Keep the same restriction for other compatible
string.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../bindings/memory-controllers/fsl/fsl,ddr.yaml   | 31 +++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/fsl,ddr.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl/fsl,ddr.yaml
index 84f778a99546b..e0786153eec73 100644
--- a/Documentation/devicetree/bindings/memory-controllers/fsl/fsl,ddr.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/fsl/fsl,ddr.yaml
@@ -40,6 +40,7 @@ properties:
           - fsl,p1021-memory-controller
           - fsl,p2020-memory-controller
           - fsl,qoriq-memory-controller
+          - nxp,imx9-memory-controller
 
   interrupts:
     maxItems: 1
@@ -51,13 +52,41 @@ properties:
     type: boolean
 
   reg:
-    maxItems: 1
+    items:
+      - description: Controller register space
+      - description: Inject register space
+    minItems: 1
+
+  reg-names:
+    items:
+      - const: ctrl
+      - const: inject
+    minItems: 1
 
 required:
   - compatible
   - interrupts
   - reg
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - nxp,imx9-memory-controller
+    then:
+      properties:
+        reg:
+          minItems: 2
+        reg-names:
+          minItems: 2
+    else:
+      properties:
+        reg:
+          maxItems: 1
+        reg-names: false
+
 additionalProperties: false
 
 examples:

-- 
2.34.1

[PATCH v2 5/6] EDAC/fsl_ddr: Add support for i.MX9 DDR controller

[Frank Li]

From: Ye Li <ye.li@nxp.com>

Add support for the i.MX9 DDR controller, which has different register
offsets and some function changes compared to the existing fsl_ddr
controller. The ECC and error injection functions are almost the same, so
update and reuse the driver for i.MX9. A special type 'TYPE_IMX9' is added
specifically for the i.MX9 controller to distinguish the differences.

Signed-off-by: Ye Li <ye.li@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/edac/fsl_ddr_edac.c    | 48 ++++++++++++++++++++++++++++++++++++------
 drivers/edac/fsl_ddr_edac.h    | 10 +++++++++
 drivers/edac/layerscape_edac.c |  1 +
 3 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
index ccc13c2adfd6f..3e4c2869a07cd 100644
--- a/drivers/edac/fsl_ddr_edac.c
+++ b/drivers/edac/fsl_ddr_edac.c
@@ -31,16 +31,28 @@
 
 static int edac_mc_idx;
 
+static inline void __iomem *ddr_reg_addr(struct fsl_mc_pdata *pdata, unsigned int off)
+{
+	if (pdata->flag == TYPE_IMX9 && off >= FSL_MC_DATA_ERR_INJECT_HI && off <= FSL_MC_ERR_SBE)
+		return pdata->inject_vbase + off - FSL_MC_DATA_ERR_INJECT_HI
+		       + IMX9_MC_DATA_ERR_INJECT_OFF;
+
+	if (pdata->flag == TYPE_IMX9 && off >= IMX9_MC_ERR_EN)
+		return pdata->inject_vbase + off - IMX9_MC_ERR_EN;
+
+	return pdata->mc_vbase + off;
+}
+
 static inline u32 ddr_in32(struct fsl_mc_pdata *pdata, unsigned int off)
 {
-	void __iomem *addr = pdata->mc_vbase + off;
+	void __iomem *addr = ddr_reg_addr(pdata, off);
 
 	return pdata->little_endian ? ioread32(addr) : ioread32be(addr);
 }
 
 static inline void ddr_out32(struct fsl_mc_pdata *pdata, unsigned int off, u32 value)
 {
-	void __iomem *addr = pdata->mc_vbase + off;
+	void __iomem *addr = ddr_reg_addr(pdata, off);
 
 	if (pdata->little_endian)
 		iowrite32(value, addr);
@@ -438,6 +450,9 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 		case 0x05000000:
 			mtype = MEM_DDR4;
 			break;
+		case 0x04000000:
+			mtype = MEM_LPDDR4;
+			break;
 		default:
 			mtype = MEM_UNKNOWN;
 			break;
@@ -471,7 +486,9 @@ static void fsl_ddr_init_csrows(struct mem_ctl_info *mci)
 		dimm->grain = 8;
 		dimm->mtype = mtype;
 		dimm->dtype = DEV_UNKNOWN;
-		if (sdram_ctl & DSC_X32_EN)
+		if (pdata->flag == TYPE_IMX9)
+			dimm->dtype = DEV_X16;
+		else if (sdram_ctl & DSC_X32_EN)
 			dimm->dtype = DEV_X32;
 		dimm->edac_mode = EDAC_SECDED;
 	}
@@ -483,6 +500,7 @@ int fsl_mc_err_probe(struct platform_device *op)
 	struct edac_mc_layer layers[2];
 	struct fsl_mc_pdata *pdata;
 	struct resource r;
+	u32 ecc_en_mask;
 	u32 sdram_ctl;
 	int res;
 
@@ -510,6 +528,8 @@ int fsl_mc_err_probe(struct platform_device *op)
 	mci->ctl_name = pdata->name;
 	mci->dev_name = pdata->name;
 
+	pdata->flag = (unsigned long)device_get_match_data(&op->dev);
+
 	/*
 	 * Get the endianness of DDR controller registers.
 	 * Default is big endian.
@@ -538,8 +558,23 @@ int fsl_mc_err_probe(struct platform_device *op)
 		goto err;
 	}
 
-	sdram_ctl = ddr_in32(pdata, FSL_MC_DDR_SDRAM_CFG);
-	if (!(sdram_ctl & DSC_ECC_EN)) {
+	if (pdata->flag == TYPE_IMX9) {
+		pdata->inject_vbase = devm_platform_ioremap_resource_byname(op, "inject");
+		if (IS_ERR(pdata->inject_vbase)) {
+			res = -ENOMEM;
+			goto err;
+		}
+	}
+
+	if (pdata->flag == TYPE_IMX9) {
+		sdram_ctl = ddr_in32(pdata, IMX9_MC_ERR_EN);
+		ecc_en_mask = ERR_ECC_EN | ERR_INLINE_ECC;
+	} else {
+		sdram_ctl = ddr_in32(pdata, FSL_MC_DDR_SDRAM_CFG);
+		ecc_en_mask = DSC_ECC_EN;
+	}
+
+	if ((sdram_ctl & ecc_en_mask) != ecc_en_mask) {
 		/* no ECC */
 		pr_warn("%s: No ECC DIMMs discovered\n", __func__);
 		res = -ENODEV;
@@ -550,7 +585,8 @@ int fsl_mc_err_probe(struct platform_device *op)
 	mci->mtype_cap = MEM_FLAG_DDR | MEM_FLAG_RDDR |
 			 MEM_FLAG_DDR2 | MEM_FLAG_RDDR2 |
 			 MEM_FLAG_DDR3 | MEM_FLAG_RDDR3 |
-			 MEM_FLAG_DDR4 | MEM_FLAG_RDDR4;
+			 MEM_FLAG_DDR4 | MEM_FLAG_RDDR4 |
+			 MEM_FLAG_LPDDR4;
 	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
 	mci->edac_cap = EDAC_FLAG_SECDED;
 	mci->mod_name = EDAC_MOD_STR;
diff --git a/drivers/edac/fsl_ddr_edac.h b/drivers/edac/fsl_ddr_edac.h
index de66f9822fba1..73618f79e587f 100644
--- a/drivers/edac/fsl_ddr_edac.h
+++ b/drivers/edac/fsl_ddr_edac.h
@@ -39,6 +39,9 @@
 #define FSL_MC_CAPTURE_EXT_ADDRESS	0x0e54
 #define FSL_MC_ERR_SBE		0x0e58
 
+#define IMX9_MC_ERR_EN			0x1000
+#define IMX9_MC_DATA_ERR_INJECT_OFF	0x100
+
 #define DSC_MEM_EN	0x80000000
 #define DSC_ECC_EN	0x20000000
 #define DSC_RD_EN	0x10000000
@@ -46,6 +49,9 @@
 #define DSC_DBW_32	0x00080000
 #define DSC_DBW_64	0x00000000
 
+#define ERR_ECC_EN      0x80000000
+#define ERR_INLINE_ECC  0x40000000
+
 #define DSC_SDTYPE_MASK		0x07000000
 #define DSC_X32_EN	0x00000020
 
@@ -65,14 +71,18 @@
 #define	DDR_EDI_SBED	0x4	/* single-bit ECC error disable */
 #define	DDR_EDI_MBED	0x8	/* multi-bit ECC error disable */
 
+#define TYPE_IMX9	0x1	/* MC used by iMX9 having registers changed */
+
 struct fsl_mc_pdata {
 	char *name;
 	int edac_idx;
 	void __iomem *mc_vbase;
+	void __iomem *inject_vbase;
 	int irq;
 	u32 orig_ddr_err_disable;
 	u32 orig_ddr_err_sbe;
 	bool little_endian;
+	unsigned long flag;
 };
 int fsl_mc_err_probe(struct platform_device *op);
 void fsl_mc_err_remove(struct platform_device *op);
diff --git a/drivers/edac/layerscape_edac.c b/drivers/edac/layerscape_edac.c
index 0d42c1238908b..9a0c92ebbc3c4 100644
--- a/drivers/edac/layerscape_edac.c
+++ b/drivers/edac/layerscape_edac.c
@@ -21,6 +21,7 @@
 
 static const struct of_device_id fsl_ddr_mc_err_of_match[] = {
 	{ .compatible = "fsl,qoriq-memory-controller", },
+	{ .compatible = "nxp,imx9-memory-controller", .data = (void *)TYPE_IMX9, },
 	{},
 };
 MODULE_DEVICE_TABLE(of, fsl_ddr_mc_err_of_match);

-- 
2.34.1

[PATCH v2 6/6] arm64: dts: imx93: add ddr edac support

[Frank Li]

Add ddr edac support for imx93.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx93.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
index 04b9b3d31f4fa..ea83e38ae4d88 100644
--- a/arch/arm64/boot/dts/freescale/imx93.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
@@ -1326,6 +1326,14 @@ usbmisc2: usbmisc@4c200200 {
 			#index-cells = <1>;
 		};
 
+		memory-controller@4e300000 {
+			compatible = "nxp,imx9-memory-controller";
+			reg = <0x4e300000 0x800>, <0x4e301000 0x1000>;
+			reg-names = "ctrl", "inject";
+			interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+			little-endian;
+		};
+
 		ddr-pmu@4e300dc0 {
 			compatible = "fsl,imx93-ddr-pmu";
 			reg = <0x4e300dc0 0x200>;

-- 
2.34.1

Re: [PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

[Borislav Petkov]

On Fri, Oct 11, 2024 at 11:31:31AM -0400, Frank Li wrote:
> From: Priyanka Singh <priyanka.singh@nxp.com>
> 
> Fix undefined behavior caused by left-shifting a negative value in the
> expression:
> 
>     cap_high ^ (1 << (bad_data_bit - 32))
> 
> The variable `bad_data_bit` ranges from 0 to 63. When `bad_data_bit` is
> less than 32, `bad_data_bit - 32` becomes negative, and left-shifting by a
> negative value in C is undefined behavior.
> 
> Fix this by checking the range of `bad_data_bit` before performing the
> shift.
> 
> Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")

Is this an urgent fix which needs to go to stable or someone just caught it
from code review?

Does it trigger in real life, IOW?

> Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
> Signed-off-by: Li Yang <leoyang.li@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/edac/fsl_ddr_edac.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index 7a9fb1202f1a0..ccc13c2adfd6f 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -338,11 +338,18 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
>  			fsl_mc_printk(mci, KERN_ERR,
>  				"Faulty ECC bit: %d\n", bad_ecc_bit);
>  
> -		fsl_mc_printk(mci, KERN_ERR,
> -			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> -			cap_high ^ (1 << (bad_data_bit - 32)),
> -			cap_low ^ (1 << bad_data_bit),
> -			syndrome ^ (1 << bad_ecc_bit));
> +		if ((bad_data_bit > 0 && bad_data_bit < 32) && bad_ecc_bit > 0) {
> +			fsl_mc_printk(mci, KERN_ERR,
> +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> +				      cap_high, cap_low ^ (1 << bad_data_bit),
> +				      syndrome ^ (1 << bad_ecc_bit));
> +		}
> +		if (bad_data_bit >= 32 && bad_ecc_bit > 0) {
> +			fsl_mc_printk(mci, KERN_ERR,
> +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> +				      cap_high ^ (1 << (bad_data_bit - 32)),
> +				      cap_low, syndrome ^ (1 << bad_ecc_bit));
> +		}

This is getting unnecessarily clumsy than it should be. Please do the
following:

	if (bad_data_bit != 1 && bad_ecc_bit != -1) {

		// prep the values you need to print

		// do an exactly one fsl_mc_printk() with the prepared values.

	}

Not have 4 fsl_mc_printks with a bunch of silly if-checks in front.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 5/6] EDAC/fsl_ddr: Add support for i.MX9 DDR controller

[Borislav Petkov]

On Fri, Oct 11, 2024 at 11:31:33AM -0400, Frank Li wrote:
> From: Ye Li <ye.li@nxp.com>
> 
> Add support for the i.MX9 DDR controller, which has different register
> offsets and some function changes compared to the existing fsl_ddr
> controller. The ECC and error injection functions are almost the same, so
> update and reuse the driver for i.MX9. A special type 'TYPE_IMX9' is added
> specifically for the i.MX9 controller to distinguish the differences.
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/edac/fsl_ddr_edac.c    | 48 ++++++++++++++++++++++++++++++++++++------
>  drivers/edac/fsl_ddr_edac.h    | 10 +++++++++
>  drivers/edac/layerscape_edac.c |  1 +
>  3 files changed, 53 insertions(+), 6 deletions(-)

checking file drivers/edac/fsl_ddr_edac.c
Hunk #1 FAILED at 31.
Hunk #2 succeeded at 442 (offset 4 lines).
Hunk #3 succeeded at 478 (offset 4 lines).
Hunk #4 succeeded at 492 (offset 4 lines).
Hunk #5 succeeded at 520 (offset 4 lines).
Hunk #6 succeeded at 550 (offset 4 lines).
Hunk #7 succeeded at 577 (offset 4 lines).
1 out of 7 hunks FAILED
checking file drivers/edac/fsl_ddr_edac.h
Hunk #3 FAILED at 71.
1 out of 3 hunks FAILED
checking file drivers/edac/layerscape_edac.c

What tree have you created your patches against?

> diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> index ccc13c2adfd6f..3e4c2869a07cd 100644
> --- a/drivers/edac/fsl_ddr_edac.c
> +++ b/drivers/edac/fsl_ddr_edac.c
> @@ -31,16 +31,28 @@
>  
>  static int edac_mc_idx;

I see here:

|static int edac_mc_idx;
|
|static u32 orig_ddr_err_disable; 
|static u32 orig_ddr_err_sbe;
|static bool little_endian;
|
|static inline u32 ddr_in32(struct fsl_mc_pdata *pdata, unsigned int off)

and you don't have those in your diff.

What's up?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 5/6] EDAC/fsl_ddr: Add support for i.MX9 DDR controller

[Frank Li]

On Tue, Oct 15, 2024 at 04:52:34PM +0200, Borislav Petkov wrote:
> On Fri, Oct 11, 2024 at 11:31:33AM -0400, Frank Li wrote:
> > From: Ye Li <ye.li@nxp.com>
> >
> > Add support for the i.MX9 DDR controller, which has different register
> > offsets and some function changes compared to the existing fsl_ddr
> > controller. The ECC and error injection functions are almost the same, so
> > update and reuse the driver for i.MX9. A special type 'TYPE_IMX9' is added
> > specifically for the i.MX9 controller to distinguish the differences.
> >
> > Signed-off-by: Ye Li <ye.li@nxp.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/edac/fsl_ddr_edac.c    | 48 ++++++++++++++++++++++++++++++++++++------
> >  drivers/edac/fsl_ddr_edac.h    | 10 +++++++++
> >  drivers/edac/layerscape_edac.c |  1 +
> >  3 files changed, 53 insertions(+), 6 deletions(-)
>
> checking file drivers/edac/fsl_ddr_edac.c
> Hunk #1 FAILED at 31.
> Hunk #2 succeeded at 442 (offset 4 lines).
> Hunk #3 succeeded at 478 (offset 4 lines).
> Hunk #4 succeeded at 492 (offset 4 lines).
> Hunk #5 succeeded at 520 (offset 4 lines).
> Hunk #6 succeeded at 550 (offset 4 lines).
> Hunk #7 succeeded at 577 (offset 4 lines).
> 1 out of 7 hunks FAILED
> checking file drivers/edac/fsl_ddr_edac.h
> Hunk #3 FAILED at 71.
> 1 out of 3 hunks FAILED
> checking file drivers/edac/layerscape_edac.c
>
> What tree have you created your patches against?

I base on linux-next: next-20241010. I can rebase to edac-for-next
https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/

Is it what you expect base?

Frank

>
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index ccc13c2adfd6f..3e4c2869a07cd 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -31,16 +31,28 @@
> >
> >  static int edac_mc_idx;
>
> I see here:
>
> |static int edac_mc_idx;
> |
> |static u32 orig_ddr_err_disable;
> |static u32 orig_ddr_err_sbe;
> |static bool little_endian;
> |
> |static inline u32 ddr_in32(struct fsl_mc_pdata *pdata, unsigned int off)
>
> and you don't have those in your diff.
>
> What's up?
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

[Frank Li]

On Mon, Oct 14, 2024 at 08:16:47PM +0200, Borislav Petkov wrote:
> On Fri, Oct 11, 2024 at 11:31:31AM -0400, Frank Li wrote:
> > From: Priyanka Singh <priyanka.singh@nxp.com>
> >
> > Fix undefined behavior caused by left-shifting a negative value in the
> > expression:
> >
> >     cap_high ^ (1 << (bad_data_bit - 32))
> >
> > The variable `bad_data_bit` ranges from 0 to 63. When `bad_data_bit` is
> > less than 32, `bad_data_bit - 32` becomes negative, and left-shifting by a
> > negative value in C is undefined behavior.
> >
> > Fix this by checking the range of `bad_data_bit` before performing the
> > shift.
> >
> > Fixes: ea2eb9a8b620 ("EDAC, fsl-ddr: Separate FSL DDR driver from MPC85xx")
>
> Is this an urgent fix which needs to go to stable or someone just caught it
> from code review?

I don't think it is urgent. In most system the return value is 0. I am not
sure who caught it because patch already exist at downstream tree for a
whole.

>
> Does it trigger in real life, IOW?

The problem is triggered. But the output result is correct at our hardware.
The result may change depend on compiler and cpu version.

Frank
>
> > Signed-off-by: Priyanka Singh <priyanka.singh@nxp.com>
> > Reviewed-by: Sherry Sun <sherry.sun@nxp.com>
> > Signed-off-by: Li Yang <leoyang.li@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/edac/fsl_ddr_edac.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > index 7a9fb1202f1a0..ccc13c2adfd6f 100644
> > --- a/drivers/edac/fsl_ddr_edac.c
> > +++ b/drivers/edac/fsl_ddr_edac.c
> > @@ -338,11 +338,18 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> >  			fsl_mc_printk(mci, KERN_ERR,
> >  				"Faulty ECC bit: %d\n", bad_ecc_bit);
> >
> > -		fsl_mc_printk(mci, KERN_ERR,
> > -			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > -			cap_high ^ (1 << (bad_data_bit - 32)),
> > -			cap_low ^ (1 << bad_data_bit),
> > -			syndrome ^ (1 << bad_ecc_bit));
> > +		if ((bad_data_bit > 0 && bad_data_bit < 32) && bad_ecc_bit > 0) {
> > +			fsl_mc_printk(mci, KERN_ERR,
> > +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > +				      cap_high, cap_low ^ (1 << bad_data_bit),
> > +				      syndrome ^ (1 << bad_ecc_bit));
> > +		}
> > +		if (bad_data_bit >= 32 && bad_ecc_bit > 0) {
> > +			fsl_mc_printk(mci, KERN_ERR,
> > +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > +				      cap_high ^ (1 << (bad_data_bit - 32)),
> > +				      cap_low, syndrome ^ (1 << bad_ecc_bit));
> > +		}
>
> This is getting unnecessarily clumsy than it should be. Please do the
> following:
>
> 	if (bad_data_bit != 1 && bad_ecc_bit != -1) {
>
> 		// prep the values you need to print
>
> 		// do an exactly one fsl_mc_printk() with the prepared values.
>
> 	}
>
> Not have 4 fsl_mc_printks with a bunch of silly if-checks in front.
>
> Thx.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 5/6] EDAC/fsl_ddr: Add support for i.MX9 DDR controller

[Borislav Petkov]

On Tue, Oct 15, 2024 at 11:19:42AM -0400, Frank Li wrote:
> I base on linux-next: next-20241010.

Found what the problem is. Your patch 2/6 was in my spam folder. And that one
removes those variables.

> I can rebase to edac-for-next
> https://git.kernel.org/pub/scm/linux/kernel/git/ras/ras.git/
> 
> Is it what you expect base?

Yes, please.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

[Borislav Petkov]

On Tue, Oct 15, 2024 at 11:31:41AM -0400, Frank Li wrote:
> I don't think it is urgent. In most system the return value is 0. I am not
> sure who caught it because patch already exist at downstream tree for a
> whole.

That current patch looks like it needs rethinking and making it sane - see
below.

> > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > > index 7a9fb1202f1a0..ccc13c2adfd6f 100644
> > > --- a/drivers/edac/fsl_ddr_edac.c
> > > +++ b/drivers/edac/fsl_ddr_edac.c
> > > @@ -338,11 +338,18 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> > >  			fsl_mc_printk(mci, KERN_ERR,
> > >  				"Faulty ECC bit: %d\n", bad_ecc_bit);
> > >
> > > -		fsl_mc_printk(mci, KERN_ERR,
> > > -			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > > -			cap_high ^ (1 << (bad_data_bit - 32)),
> > > -			cap_low ^ (1 << bad_data_bit),
> > > -			syndrome ^ (1 << bad_ecc_bit));
> > > +		if ((bad_data_bit > 0 && bad_data_bit < 32) && bad_ecc_bit > 0) {
> > > +			fsl_mc_printk(mci, KERN_ERR,
> > > +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > > +				      cap_high, cap_low ^ (1 << bad_data_bit),
> > > +				      syndrome ^ (1 << bad_ecc_bit));
> > > +		}
> > > +		if (bad_data_bit >= 32 && bad_ecc_bit > 0) {
> > > +			fsl_mc_printk(mci, KERN_ERR,
> > > +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > > +				      cap_high ^ (1 << (bad_data_bit - 32)),
> > > +				      cap_low, syndrome ^ (1 << bad_ecc_bit));
> > > +		}
> >
> > This is getting unnecessarily clumsy than it should be. Please do the
> > following:
> >
> > 	if (bad_data_bit != 1 && bad_ecc_bit != -1) {
> >
> > 		// prep the values you need to print
> >
> > 		// do an exactly one fsl_mc_printk() with the prepared values.
> >
> > 	}
> >
> > Not have 4 fsl_mc_printks with a bunch of silly if-checks in front.

You missed the most important feedback. See above. ^^^^^^^

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2 3/6] EDAC/fsl_ddr: Fix bad bit shift operations

[Frank Li]

On Tue, Oct 15, 2024 at 06:11:39PM +0200, Borislav Petkov wrote:
> On Tue, Oct 15, 2024 at 11:31:41AM -0400, Frank Li wrote:
> > I don't think it is urgent. In most system the return value is 0. I am not
> > sure who caught it because patch already exist at downstream tree for a
> > whole.
>
> That current patch looks like it needs rethinking and making it sane - see
> below.
>
> > > > diff --git a/drivers/edac/fsl_ddr_edac.c b/drivers/edac/fsl_ddr_edac.c
> > > > index 7a9fb1202f1a0..ccc13c2adfd6f 100644
> > > > --- a/drivers/edac/fsl_ddr_edac.c
> > > > +++ b/drivers/edac/fsl_ddr_edac.c
> > > > @@ -338,11 +338,18 @@ static void fsl_mc_check(struct mem_ctl_info *mci)
> > > >  			fsl_mc_printk(mci, KERN_ERR,
> > > >  				"Faulty ECC bit: %d\n", bad_ecc_bit);
> > > >
> > > > -		fsl_mc_printk(mci, KERN_ERR,
> > > > -			"Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > > > -			cap_high ^ (1 << (bad_data_bit - 32)),
> > > > -			cap_low ^ (1 << bad_data_bit),
> > > > -			syndrome ^ (1 << bad_ecc_bit));
> > > > +		if ((bad_data_bit > 0 && bad_data_bit < 32) && bad_ecc_bit > 0) {
> > > > +			fsl_mc_printk(mci, KERN_ERR,
> > > > +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > > > +				      cap_high, cap_low ^ (1 << bad_data_bit),
> > > > +				      syndrome ^ (1 << bad_ecc_bit));
> > > > +		}
> > > > +		if (bad_data_bit >= 32 && bad_ecc_bit > 0) {
> > > > +			fsl_mc_printk(mci, KERN_ERR,
> > > > +				      "Expected Data / ECC:\t%#8.8x_%08x / %#2.2x\n",
> > > > +				      cap_high ^ (1 << (bad_data_bit - 32)),
> > > > +				      cap_low, syndrome ^ (1 << bad_ecc_bit));
> > > > +		}
> > >
> > > This is getting unnecessarily clumsy than it should be. Please do the
> > > following:
> > >
> > > 	if (bad_data_bit != 1 && bad_ecc_bit != -1) {
> > >
> > > 		// prep the values you need to print
> > >
> > > 		// do an exactly one fsl_mc_printk() with the prepared values.
> > >
> > > 	}
> > >
> > > Not have 4 fsl_mc_printks with a bunch of silly if-checks in front.
>
> You missed the most important feedback. See above. ^^^^^^^

Sorry, will fix at next version.

Frank
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette