[PATCH v5 0/3] Add Renesas RZ/N1x EDAC driver

[PATCH v5 0/3] Add Renesas RZ/N1x EDAC driver

[Paul Louvel]

The Cadence memory controller found on Renesas RZ/N1x SoCs supports ECC
with SECDED.

The memory controller found on the r9a06g032 supports at most a single
DIMM of DDR2/3, up to 2GB.

Add the EDAC driver for this memory controller, and the relevant
device-tree binding. Also add the EDAC node to the existing r9a06g032
SoC base device-tree.

Signed-off-by: Paul Louvel (Schneider Electric) <paul.louvel@bootlin.com>
---
Changes in v5:

- Patch 1:

    - Reworded commit title.

    - Dropped unused label.

    - Switch to plain flow scalar for description.

    Note: keeping cdns,ddr-edac compatible after Krzysztof's comment.

- Patch 2:

    - Setup IRQ after allocating and registering the device to EDAC
      core.

    - Explicitly free the interrupt line with devm_free_irq() to avoid
      any UAF before releasing MC.

- Link to v4: https://patch.msgid.link/20260928-paul-v7-3-rc1-edac-v4-0-4646a5b7cac9@bootlin.com

Changes in v4:
- Patch 1 and 3: drop the RZN1 family compatible, as requested by Geert.
- Patch 2:

    - Instead of using FIELD_MODIFY macro that expect a compile-time
      constant mask, use binary operators instead. Note that the new
      code is doing exactly what FIELD_MODIFY do, minus the error if
      cdns_rmw() is not inlined.

    - Use a spinlock instead of mutex since the locked region will be
      really fast to execute. Only lock it once for two register rmw.

    - Use edac_debugfs_* facility instead of sysfs attributes. Use a
      separate "bits" and "inject" file instead of just a single file.
      This is better since clients can read the previously set syndrome.

    - Cleaned up the probe function in separate functions. Use a remove
      function instead of registering custom devm action. Use a goto for
      resources cleaning.

    - Mask all interrupts before un-registering the memory controller.

- Link to v3: https://patch.msgid.link/20260924-paul-v7-3-rc1-edac-v3-0-bd8054a5b180@bootlin.com

Changes in v3:
- Patch 2:

    - Added trailing newline to end message.

    - Forgot mutex_init() in probe...

    - Registering the MC at the end of the probe.

- Link to v2: https://patch.msgid.link/20260924-paul-v7-3-rc1-edac-v2-0-bc1406161ecc@bootlin.com

Changes in v2:
- Patch 2:

    - FIELD_MODIFY() is always called with a compile-time constant mask
      in this driver. I see no problem here.

    - Use devm_clk_get_enabled() so the driver does not need to store
      struct clk_bulk_data outside the stack space.
      Clocks are not manipulated outside of probe.

    - Introduced a mutex for rmw operations in inject_ctrl_store() sysfs
      callback. Rename drv to priv in cdns_rmw().

    - Return IRQ_NONE if the stat int register is empty.

    - I see no reason to follow Sashiko last remark on this patch.
      Is an unhandled interrupt a big deal in this case ?

- Link to v1: https://patch.msgid.link/20260924-paul-v7-3-rc1-edac-v1-0-70be37c41a18@bootlin.com

---
Paul Louvel (3):
      dt-bindings: edac: cdns,ddr-edac: add Cadence DDR EDAC
      EDAC/cadence: Add Cadence DDR EDAC driver
      ARM: dts: renesas: r9a06g032: add EDAC node

 .../devicetree/bindings/edac/cdns,ddr-edac.yaml    |  61 ++++
 MAINTAINERS                                        |   7 +
 arch/arm/boot/dts/renesas/r9a06g032.dtsi           |   9 +
 drivers/edac/Kconfig                               |  10 +
 drivers/edac/Makefile                              |   1 +
 drivers/edac/cadence_edac.c                        | 399 +++++++++++++++++++++
 6 files changed, 487 insertions(+)
---
base-commit: d9f39b2c0579f313d14954a9ec584511c543aab5
change-id: 20260916-paul-v7-3-rc1-edac-1f479f96fc57

Best regards,
--  
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v5 1/3] dt-bindings: edac: cdns,ddr-edac: add Cadence DDR EDAC

[Paul Louvel]

Add the Cadence EDAC dt binding.
The memory controller optionally supports ECC SECDED with DDR2/DDR3
memory.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Paul Louvel (Schneider Electric) <paul.louvel@bootlin.com>
---
 .../devicetree/bindings/edac/cdns,ddr-edac.yaml    | 61 ++++++++++++++++++++++
 MAINTAINERS                                        |  6 +++
 2 files changed, 67 insertions(+)

