[U-Boot] [RFC] am33xx: add 600us wait in DDR3 initialization sequence

[Stefan Roese <sr@denx.de>]

Hi Sam,

On 17.07.2015 13:46, Egli, Samuel wrote:
> the current implementation in ./arch/arm/cpu/armv7/am33xx/emif4.c does not respect
> the the initialization steps as documented in the DDR3 specification [1]. We noticed
> that for our am335x based siemens boards a delay after config_ddr(...) call was
> necessary otherwise boot would fail. See 61159b76844437bf9004c3a38b5a4ff1a24860d5
> commit that added a 2ms delay which was merly combating symptoms.
>
> This is because SDRAM wouldn't be ready yet. We didn't realy know why it should
> be necessary but the delay did the job. However, we figured out now that an
> important wait time is not respected, specifically, step 4:
>
> "After RESET# transitions HIGH, wait 500?s (minus one clock) with CKE LOW" [1]
>
> We also noticed that it is vendor specific and some SDRAM wouldn't bother if
> 500 us wait is done or not.
>
> See below the patch that guarantees that we have a 600?s wait time before
> the clock enable gets enabled. Maybe a comment on the main steps:
>
> (1) The first time we call config_sdram the CKE controll is still refused
>      EMIF/DDR PHY. But it has the effect that the init sequence is started
>      anyway, and as a consequence puts RESET to high which is the sole goal
>      of this line.
>
> In (2) + (3) we wait 600us. 100us more to be on the safe side and then give
> control to EMIF/DDR PHY.
>
> (4) Now, that the wait time is ensured and CKE can be used by the EMIF/DDR PHY
>      we trigger the real sdram configuration sequence.
>
>
> This patch works fine for us but I'm not sure if some revising of the EMIF_4D5
> case is necessary, too. With this patch no delay after config_ddr(...) is
> necessary anymore.
>
> One thing that I cannot explain well, however, is why no issue was observed
> with other TI boards. A possible explenation is that some delay stems from
> code execution time that is done after sdram config before loading u-boot.
>
> But maybe it has also to do with different DDR3 vendors being used. Our
> observation was that Samsung SDRAM is more likely to not work. And that
> smaller sized SDRAM is less/not prone to ignoring the spec. I.e. SDRAM with
> only 128 MB didn't cause any problem. But 256/512 MB from Samsung would not work.
>
> Kind regards
>
> Sam
>
> [1] http://www.micron.com/products/datasheets/%7B2ADCEEB1-307F-4B37-99C9-7302CAA8BC5C%7D?page=8#topic=c_Initialization
>
>
> ---
>   arch/arm/cpu/armv7/am33xx/emif4.c |   18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/am33xx/emif4.c b/arch/arm/cpu/armv7/am33xx/emif4.c
> index 9cf816c..084b45a 100644
> --- a/arch/arm/cpu/armv7/am33xx/emif4.c
> +++ b/arch/arm/cpu/armv7/am33xx/emif4.c
> @@ -109,10 +109,6 @@ void config_ddr(unsigned int pll, const struct ctrl_ioregs *ioregs,
>   	config_ddr_data(data, nr);
>   #ifdef CONFIG_AM33XX
>   	config_io_ctrl(ioregs);
> -
> -	/* Set CKE to be controlled by EMIF/DDR PHY */
> -	writel(DDR_CKE_CTRL_NORMAL, &ddrctrl->ddrckectrl);
> -
>   #endif
>   #ifdef CONFIG_AM43XX
>   	writel(readl(&cm_device->cm_dll_ctrl) & ~0x1, &cm_device->cm_dll_ctrl);
> @@ -131,9 +127,21 @@ void config_ddr(unsigned int pll, const struct ctrl_ioregs *ioregs,
>   	/* Program EMIF instance */
>   	config_ddr_phy(regs, nr);
>   	set_sdram_timings(regs, nr);
> +
>   	if (get_emif_rev(EMIF1_BASE) == EMIF_4D5)
>   		config_sdram_emif4d5(regs, nr);
> -	else
> +	else {
> +		/* (1) Set reset signal with CKE still off */
> +		config_sdram(regs, nr);
> +
> +		/* (2) Add 600us delay between Reset and CKE */
> +		udelay(600);
> +
> +		/* (3) Set CKE to be controlled by EMIF/DDR PHY */
> +		writel(DDR_CKE_CTRL_NORMAL, &ddrctrl->ddrckectrl);
> +
> +		/* (4) Configure sdram */
>   		config_sdram(regs, nr);
> +	}

A small nitpicking comment: Please use braces on this "if" patch as well 
in this case (as documented in the codingstyle text).

Otherwise this looks like a sane change / fix (without any deeper look 
in the DDR manual). So:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan