[v2,2/3] ARM: keystone: pm: switch to use generic pm domains

Commit Message

Grygorii Strashko Oct. 20, 2014, 12:56 p.m. UTC

This patch switches Keystone 2 PM code to use Generic PM domains
instead of PM clock domains because of the lack of DT support
for the last.

Reviewed-by: Kevin Hilman <[e-mail]>
Signed-off-by: Grygorii Strashko <[e-mail]>
---
 .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
 arch/arm/mach-keystone/Kconfig                     |   1 +
 arch/arm/mach-keystone/pm_domain.c                 | 112 ++++++++++++++-------
 3 files changed, 107 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt

Comments

Santosh Shilimkar Oct. 21, 2014, 6:05 p.m. UTC | #1

On 10/20/2014 05:56 AM, Grygorii Strashko wrote:
> This patch switches Keystone 2 PM code to use Generic PM domains
> instead of PM clock domains because of the lack of DT support
> for the last.
>
> Reviewed-by: Kevin Hilman <[e-mail]>
> Signed-off-by: Grygorii Strashko <[e-mail]>
> ---
>   .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
>   arch/arm/mach-keystone/Kconfig                     |   1 +
>   arch/arm/mach-keystone/pm_domain.c                 | 112 ++++++++++++++-------
>   3 files changed, 107 insertions(+), 37 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>
> diff --git a/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
> new file mode 100644
> index 0000000..4bbf2aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
> @@ -0,0 +1,31 @@
> +* TI Keystone 2 Generic PM Controller
> +
> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
> +for each controlled IP module.
> +
> +Required properties:
> +- compatible: Should be "ti,keystone-powerdomain"
> +- #power-domain-cells: Should be 0, see below:
> +
> +The gpc node is a power-controller as documented by the generic power domain
You renamed gpc but missed to fix the comment ? Pls update it.

> +bindings in Documentation/devicetree/bindings/power/power_domain.txt.
> +
> +Example:
> +
> +	pm_controller: pm-controller {
> +		compatible = "ti,keystone-powerdomain";
> +		#power-domain-cells = <0>;
> +	};
> +
> +	netcp: netcp@2090000 {
> +		reg = <0x2620110 0x8>;
> +		reg-names = "efuse";
> +		...
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +		power-domains = <&pm_controller>;
> +
> +		clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
> +		dma-coherent;
> +	}
> diff --git a/arch/arm/mach-keystone/Kconfig b/arch/arm/mach-keystone/Kconfig
> index 98a156a..de43107 100644
> --- a/arch/arm/mach-keystone/Kconfig
> +++ b/arch/arm/mach-keystone/Kconfig
> @@ -9,6 +9,7 @@ config ARCH_KEYSTONE
>   	select COMMON_CLK_KEYSTONE
>   	select ARCH_SUPPORTS_BIG_ENDIAN
>   	select ZONE_DMA if ARM_LPAE
> +	select PM_GENERIC_DOMAINS if PM
>   	help
>   	  Support for boards based on the Texas Instruments Keystone family of
>   	  SoCs.
> diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
> index ca79dda..d58759d 100644
> --- a/arch/arm/mach-keystone/pm_domain.c
> +++ b/arch/arm/mach-keystone/pm_domain.c
> @@ -12,69 +12,107 @@
>    * version 2, as published by the Free Software Foundation.
>    */
>
> +#include <linux/clk.h>
>   #include <linux/init.h>
> -#include <linux/pm_runtime.h>
>   #include <linux/pm_clock.h>
> +#include <linux/pm_domain.h>
>   #include <linux/platform_device.h>
> -#include <linux/clk-provider.h>
>   #include <linux/of.h>
>
> -#ifdef CONFIG_PM_RUNTIME
> -static int keystone_pm_runtime_suspend(struct device *dev)
> +#ifdef CONFIG_PM_GENERIC_DOMAINS
> +
> +struct keystone_domain {
> +	struct generic_pm_domain genpd;
> +	struct device	*dev;
> +};
> +
> +void keystone_pm_domain_attach_dev(struct device *dev)
>   {
> +	struct clk *clk;
>   	int ret;
> +	int i = 0;
>
>   	dev_dbg(dev, "%s\n", __func__);
>
> -	ret = pm_generic_runtime_suspend(dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = pm_clk_suspend(dev);
> +	ret = pm_clk_create(dev);
>   	if (ret) {
> -		pm_generic_runtime_resume(dev);
> -		return ret;
> +		dev_err(dev, "pm_clk_create failed %d\n", ret);
> +		return;
> +	};
> +
> +	while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
> +		ret = pm_clk_add_clk(dev, clk);
> +		if (ret) {
> +			dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
> +			goto clk_err;
> +		};
>   	}
>
> -	return 0;
> +	if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
Can we not okkup two seperate callbacks instead of above check ?
I don't like this CONFIG check here. Its slightly better version of
ifdef in middle of the code.

Regards,
Santosh

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grygorii Strashko Oct. 22, 2014, 11:23 a.m. UTC | #2

Hi Santosh,

On 10/21/2014 09:05 PM, Santosh Shilimkar wrote:
> On 10/20/2014 05:56 AM, Grygorii Strashko wrote:
>> This patch switches Keystone 2 PM code to use Generic PM domains
>> instead of PM clock domains because of the lack of DT support
>> for the last.
>>
>> Reviewed-by: Kevin Hilman <[e-mail]>
>> Signed-off-by: Grygorii Strashko <[e-mail]>
>> ---
>>   .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
>>   arch/arm/mach-keystone/Kconfig                     |   1 +
>>   arch/arm/mach-keystone/pm_domain.c                 | 112 
>> ++++++++++++++-------
>>   3 files changed, 107 insertions(+), 37 deletions(-)
>>   create mode 100644 
>> Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt 
>> b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>> new file mode 100644
>> index 0000000..4bbf2aa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>> @@ -0,0 +1,31 @@
>> +* TI Keystone 2 Generic PM Controller
>> +
>> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
>> +for each controlled IP module.
>> +
>> +Required properties:
>> +- compatible: Should be "ti,keystone-powerdomain"
>> +- #power-domain-cells: Should be 0, see below:
>> +
>> +The gpc node is a power-controller as documented by the generic power 
>> domain
> You renamed gpc but missed to fix the comment ? Pls update it.

ok.

> 
>> +bindings in Documentation/devicetree/bindings/power/power_domain.txt.
>> +
>> +Example:
>> +
>> +    pm_controller: pm-controller {
>> +        compatible = "ti,keystone-powerdomain";
>> +        #power-domain-cells = <0>;
>> +    };
>> +
>> +    netcp: netcp@2090000 {
>> +        reg = <0x2620110 0x8>;
>> +        reg-names = "efuse";
>> +        ...
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges;
>> +        power-domains = <&pm_controller>;
>> +
>> +        clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
>> +        dma-coherent;
>> +    }
>> diff --git a/arch/arm/mach-keystone/Kconfig 
>> b/arch/arm/mach-keystone/Kconfig
>> index 98a156a..de43107 100644
>> --- a/arch/arm/mach-keystone/Kconfig
>> +++ b/arch/arm/mach-keystone/Kconfig
>> @@ -9,6 +9,7 @@ config ARCH_KEYSTONE
>>       select COMMON_CLK_KEYSTONE
>>       select ARCH_SUPPORTS_BIG_ENDIAN
>>       select ZONE_DMA if ARM_LPAE
>> +    select PM_GENERIC_DOMAINS if PM
>>       help
>>         Support for boards based on the Texas Instruments Keystone 
>> family of
>>         SoCs.
>> diff --git a/arch/arm/mach-keystone/pm_domain.c 
>> b/arch/arm/mach-keystone/pm_domain.c
>> index ca79dda..d58759d 100644
>> --- a/arch/arm/mach-keystone/pm_domain.c
>> +++ b/arch/arm/mach-keystone/pm_domain.c
>> @@ -12,69 +12,107 @@
>>    * version 2, as published by the Free Software Foundation.
>>    */
>>
>> +#include <linux/clk.h>
>>   #include <linux/init.h>
>> -#include <linux/pm_runtime.h>
>>   #include <linux/pm_clock.h>
>> +#include <linux/pm_domain.h>
>>   #include <linux/platform_device.h>
>> -#include <linux/clk-provider.h>
>>   #include <linux/of.h>
>>
>> -#ifdef CONFIG_PM_RUNTIME
>> -static int keystone_pm_runtime_suspend(struct device *dev)
>> +#ifdef CONFIG_PM_GENERIC_DOMAINS
>> +
>> +struct keystone_domain {
>> +    struct generic_pm_domain genpd;
>> +    struct device    *dev;
>> +};
>> +
>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>   {
>> +    struct clk *clk;
>>       int ret;
>> +    int i = 0;
>>
>>       dev_dbg(dev, "%s\n", __func__);
>>
>> -    ret = pm_generic_runtime_suspend(dev);
>> -    if (ret)
>> -        return ret;
>> -
>> -    ret = pm_clk_suspend(dev);
>> +    ret = pm_clk_create(dev);
>>       if (ret) {
>> -        pm_generic_runtime_resume(dev);
>> -        return ret;
>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>> +        return;
>> +    };
>> +
>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>> +        ret = pm_clk_add_clk(dev, clk);
>> +        if (ret) {
>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>> +            goto clk_err;
>> +        };
>>       }
>>
>> -    return 0;
>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
> Can we not okkup two seperate callbacks instead of above check ?
> I don't like this CONFIG check here. Its slightly better version of
> ifdef in middle of the code.

I've found more-less similar comment on patch
"Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
https://lkml.org/lkml/2014/10/17/257

So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
in case !IS_ENABLED(CONFIG_PM_RUNTIME)

regards,
-grygorii
 

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson Oct. 22, 2014, 3:01 p.m. UTC | #3

On 22 October 2014 13:23, Grygorii Strashko <[e-mail]> wrote:
> Hi Santosh,
>
> On 10/21/2014 09:05 PM, Santosh Shilimkar wrote:
>> On 10/20/2014 05:56 AM, Grygorii Strashko wrote:
>>> This patch switches Keystone 2 PM code to use Generic PM domains
>>> instead of PM clock domains because of the lack of DT support
>>> for the last.
>>>
>>> Reviewed-by: Kevin Hilman <[e-mail]>
>>> Signed-off-by: Grygorii Strashko <[e-mail]>
>>> ---
>>>   .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
>>>   arch/arm/mach-keystone/Kconfig                     |   1 +
>>>   arch/arm/mach-keystone/pm_domain.c                 | 112
>>> ++++++++++++++-------
>>>   3 files changed, 107 insertions(+), 37 deletions(-)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>> b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>> new file mode 100644
>>> index 0000000..4bbf2aa
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>> @@ -0,0 +1,31 @@
>>> +* TI Keystone 2 Generic PM Controller
>>> +
>>> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
>>> +for each controlled IP module.
>>> +
>>> +Required properties:
>>> +- compatible: Should be "ti,keystone-powerdomain"
>>> +- #power-domain-cells: Should be 0, see below:
>>> +
>>> +The gpc node is a power-controller as documented by the generic power
>>> domain
>> You renamed gpc but missed to fix the comment ? Pls update it.
>
> ok.
>
>>
>>> +bindings in Documentation/devicetree/bindings/power/power_domain.txt.
>>> +
>>> +Example:
>>> +
>>> +    pm_controller: pm-controller {
>>> +        compatible = "ti,keystone-powerdomain";
>>> +        #power-domain-cells = <0>;
>>> +    };
>>> +
>>> +    netcp: netcp@2090000 {
>>> +        reg = <0x2620110 0x8>;
>>> +        reg-names = "efuse";
>>> +        ...
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges;
>>> +        power-domains = <&pm_controller>;
>>> +
>>> +        clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
>>> +        dma-coherent;
>>> +    }
>>> diff --git a/arch/arm/mach-keystone/Kconfig
>>> b/arch/arm/mach-keystone/Kconfig
>>> index 98a156a..de43107 100644
>>> --- a/arch/arm/mach-keystone/Kconfig
>>> +++ b/arch/arm/mach-keystone/Kconfig
>>> @@ -9,6 +9,7 @@ config ARCH_KEYSTONE
>>>       select COMMON_CLK_KEYSTONE
>>>       select ARCH_SUPPORTS_BIG_ENDIAN
>>>       select ZONE_DMA if ARM_LPAE
>>> +    select PM_GENERIC_DOMAINS if PM
>>>       help
>>>         Support for boards based on the Texas Instruments Keystone
>>> family of
>>>         SoCs.
>>> diff --git a/arch/arm/mach-keystone/pm_domain.c
>>> b/arch/arm/mach-keystone/pm_domain.c
>>> index ca79dda..d58759d 100644
>>> --- a/arch/arm/mach-keystone/pm_domain.c
>>> +++ b/arch/arm/mach-keystone/pm_domain.c
>>> @@ -12,69 +12,107 @@
>>>    * version 2, as published by the Free Software Foundation.
>>>    */
>>>
>>> +#include <linux/clk.h>
>>>   #include <linux/init.h>
>>> -#include <linux/pm_runtime.h>
>>>   #include <linux/pm_clock.h>
>>> +#include <linux/pm_domain.h>
>>>   #include <linux/platform_device.h>
>>> -#include <linux/clk-provider.h>
>>>   #include <linux/of.h>
>>>
>>> -#ifdef CONFIG_PM_RUNTIME
>>> -static int keystone_pm_runtime_suspend(struct device *dev)
>>> +#ifdef CONFIG_PM_GENERIC_DOMAINS
>>> +
>>> +struct keystone_domain {
>>> +    struct generic_pm_domain genpd;
>>> +    struct device    *dev;
>>> +};
>>> +
>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>   {
>>> +    struct clk *clk;
>>>       int ret;
>>> +    int i = 0;
>>>
>>>       dev_dbg(dev, "%s\n", __func__);
>>>
>>> -    ret = pm_generic_runtime_suspend(dev);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>> -    ret = pm_clk_suspend(dev);
>>> +    ret = pm_clk_create(dev);
>>>       if (ret) {
>>> -        pm_generic_runtime_resume(dev);
>>> -        return ret;
>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>> +        return;
>>> +    };
>>> +
>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>> +        ret = pm_clk_add_clk(dev, clk);
>>> +        if (ret) {
>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>> +            goto clk_err;
>>> +        };
>>>       }
>>>
>>> -    return 0;
>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>> Can we not okkup two seperate callbacks instead of above check ?
>> I don't like this CONFIG check here. Its slightly better version of
>> ifdef in middle of the code.
>
> I've found more-less similar comment on patch
> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
> https://lkml.org/lkml/2014/10/17/257
>
> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
> in case !IS_ENABLED(CONFIG_PM_RUNTIME)

I am wondering whether we actually should/could do this, no matter of
CONFIG_PM_RUNTIME.

Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
clocks through pm_clk_suspend(), will be gated once the device becomes
runtime PM suspended. Right?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven Oct. 22, 2014, 3:09 p.m. UTC | #4

Hi Ulf,

On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>   {
>>>> +    struct clk *clk;
>>>>       int ret;
>>>> +    int i = 0;
>>>>
>>>>       dev_dbg(dev, "%s\n", __func__);
>>>>
>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>> -    if (ret)
>>>> -        return ret;
>>>> -
>>>> -    ret = pm_clk_suspend(dev);
>>>> +    ret = pm_clk_create(dev);
>>>>       if (ret) {
>>>> -        pm_generic_runtime_resume(dev);
>>>> -        return ret;
>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>> +        return;
>>>> +    };
>>>> +
>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>> +            goto clk_err;
>>>> +        };
>>>>       }
>>>>
>>>> -    return 0;
>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>> Can we not okkup two seperate callbacks instead of above check ?
>>> I don't like this CONFIG check here. Its slightly better version of
>>> ifdef in middle of the code.
>>
>> I've found more-less similar comment on patch
>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>> https://lkml.org/lkml/2014/10/17/257
>>
>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>
> I am wondering whether we actually should/could do this, no matter of
> CONFIG_PM_RUNTIME.
>
> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
> clocks through pm_clk_suspend(), will be gated once the device becomes
> runtime PM suspended. Right?