diff --git a/Documentation/devicetree/bindings/edac/cdns,ddr-edac.yaml b/Documentation/devicetree/bindings/edac/cdns,ddr-edac.yaml
new file mode 100644
index 000000000000..9414cb9c59b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/edac/cdns,ddr-edac.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/edac/cdns,ddr-edac.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cadence DDR EDAC
+
+maintainers:
+  - Paul Louvel <paul.louvel@bootlin.com>
+
+description:
+  The Cadence DDR supports DDR2 and DDR3 memory with or without ECC.
+  The bootloader must configure ECC mode in the memory controller.
+
+  The memory controller supports SECDED (single bit error correction, double bit
+  error detection). ECC scrubbing has to be done via software.
+
+properties:
+  compatible:
+    items:
+      - const: renesas,r9a06g032-ddr-edac # RZ/N1D
+      - const: cdns,ddr-edac
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: DDR controller clock
+      - description: APB internal bus clock
+
+  clock-names:
+    items:
+      - const: ddrc
+      - const: pclk
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/r9a06g032-sysctrl.h>
+
+    memory-controller@4000d000 {
+      compatible = "renesas,r9a06g032-ddr-edac", "cdns,ddr-edac";
+      reg = <0x4000d000 0x1000>;
+      interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&sysctrl R9A06G032_CLK_DDRC>, <&sysctrl R9A06G032_HCLK_DDRC>;
+      clock-names = "ddrc", "pclk";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3a19da74d00c..60db3734df5b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5760,6 +5760,12 @@ L:	linux-mm@kvack.org
 S:	Maintained
 F:	tools/testing/selftests/cachestat/test_cachestat.c
 
+CADENCE DDR EDAC DRIVER
+M:	Paul Louvel <paul.louvel@bootlin.com>
+L:	linux-edac@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/edac/cdns,ddr-edac.yaml
+
 CADENCE MIPI-CSI2 BRIDGES
 M:	Maxime Ripard <mripard@kernel.org>
 L:	linux-media@vger.kernel.org

-- 
2.55.0

[PATCH v5 2/3] EDAC/cadence: Add Cadence DDR EDAC driver

[Paul Louvel]

Add the Cadence EDAC driver found on Renesas RZ/N1x SoC.
The memory controller supports ECC, software scrubbing, and SECDED.

Signed-off-by: Paul Louvel (Schneider Electric) <paul.louvel@bootlin.com>
---
 MAINTAINERS                 |   1 +
 drivers/edac/Kconfig        |  10 ++
 drivers/edac/Makefile       |   1 +
 drivers/edac/cadence_edac.c | 399 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 411 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 60db3734df5b..0c7ab1d22172 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5765,6 +5765,7 @@ M:	Paul Louvel <paul.louvel@bootlin.com>
 L:	linux-edac@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/edac/cdns,ddr-edac.yaml
+F:	drivers/edac/cadence_edac.c
 
 CADENCE MIPI-CSI2 BRIDGES
 M:	Maxime Ripard <mripard@kernel.org>
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index a44b85c440ca..1707fe76b53c 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -503,6 +503,16 @@ config EDAC_QCOM
 	  For debugging issues having to do with stability and overall system
 	  health, you should probably say 'Y' here.
 
+config EDAC_CADENCE
+	tristate "Cadence EDAC Controller"
+	depends on HAS_IOMEM && OF
+	depends on ARCH_RZN1 || COMPILE_TEST
+	help
+	  Support for error detection and correction on RZN1x SoCs. ECC must be
+	  configured by the bootloader.
+	  The controller supports single bit error correction, double bit error
+	  detection.
+
 config EDAC_ASPEED
 	tristate "Aspeed AST BMC SoC"
 	depends on ARCH_ASPEED
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index a37534300ab9..0d66a072b15c 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
 obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
 obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
 obj-$(CONFIG_EDAC_QCOM)			+= qcom_edac.o
+obj-$(CONFIG_EDAC_CADENCE)		+= cadence_edac.o
 obj-$(CONFIG_EDAC_ASPEED)		+= aspeed_edac.o
 obj-$(CONFIG_EDAC_BLUEFIELD)		+= bluefield_edac.o
 obj-$(CONFIG_EDAC_DMC520)		+= dmc520_edac.o
diff --git a/drivers/edac/cadence_edac.c b/drivers/edac/cadence_edac.c
new file mode 100644
index 000000000000..26b9a7facd57
--- /dev/null
+++ b/drivers/edac/cadence_edac.c
@@ -0,0 +1,399 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2015 Renesas Electronics Europe Ltd.
+ * Copyright 2026 Bootlin
+ *
+ * Based on highbank EDAC driver:
+ *
+ * Copyright 2011-2012 Calxeda, Inc.
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/of_address.h>
+#include <linux/string.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+
+#include "edac_mc.h"
+#include "edac_module.h"
+
+#define DRV_NAME "cdns_edac"
+
+#define REG_BYTE_SZ 4
+#define DDR_CTL(n) ((n) * REG_BYTE_SZ)
+
+#define CDNS_DDR_DDR_STAT DDR_CTL(0)
+#define CDNS_DDR_DDR_STAT_DRAM_CLASS GENMASK_U32(11, 8)
+#define CDNS_DDR_DDR_STAT_DRAM_DDR2 BIT(2)
+#define CDNS_DDR_DDR_STAT_GET_DRAM_CLASS(reg) FIELD_GET(CDNS_DDR_DDR_STAT_DRAM_CLASS, reg)
+
+#define CDNS_DDR_ECC_STAT DDR_CTL(36)
+#define CDNS_DDR_ECC_STAT_ENABLED BIT(16)
+#define CDNS_DDR_ECC_STAT_IS_ENABLED(reg) FIELD_GET(CDNS_DDR_ECC_STAT_ENABLED, reg)
+#define CDNS_DDR_ECC_STAT_FWC BIT(24)
+
+#define CDNS_DDR_ECC_XOR DDR_CTL(37)
+#define CDNS_DDR_ECC_XOR_CHECK_BITS GENMASK_U32(13, 0)
+
+#define CDNS_DDR_BUS_CTRL DDR_CTL(54)
+#define CDNS_DDR_BUS_CTRL_REDUC BIT(1)
+
+/* DDR Controller Error Registers */
+
+#define CDNS_DDR_ECC_U_ERR_ADDR DDR_CTL(38)
+#define CDNS_DDR_ECC_U_ERR_STAT DDR_CTL(39)
+
+#define CDNS_DDR_ECC_C_ERR_ADDR DDR_CTL(41)
+#define CDNS_DDR_ECC_C_ERR_STAT DDR_CTL(42)
+
+#define CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg) FIELD_GET(GENMASK(6, 0), reg)
+
+#define CDNS_DDR_PORT_CMD_ERR_ADDR DDR_CTL(61)
+#define CDNS_DDR_PORT_CMD_ERR_TYPE DDR_CTL(62)
+#define CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg) FIELD_GET(GENMASK_U32(10, 8), reg)
+
+/* DDR Controller Interrupt Registers */
+
+#define CDNS_DDR_ECC_INT_STAT DDR_CTL(56)
+#define CDNS_DDR_ECC_INT_STAT_CE BIT(3)
+#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE BIT(4)
+#define CDNS_DDR_ECC_INT_STAT_UE BIT(5)
+#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE BIT(6)
+#define CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN BIT(7)
+
+#define CDNS_DDR_ECC_INT_ACK DDR_CTL(57)
+#define CDNS_DDR_ECC_INT_ACK_MASK GENMASK_U32(21, 0)
+
+#define CDNS_DDR_ECC_INT_CTRL DDR_CTL(58)
+#define CDNS_DDR_ECC_INT_CTRL_MASK GENMASK_U32(21, 0)
+#define CDNS_DDR_ECC_INT_CTRL_MASK_ALL BIT(22)
+#define CDNS_DDR_ECC_INT_CTRL_UNMASK(i) ((~(i)) & CDNS_DDR_ECC_INT_CTRL_MASK)
+
+struct cdns_mc_priv {
+	int irq;
+	void __iomem *io_base;
+	struct dentry *debugfs;
+	spinlock_t lock;
+	u16 xor_check_bits;
+};
+
+static irqreturn_t cdns_mc_err_handler(int irq, void *dev_id)
+{
+	struct mem_ctl_info *mci = dev_id;
+	struct cdns_mc_priv *priv = mci->pvt_info;
+	u32 addr, status, err_addr, syndrome, reg;
+	char other_details_str[32];
+	u8 type;
+
+	/* Read the interrupt status register */
+	status = readl(priv->io_base + CDNS_DDR_ECC_INT_STAT);
+	if (!status)
+		return IRQ_NONE;
+
+	/*
+	 * We can't know how many CE / UE occurred since last ACK in case of
+	 * multiple errors. Just report it.
+	 */
+
+	if ((status & CDNS_DDR_ECC_INT_STAT_UE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE)) {
+		reg = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_STAT);
+		syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);
+
+		err_addr = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_ADDR);
+
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1, err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, "");
+	}
+
+	if ((status & CDNS_DDR_ECC_INT_STAT_CE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE)) {
+		reg = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_STAT);
+		syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);
+
+		err_addr = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_ADDR);
+
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1, err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, "");
+	}
+
+	if (status & CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN) {
+		addr = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_ADDR);
+		reg = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_TYPE);
+		type = CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg);
+
+		snprintf(other_details_str, sizeof(other_details_str), "type 0x%02x", type);
+
+		edac_mc_handle_error(HW_EVENT_ERR_INFO, mci, 1, addr >> PAGE_SHIFT,
+				     addr & ~PAGE_MASK, 0, 0, 0, -1, mci->ctl_name,
+				     other_details_str);
+	}
+
+	/* clear the error, clears the interrupt */
+	writel(status & CDNS_DDR_ECC_INT_ACK_MASK, priv->io_base + CDNS_DDR_ECC_INT_ACK);
+
+	return IRQ_HANDLED;
+}
+
+static int cdns_get_mem_sz(resource_size_t *mem_sz)
+{
+	struct device_node *np;
+	struct resource res;
+	int ret;
+
+	np = of_find_node_by_name(NULL, "memory");
+	if (!np)
+		return -ENODEV;
+
+	ret = of_address_to_resource(np, 0, &res);
+
+	of_node_put(np);
+
+	if (ret)
+		return ret;
+
+	*mem_sz = resource_size(&res);
+
+	return 0;
+}
+
+#ifdef CONFIG_EDAC_DEBUG
+
+static void cdns_rmw(struct cdns_mc_priv *priv, u32 reg, u32 mask, u32 val)
+{
+	u32 regval;
+
+	regval = readl(priv->io_base + reg);
+	regval &= ~mask;
+	regval |= (val << __bf_shf(mask)) & mask;
+	writel(regval, priv->io_base + reg);
+}
+
+static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count,
+				    loff_t *ppos)
+{
+	struct device *dev = file->private_data;
+	struct mem_ctl_info *mci = to_mci(dev);
+	struct cdns_mc_priv *priv = mci->pvt_info;
+
+	spin_lock(&priv->lock);
+
+	cdns_rmw(priv, CDNS_DDR_ECC_XOR, CDNS_DDR_ECC_XOR_CHECK_BITS, priv->xor_check_bits);
+	cdns_rmw(priv, CDNS_DDR_ECC_STAT, CDNS_DDR_ECC_STAT_FWC, 1);
+
+	spin_unlock(&priv->lock);
+
+	return count;
+}
+
+static const struct file_operations cdns_ecc_error_fops = {
+	.open = simple_open,
+	.write = cdns_force_ecc_error,
+	.llseek = generic_file_llseek,
+};
+
+static void cdns_setup_debugfs(struct mem_ctl_info *mci)
+{
+	struct cdns_mc_priv *priv = mci->pvt_info;
+
+	priv->debugfs = edac_debugfs_create_dir(DRV_NAME);
+	if (!priv->debugfs) {
+		dev_dbg(mci->pdev, "failed to create debugfs dir\n");
+		return;
+	}
+
+	edac_debugfs_create_x16("bits", 0644, priv->debugfs, &priv->xor_check_bits);
+	edac_debugfs_create_file("inject", 0200, priv->debugfs, &mci->dev, &cdns_ecc_error_fops);
+}
+
+#endif
+
+static int cdns_setup_irq(struct platform_device *pdev, struct mem_ctl_info *mci)
+{
+	struct cdns_mc_priv *priv = mci->pvt_info;
+	int ret;
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return dev_err_probe(&pdev->dev, priv->irq, "unable to get irq\n");
+
+	ret = devm_request_irq(&pdev->dev, priv->irq, cdns_mc_err_handler, 0, dev_name(&pdev->dev),
+			       mci);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "unable to request irq %d\n", priv->irq);
+
+	return 0;
+}
+
+static int cdns_setup_mc(struct platform_device *pdev, void __iomem *io_base)
+{
+	struct edac_mc_layer layers[2];
+	struct cdns_mc_priv *priv;
+	struct mem_ctl_info *mci;
+	resource_size_t mem_sz;
+	struct dimm_info *dimm;
+	int ret;
+	u32 reg;
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = 1;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = 1;
+	layers[1].is_virt_csrow = false;
+
+	ret = cdns_get_mem_sz(&mem_sz);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "unable to get memory size\n");
+
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(struct cdns_mc_priv));
+	if (!mci)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "unable to allocate edac mc\n");
+
+	mci->pdev = &pdev->dev;
+	priv = mci->pvt_info;
+
+	priv->io_base = io_base;
+	spin_lock_init(&priv->lock);
+	priv->xor_check_bits = 0;
+	platform_set_drvdata(pdev, mci);
+
+	reg = readl(priv->io_base + CDNS_DDR_ECC_STAT);
+	if (!CDNS_DDR_ECC_STAT_IS_ENABLED(reg))
+		mci->edac_cap = EDAC_FLAG_NONE;
+	else
+		mci->edac_cap = EDAC_FLAG_SECDED;
+
+	mci->mtype_cap = MEM_FLAG_DDR2 | MEM_FLAG_DDR3;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->mod_name = pdev->dev.driver->name;
+	mci->ctl_name = "cdns-ddr-ctrl";
+	mci->dev_name = dev_name(&pdev->dev);
+	mci->scrub_mode = SCRUB_SW_SRC;
+
+	dimm = *mci->dimms;
+	dimm->nr_pages = PFN_UP(mem_sz);
+	dimm->grain = 4;
+	dimm->edac_mode = EDAC_SECDED;
+
+	/* Check if half datapath feature of the controller is active. */
+	reg = readl(priv->io_base + CDNS_DDR_BUS_CTRL);
+	if (reg & CDNS_DDR_BUS_CTRL_REDUC)
+		dimm->dtype = DEV_X8;
+	else
+		dimm->dtype = DEV_X16;
+
+	strscpy(dimm->label, "Channel#0_DIMM#0", sizeof(dimm->label));
+
+	reg = readl(priv->io_base + CDNS_DDR_DDR_STAT);
+	if (CDNS_DDR_DDR_STAT_GET_DRAM_CLASS(reg) == CDNS_DDR_DDR_STAT_DRAM_DDR2)
+		dimm->mtype = MEM_DDR2;
+	else
+		dimm->mtype = MEM_DDR3;
+
+	ret = edac_mc_add_mc(mci);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "failed to add mc\n");
+		goto free_edac_mc;
+	}
+
+	ret = cdns_setup_irq(pdev, mci);
+	if (ret)
+		goto del_edac_mc;
+
+#ifdef CONFIG_EDAC_DEBUG
+	cdns_setup_debugfs(mci);
+#endif
+
+	edac_mc_chipset_printk(mci, KERN_DEBUG, DRV_NAME, "ECC %s\n",
+			       (mci->edac_cap == EDAC_FLAG_NONE) ? "disabled" : "enabled");
+
+	return 0;
+
+del_edac_mc:
+	edac_mc_del_mc(&pdev->dev);
+free_edac_mc:
+	edac_mc_free(mci);
+	return ret;
+}
+
+static int cdns_mc_probe(struct platform_device *pdev)
+{
+	void __iomem *io_base;
+	struct clk *clk;
+	int ret;
+
+	io_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(io_base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(io_base), "unable to map regs\n");
+
+	clk = devm_clk_get_enabled(&pdev->dev, "ddrc");
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "unable to get ddrc\n");
+
+	clk = devm_clk_get_enabled(&pdev->dev, "pclk");
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "unable to get pclk\n");
+
+	writel(CDNS_DDR_ECC_INT_CTRL_MASK_ALL, io_base + CDNS_DDR_ECC_INT_CTRL);
+
+	ret = cdns_setup_mc(pdev, io_base);
+	if (ret)
+		return ret;
+
+	/*
+	 * Unmask ECC recoverable and unrecoverable interrupts, and port
+	 * command errors.
+	 */
+	writel(CDNS_DDR_ECC_INT_CTRL_UNMASK(
+		       CDNS_DDR_ECC_INT_STAT_CE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE |
+		       CDNS_DDR_ECC_INT_STAT_UE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE |
+		       CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN),
+	       io_base + CDNS_DDR_ECC_INT_CTRL);
+
+	return 0;
+}
+
+static void cdns_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+	struct cdns_mc_priv *priv = mci->pvt_info;
+
+	writel(CDNS_DDR_ECC_INT_CTRL_MASK_ALL, priv->io_base + CDNS_DDR_ECC_INT_CTRL);
+
+	/* Wait for any pending interrupt before releasing resources. */
+	devm_free_irq(&pdev->dev, priv->irq, mci);
+
+#ifdef CONFIG_EDAC_DEBUG
+	edac_debugfs_remove_recursive(priv->debugfs);
+#endif
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+}
+
+static const struct of_device_id cdns_ddr_ctrl_of_match[] = {
+	{ .compatible = "cdns,ddr-edac" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cdns_ddr_ctrl_of_match);
+
+static struct platform_driver cdns_mc_edac_driver = {
+	.probe = cdns_mc_probe,
+	.remove = cdns_mc_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = cdns_ddr_ctrl_of_match,
+	},
+};
+
+module_platform_driver(cdns_mc_edac_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Renesas Electronics Europe Ltd.");
+MODULE_AUTHOR("Paul Louvel <paul.louvel@bootlin.com>");
+MODULE_DESCRIPTION("EDAC driver for Cadence DDR controller");

-- 
2.55.0

[PATCH v5 3/3] ARM: dts: renesas: r9a06g032: add EDAC node

[Paul Louvel]

Add EDAC node to the SoC base device tree file.

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Paul Louvel (Schneider Electric) <paul.louvel@bootlin.com>
---
 arch/arm/boot/dts/renesas/r9a06g032.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/renesas/r9a06g032.dtsi b/arch/arm/boot/dts/renesas/r9a06g032.dtsi
index 19c9bce0a26d..8152464d4e6b 100644
--- a/arch/arm/boot/dts/renesas/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/renesas/r9a06g032.dtsi
@@ -164,6 +164,15 @@ dmamux: dma-router@a0 {
 			};
 		};
 
