I wrote:

Sergei Shtylyov wrote:

quoted

On 1/27/2015 8:53 AM, Wenyou Yang wrote:

quoted

From: Peter Rosin <redacted>

quoted

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.

quoted

Assume that the second controller has the same fault, but that is
untested.

quoted

Signed-off-by: Peter Rosin <redacted>
Acked-by: Nicolas Ferre <redacted>
---
  arch/arm/mach-at91/pm_slowclock.S  |   43

+++++++++++++++++++++++++++++++-----

quoted

  include/soc/at91/at91sam9_ddrsdr.h |    2 +-
  2 files changed, 39 insertions(+), 6 deletions(-)

quoted

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

[...]

quoted

@@ -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.

quoted

quoted

+	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?

quoted

quoted

+	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