Re: [PATCH v2 3/4] drivers: ddr: imx8m: ddr_init.c: support ECC scrubbing

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hi,

On 3/9/26 8:46 AM, Steffen Trumtrar wrote:
> From: David Jander <david@protonic.nl>
> 
> This code comes from u-boot [1] and was introducesd in commit [2].

introduced*

> A fix from the patch [3] is also included, which doesn't seem to be
> added to u-boot, yet.
> 
> [1] https://github.com/u-boot/u-boot/
> [2] commit f3acb02386f4 ("drivers: ddr: imx8mp: Add inline ECC feature support")
> [3] https://patchwork.ozlabs.org/project/uboot/patch/20230123091702.7472-32-peng.fan@oss.nxp.com/

Interesting. Appears to still be unmerged.

> +#ifdef CONFIG_IMX8MP_DRAM_ECC

Remove the #ifdef here and use IS_ENABLED() below.

> +static void ddrc_inline_ecc_scrub(unsigned int start_address,
> +				  unsigned int range_address)
> +{
> +	unsigned int tmp;
> +
> +	pr_debug("ECC scrub %08x-%08x\n", start_address, range_address);
> +	/* Step1: Enable quasi-dynamic programming */
> +	reg32_write(DDRC_SWCTL(0), 0x00000000);
> +	/* Step2: Set ECCCFG1.ecc_parity_region_lock to 1 */
> +	reg32setbit(DDRC_ECCCFG1(0), 0x4);
> +	/* Step3: Block the AXI ports from taking the transaction */
> +	reg32_write(DDRC_PCTRL_0(0), 0x0);
> +	/* Step4: Set scrub start address */
> +	reg32_write(DDRC_SBRSTART0(0), start_address);
> +	/* Step5: Set scrub range address */
> +	reg32_write(DDRC_SBRRANGE0(0), range_address);
> +	/* Step6: Set scrub_mode to write */
> +	reg32_write(DDRC_SBRCTL(0), 0x00000014);
> +	/* Step7: Set the desired pattern through SBRWDATA0 registers */
> +	reg32_write(DDRC_SBRWDATA0(0), 0x55aa55aa);
> +	/* Step8: Enable the SBR by programming SBRCTL.scrub_en=1 */
> +	reg32setbit(DDRC_SBRCTL(0), 0x0);
> +	/* Step9: Poll SBRSTAT.scrub_done=1 */
> +	tmp = reg32_read(DDRC_SBRSTAT(0));
> +	while (tmp != 0x00000002)
> +		tmp = reg32_read(DDRC_SBRSTAT(0)) & 0x2;
> +	/* Step10: Poll SBRSTAT.scrub_busy=0 */
> +	tmp = reg32_read(DDRC_SBRSTAT(0));
> +	while (tmp != 0x0)
> +		tmp = reg32_read(DDRC_SBRSTAT(0)) & 0x1;
> +	/* Step11: Disable SBR by programming SBRCTL.scrub_en=0 */
> +	clrbits_le32(DDRC_SBRCTL(0), 0x1);
> +	/* Step12: Prepare for normal scrub operation(Read) and set scrub_interval*/
> +	reg32_write(DDRC_SBRCTL(0), 0xff20);
> +	/* Step13: Enable the SBR by programming SBRCTL.scrub_en=1 */
> +	reg32_write(DDRC_SBRCTL(0), 0xff21);
> +	/* Step14: Enable AXI ports by programming */
> +	reg32_write(DDRC_PCTRL_0(0), 0x1);
> +	/* Step15: Disable quasi-dynamic programming */
> +	reg32_write(DDRC_SWCTL(0), 0x00000001);
> +}
> +
> +static void ddrc_inline_ecc_scrub_end(unsigned int start_address,
> +				      unsigned int range_address)
> +{
> +	pr_debug("ECC  end %08x-%08x\n", start_address, range_address);
> +	/* Step1: Enable quasi-dynamic programming */
> +	reg32_write(DDRC_SWCTL(0), 0x00000000);
> +	/* Step2: Block the AXI ports from taking the transaction */
> +	reg32_write(DDRC_PCTRL_0(0), 0x0);
> +	/* Step3: Set scrub start address */
> +	reg32_write(DDRC_SBRSTART0(0), start_address);
> +	/* Step4: Set scrub range address */
> +	reg32_write(DDRC_SBRRANGE0(0), range_address);
> +	/* Step5: Disable SBR by programming SBRCTL.scrub_en=0 */
> +	clrbits_le32(DDRC_SBRCTL(0), 0x1);
> +	/* Step6: Prepare for normal scrub operation(Read) and set scrub_interval */
> +	reg32_write(DDRC_SBRCTL(0), 0x100);
> +	/* Step7: Enable the SBR by programming SBRCTL.scrub_en=1 */
> +	reg32_write(DDRC_SBRCTL(0), 0x101);
> +	/* Step8: Enable AXI ports by programming */
> +	reg32_write(DDRC_PCTRL_0(0), 0x1);
> +	/* Step9: Disable quasi-dynamic programming */
> +	reg32_write(DDRC_SWCTL(0), 0x00000001);
> +}
> +
> +static void dram_ecc_scrub(struct dram_timing_info *dram_timing)
> +{
> +	/* memory range to scrub in words (=1.75G) */
> +	unsigned int range_in_words = 0x1c000000;

For clarity, maybe define some macros? Something like

ECC_STRIDE	SZ_2G
ECC_INLINE_SIZE	...
ECC_SCRUB_CHUNK ECC_STRIDE - ECC_INLINE_SIZE

and use that instead of the hex constants?

> +	/* start scrubbing at RAM address */
> +	unsigned int start_address;
> +	/* scrub up until this address */
> +	unsigned int range_address;
> +
> +	BUG_ON(!dram_timing->size);

Wouldn't this panic on all non-ECC board entry points? Mixing boards
with inline ECC and without should continue to work.

See also below for naming the member for clarity.

> +
> +	for (start_address = 0x0;
> +	     start_address + range_in_words - 1 <= dram_timing->size / 4;
> +	     start_address += range_in_words + 0x04000000) {
> +		range_address = start_address + range_in_words - 1;
> +		/* scrub in 1.75G chunk sizes */
> +		ddrc_inline_ecc_scrub(start_address, range_address);
> +	}
> +	
> +	ddrc_inline_ecc_scrub_end(0x0, dram_timing->size / 4 - 1);
> +}
> +#endif
> +
>  static unsigned int g_cdd_rr_max[4];
>  static unsigned int g_cdd_rw_max[4];
>  static unsigned int g_cdd_wr_max[4];
> @@ -642,6 +732,11 @@ int imx8m_ddr_init(struct dram_controller *dram, struct dram_timing_info *dram_t
>  	reg32_write(DDRC_PCTRL_0(0), 0x00000001);
>  	pr_debug("ddrmix config done\n");
>  
> +#ifdef CONFIG_IMX8MP_DRAM_ECC
> +	if (dram->ddrc_type == DDRC_TYPE_MP)

if (IS_ENABLED(CONFIG_IMX8MP_DRAM_ECC) && dram->ddrc_type == DDRC_TYPE_MP)

> +		dram_ecc_scrub(dram_timing);
> +#endif
> +
>  	/* save the dram timing config into memory */
>  	dram_config_save(dram, dram_timing, IMX8M_SAVED_DRAM_TIMING_BASE);
>  
> diff --git a/include/soc/imx/ddr.h b/include/soc/imx/ddr.h
> index 6426062900..9a5d341100 100644
> --- a/include/soc/imx/ddr.h
> +++ b/include/soc/imx/ddr.h
> @@ -101,6 +101,7 @@ struct dram_timing_info {
>  		unsigned int ddrphy_pie_num;
>  		/* initialized drate table */
>  		unsigned int fsp_table[4];
> +		resource_size_t size;

The member name should either have ecc in it or there should be an extra
member that enables ecc, e.g.:

dram_timing::ecc_full_size

or

dram_timing::size
dram_timing::enable_ecc

or something along these lines.

>  	);
>  };
>  
> diff --git a/include/soc/imx8m/ddr.h b/include/soc/imx8m/ddr.h
> index 5df07772b3..f07cef9a9e 100644
> --- a/include/soc/imx8m/ddr.h
> +++ b/include/soc/imx8m/ddr.h
> @@ -186,6 +186,8 @@
>  #define DDRC_SBRWDATA0(X)        (DDRC_IPS_BASE_ADDR(X) + 0xf2c)
>  #define DDRC_SBRWDATA1(X)        (DDRC_IPS_BASE_ADDR(X) + 0xf30)
>  #define DDRC_PDCH(X)             (DDRC_IPS_BASE_ADDR(X) + 0xf34)
> +#define DDRC_SBRSTART0(X)        (DDRC_IPS_BASE_ADDR(X) + 0xf38)
> +#define DDRC_SBRRANGE0(X)        (DDRC_IPS_BASE_ADDR(X) + 0xf40)
>  
>  #define DDRC_FREQ1_DERATEEN(X)         (DDRC_IPS_BASE_ADDR(X) + 0x2020)
>  #define DDRC_FREQ1_DERATEINT(X)        (DDRC_IPS_BASE_ADDR(X) + 0x2024)
> @@ -380,4 +382,6 @@ static inline void imx8m_ddr_load_train_code(enum dram_type dram_type,
>  	ddr_load_train_code(&imx8m_dram_controller, dram_type, fw_type);
>  }
>  
> +#define DDRC_PHY_REG(x)	((x) * 4)

Unused?

> +
>  #endif
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |