[PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support

[PATCH v2 0/8] 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 2-5 are bug fixes and cleanups to the existing driver. Patches 6
and 7 rework the implementation to accommodate the AST2700. Patch 8 adds
the AST2700 support itself.

Signed-off-by: Ryan Chen <ryan_chen@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, so taking it with interrupts enabled would have tripped lockdep
  on the first reported error.
- 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):
      dt-bindings: edac: aspeed: Add AST2700 SDRAM EDAC
      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
      EDAC/aspeed: Replace regmap with direct register access
      EDAC/aspeed: Abstract SoC differences behind chip data
      EDAC/aspeed: Add AST2700 support

 .../bindings/edac/aspeed,ast2400-sdram-edac.yaml   |   6 +-
 drivers/edac/Makefile                              |   1 +
 drivers/edac/aspeed_edac.c                         | 426 ++++++++++++++-------
 3 files changed, 284 insertions(+), 149 deletions(-)
---
base-commit: 5464985e42c04e335fb30e38fbc409c997db9bec
change-id: 20260625-edac-8e960e02e7f7

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

[PATCH v2 2/8] EDAC/aspeed: Set the DIMM grain

[Ryan Chen]

The driver never sets dimm->grain, leaving it zero. Since commit
3724ace582d9 ("EDAC/mc: Fix grain_bits calculation")
edac_raw_mc_handle_error() runs WARN_ON_ONCE(!e->grain) and forces the
grain to 1, so the first ECC error reported on any Aspeed BMC SoC emits a
warning splat, e.g. on the AST2600:

  WARNING: CPU: 0 PID: 0 at drivers/edac/edac_mc.c:924 edac_raw_mc_handle_error+0x4b4/0x604
  ...
  edac_raw_mc_handle_error from edac_mc_handle_error+0x364/0x4a8
  edac_mc_handle_error from count_rec+0xdc/0x124
  count_rec from mcr_isr+0x110/0x1e8

Fixes: 9b7e6242ee4e ("EDAC, aspeed: Add an Aspeed AST2500 EDAC driver")
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
Cc: stable@vger.kernel.org
---
 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 v2 3/8] 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>
Cc: stable@vger.kernel.org

---
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 v2 0/8] Add Aspeed AST2700 SDRAM EDAC support

[Borislav Petkov]

On Mon, Aug 24, 2026 at 10:42:27AM +0800, Ryan Chen wrote:
> 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 2-5 are bug fixes and cleanups to the existing driver. Patches 6
> and 7 rework the implementation to accommodate the AST2700. Patch 8 adds
> the AST2700 support itself.

Sashiko has comments:

https://sashiko.dev/#/patchset/20260824-edac-v2-0-c8d8bb693586%40aspeedtech.com

Please address them: if valid, fix them, if not, explain why they don't need
to be fixed.

Btw, I don't see Stefan reviewing this driver and his last mail on LKML is
from 2025 so it looks like we'll orphan this driver too.

Thx.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support

[Ryan Chen]