Doing it unconditionally means we'll have lots of unneeded clocks running
for a short while.

Are you trying to repeat power-up-all-PM-domains-during-boot for
clocks, too? ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson Oct. 22, 2014, 3:28 p.m. UTC | #5

On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
> Hi Ulf,
>
> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>>   {
>>>>> +    struct clk *clk;
>>>>>       int ret;
>>>>> +    int i = 0;
>>>>>
>>>>>       dev_dbg(dev, "%s\n", __func__);
>>>>>
>>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>>> -    if (ret)
>>>>> -        return ret;
>>>>> -
>>>>> -    ret = pm_clk_suspend(dev);
>>>>> +    ret = pm_clk_create(dev);
>>>>>       if (ret) {
>>>>> -        pm_generic_runtime_resume(dev);
>>>>> -        return ret;
>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>>> +        return;
>>>>> +    };
>>>>> +
>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>>> +        if (ret) {
>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>>> +            goto clk_err;
>>>>> +        };
>>>>>       }
>>>>>
>>>>> -    return 0;
>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>>> Can we not okkup two seperate callbacks instead of above check ?
>>>> I don't like this CONFIG check here. Its slightly better version of
>>>> ifdef in middle of the code.
>>>
>>> I've found more-less similar comment on patch
>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>>> https://lkml.org/lkml/2014/10/17/257
>>>
>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>>
>> I am wondering whether we actually should/could do this, no matter of
>> CONFIG_PM_RUNTIME.
>>
>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
>> clocks through pm_clk_suspend(), will be gated once the device becomes
>> runtime PM suspended. Right?
>
> Doing it unconditionally means we'll have lots of unneeded clocks running
> for a short while.
>
> Are you trying to repeat power-up-all-PM-domains-during-boot for
> clocks, too? ;-)

This is related, but there are a difference. :-)

As long as the pm_clk_add() is being invoked from the ->attach_dev()
callback, we are in the probe path. Certainly we would like to have
clocks enabled while probing, don't you think?

If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
those be enabled?

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Geert Uytterhoeven Oct. 22, 2014, 3:44 p.m. UTC | #6

