| Message ID | 27E9275BC1C8554E840881B19B62BE421B3C2D@DENBGAT9EI1MSX.ww902.siemens.net |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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
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. Thanks, Sam > -----Original Message----- > From: Egli, Samuel > Sent: Freitag, 17. Juli 2015 13:47 > To: trini@konsulko.com; doublesin@ti.com; balbi@ti.com > Cc: u-boot@lists.denx.de; hs@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
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@konsulko.com; doublesin@ti.com; balbi@ti.com > > Cc: u-boot@lists.denx.de; hs@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
Hi Samuel, I don't think these changes are necessary. I think your concern about the initialization steps is not correct. The first setting of the REF_CTRL (before SDRAM_CONFIG write) will be used by the EMIF controller to determine the delay between the rising edge of DDR_RESETn and DDR_CKE. We typically write a value of 0x3100 in REF_CTRL to achieve this, which comes from this formula: (16*SDRAM_REF_CTRL)/400MHz > 500us This value will change for different frequencies, but for AM335x and AM437x, the max freq is 400MHz. When SDRAM_CONFIG is written, the EMIF controller will kick off a H/W sequence which will configure the memory, and will use this setting for the proper RESET to CKE timing. Once that is finished, another write to REF_CTRL to set the proper refresh rate will be necessary. A separate issue which we have seen does require a delay, however. We have seen some timeout errors reported on the L3 bus as a result of this EMIF sequence. This error does not result in any functional problem with the EMIF initialization, but does result in one of the L3 error flags being set. We have determined that this is a result of an EMIF access immediately following the SDRAM_CONFIG write. The EMIF kicks back an error while the H/W initialization sequence is still active, thus a delay is required to eliminate this bus error. Again, functionally the EMIF sequence will work without this delay, but to avoid the error flag, the delay is needed. So to summarize, the sequence should be: -Configure all other EMIF registers -Write SDRAM_REF_CTRL=0x3100 -Write SDRAM_CONFIG with appropriate value to kick off H/W initialization of the DDR -wait 1ms (to avoid L3 timeout error) -Write SDRAM_REF_CTRL with refresh rate value for your DDR Regards , James Doublesin -----Original Message----- From: Egli, Samuel [mailto:samuel.egli@siemens.com] Sent: Tuesday, July 28, 2015 9:32 AM To: 'trini@konsulko.com'; Doublesin, James; Balbi, Felipe Cc: 'u-boot@lists.denx.de'; 'hs@denx.de'; 'Stefan Roese'; Meier, Roger; Senn, Joerg; Belogolov, Oleg Subject: [RFC] am33xx: add 600us wait in DDR3 initialization sequence 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. Thanks, Sam > -----Original Message----- > From: Egli, Samuel > Sent: Freitag, 17. Juli 2015 13:47 > To: trini@konsulko.com; doublesin@ti.com; balbi@ti.com > Cc: u-boot@lists.denx.de; hs@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
Hi Tom, > -----Original Message----- > From: Tom Rini [mailto:trini@konsulko.com] > Sent: Dienstag, 28. Juli 2015 16:42 > To: Egli, Samuel > Cc: 'doublesin@ti.com'; 'balbi@ti.com'; 'u-boot@lists.denx.de'; > 'hs@denx.de'; 'Stefan Roese'; Meier, Roger; Senn, Joerg; Oleg Belogolov > (o-belogolov@ti.com) > Subject: Re: [RFC] am33xx: add 600us wait in DDR3 initialization sequence > > 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? I tried to use arch_timer instead of .../omap-common/timer.c but it was not straight forward. Just enabling CONFIG_SYS_ARCH_TIMER wouldn't do the trick. Because of CONFIG_OMAP_COMMON we have automatically the omap timer built. Disabling CONFIG_OMAP_COMMON, however, breaks the build. So I couldn't go further. > And wrt the EMIF changes, I'm hoping James can chime in since he knows > that block inside and out :) >[...] Thanks, Sam
Hi James, thank you for your reply! > -----Original Message----- > From: Doublesin, James [mailto:doublesin@ti.com] > Sent: Dienstag, 28. Juli 2015 21:59 > To: Egli, Samuel; 'trini@konsulko.com'; Balbi, Felipe > Cc: 'u-boot@lists.denx.de'; 'hs@denx.de'; 'Stefan Roese'; Meier, Roger; > Senn, Joerg; Belogolov, Oleg > Subject: RE: [RFC] am33xx: add 600us wait in DDR3 initialization sequence > > Hi Samuel, I don't think these changes are necessary. > > I think your concern about the initialization steps is not correct. The > first setting of the REF_CTRL (before SDRAM_CONFIG write) will be used by > the EMIF controller to determine the delay between the rising edge of > DDR_RESETn and DDR_CKE. We typically write a value of 0x3100 in REF_CTRL > to achieve this, which comes from this formula: > > (16*SDRAM_REF_CTRL)/400MHz > 500us That's interesting. This isn't really documented in the TRM. But I see ddr.c: ... if (regs->zq_config) writel(0x80003100, &emif_reg[nr]->emif_sdram_ref_ctrl); ... Why does this depend on zq_config? While debugging I saw that we enter case writing 0x800031000. But the 500us are not waited. > This value will change for different frequencies, but for AM335x and > AM437x, the max freq is 400MHz. When SDRAM_CONFIG is written, the EMIF > controller will kick off a H/W sequence which will configure the memory, Hmm, I'm confused now. In TRM (rev. L) there nothing written that a write to SDRAM_CONFIG will trigger the sequence. However, it is written that a write to SDRAM_REF_CTRL (spec. bits 29,28,26-24) will trigger sequence: "... A write to this field will cause the EMIF to start the SDRAM initialization sequence." So documentation in TRM is seriously wrong? > and will use this setting for the proper RESET to CKE timing. Once that > is finished, another write to REF_CTRL to set the proper refresh rate will > be necessary. > > A separate issue which we have seen does require a delay, however. We > have seen some timeout errors reported on the L3 bus as a result of this > EMIF sequence. This error does not result in any functional problem with > the EMIF initialization, but does result in one of the L3 error flags > being set. We have determined that this is a result of an EMIF access > immediately following the SDRAM_CONFIG write. The EMIF kicks back an > error while the H/W initialization sequence is still active, thus a delay > is required to eliminate this bus error. Again, functionally the EMIF > sequence will work without this delay, but to avoid the error flag, the > delay is needed. Okay, but the code doesn't wait 1ms at any place. I can only say what we experienced and measured on our am3352 based boards: A delay at the end of ddr3 configuration was necessary in order to make it work with the code how it is implemented now. And yes, the order of magnitude of waiting was something like 1-2ms. But, measuring the wait time showed that even with SDRAM_REF_CTRL=0x3100 the delay of 500us was not respected. It was something like ~180us instead. I conclude that my changes are not the way to go but some things need to be changed. However, I'm a bit confused due to the contradictions of your statements with the TRM. I was talking with Oleg too and he pointed me to ddr_cofig.gel file that's used with CCS. In the EMIF config sequence a 1 ms delay is done too: ... WR_MEM_32(base_addr + 0x00000008, SDRAM_CONFIG); /* If delay is not present, interconnect can throw a false error */ us_delay(1000); ... So at least this 1ms delay needs to be added in ddr.c@ void config_sdram(const struct emif_regs *regs, int nr) { if (regs->zq_config) { writel(regs->zq_config, &emif_reg[nr]->emif_zq_config); writel(regs->sdram_config, &cstat->secure_emif_sdram_config); writel(regs->sdram_config, &emif_reg[nr]->emif_sdram_config); writel(regs->ref_ctrl, &emif_reg[nr]->emif_sdram_ref_ctrl); writel(regs->ref_ctrl, &emif_reg[nr]->emif_sdram_ref_ctrl_shdw); } writel(regs->ref_ctrl, &emif_reg[nr]->emif_sdram_ref_ctrl); writel(regs->ref_ctrl, &emif_reg[nr]->emif_sdram_ref_ctrl_shdw); writel(regs->sdram_config, &emif_reg[nr]->emif_sdram_config); } But I don't understand why emif_sdram_config, emif_sdram_ref_ctrl and emif_sdram_ref_ctrl_shdw should be written in if case twice? Is there a reason for this? > > So to summarize, the sequence should be: > > -Configure all other EMIF registers > -Write SDRAM_REF_CTRL=0x3100 > -Write SDRAM_CONFIG with appropriate value to kick off H/W initialization > of the DDR -wait 1ms (to avoid L3 timeout error) -Write SDRAM_REF_CTRL > with refresh rate value for your DDR > > Regards , > James Doublesin > -----Original Message----- > From: Egli, Samuel [mailto:samuel.egli@siemens.com] > Sent: Tuesday, July 28, 2015 9:32 AM > To: 'trini@konsulko.com'; Doublesin, James; Balbi, Felipe > Cc: 'u-boot@lists.denx.de'; 'hs@denx.de'; 'Stefan Roese'; Meier, Roger; > Senn, Joerg; Belogolov, Oleg > Subject: [RFC] am33xx: add 600us wait in DDR3 initialization sequence > > 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. > > Thanks, > > Sam > > > -----Original Message----- > > From: Egli, Samuel > > Sent: Freitag, 17. Juli 2015 13:47 > > To: trini@konsulko.com; doublesin@ti.com; balbi@ti.com > > Cc: u-boot@lists.denx.de; hs@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 Kind regards Sam
On Wed, Jul 29, 2015 at 11:57:26AM +0000, Egli, Samuel wrote: > Hi James, > thank you for your reply! > > > -----Original Message----- > > From: Doublesin, James [mailto:doublesin@ti.com] > > Sent: Dienstag, 28. Juli 2015 21:59 > > To: Egli, Samuel; 'trini@konsulko.com'; Balbi, Felipe > > Cc: 'u-boot@lists.denx.de'; 'hs@denx.de'; 'Stefan Roese'; Meier, Roger; > > Senn, Joerg; Belogolov, Oleg > > Subject: RE: [RFC] am33xx: add 600us wait in DDR3 initialization sequence > > > > Hi Samuel, I don't think these changes are necessary. > > > > I think your concern about the initialization steps is not correct. The > > first setting of the REF_CTRL (before SDRAM_CONFIG write) will be used by > > the EMIF controller to determine the delay between the rising edge of > > DDR_RESETn and DDR_CKE. We typically write a value of 0x3100 in REF_CTRL > > to achieve this, which comes from this formula: > > > > (16*SDRAM_REF_CTRL)/400MHz > 500us > > That's interesting. This isn't really documented in the TRM. But I see > ddr.c: > ... > if (regs->zq_config) > writel(0x80003100, &emif_reg[nr]->emif_sdram_ref_ctrl); > ... > Why does this depend on zq_config? While debugging I saw that we This part I can answer. We use the same functions on DDR2 and DDR3 and use the presence of zq_config in regs to know if this is a DDR2 or DDR3 setup.
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
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(-) -- 1.7.10.4