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

[Wolfram Sang]

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.

> +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.

> +
> +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_*?

Happy hacking,

   Wolfram

Attachment: signature.asc
Description: PGP signature