[PATCH v4 0/9] Add Aspeed AST2700 SDRAM EDAC support

[PATCH v4 0/9] Add Aspeed AST2700 SDRAM EDAC support

[Ryan Chen]

Add ECC error reporting for the Aspeed AST2700 SoC to the existing
aspeed_edac driver.

The AST2700 memory controller keeps the same overall EDAC programming
model as the earlier Aspeed BMC SoCs, but uses a different register
layout, a split interrupt status/clear/mask scheme and DDR4/DDR5 memory.
Rather than fork the driver, the existing code is first tidied and
generalised, then the AST2700 is added as one more per-SoC variant.

Patches 1-4 are bug fixes and cleanups to the existing driver. Patch 5
documents the AST2700 binding, patches 6 and 7 rework the implementation
to accommodate it, and patch 8 adds the AST2700 support itself. Patch 9 is
Stefan's step-down patch, amended to leave the entry in place with me
as its reviewer.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
Changes in v4:
- Put the bug fixes first: the two fixes are now patches 1 and 2, the
  cleanups 3 and 4, and the bindings patch moves to 5 ahead of the
  AST2700 work.
- Explain in patch 1 where the grain value comes from, rather than the
  warning it silences.
- Drop the Cc: stable tags from the two fixes. Neither meets the bar in
  Documentation/process/stable-kernel-rules.rst; the Fixes: tags stay.
- Merge Stefan's step-down patch into patch 9, keeping the MAINTAINERS
  entry alive with a reviewer instead of removing it.
- Link to v3: https://lore.kernel.org/r/20260916-edac-v3-0-23e9ed2fbdf9@aspeedtech.com

Changes in v3:
- Establish the AST2700 interrupt mask in the driver rather than
  deriving it from the register's contents: ast2700_set_irq() now
  writes the mask outright, masking every source and unmasking only
  the ECC ones, and the mask is established before the interrupt is
  requested.
- New patch: add myself as a reviewer for the driver in MAINTAINERS.
- Tested on an AST2600 with CONFIG_PROVE_LOCKING=y; injected correctable
  errors are reported from the interrupt handler with no lockdep reports.
- Link to v2: https://lore.kernel.org/r/20260824-edac-v2-0-c8d8bb693586@aspeedtech.com

Changes in v2:
- New patch: free the ECC interrupt before the mem_ctl_info in
  aspeed_remove(). The interrupt is requested with devm_request_irq() and
  is only released after .remove() has returned, so edac_mc_free() could
  free the handler's context while the handler was still running on
  another CPU.
- Acquire the register lock with the irqsave variant in init_csrows() and
  in aspeed_probe(). The interrupt handler takes the same lock in hardirq
  context.
- Move the dev_dbg() of the interrupt status register out of the
  raw_spinlock critical section in the interrupt handler.
- Opt aspeed_edac.o into context analysis in drivers/edac/Makefile. The
  check is opt-in per object, so without it the __guarded_by() annotation
  added by the same patch was never actually verified.
- Return IRQ_NONE from ast2700_dramc_isr() when no ECC interrupt status
  bit is set; the handler clears only the ECC bits, so unconditionally
  claiming the interrupt could livelock the level-triggered line.
- Tested v2 on both AST2600 and AST2700 by injecting a correctable
  error with the memory controller's ECC error injection; each is
  reported as a CE with the expected failure address.
- Link to v1: https://lore.kernel.org/r/20260812-edac-v1-0-03992edea297@aspeedtech.com

---
Ryan Chen (8):
      EDAC/aspeed: Set the DIMM grain
      EDAC/aspeed: Free the interrupt before the mem_ctl_info on remove
      EDAC/aspeed: Clean up whitespace and include ordering
      EDAC/aspeed: Free the mem_ctl_info unconditionally on remove
      dt-bindings: edac: aspeed: Add AST2700 SDRAM EDAC
      EDAC/aspeed: Replace regmap with direct register access
      EDAC/aspeed: Abstract SoC differences behind chip data
      EDAC/aspeed: Add AST2700 support

Stefan M Schaeckeler (1):
      MAINTAINERS: Step down as Aspeed AST2500 EDAC driver maintainer

 CREDITS                                            |   8 +
 .../bindings/edac/aspeed,ast2400-sdram-edac.yaml   |   6 +-
 MAINTAINERS                                        |   2 +-
 drivers/edac/Makefile                              |   1 +
 drivers/edac/aspeed_edac.c                         | 426 ++++++++++++++-------
 5 files changed, 293 insertions(+), 150 deletions(-)
---
base-commit: 5464985e42c04e335fb30e38fbc409c997db9bec
change-id: 20260625-edac-8e960e02e7f7

Best regards,
-- 
Ryan Chen <ryan_chen@aspeedtech.com>

[PATCH v4 1/9] EDAC/aspeed: Set the DIMM grain

[Ryan Chen]

dimm->grain is never set, so every error is reported with a granularity
of zero, which edac_raw_mc_handle_error() warns about and substitutes
with 1.

The controller records the address of a failing access with 16-byte
granularity: MCR58 and MCR5C hold it in bits [30:4], with the low four
bits reserved. Set the grain to 16 accordingly.

Fixes: 9b7e6242ee4e ("EDAC, aspeed: Add an Aspeed AST2500 EDAC driver")
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>

---
Changes in v4:
- Explain where the grain value comes from rather than the warning it
  silences.
- Drop the Cc: stable tag.
---
 drivers/edac/aspeed_edac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
index 6e069b255595..83d60414f89a 100644
--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -266,6 +266,7 @@ static int init_csrows(struct mem_ctl_info *mci)
 	dimm->mtype = dram_type;
 	dimm->edac_mode = EDAC_SECDED;
 	dimm->nr_pages = nr_pages / csrow->nr_channels;
+	dimm->grain = 16;
 
 	dev_dbg(mci->pdev, "initialized dimm with first_page=0x%lx and nr_pages=0x%x\n",
 		csrow->first_page, nr_pages);

-- 
2.34.1

[PATCH v4 2/9] EDAC/aspeed: Free the interrupt before the mem_ctl_info on remove

[Ryan Chen]

The ECC interrupt is devm-managed, so it is only released after .remove()
has returned, and masking the controller does not wait for a handler
already running on another CPU. edac_mc_free() can therefore free the
mem_ctl_info the handler uses as its context while it is still running.

Fix the ordering and synchronise by freeing the interrupt prior to
releasing related memory.

Fixes: 9b7e6242ee4e ("EDAC, aspeed: Add an Aspeed AST2500 EDAC driver")
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>

---
Changes in v4:
- Drop the Cc: stable tag.

Changes in v2:
- New patch.
---
 drivers/edac/aspeed_edac.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
