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;
}