On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <[e-mail]> wrote:
> On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>>>   {
>>>>>> +    struct clk *clk;
>>>>>>       int ret;
>>>>>> +    int i = 0;
>>>>>>
>>>>>>       dev_dbg(dev, "%s\n", __func__);
>>>>>>
>>>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>>>> -    if (ret)
>>>>>> -        return ret;
>>>>>> -
>>>>>> -    ret = pm_clk_suspend(dev);
>>>>>> +    ret = pm_clk_create(dev);
>>>>>>       if (ret) {
>>>>>> -        pm_generic_runtime_resume(dev);
>>>>>> -        return ret;
>>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>>>> +        return;
>>>>>> +    };
>>>>>> +
>>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>>>> +        if (ret) {
>>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>>>> +            goto clk_err;
>>>>>> +        };
>>>>>>       }
>>>>>>
>>>>>> -    return 0;
>>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>>>> Can we not okkup two seperate callbacks instead of above check ?
>>>>> I don't like this CONFIG check here. Its slightly better version of
>>>>> ifdef in middle of the code.
>>>>
>>>> I've found more-less similar comment on patch
>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>>>> https://lkml.org/lkml/2014/10/17/257
>>>>
>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>>>
>>> I am wondering whether we actually should/could do this, no matter of
>>> CONFIG_PM_RUNTIME.
>>>
>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
>>> clocks through pm_clk_suspend(), will be gated once the device becomes
>>> runtime PM suspended. Right?
>>
>> Doing it unconditionally means we'll have lots of unneeded clocks running
>> for a short while.

> As long as the pm_clk_add() is being invoked from the ->attach_dev()
> callback, we are in the probe path. Certainly we would like to have
> clocks enabled while probing, don't you think?
>
> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
> those be enabled?

They will be enabled when the driver does

        pm_runtime_enable(dev);
        pm_runtime_get_sync(dev);

in its .probe() method.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kevin Hilman Oct. 22, 2014, 3:58 p.m. UTC | #7

Grygorii Strashko <[e-mail]> writes:

> Hi Santosh,
>
> On 10/21/2014 09:05 PM, Santosh Shilimkar wrote:
>> On 10/20/2014 05:56 AM, Grygorii Strashko wrote:
>>> This patch switches Keystone 2 PM code to use Generic PM domains
>>> instead of PM clock domains because of the lack of DT support
>>> for the last.
>>>
>>> Reviewed-by: Kevin Hilman <[e-mail]>
>>> Signed-off-by: Grygorii Strashko <[e-mail]>
>>> ---
>>>   .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
>>>   arch/arm/mach-keystone/Kconfig                     |   1 +
>>>   arch/arm/mach-keystone/pm_domain.c                 | 112 
>>> ++++++++++++++-------
>>>   3 files changed, 107 insertions(+), 37 deletions(-)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt 
>>> b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>> new file mode 100644
>>> index 0000000..4bbf2aa
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>> @@ -0,0 +1,31 @@
>>> +* TI Keystone 2 Generic PM Controller
>>> +
>>> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
>>> +for each controlled IP module.
>>> +
>>> +Required properties:
>>> +- compatible: Should be "ti,keystone-powerdomain"
>>> +- #power-domain-cells: Should be 0, see below:
>>> +
>>> +The gpc node is a power-controller as documented by the generic power 
>>> domain
>> You renamed gpc but missed to fix the comment ? Pls update it.
>
> ok.
>
>> 
>>> +bindings in Documentation/devicetree/bindings/power/power_domain.txt.
>>> +
>>> +Example:
>>> +
>>> +    pm_controller: pm-controller {
>>> +        compatible = "ti,keystone-powerdomain";
>>> +        #power-domain-cells = <0>;
>>> +    };
>>> +
>>> +    netcp: netcp@2090000 {
>>> +        reg = <0x2620110 0x8>;
>>> +        reg-names = "efuse";
>>> +        ...
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges;
>>> +        power-domains = <&pm_controller>;
>>> +
>>> +        clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
>>> +        dma-coherent;
>>> +    }
>>> diff --git a/arch/arm/mach-keystone/Kconfig 
>>> b/arch/arm/mach-keystone/Kconfig
>>> index 98a156a..de43107 100644
>>> --- a/arch/arm/mach-keystone/Kconfig
>>> +++ b/arch/arm/mach-keystone/Kconfig
>>> @@ -9,6 +9,7 @@ config ARCH_KEYSTONE
>>>       select COMMON_CLK_KEYSTONE
>>>       select ARCH_SUPPORTS_BIG_ENDIAN
>>>       select ZONE_DMA if ARM_LPAE
>>> +    select PM_GENERIC_DOMAINS if PM
>>>       help
>>>         Support for boards based on the Texas Instruments Keystone 
>>> family of
>>>         SoCs.
>>> diff --git a/arch/arm/mach-keystone/pm_domain.c 
>>> b/arch/arm/mach-keystone/pm_domain.c
>>> index ca79dda..d58759d 100644
>>> --- a/arch/arm/mach-keystone/pm_domain.c
>>> +++ b/arch/arm/mach-keystone/pm_domain.c
>>> @@ -12,69 +12,107 @@
>>>    * version 2, as published by the Free Software Foundation.
>>>    */
>>>
>>> +#include <linux/clk.h>
>>>   #include <linux/init.h>
>>> -#include <linux/pm_runtime.h>
>>>   #include <linux/pm_clock.h>
>>> +#include <linux/pm_domain.h>
>>>   #include <linux/platform_device.h>
>>> -#include <linux/clk-provider.h>
>>>   #include <linux/of.h>
>>>
>>> -#ifdef CONFIG_PM_RUNTIME
>>> -static int keystone_pm_runtime_suspend(struct device *dev)
>>> +#ifdef CONFIG_PM_GENERIC_DOMAINS
>>> +
>>> +struct keystone_domain {
>>> +    struct generic_pm_domain genpd;
>>> +    struct device    *dev;
>>> +};
>>> +
>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>   {
>>> +    struct clk *clk;
>>>       int ret;
>>> +    int i = 0;
>>>
>>>       dev_dbg(dev, "%s\n", __func__);
>>>
>>> -    ret = pm_generic_runtime_suspend(dev);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>> -    ret = pm_clk_suspend(dev);
>>> +    ret = pm_clk_create(dev);
>>>       if (ret) {
>>> -        pm_generic_runtime_resume(dev);
>>> -        return ret;
>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>> +        return;
>>> +    };
>>> +
>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>> +        ret = pm_clk_add_clk(dev, clk);
>>> +        if (ret) {
>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>> +            goto clk_err;
>>> +        };
>>>       }
>>>
>>> -    return 0;
>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>> Can we not okkup two seperate callbacks instead of above check ?
>> I don't like this CONFIG check here. Its slightly better version of
>> ifdef in middle of the code.
>
> I've found more-less similar comment on patch
> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
> https://lkml.org/lkml/2014/10/17/257
>
> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
> in case !IS_ENABLED(CONFIG_PM_RUNTIME)

Yes, I think it's a good idea to propose that change and propose to
Rafael on linux-pm.  Be sure that myself, Ulf and Geert are Cc'd.

Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Santosh Shilimkar Oct. 22, 2014, 6:49 p.m. UTC | #8

On 10/22/2014 08:58 AM, Kevin Hilman wrote:
> Grygorii Strashko <[e-mail]> writes:
>
>> Hi Santosh,
>>
>> On 10/21/2014 09:05 PM, Santosh Shilimkar wrote:
>>> On 10/20/2014 05:56 AM, Grygorii Strashko wrote:
>>>> This patch switches Keystone 2 PM code to use Generic PM domains
>>>> instead of PM clock domains because of the lack of DT support
>>>> for the last.
>>>>
>>>> Reviewed-by: Kevin Hilman <[e-mail]>
>>>> Signed-off-by: Grygorii Strashko <[e-mail]>
>>>> ---
>>>>    .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
>>>>    arch/arm/mach-keystone/Kconfig                     |   1 +
>>>>    arch/arm/mach-keystone/pm_domain.c                 | 112
>>>> ++++++++++++++-------
>>>>    3 files changed, 107 insertions(+), 37 deletions(-)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>>>>

[..]

>>>> diff --git a/arch/arm/mach-keystone/pm_domain.c
>>>> b/arch/arm/mach-keystone/pm_domain.c
>>>> index ca79dda..d58759d 100644
>>>> --- a/arch/arm/mach-keystone/pm_domain.c
>>>> +++ b/arch/arm/mach-keystone/pm_domain.c
>>>> @@ -12,69 +12,107 @@
>>>>     * version 2, as published by the Free Software Foundation.
>>>>     */
>>>>
>>>> +#include <linux/clk.h>
>>>>    #include <linux/init.h>
>>>> -#include <linux/pm_runtime.h>
>>>>    #include <linux/pm_clock.h>
>>>> +#include <linux/pm_domain.h>
>>>>    #include <linux/platform_device.h>
>>>> -#include <linux/clk-provider.h>
>>>>    #include <linux/of.h>
>>>>
>>>> -#ifdef CONFIG_PM_RUNTIME
>>>> -static int keystone_pm_runtime_suspend(struct device *dev)
>>>> +#ifdef CONFIG_PM_GENERIC_DOMAINS
>>>> +
>>>> +struct keystone_domain {
>>>> +    struct generic_pm_domain genpd;
>>>> +    struct device    *dev;
>>>> +};
>>>> +
>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>    {
>>>> +    struct clk *clk;
>>>>        int ret;
>>>> +    int i = 0;
>>>>
>>>>        dev_dbg(dev, "%s\n", __func__);
>>>>
>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>> -    if (ret)
>>>> -        return ret;
>>>> -
>>>> -    ret = pm_clk_suspend(dev);
>>>> +    ret = pm_clk_create(dev);
>>>>        if (ret) {
>>>> -        pm_generic_runtime_resume(dev);
>>>> -        return ret;
>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>> +        return;
>>>> +    };
>>>> +
>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>> +            goto clk_err;
>>>> +        };
>>>>        }
>>>>
>>>> -    return 0;
>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>> Can we not okkup two seperate callbacks instead of above check ?
>>> I don't like this CONFIG check here. Its slightly better version of
>>> ifdef in middle of the code.
>>
>> I've found more-less similar comment on patch
>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>> https://lkml.org/lkml/2014/10/17/257
>>
>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>
> Yes, I think it's a good idea to propose that change and propose to
> Rafael on linux-pm.  Be sure that myself, Ulf and Geert are Cc'd.
>
Lets do that.

regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson Oct. 23, 2014, 8:11 a.m. UTC | #9

On 22 October 2014 17:44, Geert Uytterhoeven <[e-mail]> wrote:
> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <[e-mail]> wrote:
>> On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>>>>   {
>>>>>>> +    struct clk *clk;
>>>>>>>       int ret;
>>>>>>> +    int i = 0;
>>>>>>>
>>>>>>>       dev_dbg(dev, "%s\n", __func__);
>>>>>>>
>>>>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>>>>> -    if (ret)
>>>>>>> -        return ret;
>>>>>>> -
>>>>>>> -    ret = pm_clk_suspend(dev);
>>>>>>> +    ret = pm_clk_create(dev);
>>>>>>>       if (ret) {
>>>>>>> -        pm_generic_runtime_resume(dev);
>>>>>>> -        return ret;
>>>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>>>>> +        return;
>>>>>>> +    };
>>>>>>> +
>>>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>>>>> +        if (ret) {
>>>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>>>>> +            goto clk_err;
>>>>>>> +        };
>>>>>>>       }
>>>>>>>
>>>>>>> -    return 0;
>>>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>>>>> Can we not okkup two seperate callbacks instead of above check ?
>>>>>> I don't like this CONFIG check here. Its slightly better version of
>>>>>> ifdef in middle of the code.
>>>>>
>>>>> I've found more-less similar comment on patch
>>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>>>>> https://lkml.org/lkml/2014/10/17/257
>>>>>
>>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>>>>
>>>> I am wondering whether we actually should/could do this, no matter of
>>>> CONFIG_PM_RUNTIME.
>>>>
>>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
>>>> clocks through pm_clk_suspend(), will be gated once the device becomes
>>>> runtime PM suspended. Right?
>>>
>>> Doing it unconditionally means we'll have lots of unneeded clocks running
>>> for a short while.
>
>> As long as the pm_clk_add() is being invoked from the ->attach_dev()
>> callback, we are in the probe path. Certainly we would like to have
>> clocks enabled while probing, don't you think?
>>
>> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
>> those be enabled?
>
> They will be enabled when the driver does
>
>         pm_runtime_enable(dev);
>         pm_runtime_get_sync(dev);
>
> in its .probe() method.

No! This doesn't work for drivers which have used
pm_runtime_set_active() prior pm_runtime_enable().

That should also be a common good practice for most drivers, otherwise
they wouldn’t work unless CONFIG_PM_RUNTIME is enabled.

Please have a look at the following patchset, which is fixing up one
driver to behave better.
http://marc.info/?l=linux-pm&m=141327095713390&w=2

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grygorii Strashko Oct. 23, 2014, 2:37 p.m. UTC | #10

Hi Ulf,

On 10/23/2014 11:11 AM, Ulf Hansson wrote:
> On 22 October 2014 17:44, Geert Uytterhoeven <[e-mail]> wrote:
>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <[e-mail]> wrote:
>>> On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>>>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>>>>>    {
>>>>>>>> +    struct clk *clk;
>>>>>>>>        int ret;
>>>>>>>> +    int i = 0;
>>>>>>>>
>>>>>>>>        dev_dbg(dev, "%s\n", __func__);
>>>>>>>>
>>>>>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>>>>>> -    if (ret)
>>>>>>>> -        return ret;
>>>>>>>> -
>>>>>>>> -    ret = pm_clk_suspend(dev);
>>>>>>>> +    ret = pm_clk_create(dev);
>>>>>>>>        if (ret) {
>>>>>>>> -        pm_generic_runtime_resume(dev);
>>>>>>>> -        return ret;
>>>>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>>>>>> +        return;
>>>>>>>> +    };
>>>>>>>> +
>>>>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>>>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>>>>>> +        if (ret) {
>>>>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>>>>>> +            goto clk_err;
>>>>>>>> +        };
>>>>>>>>        }
>>>>>>>>
>>>>>>>> -    return 0;
>>>>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>>>>>> Can we not okkup two seperate callbacks instead of above check ?
>>>>>>> I don't like this CONFIG check here. Its slightly better version of
>>>>>>> ifdef in middle of the code.
>>>>>>
>>>>>> I've found more-less similar comment on patch
>>>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>>>>>> https://lkml.org/lkml/2014/10/17/257
>>>>>>
>>>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>>>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>>>>>
>>>>> I am wondering whether we actually should/could do this, no matter of
>>>>> CONFIG_PM_RUNTIME.
>>>>>
>>>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
>>>>> clocks through pm_clk_suspend(), will be gated once the device becomes
>>>>> runtime PM suspended. Right?
>>>>
>>>> Doing it unconditionally means we'll have lots of unneeded clocks running
>>>> for a short while.
>>
>>> As long as the pm_clk_add() is being invoked from the ->attach_dev()
>>> callback, we are in the probe path. Certainly we would like to have
>>> clocks enabled while probing, don't you think?
>>>
>>> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
>>> those be enabled?
>>
>> They will be enabled when the driver does
>>
>>          pm_runtime_enable(dev);
>>          pm_runtime_get_sync(dev);
>>
>> in its .probe() method.
> 
> No! This doesn't work for drivers which have used
> pm_runtime_set_active() prior pm_runtime_enable().

Sorry, but some misunderstanding is here:
1) If some code call pm_runtime_set_active() it has to ensure
that all PM resources switched to ON state. All! So, it will 
be ok to call enable & get after that - these functions will only 
adjust counters.

2) if CONFIG_PM_RUNTIME=n the pm_runtime_set_active() will
be empty (see pm_runtime.h) and you can't relay on it.

3) if CONFIG_PM_RUNTIME=n the pm_runtime_enable/disable() will
be empty - and disable_depth == 1 all the time.

In my case, the combination of generic PD and PM clock framework
will do everything I need for both cases CONFIG_PM_RUNTIME=y/n.

PM domain attach_dev/detach_dev callbacks - will fill PM resources
and enable them if CONFIG_PM_RUNTIME=n.

if CONFIG_PM_RUNTIME - PM resources will be enabled/disabled
by Runtime PM through .start()/.stop() callbacks.

And seems suspend/resume will work too - can't try it now, but it
should work, because .start()/.stop() callbacks have to be called
from pm_genpd_suspend_noirq.


> 
> That should also be a common good practice for most drivers, otherwise
> they wouldn’t work unless CONFIG_PM_RUNTIME is enabled.
> 
> Please have a look at the following patchset, which is fixing up one
> driver to behave better.
> http://marc.info/?l=linux-pm&m=141327095713390&w=2

It always was (and seems will) a big challenge to support both
CONFIG_PM_RUNTIME=y and system suspend in drivers ;), especially if driver was
initially created using Runtime PM centric approach.

