AT91 slow clock mode regression/fixes, improvement proposal

AT91 slow clock mode regression/fixes, improvement proposal

[Sylvain Rochet]

Hello Wenyou,

You worked recently on AT91 slow clock mode in the linux4sam/linux-at91 
master branch, however I am seeing regressions on a AT91SAM9G35-EK board 
concerning the following commit-id:

e0c8ba9b0ec3154e87da747098ee56e96ca3cee6
  pm: at91: pm_slowclock: move disable/enable PLLB out of the route

cc4dc9885103af3b3262f7ac2b6aa887df1618b3
  pm: at91: pm_slowclock: improvement to disable the UTMI PLL



About cc4dc9885103af3b3262f7ac2b6aa887df1618b3:

The assembly look wrong for me, it seems you can't have two following 
labels with the same identifier, so:

/* Turn on UTMI PLL */
cmp flag, #1
bne 1f
  (...)
1:
(...)
1:
wait_pllblock

Is actually jumping to wait_pllblock before enabling PLLB instead of 
skipping UTMI PLL. Unfortunately the compiler does not warn about that.



About e0c8ba9b0ec3154e87da747098ee56e96ca3cee6:

I have mixed feeling about moving the PLL enabling from slow clock mode 
to master clock mode. Starting PLL is almost all about waiting, waiting, 
and waiting until they are stable enough to be used, the few CPU 
instructions required to switch ON the PLL is nothing compared to the 
wait time.

To be sure I benchmarked the required time to set up the UTMI PLL (using 
a GPIO + scope) on my AT91SAM9G35-CM module in both slow clock and 
master clock mode, I found out the required time to start up the PLL is, 
as expected, -exactly- the same.

But, previously, we were waiting with the CPU in slow clock mode, when 
the CPU power consumption is very very low, now we are waiting when the 
CPU is in full speed, which is worse.

Or, I am missing something ?


Anyway, if we wait for the PLL in master clock mode we *MUST* increase 
the PLL timeout a lot, with the current code I guess we are leaving the 
resume code when the PLL are not yet ready at all since we are only 
waiting out of a 1000 iteration loop on master clock.

(Note that I didn't get fooled by that and I disabled the timeout when I 
checked the UPLL start time.)



Furthermore, it looks like MCKRDY_TIMEOUT set to 1000 is not enough, my board 
crashes in about 1 wake up to 10 with this value and works perfectly 
fine with 4000.


I have attached a patch that do the following:

 * Increase MCKRDY_TIMEOUT from 1000 to 4000 so my board don't crash.
 * Fix the assembly on the "Turn on UTMI PLL" part
 * Increase timeouts for PLLB and UPLL


However I still disagree with this patch (this is why I didn't do a pull 
request ;-), I am proposing doing a patch set that:

 * Increase MCKRDY_TIMEOUT from 1000 to 4000 so my board don't crash, I 
   would be happy to have a feed back on this with your boards ;-)
 * Fix the assembly on the "Turn on UTMI PLL" part
 * Move back PLLB and UPLL to slow clock mode to save power

What do you think ?


I have also mixed feeling about those timeouts, is it really useful in 
the wild ?  We don't event catch the timeout event to do something else 
if it happens, we still switch to the master clock whatever happened or 
even worse we continue even if AT91_PMC_MCKRDY isn't set? which is why I 
am having hard fault. In my opinion this is the watchdog job to handle 
those cases cleanly, there is nothing much we can do if something fail 
other than waiting a watchdog reset.

(Yeah, I know the watchdog is hard to use with sleep mode on AT91 chip, 
I have a work in progress about using the RTC to wake up before the 
watchdog expire to clear the watchdog and go back to sleep as fast as 
possible without resuming all the devices. It works, but I am still not 
happy at all with what I did for now.)

Therefore I am also proposing to remove all the timeout.


By the way, I don't know for other AT91 boards (or without DT, or 
something else), but the UTMI PLL suspend/resume seem unnecessary for 
me, the USB driver is already disabling the USB PLL when suspending, 
could you confirm ?



Regards,
Sylvain
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AT91-PM-slow-clock-mode-fixes.patch
Type: text/x-diff
Size: 1769 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141218/2a3e922f/attachment-0001.bin>

AT91 slow clock mode regression/fixes, improvement proposal

[Sylvain Rochet]

Hello,

