RE: [PATCH v3 03/13] pm: at91: Workaround DDRSDRC self-refresh bug with LPDDR1 memories.

[Peter Rosin <peda@axentia.se>]

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