But, for the case CONFIG_PM_RUNTIME + !CONFIG_PM_RUNTIME + suspend...
It will be painful :..((


For example your patches (may be I'm not fully understand your problem, 
so here are just comments to code):
patch 3:
  - I think you can do smth like this in probe 
        ret = pm_runtime_get_sync(&pdev->dev);
        if (ret < 0)
                goto err_m2m;
+       
+       if (!pm_runtime_enabled(dev)) {
+               gsc_runtime_resume(dev);
+       } 

 - and similar thing in remove, before pm_runtime_disable
 
patch 5 - pm_runtime_force_suspend/resume() will not take into
account or change Runtime PM state of the device if !CONFIG_PM_RUNTIME.
runtime_status == RPM_SUSPENDED always in this case!
So, there may be some side-effects.

patch 7 - you can't call clk_prepare/unprepare from Runtime PM
callbacks, because they aren't atomic

Oh, You definitely will be enjoyed ;)

regards,
-grygorii

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson Oct. 24, 2014, 9:53 a.m. UTC | #11

On 23 October 2014 16:37, Grygorii Strashko <[e-mail]> wrote:
> Hi Ulf,
>
> On 10/23/2014 11:11 AM, Ulf Hansson wrote:
>> On 22 October 2014 17:44, Geert Uytterhoeven <[e-mail]> wrote:
>>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <[e-mail]> wrote:
>>>> On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
>>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>>>>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>>>>>>    {
>>>>>>>>> +    struct clk *clk;
>>>>>>>>>        int ret;
>>>>>>>>> +    int i = 0;
>>>>>>>>>
>>>>>>>>>        dev_dbg(dev, "%s\n", __func__);
>>>>>>>>>
>>>>>>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>>>>>>> -    if (ret)
>>>>>>>>> -        return ret;
>>>>>>>>> -
>>>>>>>>> -    ret = pm_clk_suspend(dev);
>>>>>>>>> +    ret = pm_clk_create(dev);
>>>>>>>>>        if (ret) {
>>>>>>>>> -        pm_generic_runtime_resume(dev);
>>>>>>>>> -        return ret;
>>>>>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>>>>>>> +        return;
>>>>>>>>> +    };
>>>>>>>>> +
>>>>>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>>>>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>>>>>>> +        if (ret) {
>>>>>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>>>>>>> +            goto clk_err;
>>>>>>>>> +        };
>>>>>>>>>        }
>>>>>>>>>
>>>>>>>>> -    return 0;
>>>>>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>>>>>>> Can we not okkup two seperate callbacks instead of above check ?
>>>>>>>> I don't like this CONFIG check here. Its slightly better version of
>>>>>>>> ifdef in middle of the code.
>>>>>>>
>>>>>>> I've found more-less similar comment on patch
>>>>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>>>>>>> https://lkml.org/lkml/2014/10/17/257
>>>>>>>
>>>>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>>>>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>>>>>>
>>>>>> I am wondering whether we actually should/could do this, no matter of
>>>>>> CONFIG_PM_RUNTIME.
>>>>>>
>>>>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
>>>>>> clocks through pm_clk_suspend(), will be gated once the device becomes
>>>>>> runtime PM suspended. Right?
>>>>>
>>>>> Doing it unconditionally means we'll have lots of unneeded clocks running
>>>>> for a short while.
>>>
>>>> As long as the pm_clk_add() is being invoked from the ->attach_dev()
>>>> callback, we are in the probe path. Certainly we would like to have
>>>> clocks enabled while probing, don't you think?
>>>>
>>>> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
>>>> those be enabled?
>>>
>>> They will be enabled when the driver does
>>>
>>>          pm_runtime_enable(dev);
>>>          pm_runtime_get_sync(dev);
>>>
>>> in its .probe() method.
>>
>> No! This doesn't work for drivers which have used
>> pm_runtime_set_active() prior pm_runtime_enable().
>
> Sorry, but some misunderstanding is here:
> 1) If some code call pm_runtime_set_active() it has to ensure
> that all PM resources switched to ON state. All! So, it will
> be ok to call enable & get after that - these functions will only
> adjust counters.

Correct.

This is also the key problem with your approach. You requires a
pm_runtime_get_sync() to trigger the runtime PM resume callbacks to be
invoked. That's a fragile design.

The solution that I propose is to "manually" enable your PM clks
during the probe sequence. We can do that as a part of pm_clk_add() or
we invoke pm_clk_resume() separately, but more important no matter of
CONFIG_PM_RUNTIME.

The driver could then be responsible to invoke pm_runtime_set_active()
to reflect that all runtime PM resources are enabled.

>
> 2) if CONFIG_PM_RUNTIME=n the pm_runtime_set_active() will
> be empty (see pm_runtime.h) and you can't relay on it.
>
> 3) if CONFIG_PM_RUNTIME=n the pm_runtime_enable/disable() will
> be empty - and disable_depth == 1 all the time.
>
> In my case, the combination of generic PD and PM clock framework
> will do everything I need for both cases CONFIG_PM_RUNTIME=y/n.
>
> PM domain attach_dev/detach_dev callbacks - will fill PM resources
> and enable them if CONFIG_PM_RUNTIME=n.
>
> if CONFIG_PM_RUNTIME - PM resources will be enabled/disabled
> by Runtime PM through .start()/.stop() callbacks.
>
> And seems suspend/resume will work too - can't try it now, but it
> should work, because .start()/.stop() callbacks have to be called
> from pm_genpd_suspend_noirq.
>
>
>>
>> That should also be a common good practice for most drivers, otherwise
>> they wouldn’t work unless CONFIG_PM_RUNTIME is enabled.
>>
>> Please have a look at the following patchset, which is fixing up one
>> driver to behave better.
>> http://marc.info/?l=linux-pm&m=141327095713390&w=2
>
> It always was (and seems will) a big challenge to support both
> CONFIG_PM_RUNTIME=y and system suspend in drivers ;), especially if driver was
> initially created using Runtime PM centric approach.
>
> But, for the case CONFIG_PM_RUNTIME + !CONFIG_PM_RUNTIME + suspend...
> It will be painful :..((

I agree to that this _has_ been an issue. It also remarkable that
people have been just accepting that for so long.

Now, we have added the pm_runtime_force_suspend|resume() helpers.
Those will help to solve these cases.

>
>
> For example your patches (may be I'm not fully understand your problem,
> so here are just comments to code):
> patch 3:
>   - I think you can do smth like this in probe
>         ret = pm_runtime_get_sync(&pdev->dev);
>         if (ret < 0)
>                 goto err_m2m;

This is wrong!

1) It will break the driver for !CONFIG_PM_RUNTIME.

2) It would also be broken for CONFIG_PM_RUNTIME for the scenario
where a bus also handles runtime PM resources.
Typically from the bus' ->probe() this is done:
pm_runtime_get_noresume()
pm_runtime_set_active()

As stated earlier, we shouldn't require the runtime PM resume callback
to be invoked just because a pm_runtime_get_sync(). It's fragile.

> +
> +       if (!pm_runtime_enabled(dev)) {
> +               gsc_runtime_resume(dev);
> +       }
>
>  - and similar thing in remove, before pm_runtime_disable
>
> patch 5 - pm_runtime_force_suspend/resume() will not take into
> account or change Runtime PM state of the device if !CONFIG_PM_RUNTIME.
> runtime_status == RPM_SUSPENDED always in this case!
> So, there may be some side-effects.

pm_runtime_status_suspended() will always return false for !CONFIG_PM_RUNTIME.

There are no side effects as long as you have defined your runtime PM
callbacks within CONFIG_PM. SET_PM_RUNTIME_PM_OPS() also helps out
here.

>
> patch 7 - you can't call clk_prepare/unprepare from Runtime PM
> callbacks, because they aren't atomic

If the runtime PM callbacks are invoked in atomic context, the driver
needs to tell the runtime PM core about it. That's done through,
pm_runtime_irq_safe(), which it doesn't.

>
> Oh, You definitely will be enjoyed ;)

Likely you to. :-)

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grygorii Strashko Oct. 24, 2014, 12:07 p.m. UTC | #12

Hi Ulf,

On 10/24/2014 12:53 PM, Ulf Hansson wrote:
> On 23 October 2014 16:37, Grygorii Strashko <[e-mail]> wrote:
>> On 10/23/2014 11:11 AM, Ulf Hansson wrote:
>>> On 22 October 2014 17:44, Geert Uytterhoeven <[e-mail]> wrote:
>>>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <[e-mail]> wrote:
>>>>> On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
>>>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>>>>>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>>>>>>>>>>     {
>>>>>>>>>> +    struct clk *clk;
>>>>>>>>>>         int ret;
>>>>>>>>>> +    int i = 0;
>>>>>>>>>>
>>>>>>>>>>         dev_dbg(dev, "%s\n", __func__);
>>>>>>>>>>
>>>>>>>>>> -    ret = pm_generic_runtime_suspend(dev);
>>>>>>>>>> -    if (ret)
>>>>>>>>>> -        return ret;
>>>>>>>>>> -
>>>>>>>>>> -    ret = pm_clk_suspend(dev);
>>>>>>>>>> +    ret = pm_clk_create(dev);
>>>>>>>>>>         if (ret) {
>>>>>>>>>> -        pm_generic_runtime_resume(dev);
>>>>>>>>>> -        return ret;
>>>>>>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>>>>>>>> +        return;
>>>>>>>>>> +    };
>>>>>>>>>> +
>>>>>>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>>>>>>>>> +        ret = pm_clk_add_clk(dev, clk);
>>>>>>>>>> +        if (ret) {
>>>>>>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>>>>>>>>>> +            goto clk_err;
>>>>>>>>>> +        };
>>>>>>>>>>         }
>>>>>>>>>>
>>>>>>>>>> -    return 0;
>>>>>>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>>>>>>>>> Can we not okkup two seperate callbacks instead of above check ?
>>>>>>>>> I don't like this CONFIG check here. Its slightly better version of
>>>>>>>>> ifdef in middle of the code.
>>>>>>>>
>>>>>>>> I've found more-less similar comment on patch
>>>>>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>>>>>>>> https://lkml.org/lkml/2014/10/17/257
>>>>>>>>
>>>>>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>>>>>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>>>>>>>
>>>>>>> I am wondering whether we actually should/could do this, no matter of
>>>>>>> CONFIG_PM_RUNTIME.
>>>>>>>
>>>>>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
>>>>>>> clocks through pm_clk_suspend(), will be gated once the device becomes
>>>>>>> runtime PM suspended. Right?
>>>>>>
>>>>>> Doing it unconditionally means we'll have lots of unneeded clocks running
>>>>>> for a short while.
>>>>
>>>>> As long as the pm_clk_add() is being invoked from the ->attach_dev()
>>>>> callback, we are in the probe path. Certainly we would like to have
>>>>> clocks enabled while probing, don't you think?
>>>>>
>>>>> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
>>>>> those be enabled?
>>>>
>>>> They will be enabled when the driver does
>>>>
>>>>           pm_runtime_enable(dev);
>>>>           pm_runtime_get_sync(dev);
>>>>
>>>> in its .probe() method.
>>>
>>> No! This doesn't work for drivers which have used
>>> pm_runtime_set_active() prior pm_runtime_enable().
>>
>> Sorry, but some misunderstanding is here:
>> 1) If some code call pm_runtime_set_active() it has to ensure
>> that all PM resources switched to ON state. All! So, it will
>> be ok to call enable & get after that - these functions will only
>> adjust counters.
> 
> Correct.
> 
> This is also the key problem with your approach. You requires a
> pm_runtime_get_sync() to trigger the runtime PM resume callbacks to be
> invoked. That's a fragile design.

