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

[Paul Louvel]

Hi,

On Fri Sep 25, 2026 at 2:02 PM CEST, Wolfram Sang wrote:
> Hi Paul,
>
> thanks for your patches!
>
> On Thu, Sep 24, 2026 at 04:12:48PM +0200, Paul Louvel wrote:
>> 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@xxxxxxxxxxx>
>
> Disclaimer: I don't know the technology nor the subsystem. So, only some
> high level comments.
>
>> +
>> +static void cdns_rmw(struct cdns_mc_priv *priv, u32 reg, u32 mask, u32 val)
>> +{
>> +	u32 regval;
>> +
>> +	mutex_lock(&priv->lock);
>> +	regval = readl(priv->io_base + reg);
>> +	FIELD_MODIFY(mask, &regval, val);
>> +	writel(regval, priv->io_base + reg);
>> +	mutex_unlock(&priv->lock);
>
> Hmm, a spinlock is probably more suitable for such short operations? You
> could even save a lock here and use a generic mutex in
> inject_ctrl_store() for the whole operation. That would work for now. In
> terms of defensive programming, a spinlock could be argued, too, to make
> future additions more robust.

Yes, I wanted to use the lock inside the function so that in the future, if it
is used, the caller won't have to bother locking anything.
Noted for the spinlock, that seems more appropriate indeed.

>
>> +static void cdns_mc_err_inject(struct mem_ctl_info *mci, u16 synd)
>> +{
>> +	struct cdns_mc_priv *priv = mci->pvt_info;
>> +
>> +	cdns_rmw(priv, CDNS_DDR_ECC_XOR, CDNS_DDR_ECC_XOR_CHECK_BITS, synd);
>> +	cdns_rmw(priv, CDNS_DDR_ECC_STAT, CDNS_DDR_ECC_STAT_FWC, 1);
>> +}
>
> Bike shedding: I think this is too short for a seperate function and it
> should be folded into its caller.

ACK.

>
>> +
>> +static ssize_t inject_ctrl_store(struct device *dev, struct device_attribute *attr, const char *buf,
>> +				 size_t count)
>> +{
>> +	struct mem_ctl_info *mci = to_mci(dev);
>> +	u16 synd;
>> +
>> +	if (kstrtou16(buf, 16, &synd))
>> +		return -EINVAL;
>> +
>> +	cdns_mc_err_inject(mci, synd);
>> +
>> +	return count;
>> +}
>> +
>> +static DEVICE_ATTR_WO(inject_ctrl);
>> +
>> +static struct attribute *cdns_edac_attrs[] = { &dev_attr_inject_ctrl.attr, NULL };
>> +
>> +ATTRIBUTE_GROUPS(cdns_edac);
>
> What about using debugfs instead via edac_debugfs_create_*?

Seems to be more appropriate since inducing a CE or UE would only be for
debugging purposes.

>
> Happy hacking,
>
>    Wolfram


Thanks,



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