+		edac: memory-controller@4000d000 {
+			compatible = "renesas,r9a06g032-ddr-edac", "cdns,ddr-edac";
+			reg = <0x4000d000 0x1000>;
+			interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&sysctrl R9A06G032_CLK_DDRC>, <&sysctrl R9A06G032_HCLK_DDRC>;
+			clock-names = "ddrc", "pclk";
+			status = "disabled";
+		};
+
 		udc: usb@4001e000 {
 			compatible = "renesas,r9a06g032-usbf", "renesas,rzn1-usbf";
 			reg = <0x4001e000 0x2000>;

-- 
2.55.0

Re: [PATCH v5 1/3] dt-bindings: edac: cdns,ddr-edac: add Cadence DDR EDAC

[Borislav Petkov]

On Mon, Oct 05, 2026 at 02:49:22PM +0200, Paul Louvel wrote:
> Add the Cadence EDAC dt binding.
> The memory controller optionally supports ECC SECDED with DDR2/DDR3
> memory.
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Paul Louvel (Schneider Electric) <paul.louvel@bootlin.com>
> ---
>  .../devicetree/bindings/edac/cdns,ddr-edac.yaml    | 61 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 +++
>  2 files changed, 67 insertions(+)

Needs review by DT maintainers if it is going to go through the EDAC tree, see
output of:

./scripts/get_maintainer.pl -f Documentation/devicetree/bindings/edac/

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 1/3] dt-bindings: edac: cdns,ddr-edac: add Cadence DDR EDAC

