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

[Tom Rini]

On Tue, Jul 28, 2015 at 02:31:42PM +0000, Egli, Samuel wrote:

> Hi all,
> me again. I was wondering, if somebody of you has time to check these 
> changes? I would appreciate some feedback. Ultimately, it should
> also go upstream but my focus here is reviewing the content of
> these changes.

Did you have a chance to look into the thing I mentioned in the other
thread, about switching to arch/arm/cpu/armv7/arch_timer.c for am33xx?
And wrt the EMIF changes, I'm hoping James can chime in since he knows
that block inside and out :)

> 
> Thanks,
> 
> Sam
> 
> > -----Original Message-----
> > From: Egli, Samuel
> > Sent: Freitag, 17. Juli 2015 13:47
> > To: trini at konsulko.com; doublesin at ti.com; balbi at ti.com
> > Cc: u-boot at lists.denx.de; hs at denx.de; Stefan Roese; Meier, Roger; Senn,
> > Joerg
> > Subject: [RFC] am33xx: add 600us wait in DDR3 initialization sequence
> > 
> > Hi all,
> > 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);
> > +	}
> >  }
> >  #endif
> > --
> > 1.7.10.4

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150728/c4e646d0/attachment.sig>