Re: [PATCH v9 2/2] EDAC/versal: Add a Xilinx Versal memory controller driver

[Michael Ellerman <mpe@ellerman.id.au>]

Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> writes:
> Add a EDAC driver for the RAS capabilities on the Xilinx integrated DDR
> Memory Controllers (DDRMCs) which support both DDR4 and LPDDR4/4X memory
> interfaces. It has four programmable NoC interface ports and is designed
> to handle multiple streams of traffic.The driver
> reports correctable and uncorrectable errors , and also creates
> debugfs entries for error injection.
...
> diff --git a/drivers/edac/versal_edac.c b/drivers/edac/versal_edac.c
> new file mode 100644
> index 000000000000..5ce2e9585a00
> --- /dev/null
> +++ b/drivers/edac/versal_edac.c
...
> +#define XDDR_REG_CONFIG0_NUM_CHANS_MASK		BIT(17)
...
> +static int mc_probe(struct platform_device *pdev)
> +{
> +	void __iomem *ddrmc_baseaddr, *ddrmc_noc_baseaddr;
> +	struct edac_mc_layer layers[2];
> +	struct mem_ctl_info *mci;
> +	u8 num_chans, num_csrows;
> +	struct edac_priv *priv;
> +	u32 edac_mc_id, regval;
> +	int rc;
> +
...
> +
> +	regval = readl(ddrmc_baseaddr + XDDR_REG_CONFIG0_OFFSET);
> +	num_chans = FIELD_PREP(XDDR_REG_CONFIG0_NUM_CHANS_MASK, regval);
 
Shouldn't this be FIELD_GET?

Otherwise it's shifting regval into BIT(17), and then assigning that to a u8.

> +	num_chans++;
> +
> +	num_csrows = FIELD_PREP(XDDR_REG_CONFIG0_NUM_RANKS_MASK, regval);
 
And here too?

> +	num_csrows *= 2;
> +	if (!num_csrows)
> +		num_csrows = 1;

cheers