[PATCH] clk: vf610: don't stop enabled clocks in suspend

[PATCH] clk: vf610: don't stop enabled clocks in suspend

[Sergei Miroshnichenko]

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 <sergeimir@emcraft.com>
---
 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;
-- 
1.9.3

[PATCH] clk: vf610: don't stop enabled clocks in suspend

[Stefan Agner]

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 <sergeimir@emcraft.com>
> ---
>  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;

[PATCH] clk: vf610: don't stop enabled clocks in suspend

[Sergei Miroshnichenko]

Hi Stefan,

Thanks for the review!

Our goal is dynamic run-time configuration of wakeup sources in
userspace using standard sysfs API:

echo enabled/disabled > /sys/class/<deviceX>/power/wakeup

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.

What do you think about this?

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 <sergeimir@emcraft.com>
>> ---
>>  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;
> 

[PATCH] clk: vf610: don't stop enabled clocks in suspend

[Stefan Agner]

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/<deviceX>/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 <sergeimir@emcraft.com>
>>> ---
>>>  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;
>>

[PATCH] clk: vf610: don't stop enabled clocks in suspend

[Sergei Miroshnichenko]

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/<deviceX>/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 <sergeimir@emcraft.com>
>>>> ---
>>>>  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;
>>>