| Message ID | 20141218203904.GA25215@gradator.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi Sylvain, > -----Original Message----- > From: Sylvain Rochet [mailto:sylvain.rochet@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@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
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
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
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
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
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
Hi Sylvain, > -----Original Message----- > From: Sylvain Rochet [mailto:sylvain.rochet@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@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
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.
From 5ba960969738091f5e0e9bd18090df7e64c39ace Mon Sep 17 00:00:00 2001 From: Sylvain Rochet <sylvain.rochet@finsecur.com> Date: Thu, 18 Dec 2014 20:11:48 +0100 Subject: [PATCH] AT91/PM: slow clock mode fixes --- arch/arm/mach-at91/pm_slowclock.S | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/arch/arm/mach-at91/pm_slowclock.S b/arch/arm/mach-at91/pm_slowclock.S index 6194749..9cad710 100644 --- a/arch/arm/mach-at91/pm_slowclock.S +++ b/arch/arm/mach-at91/pm_slowclock.S @@ -33,11 +33,11 @@ */ #undef SLOWDOWN_MASTER_CLOCK -#define MCKRDY_TIMEOUT 1000 +#define MCKRDY_TIMEOUT 4000 #define MOSCRDY_TIMEOUT 1000 #define PLLALOCK_TIMEOUT 1000 -#define PLLBLOCK_TIMEOUT 1000 -#define UPLLLOCK_TIMEOUT 1000 +#define PLLBLOCK_TIMEOUT 15000000 +#define UPLLLOCK_TIMEOUT 15000000 pmc .req r0 sdramc .req r1 @@ -94,7 +94,7 @@ flag .req r7 * Wait until PLLB has locked. */ .macro wait_pllblock - mov tmp2, #PLLBLOCK_TIMEOUT + ldr tmp2, .pllblock_timeout 1: sub tmp2, tmp2, #1 cmp tmp2, #0 beq 2f @@ -108,7 +108,7 @@ flag .req r7 * Wait until UTMI PLL has locked. */ .macro wait_uplllock - mov tmp2, #UPLLLOCK_TIMEOUT + ldr tmp2, .uplllock_timeout 1: sub tmp2, tmp2, #1 cmp tmp2, #0 beq 2f @@ -340,13 +340,13 @@ sdr_sr_done: /* Turn on UTMI PLL */ cmp flag, #1 - bne 1f + bne 3f ldr tmp1, [pmc, #AT91_CKGR_UCKR] orr tmp1, tmp1, #AT91_PMC_UPLLEN str tmp1, [pmc, #AT91_CKGR_UCKR] wait_uplllock -1: +3: /* Restore PLLB setting */ ldr tmp1, .saved_pllbr @@ -430,5 +430,11 @@ ram_restored: .saved_sam9_lpr1: .word 0 +.pllblock_timeout: + .word PLLBLOCK_TIMEOUT + +.uplllock_timeout: + .word UPLLLOCK_TIMEOUT + ENTRY(at91_slow_clock_sz) .word .-at91_slow_clock -- 2.1.3