From patchwork Thu Dec 18 20:39:04 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Sylvain Rochet X-Patchwork-Id: 5515901 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id A0DB79F54F for ; Thu, 18 Dec 2014 20:42:01 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9CC51202F0 for ; Thu, 18 Dec 2014 20:42:00 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 41476203B7 for ; Thu, 18 Dec 2014 20:41:59 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1Y1hrK-0002uR-OP; Thu, 18 Dec 2014 20:39:42 +0000 Received: from mx-guillaumet.finsecur.com ([91.217.234.131] helo=guillaumet.finsecur.com) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Y1hrD-0002iy-Ez for linux-arm-kernel@lists.infradead.org; Thu, 18 Dec 2014 20:39:36 +0000 Received: from [172.16.8.13] (helo=spice.lan) by guillaumet.finsecur.com with esmtps (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1Y1hqi-0000E9-TT; Thu, 18 Dec 2014 21:39:05 +0100 Received: from gradator by spice.lan with local (Exim 4.84) (envelope-from ) id 1Y1hqi-00006R-Lk; Thu, 18 Dec 2014 21:39:04 +0100 Date: Thu, 18 Dec 2014 21:39:04 +0100 From: Sylvain Rochet To: Wenyou Yang , Nicolas Ferre , Ludovic Desroches , Alexandre Belloni , Maxime Ripard Message-ID: <20141218203904.GA25215@gradator.net> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: 172.16.8.13 X-SA-Exim-Mail-From: sylvain.rochet@finsecur.com X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Subject: AT91 slow clock mode regression/fixes, improvement proposal X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on guillaumet.finsecur.com) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20141218_123935_675234_E561227A X-CRM114-Status: GOOD ( 22.13 ) X-Spam-Score: -0.0 (/) Cc: linux-arm-kernel@lists.infradead.org X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP 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 From 5ba960969738091f5e0e9bd18090df7e64c39ace Mon Sep 17 00:00:00 2001 From: Sylvain Rochet 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