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

[David Jander <david@protonic.nl>]

On Wed, 4 Mar 2026 13:33:42 +0100
Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

> Hello David,
> 
> On 3/4/26 1:23 PM, David Jander wrote:
> > On Wed, 4 Mar 2026 12:43:55 +0100
> > Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:  
> >>> +config IMX8MP_DRAM_ECC
> >>> +	bool "Enable LPDDR4 ECC feature on i.MX8MP boards"
> >>> +	depends on ARCH_IMX8MP
> >>> +	help
> >>> +	  The i.MX8MP SoC supports ECC on the LPDDR4 memory. Select Y to enable
> >>> +	  this feature. The total amount of memory available will be reduced by
> >>> +	  1/8th.    
> >>
> >> A generic option is too confusing when it's only affecting a single board.  
> > 
> > Why should it affect only a single board? This is a general feature of this
> > SoC and it can be enabled for any other board that uses this SoC with LPDDR4
> > chips.  
> 
> Sure and a user would expect that enabling this option would turn on
> inline ECC, but it currently does so only for the Protonic board.
> The i.MX8MP-EVK won't magically get ECC support with this enabled and I
> think it may even fail to compile as it doesn't provide
> board_dram_ecc_scrub on its own, so having this option in this form is
> IMO confusing to users.
> 
> My suggestion below is thus to make ECC use a per-board (entry point)
> decision instead of a build-wide decision.

Oh, sorry for the missing context on my side. Ideally the ecc scrubbing
function should be made generic, so that all boards can use it. There is
already enough code duplication going on IMHO... why add even more?
Or, likewise, if you add more code to one copy of lpddr4-timing.c, shouldn't
it be added to all of them?

> > Even more so, I suspect this feature is present on some other SoCs as well
> > which use the same Synopsys DesignWare IP with this feature enabled. In that
> > case you'd probably want to make a more generic dram driver though. I suspect
> > chips like the Rockchip RK3588 or RK3576 might support this also, but I
> > haven't tried, and since those chips have binary dram init blobs this is
> > probably not easy to figure out.  
> 
> Interesting. While I have seen ECC documented for RK3568, I thought it
> was not available for the RK3588.

Hmm... yes the RK3568 probably also supports it. They all have the same
Synopsys DesignWare IP with different configurations. The 88 and 76 have a
newer version that adds LPDDR5 and drops (LP)DDR3 support.
Not sure, but I suspect i.MX935 and i.MX95 might use the same.

> By the way, Daniel (Cc'd) has been working on reverse engineering the
> Rockchip DRAM init blob, so maybe that's moving within reach :)

This is pretty cool!
Keep me posted with news about this. I have a lot of questions about this blob
with regards to hardware design details. I don't think I'll get answers from
Rockchip besides the usual "copy our reference design", but this effort might
help a lot.

Best regards,

> > 
> > Best regards,
> >   
> >> Alternative suggestion:
> >>
> >>   - make IMX8MP_DRAM_ECC a hidden symbol
> >>   - select it from the protonic board
> >>   - Turn the RAM timings of the protonic board into a header and
> >>     replace #ifdef CONFIG_IMX8MP_DRAM_ECC with #ifdef USE_ECC
> >>   - Add two source code files, that include the new header once with
> >>     USE_ECC supported and once without
> >>   - Add two entry points which only differ in what RAM timings are used
> >>
> >> Also worth considering: adding the size of the region to be scrubbed or
> >> the function doing the scrubbing into dram_timing_info.
> >>
> >> Thoughts?
> >>
> >> Cheers,
> >> Ahmad
> >>  
> >>> +
> >>>  endmenu
> >>>  
> >>>  endif
> >>> diff --git a/drivers/ddr/imx/imx8m_ddr_init.c b/drivers/ddr/imx/imx8m_ddr_init.c
> >>> index c16e04d274..6398a2b971 100644
> >>> --- a/drivers/ddr/imx/imx8m_ddr_init.c
> >>> +++ b/drivers/ddr/imx/imx8m_ddr_init.c
> >>> @@ -45,6 +45,74 @@ static void ddr_cfg_umctl2(struct dram_controller *dram, struct dram_cfg_param *
> >>>  	}
> >>>  }
> >>>  
> >>> +#ifdef CONFIG_IMX8MP_DRAM_ECC
> >>> +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);
> >>> +}
> >>> +
> >>> +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);
> >>> +}
> >>> +#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 +710,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)
> >>> +		board_dram_ecc_scrub();
> >>> +#endif
> >>> +
> >>>  	/* save the dram timing config into memory */
> >>>  	dram_config_save(dram, dram_timing, IMX8M_SAVED_DRAM_TIMING_BASE);
> >>>  
> >>> diff --git a/include/soc/imx8m/ddr.h b/include/soc/imx8m/ddr.h
> >>> index 5df07772b3..08ebf61da0 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,13 @@ 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)
> >>> +
> >>> +void board_dram_ecc_scrub(void);
> >>> +
> >>> +void ddrc_inline_ecc_scrub(unsigned int start_address,
> >>> +			   unsigned int range_address);
> >>> +void ddrc_inline_ecc_scrub_end(unsigned int start_address,
> >>> +			       unsigned int range_address);
> >>> +
> >>>  #endif
> >>>     
> >>  
> > 
> > 
> >   
> 



-- 
David Jander
Protonic Holland.
tel.: +31 (0) 229 212928
De Factorij 36 / 1689 AL Zwaag