[Rob Herring (Arm)]

On Mon, 05 Oct 2026 14:49:22 +0200, Paul Louvel wrote:
> Add the Cadence EDAC dt binding.
> The memory controller optionally supports ECC SECDED with DDR2/DDR3
> memory.
> 
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Paul Louvel (Schneider Electric) <paul.louvel@bootlin.com>
> ---
>  .../devicetree/bindings/edac/cdns,ddr-edac.yaml    | 61 ++++++++++++++++++++++
>  MAINTAINERS                                        |  6 +++
>  2 files changed, 67 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH v5 2/3] EDAC/cadence: Add Cadence DDR EDAC driver

[Borislav Petkov]

On Mon, Oct 05, 2026 at 02:49:23PM +0200, Paul Louvel wrote:
> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
> index a37534300ab9..0d66a072b15c 100644
> --- a/drivers/edac/Makefile
> +++ b/drivers/edac/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
>  obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
>  obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
>  obj-$(CONFIG_EDAC_QCOM)			+= qcom_edac.o
> +obj-$(CONFIG_EDAC_CADENCE)		+= cadence_edac.o

This goes at the end of that file.

>  obj-$(CONFIG_EDAC_ASPEED)		+= aspeed_edac.o
>  obj-$(CONFIG_EDAC_BLUEFIELD)		+= bluefield_edac.o
>  obj-$(CONFIG_EDAC_DMC520)		+= dmc520_edac.o
> diff --git a/drivers/edac/cadence_edac.c b/drivers/edac/cadence_edac.c
> new file mode 100644
> index 000000000000..26b9a7facd57
> --- /dev/null
> +++ b/drivers/edac/cadence_edac.c
> @@ -0,0 +1,399 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2015 Renesas Electronics Europe Ltd.
> + * Copyright 2026 Bootlin
> + *
> + * Based on highbank EDAC driver:
> + *
> + * Copyright 2011-2012 Calxeda, Inc.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/edac.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/of_address.h>
> +#include <linux/string.h>
> +#include <linux/spinlock.h>
> +#include <linux/platform_device.h>