Sorry, but what I'm expecting is that these APIs will work according to
documentation - nothing specific actually :) And for Paltform bus devices
it's usual way to enable device.

> 
> The solution that I propose is to "manually" enable your PM clks
> during the probe sequence. We can do that as a part of pm_clk_add() or

Done in patch set 3 - but only if !CONFIG_PM_RUNTIME

> we invoke pm_clk_resume() separately, but more important no matter of
> CONFIG_PM_RUNTIME.
> 

Why? What benefits will be doing this if CONFIG_PM_RUNTIME=y?
For Keystone 2 CONFIG_PM_RUNTIME=y is intended to be normal operational mode and 
all devices belongs to Platform bus.

Also, device's resuming operation is usually heavy operation and, taking into
account deferred probing mechanism and usual implementation of .probe() function,
your proposition will lead to runtime overhead at least for Platform devices.

What is usually done in probe:
<- here you propose to resume device

1) get resources (IO, IRQ, regulators, GPIO, phys, ..) - for each
resource -EPROBE_DEFER can be returned.

2) allocate and fill device context - can fail.

3) configure resources (set gpio, enable regulators or phys,..) - can fail

4) [now] resume device 

5) configure device

6) setup irq
 
7) [optional] suspend device

As you can see from above, the Platform devices aren't need to be enabled before step 5 and, 
if your proposition will be accepted, it will lead to few additional resume/suspend
cycles per-device. It's not good as for me. Is it?


> The driver could then be responsible to invoke pm_runtime_set_active()
> to reflect that all runtime PM resources are enabled.

[runtime_pm.txt] - this is recovery function and caller should be very careful.
 
Again, from implementation point of view:
-- how it's done now:
    .probe()
	pm_runtime_enable(&pdev->dev);
	pm_runtime_get_sync(&pdev->dev);

    .remove()
	pm_runtime_put_sync(&pdev->dev);
	pm_runtime_disable(&pdev->dev);

-- how it will be:
     .probe()
	//pm_runtime_enable(&pdev->dev);
	//pm_runtime_get_sync(&pdev->dev);

	[optional] call .runtime_resume();

	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);
	[optional, to keep device active] pm_runtime_get_sync()

     .remove()
	[optional] pm_runtime_put_sync(&pdev->dev);
	pm_runtime_disable(&pdev->dev);
	
	call .runtime_suspend();
	pm_runtime_set_suspended(dev);

And that would need to be done for all drivers.

> 
>>
>> 2) if CONFIG_PM_RUNTIME=n the pm_runtime_set_active() will
>> be empty (see pm_runtime.h) and you can't relay on it.
>>
>> 3) if CONFIG_PM_RUNTIME=n the pm_runtime_enable/disable() will
>> be empty - and disable_depth == 1 all the time.
>>
>> In my case, the combination of generic PD and PM clock framework
>> will do everything I need for both cases CONFIG_PM_RUNTIME=y/n.
>>
>> PM domain attach_dev/detach_dev callbacks - will fill PM resources
>> and enable them if CONFIG_PM_RUNTIME=n.
>>
>> if CONFIG_PM_RUNTIME - PM resources will be enabled/disabled
>> by Runtime PM through .start()/.stop() callbacks.
>>
>> And seems suspend/resume will work too - can't try it now, but it
>> should work, because .start()/.stop() callbacks have to be called
>> from pm_genpd_suspend_noirq.
>>
>>
>>>
>>> That should also be a common good practice for most drivers, otherwise
>>> they wouldn’t work unless CONFIG_PM_RUNTIME is enabled.
>>>
>>> Please have a look at the following patchset, which is fixing up one
>>> driver to behave better.
>>> http://marc.info/?l=linux-pm&m=141327095713390&w=2
>>
>> It always was (and seems will) a big challenge to support both
>> CONFIG_PM_RUNTIME=y and system suspend in drivers ;), especially if driver was
>> initially created using Runtime PM centric approach.
>>
>> But, for the case CONFIG_PM_RUNTIME + !CONFIG_PM_RUNTIME + suspend...
>> It will be painful :..((
> 
> I agree to that this _has_ been an issue. It also remarkable that
> people have been just accepting that for so long.
> 
> Now, we have added the pm_runtime_force_suspend|resume() helpers.
> Those will help to solve these cases.
> 
>>
>>
>> For example your patches (may be I'm not fully understand your problem,
>> so here are just comments to code):
>> patch 3:
>>    - I think you can do smth like this in probe
>>          ret = pm_runtime_get_sync(&pdev->dev);
>>          if (ret < 0)
>>                  goto err_m2m;
> 
> This is wrong!
> 
> 1) It will break the driver for !CONFIG_PM_RUNTIME.

Hm. It should work. In your driver you have (for the case !CONFIG_PM_RUNTIME):
	pm_runtime_enable(dev); ------------------------ NOP
	ret = pm_runtime_get_sync(&pdev->dev); --------- NOP
	if (ret < 0)
		goto err_m2m;
so, if you add:
	if (!pm_runtime_enabled(dev)) { ---------------- always FALSE
               gsc_runtime_resume(dev); 
	       /* ^ is the same as
		gsc_hw_set_sw_reset(gsc);
		gsc_wait_reset(gsc);
		gsc_m2m_resume(gsc);
	       */
       }
it will work in both cases, because pm_runtime_enabled() == true
when  CONFIG_PM_RUNTIME=y.

> 
> 2) It would also be broken for CONFIG_PM_RUNTIME for the scenario
> where a bus also handles runtime PM resources.
> Typically from the bus' ->probe() this is done:
> pm_runtime_get_noresume()
> pm_runtime_set_active()

So, Has your device been enabled by bus?

> 
> As stated earlier, we shouldn't require the runtime PM resume callback
> to be invoked just because a pm_runtime_get_sync(). It's fragile.
> 
>> +
>> +       if (!pm_runtime_enabled(dev)) {
>> +               gsc_runtime_resume(dev);
>> +       }
>>
>>   - and similar thing in remove, before pm_runtime_disable
>>
>> patch 5 - pm_runtime_force_suspend/resume() will not take into
>> account or change Runtime PM state of the device if !CONFIG_PM_RUNTIME.
>> runtime_status == RPM_SUSPENDED always in this case!
>> So, there may be some side-effects.
> 
> pm_runtime_status_suspended() will always return false for !CONFIG_PM_RUNTIME.

Nice workaround.

> 
> There are no side effects as long as you have defined your runtime PM
> callbacks within CONFIG_PM. SET_PM_RUNTIME_PM_OPS() also helps out
> here.
> 
>>
>> patch 7 - you can't call clk_prepare/unprepare from Runtime PM
>> callbacks, because they aren't atomic
> 
> If the runtime PM callbacks are invoked in atomic context, the driver
> needs to tell the runtime PM core about it. That's done through,
> pm_runtime_irq_safe(), which it doesn't.
> 
>>
>> Oh, You definitely will be enjoyed ;)
> 
> Likely you to. :-)

