| Message ID | 1422265139-23011-1-git-send-email-wenyou.yang@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Wenyou, On Mon, Jan 26, 2015 at 05:38:59PM +0800, Wenyou Yang wrote: > From: Peter Rosin <peda@axentia.se> > > The DDRSDR controller fails miserably to put LPDDR1 memories in > self-refresh. Force the controller to think it has DDR2 memories > during the self-refresh period, as the DDR2 self-refresh spec is > equivalent to LPDDR1, and is correctly implemented in the > controller. > > Assume that the second controller has the same fault, but that is > untested. > > Signed-off-by: Peter Rosin <peda@axentia.se> > Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> > --- > arch/arm/mach-at91/pm_slowclock.S | 43 +++++++++++++++++++++++++++++++----- > include/soc/at91/at91sam9_ddrsdr.h | 2 +- > 2 files changed, 39 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S > index e2bfaf5..1155217 100644 > --- a/arch/arm/mach-at91/pm_slowclock.S > +++ b/arch/arm/mach-at91/pm_slowclock.S > @@ -100,6 +100,16 @@ ddr_sr_enable: > cmp memctrl, #AT91_MEMCTRL_DDRSDR > bne sdr_sr_enable > > + /* LPDDR1 --> force DDR2 mode during self-refresh */ I think we should explain we are dealing with an errata here, this is not obvious at first sight, the patch summary may find its place here :-) Sylvain
Le 26/01/2015 11:36, Sylvain Rochet a écrit : > Hello Wenyou, > > On Mon, Jan 26, 2015 at 05:38:59PM +0800, Wenyou Yang wrote: >> From: Peter Rosin <peda@axentia.se> >> >> The DDRSDR controller fails miserably to put LPDDR1 memories in >> self-refresh. Force the controller to think it has DDR2 memories >> during the self-refresh period, as the DDR2 self-refresh spec is >> equivalent to LPDDR1, and is correctly implemented in the >> controller. >> >> Assume that the second controller has the same fault, but that is >> untested. >> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> >> --- >> arch/arm/mach-at91/pm_slowclock.S | 43 +++++++++++++++++++++++++++++++----- >> include/soc/at91/at91sam9_ddrsdr.h | 2 +- >> 2 files changed, 39 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S >> index e2bfaf5..1155217 100644 >> --- a/arch/arm/mach-at91/pm_slowclock.S >> +++ b/arch/arm/mach-at91/pm_slowclock.S >> @@ -100,6 +100,16 @@ ddr_sr_enable: >> cmp memctrl, #AT91_MEMCTRL_DDRSDR >> bne sdr_sr_enable >> >> + /* LPDDR1 --> force DDR2 mode during self-refresh */ > > I think we should explain we are dealing with an errata here, this is > not obvious at first sight, the patch summary may find its place here :-) True but the problem is that this errata is not public yet, it will be in a couple of weeks. I have the feeling though that the commit message is pretty clear. We'll maybe add that it"s an actual errata. Bye,
Hello Nicolas, On Mon, Jan 26, 2015 at 02:34:38PM +0100, Nicolas Ferre wrote: > Le 26/01/2015 11:36, Sylvain Rochet a écrit : > > > > I think we should explain we are dealing with an errata here, this is > > not obvious at first sight, the patch summary may find its place here :-) > > True but the problem is that this errata is not public yet, it will be > in a couple of weeks. > > I have the feeling though that the commit message is pretty clear. We'll > maybe add that it"s an actual errata. Humm, this is not what I meant actually. I only proposed a code source comment explaining why this is done this way, the current patch summary looked like it will be perfect between /* */ ;-) Sylvain
Sylvain Rochet wrote: > Hello Nicolas, > > On Mon, Jan 26, 2015 at 02:34:38PM +0100, Nicolas Ferre wrote: > > Le 26/01/2015 11:36, Sylvain Rochet a écrit : > > > > > > I think we should explain we are dealing with an errata here, this > > > is not obvious at first sight, the patch summary may find its place > > > here :-) > > > > True but the problem is that this errata is not public yet, it will be > > in a couple of weeks. > > > > I have the feeling though that the commit message is pretty clear. > > We'll maybe add that it"s an actual errata. > > Humm, this is not what I meant actually. I only proposed a code source > comment explaining why this is done this way, the current patch summary > looked like it will be perfect between /* */ ;-) I did not want to fill up the source with wordy comments, and settled for a one-liner. I don't know much about the underlying reasons other than the fact that LPDDR1 mode of the controller isn't working properly in self-refresh and that the DDR2 spec is similar enough to work. The one-liner comment says about the same thing, but not with so many words. The comment does make it clear that the switch to DDR2 is intentional, and that is all that is needed as protection from some future cleanup. I mean, anyone seeing that comment and just erasing the whole thing without further investigation is not doing a very good job as there is no reason to intentionally switch from LPDDR1 mode to DDR2 mode, other that the fact that the LPDDR1 mode isn't working for some reason. That reason is not to be found in the commit message and I have no information to improve the situation. IMO, the only thing missing is a pointer to the as yet unreleased errata, which should explain the situation clearly for any and all interested parties. May I suggest that someone who cares sends a patch with the comment update when the errata is released? If others feel differently, by all means please reword and expand the comment. Cheers, Peter
Le 26/01/2015 16:58, Peter Rosin a écrit : > Sylvain Rochet wrote: >> Hello Nicolas, >> >> On Mon, Jan 26, 2015 at 02:34:38PM +0100, Nicolas Ferre wrote: >>> Le 26/01/2015 11:36, Sylvain Rochet a écrit : >>>> >>>> I think we should explain we are dealing with an errata here, this >>>> is not obvious at first sight, the patch summary may find its place >>>> here :-) >>> >>> True but the problem is that this errata is not public yet, it will be >>> in a couple of weeks. >>> >>> I have the feeling though that the commit message is pretty clear. >>> We'll maybe add that it"s an actual errata. >> >> Humm, this is not what I meant actually. I only proposed a code source >> comment explaining why this is done this way, the current patch summary >> looked like it will be perfect between /* */ ;-) > > I did not want to fill up the source with wordy comments, and settled > for a one-liner. I don't know much about the underlying reasons other > than the fact that LPDDR1 mode of the controller isn't working properly > in self-refresh and that the DDR2 spec is similar enough to work. > > The one-liner comment says about the same thing, but not with so > many words. The comment does make it clear that the switch to DDR2 > is intentional, and that is all that is needed as protection from some > future cleanup. I mean, anyone seeing that comment and just erasing > the whole thing without further investigation is not doing a very good > job as there is no reason to intentionally switch from LPDDR1 mode to > DDR2 mode, other that the fact that the LPDDR1 mode isn't working for > some reason. That reason is not to be found in the commit message > and I have no information to improve the situation. IMO, the only thing > missing is a pointer to the as yet unreleased errata, which should explain > the situation clearly for any and all interested parties. May I suggest that > someone who cares sends a patch with the comment update when the > errata is released? That's the option that I'll take. Let's go for it (and anyone remind me if I don't when the errata is released). Bye, > If others feel differently, by all means please reword and expand the > comment. > > Cheers, > Peter >
Hello Nicolas and Peter, On Mon, Jan 26, 2015 at 05:04:25PM +0100, Nicolas Ferre wrote: > Le 26/01/2015 16:58, Peter Rosin a écrit : > > Sylvain Rochet wrote: > >> Hello Nicolas, > >> > >> On Mon, Jan 26, 2015 at 02:34:38PM +0100, Nicolas Ferre wrote: > >>> Le 26/01/2015 11:36, Sylvain Rochet a écrit : > >>>> > >>>> I think we should explain we are dealing with an errata here, this > >>>> is not obvious at first sight, the patch summary may find its place > >>>> here :-) > >>> > >>> True but the problem is that this errata is not public yet, it will be > >>> in a couple of weeks. > >>> > >>> I have the feeling though that the commit message is pretty clear. > >>> We'll maybe add that it"s an actual errata. > >> > >> Humm, this is not what I meant actually. I only proposed a code source > >> comment explaining why this is done this way, the current patch summary > >> looked like it will be perfect between /* */ ;-) > > > > I did not want to fill up the source with wordy comments, and settled > > for a one-liner. I don't know much about the underlying reasons other > > than the fact that LPDDR1 mode of the controller isn't working properly > > in self-refresh and that the DDR2 spec is similar enough to work. > > > > The one-liner comment says about the same thing, but not with so > > many words. The comment does make it clear that the switch to DDR2 > > is intentional, and that is all that is needed as protection from some > > future cleanup. I mean, anyone seeing that comment and just erasing > > the whole thing without further investigation is not doing a very good > > job as there is no reason to intentionally switch from LPDDR1 mode to > > DDR2 mode, other that the fact that the LPDDR1 mode isn't working for > > some reason. That reason is not to be found in the commit message > > and I have no information to improve the situation. IMO, the only thing > > missing is a pointer to the as yet unreleased errata, which should explain > > the situation clearly for any and all interested parties. May I suggest that > > someone who cares sends a patch with the comment update when the > > errata is released? > > That's the option that I'll take. > > Let's go for it (and anyone remind me if I don't when the errata is > released). I fully agree with that, a pointer to a datasheet errata is exactly the same thing as explaining the issue. Sylvain
diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S index e2bfaf5..1155217 100644 --- a/arch/arm/mach-at91/pm_slowclock.S +++ b/arch/arm/mach-at91/pm_slowclock.S @@ -100,6 +100,16 @@ ddr_sr_enable: cmp memctrl, #AT91_MEMCTRL_DDRSDR bne sdr_sr_enable + /* LPDDR1 --> force DDR2 mode during self-refresh */ + ldr tmp1, [sdramc, #AT91_DDRSDRC_MDR] + str tmp1, .saved_sam9_mdr + bic tmp1, tmp1, #~AT91_DDRSDRC_MD + cmp tmp1, #AT91_DDRSDRC_MD_LOW_POWER_DDR + ldreq tmp1, [sdramc, #AT91_DDRSDRC_MDR] + biceq tmp1, tmp1, #AT91_DDRSDRC_MD + orreq tmp1, tmp1, #AT91_DDRSDRC_MD_DDR2 + streq tmp1, [sdramc, #AT91_DDRSDRC_MDR] + /* prepare for DDRAM self-refresh mode */ ldr tmp1, [sdramc, #AT91_DDRSDRC_LPR] str tmp1, .saved_sam9_lpr @@ -108,14 +118,26 @@ ddr_sr_enable: /* figure out if we use the second ram controller */ cmp ramc1, #0 - ldrne tmp2, [ramc1, #AT91_DDRSDRC_LPR] - strne tmp2, .saved_sam9_lpr1 - bicne tmp2, #AT91_DDRSDRC_LPCB - orrne tmp2, #AT91_DDRSDRC_LPCB_SELF_REFRESH + beq ddr_no_2nd_ctrl + + ldr tmp2, [ramc1, #AT91_DDRSDRC_MDR] + str tmp2, .saved_sam9_mdr1 + bic tmp2, tmp2, #~AT91_DDRSDRC_MD + cmp tmp2, #AT91_DDRSDRC_MD_LOW_POWER_DDR + ldreq tmp2, [ramc1, #AT91_DDRSDRC_MDR] + biceq tmp2, tmp2, #AT91_DDRSDRC_MD + orreq tmp2, tmp2, #AT91_DDRSDRC_MD_DDR2 + streq tmp2, [ramc1, #AT91_DDRSDRC_MDR] + + ldr tmp2, [ramc1, #AT91_DDRSDRC_LPR] + str tmp2, .saved_sam9_lpr1 + bic tmp2, #AT91_DDRSDRC_LPCB + orr tmp2, #AT91_DDRSDRC_LPCB_SELF_REFRESH /* Enable DDRAM self-refresh mode */ + str tmp2, [ramc1, #AT91_DDRSDRC_LPR] +ddr_no_2nd_ctrl: str tmp1, [sdramc, #AT91_DDRSDRC_LPR] - strne tmp2, [ramc1, #AT91_DDRSDRC_LPR] b sdr_sr_done @@ -227,12 +249,17 @@ sdr_sr_done: */ cmp memctrl, #AT91_MEMCTRL_DDRSDR bne sdr_en_restore + /* Restore MDR in case of LPDDR1 */ + ldr tmp1, .saved_sam9_mdr + str tmp1, [sdramc, #AT91_DDRSDRC_MDR] /* Restore LPR on AT91 with DDRAM */ ldr tmp1, .saved_sam9_lpr str tmp1, [sdramc, #AT91_DDRSDRC_LPR] /* if we use the second ram controller */ cmp ramc1, #0 + ldrne tmp2, .saved_sam9_mdr1 + strne tmp2, [ramc1, #AT91_DDRSDRC_MDR] ldrne tmp2, .saved_sam9_lpr1 strne tmp2, [ramc1, #AT91_DDRSDRC_LPR] @@ -263,5 +290,11 @@ ram_restored: .saved_sam9_lpr1: .word 0 +.saved_sam9_mdr: + .word 0 + +.saved_sam9_mdr1: + .word 0 + ENTRY(at91_slow_clock_sz) .word .-at91_slow_clock diff --git a/include/soc/at91/at91sam9_ddrsdr.h b/include/soc/at91/at91sam9_ddrsdr.h index 0210797..dc10c52 100644 --- a/include/soc/at91/at91sam9_ddrsdr.h +++ b/include/soc/at91/at91sam9_ddrsdr.h @@ -92,7 +92,7 @@ #define AT91_DDRSDRC_UPD_MR (3 << 20) /* Update load mode register and extended mode register */ #define AT91_DDRSDRC_MDR 0x20 /* Memory Device Register */ -#define AT91_DDRSDRC_MD (3 << 0) /* Memory Device Type */ +#define AT91_DDRSDRC_MD (7 << 0) /* Memory Device Type */ #define AT91_DDRSDRC_MD_SDR 0 #define AT91_DDRSDRC_MD_LOW_POWER_SDR 1 #define AT91_DDRSDRC_MD_LOW_POWER_DDR 3