[PATCH v3 03/13] pm: at91: Workaround DDRSDRC self-refresh bug with LPDDR1 memories.
Peter Rosin
peda at axentia.se
Tue Jan 27 13:55:52 PST 2015
I wrote:
> Sergei Shtylyov wrote:
> > On 1/27/2015 8:53 AM, Wenyou Yang wrote:
> >
> > > From: Peter Rosin <peda at 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 at axentia.se>
> > > Acked-by: Nicolas Ferre <nicolas.ferre at 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
More information about the linux-arm-kernel
mailing list