Oh. Yes definitely :) I'm trying to reuse what is already in kernel
(even not to implement smth. new) more then 3 months already :( - It's sad.

regards,
-grygorii

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov Oct. 24, 2014, 4:39 p.m. UTC | #13

On Fri, Oct 24, 2014 at 11:53:05AM +0200, Ulf Hansson wrote:
> On 23 October 2014 16:37, Grygorii Strashko <[e-mail]> wrote:
> > Hi Ulf,
> >
> > On 10/23/2014 11:11 AM, Ulf Hansson wrote:
> >> On 22 October 2014 17:44, Geert Uytterhoeven <[e-mail]> wrote:
> >>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <[e-mail]> wrote:
> >>>> On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
> >>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
> >>>>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
> >>>>>>>>>    {
> >>>>>>>>> +    struct clk *clk;
> >>>>>>>>>        int ret;
> >>>>>>>>> +    int i = 0;
> >>>>>>>>>
> >>>>>>>>>        dev_dbg(dev, "%s\n", __func__);
> >>>>>>>>>
> >>>>>>>>> -    ret = pm_generic_runtime_suspend(dev);
> >>>>>>>>> -    if (ret)
> >>>>>>>>> -        return ret;
> >>>>>>>>> -
> >>>>>>>>> -    ret = pm_clk_suspend(dev);
> >>>>>>>>> +    ret = pm_clk_create(dev);
> >>>>>>>>>        if (ret) {
> >>>>>>>>> -        pm_generic_runtime_resume(dev);
> >>>>>>>>> -        return ret;
> >>>>>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
> >>>>>>>>> +        return;
> >>>>>>>>> +    };
> >>>>>>>>> +
> >>>>>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
> >>>>>>>>> +        ret = pm_clk_add_clk(dev, clk);
> >>>>>>>>> +        if (ret) {
> >>>>>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
> >>>>>>>>> +            goto clk_err;
> >>>>>>>>> +        };
> >>>>>>>>>        }
> >>>>>>>>>
> >>>>>>>>> -    return 0;
> >>>>>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
> >>>>>>>> Can we not okkup two seperate callbacks instead of above check ?
> >>>>>>>> I don't like this CONFIG check here. Its slightly better version of
> >>>>>>>> ifdef in middle of the code.
> >>>>>>>
> >>>>>>> I've found more-less similar comment on patch
> >>>>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
> >>>>>>> https://lkml.org/lkml/2014/10/17/257
> >>>>>>>
> >>>>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
> >>>>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
> >>>>>>
> >>>>>> I am wondering whether we actually should/could do this, no matter of
> >>>>>> CONFIG_PM_RUNTIME.
> >>>>>>
> >>>>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
> >>>>>> clocks through pm_clk_suspend(), will be gated once the device becomes
> >>>>>> runtime PM suspended. Right?
> >>>>>
> >>>>> Doing it unconditionally means we'll have lots of unneeded clocks running
> >>>>> for a short while.
> >>>
> >>>> As long as the pm_clk_add() is being invoked from the ->attach_dev()
> >>>> callback, we are in the probe path. Certainly we would like to have
> >>>> clocks enabled while probing, don't you think?
> >>>>
> >>>> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
> >>>> those be enabled?
> >>>
> >>> They will be enabled when the driver does
> >>>
> >>>          pm_runtime_enable(dev);
> >>>          pm_runtime_get_sync(dev);
> >>>
> >>> in its .probe() method.
> >>
> >> No! This doesn't work for drivers which have used
> >> pm_runtime_set_active() prior pm_runtime_enable().
> >
> > Sorry, but some misunderstanding is here:
> > 1) If some code call pm_runtime_set_active() it has to ensure
> > that all PM resources switched to ON state. All! So, it will
> > be ok to call enable & get after that - these functions will only
> > adjust counters.
> 
> Correct.
> 
> This is also the key problem with your approach. You requires a
> pm_runtime_get_sync() to trigger the runtime PM resume callbacks to be
> invoked. That's a fragile design.

Why is this fragile design? Having pm_runtime_get_sync() result in
resuming the device (and in turn the PM domain it is in) if device is
suspended is the proper behavior, no?

Thanks.

Ulf Hansson Oct. 25, 2014, 10:45 a.m. UTC | #14

On 24 October 2014 18:39, Dmitry Torokhov <[e-mail]> wrote:
> On Fri, Oct 24, 2014 at 11:53:05AM +0200, Ulf Hansson wrote:
>> On 23 October 2014 16:37, Grygorii Strashko <[e-mail]> wrote:
>> > Hi Ulf,
>> >
>> > On 10/23/2014 11:11 AM, Ulf Hansson wrote:
>> >> On 22 October 2014 17:44, Geert Uytterhoeven <[e-mail]> wrote:
>> >>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <[e-mail]> wrote:
>> >>>> On 22 October 2014 17:09, Geert Uytterhoeven <[e-mail]> wrote:
>> >>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <[e-mail]> wrote:
>> >>>>>>>>> +void keystone_pm_domain_attach_dev(struct device *dev)
>> >>>>>>>>>    {
>> >>>>>>>>> +    struct clk *clk;
>> >>>>>>>>>        int ret;
>> >>>>>>>>> +    int i = 0;
>> >>>>>>>>>
>> >>>>>>>>>        dev_dbg(dev, "%s\n", __func__);
>> >>>>>>>>>
>> >>>>>>>>> -    ret = pm_generic_runtime_suspend(dev);
>> >>>>>>>>> -    if (ret)
>> >>>>>>>>> -        return ret;
>> >>>>>>>>> -
>> >>>>>>>>> -    ret = pm_clk_suspend(dev);
>> >>>>>>>>> +    ret = pm_clk_create(dev);
>> >>>>>>>>>        if (ret) {
>> >>>>>>>>> -        pm_generic_runtime_resume(dev);
>> >>>>>>>>> -        return ret;
>> >>>>>>>>> +        dev_err(dev, "pm_clk_create failed %d\n", ret);
>> >>>>>>>>> +        return;
>> >>>>>>>>> +    };
>> >>>>>>>>> +
>> >>>>>>>>> +    while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>> >>>>>>>>> +        ret = pm_clk_add_clk(dev, clk);
>> >>>>>>>>> +        if (ret) {
>> >>>>>>>>> +            dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
>> >>>>>>>>> +            goto clk_err;
>> >>>>>>>>> +        };
>> >>>>>>>>>        }
>> >>>>>>>>>
>> >>>>>>>>> -    return 0;
>> >>>>>>>>> +    if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
>> >>>>>>>> Can we not okkup two seperate callbacks instead of above check ?
>> >>>>>>>> I don't like this CONFIG check here. Its slightly better version of
>> >>>>>>>> ifdef in middle of the code.
>> >>>>>>>
>> >>>>>>> I've found more-less similar comment on patch
>> >>>>>>> "Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
>> >>>>>>> https://lkml.org/lkml/2014/10/17/257
>> >>>>>>>
>> >>>>>>> So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
>> >>>>>>> in case !IS_ENABLED(CONFIG_PM_RUNTIME)
>> >>>>>>
>> >>>>>> I am wondering whether we actually should/could do this, no matter of
>> >>>>>> CONFIG_PM_RUNTIME.
>> >>>>>>
>> >>>>>> Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
>> >>>>>> clocks through pm_clk_suspend(), will be gated once the device becomes
>> >>>>>> runtime PM suspended. Right?
>> >>>>>
>> >>>>> Doing it unconditionally means we'll have lots of unneeded clocks running
>> >>>>> for a short while.
>> >>>
>> >>>> As long as the pm_clk_add() is being invoked from the ->attach_dev()
>> >>>> callback, we are in the probe path. Certainly we would like to have
>> >>>> clocks enabled while probing, don't you think?
>> >>>>
>> >>>> If we wouldn't enable the clocks for CONFIG_PM_RUNTIME, when will
>> >>>> those be enabled?
>> >>>
>> >>> They will be enabled when the driver does
>> >>>
>> >>>          pm_runtime_enable(dev);
>> >>>          pm_runtime_get_sync(dev);
>> >>>
>> >>> in its .probe() method.
>> >>
>> >> No! This doesn't work for drivers which have used
>> >> pm_runtime_set_active() prior pm_runtime_enable().
>> >
>> > Sorry, but some misunderstanding is here:
>> > 1) If some code call pm_runtime_set_active() it has to ensure
>> > that all PM resources switched to ON state. All! So, it will
>> > be ok to call enable & get after that - these functions will only
>> > adjust counters.
>>
>> Correct.
>>
>> This is also the key problem with your approach. You requires a
>> pm_runtime_get_sync() to trigger the runtime PM resume callbacks to be
>> invoked. That's a fragile design.
>
> Why is this fragile design? Having pm_runtime_get_sync() result in
> resuming the device (and in turn the PM domain it is in) if device is
> suspended is the proper behavior, no?

It's fragile, because the device may very well be "runtime PM active"
at the point when we invoke pm_runtime_get_sync(). Thus the runtime PM
resume callback isn't invoked, which is a requirement for these cases
to work.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ulf Hansson Oct. 27, 2014, 9:39 a.m. UTC | #15

Hi Grygorii,

[...]

>>
>> The solution that I propose is to "manually" enable your PM clks
>> during the probe sequence. We can do that as a part of pm_clk_add() or
>
> Done in patch set 3 - but only if !CONFIG_PM_RUNTIME
>
>> we invoke pm_clk_resume() separately, but more important no matter of
>> CONFIG_PM_RUNTIME.
>>
> Why? What benefits will be doing this if CONFIG_PM_RUNTIME=y?
> For Keystone 2 CONFIG_PM_RUNTIME=y is intended to be normal operational mode and
> all devices belongs to Platform bus.
>
> Also, device's resuming operation is usually heavy operation and, taking into
> account deferred probing mechanism and usual implementation of .probe() function,
> your proposition will lead to runtime overhead at least for Platform devices.

I don't quite follow. Why should there be an overhead?

>
> What is usually done in probe:
> <- here you propose to resume device

I didn't suggest we should resume the device here.

I said we should enable the device's PM clocks during ->probe().
That's a different thing.

>
> 1) get resources (IO, IRQ, regulators, GPIO, phys, ..) - for each
> resource -EPROBE_DEFER can be returned.
>
> 2) allocate and fill device context - can fail.

For your information, writing/reading to the device's registers might
not be safe, unless its PM domain is powered.

>
> 3) configure resources (set gpio, enable regulators or phys,..) - can fail
>
> 4) [now] resume device
>
> 5) configure device
>
> 6) setup irq
>
> 7) [optional] suspend device
>
> As you can see from above, the Platform devices aren't need to be enabled before step 5 and,
> if your proposition will be accepted, it will lead to few additional resume/suspend
> cycles per-device. It's not good as for me. Is it?