On Thu, Dec 18, 2014 at 09:39:04PM +0100, Sylvain Rochet wrote:
> 
> About e0c8ba9b0ec3154e87da747098ee56e96ca3cee6:
> 
> I have mixed feeling about moving the PLL enabling from slow clock mode 
> to master clock mode. Starting PLL is almost all about waiting, waiting, 
> and waiting until they are stable enough to be used, the few CPU 
> instructions required to switch ON the PLL is nothing compared to the 
> wait time.
> 
> To be sure I benchmarked the required time to set up the UTMI PLL (using 
> a GPIO + scope) on my AT91SAM9G35-CM module in both slow clock and 
> master clock mode, I found out the required time to start up the PLL is, 
> as expected, -exactly- the same.
> 
> But, previously, we were waiting with the CPU in slow clock mode, when 
> the CPU power consumption is very very low, now we are waiting when the 
> CPU is in full speed, which is worse.
> 
> Or, I am missing something ?

Responding myself on this point, just did further mesurements, and... 
well, this can be discarded. I didn't notice at first that PLL startup 
time are only about to take ~500 us, which is a negligible amount of 
time and is actually a bit faster (just a bit) on master clock. I 
really should have checked the timing as well with PLL start up 
disabled, sorry :)

Sylvain

AT91 slow clock mode regression/fixes, improvement proposal

[Yang, Wenyou]

Hi Sylvain,

> -----Original Message-----
> From: Sylvain Rochet [mailto:sylvain.rochet at finsecur.com]
> Sent: Friday, December 19, 2014 4:39 AM
> To: Yang, Wenyou; Ferre, Nicolas; Desroches, Ludovic; Alexandre Belloni; Maxime
> Ripard
> Cc: linux-arm-kernel at lists.infradead.org
> Subject: AT91 slow clock mode regression/fixes, improvement proposal
> 
> Hello Wenyou,
> 
> You worked recently on AT91 slow clock mode in the linux4sam/linux-at91 master
> branch, however I am seeing regressions on a AT91SAM9G35-EK board
> concerning the following commit-id:
> 
> e0c8ba9b0ec3154e87da747098ee56e96ca3cee6
>   pm: at91: pm_slowclock: move disable/enable PLLB out of the route
> 
> cc4dc9885103af3b3262f7ac2b6aa887df1618b3
>   pm: at91: pm_slowclock: improvement to disable the UTMI PLL
> 
> 
> 
> About cc4dc9885103af3b3262f7ac2b6aa887df1618b3:
> 
> The assembly look wrong for me, it seems you can't have two following labels with
> the same identifier, so:
> 
> /* Turn on UTMI PLL */
> cmp flag, #1
> bne 1f
>   (...)
> 1:
> (...)
> 1:
> wait_pllblock
> 
> Is actually jumping to wait_pllblock before enabling PLLB instead of skipping UTMI
> PLL. Unfortunately the compiler does not warn about that.
Thanks. I will check it

> 
> 
> 
> About e0c8ba9b0ec3154e87da747098ee56e96ca3cee6:
> 
> I have mixed feeling about moving the PLL enabling from slow clock mode
> to master clock mode. Starting PLL is almost all about waiting, waiting,
> and waiting until they are stable enough to be used, the few CPU
> instructions required to switch ON the PLL is nothing compared to the
> wait time.
> 
> To be sure I benchmarked the required time to set up the UTMI PLL (using
> a GPIO + scope) on my AT91SAM9G35-CM module in both slow clock and
> master clock mode, I found out the required time to start up the PLL is,
> as expected, -exactly- the same.
> 
> But, previously, we were waiting with the CPU in slow clock mode, when
> the CPU power consumption is very very low, now we are waiting when the
> CPU is in full speed, which is worse.
> 
> Or, I am missing something ?
> 
> 
> Anyway, if we wait for the PLL in master clock mode we *MUST* increase
> the PLL timeout a lot, with the current code I guess we are leaving the
> resume code when the PLL are not yet ready at all since we are only
> waiting out of a 1000 iteration loop on master clock.
> 
> (Note that I didn't get fooled by that and I disabled the timeout when I
> checked the UPLL start time.)
> 
> 
> 
> Furthermore, it looks like MCKRDY_TIMEOUT set to 1000 is not enough, my board
> crashes in about 1 wake up to 10 with this value and works perfectly
> fine with 4000.
I also encountered this issue. 
Moreover the Main Crystal Oscillator Startup Time seems not enough too, not sure, I am confirming it.