How many of those includes are *actually* needed?

> +#include "edac_mc.h"
> +#include "edac_module.h"
> +
> +#define DRV_NAME "cdns_edac"

"cadence_edac" is a perfectly fine name.

> +#define REG_BYTE_SZ 4
> +#define DDR_CTL(n) ((n) * REG_BYTE_SZ)
> +
> +#define CDNS_DDR_DDR_STAT DDR_CTL(0)

Align all defines vertically like this:

#define REG_BYTE_SZ		4
#define DDR_CTL(n)		((n) * REG_BYTE_SZ)

#define CDNS_DDR_DDR_STAT	DDR_CTL(0)
...

> +#define CDNS_DDR_DDR_STAT_DRAM_CLASS GENMASK_U32(11, 8)
> +#define CDNS_DDR_DDR_STAT_DRAM_DDR2 BIT(2)
> +#define CDNS_DDR_DDR_STAT_GET_DRAM_CLASS(reg) FIELD_GET(CDNS_DDR_DDR_STAT_DRAM_CLASS, reg)

Also, I would shorten those long mouthfuls so that the code remains relatively
readable. The "DDR_DDR" thing above is the first I'd whack. And so on.

> +#define CDNS_DDR_ECC_STAT DDR_CTL(36)
> +#define CDNS_DDR_ECC_STAT_ENABLED BIT(16)
> +#define CDNS_DDR_ECC_STAT_IS_ENABLED(reg) FIELD_GET(CDNS_DDR_ECC_STAT_ENABLED, reg)
> +#define CDNS_DDR_ECC_STAT_FWC BIT(24)
> +
> +#define CDNS_DDR_ECC_XOR DDR_CTL(37)
> +#define CDNS_DDR_ECC_XOR_CHECK_BITS GENMASK_U32(13, 0)
> +
> +#define CDNS_DDR_BUS_CTRL DDR_CTL(54)
> +#define CDNS_DDR_BUS_CTRL_REDUC BIT(1)
> +
> +/* DDR Controller Error Registers */
> +
> +#define CDNS_DDR_ECC_U_ERR_ADDR DDR_CTL(38)
> +#define CDNS_DDR_ECC_U_ERR_STAT DDR_CTL(39)
> +
> +#define CDNS_DDR_ECC_C_ERR_ADDR DDR_CTL(41)
> +#define CDNS_DDR_ECC_C_ERR_STAT DDR_CTL(42)
> +
> +#define CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg) FIELD_GET(GENMASK(6, 0), reg)
> +
> +#define CDNS_DDR_PORT_CMD_ERR_ADDR DDR_CTL(61)
> +#define CDNS_DDR_PORT_CMD_ERR_TYPE DDR_CTL(62)
> +#define CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg) FIELD_GET(GENMASK_U32(10, 8), reg)
> +
> +/* DDR Controller Interrupt Registers */
> +
> +#define CDNS_DDR_ECC_INT_STAT DDR_CTL(56)
> +#define CDNS_DDR_ECC_INT_STAT_CE BIT(3)
> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE BIT(4)
> +#define CDNS_DDR_ECC_INT_STAT_UE BIT(5)
> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE BIT(6)
> +#define CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN BIT(7)
> +
> +#define CDNS_DDR_ECC_INT_ACK DDR_CTL(57)
> +#define CDNS_DDR_ECC_INT_ACK_MASK GENMASK_U32(21, 0)
> +
> +#define CDNS_DDR_ECC_INT_CTRL DDR_CTL(58)
> +#define CDNS_DDR_ECC_INT_CTRL_MASK GENMASK_U32(21, 0)
> +#define CDNS_DDR_ECC_INT_CTRL_MASK_ALL BIT(22)
> +#define CDNS_DDR_ECC_INT_CTRL_UNMASK(i) ((~(i)) & CDNS_DDR_ECC_INT_CTRL_MASK)
> +
> +struct cdns_mc_priv {

For all privately used struct names and static functions, drop the "cdns_"
namespace prefix - it is not necessary.

> +	int irq;
> +	void __iomem *io_base;
> +	struct dentry *debugfs;
> +	spinlock_t lock;
> +	u16 xor_check_bits;
> +};
> +
> +static irqreturn_t cdns_mc_err_handler(int irq, void *dev_id)
> +{
> +	struct mem_ctl_info *mci = dev_id;
> +	struct cdns_mc_priv *priv = mci->pvt_info;
> +	u32 addr, status, err_addr, syndrome, reg;
> +	char other_details_str[32];
> +	u8 type;
> +
> +	/* Read the interrupt status register */

The fact that you have to put an obvious comment above the read of a register
basically says that your register naming is not optimal enough. If you name it
properly, you don't need a comment.

> +	status = readl(priv->io_base + CDNS_DDR_ECC_INT_STAT);
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	/*
> +	 * We can't know how many CE / UE occurred since last ACK in case of

Please use passive voice: no "we" or "I", etc, and describe things in an
imperative mood.

> +	 * multiple errors. Just report it.
> +	 */
> +
> +	if ((status & CDNS_DDR_ECC_INT_STAT_UE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE)) {

This is what I mean with too long lines. That one and others like it needs
shortening.

Also this test can be merged into a single one by ORing the flags.

> +		reg = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_STAT);
> +		syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);
> +
> +		err_addr = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_ADDR);
> +
> +		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1, err_addr >> PAGE_SHIFT,
> +				     err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, "");
> +	}

ditto for that one below:

