* AT91 slow clock mode regression/fixes, improvement proposal @ 2014-12-18 20:39 Sylvain Rochet 2014-12-18 21:23 ` Sylvain Rochet 2014-12-19 2:50 ` Yang, Wenyou 0 siblings, 2 replies; 13+ messages in thread From: Sylvain Rochet @ 2014-12-18 20:39 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2014-12-18 20:39 AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet @ 2014-12-18 21:23 ` Sylvain Rochet 2014-12-19 2:50 ` Yang, Wenyou 1 sibling, 0 replies; 13+ messages in thread From: Sylvain Rochet @ 2014-12-18 21:23 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2014-12-18 20:39 AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 2014-12-18 21:23 ` Sylvain Rochet @ 2014-12-19 2:50 ` Yang, Wenyou 2014-12-19 15:04 ` [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume Sylvain Rochet 2014-12-19 15:26 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 1 sibling, 2 replies; 13+ messages in thread From: Yang, Wenyou @ 2014-12-19 2:50 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume 2014-12-19 2:50 ` Yang, Wenyou @ 2014-12-19 15:04 ` Sylvain Rochet 2014-12-19 15:26 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 1 sibling, 0 replies; 13+ messages in thread From: Sylvain Rochet @ 2014-12-19 15:04 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2014-12-19 2:50 ` Yang, Wenyou 2014-12-19 15:04 ` [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume Sylvain Rochet @ 2014-12-19 15:26 ` Sylvain Rochet 2014-12-22 8:32 ` Yang, Wenyou 1 sibling, 1 reply; 13+ messages in thread From: Sylvain Rochet @ 2014-12-19 15:26 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2014-12-19 15:26 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet @ 2014-12-22 8:32 ` Yang, Wenyou 2014-12-22 10:03 ` Sylvain Rochet 2015-01-05 3:32 ` Yang, Wenyou 0 siblings, 2 replies; 13+ messages in thread From: Yang, Wenyou @ 2014-12-22 8:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2014-12-22 8:32 ` Yang, Wenyou @ 2014-12-22 10:03 ` Sylvain Rochet 2015-01-05 3:32 ` Yang, Wenyou 1 sibling, 0 replies; 13+ messages in thread From: Sylvain Rochet @ 2014-12-22 10:03 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2014-12-22 8:32 ` Yang, Wenyou 2014-12-22 10:03 ` Sylvain Rochet @ 2015-01-05 3:32 ` Yang, Wenyou 2015-01-06 14:15 ` [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume Sylvain Rochet ` (2 more replies) 1 sibling, 3 replies; 13+ messages in thread From: Yang, Wenyou @ 2015-01-05 3:32 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume 2015-01-05 3:32 ` Yang, Wenyou @ 2015-01-06 14:15 ` Sylvain Rochet 2015-01-06 14:16 ` Sylvain Rochet 2015-01-06 14:25 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 2 siblings, 0 replies; 13+ messages in thread From: Sylvain Rochet @ 2015-01-06 14:15 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume 2015-01-05 3:32 ` Yang, Wenyou 2015-01-06 14:15 ` [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume Sylvain Rochet @ 2015-01-06 14:16 ` Sylvain Rochet 2015-01-06 14:25 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 2 siblings, 0 replies; 13+ messages in thread From: Sylvain Rochet @ 2015-01-06 14:16 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2015-01-05 3:32 ` Yang, Wenyou 2015-01-06 14:15 ` [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume Sylvain Rochet 2015-01-06 14:16 ` Sylvain Rochet @ 2015-01-06 14:25 ` Sylvain Rochet 2015-01-07 1:17 ` Yang, Wenyou 2015-01-07 9:14 ` Alexandre Belloni 2 siblings, 2 replies; 13+ messages in thread From: Sylvain Rochet @ 2015-01-06 14:25 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2015-01-06 14:25 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet @ 2015-01-07 1:17 ` Yang, Wenyou 2015-01-07 9:14 ` Alexandre Belloni 1 sibling, 0 replies; 13+ messages in thread From: Yang, Wenyou @ 2015-01-07 1:17 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* AT91 slow clock mode regression/fixes, improvement proposal 2015-01-06 14:25 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 2015-01-07 1:17 ` Yang, Wenyou @ 2015-01-07 9:14 ` Alexandre Belloni 1 sibling, 0 replies; 13+ messages in thread From: Alexandre Belloni @ 2015-01-07 9:14 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-01-07 9:14 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-18 20:39 AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 2014-12-18 21:23 ` Sylvain Rochet 2014-12-19 2:50 ` Yang, Wenyou 2014-12-19 15:04 ` [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume Sylvain Rochet 2014-12-19 15:26 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 2014-12-22 8:32 ` Yang, Wenyou 2014-12-22 10:03 ` Sylvain Rochet 2015-01-05 3:32 ` Yang, Wenyou 2015-01-06 14:15 ` [PATCH] pm: at91: pm_slowclock: improve reliability of suspend/resume Sylvain Rochet 2015-01-06 14:16 ` Sylvain Rochet 2015-01-06 14:25 ` AT91 slow clock mode regression/fixes, improvement proposal Sylvain Rochet 2015-01-07 1:17 ` Yang, Wenyou 2015-01-07 9:14 ` Alexandre Belloni
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.