[PATCH 2/2] AT91 slow-clock resume: don't restore the PLL settings when the PLL was off

[PATCH 2/2] AT91 slow-clock resume: don't restore the PLL settings when the PLL was off

[Anders Larsen]

From: Julien Langer <julien.langer@gmail.com>

AT91: Don't try to restore the PLL settings on resume when the PLLs were turned
off before suspending.

We run into this problem with the PLLB on the at91: ohci-at91 disables the PLLB
when going to suspend. The slowclock code however tries to do the same: It
saves the PLLB register value and when restoring the value during resume it
waits for the PLLB to lock again. However the PLL will never lock and the loop
would run into its timeout because the slowclock code just stored and restored
an empty register.
Fix the problem by only restoring PLLA/PLLB when the registers were != 0.

Signed-off-by: Julien Langer <julien.langer@gmail.com>
Signed-off-by: Anders Larsen <al@alarsen.net>
Cc: Andrew Victor <avictor.za@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/mach-at91/pm_slowclock.S |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: b/arch/arm/mach-at91/pm_slowclock.S
===================================================================
--- a/arch/arm/mach-at91/pm_slowclock.S
+++ b/arch/arm/mach-at91/pm_slowclock.S
@@ -171,17 +171,25 @@ ENTRY(at91_slow_clock)
 	ldr	r3, [r1, #(AT91_CKGR_PLLAR - AT91_PMC)]
 	str	r3, .saved_pllar
 
+	cmp r3, #0
+	beq 3f
+
 	mov	r3, #AT91_PMC_PLLCOUNT
 	orr	r3, r3, #(1 << 29)		/* bit 29 always set */
 	str	r3, [r1, #(AT91_CKGR_PLLAR - AT91_PMC)]
 
+3:
 	/* Save PLLB setting and disable it */
 	ldr	r3, [r1, #(AT91_CKGR_PLLBR - AT91_PMC)]
 	str	r3, .saved_pllbr
 
+	cmp r3, #0
+	beq 4f
+
 	mov	r3, #AT91_PMC_PLLCOUNT
 	str	r3, [r1, #(AT91_CKGR_PLLBR - AT91_PMC)]
 
+4:
 	/* Turn off the main oscillator */
 	ldr	r3, [r1, #(AT91_CKGR_MOR - AT91_PMC)]
 	bic	r3, r3, #AT91_PMC_MOSCEN
@@ -199,16 +207,22 @@ ENTRY(at91_slow_clock)
 
 	/* Restore PLLB setting */
 	ldr	r3, .saved_pllbr
+	cmp r3, #0
+	beq 5f
 	str	r3, [r1, #(AT91_CKGR_PLLBR - AT91_PMC)]
 
 	wait_pllblock
 
+5:
 	/* Restore PLLA setting */
 	ldr	r3, .saved_pllar
+	cmp r3, #0
+	beq 6f
 	str	r3, [r1, #(AT91_CKGR_PLLAR - AT91_PMC)]
 
 	wait_pllalock
 
+6:
 #ifdef SLOWDOWN_MASTER_CLOCK
 	/*
 	 * First set PRES if it was not 0,

[PATCH 2/2] AT91 slow-clock resume: don't restore the PLL settings when the PLL was off

[Andrew Victor]

hi Anders,

> From: Julien Langer <julien.langer@gmail.com>
>
> AT91: Don't try to restore the PLL settings on resume when the PLLs were turned
> off before suspending.
>
> We run into this problem with the PLLB on the at91: ohci-at91 disables the PLLB
> when going to suspend. The slowclock code however tries to do the same: It
> saves the PLLB register value and when restoring the value during resume it
> waits for the PLLB to lock again. However the PLL will never lock and the loop
> would run into its timeout because the slowclock code just stored and restored
> an empty register.
> Fix the problem by only restoring PLLA/PLLB when the registers were != 0.
>
> Signed-off-by: Julien Langer <julien.langer@gmail.com>
> Signed-off-by: Anders Larsen <al@alarsen.net>
> Cc: Andrew Victor <avictor.za@gmail.com>
> Cc: Russell King <linux@arm.linux.org.uk>

> @@ -199,16 +207,22 @@ ENTRY(at91_slow_clock)
>
>        /* Restore PLLB setting */
>        ldr     r3, .saved_pllbr
> +       cmp r3, #0
> +       beq 5f
>        str     r3, [r1, #(AT91_CKGR_PLLBR - AT91_PMC)]
>
>        wait_pllblock
>
> +5:
>        /* Restore PLLA setting */
>        ldr     r3, .saved_pllar
> +       cmp r3, #0
> +       beq 6f
>        str     r3, [r1, #(AT91_CKGR_PLLAR - AT91_PMC)]
>
>        wait_pllalock
>
> +6:

I don't think it's sufficient skip the "wait for lock" if the
PLLA/PLLB value is 0.
For example, since bit 29 of PLLA is always 1, the wait_pllalock will
always run - even if MULA is 0 (which means the PLLA is disabled) and
will therefore never lock.
Similarly, for other bits in the register which might happen to be set.

The code should rather be something like:
  Save PLLA
  Save PLLB
  ... wait for interrupt ....
  Restore PLLB
  if (PLLB & AT91_PMC_MUL != 0)
      Wait for PLLB to lock
  Restore PLLA
  if (PLLA & AT91_PMC_MUL != 0)
      Wait for PLLA to lock


Regards,
  Andrew Victor

[PATCH 2/2] AT91 slow-clock resume: don't restore the PLL settings when the PLL was off

[Anders Larsen]

Hi Andrew,

On 2010-04-06 23:45:41, Andrew Victor wrote:
> I don't think it's sufficient skip the "wait for lock" if the
> PLLA/PLLB value is 0.
> For example, since bit 29 of PLLA is always 1, the wait_pllalock will
> always run - even if MULA is 0 (which means the PLLA is disabled) and
> will therefore never lock.
> Similarly, for other bits in the register which might happen to be set.
> 
> The code should rather be something like:
>   Save PLLA
>   Save PLLB
>   ... wait for interrupt ....
>   Restore PLLB
>   if (PLLB & AT91_PMC_MUL != 0)
>       Wait for PLLB to lock
>   Restore PLLA
>   if (PLLA & AT91_PMC_MUL != 0)
>       Wait for PLLA to lock

point taken.

The (more simple) patch below should match your suggestion
(added as #6043/1 to rmk's tracker):

at91: slow-clock resume: Don't wait for a disabled PLL to lock.

We run into this problem with the PLLB on the at91: ohci-at91 disables the PLLB
when going to suspend. The slowclock code however tries to do the same: It
saves the PLLB register value and when restoring the value during resume, it
waits for the PLLB to lock again. However the PLL will never lock and the loop
would run into its timeout because the slowclock code just stored and restored
an empty register.
This fixes the problem by only restoring PLLA/PLLB when they were enabled
at suspend time.

Signed-off-by: Anders Larsen <al@alarsen.net>
Cc: Andrew Victor <avictor.za@gmail.com>
Cc: Julien Langer <julien.langer@gmail.com>
---

 arch/arm/mach-at91/pm_slowclock.S |   12 ++++++++++++
 1 file changed, 12 insertions(+)


Index: b/arch/arm/mach-at91/pm_slowclock.S
===================================================================
--- a/arch/arm/mach-at91/pm_slowclock.S
+++ b/arch/arm/mach-at91/pm_slowclock.S
@@ -201,13 +201,25 @@ ENTRY(at91_slow_clock)
 	ldr	r3, .saved_pllbr
 	str	r3, [r1, #(AT91_CKGR_PLLBR - AT91_PMC)]
 
+	tst	r3, #(AT91_PMC_MUL &  0xff0000)
+	bne	1f
+	tst	r3, #(AT91_PMC_MUL & ~0xff0000)
+	beq	2f
+1:
 	wait_pllblock
+2:
 
 	/* Restore PLLA setting */
 	ldr	r3, .saved_pllar
 	str	r3, [r1, #(AT91_CKGR_PLLAR - AT91_PMC)]
 
+	tst	r3, #(AT91_PMC_MUL &  0xff0000)
+	bne	3f
+	tst	r3, #(AT91_PMC_MUL & ~0xff0000)
+	beq	4f
+3:
 	wait_pllalock
+4:
 
 #ifdef SLOWDOWN_MASTER_CLOCK
 	/*

[PATCH 2/2] AT91 slow-clock resume: don't restore the PLL settings when the PLL was off

[Andrew Victor]

hi Anders,

> at91: slow-clock resume: Don't wait for a disabled PLL to lock.
>
> We run into this problem with the PLLB on the at91: ohci-at91 disables the PLLB
> when going to suspend. The slowclock code however tries to do the same: It
> saves the PLLB register value and when restoring the value during resume, it
> waits for the PLLB to lock again. However the PLL will never lock and the loop
> would run into its timeout because the slowclock code just stored and restored
> an empty register.
> This fixes the problem by only restoring PLLA/PLLB when they were enabled
> at suspend time.
>
> Signed-off-by: Anders Larsen <al@alarsen.net>
> Cc: Andrew Victor <avictor.za@gmail.com>
> Cc: Julien Langer <julien.langer@gmail.com>

> + ? ? ? tst ? ? r3, #(AT91_PMC_MUL & ?0xff0000)
> + ? ? ? bne ? ? 1f
> + ? ? ? tst ? ? r3, #(AT91_PMC_MUL & ~0xff0000)
> + ? ? ? beq ? ? 2f
> +1:
> ? ? ? ?wait_pllblock
> +2:

AT91_PMC_MUL is 11 bits (so 0x7ff0000)

Is the mask (0xff0000) correct in the above code?
It looks like wait_pllblock will be skipped if the MUL field is set to
0x100, 0x200, 0x300, etc.


Regards,
  Andrew Victor

[PATCH 2/2] AT91 slow-clock resume: don't restore the PLL settings when the PLL was off

[Anders Larsen]

Hi Andrew,

On 2010-04-13 10:14:53, Andrew Victor wrote:
> > + ? ? ? tst ? ? r3, #(AT91_PMC_MUL & ?0xff0000)
> > + ? ? ? bne ? ? 1f
> > + ? ? ? tst ? ? r3, #(AT91_PMC_MUL & ~0xff0000)
> > + ? ? ? beq ? ? 2f
> > +1:
> > ? ? ? ?wait_pllblock
> > +2:
> 
> AT91_PMC_MUL is 11 bits (so 0x7ff0000)

it's not possible to use that constant directly; there are too many bits
set (the ARM instruction set has room for an 8-bit constant and a shift
value)...

> Is the mask (0xff0000) correct in the above code?
> It looks like wait_pllblock will be skipped if the MUL field is set to
> 0x100, 0x200, 0x300, etc.

...so I made the test a two-step process; the first 'tst' checks if
the bits masked by (AT91_PMC_MUL & 0xff0000) = 0x00ff0000 are all zero,
the second 'tst' checks if the bits masked by
(AT91_PMC_MUL & ~0xff0000) = 0x07000000 are zero, so all 11 bits are
indeed tested.

There might well be an easier way to do this, but I didn't manage to
find it - the code isn't on a hot path anyway (it's executed _once_ upon
resume after suspend).

Cheers
Anders