> +	if ((status & CDNS_DDR_ECC_INT_STAT_CE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE)) {
> +		reg = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_STAT);
> +		syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);
> +
> +		err_addr = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_ADDR);
> +
> +		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1, err_addr >> PAGE_SHIFT,
> +				     err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, "");
> +	}
> +
> +	if (status & CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN) {

What kind of an error is that one so that you have to call
edac_mc_handle_error() for it separately?

> +		addr = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_ADDR);
> +		reg = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_TYPE);
> +		type = CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg);
> +
> +		snprintf(other_details_str, sizeof(other_details_str), "type 0x%02x", type);
> +
> +		edac_mc_handle_error(HW_EVENT_ERR_INFO, mci, 1, addr >> PAGE_SHIFT,
> +				     addr & ~PAGE_MASK, 0, 0, 0, -1, mci->ctl_name,
> +				     other_details_str);
> +	}
> +
> +	/* clear the error, clears the interrupt */

No need for obvious comments. Audit your whole driver pls.

> +	writel(status & CDNS_DDR_ECC_INT_ACK_MASK, priv->io_base + CDNS_DDR_ECC_INT_ACK);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int cdns_get_mem_sz(resource_size_t *mem_sz)

Do not use an I/O function param but return the correct size or an error and
have call site handle that.

Looking how that function is called only once, simply merge it into the call
site.

> +{
> +	struct device_node *np;
> +	struct resource res;
> +	int ret;
> +
> +	np = of_find_node_by_name(NULL, "memory");
> +	if (!np)
> +		return -ENODEV;
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +
> +	of_node_put(np);
> +
> +	if (ret)
> +		return ret;
> +
> +	*mem_sz = resource_size(&res);
> +
> +	return 0;
> +}

> +
> +#ifdef CONFIG_EDAC_DEBUG
> +
> +static void cdns_rmw(struct cdns_mc_priv *priv, u32 reg, u32 mask, u32 val)
> +{
> +	u32 regval;
> +
> +	regval = readl(priv->io_base + reg);
> +	regval &= ~mask;
> +	regval |= (val << __bf_shf(mask)) & mask;
> +	writel(regval, priv->io_base + reg);
> +}
> +
> +static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count,
> +				    loff_t *ppos)
> +{
> +	struct device *dev = file->private_data;
> +	struct mem_ctl_info *mci = to_mci(dev);
> +	struct cdns_mc_priv *priv = mci->pvt_info;
> +
> +	spin_lock(&priv->lock);
> +
> +	cdns_rmw(priv, CDNS_DDR_ECC_XOR, CDNS_DDR_ECC_XOR_CHECK_BITS, priv->xor_check_bits);
> +	cdns_rmw(priv, CDNS_DDR_ECC_STAT, CDNS_DDR_ECC_STAT_FWC, 1);
> +
> +	spin_unlock(&priv->lock);
> +
> +	return count;
> +}
> +
> +static const struct file_operations cdns_ecc_error_fops = {
> +	.open = simple_open,
> +	.write = cdns_force_ecc_error,
> +	.llseek = generic_file_llseek,
> +};

#else
static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count, loff_t *ppos)
{
	return 0
}
#endif

and get rid of the ifdeffery below.

> +static void cdns_setup_debugfs(struct mem_ctl_info *mci)
> +{
> +	struct cdns_mc_priv *priv = mci->pvt_info;
> +
> +	priv->debugfs = edac_debugfs_create_dir(DRV_NAME);
> +	if (!priv->debugfs) {
> +		dev_dbg(mci->pdev, "failed to create debugfs dir\n");
> +		return;
> +	}
> +
> +	edac_debugfs_create_x16("bits", 0644, priv->debugfs, &priv->xor_check_bits);
> +	edac_debugfs_create_file("inject", 0200, priv->debugfs, &mci->dev, &cdns_ecc_error_fops);
> +}
> +
> +#endif

...

> +
> +	/*
> +	 * Unmask ECC recoverable and unrecoverable interrupts, and port
> +	 * command errors.
> +	 */
> +	writel(CDNS_DDR_ECC_INT_CTRL_UNMASK(
> +		       CDNS_DDR_ECC_INT_STAT_CE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE |
> +		       CDNS_DDR_ECC_INT_STAT_UE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE |
> +		       CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN),

Yah, unreadable mess that. Shorten pls.

> +	       io_base + CDNS_DDR_ECC_INT_CTRL);
> +
> +	return 0;
> +}

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v5 2/3] EDAC/cadence: Add Cadence DDR EDAC driver

[Paul Louvel]

On Wed Oct 7, 2026 at 4:11 AM CEST, Borislav Petkov wrote:
> On Mon, Oct 05, 2026 at 02:49:23PM +0200, Paul Louvel wrote:
>> diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
>> index a37534300ab9..0d66a072b15c 100644
>> --- a/drivers/edac/Makefile
>> +++ b/drivers/edac/Makefile
>> @@ -82,6 +82,7 @@ obj-$(CONFIG_EDAC_SYNOPSYS)		+= synopsys_edac.o
>>  obj-$(CONFIG_EDAC_XGENE)		+= xgene_edac.o
>>  obj-$(CONFIG_EDAC_TI)			+= ti_edac.o
>>  obj-$(CONFIG_EDAC_QCOM)			+= qcom_edac.o
>> +obj-$(CONFIG_EDAC_CADENCE)		+= cadence_edac.o
>
> This goes at the end of that file.
>
>>  obj-$(CONFIG_EDAC_ASPEED)		+= aspeed_edac.o
>>  obj-$(CONFIG_EDAC_BLUEFIELD)		+= bluefield_edac.o
>>  obj-$(CONFIG_EDAC_DMC520)		+= dmc520_edac.o
>> diff --git a/drivers/edac/cadence_edac.c b/drivers/edac/cadence_edac.c
>> new file mode 100644
>> index 000000000000..26b9a7facd57
>> --- /dev/null
>> +++ b/drivers/edac/cadence_edac.c
>> @@ -0,0 +1,399 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright 2015 Renesas Electronics Europe Ltd.
>> + * Copyright 2026 Bootlin
>> + *
>> + * Based on highbank EDAC driver:
>> + *
>> + * Copyright 2011-2012 Calxeda, Inc.
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/clk.h>
>> +#include <linux/edac.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mm.h>
             ^^^^^^^^^^^^
I think this one is not needed.

>> +#include <linux/of_address.h>
>> +#include <linux/string.h>
             ^^^^^^^^^^^^^^^^
This one too since I will remove the port channel protection feature, and the
snprintf will go away.