Why will it lead to a few additional resume/suspend cycles per device?
I don't follow.

>
>
>> The driver could then be responsible to invoke pm_runtime_set_active()
>> to reflect that all runtime PM resources are enabled.
>
> [runtime_pm.txt] - this is recovery function and caller should be very careful.

What? I couldn't find this stated anywhere in the documentation.

This is what's stated though:

5. Runtime PM Initialization, Device Probing and Removal
[...]
In addition to that, the initial runtime PM status of all devices is
'suspended', but it need not reflect the actual physical state of the device.
Thus, if the device is initially active (i.e. it is able to process I/O), its
runtime PM status must be changed to 'active', with the help of
pm_runtime_set_active(), before pm_runtime_enable() is called for the device.
[...]

>
> Again, from implementation point of view:
> -- how it's done now:
>     .probe()
>         pm_runtime_enable(&pdev->dev);
>         pm_runtime_get_sync(&pdev->dev);
>
>     .remove()
>         pm_runtime_put_sync(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>
> -- how it will be:
>      .probe()
>         //pm_runtime_enable(&pdev->dev);
>         //pm_runtime_get_sync(&pdev->dev);
>
>         [optional] call .runtime_resume();
>
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>         [optional, to keep device active] pm_runtime_get_sync()
>
>      .remove()
>         [optional] pm_runtime_put_sync(&pdev->dev);
>         pm_runtime_disable(&pdev->dev);
>
>         call .runtime_suspend();
>         pm_runtime_set_suspended(dev);
>
> And that would need to be done for all drivers.

I can't tell the number of drivers you are referring to and how big
impact it would have. Could you maybe summarize which drivers you are
concerned about?

I guess, if we have screwed things up regarding the runtime PM support
for some drivers, we need to fix them. I am also happy to help.

[...]

>> This is wrong!
>>
>> 1) It will break the driver for !CONFIG_PM_RUNTIME.
>
> Hm. It should work. In your driver you have (for the case !CONFIG_PM_RUNTIME):
>         pm_runtime_enable(dev); ------------------------ NOP
>         ret = pm_runtime_get_sync(&pdev->dev); --------- NOP
>         if (ret < 0)
>                 goto err_m2m;
> so, if you add:
>         if (!pm_runtime_enabled(dev)) { ---------------- always FALSE
>                gsc_runtime_resume(dev);
>                /* ^ is the same as
>                 gsc_hw_set_sw_reset(gsc);
>                 gsc_wait_reset(gsc);
>                 gsc_m2m_resume(gsc);
>                */
>        }
> it will work in both cases, because pm_runtime_enabled() == true
> when  CONFIG_PM_RUNTIME=y.

So this might work for some cases and for some not. As stated earlier,
it won't work if the device is runtime PM active.

The better solution is the follow my proposal and thus also conform to
the runtime PM documentation for how ->probe() should be done.

>
>>
>> 2) It would also be broken for CONFIG_PM_RUNTIME for the scenario
>> where a bus also handles runtime PM resources.
>> Typically from the bus' ->probe() this is done:
>> pm_runtime_get_noresume()
>> pm_runtime_set_active()
>
> So, Has your device been enabled by bus?

Yes we have examples of that. drivers/amba/bus.c.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
new file mode 100644
index 0000000..4bbf2aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
@@ -0,0 +1,31 @@ 
+* TI Keystone 2 Generic PM Controller
+
+The TI Keystone 2 Generic PM Controller is responsible for Clock gating
+for each controlled IP module.
+
+Required properties:
+- compatible: Should be "ti,keystone-powerdomain"
+- #power-domain-cells: Should be 0, see below:
+
+The gpc node is a power-controller as documented by the generic power domain
+bindings in Documentation/devicetree/bindings/power/power_domain.txt.
+
+Example:
+
+	pm_controller: pm-controller {
+		compatible = "ti,keystone-powerdomain";
+		#power-domain-cells = <0>;
+	};
+
+	netcp: netcp@2090000 {
+		reg = <0x2620110 0x8>;
+		reg-names = "efuse";
+		...
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		power-domains = <&pm_controller>;
+
+		clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
+		dma-coherent;
+	}
diff --git a/arch/arm/mach-keystone/Kconfig b/arch/arm/mach-keystone/Kconfig
index 98a156a..de43107 100644
--- a/arch/arm/mach-keystone/Kconfig
+++ b/arch/arm/mach-keystone/Kconfig
@@ -9,6 +9,7 @@  config ARCH_KEYSTONE
 	select COMMON_CLK_KEYSTONE
 	select ARCH_SUPPORTS_BIG_ENDIAN
 	select ZONE_DMA if ARM_LPAE
+	select PM_GENERIC_DOMAINS if PM
 	help
 	  Support for boards based on the Texas Instruments Keystone family of
 	  SoCs.
diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
index ca79dda..d58759d 100644
--- a/arch/arm/mach-keystone/pm_domain.c
+++ b/arch/arm/mach-keystone/pm_domain.c
@@ -12,69 +12,107 @@ 
  * version 2, as published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/init.h>
-#include <linux/pm_runtime.h>
 #include <linux/pm_clock.h>
+#include <linux/pm_domain.h>
 #include <linux/platform_device.h>
-#include <linux/clk-provider.h>
 #include <linux/of.h>
 
-#ifdef CONFIG_PM_RUNTIME
-static int keystone_pm_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_GENERIC_DOMAINS
+
+struct keystone_domain {
+	struct generic_pm_domain genpd;
+	struct device	*dev;
+};
+
+void keystone_pm_domain_attach_dev(struct device *dev)
 {
+	struct clk *clk;
 	int ret;
+	int i = 0;
 
 	dev_dbg(dev, "%s\n", __func__);
 
-	ret = pm_generic_runtime_suspend(dev);
-	if (ret)
-		return ret;
-
-	ret = pm_clk_suspend(dev);
+	ret = pm_clk_create(dev);
 	if (ret) {
-		pm_generic_runtime_resume(dev);
-		return ret;
+		dev_err(dev, "pm_clk_create failed %d\n", ret);
+		return;
+	};
+
+	while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
+		ret = pm_clk_add_clk(dev, clk);
+		if (ret) {
+			dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
+			goto clk_err;
+		};
 	}
 
-	return 0;
+	if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {
+		ret = pm_clk_resume(dev);
+		if (ret) {
+			dev_err(dev, "pm_clk_resume failed %d\n", ret);
+			goto clk_err;
+		};
+	}
+	return;
+
+clk_err:
+	pm_clk_destroy(dev);
 }
 
-static int keystone_pm_runtime_resume(struct device *dev)
+void keystone_pm_domain_detach_dev(struct device *dev)
 {
 	dev_dbg(dev, "%s\n", __func__);
-
-	pm_clk_resume(dev);
-
-	return pm_generic_runtime_resume(dev);
+	pm_clk_destroy(dev);
 }
-#endif
 
-static struct dev_pm_domain keystone_pm_domain = {
-	.ops = {
-		SET_RUNTIME_PM_OPS(keystone_pm_runtime_suspend,
-				   keystone_pm_runtime_resume, NULL)
-		USE_PLATFORM_PM_SLEEP_OPS
+static const struct keystone_domain keystone_domain = {
+	.genpd = {
+		.name = "keystone",
+		.attach_dev = keystone_pm_domain_attach_dev,
+		.detach_dev = keystone_pm_domain_detach_dev,
+		.dev_ops = {
+			.stop = pm_clk_suspend,
+			.start = pm_clk_resume,
+		},
 	},
 };
 
-static struct pm_clk_notifier_block platform_domain_notifier = {
-	.pm_domain = &keystone_pm_domain,
+static int keystone_pm_domain_probe(struct platform_device *pdev)
+{
+	struct keystone_domain *domain;
+
+	domain = devm_kzalloc(&pdev->dev,
+			      sizeof(struct keystone_domain), GFP_KERNEL);
+	if (!domain)
+		return -ENOMEM;
+
+	domain->genpd = keystone_domain.genpd;
+	domain->dev = &pdev->dev;
+
+	pm_genpd_init(&domain->genpd, NULL, false);
+	return of_genpd_add_provider_simple(pdev->dev.of_node, &domain->genpd);
+}
+
+static struct of_device_id keystone_pm_domain_dt_ids[] = {
+	{ .compatible = "ti,keystone-powerdomain" },
+	{ }
 };
 
-static struct of_device_id of_keystone_table[] = {
-	{.compatible = "ti,keystone"},
-	{ /* end of list */ },
+static struct platform_driver keystone_pm_domain_driver = {
+	.driver = {
+		.name = "ti,keystone-powerdomain",
+		.owner = THIS_MODULE,
+		.of_match_table = keystone_pm_domain_dt_ids,
+	},
+	.probe = keystone_pm_domain_probe,
 };
 
 int __init keystone_pm_runtime_init(void)
 {
-	struct device_node *np;
-
-	np = of_find_matching_node(NULL, of_keystone_table);
-	if (!np)
-		return 0;
-
-	pm_clk_add_notifier(&platform_bus_type, &platform_domain_notifier);
-
-	return 0;
+	return platform_driver_register(&keystone_pm_domain_driver);
 }
+#else
+int __init keystone_pm_runtime_init(void) { return 0; }
+#endif /* CONFIG_PM_GENERIC_DOMAINS */