From mboxrd@z Thu Jan 1 00:00:00 1970 From: sergeimir@emcraft.com (Sergei Miroshnichenko) Date: Mon, 26 Oct 2015 10:07:20 +0300 Subject: [PATCH] clk: vf610: don't stop enabled clocks in suspend In-Reply-To: <1293e7f724caefa7c184536096e82e04@agner.ch> References: <1445007085-28250-1-git-send-email-sergeimir@emcraft.com> <95c4ad8c37f6f0a7429e8b1bdea17bb1@agner.ch> <56248541.3050904@emcraft.com> <1293e7f724caefa7c184536096e82e04@agner.ch> Message-ID: <562DD128.3060900@emcraft.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Stefan, Ok, in a couple of days I'll prepare a patchset with: - device_may_wakeup()-template for drivers we are working with; - declaration of the rest of Vybrid clocks; - patch from this thread; - initial platform code to support "freeze" low-power state for Vybrid. Best regards, Sergei On 10/24/2015 02:53 AM, Stefan Agner wrote: > Hi Sergei, > > On 2015-10-18 22:53, Sergei Miroshnichenko wrote: >> Our goal is dynamic run-time configuration of wakeup sources in >> userspace using standard sysfs API: >> >> echo enabled/disabled > /sys/class//power/wakeup > > Yes, that is exactly what I am talking about. In the tree I linked below > it is shown how I implemented this API for UART on Vybrid. > >> >> To implement wakeup support in drivers, the following template can be >> used in suspend()/resume() handlers: >> >> if (device_may_wakeup(...)) { >> enable_irq_wake(...);/disable_irq_wake(...); >> } else { >> clk_disable_unprepare(...);/clk_prepare_enable(...); >> } >> >> Next step will be declaring the rest of Vybrid clocks in >> drivers/clk/imx/clk-vf610.c (about 15 more). Those of them that aren't >> used by any driver, will be automatically turned off by kernel at >> startup (after all drivers are initialized) via clk_disable_unused(). >> >> With this approach, we utilize standard mechanism of waking up used all >> over the kernel, and make it configurable even from shell scripts. We've >> tested it with uart, gpio, rtc, spi, i2c, lan and mmc (SDIO): these >> changes are also planned to be sent upstream. > > That is exactly the point: As long as we don't have the template above > for all drivers upstream, we should still let the hardware take care. > >> >> What do you think about this? > > My suggestion is to use a "per clock" configuration in clk-vf610.c, as I > did it with UART with those two changes. This way we can make a slow > transition: > http://git.toradex.com/cgit/linux-toradex.git/commit/?h=toradex_vf_4.1-next&id=66e003c1ad2acdaa7d42e1113add7d44479392ab > http://git.toradex.com/cgit/linux-toradex.git/commit/?h=toradex_vf_4.1-next&id=fc60861b91c6585740e9a502fa4008b2f4694b22 > > Or we make sure all drivers really disable clocks correctly on suspend, > and then introduce your change... > > -- > Stefan > >> >> Best regards, >> Sergei >> >> On 10/16/2015 10:46 PM, Stefan Agner wrote: >>> Hi Sergei, >>> >>> If all peripherals which do not act as wakeup source disable clocks >>> explicily, this would be the right approach. However, currently not all >>> of them do, hence this patch will raise the energy consumption during >>> sleep modes. The peripherals of course rely on the clock driver to >>> restore the state (gates) at wake-up time, but I think that is how it >>> works for most SoCs anyway. >>> >>> In the Toradex branch I came up with a solution which allows to >>> selectively define which clock should be disabled and which should be >>> left on by the hardware: >>> http://git.toradex.com/cgit/linux-toradex.git/commit/?h=toradex_vf_4.1-next&id=66e003c1ad2acdaa7d42e1113add7d44479392ab >>> >>> I then started to fix some peripherals so that they explicitly disable >>> clock if wakeup is not enabled. I changed those clocks so they do not >>> get automatically disabled: >>> http://git.toradex.com/cgit/linux-toradex.git/commit/?h=toradex_vf_4.1-next&id=fc60861b91c6585740e9a502fa4008b2f4694b22 >>> >>> I plan to upstream those changes but did not came around to put >>> toghether a rebased and cleaned up patchset so far. >>> >>> -- >>> Stefan >>> >>> On 2015-10-16 07:51, Sergei Miroshnichenko wrote: >>>> Currently Vybrid Clock Gating is configured to turn off all clocks in stop >>>> mode, while kernel assumes them enabled unless explicitly disabled. This >>>> behavior prevents waking up from wakeup sources. >>>> >>>> Fix this by replacing Clock Gating Register bitmask 3 (Clock is on during >>>> all modes, except stop mode) with bitmask 2 (Clock is on during all modes, >>>> even stop mode): refer to Vybrid Reference Manual Chapter 10 "Clock >>>> Controller Module (CCM)". >>>> >>>> Tested on a VF610-based board. >>>> >>>> Signed-off-by: Sergei Miroshnichenko >>>> --- >>>> drivers/clk/imx/clk-gate2.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c >>>> index 8935bff..ac35d75 100644 >>>> --- a/drivers/clk/imx/clk-gate2.c >>>> +++ b/drivers/clk/imx/clk-gate2.c >>>> @@ -50,7 +50,8 @@ static int clk_gate2_enable(struct clk_hw *hw) >>>> goto out; >>>> >>>> reg = readl(gate->reg); >>>> - reg |= 3 << gate->bit_idx; >>>> + reg &= ~(3 << gate->bit_idx); >>>> + reg |= 2 << gate->bit_idx; >>>> writel(reg, gate->reg); >>>> >>>> out: >>>> @@ -86,7 +87,7 @@ static int clk_gate2_reg_is_enabled(void __iomem >>>> *reg, u8 bit_idx) >>>> { >>>> u32 val = readl(reg); >>>> >>>> - if (((val >> bit_idx) & 1) == 1) >>>> + if (((val >> bit_idx) & 3) == 2) >>>> return 1; >>>> >>>> return 0; >>>