>> +#include <linux/spinlock.h>
>> +#include <linux/platform_device.h>
>
> How many of those includes are *actually* needed?
>
>> +#include "edac_mc.h"
>> +#include "edac_module.h"
>> +
>> +#define DRV_NAME "cdns_edac"
>
> "cadence_edac" is a perfectly fine name.
>
>> +#define REG_BYTE_SZ 4
>> +#define DDR_CTL(n) ((n) * REG_BYTE_SZ)
>> +
>> +#define CDNS_DDR_DDR_STAT DDR_CTL(0)
>
> Align all defines vertically like this:
>
> #define REG_BYTE_SZ		4
> #define DDR_CTL(n)		((n) * REG_BYTE_SZ)
>
> #define CDNS_DDR_DDR_STAT	DDR_CTL(0)
> ...
>
>> +#define CDNS_DDR_DDR_STAT_DRAM_CLASS GENMASK_U32(11, 8)
>> +#define CDNS_DDR_DDR_STAT_DRAM_DDR2 BIT(2)
>> +#define CDNS_DDR_DDR_STAT_GET_DRAM_CLASS(reg) FIELD_GET(CDNS_DDR_DDR_STAT_DRAM_CLASS, reg)
>
> Also, I would shorten those long mouthfuls so that the code remains relatively
> readable. The "DDR_DDR" thing above is the first I'd whack. And so on.

I will drop the first "DDR" prefix.

>
>> +#define CDNS_DDR_ECC_STAT DDR_CTL(36)
>> +#define CDNS_DDR_ECC_STAT_ENABLED BIT(16)
>> +#define CDNS_DDR_ECC_STAT_IS_ENABLED(reg) FIELD_GET(CDNS_DDR_ECC_STAT_ENABLED, reg)
>> +#define CDNS_DDR_ECC_STAT_FWC BIT(24)
>> +
>> +#define CDNS_DDR_ECC_XOR DDR_CTL(37)
>> +#define CDNS_DDR_ECC_XOR_CHECK_BITS GENMASK_U32(13, 0)
>> +
>> +#define CDNS_DDR_BUS_CTRL DDR_CTL(54)
>> +#define CDNS_DDR_BUS_CTRL_REDUC BIT(1)
>> +
>> +/* DDR Controller Error Registers */
>> +
>> +#define CDNS_DDR_ECC_U_ERR_ADDR DDR_CTL(38)
>> +#define CDNS_DDR_ECC_U_ERR_STAT DDR_CTL(39)
>> +
>> +#define CDNS_DDR_ECC_C_ERR_ADDR DDR_CTL(41)
>> +#define CDNS_DDR_ECC_C_ERR_STAT DDR_CTL(42)
>> +
>> +#define CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg) FIELD_GET(GENMASK(6, 0), reg)
>> +
>> +#define CDNS_DDR_PORT_CMD_ERR_ADDR DDR_CTL(61)
>> +#define CDNS_DDR_PORT_CMD_ERR_TYPE DDR_CTL(62)
>> +#define CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg) FIELD_GET(GENMASK_U32(10, 8), reg)
>> +
>> +/* DDR Controller Interrupt Registers */
>> +
>> +#define CDNS_DDR_ECC_INT_STAT DDR_CTL(56)
>> +#define CDNS_DDR_ECC_INT_STAT_CE BIT(3)
>> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE BIT(4)
>> +#define CDNS_DDR_ECC_INT_STAT_UE BIT(5)
>> +#define CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE BIT(6)
>> +#define CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN BIT(7)
>> +
>> +#define CDNS_DDR_ECC_INT_ACK DDR_CTL(57)
>> +#define CDNS_DDR_ECC_INT_ACK_MASK GENMASK_U32(21, 0)
>> +
>> +#define CDNS_DDR_ECC_INT_CTRL DDR_CTL(58)
>> +#define CDNS_DDR_ECC_INT_CTRL_MASK GENMASK_U32(21, 0)
>> +#define CDNS_DDR_ECC_INT_CTRL_MASK_ALL BIT(22)
>> +#define CDNS_DDR_ECC_INT_CTRL_UNMASK(i) ((~(i)) & CDNS_DDR_ECC_INT_CTRL_MASK)
>> +
>> +struct cdns_mc_priv {
>
> For all privately used struct names and static functions, drop the "cdns_"
> namespace prefix - it is not necessary.
>
>> +	int irq;
>> +	void __iomem *io_base;
>> +	struct dentry *debugfs;
>> +	spinlock_t lock;
>> +	u16 xor_check_bits;
>> +};
>> +
>> +static irqreturn_t cdns_mc_err_handler(int irq, void *dev_id)
>> +{
>> +	struct mem_ctl_info *mci = dev_id;
>> +	struct cdns_mc_priv *priv = mci->pvt_info;
>> +	u32 addr, status, err_addr, syndrome, reg;
>> +	char other_details_str[32];
>> +	u8 type;
>> +
>> +	/* Read the interrupt status register */
>
> The fact that you have to put an obvious comment above the read of a register
> basically says that your register naming is not optimal enough. If you name it
> properly, you don't need a comment.
>

I do think the register naming is fine and follows common conventions
(INT=interrupt, STAT=status). Also, the datasheet does not give any meaningful
name to these registers (generic like DDR_CTL_##) and usually a single register
is used for multiple purposes..
I will remove the comment.

>> +	status = readl(priv->io_base + CDNS_DDR_ECC_INT_STAT);
>> +	if (!status)
>> +		return IRQ_NONE;
>> +
>> +	/*
>> +	 * We can't know how many CE / UE occurred since last ACK in case of
>
> Please use passive voice: no "we" or "I", etc, and describe things in an
> imperative mood.
>
>> +	 * multiple errors. Just report it.
>> +	 */
>> +
>> +	if ((status & CDNS_DDR_ECC_INT_STAT_UE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE)) {
>
> This is what I mean with too long lines. That one and others like it needs
> shortening.
>
> Also this test can be merged into a single one by ORing the flags.
>
>> +		reg = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_STAT);
>> +		syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);
>> +
>> +		err_addr = readl(priv->io_base + CDNS_DDR_ECC_U_ERR_ADDR);
>> +
>> +		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1, err_addr >> PAGE_SHIFT,
>> +				     err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, "");
>> +	}
>
> ditto for that one below:
>
>> +	if ((status & CDNS_DDR_ECC_INT_STAT_CE) || (status & CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE)) {
>> +		reg = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_STAT);
>> +		syndrome = CDNS_DDR_ECC_ERR_STAT_GET_SYNDROME(reg);
>> +
>> +		err_addr = readl(priv->io_base + CDNS_DDR_ECC_C_ERR_ADDR);
>> +
>> +		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, 1, err_addr >> PAGE_SHIFT,
>> +				     err_addr & ~PAGE_MASK, syndrome, 0, 0, -1, mci->ctl_name, "");
>> +	}
>> +
>> +	if (status & CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN) {
>
> What kind of an error is that one so that you have to call
> edac_mc_handle_error() for it separately?

This is a leftover from the downstream driver I picked up.
TBH, I do not think this error belongs in an EDAC driver so I will remove it.
This is a protection feature of the memory controller that is not related to
ECC.

>
>> +		addr = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_ADDR);
>> +		reg = readl(priv->io_base + CDNS_DDR_PORT_CMD_ERR_TYPE);
>> +		type = CDNS_DDR_PORT_CMD_ERR_TYPE_GET(reg);
>> +
>> +		snprintf(other_details_str, sizeof(other_details_str), "type 0x%02x", type);
>> +
>> +		edac_mc_handle_error(HW_EVENT_ERR_INFO, mci, 1, addr >> PAGE_SHIFT,
>> +				     addr & ~PAGE_MASK, 0, 0, 0, -1, mci->ctl_name,
>> +				     other_details_str);
>> +	}
>> +
>> +	/* clear the error, clears the interrupt */
>
> No need for obvious comments. Audit your whole driver pls.
>
>> +	writel(status & CDNS_DDR_ECC_INT_ACK_MASK, priv->io_base + CDNS_DDR_ECC_INT_ACK);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int cdns_get_mem_sz(resource_size_t *mem_sz)
>
> Do not use an I/O function param but return the correct size or an error and
> have call site handle that.
>
> Looking how that function is called only once, simply merge it into the call
> site.