> 
> 
> I have attached a patch that do the following:
> 
>  * Increase MCKRDY_TIMEOUT from 1000 to 4000 so my board don't crash.
>  * Fix the assembly on the "Turn on UTMI PLL" part
>  * Increase timeouts for PLLB and UPLL
> 
> 
> However I still disagree with this patch (this is why I didn't do a pull
> request ;-), I am proposing doing a patch set that:
> 
>  * Increase MCKRDY_TIMEOUT from 1000 to 4000 so my board don't crash, I
>    would be happy to have a feed back on this with your boards ;-)
>  * Fix the assembly on the "Turn on UTMI PLL" part
>  * Move back PLLB and UPLL to slow clock mode to save power
> 
> What do you think ?
> 
> 
> I have also mixed feeling about those timeouts, is it really useful in
> the wild ?  We don't event catch the timeout event to do something else
> if it happens, we still switch to the master clock whatever happened or
> even worse we continue even if AT91_PMC_MCKRDY isn't set? which is why I
> am having hard fault. In my opinion this is the watchdog job to handle
> those cases cleanly, there is nothing much we can do if something fail
> other than waiting a watchdog reset.
> 
> (Yeah, I know the watchdog is hard to use with sleep mode on AT91 chip,
> I have a work in progress about using the RTC to wake up before the
> watchdog expire to clear the watchdog and go back to sleep as fast as
> possible without resuming all the devices. It works, but I am still not
> happy at all with what I did for now.)
> 
> Therefore I am also proposing to remove all the timeout.
Good idea, but I still worry the deadlock without timeout. Maybe increasing timeout is better.

> 
> 
> By the way, I don't know for other AT91 boards (or without DT, or
> something else), but the UTMI PLL suspend/resume seem unnecessary for
> me, the USB driver is already disabling the USB PLL when suspending,
> could you confirm ?
Yes, I agree.
Remove disabling the PLLB code as well.  

> 
> 
> 
> Regards,
> Sylvain

Best Regards,
Wenyou Yang

[PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume

[Sylvain Rochet]

Assume USB PLL and PLL B are already stopped before entering sleep mode,
print a warning if this isn't the case.

Removed timeout on XTAL, PLL lock and Master Clock Ready, hang if
something went wrong instead of continuing in unknown condition. There
is not much we can do if a PLL lock never ends, we are running in SRAM
and we will not be able to connect back the sdram or ddram in order to
be able to fire up a message or just panic.

As a bonus, not decounting the timeout register in slow clock mode
reduce cumulated suspend time and resume time from ~17ms to ~15ms.
---
 arch/arm/mach-at91/pm.c           |  12 +++++
 arch/arm/mach-at91/pm_slowclock.S | 101 ++------------------------------------
 2 files changed, 16 insertions(+), 97 deletions(-)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index d213a00..3b0d804 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -168,6 +168,18 @@ static int at91_pm_verify_clocks(void)
 		}
 	}
 
+	/* Drivers should have previously suspended USB PLL */
+	if (at91_pmc_read(AT91_CKGR_UCKR) & AT91_PMC_UPLLEN) {
+		pr_err("AT91: PM - Suspend-to-RAM with USB PLL running\n");
+		return 0;
+	}
+
+	/* Drivers should have previously suspended PLL B */
+	if (at91_pmc_read(AT91_PMC_SR) & AT91_PMC_LOCKB) {
+		pr_err("AT91: PM - Suspend-to-RAM with PLL B running\n");
+		return 0;
+	}
+
 	return 1;
 }
 #endif
diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S
index 6194749..530b996 100644
--- a/arch/arm/mach-at91/pm_slowclock.S
+++ b/arch/arm/mach-at91/pm_slowclock.S
@@ -33,12 +33,6 @@
  */
 #undef SLOWDOWN_MASTER_CLOCK
 
-#define MCKRDY_TIMEOUT		1000
-#define MOSCRDY_TIMEOUT 	1000
-#define PLLALOCK_TIMEOUT	1000
-#define PLLBLOCK_TIMEOUT	1000
-#define UPLLLOCK_TIMEOUT	1000
-
 pmc	.req	r0
 sdramc	.req	r1
 ramc1	.req	r2
@@ -46,76 +40,32 @@ memctrl	.req	r3
 tmp1	.req	r4
 tmp2	.req	r5
 ddrcid	.req	r6
-flag	.req	r7
 
 /*
  * Wait until master clock is ready (after switching master clock source)
  */
 	.macro wait_mckrdy
-	mov	tmp2, #MCKRDY_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_MCKRDY
 	beq	1b
-2:
 	.endm
 
 /*
  * Wait until master oscillator has stabilized.
  */
 	.macro wait_moscrdy
-	mov	tmp2, #MOSCRDY_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_MOSCS
 	beq	1b
-2:
 	.endm
 
 /*
  * Wait until PLLA has locked.
  */
 	.macro wait_pllalock
-	mov	tmp2, #PLLALOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_LOCKA
 	beq	1b