index 83d60414f89a..e05ebed5c2f2 100644
--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -359,11 +359,16 @@ static int aspeed_probe(struct platform_device *pdev)
 static void aspeed_remove(struct platform_device *pdev)
 {
 	struct mem_ctl_info *mci;
+	int irq;
 
 	/* disable interrupts */
 	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
 			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
 
+	irq = platform_get_irq(pdev, 0);
+	WARN_ON(irq < 0);
+	devm_free_irq(&pdev->dev, irq, platform_get_drvdata(pdev));
+
 	/* free resources */
 	mci = edac_mc_del_mc(&pdev->dev);
 	if (mci)

-- 
2.34.1

Re: [PATCH v4 2/9] EDAC/aspeed: Free the interrupt before the mem_ctl_info on remove

[Borislav Petkov]

On Wed, Sep 30, 2026 at 01:15:00PM +0800, Ryan Chen wrote:
> The ECC interrupt is devm-managed, so it is only released after .remove()
> has returned, and masking the controller does not wait for a handler
> already running on another CPU. edac_mc_free() can therefore free the
> mem_ctl_info the handler uses as its context while it is still running.
> 
> Fix the ordering and synchronise by freeing the interrupt prior to
> releasing related memory.
> 
> Fixes: 9b7e6242ee4e ("EDAC, aspeed: Add an Aspeed AST2500 EDAC driver")
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> 
> ---
> Changes in v4:
> - Drop the Cc: stable tag.
> 
> Changes in v2:
> - New patch.
> ---
>  drivers/edac/aspeed_edac.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> index 83d60414f89a..e05ebed5c2f2 100644
> --- a/drivers/edac/aspeed_edac.c
> +++ b/drivers/edac/aspeed_edac.c
> @@ -359,11 +359,16 @@ static int aspeed_probe(struct platform_device *pdev)
>  static void aspeed_remove(struct platform_device *pdev)
>  {
>  	struct mem_ctl_info *mci;
> +	int irq;
>  
>  	/* disable interrupts */
>  	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
>  			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
>  
> +	irq = platform_get_irq(pdev, 0);
> +	WARN_ON(irq < 0);
> +	devm_free_irq(&pdev->dev, irq, platform_get_drvdata(pdev));

A couple of things come to mind when looking at this:

- if you have to do platform_get_irq(), then something in the design of this
  driver is missing. Perhaps a private structure:

  struct mem_ctl_info.pvt_info

  which contains driver-specific stuff like the irq number and so on. Look at
  the other EDAC drivers for examples.

- then, what is the point of this being a devm-managed IRQ if you have to
  explicitly free it?

  By that logic, you might as well use the normal IRQ registration stuff.

  But then looking at the other EDAC drivers, they all do devm_request_irq()
  but only the DMC-520 one does free_irq().

  So that would mean that either all the other EDAC drivers need such a fix.
  And they would all need proper auditing to make sure when they disable IRQs,
  all IRQ handlers running have finished.

  Or, their hw can handle that and IRQs firing at driver remove time is not an
  issue...

  Sounds to me like I need to do a EDAC subsystem TODO for all the driver
  owners to go read and check...

So I'm presuming this is Sashiko reporting an issue and you're addressing it.
Or is it a real issue that someone hits?

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH v4 2/9] EDAC/aspeed: Free the interrupt before the mem_ctl_info on remove

[Ryan Chen]

> Subject: Re: [PATCH v4 2/9] EDAC/aspeed: Free the interrupt before the
> mem_ctl_info on remove
> 
> On Wed, Sep 30, 2026 at 01:15:00PM +0800, Ryan Chen wrote:
> > The ECC interrupt is devm-managed, so it is only released after
> > .remove() has returned, and masking the controller does not wait for a
> > handler already running on another CPU. edac_mc_free() can therefore
> > free the mem_ctl_info the handler uses as its context while it is still running.
> >
> > Fix the ordering and synchronise by freeing the interrupt prior to
> > releasing related memory.
> >
> > Fixes: 9b7e6242ee4e ("EDAC, aspeed: Add an Aspeed AST2500 EDAC
> > driver")
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >
> > ---
> > Changes in v4:
> > - Drop the Cc: stable tag.
> >
> > Changes in v2:
> > - New patch.
> > ---
> >  drivers/edac/aspeed_edac.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
> > index 83d60414f89a..e05ebed5c2f2 100644
> > --- a/drivers/edac/aspeed_edac.c
> > +++ b/drivers/edac/aspeed_edac.c
> > @@ -359,11 +359,16 @@ static int aspeed_probe(struct platform_device
> > *pdev)  static void aspeed_remove(struct platform_device *pdev)  {
> >  	struct mem_ctl_info *mci;
> > +	int irq;
> >
> >  	/* disable interrupts */
> >  	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
> >  			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
> >
> > +	irq = platform_get_irq(pdev, 0);
> > +	WARN_ON(irq < 0);
> > +	devm_free_irq(&pdev->dev, irq, platform_get_drvdata(pdev));
> 
> A couple of things come to mind when looking at this:
> 
> - if you have to do platform_get_irq(), then something in the design of this
>   driver is missing. Perhaps a private structure:
> 
>   struct mem_ctl_info.pvt_info
> 
>   which contains driver-specific stuff like the irq number and so on. Look at
>   the other EDAC drivers for examples.

Patch 7 does that - it moves the register base, the lock and the IRQ
number into a struct in mci->pvt_info, and aspeed_remove() uses
priv->irq from there. This patch sits ahead of it, while the driver
still keeps its state in file-scope variables, which is why it reached
for platform_get_irq().


> 
> - then, what is the point of this being a devm-managed IRQ if you have to
>   explicitly free it?
> 
>   By that logic, you might as well use the normal IRQ registration stuff.
> 

Agreed. I will update with following in V5.
The IRQ number goes next to the state the driver already
has and the handler is registered normally, so neither
platform_get_irq() nor devres is involved:

--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -36,6 +36,7 @@
 
 static struct regmap *aspeed_regmap;
+static int aspeed_irq;
 
@@ -333,11 +334,12 @@ static int config_irq(void *ctx, struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	rc = devm_request_irq(&pdev->dev, irq, mcr_isr, IRQF_TRIGGER_HIGH,
-			      DRV_NAME, ctx);
+	rc = request_irq(irq, mcr_isr, IRQF_TRIGGER_HIGH, DRV_NAME, ctx);
 	if (rc)
 		return rc;
 
+	aspeed_irq = irq;
+
 	/* enable interrupts */
 	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
 			   ASPEED_MCR_INTR_CTRL_ENABLE,
@@ -359,6 +361,8 @@ static void aspeed_remove(struct platform_device *pdev)
 	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
 			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
 
+	free_irq(aspeed_irq, platform_get_drvdata(pdev));
+
 	/* free resources */
 	mci = edac_mc_del_mc(&pdev->dev);
 	if (mci)

config_irq() is the last thing aspeed_probe() does and it returns 0
straight afterwards, so there is no error path for devres to unwind in
the first place. Patch 7 then folds aspeed_irq into the private
structure along with the register base and the lock, and patches 7 and
8 use free_irq(priv->irq, mci).

>   But then looking at the other EDAC drivers, they all do devm_request_irq()
>   but only the DMC-520 one does free_irq().
> 
>   So that would mean that either all the other EDAC drivers need such a fix.
>   And they would all need proper auditing to make sure when they disable
> IRQs,
>   all IRQ handlers running have finished.
> 
>   Or, their hw can handle that and IRQs firing at driver remove time is not an
>   issue...
> 
>   Sounds to me like I need to do a EDAC subsystem TODO for all the driver
>   owners to go read and check...
> 
> So I'm presuming this is Sashiko reporting an issue and you're addressing it.
> Or is it a real issue that someone hits?

Sashiko, on v1 - it flagged the use-after-free as pre-existing and
suggested either freeing the interrupt before the mem_ctl_info or
dropping devm.
Nobody has hit it: I have not seen it and I have no report of one.

Regards
Ryan

[PATCH v4 3/9] EDAC/aspeed: Clean up whitespace and include ordering

[Ryan Chen]

The driver separates functions and definition groups with two blank
lines where the kernel style uses one, its headers are not sorted, and it
includes linux/stop_machine.h without using it. Collapse the double blank
lines, drop the unused include and sort the rest alphabetically so the
following changes start from a consistent style.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/edac/aspeed_edac.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
index e05ebed5c2f2..cd2a6fcca355 100644
--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -4,20 +4,17 @@
  */
 
 #include <linux/edac.h>
-#include <linux/module.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
-#include <linux/platform_device.h>
-#include <linux/stop_machine.h>
 #include <linux/io.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include "edac_module.h"
 
-
 #define DRV_NAME "aspeed-edac"
 
-
 #define ASPEED_MCR_PROT        0x00 /* protection key register */
 #define ASPEED_MCR_CONF        0x04 /* configuration register */
 #define ASPEED_MCR_INTR_CTRL   0x50 /* interrupt control/status register */
@@ -25,19 +22,16 @@
 #define ASPEED_MCR_ADDR_REC    0x5c /* address of last recoverable error */
 #define ASPEED_MCR_LAST        ASPEED_MCR_ADDR_REC
 
-
-#define ASPEED_MCR_PROT_PASSWD	            0xfc600309
-#define ASPEED_MCR_CONF_DRAM_TYPE               BIT(4)
-#define ASPEED_MCR_CONF_ECC                     BIT(7)
-#define ASPEED_MCR_INTR_CTRL_CLEAR             BIT(31)
-#define ASPEED_MCR_INTR_CTRL_CNT_REC   GENMASK(23, 16)
-#define ASPEED_MCR_INTR_CTRL_CNT_UNREC GENMASK(15, 12)
-#define ASPEED_MCR_INTR_CTRL_ENABLE  (BIT(0) | BIT(1))
-
+#define ASPEED_MCR_PROT_PASSWD          0xfc600309
+#define ASPEED_MCR_CONF_DRAM_TYPE       BIT(4)
+#define ASPEED_MCR_CONF_ECC             BIT(7)
+#define ASPEED_MCR_INTR_CTRL_CLEAR      BIT(31)
+#define ASPEED_MCR_INTR_CTRL_CNT_REC    GENMASK(23, 16)
+#define ASPEED_MCR_INTR_CTRL_CNT_UNREC  GENMASK(15, 12)
+#define ASPEED_MCR_INTR_CTRL_ENABLE     (BIT(0) | BIT(1))
 
 static struct regmap *aspeed_regmap;
 
-
 static int regmap_reg_write(void *context, unsigned int reg, unsigned int val)
 {
 	void __iomem *regs = (void __iomem *)context;
@@ -53,7 +47,6 @@ static int regmap_reg_write(void *context, unsigned int reg, unsigned int val)
 	return 0;
 }
 
-
 static int regmap_reg_read(void *context, unsigned int reg, unsigned int *val)
 {
 	void __iomem *regs = (void __iomem *)context;
@@ -76,7 +69,6 @@ static bool regmap_is_volatile(struct device *dev, unsigned int reg)
 	}
 }
 
-
 static const struct regmap_config aspeed_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -88,7 +80,6 @@ static const struct regmap_config aspeed_regmap_config = {
 	.fast_io = true,
 };
 
-
 static void count_rec(struct mem_ctl_info *mci, u8 rec_cnt, u32 rec_addr)
 {
 	struct csrow_info *csrow = mci->csrows[0];
@@ -120,7 +111,6 @@ static void count_rec(struct mem_ctl_info *mci, u8 rec_cnt, u32 rec_addr)
 			     0, 0, -1, "", "");
 }
 
-
 static void count_un_rec(struct mem_ctl_info *mci, u8 un_rec_cnt,
 			 u32 un_rec_addr)
 {
@@ -153,7 +143,6 @@ static void count_un_rec(struct mem_ctl_info *mci, u8 un_rec_cnt,
 	}
 }
 
-
 static irqreturn_t mcr_isr(int irq, void *arg)
 {
 	struct mem_ctl_info *mci = arg;
@@ -200,7 +189,6 @@ static irqreturn_t mcr_isr(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-
 static int config_irq(void *ctx, struct platform_device *pdev)
 {
 	int irq;
@@ -225,7 +213,6 @@ static int config_irq(void *ctx, struct platform_device *pdev)
 	return 0;
 }
 
-
 static int init_csrows(struct mem_ctl_info *mci)
 {
 	struct csrow_info *csrow = mci->csrows[0];
@@ -274,7 +261,6 @@ static int init_csrows(struct mem_ctl_info *mci)
 	return 0;
 }
 
-
 static int aspeed_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -355,7 +341,6 @@ static int aspeed_probe(struct platform_device *pdev)
 	return rc;
 }
 
-
 static void aspeed_remove(struct platform_device *pdev)
 {
 	struct mem_ctl_info *mci;
@@ -375,7 +360,6 @@ static void aspeed_remove(struct platform_device *pdev)
 		edac_mc_free(mci);
 }
 
-
 static const struct of_device_id aspeed_of_match[] = {
 	{ .compatible = "aspeed,ast2400-sdram-edac" },
 	{ .compatible = "aspeed,ast2500-sdram-edac" },

-- 
2.34.1

[PATCH v4 4/9] EDAC/aspeed: Free the mem_ctl_info unconditionally on remove

[Ryan Chen]

aspeed_remove() is a driver .remove callback, so it only runs for a device
that has already probed successfully and registered its mem_ctl_info. In
that case edac_mc_del_mc() always returns the same, valid pointer, and the
NULL check on its return value can never be false.

Fetch the mem_ctl_info from the platform device's driver data instead and
free it unconditionally, dropping the redundant check. This also decouples
the teardown from the return value of edac_mc_del_mc().

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/edac/aspeed_edac.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
index cd2a6fcca355..352910e1defc 100644
--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -343,7 +343,7 @@ static int aspeed_probe(struct platform_device *pdev)
 
 static void aspeed_remove(struct platform_device *pdev)
 {
-	struct mem_ctl_info *mci;
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
 	int irq;
 
 	/* disable interrupts */
@@ -352,12 +352,11 @@ static void aspeed_remove(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	WARN_ON(irq < 0);
-	devm_free_irq(&pdev->dev, irq, platform_get_drvdata(pdev));
+	devm_free_irq(&pdev->dev, irq, mci);
 
 	/* free resources */
-	mci = edac_mc_del_mc(&pdev->dev);
-	if (mci)
-		edac_mc_free(mci);
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
 }
 
 static const struct of_device_id aspeed_of_match[] = {

-- 
2.34.1

[PATCH v4 5/9] dt-bindings: edac: aspeed: Add AST2700 SDRAM EDAC

[Ryan Chen]

Add the "aspeed,ast2700-sdram-edac" compatible and note DDR5 support.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 .../devicetree/bindings/edac/aspeed,ast2400-sdram-edac.yaml         | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/edac/aspeed,ast2400-sdram-edac.yaml b/Documentation/devicetree/bindings/edac/aspeed,ast2400-sdram-edac.yaml
index 09735826d707..685b6815f293 100644
--- a/Documentation/devicetree/bindings/edac/aspeed,ast2400-sdram-edac.yaml
+++ b/Documentation/devicetree/bindings/edac/aspeed,ast2400-sdram-edac.yaml
@@ -10,8 +10,9 @@ maintainers:
   - Stefan Schaeckeler <sschaeck@cisco.com>
 
 description: >
-  The Aspeed BMC SoC supports DDR3 and DDR4 memory with and without ECC (error
-  correction check).
+  The Aspeed BMC SoCs support DDR memory with and without ECC (error
+  correction check): DDR3 and DDR4 on the AST2400, AST2500 and AST2600,
+  and DDR4 and DDR5 on the AST2700.
 
   The memory controller supports SECDED (single bit error correction, double bit
   error detection) and single bit error auto scrubbing by reserving 8 bits for
@@ -25,6 +26,7 @@ properties:
       - aspeed,ast2400-sdram-edac
       - aspeed,ast2500-sdram-edac
       - aspeed,ast2600-sdram-edac
+      - aspeed,ast2700-sdram-edac
 
   reg:
     maxItems: 1

-- 
2.34.1

[PATCH v4 6/9] EDAC/aspeed: Replace regmap with direct register access

[Ryan Chen]

The driver instantiates its own regmap purely as an MMIO wrapper: it has
no register cache, uses custom .reg_read()/.reg_write() callbacks, and is
not shared as a syscon with other drivers. So it brings nothing here
beyond the spinlock that regmap takes around each access when fast_io is
set.

Drop the regmap and access the registers directly with readl()/writel()
under an explicit raw spinlock, held across the whole read-modify-write so
the controller is unlocked once around the grouped writes rather than on
every register write.

Annotate the register base with __guarded_by() so that, under
CONFIG_WARN_CONTEXT_ANALYSIS, the compiler checks at build time that every
hardware register access is performed while holding the lock.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>

---
Changes in v2:
- Take the register lock with the irqsave variant in init_csrows() and in
  aspeed_probe(); the interrupt handler takes the same lock in hardirq
  context.
- Move the dev_dbg() of the interrupt status register out of the
  raw_spinlock critical section in the interrupt handler.
- Opt aspeed_edac.o into context analysis in drivers/edac/Makefile, so
  that the __guarded_by() annotation is actually checked.
- Note in the interrupt handler that the counter and interrupt flag
  fields are read-only, so writing back the read value is harmless.
---
 drivers/edac/Makefile      |   1 +
 drivers/edac/aspeed_edac.c | 131 +++++++++++++++++----------------------------
 2 files changed, 51 insertions(+), 81 deletions(-)

diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index a37534300ab9..9215dd0bb835 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
+CONTEXT_ANALYSIS_aspeed_edac.o		:= y
 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/aspeed_edac.c b/drivers/edac/aspeed_edac.c
index 352910e1defc..26d2c456cc0d 100644
--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -3,6 +3,7 @@
  * Copyright 2018, 2019 Cisco Systems
  */
 
+#include <linux/cleanup.h>
 #include <linux/edac.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -10,7 +11,7 @@
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
-#include <linux/regmap.h>
+#include <linux/spinlock.h>
 #include "edac_module.h"
 
 #define DRV_NAME "aspeed-edac"
@@ -20,7 +21,6 @@
 #define ASPEED_MCR_INTR_CTRL   0x50 /* interrupt control/status register */
 #define ASPEED_MCR_ADDR_UNREC  0x58 /* address of first un-recoverable error */
 #define ASPEED_MCR_ADDR_REC    0x5c /* address of last recoverable error */
-#define ASPEED_MCR_LAST        ASPEED_MCR_ADDR_REC
 
 #define ASPEED_MCR_PROT_PASSWD          0xfc600309
 #define ASPEED_MCR_CONF_DRAM_TYPE       BIT(4)
@@ -30,55 +30,8 @@
 #define ASPEED_MCR_INTR_CTRL_CNT_UNREC  GENMASK(15, 12)
 #define ASPEED_MCR_INTR_CTRL_ENABLE     (BIT(0) | BIT(1))
 
-static struct regmap *aspeed_regmap;
-
-static int regmap_reg_write(void *context, unsigned int reg, unsigned int val)
-{
-	void __iomem *regs = (void __iomem *)context;
-
-	/* enable write to MCR register set */
-	writel(ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT);
-
-	writel(val, regs + reg);
-
-	/* disable write to MCR register set */
-	writel(~ASPEED_MCR_PROT_PASSWD, regs + ASPEED_MCR_PROT);
-
-	return 0;
-}
-
-static int regmap_reg_read(void *context, unsigned int reg, unsigned int *val)
-{
-	void __iomem *regs = (void __iomem *)context;
-
-	*val = readl(regs + reg);
-
-	return 0;
-}
-
-static bool regmap_is_volatile(struct device *dev, unsigned int reg)
-{
-	switch (reg) {
-	case ASPEED_MCR_PROT:
-	case ASPEED_MCR_INTR_CTRL:
-	case ASPEED_MCR_ADDR_UNREC:
-	case ASPEED_MCR_ADDR_REC:
-		return true;
-	default:
-		return false;
-	}
-}
-
-static const struct regmap_config aspeed_regmap_config = {
-	.reg_bits = 32,
-	.val_bits = 32,
-	.reg_stride = 4,
-	.max_register = ASPEED_MCR_LAST,
-	.reg_write = regmap_reg_write,
-	.reg_read = regmap_reg_read,
-	.volatile_reg = regmap_is_volatile,
-	.fast_io = true,
-};
+static DEFINE_RAW_SPINLOCK(aspeed_lock);
+static void __iomem *aspeed_regs __guarded_by(&aspeed_lock);
 
 static void count_rec(struct mem_ctl_info *mci, u8 rec_cnt, u32 rec_addr)
 {
@@ -147,10 +100,27 @@ static irqreturn_t mcr_isr(int irq, void *arg)
 {
 	struct mem_ctl_info *mci = arg;
 	u32 rec_addr, un_rec_addr;
-	u32 reg50, reg5c, reg58;
-	u8  rec_cnt, un_rec_cnt;
+	u8 rec_cnt, un_rec_cnt;
+	u32 reg50;
+
+	scoped_guard(raw_spinlock, &aspeed_lock) {
+		reg50 = readl(aspeed_regs + ASPEED_MCR_INTR_CTRL);
+		un_rec_addr = readl(aspeed_regs + ASPEED_MCR_ADDR_UNREC);
+		rec_addr = readl(aspeed_regs + ASPEED_MCR_ADDR_REC);
+
+		/*
+		 * Clearing the counters needs a set-then-clear of CLEAR. The
+		 * counter and interrupt flag fields are read-only, so writing
+		 * back the values read above leaves them unaffected.
+		 */
+		writel(ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
+		writel(reg50 | ASPEED_MCR_INTR_CTRL_CLEAR,
+		       aspeed_regs + ASPEED_MCR_INTR_CTRL);
+		writel(reg50 & ~ASPEED_MCR_INTR_CTRL_CLEAR,
+		       aspeed_regs + ASPEED_MCR_INTR_CTRL);
+		writel(~ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
+	}
 
-	regmap_read(aspeed_regmap, ASPEED_MCR_INTR_CTRL, &reg50);
 	dev_dbg(mci->pdev, "received edac interrupt w/ mcr register 50: 0x%x\n",
 		reg50);
 
@@ -161,20 +131,6 @@ static irqreturn_t mcr_isr(int irq, void *arg)
 	dev_dbg(mci->pdev, "%d recoverable interrupts and %d unrecoverable interrupts\n",
 		rec_cnt, un_rec_cnt);
 
-	regmap_read(aspeed_regmap, ASPEED_MCR_ADDR_UNREC, &reg58);
-	un_rec_addr = reg58;
-
-	regmap_read(aspeed_regmap, ASPEED_MCR_ADDR_REC, &reg5c);
-	rec_addr = reg5c;
-
-	/* clear interrupt flags and error counters: */
-	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
-			   ASPEED_MCR_INTR_CTRL_CLEAR,
-			   ASPEED_MCR_INTR_CTRL_CLEAR);
-
-	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
-			   ASPEED_MCR_INTR_CTRL_CLEAR, 0);
-
 	/* process recoverable and unrecoverable errors */
 	count_rec(mci, rec_cnt, rec_addr);
 	count_un_rec(mci, un_rec_cnt, un_rec_addr);
@@ -182,13 +138,31 @@ static irqreturn_t mcr_isr(int irq, void *arg)
 	if (!rec_cnt && !un_rec_cnt)
 		dev_dbg(mci->pdev, "received edac interrupt, but did not find any ECC counters\n");
 
-	regmap_read(aspeed_regmap, ASPEED_MCR_INTR_CTRL, &reg50);
+	scoped_guard(raw_spinlock, &aspeed_lock)
+		reg50 = readl(aspeed_regs + ASPEED_MCR_INTR_CTRL);
 	dev_dbg(mci->pdev, "edac interrupt handled. mcr reg 50 is now: 0x%x\n",
 		reg50);
 
 	return IRQ_HANDLED;
 }
 
+static void aspeed_set_irq(bool enable)
+{
+	u32 val;
+
+	guard(raw_spinlock_irqsave)(&aspeed_lock);
+
+	val = readl(aspeed_regs + ASPEED_MCR_INTR_CTRL);
+	if (enable)
+		val |= ASPEED_MCR_INTR_CTRL_ENABLE;
+	else
+		val &= ~ASPEED_MCR_INTR_CTRL_ENABLE;
+
+	writel(ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
+	writel(val, aspeed_regs + ASPEED_MCR_INTR_CTRL);
+	writel(~ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
+}
+
 static int config_irq(void *ctx, struct platform_device *pdev)
 {
 	int irq;
@@ -206,9 +180,7 @@ static int config_irq(void *ctx, struct platform_device *pdev)
 		return rc;
 
 	/* enable interrupts */
-	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
-			   ASPEED_MCR_INTR_CTRL_ENABLE,
-			   ASPEED_MCR_INTR_CTRL_ENABLE);
+	aspeed_set_irq(true);
 
 	return 0;
 }
@@ -246,7 +218,8 @@ static int init_csrows(struct mem_ctl_info *mci)
 	nr_pages = resource_size(&r) >> PAGE_SHIFT;
 	csrow->last_page = csrow->first_page + nr_pages - 1;
 
-	regmap_read(aspeed_regmap, ASPEED_MCR_CONF, &reg04);
+	scoped_guard(raw_spinlock_irqsave, &aspeed_lock)
+		reg04 = readl(aspeed_regs + ASPEED_MCR_CONF);
 	dram_type = (reg04 & ASPEED_MCR_CONF_DRAM_TYPE) ? MEM_DDR4 : MEM_DDR3;
 
 	dimm = csrow->channels[0]->dimm;
@@ -263,7 +236,6 @@ static int init_csrows(struct mem_ctl_info *mci)
 
 static int aspeed_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
 	struct edac_mc_layer layers[2];
 	struct mem_ctl_info *mci;
 	void __iomem *regs;
@@ -274,13 +246,11 @@ static int aspeed_probe(struct platform_device *pdev)
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-	aspeed_regmap = devm_regmap_init(dev, NULL, (__force void *)regs,
-					 &aspeed_regmap_config);
-	if (IS_ERR(aspeed_regmap))
-		return PTR_ERR(aspeed_regmap);
+	scoped_guard(raw_spinlock_irqsave, &aspeed_lock)
+		aspeed_regs = regs;
 
 	/* bail out if ECC mode is not configured */
-	regmap_read(aspeed_regmap, ASPEED_MCR_CONF, &reg04);
+	reg04 = readl(regs + ASPEED_MCR_CONF);
 	if (!(reg04 & ASPEED_MCR_CONF_ECC)) {
 		dev_err(&pdev->dev, "ECC mode is not configured in u-boot\n");
 		return -EPERM;
@@ -347,8 +317,7 @@ static void aspeed_remove(struct platform_device *pdev)
 	int irq;
 
 	/* disable interrupts */
-	regmap_update_bits(aspeed_regmap, ASPEED_MCR_INTR_CTRL,
-			   ASPEED_MCR_INTR_CTRL_ENABLE, 0);
+	aspeed_set_irq(false);
 
 	irq = platform_get_irq(pdev, 0);
 	WARN_ON(irq < 0);

-- 
2.34.1

[PATCH v4 7/9] EDAC/aspeed: Abstract SoC differences behind chip data

[Ryan Chen]

The driver hard-codes the AST2400/2500/2600 register layout, ECC and
DRAM-type bits, memory types and write-protection key. Abstract these
SoC-specific details behind a per-SoC struct aspeed_edac_chip selected by
the compatible, and move the per-instance state (register base, lock)
into mci->pvt_info instead of globals, so controller variants that differ
in these details can be added as table data.

Only the AST2400 and AST2500 key-protect the interrupt control register
(MCR50); the AST2600 does not. Gate the unlock/relock on the chip carrying
a protection key and split the shared entry into keyed (AST2400/2500) and
unkeyed (AST2600) variants, so the AST2600 no longer performs the
unnecessary unlock.

Tested on an AST2600: A correctable error was injected from the console by
unlocking the controller and writing its ECC error inject test register:

  # mw 1e6e0000 fc600309
  # mw 1e6e00b0 81
  EDAC MC0: 1 CE address(es) not available on mc#0csrow#0channel#0 (csrow:0 channel:0 page:0x0 offset:0x0 grain:16 syndrome:0x0)
  EDAC MC0: 1 CE on mc#0csrow#0channel#0 (csrow:0 channel:0 page:0x8a543 offset:0xec0 grain:16 syndrome:0x0)

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>

---
Changes in v2:
- Carry over the irqsave lock acquisition in init_csrows() and the
  dev_dbg() moved out of the raw_spinlock critical section.
- Store the interrupt number in struct aspeed_edac so that
  aspeed_remove() can free the interrupt without looking it up again.
---
 drivers/edac/aspeed_edac.c | 163 +++++++++++++++++++++++++++++++--------------
 1 file changed, 114 insertions(+), 49 deletions(-)

diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
index 26d2c456cc0d..e88d9d2646de 100644
--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -3,12 +3,14 @@
  * Copyright 2018, 2019 Cisco Systems
  */
 
+#include <linux/bitfield.h>
 #include <linux/cleanup.h>
 #include <linux/edac.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
@@ -30,8 +32,23 @@
 #define ASPEED_MCR_INTR_CTRL_CNT_UNREC  GENMASK(15, 12)
 #define ASPEED_MCR_INTR_CTRL_ENABLE     (BIT(0) | BIT(1))
 
-static DEFINE_RAW_SPINLOCK(aspeed_lock);
-static void __iomem *aspeed_regs __guarded_by(&aspeed_lock);
+struct aspeed_edac_chip {
+	unsigned int conf_reg;
+	u32 conf_ecc;
+	u32 conf_dram_type;
+	enum mem_type dram_type[2];
+	unsigned long mtype_cap;
+	unsigned int prot_reg;
+	u32 prot_key;
+};
+
+struct aspeed_edac {
+	raw_spinlock_t lock;
+
+	void __iomem *regs __guarded_by(&lock);
+	const struct aspeed_edac_chip *chip;
+	int irq;
+};
 
 static void count_rec(struct mem_ctl_info *mci, u8 rec_cnt, u32 rec_addr)
 {
@@ -96,37 +113,54 @@ static void count_un_rec(struct mem_ctl_info *mci, u8 un_rec_cnt,
 	}
 }
 
-static irqreturn_t mcr_isr(int irq, void *arg)
+static void aspeed_mcr_irq_update_enter(struct aspeed_edac *priv)
+	__must_hold(&priv->lock)
+{
+	if (priv->chip->prot_key)
+		writel(priv->chip->prot_key, priv->regs + priv->chip->prot_reg);
+}
+
+static void aspeed_mcr_irq_update_exit(struct aspeed_edac *priv)
+	__must_hold(&priv->lock)
+{
+	if (priv->chip->prot_key)
+		writel(~priv->chip->prot_key, priv->regs + priv->chip->prot_reg);
+}
+
+static irqreturn_t aspeed_mcr_isr(int irq, void *arg)
 {
 	struct mem_ctl_info *mci = arg;
 	u32 rec_addr, un_rec_addr;
+	struct aspeed_edac *priv;
 	u8 rec_cnt, un_rec_cnt;
 	u32 reg50;
 
-	scoped_guard(raw_spinlock, &aspeed_lock) {
-		reg50 = readl(aspeed_regs + ASPEED_MCR_INTR_CTRL);
-		un_rec_addr = readl(aspeed_regs + ASPEED_MCR_ADDR_UNREC);
-		rec_addr = readl(aspeed_regs + ASPEED_MCR_ADDR_REC);
+	priv = mci->pvt_info;
+
+	scoped_guard(raw_spinlock, &priv->lock) {
+		reg50 = readl(priv->regs + ASPEED_MCR_INTR_CTRL);
+		un_rec_addr = readl(priv->regs + ASPEED_MCR_ADDR_UNREC);
+		rec_addr = readl(priv->regs + ASPEED_MCR_ADDR_REC);
 
 		/*
 		 * Clearing the counters needs a set-then-clear of CLEAR. The
 		 * counter and interrupt flag fields are read-only, so writing
 		 * back the values read above leaves them unaffected.
 		 */
-		writel(ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
+		aspeed_mcr_irq_update_enter(priv);
 		writel(reg50 | ASPEED_MCR_INTR_CTRL_CLEAR,
-		       aspeed_regs + ASPEED_MCR_INTR_CTRL);
+		       priv->regs + ASPEED_MCR_INTR_CTRL);
 		writel(reg50 & ~ASPEED_MCR_INTR_CTRL_CLEAR,
-		       aspeed_regs + ASPEED_MCR_INTR_CTRL);
-		writel(~ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
+		       priv->regs + ASPEED_MCR_INTR_CTRL);
+		aspeed_mcr_irq_update_exit(priv);
 	}
 
 	dev_dbg(mci->pdev, "received edac interrupt w/ mcr register 50: 0x%x\n",
 		reg50);
 
 	/* collect data about recoverable and unrecoverable errors */
-	rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_REC) >> 16;
-	un_rec_cnt = (reg50 & ASPEED_MCR_INTR_CTRL_CNT_UNREC) >> 12;
+	rec_cnt = FIELD_GET(ASPEED_MCR_INTR_CTRL_CNT_REC, reg50);
+	un_rec_cnt = FIELD_GET(ASPEED_MCR_INTR_CTRL_CNT_UNREC, reg50);
 
 	dev_dbg(mci->pdev, "%d recoverable interrupts and %d unrecoverable interrupts\n",
 		rec_cnt, un_rec_cnt);
@@ -136,35 +170,36 @@ static irqreturn_t mcr_isr(int irq, void *arg)
 	count_un_rec(mci, un_rec_cnt, un_rec_addr);
 
 	if (!rec_cnt && !un_rec_cnt)
-		dev_dbg(mci->pdev, "received edac interrupt, but did not find any ECC counters\n");
+		dev_dbg_ratelimited(mci->pdev, "received edac interrupt, but did not find any ECC counters\n");
 
-	scoped_guard(raw_spinlock, &aspeed_lock)
-		reg50 = readl(aspeed_regs + ASPEED_MCR_INTR_CTRL);
+	scoped_guard(raw_spinlock, &priv->lock)
+		reg50 = readl(priv->regs + ASPEED_MCR_INTR_CTRL);
 	dev_dbg(mci->pdev, "edac interrupt handled. mcr reg 50 is now: 0x%x\n",
 		reg50);
 
 	return IRQ_HANDLED;
 }
 
-static void aspeed_set_irq(bool enable)
+static void aspeed_set_irq(struct aspeed_edac *priv, bool enable)
 {
 	u32 val;
 
-	guard(raw_spinlock_irqsave)(&aspeed_lock);
+	guard(raw_spinlock_irqsave)(&priv->lock);
 
-	val = readl(aspeed_regs + ASPEED_MCR_INTR_CTRL);
+	val = readl(priv->regs + ASPEED_MCR_INTR_CTRL);
 	if (enable)
 		val |= ASPEED_MCR_INTR_CTRL_ENABLE;
 	else
 		val &= ~ASPEED_MCR_INTR_CTRL_ENABLE;
 
-	writel(ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
-	writel(val, aspeed_regs + ASPEED_MCR_INTR_CTRL);
-	writel(~ASPEED_MCR_PROT_PASSWD, aspeed_regs + ASPEED_MCR_PROT);
+	aspeed_mcr_irq_update_enter(priv);
+	writel(val, priv->regs + ASPEED_MCR_INTR_CTRL);
+	aspeed_mcr_irq_update_exit(priv);
 }
 
-static int config_irq(void *ctx, struct platform_device *pdev)
+static int config_irq(struct mem_ctl_info *mci, struct platform_device *pdev)
 {
+	struct aspeed_edac *priv = mci->pvt_info;
 	int irq;
 	int rc;
 
@@ -174,13 +209,15 @@ static int config_irq(void *ctx, struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	rc = devm_request_irq(&pdev->dev, irq, mcr_isr, IRQF_TRIGGER_HIGH,
-			      DRV_NAME, ctx);
+	rc = devm_request_irq(&pdev->dev, irq, aspeed_mcr_isr, IRQF_TRIGGER_HIGH,
+			      DRV_NAME, mci);
 	if (rc)
 		return rc;
 
+	priv->irq = irq;
+
 	/* enable interrupts */
-	aspeed_set_irq(true);
+	aspeed_set_irq(priv, true);
 
 	return 0;
 }
@@ -188,11 +225,13 @@ static int config_irq(void *ctx, struct platform_device *pdev)
 static int init_csrows(struct mem_ctl_info *mci)
 {
 	struct csrow_info *csrow = mci->csrows[0];
-	u32 nr_pages, dram_type;
-	struct dimm_info *dimm;
+	struct aspeed_edac *priv = mci->pvt_info;
 	struct device_node *np;
+	struct dimm_info *dimm;
 	struct resource r;
-	u32 reg04;
+	unsigned int type;
+	u32 nr_pages;
+	u32 conf;
 	int rc;
 
 	/* retrieve info about physical memory from device tree */
@@ -218,12 +257,12 @@ static int init_csrows(struct mem_ctl_info *mci)
 	nr_pages = resource_size(&r) >> PAGE_SHIFT;
 	csrow->last_page = csrow->first_page + nr_pages - 1;
 
-	scoped_guard(raw_spinlock_irqsave, &aspeed_lock)
-		reg04 = readl(aspeed_regs + ASPEED_MCR_CONF);
-	dram_type = (reg04 & ASPEED_MCR_CONF_DRAM_TYPE) ? MEM_DDR4 : MEM_DDR3;
+	scoped_guard(raw_spinlock_irqsave, &priv->lock)
+		conf = readl(priv->regs + priv->chip->conf_reg);
+	type = field_get(priv->chip->conf_dram_type, conf);
 
 	dimm = csrow->channels[0]->dimm;
-	dimm->mtype = dram_type;
+	dimm->mtype = priv->chip->dram_type[type];
 	dimm->edac_mode = EDAC_SECDED;
 	dimm->nr_pages = nr_pages / csrow->nr_channels;
 	dimm->grain = 16;
@@ -236,22 +275,26 @@ static int init_csrows(struct mem_ctl_info *mci)
 
 static int aspeed_probe(struct platform_device *pdev)
 {
+	const struct aspeed_edac_chip *chip;
+	struct device *dev = &pdev->dev;
 	struct edac_mc_layer layers[2];
+	struct aspeed_edac *priv;
 	struct mem_ctl_info *mci;
 	void __iomem *regs;
-	u32 reg04;
+	u32 conf;
 	int rc;
 
+	chip = of_device_get_match_data(dev);
+	if (!chip)
+		return -EINVAL;
+
 	regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
 
-	scoped_guard(raw_spinlock_irqsave, &aspeed_lock)
-		aspeed_regs = regs;
-
 	/* bail out if ECC mode is not configured */
-	reg04 = readl(regs + ASPEED_MCR_CONF);
-	if (!(reg04 & ASPEED_MCR_CONF_ECC)) {
+	conf = readl(regs + chip->conf_reg);
+	if (!field_get(chip->conf_ecc, conf)) {
 		dev_err(&pdev->dev, "ECC mode is not configured in u-boot\n");
 		return -EPERM;
 	}
@@ -266,12 +309,17 @@ static int aspeed_probe(struct platform_device *pdev)
 	layers[1].size = 1;
 	layers[1].is_virt_csrow = false;
 
-	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, 0);
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, sizeof(*priv));
 	if (!mci)
 		return -ENOMEM;
 
+	priv = mci->pvt_info;
+	priv->chip = chip;
+	scoped_guard(raw_spinlock_init, &priv->lock)
+		priv->regs = regs;
+
 	mci->pdev = &pdev->dev;
-	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4;
+	mci->mtype_cap = chip->mtype_cap;
 	mci->edac_ctl_cap = EDAC_FLAG_SECDED;
 	mci->edac_cap = EDAC_FLAG_SECDED;
 	mci->scrub_cap = SCRUB_FLAG_HW_SRC;
@@ -314,24 +362,41 @@ static int aspeed_probe(struct platform_device *pdev)
 static void aspeed_remove(struct platform_device *pdev)
 {
 	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
-	int irq;
+	struct aspeed_edac *priv = mci->pvt_info;
 
 	/* disable interrupts */
-	aspeed_set_irq(false);
+	aspeed_set_irq(priv, false);
 
-	irq = platform_get_irq(pdev, 0);
-	WARN_ON(irq < 0);
-	devm_free_irq(&pdev->dev, irq, mci);
+	devm_free_irq(&pdev->dev, priv->irq, mci);
 
 	/* free resources */
 	edac_mc_del_mc(&pdev->dev);
 	edac_mc_free(mci);
 }
 
+static const struct aspeed_edac_chip ast2400_edac = {
+	.conf_reg = ASPEED_MCR_CONF,
+	.conf_ecc = ASPEED_MCR_CONF_ECC,
+	.conf_dram_type = ASPEED_MCR_CONF_DRAM_TYPE,
+	.dram_type = { MEM_DDR3, MEM_DDR4 },
+	.mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4,
+	.prot_reg = ASPEED_MCR_PROT,
+	.prot_key = ASPEED_MCR_PROT_PASSWD,
+};
+
+/* The AST2600 does not key-protect the interrupt control register (MCR50). */
+static const struct aspeed_edac_chip ast2600_edac = {
+	.conf_reg = ASPEED_MCR_CONF,
+	.conf_ecc = ASPEED_MCR_CONF_ECC,
+	.conf_dram_type = ASPEED_MCR_CONF_DRAM_TYPE,
+	.dram_type = { MEM_DDR3, MEM_DDR4 },
+	.mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4,
+};
+
 static const struct of_device_id aspeed_of_match[] = {
-	{ .compatible = "aspeed,ast2400-sdram-edac" },
-	{ .compatible = "aspeed,ast2500-sdram-edac" },
-	{ .compatible = "aspeed,ast2600-sdram-edac" },
+	{ .compatible = "aspeed,ast2400-sdram-edac", .data = &ast2400_edac },
+	{ .compatible = "aspeed,ast2500-sdram-edac", .data = &ast2400_edac },
+	{ .compatible = "aspeed,ast2600-sdram-edac", .data = &ast2600_edac },
 	{},
 };
 

-- 
2.34.1

[PATCH v4 8/9] EDAC/aspeed: Add AST2700 support

[Ryan Chen]

Add SDRAM ECC reporting for the Aspeed AST2700. Its DRAMC has a different
register layout, a split interrupt status/clear/mask scheme, DDR4/DDR5
memory and interrupt registers that are not key-protected.

Its interrupt status/clear and enable sequences differ from the earlier
SoCs, so add per-chip isr() and set_irq() hooks and route the
devm_request_irq() and enable/disable paths through them, keeping the
existing AST2400/2500/2600 behaviour under the shared
aspeed_mcr_isr()/aspeed_set_irq().

Unlike the earlier SoCs it records a single failure address shared by
both error types, so extend the shared count_rec()/count_un_rec()
helpers with a have_addr flag to report an error without an address
(existing SoCs pass have_addr = true, unchanged) and widen their address
argument to phys_addr_t as the AST2700 address can exceed 32 bits.

Tested on an AST2700: A correctable error was injected from the console by
unlocking the controller and writing its ECC error inject test register:

  # mw 12c00000 1688a8a8
  # mw 12c00080 31
  EDAC MC0: 1 CE on mc#0csrow#0channel#0 (csrow:0 channel:0 page:0x40f6da offset:0xdb0 grain:16 syndrome:0x0)

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>

---
Changes in v3:
- Write the AST2700 interrupt mask register outright rather than
  read-modify-write: mask every source and unmask only the ECC ones.
  The driver owns the line exclusively, so it should establish the
  mask itself instead of deriving it from the register's contents.
- Mask the interrupts before requesting them rather than after, so the
  mask is established before the handler can run.

Changes in v2:
- Return IRQ_NONE from ast2700_dramc_isr() when no ECC interrupt status
  bit is set. The handler clears only the ECC bits, so unconditionally
  claiming the interrupt could livelock the level-triggered line if
  another DRAMC source was left unmasked.
- Add AST2700_INT_ECC for the pair of ECC interrupt bits, now used by both
  the handler and the enable path.
- Expand the enable/disable ternary in ast2700_set_irq() into if/else,
  matching the shape of aspeed_set_irq().
- Pass struct aspeed_edac to the set_irq() hooks instead of
  struct mem_ctl_info, which the callees only used to reach pvt_info.
---
 drivers/edac/aspeed_edac.c | 161 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 135 insertions(+), 26 deletions(-)

diff --git a/drivers/edac/aspeed_edac.c b/drivers/edac/aspeed_edac.c
index e88d9d2646de..6fe6314cda72 100644
--- a/drivers/edac/aspeed_edac.c
+++ b/drivers/edac/aspeed_edac.c
@@ -32,6 +32,24 @@
 #define ASPEED_MCR_INTR_CTRL_CNT_UNREC  GENMASK(15, 12)
 #define ASPEED_MCR_INTR_CTRL_ENABLE     (BIT(0) | BIT(1))
 
+#define AST2700_INT_STS			0x04
+#define AST2700_INT_CLR			0x08
+#define AST2700_INT_MASK		0x0c
+#define   AST2700_INT_ECC_RECOVERABLE	BIT(5)
+#define   AST2700_INT_ECC_UNRECOVERABLE	BIT(4)
+#define   AST2700_INT_ECC		(AST2700_INT_ECC_RECOVERABLE | \
+					 AST2700_INT_ECC_UNRECOVERABLE)
+#define   AST2700_INT_ALL		GENMASK(16, 0)
+#define AST2700_MCFG			0x10
+#define   AST2700_MCFG_ECC		BIT(6)
+#define   AST2700_MCFG_DRAM_TYPE	BIT(0) /* 0=DDR4, 1=DDR5 */
+#define AST2700_ECC_STS			0x78
+#define   AST2700_ECC_REC_CNT		GENMASK(15, 8)
+#define   AST2700_ECC_UNREC_CNT		GENMASK(7, 0)
+#define AST2700_ECC_FAIL_ADDR		0x7c
+
+struct aspeed_edac;
+
 struct aspeed_edac_chip {
 	unsigned int conf_reg;
 	u32 conf_ecc;
@@ -40,6 +58,8 @@ struct aspeed_edac_chip {
 	unsigned long mtype_cap;
 	unsigned int prot_reg;
 	u32 prot_key;
+	irqreturn_t (*isr)(int irq, void *arg);
+	void (*set_irq)(struct aspeed_edac *priv, bool enable);
 };
 
 struct aspeed_edac {
@@ -50,26 +70,34 @@ struct aspeed_edac {
 	int irq;
 };
 
-static void count_rec(struct mem_ctl_info *mci, u8 rec_cnt, u32 rec_addr)
+static void count_rec(struct mem_ctl_info *mci, u8 rec_cnt, phys_addr_t rec_addr,
+		      bool have_addr)
 {
 	struct csrow_info *csrow = mci->csrows[0];
-	u32 page, offset, syndrome;
+	unsigned long page, offset, syndrome;
 
 	if (!rec_cnt)
 		return;
 
-	/* report first few errors (if there are) */
-	/* note: no addresses are recorded */
-	if (rec_cnt > 1) {
+	/*
+	 * Report the errors whose address is not recorded: all of them when
+	 * no address is available, otherwise all but the last one (reported
+	 * with its address below).
+	 */
+	if (rec_cnt > 1 || !have_addr) {
 		/* page, offset and syndrome are not available */
 		page = 0;
 		offset = 0;
 		syndrome = 0;
-		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, rec_cnt-1,
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
+				     have_addr ? rec_cnt - 1 : rec_cnt,
 				     page, offset, syndrome, 0, 0, -1,
 				     "address(es) not available", "");
 	}
 
+	if (!have_addr)
+		return;
+
 	/* report last error */
 	/* note: rec_addr is the last recoverable error addr */
 	page = rec_addr >> PAGE_SHIFT;
@@ -82,32 +110,34 @@ static void count_rec(struct mem_ctl_info *mci, u8 rec_cnt, u32 rec_addr)
 }
 
 static void count_un_rec(struct mem_ctl_info *mci, u8 un_rec_cnt,
-			 u32 un_rec_addr)
+			 phys_addr_t un_rec_addr, bool have_addr)
 {
 	struct csrow_info *csrow = mci->csrows[0];
-	u32 page, offset, syndrome;
+	unsigned long page, offset, syndrome;
 
 	if (!un_rec_cnt)
 		return;
 
-	/* report 1. error */
-	/* note: un_rec_addr is the first unrecoverable error addr */
-	page = un_rec_addr >> PAGE_SHIFT;
-	offset = un_rec_addr & ~PAGE_MASK;
-	/* syndrome is not available */
-	syndrome = 0;
-	edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
-			     csrow->first_page + page, offset, syndrome,
-			     0, 0, -1, "", "");
+	/* report the first error with its address when one is available */
+	if (have_addr) {
+		/* note: un_rec_addr is the first unrecoverable error addr */
+		page = un_rec_addr >> PAGE_SHIFT;
+		offset = un_rec_addr & ~PAGE_MASK;
+		/* syndrome is not available */
+		syndrome = 0;
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, 1,
+				     csrow->first_page + page, offset, syndrome,
+				     0, 0, -1, "", "");
+	}
 
-	/* report further errors (if there are) */
-	/* note: no addresses are recorded */
-	if (un_rec_cnt > 1) {
+	/* report the remaining errors without a recorded address */
+	if (un_rec_cnt > 1 || !have_addr) {
 		/* page, offset and syndrome are not available */
 		page = 0;
 		offset = 0;
 		syndrome = 0;
-		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, un_rec_cnt-1,
+		edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
+				     have_addr ? un_rec_cnt - 1 : un_rec_cnt,
 				     page, offset, syndrome, 0, 0, -1,
 				     "address(es) not available", "");
 	}
@@ -166,8 +196,8 @@ static irqreturn_t aspeed_mcr_isr(int irq, void *arg)
 		rec_cnt, un_rec_cnt);
 
 	/* process recoverable and unrecoverable errors */
-	count_rec(mci, rec_cnt, rec_addr);
-	count_un_rec(mci, un_rec_cnt, un_rec_addr);
+	count_rec(mci, rec_cnt, rec_addr, true);
+	count_un_rec(mci, un_rec_cnt, un_rec_addr, true);
 
 	if (!rec_cnt && !un_rec_cnt)
 		dev_dbg_ratelimited(mci->pdev, "received edac interrupt, but did not find any ECC counters\n");
@@ -180,6 +210,54 @@ static irqreturn_t aspeed_mcr_isr(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t ast2700_dramc_isr(int irq, void *arg)
+{
+	u32 int_sts, ecc_sts, fail_addr;
+	struct mem_ctl_info *mci = arg;
+	struct aspeed_edac *priv;
+	u8 rec_cnt, un_rec_cnt;
+	phys_addr_t addr;
+
+	priv = mci->pvt_info;
+
+	scoped_guard(raw_spinlock, &priv->lock) {
+		int_sts = readl(priv->regs + AST2700_INT_STS);
+		if (!(int_sts & AST2700_INT_ECC))
+			return IRQ_NONE;
+
+		ecc_sts = readl(priv->regs + AST2700_ECC_STS);
+		fail_addr = readl(priv->regs + AST2700_ECC_FAIL_ADDR);
+
+		/* the interrupt registers are not key-protected; clear only ECC */
+		writel(int_sts & AST2700_INT_ECC, priv->regs + AST2700_INT_CLR);
+	}
+
+	rec_cnt = FIELD_GET(AST2700_ECC_REC_CNT, ecc_sts);
+	un_rec_cnt = FIELD_GET(AST2700_ECC_UNREC_CNT, ecc_sts);
+
+	/* the register holds address bits [35:4], in units of 16 bytes */
+	addr = (phys_addr_t)fail_addr << 4;
+
+	/*
+	 * The controller records only the address of the latest failure,
+	 * shared by both error types. When only one type occurred it owns
+	 * that address; when both occurred attribute it to the uncorrectable
+	 * error and report the corrected ones without an address.
+	 */
+	if (un_rec_cnt && !rec_cnt) {
+		count_un_rec(mci, un_rec_cnt, addr, true);
+	} else if (!un_rec_cnt && rec_cnt) {
+		count_rec(mci, rec_cnt, addr, true);
+	} else if (un_rec_cnt && rec_cnt) {
+		count_un_rec(mci, un_rec_cnt, addr, true);
+		count_rec(mci, rec_cnt, 0, false);
+	} else {
+		dev_dbg_ratelimited(mci->pdev, "received interrupt with no ECC counters set\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
 static void aspeed_set_irq(struct aspeed_edac *priv, bool enable)
 {
 	u32 val;
@@ -197,6 +275,19 @@ static void aspeed_set_irq(struct aspeed_edac *priv, bool enable)
 	aspeed_mcr_irq_update_exit(priv);
 }
 
+static void ast2700_set_irq(struct aspeed_edac *priv, bool enable)
+{
+	u32 val = AST2700_INT_ALL;
+
+	guard(raw_spinlock_irqsave)(&priv->lock);
+
+	/* interrupts are enabled by clearing their mask bits */
+	if (enable)
+		val &= ~AST2700_INT_ECC;
+
+	writel(val, priv->regs + AST2700_INT_MASK);
+}
+
 static int config_irq(struct mem_ctl_info *mci, struct platform_device *pdev)
 {
 	struct aspeed_edac *priv = mci->pvt_info;
@@ -209,7 +300,9 @@ static int config_irq(struct mem_ctl_info *mci, struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	rc = devm_request_irq(&pdev->dev, irq, aspeed_mcr_isr, IRQF_TRIGGER_HIGH,
+	priv->chip->set_irq(priv, false);
+
+	rc = devm_request_irq(&pdev->dev, irq, priv->chip->isr, IRQF_TRIGGER_HIGH,
 			      DRV_NAME, mci);
 	if (rc)
 		return rc;
@@ -217,7 +310,7 @@ static int config_irq(struct mem_ctl_info *mci, struct platform_device *pdev)
 	priv->irq = irq;
 
 	/* enable interrupts */
-	aspeed_set_irq(priv, true);
+	priv->chip->set_irq(priv, true);
 
 	return 0;
 }
@@ -365,7 +458,7 @@ static void aspeed_remove(struct platform_device *pdev)
 	struct aspeed_edac *priv = mci->pvt_info;
 
 	/* disable interrupts */
-	aspeed_set_irq(priv, false);
+	priv->chip->set_irq(priv, false);
 
 	devm_free_irq(&pdev->dev, priv->irq, mci);
 
@@ -382,6 +475,8 @@ static const struct aspeed_edac_chip ast2400_edac = {
 	.mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4,
 	.prot_reg = ASPEED_MCR_PROT,
 	.prot_key = ASPEED_MCR_PROT_PASSWD,
+	.isr = aspeed_mcr_isr,
+	.set_irq = aspeed_set_irq,
 };
 
 /* The AST2600 does not key-protect the interrupt control register (MCR50). */
@@ -391,12 +486,26 @@ static const struct aspeed_edac_chip ast2600_edac = {
 	.conf_dram_type = ASPEED_MCR_CONF_DRAM_TYPE,
 	.dram_type = { MEM_DDR3, MEM_DDR4 },
 	.mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR4,
+	.isr = aspeed_mcr_isr,
+	.set_irq = aspeed_set_irq,
+};
+
+/* The AST2700 interrupt registers are not key-protected either. */
+static const struct aspeed_edac_chip ast2700_edac = {
+	.conf_reg = AST2700_MCFG,
+	.conf_ecc = AST2700_MCFG_ECC,
+	.conf_dram_type = AST2700_MCFG_DRAM_TYPE,
+	.dram_type = { MEM_DDR4, MEM_DDR5 },
+	.mtype_cap = MEM_FLAG_DDR4 | MEM_FLAG_DDR5,
+	.isr = ast2700_dramc_isr,
+	.set_irq = ast2700_set_irq,
 };
 
 static const struct of_device_id aspeed_of_match[] = {
 	{ .compatible = "aspeed,ast2400-sdram-edac", .data = &ast2400_edac },
 	{ .compatible = "aspeed,ast2500-sdram-edac", .data = &ast2400_edac },
 	{ .compatible = "aspeed,ast2600-sdram-edac", .data = &ast2600_edac },
+	{ .compatible = "aspeed,ast2700-sdram-edac", .data = &ast2700_edac },
 	{},
 };
 

-- 
2.34.1

[PATCH v4 9/9] MAINTAINERS: Step down as Aspeed AST2500 EDAC driver maintainer

[Ryan Chen]

From: Stefan M Schaeckeler <sschaeck@cisco.com>

I am stepping down as a maintainer for drivers/edac/aspeed_edac.c and
Documentation/devicetree/bindings/edac/aspeed,ast2400-sdram-edac.yaml

Move my entry from the MAINTAINERS file to CREDITS to reflect my past
contributions.

[ ryan: keep the EDAC-AST2500 entry and list myself as its reviewer
  rather than removing it ]

Signed-off-by: Stefan M Schaeckeler <sschaeck@cisco.com>
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
Link: https://lore.kernel.org/r/20260920225846.4039017-1-sschaeck@cisco.com

---
Changes in v4:
- Merge Stefan's step-down patch into this one, keeping the MAINTAINERS
  entry alive with a reviewer instead of removing it.

Changes in v3:
- New patch.
---
 CREDITS     | 8 ++++++++
 MAINTAINERS | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/CREDITS b/CREDITS
index 091412875a66..28e5b274bc28 100644
--- a/CREDITS
+++ b/CREDITS
@@ -3655,6 +3655,14 @@ D: I2O kernel layer (config, block, core, pci, net). I2O disk support for LILO
 D: XScale (IOP, IXP) porting and other random ARM bits
 S: Portland, OR
 
+N: Stefan Schaeckeler
+E: sschaeck@cisco.com
+E: schaecsn@gmx.net
+D: Author and initial maintainer of the Aspeed AST2500 EDAC driver
+S: 2253 Anna Dr.
+S: Santa Clara, CA 95050
+S: USA
+
 N: Eric Schenk
 E: Eric.Schenk@dna.lth.se
 D: Random kernel debugging.
diff --git a/MAINTAINERS b/MAINTAINERS
index d413dac5c8b3..2001466fd6da 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9283,7 +9283,7 @@ F:	Documentation/devicetree/bindings/memory-controllers/marvell,mvebu-sdram-cont
 F:	drivers/edac/armada_xp_*
 
 EDAC-AST2500
-M:	Stefan Schaeckeler <sschaeck@cisco.com>
+R:	Ryan Chen <ryan_chen@aspeedtech.com>
 S:	Supported
 F:	Documentation/devicetree/bindings/edac/aspeed,ast2400-sdram-edac.yaml
 F:	drivers/edac/aspeed_edac.c

-- 
2.34.1