Re: AM335x DDR3 initialization / timing violation

[Sascha Hauer <sha@pengutronix.de>]

On Thu, Dec 15, 2022 at 10:54:00AM +0100, Matthias Feser wrote:
> Hi, 
> 
> we are using the AM3352 processor in combination with a single 512MB
> Micron DDR3 chip running the barebox bootloader in our products for
> several years now. A minor amount of boards (around 5 of 2000) fail
> the production test, because they do not boot properly after warm
> reset. In such cases the MLO is loaded, initializes the EMIF and then
> crashes after a certain amount of accesses to the DDR3. After a cold
> reset all of these boards run stable and produce no errors when
> running a deep RAM test. 
> 
> I am currently in discussion with a TI employee about this topic. He
> told me that the bootloader should detect a warm reset and EMIF should
> not be reinitialized, because DDR3 is automatically put into
> self-refresh on warm reset. So far he hasn’t told me what the
> desired init sequence actually is. From what I have observed while
> debugging, at least the EMIF clock has to be enabled and CKE brought
> high. Unfortunately the TRM does not give guidance about this.
> 
> Our board initialization code is very similar to other AM335x based
> boards like beaglebone (400MHz DDR clock), which effectively always
> initializes the EMIF in the same way by calling am335x_sdram_init(),
> no matter if cold or warm reset has brought up the processor. From
> investigating the signals DDR_RESET and DDR_CKE with an oscilloscope,
> I can tell that even with this same init code the hardware behaves
> differently in both reset cases.
> 
> On cold reset both DDR_RESET and DDR_CKE remain low until
> initialization, and there is a delay of roughly 436us between the
> rising edges of DDR_RESET and DDR_CKE. After warm reset DDR_RESET is
> high and DDR_CKE is low. EMIF initialization results in a short pulse
> on DDR_RESET with 5us low phase and there is only about 38us delay
> between the rising edges of DDR_RESET and DDR_CKE.
> Both cases violate the DDR3 specification, according to which the
> delay between the rising edges of DDR_RESET and DDR_CKE has to be
> 500us min.
> 
> In am33xx_config_sdram() a value of 0x2800 is written to
> EMIF4_SDRAM_REF_CTRL. TI recommends a value of 0x3100 during
> initialization, which is used in u-boot EMIF initialization code and
> does not violate the DDR3 specification. I think barebox EMIF init
> code requires some revision.
> 
> I also wonder why EMIF4_SDRAM_CONFIG, REF_CTRL and REF_CTRL_SHADOW are
> accessed twice when regs->zq_config is not zero (see code snippet
> below taken from barebox 2022.12.0). Is there any reason behind this?

The code is from U-Boot.

> 
>                 if (regs->zq_config) {
>                                /*
>                                * A value of 0x2800 for the REF CTRL will give us
>                                * about 570us for a delay, which will be long enough
>                                * to configure things.
>                                */
>                                writel(0x2800, emif4 + EMIF4_SDRAM_REF_CTRL);
>                                writel(regs->zq_config, emif4 + EMIF4_ZQ_CONFIG);
>                                writel(regs->sdram_config, CM_EMIF_SDRAM_CONFIG);
>                                writel(regs->sdram_config, emif4 + EMIF4_SDRAM_CONFIG);
>                                writel(regs->sdram_ref_ctrl, emif4 + EMIF4_SDRAM_REF_CTRL);
>                                writel(regs->sdram_ref_ctrl, emif4 + EMIF4_SDRAM_REF_CTRL_SHADOW);
>                 }
> 
>                 writel(regs->sdram_ref_ctrl, emif4 + EMIF4_SDRAM_REF_CTRL);
>                 writel(regs->sdram_ref_ctrl, emif4 + EMIF4_SDRAM_REF_CTRL_SHADOW);
>                 writel(regs->sdram_config, emif4 + EMIF4_SDRAM_CONFIG);
> 

The second access to EMIF4_SDRAM_REF_CTRL goes down to U-Boot commit
1c382ead7a00 ("am33xx: Update DDR3 EMIF configuration sequence"). This
commit also introduces the 0x2800 value we still find in barebox.

The 0x2800 value was removed in fc46bae2ae38 ("arm: am437x: Enable
hardware leveling for EMIF"). In this commit we also see the value
0x3100 for the first time.

I can't say much more to this, except that I am happy to accept patches
to clean that up.

Sascha

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