-2:
-	.endm
-
-/*
- * Wait until PLLB has locked.
- */
-	.macro wait_pllblock
-	mov	tmp2, #PLLBLOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
-	tst	tmp1, #AT91_PMC_LOCKB
-	beq	1b
-2:
-	.endm
-
-/*
- * Wait until UTMI PLL has locked.
- */
-	.macro wait_uplllock
-	mov	tmp2, #UPLLLOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
-	tst	tmp1, #AT91_PMC_LOCKU
-	beq	1b
-2:
 	.endm
 
 /*
@@ -126,6 +76,7 @@ flag	.req	r7
 #if defined(CONFIG_CPU_V7)
 	dsb
 
+	/* Disable the processor clock */
 	mov	tmp1, #AT91_PMC_PCK
 	str	tmp1, [pmc, #AT91_PMC_SCDR]
 
@@ -234,25 +185,6 @@ sdr_sr_done:
 	mov 	tmp1, #AT91_PMC_SYS_DDR
 	str	tmp1, [pmc, #AT91_PMC_SCDR]
 
-	/* Save PLLB setting and disable it */
-	ldr	tmp1, [pmc, #AT91_CKGR_PLLBR]
-	str	tmp1, .saved_pllbr
-
-	mov	tmp1, #AT91_PMC_PLLCOUNT
-	str	tmp1, [pmc, #AT91_CKGR_PLLBR]
-
-	/* Disable UTMI PLL */
-	ldr	tmp1, [pmc, #AT91_CKGR_UCKR]
-	tst	tmp1, #AT91_PMC_UPLLEN
-	beq	1f
-	mov	flag, #1
-	bic	tmp1, tmp1, #AT91_PMC_UPLLEN
-	str	tmp1, [pmc, #AT91_CKGR_UCKR]
-	b	2f
-1:
-	mov	flag, #0
-2:
-
 	/* Save Master clock setting */
 	ldr	tmp1, [pmc, #AT91_PMC_MCKR]
 	str	tmp1, .saved_mckr
@@ -338,28 +270,6 @@ sdr_sr_done:
 
 	wait_mckrdy
 
-	/* Turn on UTMI PLL */
-	cmp	flag, #1
-	bne	1f
-	ldr	tmp1, [pmc, #AT91_CKGR_UCKR]
-	orr	tmp1, tmp1, #AT91_PMC_UPLLEN
-	str	tmp1, [pmc, #AT91_CKGR_UCKR]
-
-	wait_uplllock
-1:
-
-	/* Restore PLLB setting */
-	ldr	tmp1, .saved_pllbr
-	str	tmp1, [pmc, #AT91_CKGR_PLLBR]
-
-	tst	tmp1, #(AT91_PMC_MUL &  0xff0000)
-	bne	1f
-	tst	tmp1, #(AT91_PMC_MUL & ~0xff0000)
-	beq	2f
-1:
-	wait_pllblock
-2:
-
 	/* Enable MPDDRC Clock*/
 	cmp	ddrcid, #0
 	beq	4f
@@ -421,9 +331,6 @@ ram_restored:
 .saved_pllar:
 	.word 0
 
-.saved_pllbr:
-	.word 0
-
 .saved_sam9_lpr:
 	.word 0
 
-- 
2.1.3

AT91 slow clock mode regression/fixes, improvement proposal

[Sylvain Rochet]

Hello Wenyou,


On Fri, Dec 19, 2014 at 02:50:04AM +0000, Yang, Wenyou wrote:
> > 
> > Is actually jumping to wait_pllblock before enabling PLLB instead of skipping UTMI
> > PLL. Unfortunately the compiler does not warn about that.
> 
> Thanks. I will check it

Fixed in proposed patch (UPLL code removed).


> > Furthermore, it looks like MCKRDY_TIMEOUT set to 1000 is not enough, my board
> > crashes in about 1 wake up to 10 with this value and works perfectly
> > fine with 4000.
> 
> I also encountered this issue.

Fixed in proposed patch, since we don't timeout anymore.


> Moreover the Main Crystal Oscillator Startup Time seems not enough 
> too, not sure, I am confirming it.

This is OK for me, CKGR_MOR.MOSCXTST is set to 8:  (1/32768)*8*8 = 1.95 ms

I checked with a scope on XTAL of the AT91SAM9G35-CM module, 2 ms is 
about twice enough waiting time.


> > Therefore I am also proposing to remove all the timeout.
> 
> Good idea, but I still worry the deadlock without timeout. Maybe 
> increasing timeout is better.

This is just a test request patch, I am first trying without timeout to 
get some feedback. It works well for me, at least, but this isn't 
convicing myself enough :-)


> > By the way, I don't know for other AT91 boards (or without DT, or
> > something else), but the UTMI PLL suspend/resume seem unnecessary for
> > me, the USB driver is already disabling the USB PLL when suspending,
> > could you confirm ?
> Yes, I agree.

Removed.


> Remove disabling the PLLB code as well.  

Done, but my SoC don't have a PLLB, so I can't check myself, could you 
check if "AT91: PM - Suspend-to-RAM with PLL B running" does not fire on 
PLL B enabled board ?


Regards,
Sylvain

AT91 slow clock mode regression/fixes, improvement proposal

[Yang, Wenyou]

Hi Sylvain,

> 
> On Fri, Dec 19, 2014 at 02:50:04AM +0000, Yang, Wenyou wrote:
> > >
> > > Is actually jumping to wait_pllblock before enabling PLLB instead of
> > > skipping UTMI PLL. Unfortunately the compiler does not warn about that.
> >
> > Thanks. I will check it
> 
> Fixed in proposed patch (UPLL code removed).
> 
> 
> > > Furthermore, it looks like MCKRDY_TIMEOUT set to 1000 is not enough,
> > > my board crashes in about 1 wake up to 10 with this value and works
> > > perfectly fine with 4000.
> >
> > I also encountered this issue.
> 
> Fixed in proposed patch, since we don't timeout anymore.
I am verifying this patch. I still insist on remain the timeout.

> 
> 
> > Moreover the Main Crystal Oscillator Startup Time seems not enough
> > too, not sure, I am confirming it.
> 
> This is OK for me, CKGR_MOR.MOSCXTST is set to 8:  (1/32768)*8*8 = 1.95 ms
> 
> I checked with a scope on XTAL of the AT91SAM9G35-CM module, 2 ms is about
> twice enough waiting time.
Yes, you are right. Thanks.

> 
> 
> > > Therefore I am also proposing to remove all the timeout.
> >
> > Good idea, but I still worry the deadlock without timeout. Maybe
> > increasing timeout is better.
> 
> This is just a test request patch, I am first trying without timeout to
> get some feedback. It works well for me, at least, but this isn't
> convicing myself enough :-)
> 
> 
> > > By the way, I don't know for other AT91 boards (or without DT, or
> > > something else), but the UTMI PLL suspend/resume seem unnecessary for
> > > me, the USB driver is already disabling the USB PLL when suspending,
> > > could you confirm ?
> > Yes, I agree.
> 
> Removed.
> 
> 
> > Remove disabling the PLLB code as well.
> 
> Done, but my SoC don't have a PLLB, so I can't check myself, could you
> check if "AT91: PM - Suspend-to-RAM with PLL B running" does not fire on
> PLL B enabled board ?
I will check.

> 
> 
> Regards,
> Sylvain


Best Regard,
Wenyou Yang

AT91 slow clock mode regression/fixes, improvement proposal

[Sylvain Rochet]

Hello Wenyou,

On Mon, Dec 22, 2014 at 08:32:15AM +0000, Yang, Wenyou wrote:
> > On Fri, Dec 19, 2014 at 02:50:04AM +0000, Yang, Wenyou wrote:
> > 
> > > > Furthermore, it looks like MCKRDY_TIMEOUT set to 1000 is not enough,
> > > > my board crashes in about 1 wake up to 10 with this value and works
> > > > perfectly fine with 4000.
> > >
> > > I also encountered this issue.
> > 
> > Fixed in proposed patch, since we don't timeout anymore.
> 
> I am verifying this patch. I still insist on remain the timeout.

Master Clock Ready is a special case, you need a timeout value which is 
fine at slow clock and master clock, that is, a timeout value which is 
fine despite the 10^4 difference in order of magnitude. I just don't 
know what to do, a fine timeout value for master clock is an almost 
infinite timeout on slow clock.

By the way, my board suspended and resumed every 2 seconds this 
week-end, which is about 110k suspend+wake up cycles and didn't reset on 
watchdog, which is quite a good news :-)

Regards,
Sylvain

AT91 slow clock mode regression/fixes, improvement proposal

[Yang, Wenyou]

Hi Sylvain,

> > Furthermore, it looks like MCKRDY_TIMEOUT set to 1000 is not
> > > > enough, my board crashes in about 1 wake up to 10 with this value
> > > > and works perfectly fine with 4000.
> > >
> > > I also encountered this issue.
> >
> > Fixed in proposed patch, since we don't timeout anymore.
> I am verifying this patch. I still insist on remain the timeout.
>
Verified OK.

Thanks.

I remembered you said this patch is not formal. Now, would you like send a formal one?

Best Regards,
Wenyou Yang

[PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume

[Sylvain Rochet]

Assume USB PLL and PLL B are already stopped before entering sleep mode,
print a warning if this isn't the case.

Removed timeout on XTAL, PLL lock and Master Clock Ready, hang if
something went wrong instead of continuing in unknown condition. There
is not much we can do if a PLL lock never ends, we are running in SRAM
and we will not be able to connect back the sdram or ddram in order to
be able to fire up a message or just panic.

As a bonus, not decounting the timeout register in slow clock mode
reduce cumulated suspend time and resume time from ~17ms to ~15ms.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
 arch/arm/mach-at91/pm.c           |  12 +++++
 arch/arm/mach-at91/pm_slowclock.S | 101 ++------------------------------------
 2 files changed, 16 insertions(+), 97 deletions(-)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index d213a00..3b0d804 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -168,6 +168,18 @@ static int at91_pm_verify_clocks(void)
 		}
 	}
 
+	/* Drivers should have previously suspended USB PLL */
+	if (at91_pmc_read(AT91_CKGR_UCKR) & AT91_PMC_UPLLEN) {
+		pr_err("AT91: PM - Suspend-to-RAM with USB PLL running\n");
+		return 0;
+	}
+
+	/* Drivers should have previously suspended PLL B */
+	if (at91_pmc_read(AT91_PMC_SR) & AT91_PMC_LOCKB) {
+		pr_err("AT91: PM - Suspend-to-RAM with PLL B running\n");
+		return 0;
+	}
+
 	return 1;
 }
 #endif
diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S
index 6194749..530b996 100644
--- a/arch/arm/mach-at91/pm_slowclock.S
+++ b/arch/arm/mach-at91/pm_slowclock.S
@@ -33,12 +33,6 @@
  */
 #undef SLOWDOWN_MASTER_CLOCK
 
-#define MCKRDY_TIMEOUT		1000
-#define MOSCRDY_TIMEOUT 	1000
-#define PLLALOCK_TIMEOUT	1000
-#define PLLBLOCK_TIMEOUT	1000
-#define UPLLLOCK_TIMEOUT	1000
-
 pmc	.req	r0
 sdramc	.req	r1
 ramc1	.req	r2
@@ -46,76 +40,32 @@ memctrl	.req	r3
 tmp1	.req	r4
 tmp2	.req	r5
 ddrcid	.req	r6
-flag	.req	r7
 
 /*
  * Wait until master clock is ready (after switching master clock source)
  */
 	.macro wait_mckrdy
-	mov	tmp2, #MCKRDY_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_MCKRDY
 	beq	1b
-2:
 	.endm
 
 /*
  * Wait until master oscillator has stabilized.
  */
 	.macro wait_moscrdy
-	mov	tmp2, #MOSCRDY_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_MOSCS
 	beq	1b
-2:
 	.endm
 
 /*
  * Wait until PLLA has locked.
  */
 	.macro wait_pllalock
-	mov	tmp2, #PLLALOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_LOCKA
 	beq	1b
-2:
-	.endm
-
-/*
- * Wait until PLLB has locked.
- */
-	.macro wait_pllblock
-	mov	tmp2, #PLLBLOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
-	tst	tmp1, #AT91_PMC_LOCKB
-	beq	1b
-2:
-	.endm
-
-/*
- * Wait until UTMI PLL has locked.
- */
-	.macro wait_uplllock
-	mov	tmp2, #UPLLLOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
-	tst	tmp1, #AT91_PMC_LOCKU
-	beq	1b
-2:
 	.endm
 
 /*
@@ -126,6 +76,7 @@ flag	.req	r7
 #if defined(CONFIG_CPU_V7)
 	dsb
 
+	/* Disable the processor clock */
 	mov	tmp1, #AT91_PMC_PCK
 	str	tmp1, [pmc, #AT91_PMC_SCDR]
 
@@ -234,25 +185,6 @@ sdr_sr_done:
 	mov 	tmp1, #AT91_PMC_SYS_DDR
 	str	tmp1, [pmc, #AT91_PMC_SCDR]
 
-	/* Save PLLB setting and disable it */
-	ldr	tmp1, [pmc, #AT91_CKGR_PLLBR]
-	str	tmp1, .saved_pllbr
-
-	mov	tmp1, #AT91_PMC_PLLCOUNT
-	str	tmp1, [pmc, #AT91_CKGR_PLLBR]
-
-	/* Disable UTMI PLL */
-	ldr	tmp1, [pmc, #AT91_CKGR_UCKR]
-	tst	tmp1, #AT91_PMC_UPLLEN
-	beq	1f
-	mov	flag, #1
-	bic	tmp1, tmp1, #AT91_PMC_UPLLEN
-	str	tmp1, [pmc, #AT91_CKGR_UCKR]
-	b	2f
-1:
-	mov	flag, #0
-2:
-
 	/* Save Master clock setting */
 	ldr	tmp1, [pmc, #AT91_PMC_MCKR]
 	str	tmp1, .saved_mckr
@@ -338,28 +270,6 @@ sdr_sr_done:
 
 	wait_mckrdy
 
-	/* Turn on UTMI PLL */
-	cmp	flag, #1
-	bne	1f
-	ldr	tmp1, [pmc, #AT91_CKGR_UCKR]
-	orr	tmp1, tmp1, #AT91_PMC_UPLLEN
-	str	tmp1, [pmc, #AT91_CKGR_UCKR]
-
-	wait_uplllock
-1:
-
-	/* Restore PLLB setting */
-	ldr	tmp1, .saved_pllbr
-	str	tmp1, [pmc, #AT91_CKGR_PLLBR]
-
-	tst	tmp1, #(AT91_PMC_MUL &  0xff0000)
-	bne	1f
-	tst	tmp1, #(AT91_PMC_MUL & ~0xff0000)
-	beq	2f
-1:
-	wait_pllblock
-2:
-
 	/* Enable MPDDRC Clock*/
 	cmp	ddrcid, #0
 	beq	4f
@@ -421,9 +331,6 @@ ram_restored:
 .saved_pllar:
 	.word 0
 
-.saved_pllbr:
-	.word 0
-
 .saved_sam9_lpr:
 	.word 0
 
-- 
2.1.4

[PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume

[Sylvain Rochet]

Assume USB PLL and PLL B are already stopped before entering sleep mode,
print a warning if this isn't the case.

Removed timeout on XTAL, PLL lock and Master Clock Ready, hang if
something went wrong instead of continuing in unknown condition. There
is not much we can do if a PLL lock never ends, we are running in SRAM
and we will not be able to connect back the sdram or ddram in order to
be able to fire up a message or just panic.

As a bonus, not decounting the timeout register in slow clock mode
reduce cumulated suspend time and resume time from ~17ms to ~15ms.

Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
---
 arch/arm/mach-at91/pm.c           | 12 ++++++++
 arch/arm/mach-at91/pm_slowclock.S | 62 ++-------------------------------------
 2 files changed, 15 insertions(+), 59 deletions(-)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 9b15169..1cfd6e9 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -98,6 +98,18 @@ static int at91_pm_verify_clocks(void)
 		}
 	}
 
+	/* Drivers should have previously suspended USB PLL */
+	if (at91_pmc_read(AT91_CKGR_UCKR) & AT91_PMC_UPLLEN) {
+		pr_err("AT91: PM - Suspend-to-RAM with USB PLL running\n");
+		return 0;
+	}
+
+	/* Drivers should have previously suspended PLL B */
+	if (at91_pmc_read(AT91_PMC_SR) & AT91_PMC_LOCKB) {
+		pr_err("AT91: PM - Suspend-to-RAM with PLL B running\n");
+		return 0;
+	}
+
 	return 1;
 }
 
diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S
index 2001877..75d8b19 100644
--- a/arch/arm/mach-at91/pm_slowclock.S
+++ b/arch/arm/mach-at91/pm_slowclock.S
@@ -34,11 +34,6 @@
  */
 #undef SLOWDOWN_MASTER_CLOCK
 
-#define MCKRDY_TIMEOUT		1000
-#define MOSCRDY_TIMEOUT 	1000
-#define PLLALOCK_TIMEOUT	1000
-#define PLLBLOCK_TIMEOUT	1000
-
 pmc	.req	r0
 sdramc	.req	r1
 ramc1	.req	r2
@@ -50,56 +45,27 @@ tmp2	.req	r5
  * Wait until master clock is ready (after switching master clock source)
  */
 	.macro wait_mckrdy
-	mov	tmp2, #MCKRDY_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_MCKRDY
 	beq	1b
-2:
 	.endm
 
 /*
  * Wait until master oscillator has stabilized.
  */
 	.macro wait_moscrdy
-	mov	tmp2, #MOSCRDY_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_MOSCS
 	beq	1b
-2:
 	.endm
 
 /*
  * Wait until PLLA has locked.
  */
 	.macro wait_pllalock
-	mov	tmp2, #PLLALOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
+1:	ldr	tmp1, [pmc, #AT91_PMC_SR]
 	tst	tmp1, #AT91_PMC_LOCKA
 	beq	1b
-2:
-	.endm
-
-/*
- * Wait until PLLB has locked.
- */
-	.macro wait_pllblock
-	mov	tmp2, #PLLBLOCK_TIMEOUT
-1:	sub	tmp2, tmp2, #1
-	cmp	tmp2, #0
-	beq	2f
-	ldr	tmp1, [pmc, #AT91_PMC_SR]
-	tst	tmp1, #AT91_PMC_LOCKB
-	beq	1b
-2:
 	.endm
 
 	.text
@@ -207,13 +173,6 @@ sdr_sr_done:
 	orr	tmp1, tmp1, #(1 << 29)		/* bit 29 always set */
 	str	tmp1, [pmc, #AT91_CKGR_PLLAR]
 
-	/* Save PLLB setting and disable it */
-	ldr	tmp1, [pmc, #AT91_CKGR_PLLBR]
-	str	tmp1, .saved_pllbr
-
-	mov	tmp1, #AT91_PMC_PLLCOUNT
-	str	tmp1, [pmc, #AT91_CKGR_PLLBR]
-
 	/* Turn off the main oscillator */
 	ldr	tmp1, [pmc, #AT91_CKGR_MOR]
 	bic	tmp1, tmp1, #AT91_PMC_MOSCEN
@@ -229,18 +188,6 @@ sdr_sr_done:
 
 	wait_moscrdy
 
-	/* Restore PLLB setting */
-	ldr	tmp1, .saved_pllbr
-	str	tmp1, [pmc, #AT91_CKGR_PLLBR]
-
-	tst	tmp1, #(AT91_PMC_MUL &  0xff0000)
-	bne	1f
-	tst	tmp1, #(AT91_PMC_MUL & ~0xff0000)
-	beq	2f
-1:
-	wait_pllblock
-2:
-
 	/* Restore PLLA setting */
 	ldr	tmp1, .saved_pllar
 	str	tmp1, [pmc, #AT91_CKGR_PLLAR]
@@ -319,9 +266,6 @@ ram_restored:
 .saved_pllar:
 	.word 0
 
-.saved_pllbr:
-	.word 0
-
 .saved_sam9_lpr:
 	.word 0
 
-- 
2.1.4

AT91 slow clock mode regression/fixes, improvement proposal

[Sylvain Rochet]

Hello Wenyou,

On Mon, Jan 05, 2015 at 03:32:42AM +0000, Yang, Wenyou wrote:
> 
> I remembered you said this patch is not formal. Now, would you like 
> send a formal one?

Here it is, the first one is based on linux-at91/linux-3.10-at91 branch, 
the second one is rebased on linux-3.18 (Is this the right way when 
playing with multiple branch?).

By the way, Wenyou fixed some bugs in the linux-3.10-at91 branch about 
slow clock mode which should IMHO be merged and pushed to other 
branches.

Regards,
Sylvain

AT91 slow clock mode regression/fixes, improvement proposal

[Yang, Wenyou]

Hi Sylvain,

> -----Original Message-----
> From: Sylvain Rochet [mailto:sylvain.rochet at finsecur.com]
> Sent: Tuesday, January 06, 2015 10:25 PM
> To: Yang, Wenyou
> Cc: Ferre, Nicolas; Desroches, Ludovic; 'Alexandre Belloni'; 'Maxime Ripard';
> 'linux-arm-kernel at lists.infradead.org'
> Subject: Re: AT91 slow clock mode regression/fixes, improvement proposal
> 
> Hello Wenyou,
> 
> On Mon, Jan 05, 2015 at 03:32:42AM +0000, Yang, Wenyou wrote:
> >
> > I remembered you said this patch is not formal. Now, would you like
> > send a formal one?
> 
> Here it is, the first one is based on linux-at91/linux-3.10-at91 branch, the second
> one is rebased on linux-3.18 (Is this the right way when playing with multiple
> branch?).
> 
> By the way, Wenyou fixed some bugs in the linux-3.10-at91 branch about slow
> clock mode which should IMHO be merged and pushed to other branches.
Yes, I will push to the mainline after verification passed.

Thank you very much.

> 
> Regards,
> Sylvain

Best Regagrds,
Wenyou Yang

AT91 slow clock mode regression/fixes, improvement proposal

[Alexandre Belloni]

Hi,

On 06/01/2015 at 15:25:28 +0100, Sylvain Rochet wrote :
> Hello Wenyou,
> 
> On Mon, Jan 05, 2015 at 03:32:42AM +0000, Yang, Wenyou wrote:
> > 
> > I remembered you said this patch is not formal. Now, would you like 
> > send a formal one?
> 
> Here it is, the first one is based on linux-at91/linux-3.10-at91 branch, 
> the second one is rebased on linux-3.18 (Is this the right way when 
> playing with multiple branch?).

You can include that kind of information after the --- marker in the
patch so that it is clearer.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com