Liked it that way so I can avoid any potential overflow situation.
But yeah, will merge it.

>
>> +{
>> +	struct device_node *np;
>> +	struct resource res;
>> +	int ret;
>> +
>> +	np = of_find_node_by_name(NULL, "memory");
>> +	if (!np)
>> +		return -ENODEV;
>> +
>> +	ret = of_address_to_resource(np, 0, &res);
>> +
>> +	of_node_put(np);
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	*mem_sz = resource_size(&res);
>> +
>> +	return 0;
>> +}
>
>> +
>> +#ifdef CONFIG_EDAC_DEBUG
>> +
>> +static void cdns_rmw(struct cdns_mc_priv *priv, u32 reg, u32 mask, u32 val)
>> +{
>> +	u32 regval;
>> +
>> +	regval = readl(priv->io_base + reg);
>> +	regval &= ~mask;
>> +	regval |= (val << __bf_shf(mask)) & mask;
>> +	writel(regval, priv->io_base + reg);
>> +}
>> +
>> +static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count,
>> +				    loff_t *ppos)
>> +{
>> +	struct device *dev = file->private_data;
>> +	struct mem_ctl_info *mci = to_mci(dev);
>> +	struct cdns_mc_priv *priv = mci->pvt_info;
>> +
>> +	spin_lock(&priv->lock);
>> +
>> +	cdns_rmw(priv, CDNS_DDR_ECC_XOR, CDNS_DDR_ECC_XOR_CHECK_BITS, priv->xor_check_bits);
>> +	cdns_rmw(priv, CDNS_DDR_ECC_STAT, CDNS_DDR_ECC_STAT_FWC, 1);
>> +
>> +	spin_unlock(&priv->lock);
>> +
>> +	return count;
>> +}
>> +
>> +static const struct file_operations cdns_ecc_error_fops = {
>> +	.open = simple_open,
>> +	.write = cdns_force_ecc_error,
>> +	.llseek = generic_file_llseek,
>> +};
>
> #else
> static ssize_t cdns_force_ecc_error(struct file *file, const char __user *data, size_t count, loff_t *ppos)
> {
> 	return 0
> }
> #endif
>
> and get rid of the ifdeffery below.
>
>> +static void cdns_setup_debugfs(struct mem_ctl_info *mci)
>> +{
>> +	struct cdns_mc_priv *priv = mci->pvt_info;
>> +
>> +	priv->debugfs = edac_debugfs_create_dir(DRV_NAME);
>> +	if (!priv->debugfs) {
>> +		dev_dbg(mci->pdev, "failed to create debugfs dir\n");
>> +		return;
>> +	}
>> +
>> +	edac_debugfs_create_x16("bits", 0644, priv->debugfs, &priv->xor_check_bits);
>> +	edac_debugfs_create_file("inject", 0200, priv->debugfs, &mci->dev, &cdns_ecc_error_fops);
>> +}
>> +
>> +#endif
>
> ...
>
>> +
>> +	/*
>> +	 * Unmask ECC recoverable and unrecoverable interrupts, and port
>> +	 * command errors.
>> +	 */
>> +	writel(CDNS_DDR_ECC_INT_CTRL_UNMASK(
>> +		       CDNS_DDR_ECC_INT_STAT_CE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_CE |
>> +		       CDNS_DDR_ECC_INT_STAT_UE | CDNS_DDR_ECC_INT_STAT_MULTIPLE_UE |
>> +		       CDNS_DDR_ECC_INT_STAT_PORT_CMD_CHAN),
>
> Yah, unreadable mess that. Shorten pls.

I do not see any better way to express it.
I could define a macro for the OR'ing mess sure.

>
>> +	       io_base + CDNS_DDR_ECC_INT_CTRL);
>> +
>> +	return 0;
>> +}
>
> Thx.

Thanks.
Best regards,
-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 1/3] dt-bindings: edac: cdns,ddr-edac: add Cadence DDR EDAC

[Paul Louvel]

On Mon Oct 5, 2026 at 8:46 PM CEST, Borislav Petkov wrote:
> On Mon, Oct 05, 2026 at 02:49:22PM +0200, Paul Louvel wrote:
>> Add the Cadence EDAC dt binding.
>> The memory controller optionally supports ECC SECDED with DDR2/DDR3
>> memory.
>> 
>> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Signed-off-by: Paul Louvel (Schneider Electric) <paul.louvel@bootlin.com>
>> ---
>>  .../devicetree/bindings/edac/cdns,ddr-edac.yaml    | 61 ++++++++++++++++++++++
>>  MAINTAINERS                                        |  6 +++
>>  2 files changed, 67 insertions(+)
>
> Needs review by DT maintainers if it is going to go through the EDAC tree, see
> output of:
>
> ./scripts/get_maintainer.pl -f Documentation/devicetree/bindings/edac/

ACK.
For some reason b4 did not included most of the people apart from Rob /
Krzysztof / Wolfram.

-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com