> Subject: Re: [PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support
> 
> On Mon, Aug 24, 2026 at 10:42:27AM +0800, Ryan Chen wrote:
> > 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 2-5 are bug fixes and cleanups to the existing driver. Patches
> > 6 and 7 rework the implementation to accommodate the AST2700. Patch 8
> > adds the AST2700 support itself.
> 
> Sashiko has comments:
> 
> https://sashiko.dev/#/patchset/20260824-edac-v2-0-c8d8bb693586%40aspeed
> tech.com
> 
> Please address them: if valid, fix them, if not, explain why they don't need to
> be fixed.
> 
> Btw, I don't see Stefan reviewing this driver and his last mail on LKML is from
> 2025 so it looks like we'll orphan this driver too.
> 
> Thx.
> 
> --
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Hello Boris,
	Thanks your review.

I've already replied to both of Sashiko's findings on their respective
threads:
6/8: https://sashiko.dev/#/message/TY2PPF5CB9A1BE61E82EEE55F3F725362CCF2AD2%40TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com
8/8: https://sashiko.dev/#/message/TY2PPF5CB9A1BE62172BCC358347C4836CCF2AD2%40TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com

Short version: neither needs a code change. On 8/8 the premise doesn't
hold - u-boot masks every implemented DRAMC interrupt source
(INT_MASK reads GENMASK(16, 0), bits 31:17 are not implemented), so no
non-ECC source can raise the line and the spurious detector cannot
kick in. On 6/8 no goto crosses the scoped_guard(), so the hazard
cleanup.h describes cannot occur. Details are in the replies.

On the maintainership: I'd be happy to take EDAC-AST2500 over if
Stefan no longer has the time. I work on these SoCs at ASPEED, so I
have the hardware and documentation for all four variants and I'm
actively extending the driver.

Re: [PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support

[Andrew Jeffery]

On Tue, 2026-09-08 at 07:13 +0000, Ryan Chen wrote:
> 
> > Subject: Re: [PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support
> > 
> > On Mon, Aug 24, 2026 at 10:42:27AM +0800, Ryan Chen wrote:
> > > 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 2-5 are bug fixes and cleanups to the existing driver. Patches
> > > 6 and 7 rework the implementation to accommodate the AST2700. Patch 8
> > > adds the AST2700 support itself.
> > 
> > Sashiko has comments:
> > 
> > https://sashiko.dev/#/patchset/20260824-edac-v2-0-c8d8bb693586%40aspeed
> > tech.com
> > 
> > Please address them: if valid, fix them, if not, explain why they don't need to
> > be fixed.
> > 
> > Btw, I don't see Stefan reviewing this driver and his last mail on LKML is from
> > 2025 so it looks like we'll orphan this driver too.
> > 
> > Thx.
> > 
> > --
> > Regards/Gruss,
> >     Boris.
> > 
> > https://people.kernel.org/tglx/notes-about-netiquette
> 
> Hello Boris,
> 	Thanks your review.
> 
> I've already replied to both of Sashiko's findings on their respective
> threads:
> 6/8: https://sashiko.dev/#/message/TY2PPF5CB9A1BE61E82EEE55F3F725362CCF2AD2%40TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com
> 8/8: https://sashiko.dev/#/message/TY2PPF5CB9A1BE62172BCC358347C4836CCF2AD2%40TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com
> 
> Short version: neither needs a code change. On 8/8 the premise doesn't
> hold - u-boot masks every implemented DRAMC interrupt source
> (INT_MASK reads GENMASK(16, 0), bits 31:17 are not implemented), so no
> non-ECC source can raise the line and the spurious detector cannot
> kick in. 

I feel this is a fragile analysis given multiple components involved. I
think it would be easier and safer (both in terms of kernel behaviour
and guarding against change in u-boot behaviour) if we have the kernel
driver take complete ownership of the interrupt configuration.

Andrew

RE: [PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support

[Ryan Chen]

> Subject: Re: [PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support
> 
> On Tue, 2026-09-08 at 07:13 +0000, Ryan Chen wrote:
> >
> > > Subject: Re: [PATCH v2 0/8] Add Aspeed AST2700 SDRAM EDAC support
> > >
> > > On Mon, Aug 24, 2026 at 10:42:27AM +0800, Ryan Chen wrote:
> > > > 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 2-5 are bug fixes and cleanups to the existing driver.
> > > > Patches
> > > > 6 and 7 rework the implementation to accommodate the AST2700.
> > > > Patch 8 adds the AST2700 support itself.
> > >
> > > Sashiko has comments:
> > >
> > > https://sashiko.dev/#/patchset/20260824-edac-v2-0-c8d8bb693586%40asp
> > > eed
> > > tech.com
> > >
> > > Please address them: if valid, fix them, if not, explain why they
> > > don't need to be fixed.
> > >
> > > Btw, I don't see Stefan reviewing this driver and his last mail on
> > > LKML is from
> > > 2025 so it looks like we'll orphan this driver too.
> > >
> > > Thx.
> > >
> > > --
> > > Regards/Gruss,
> > >     Boris.
> > >
> > > https://people.kernel.org/tglx/notes-about-netiquette
> >
> > Hello Boris,
> > 	Thanks your review.
> >
> > I've already replied to both of Sashiko's findings on their respective
> > threads:
> > 6/8:
> >
> https://sashiko.dev/#/message/TY2PPF5CB9A1BE61E82EEE55F3F725362CCF2A
> D2
> > %40TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com
> > 8/8:
> >
> https://sashiko.dev/#/message/TY2PPF5CB9A1BE62172BCC358347C4836CCF2
> AD2
> > %40TY2PPF5CB9A1BE6.apcprd06.prod.outlook.com
> >
> > Short version: neither needs a code change. On 8/8 the premise doesn't
> > hold - u-boot masks every implemented DRAMC interrupt source (INT_MASK
> > reads GENMASK(16, 0), bits 31:17 are not implemented), so no non-ECC
> > source can raise the line and the spurious detector cannot kick in.
> 
> I feel this is a fragile analysis given multiple components involved. I think it
> would be easier and safer (both in terms of kernel behaviour and guarding
> against change in u-boot behaviour) if we have the kernel driver take complete
> ownership of the interrupt configuration.
> 
Hello Andrew.
Thanks your review. I will modify with following.

#define   AST2700_INT_ALL		GENMASK(16, 0)

static void ast2700_set_irq(struct aspeed_edac *priv, bool enable)
{
+	u32 val = AST2700_INT_ALL;

	guard(raw_spinlock_irqsave)(&priv->lock);
	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)
{
...
+	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;

	priv->irq = irq;

	/* enable interrupts */
	priv->chip->set_irq(priv, true);

	return 0;
}