| Message ID | 1422338006-3371-1-git-send-email-wenyou.yang@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 1/27/2015 8:53 AM, 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 [...] > @@ -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 Didn't you forget ~? Either that, or ~ above is not needed, I think. > + 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 Didn't you forget ~? And isn't it 3-operand instruction (as seen in the above code)? > + orr tmp2, #AT91_DDRSDRC_LPCB_SELF_REFRESH Only 2 operands? [...] WBR, Sergei
Sergei Shtylyov wrote: > Hello. Hi! > On 1/27/2015 8:53 AM, 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 > [...] > > @@ -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 > > Didn't you forget ~? Either that, or ~ above is not needed, I think. The code is correct, the first bic with ~ clears bits not in the relevant field in order to compare if LPDDR mode is active. The second bic(eq) w/o ~ clears the field, to make way for the bits in the below orreq when actually changing the register content into DDR2 mode. > > + 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 > > Didn't you forget ~? And isn't it 3-operand instruction (as seen in the above > code)? The logic for the LPR register is from the old code, the "only" thing I did to it was changing the instruction sequence to not have the ???ne form, i.e. ldrne, strne, bicne, orrne became ldr, str, bic, orr with a jump around it instead. So, the original code also had a two argument bic(ne), which indeed is strange, and I don't know why there is no warning from the assembler. Since there is no warning, my guess is that the assembler somehow mends it? Or does the patch actually break the second controller? It would be a surprise it the assembler handles 2-operand bicne differently from a 2-operand bic, no? > > + orr tmp2, #AT91_DDRSDRC_LPCB_SELF_REFRESH > > Only 2 operands? Same argument as above. I didn't touch it (sort of...) Should I update the patch and fix this collateral 2-operand problem as well? To me, it feels like a separate patch, no? > [...] > > WBR, Sergei Cheers, Peter
I wrote: > Sergei Shtylyov wrote: > > On 1/27/2015 8:53 AM, 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 > > [...] > > > @@ -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 > > > > Didn't you forget ~? Either that, or ~ above is not needed, I think. > > The code is correct, the first bic with ~ clears bits not in the relevant > field in order to compare if LPDDR mode is active. The second bic(eq) > w/o ~ clears the field, to make way for the bits in the below orreq > when actually changing the register content into DDR2 mode. > > > > + 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 > > > > Didn't you forget ~? And isn't it 3-operand instruction (as seen in the above > > code)? > > The logic for the LPR register is from the old code, the "only" thing > I did to it was changing the instruction sequence to not have the > ???ne form, i.e. ldrne, strne, bicne, orrne became ldr, str, bic, orr > with a jump around it instead. So, the original code also had a two > argument bic(ne), which indeed is strange, and I don't know why > there is no warning from the assembler. Since there is no warning, > my guess is that the assembler somehow mends it? Or does the > patch actually break the second controller? It would be a surprise > it the assembler handles 2-operand bicne differently from a s/it the/if the/ > 2-operand bic, no? > > > > + orr tmp2, #AT91_DDRSDRC_LPCB_SELF_REFRESH > > > > Only 2 operands? > > Same argument as above. I didn't touch it (sort of...) > > Should I update the patch and fix this collateral 2-operand problem as > well? To me, it feels like a separate patch, no? I have now checked the assembler output, and apparently it mends the input, just as I thought. That might be a fluke, of course, or it might be a deliberate shorthand when the destination register is the same as the following operand? But I also note that there are more instances of this 2 vs. 3 argument syntax, and I suggest that they are all fixed in one go, if it is determined that they need fixing. I am obviously not an authority when it comes to arm assembler syntax, so someone else will have to advise... Cheers, Peter
On Tue, Jan 27, 2015 at 09:55:52PM +0000, Peter Rosin wrote: > I have now checked the assembler output, and apparently it mends the > input, just as I thought. That might be a fluke, of course, or it might be a > deliberate shorthand when the destination register is the same as the > following operand? I believe it is because we have the assembler in unified asm mode, where such things are legal (due to Thumb2.) That means these two are legal: orr r2, #const orr r2, r2, #const and equivalent in this context.
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