[PATCH v2 0/3] ARM: keystone: pm: switch to use generic pm domains

[PATCH v2 0/3] ARM: keystone: pm: switch to use generic pm domains

[Grygorii Strashko]

Hi Santosh, Kevin,

This series 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.
It will finally allow to enable Runtime PM for Keystone 2.

Patch 1 was reused from [1].

RFC version of patches can be found at [2].

Changes in v2:
- minor comments applied and rebased on top of Linux 3.18-rc1.

Link on v1:
  https://lkml.org/lkml/2014/9/29/382

[1] "[PATCH/RFC 0/4] of: Register clocks for Runtime PM with PM core"
  https://lkml.org/lkml/2014/4/24/1118

[2] "[RFC PATCH 0/4] ARM: keystone: pm: switch to use generic pm domains"
  https://lkml.org/lkml/2014/9/25/364

Geert Uytterhoeven (1):
  PM / clock_ops: Add pm_clk_add_clk()

Grygorii Strashko (2):
  ARM: keystone: pm: switch to use generic pm domains
  ARM: dts: keystone: add generic pm controller node

 .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
 arch/arm/boot/dts/keystone.dtsi                    |   6 ++
 arch/arm/mach-keystone/Kconfig                     |   1 +
 arch/arm/mach-keystone/pm_domain.c                 | 112 ++++++++++++++-------
 drivers/base/power/clock_ops.c                     |  41 ++++++--
 include/linux/pm_clock.h                           |   8 ++
 6 files changed, 152 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt

-- 
1.9.1

[PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Grygorii Strashko]

From: Geert Uytterhoeven <geert+renesas@glider.be>

The existing pm_clk_add() allows to pass a clock by con_id. However,
when referring to a specific clock from DT, no con_id is available.

Add pm_clk_add_clk(), which allows to specify the struct clk * directly.

Reviewed-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---

 Pay attantion pls, that there is another series of patches
 which have been posted already and which depends from this patch
   "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support"
   https://lkml.org/lkml/2014/10/20/105

 drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
 include/linux/pm_clock.h       |  8 ++++++++
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 7836930..f14b767 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
  */
 static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 {
-	ce->clk = clk_get(dev, ce->con_id);
+	if (!ce->clk)
+		ce->clk = clk_get(dev, ce->con_id);
 	if (IS_ERR(ce->clk)) {
 		ce->status = PCE_STATUS_ERROR;
 	} else {
@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
 	}
 }
 
-/**
- * pm_clk_add - Start using a device clock for power management.
- * @dev: Device whose clock is going to be used for power management.
- * @con_id: Connection ID of the clock.
- *
- * Add the clock represented by @con_id to the list of clocks used for
- * the power management of @dev.
- */
-int pm_clk_add(struct device *dev, const char *con_id)
+static int __pm_clk_add(struct device *dev, const char *con_id,
+			struct clk *clk)
 {
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
 			kfree(ce);
 			return -ENOMEM;
 		}
+	} else {
+		ce->clk = clk;
 	}
 
 	pm_clk_acquire(dev, ce);
@@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id)
 }
 
 /**
+ * pm_clk_add - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @con_id: Connection ID of the clock.
+ *
+ * Add the clock represented by @con_id to the list of clocks used for
+ * the power management of @dev.
+ */
+int pm_clk_add(struct device *dev, const char *con_id)
+{
+	return __pm_clk_add(dev, con_id, NULL);
+}
+
+/**
+ * pm_clk_add_clk - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @clk: Clock pointer
+ *
+ * Add the clock to the list of clocks used for the power management of @dev.
+ */
+int pm_clk_add_clk(struct device *dev, struct clk *clk)
+{
+	return __pm_clk_add(dev, NULL, clk);
+}
+
+/**
  * __pm_clk_remove - Destroy PM clock entry.
  * @ce: PM clock entry to destroy.
  */
diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h
index 8348866..0b00396 100644
--- a/include/linux/pm_clock.h
+++ b/include/linux/pm_clock.h
@@ -18,6 +18,8 @@ struct pm_clk_notifier_block {
 	char *con_ids[];
 };
 
+struct clk;
+
 #ifdef CONFIG_PM_CLK
 static inline bool pm_clk_no_clocks(struct device *dev)
 {
@@ -29,6 +31,7 @@ extern void pm_clk_init(struct device *dev);
 extern int pm_clk_create(struct device *dev);
 extern void pm_clk_destroy(struct device *dev);
 extern int pm_clk_add(struct device *dev, const char *con_id);
+extern int pm_clk_add_clk(struct device *dev, struct clk *clk);
 extern void pm_clk_remove(struct device *dev, const char *con_id);
 extern int pm_clk_suspend(struct device *dev);
 extern int pm_clk_resume(struct device *dev);
@@ -51,6 +54,11 @@ static inline int pm_clk_add(struct device *dev, const char *con_id)
 {
 	return -EINVAL;
 }
+
+static inline int pm_clk_add_clk(struct device *dev, struct clk *clk)
+{
+	return -EINVAL;
+}
 static inline void pm_clk_remove(struct device *dev, const char *con_id)
 {
 }
-- 
1.9.1

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

[Grygorii Strashko]

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 <khilman@linaro.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 .../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
+bindings in Documentation/devicetree/bindings/power/power_domain.txt.
+
+Example:
+
+	pm_controller: pm-controller {
+		compatible = "ti,keystone-powerdomain";
+		#power-domain-cells = <0>;
+	};
+
+	netcp: netcp at 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 */
-- 
1.9.1

[PATCH v2 3/3] ARM: dts: keystone: add generic pd controller node

[Grygorii Strashko]

Add TI Keystone 2 Generic Power Domain Controller node and attach
the Davinci MDIO device to it.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 arch/arm/boot/dts/keystone.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index 5d3e83f..c669d0d 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -85,6 +85,11 @@
 
 		/include/ "keystone-clocks.dtsi"
 
+		pm_controller: pm-controller {
+			compatible = "ti,keystone-powerdomain";
+			#power-domain-cells = <0>;
+		};
+
 		uart0: serial at 02530c00 {
 			compatible = "ns16550a";
 			current-speed = <115200>;
@@ -275,6 +280,7 @@
 			status = "disabled";
 			clocks = <&clkpa>;
 			clock-names = "fck";
+			power-domains = <&pm_controller>;
 			bus_freq	= <2500000>;
 		};
 
-- 
1.9.1

[PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Santosh Shilimkar]

Kevin, Rafael,

On 10/20/2014 05:56 AM, Grygorii Strashko wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> The existing pm_clk_add() allows to pass a clock by con_id. However,
> when referring to a specific clock from DT, no con_id is available.
>
> Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
>
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>
>   Pay attantion pls, that there is another series of patches
>   which have been posted already and which depends from this patch
>     "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support"
>     https://lkml.org/lkml/2014/10/20/105
>
How do you go about merging the $subject patch ?
I see there are at least 2 series which depends on this one.

Regards,
Santosh

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

[Santosh Shilimkar]

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 <khilman@linaro.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>   .../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 at 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

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

[Grygorii Strashko]

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 <khilman@linaro.org>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>   .../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 at 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
 

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

[Ulf Hansson]

On 22 October 2014 13:23, Grygorii Strashko <grygorii.strashko@ti.com> 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 <khilman@linaro.org>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>> ---
>>>   .../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 at 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

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

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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 -- geert at linux-m68k.org

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

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

[Ulf Hansson]

On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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

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

[Geert Uytterhoeven]

On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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 -- geert at linux-m68k.org

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

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

[Kevin Hilman]

Grygorii Strashko <grygorii.strashko@ti.com> 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 <khilman@linaro.org>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>> ---
>>>   .../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 at 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

[PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Dmitry Torokhov]

On Mon, Oct 20, 2014 at 03:56:02PM +0300, Grygorii Strashko wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> The existing pm_clk_add() allows to pass a clock by con_id. However,
> when referring to a specific clock from DT, no con_id is available.
> 
> Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
> 
> Reviewed-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> 
>  Pay attantion pls, that there is another series of patches
>  which have been posted already and which depends from this patch
>    "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support"
>    https://lkml.org/lkml/2014/10/20/105
> 
>  drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
>  include/linux/pm_clock.h       |  8 ++++++++
>  2 files changed, 39 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> index 7836930..f14b767 100644
> --- a/drivers/base/power/clock_ops.c
> +++ b/drivers/base/power/clock_ops.c
> @@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
>   */
>  static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>  {
> -	ce->clk = clk_get(dev, ce->con_id);
> +	if (!ce->clk)
> +		ce->clk = clk_get(dev, ce->con_id);
>  	if (IS_ERR(ce->clk)) {
>  		ce->status = PCE_STATUS_ERROR;
>  	} else {
> @@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>  	}
>  }
>  
> -/**
> - * pm_clk_add - Start using a device clock for power management.
> - * @dev: Device whose clock is going to be used for power management.
> - * @con_id: Connection ID of the clock.
> - *
> - * Add the clock represented by @con_id to the list of clocks used for
> - * the power management of @dev.
> - */
> -int pm_clk_add(struct device *dev, const char *con_id)
> +static int __pm_clk_add(struct device *dev, const char *con_id,
> +			struct clk *clk)
>  {
>  	struct pm_subsys_data *psd = dev_to_psd(dev);
>  	struct pm_clock_entry *ce;
> @@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
>  			kfree(ce);
>  			return -ENOMEM;
>  		}
> +	} else {
> +		ce->clk = clk;
>  	}
>  
>  	pm_clk_acquire(dev, ce);
> @@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id)
>  }
>  
>  /**
> + * pm_clk_add - Start using a device clock for power management.
> + * @dev: Device whose clock is going to be used for power management.
> + * @con_id: Connection ID of the clock.
> + *
> + * Add the clock represented by @con_id to the list of clocks used for
> + * the power management of @dev.
> + */
> +int pm_clk_add(struct device *dev, const char *con_id)
> +{
> +	return __pm_clk_add(dev, con_id, NULL);

Bikeshedding: why do we need __pm_clk_add() and not simply have
"canonical" pm_clk_add_clk() and then do:

int pm_clk_add(struct device *dev, const char *con_id)
{
	struct clk *clk;

	clk = clk_get(dev, con_id);
	...
	return pm_clk_add_clk(dev, clk);
}

Thanks.

-- 
Dmitry

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

[Santosh Shilimkar]

On 10/22/2014 08:58 AM, Kevin Hilman wrote:
> Grygorii Strashko <grygorii.strashko@ti.com> 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 <khilman@linaro.org>
>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>>> ---
>>>>    .../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

[PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Grygorii Strashko]

On 10/22/2014 08:38 PM, Dmitry Torokhov wrote:
> On Mon, Oct 20, 2014 at 03:56:02PM +0300, Grygorii Strashko wrote:
>> From: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> The existing pm_clk_add() allows to pass a clock by con_id. However,
>> when referring to a specific clock from DT, no con_id is available.
>>
>> Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
>>
>> Reviewed-by: Kevin Hilman <khilman@linaro.org>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>>
>>   Pay attantion pls, that there is another series of patches
>>   which have been posted already and which depends from this patch
>>     "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support"
>>     https://lkml.org/lkml/2014/10/20/105
>>
>>   drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
>>   include/linux/pm_clock.h       |  8 ++++++++
>>   2 files changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
>> index 7836930..f14b767 100644
>> --- a/drivers/base/power/clock_ops.c
>> +++ b/drivers/base/power/clock_ops.c
>> @@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
>>    */
>>   static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>>   {
>> -	ce->clk = clk_get(dev, ce->con_id);
>> +	if (!ce->clk)
>> +		ce->clk = clk_get(dev, ce->con_id);
>>   	if (IS_ERR(ce->clk)) {
>>   		ce->status = PCE_STATUS_ERROR;
>>   	} else {
>> @@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
>>   	}
>>   }
>>
>> -/**
>> - * pm_clk_add - Start using a device clock for power management.
>> - * @dev: Device whose clock is going to be used for power management.
>> - * @con_id: Connection ID of the clock.
>> - *
>> - * Add the clock represented by @con_id to the list of clocks used for
>> - * the power management of @dev.
>> - */
>> -int pm_clk_add(struct device *dev, const char *con_id)
>> +static int __pm_clk_add(struct device *dev, const char *con_id,
>> +			struct clk *clk)
>>   {
>>   	struct pm_subsys_data *psd = dev_to_psd(dev);
>>   	struct pm_clock_entry *ce;
>> @@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
>>   			kfree(ce);
>>   			return -ENOMEM;
>>   		}
>> +	} else {
>> +		ce->clk = clk;
>>   	}
>>
>>   	pm_clk_acquire(dev, ce);
>> @@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id)
>>   }
>>
>>   /**
>> + * pm_clk_add - Start using a device clock for power management.
>> + * @dev: Device whose clock is going to be used for power management.
>> + * @con_id: Connection ID of the clock.
>> + *
>> + * Add the clock represented by @con_id to the list of clocks used for
>> + * the power management of @dev.
>> + */
>> +int pm_clk_add(struct device *dev, const char *con_id)
>> +{
>> +	return __pm_clk_add(dev, con_id, NULL);
>
> Bikeshedding: why do we need __pm_clk_add() and not simply have
> "canonical" pm_clk_add_clk() and then do:
>
> int pm_clk_add(struct device *dev, const char *con_id)
> {
> 	struct clk *clk;
>
> 	clk = clk_get(dev, con_id);
> 	...
> 	return pm_clk_add_clk(dev, clk);
> }

Hm. I did fast look at code and:
1) agree - there is a lot of thing which can be optimized ;)
2) in my strong opinion, this patch is the fastest and simplest
way to introduce new API (take a look on pm_clock_entry->con_id
management code) and It is exactly what we need as of now.

regards,
-grygorii

[PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Dmitry Torokhov]

On Wed, Oct 22, 2014 at 10:02:41PM +0300, Grygorii Strashko wrote:
> On 10/22/2014 08:38 PM, Dmitry Torokhov wrote:
> >On Mon, Oct 20, 2014 at 03:56:02PM +0300, Grygorii Strashko wrote:
> >>From: Geert Uytterhoeven <geert+renesas@glider.be>
> >>
> >>The existing pm_clk_add() allows to pass a clock by con_id. However,
> >>when referring to a specific clock from DT, no con_id is available.
> >>
> >>Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
> >>
> >>Reviewed-by: Kevin Hilman <khilman@linaro.org>
> >>Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >>Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >>---
> >>
> >>  Pay attantion pls, that there is another series of patches
> >>  which have been posted already and which depends from this patch
> >>    "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support"
> >>    https://lkml.org/lkml/2014/10/20/105
> >>
> >>  drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
> >>  include/linux/pm_clock.h       |  8 ++++++++
> >>  2 files changed, 39 insertions(+), 10 deletions(-)
> >>
> >>diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> >>index 7836930..f14b767 100644
> >>--- a/drivers/base/power/clock_ops.c
> >>+++ b/drivers/base/power/clock_ops.c
> >>@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
> >>   */
> >>  static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> >>  {
> >>-	ce->clk = clk_get(dev, ce->con_id);
> >>+	if (!ce->clk)
> >>+		ce->clk = clk_get(dev, ce->con_id);
> >>  	if (IS_ERR(ce->clk)) {
> >>  		ce->status = PCE_STATUS_ERROR;
> >>  	} else {
> >>@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> >>  	}
> >>  }
> >>
> >>-/**
> >>- * pm_clk_add - Start using a device clock for power management.
> >>- * @dev: Device whose clock is going to be used for power management.
> >>- * @con_id: Connection ID of the clock.
> >>- *
> >>- * Add the clock represented by @con_id to the list of clocks used for
> >>- * the power management of @dev.
> >>- */
> >>-int pm_clk_add(struct device *dev, const char *con_id)
> >>+static int __pm_clk_add(struct device *dev, const char *con_id,
> >>+			struct clk *clk)
> >>  {
> >>  	struct pm_subsys_data *psd = dev_to_psd(dev);
> >>  	struct pm_clock_entry *ce;
> >>@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
> >>  			kfree(ce);
> >>  			return -ENOMEM;
> >>  		}
> >>+	} else {
> >>+		ce->clk = clk;
> >>  	}
> >>
> >>  	pm_clk_acquire(dev, ce);
> >>@@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id)
> >>  }
> >>
> >>  /**
> >>+ * pm_clk_add - Start using a device clock for power management.
> >>+ * @dev: Device whose clock is going to be used for power management.
> >>+ * @con_id: Connection ID of the clock.
> >>+ *
> >>+ * Add the clock represented by @con_id to the list of clocks used for
> >>+ * the power management of @dev.
> >>+ */
> >>+int pm_clk_add(struct device *dev, const char *con_id)
> >>+{
> >>+	return __pm_clk_add(dev, con_id, NULL);
> >
> >Bikeshedding: why do we need __pm_clk_add() and not simply have
> >"canonical" pm_clk_add_clk() and then do:
> >
> >int pm_clk_add(struct device *dev, const char *con_id)
> >{
> >	struct clk *clk;
> >
> >	clk = clk_get(dev, con_id);
> >	...
> >	return pm_clk_add_clk(dev, clk);
> >}
> 
> Hm. I did fast look at code and:
> 1) agree - there is a lot of thing which can be optimized ;)
> 2) in my strong opinion, this patch is the fastest and simplest
> way to introduce new API (take a look on pm_clock_entry->con_id
> management code) and It is exactly what we need as of now.

Yeah, I guess. We are lucky we do not crash when we are tryign to print
NULL strings (see pm_clk_acquire).

BTW, what is the point of doing pm_clk_add(dev, NULL)? We add clock
entry with status PCE_STATUS_ERROR and then have to handle it
everywhere? Can we just return -EINVAL if someone triies to pass NULL
ass con_id?

Thanks.

-- 
Dmitry

[PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Dmitry Torokhov]

On Wed, Oct 22, 2014 at 01:14:09PM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 22, 2014 at 10:02:41PM +0300, Grygorii Strashko wrote:
> > On 10/22/2014 08:38 PM, Dmitry Torokhov wrote:
> > >On Mon, Oct 20, 2014 at 03:56:02PM +0300, Grygorii Strashko wrote:
> > >>From: Geert Uytterhoeven <geert+renesas@glider.be>
> > >>
> > >>The existing pm_clk_add() allows to pass a clock by con_id. However,
> > >>when referring to a specific clock from DT, no con_id is available.
> > >>
> > >>Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
> > >>
> > >>Reviewed-by: Kevin Hilman <khilman@linaro.org>
> > >>Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >>Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > >>---
> > >>
> > >>  Pay attantion pls, that there is another series of patches
> > >>  which have been posted already and which depends from this patch
> > >>    "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support"
> > >>    https://lkml.org/lkml/2014/10/20/105
> > >>
> > >>  drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
> > >>  include/linux/pm_clock.h       |  8 ++++++++
> > >>  2 files changed, 39 insertions(+), 10 deletions(-)
> > >>
> > >>diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> > >>index 7836930..f14b767 100644
> > >>--- a/drivers/base/power/clock_ops.c
> > >>+++ b/drivers/base/power/clock_ops.c
> > >>@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
> > >>   */
> > >>  static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> > >>  {
> > >>-	ce->clk = clk_get(dev, ce->con_id);
> > >>+	if (!ce->clk)
> > >>+		ce->clk = clk_get(dev, ce->con_id);
> > >>  	if (IS_ERR(ce->clk)) {
> > >>  		ce->status = PCE_STATUS_ERROR;
> > >>  	} else {
> > >>@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> > >>  	}
> > >>  }
> > >>
> > >>-/**
> > >>- * pm_clk_add - Start using a device clock for power management.
> > >>- * @dev: Device whose clock is going to be used for power management.
> > >>- * @con_id: Connection ID of the clock.
> > >>- *
> > >>- * Add the clock represented by @con_id to the list of clocks used for
> > >>- * the power management of @dev.
> > >>- */
> > >>-int pm_clk_add(struct device *dev, const char *con_id)
> > >>+static int __pm_clk_add(struct device *dev, const char *con_id,
> > >>+			struct clk *clk)
> > >>  {
> > >>  	struct pm_subsys_data *psd = dev_to_psd(dev);
> > >>  	struct pm_clock_entry *ce;
> > >>@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
> > >>  			kfree(ce);
> > >>  			return -ENOMEM;
> > >>  		}
> > >>+	} else {
> > >>+		ce->clk = clk;

Shouldn't this be

		ce->clk = __clk_get(clk);

to account for clk_put() in __pm_clk_remove()?

> > >>  	}
> > >>
> > >>  	pm_clk_acquire(dev, ce);
> > >>@@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id)
> > >>  }
> > >>
> > >>  /**
> > >>+ * pm_clk_add - Start using a device clock for power management.
> > >>+ * @dev: Device whose clock is going to be used for power management.
> > >>+ * @con_id: Connection ID of the clock.
> > >>+ *
> > >>+ * Add the clock represented by @con_id to the list of clocks used for
> > >>+ * the power management of @dev.
> > >>+ */
> > >>+int pm_clk_add(struct device *dev, const char *con_id)
> > >>+{
> > >>+	return __pm_clk_add(dev, con_id, NULL);
> > >
> > >Bikeshedding: why do we need __pm_clk_add() and not simply have
> > >"canonical" pm_clk_add_clk() and then do:
> > >
> > >int pm_clk_add(struct device *dev, const char *con_id)
> > >{
> > >	struct clk *clk;
> > >
> > >	clk = clk_get(dev, con_id);
> > >	...
> > >	return pm_clk_add_clk(dev, clk);
> > >}
> > 
> > Hm. I did fast look at code and:
> > 1) agree - there is a lot of thing which can be optimized ;)
> > 2) in my strong opinion, this patch is the fastest and simplest
> > way to introduce new API (take a look on pm_clock_entry->con_id
> > management code) and It is exactly what we need as of now.
> 
> Yeah, I guess. We are lucky we do not crash when we are tryign to print
> NULL strings (see pm_clk_acquire).
> 
> BTW, what is the point of doing pm_clk_add(dev, NULL)? We add clock
> entry with status PCE_STATUS_ERROR and then have to handle it
> everywhere? Can we just return -EINVAL if someone triies to pass NULL
> ass con_id?

Umm, no, ignore me here, I misread clk_get(dev, NULL) - it won't return
error. Still, do why do we need to keep clock entry if clk_get() fails
for some reason?

Thanks.

-- 
Dmitry

[PATCH v2 1/3] PM / clock_ops: Add pm_clk_add_clk()

[Dmitry Torokhov]

On Wed, Oct 22, 2014 at 02:16:31PM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 22, 2014 at 01:14:09PM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 22, 2014 at 10:02:41PM +0300, Grygorii Strashko wrote:
> > > On 10/22/2014 08:38 PM, Dmitry Torokhov wrote:
> > > >On Mon, Oct 20, 2014 at 03:56:02PM +0300, Grygorii Strashko wrote:
> > > >>From: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >>
> > > >>The existing pm_clk_add() allows to pass a clock by con_id. However,
> > > >>when referring to a specific clock from DT, no con_id is available.
> > > >>
> > > >>Add pm_clk_add_clk(), which allows to specify the struct clk * directly.
> > > >>
> > > >>Reviewed-by: Kevin Hilman <khilman@linaro.org>
> > > >>Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >>Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > > >>---
> > > >>
> > > >>  Pay attantion pls, that there is another series of patches
> > > >>  which have been posted already and which depends from this patch
> > > >>    "[PATCH v4 0/3] ARM: rk3288 : Add PM Domain support"
> > > >>    https://lkml.org/lkml/2014/10/20/105
> > > >>
> > > >>  drivers/base/power/clock_ops.c | 41 +++++++++++++++++++++++++++++++----------
> > > >>  include/linux/pm_clock.h       |  8 ++++++++
> > > >>  2 files changed, 39 insertions(+), 10 deletions(-)
> > > >>
> > > >>diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
> > > >>index 7836930..f14b767 100644
> > > >>--- a/drivers/base/power/clock_ops.c
> > > >>+++ b/drivers/base/power/clock_ops.c
> > > >>@@ -53,7 +53,8 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
> > > >>   */
> > > >>  static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> > > >>  {
> > > >>-	ce->clk = clk_get(dev, ce->con_id);
> > > >>+	if (!ce->clk)
> > > >>+		ce->clk = clk_get(dev, ce->con_id);
> > > >>  	if (IS_ERR(ce->clk)) {
> > > >>  		ce->status = PCE_STATUS_ERROR;
> > > >>  	} else {
> > > >>@@ -63,15 +64,8 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
> > > >>  	}
> > > >>  }
> > > >>
> > > >>-/**
> > > >>- * pm_clk_add - Start using a device clock for power management.
> > > >>- * @dev: Device whose clock is going to be used for power management.
> > > >>- * @con_id: Connection ID of the clock.
> > > >>- *
> > > >>- * Add the clock represented by @con_id to the list of clocks used for
> > > >>- * the power management of @dev.
> > > >>- */
> > > >>-int pm_clk_add(struct device *dev, const char *con_id)
> > > >>+static int __pm_clk_add(struct device *dev, const char *con_id,
> > > >>+			struct clk *clk)
> > > >>  {
> > > >>  	struct pm_subsys_data *psd = dev_to_psd(dev);
> > > >>  	struct pm_clock_entry *ce;
> > > >>@@ -93,6 +87,8 @@ int pm_clk_add(struct device *dev, const char *con_id)
> > > >>  			kfree(ce);
> > > >>  			return -ENOMEM;
> > > >>  		}
> > > >>+	} else {
> > > >>+		ce->clk = clk;
> 
> Shouldn't this be
> 
> 		ce->clk = __clk_get(clk);
> 
> to account for clk_put() in __pm_clk_remove()?
> 
> > > >>  	}
> > > >>
> > > >>  	pm_clk_acquire(dev, ce);
> > > >>@@ -104,6 +100,31 @@ int pm_clk_add(struct device *dev, const char *con_id)
> > > >>  }
> > > >>
> > > >>  /**
> > > >>+ * pm_clk_add - Start using a device clock for power management.
> > > >>+ * @dev: Device whose clock is going to be used for power management.
> > > >>+ * @con_id: Connection ID of the clock.
> > > >>+ *
> > > >>+ * Add the clock represented by @con_id to the list of clocks used for
> > > >>+ * the power management of @dev.
> > > >>+ */
> > > >>+int pm_clk_add(struct device *dev, const char *con_id)
> > > >>+{
> > > >>+	return __pm_clk_add(dev, con_id, NULL);
> > > >
> > > >Bikeshedding: why do we need __pm_clk_add() and not simply have
> > > >"canonical" pm_clk_add_clk() and then do:
> > > >
> > > >int pm_clk_add(struct device *dev, const char *con_id)
> > > >{
> > > >	struct clk *clk;
> > > >
> > > >	clk = clk_get(dev, con_id);
> > > >	...
> > > >	return pm_clk_add_clk(dev, clk);
> > > >}
> > > 
> > > Hm. I did fast look at code and:
> > > 1) agree - there is a lot of thing which can be optimized ;)
> > > 2) in my strong opinion, this patch is the fastest and simplest
> > > way to introduce new API (take a look on pm_clock_entry->con_id
> > > management code) and It is exactly what we need as of now.
> > 
> > Yeah, I guess. We are lucky we do not crash when we are tryign to print
> > NULL strings (see pm_clk_acquire).
> > 
> > BTW, what is the point of doing pm_clk_add(dev, NULL)? We add clock
> > entry with status PCE_STATUS_ERROR and then have to handle it
> > everywhere? Can we just return -EINVAL if someone triies to pass NULL
> > ass con_id?
> 
> Umm, no, ignore me here, I misread clk_get(dev, NULL) - it won't return
> error. Still, do why do we need to keep clock entry if clk_get() fails
> for some reason?

OK, so what if we do something like the patch below?

Thanks.

-- 
Dmitry


PM / clock_ops: Add pm_clk_remove_clk()

Implement pm_clk_remove_clk() that complements the new pm_clk_add_clk().
Fix reference counting, rework the code to avoid storing invalid clocks,
clean up the code.

Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
---
 drivers/base/power/clock_ops.c | 169 ++++++++++++++++++++---------------------
 1 file changed, 81 insertions(+), 88 deletions(-)

diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index f14b767..840e133 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -12,21 +12,21 @@
 #include <linux/pm.h>
 #include <linux/pm_clock.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
 #include <linux/slab.h>
 #include <linux/err.h>
 
 #ifdef CONFIG_PM
 
 enum pce_status {
-	PCE_STATUS_NONE = 0,
-	PCE_STATUS_ACQUIRED,
+	PCE_STATUS_ACQUIRED = 0,
+	PCE_STATUS_PREPARED,
 	PCE_STATUS_ENABLED,
-	PCE_STATUS_ERROR,
 };
 
 struct pm_clock_entry {
 	struct list_head node;
-	char *con_id;
 	struct clk *clk;
 	enum pce_status status;
 };
@@ -47,25 +47,13 @@ static inline int __pm_clk_enable(struct device *dev, struct clk *clk)
 }
 
 /**
- * pm_clk_acquire - Acquire a device clock.
- * @dev: Device whose clock is to be acquired.
- * @ce: PM clock entry corresponding to the clock.
+ * pm_clk_add_clk - Start using a device clock for power management.
+ * @dev: Device whose clock is going to be used for power management.
+ * @clk: Clock pointer
+ *
+ * Add the clock to the list of clocks used for the power management of @dev.
  */
-static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
-{
-	if (!ce->clk)
-		ce->clk = clk_get(dev, ce->con_id);
-	if (IS_ERR(ce->clk)) {
-		ce->status = PCE_STATUS_ERROR;
-	} else {
-		clk_prepare(ce->clk);
-		ce->status = PCE_STATUS_ACQUIRED;
-		dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id);
-	}
-}
-
-static int __pm_clk_add(struct device *dev, const char *con_id,
-			struct clk *clk)
+int pm_clk_add_clk(struct device *dev, struct clk *clk)
 {
 	struct pm_subsys_data *psd = dev_to_psd(dev);
 	struct pm_clock_entry *ce;
@@ -79,23 +67,19 @@ static int __pm_clk_add(struct device *dev, const char *con_id,
 		return -ENOMEM;
 	}
 
-	if (con_id) {
-		ce->con_id = kstrdup(con_id, GFP_KERNEL);
-		if (!ce->con_id) {
-			dev_err(dev,
-				"Not enough memory for clock connection ID.\n");
-			kfree(ce);
-			return -ENOMEM;
-		}
-	} else {
-		ce->clk = clk;
-	}
+	__clk_get(clk);
 
-	pm_clk_acquire(dev, ce);
+	clk_prepare(clk);
+
+	ce->status = PCE_STATUS_PREPARED;
+	ce->clk = clk;
 
 	spin_lock_irq(&psd->lock);
 	list_add_tail(&ce->node, &psd->clock_list);
 	spin_unlock_irq(&psd->lock);
+
+	dev_dbg(dev, "Clock %s managed by runtime PM.\n", __clk_get_name(clk));
+
 	return 0;
 }
 
@@ -109,19 +93,23 @@ static int __pm_clk_add(struct device *dev, const char *con_id,
  */
 int pm_clk_add(struct device *dev, const char *con_id)
 {
-	return __pm_clk_add(dev, con_id, NULL);
-}
+	struct clk *clk;
+	int retval;
 
-/**
- * pm_clk_add_clk - Start using a device clock for power management.
- * @dev: Device whose clock is going to be used for power management.
- * @clk: Clock pointer
- *
- * Add the clock to the list of clocks used for the power management of @dev.
- */
-int pm_clk_add_clk(struct device *dev, struct clk *clk)
-{
-	return __pm_clk_add(dev, NULL, clk);
+	clk = clk_get(dev, con_id);
+	if (IS_ERR(clk)) {
+		retval = PTR_ERR(clk);
+		dev_err(dev, "Failed to locate lock (con_id %s): %d\n",
+			con_id, retval);
+		return retval;
+	}
+
+	retval = pm_clk_add_clk(dev, clk);
+
+	/* pm_clk_add_clk takes its own reference to clk */
+	clk_put(clk);
+
+	return retval;
 }
 
 /**
@@ -133,32 +121,30 @@ static void __pm_clk_remove(struct pm_clock_entry *ce)
 	if (!ce)
 		return;
 
-	if (ce->status < PCE_STATUS_ERROR) {
-		if (ce->status == PCE_STATUS_ENABLED)
-			clk_disable(ce->clk);
+	if (ce->status == PCE_STATUS_ENABLED)
+		clk_disable(ce->clk);
 
-		if (ce->status >= PCE_STATUS_ACQUIRED) {
-			clk_unprepare(ce->clk);
-			clk_put(ce->clk);
-		}
+	if (ce->status >= PCE_STATUS_ACQUIRED) {
+		clk_unprepare(ce->clk);
+		clk_put(ce->clk);
 	}
 
-	kfree(ce->con_id);
 	kfree(ce);
 }
 
 /**
  * pm_clk_remove - Stop using a device clock for power management.
  * @dev: Device whose clock should not be used for PM any more.
- * @con_id: Connection ID of the clock.
+ * @clk: Clock pointer
  *
- * Remove the clock represented by @con_id from the list of clocks used for
- * the power management of @dev.
+ * Remove the clock from the list of clocks used for the power
+ * management of @dev.
  */
-void pm_clk_remove(struct device *dev, const char *con_id)
+
+void pm_clk_remove_clk(struct device *dev, struct clk *clk)
 {
 	struct pm_subsys_data *psd = dev_to_psd(dev);
-	struct pm_clock_entry *ce;
+	struct pm_clock_entry *ce, *matching_ce = NULL;
 
 	if (!psd)
 		return;
@@ -166,22 +152,35 @@ void pm_clk_remove(struct device *dev, const char *con_id)
 	spin_lock_irq(&psd->lock);
 
 	list_for_each_entry(ce, &psd->clock_list, node) {
-		if (!con_id && !ce->con_id)
-			goto remove;
-		else if (!con_id || !ce->con_id)
-			continue;
-		else if (!strcmp(con_id, ce->con_id))
-			goto remove;
+		if (ce->clk == clk) {
+			matching_ce = ce;
+			list_del(&ce->node);
+			break;
+		}
 	}
 
 	spin_unlock_irq(&psd->lock);
-	return;
 
- remove:
-	list_del(&ce->node);
-	spin_unlock_irq(&psd->lock);
+	__pm_clk_remove(matching_ce);
+}
+
+/**
+ * pm_clk_remove - Stop using a device clock for power management.
+ * @dev: Device whose clock should not be used for PM any more.
+ * @con_id: Connection ID of the clock.
+ *
+ * Remove the clock represented by @con_id from the list of clocks used for
+ * the power management of @dev.
+ */
+void pm_clk_remove(struct device *dev, const char *con_id)
+{
+	struct clk *clk;
 
-	__pm_clk_remove(ce);
+	clk = clk_get(dev, con_id);
+	if (!IS_ERR(clk)) {
+		pm_clk_remove_clk(dev, clk);
+		clk_put(clk);
+	}
 }
 
 /**
@@ -266,10 +265,9 @@ int pm_clk_suspend(struct device *dev)
 	spin_lock_irqsave(&psd->lock, flags);
 
 	list_for_each_entry_reverse(ce, &psd->clock_list, node) {
-		if (ce->status < PCE_STATUS_ERROR) {
-			if (ce->status == PCE_STATUS_ENABLED)
-				clk_disable(ce->clk);
-			ce->status = PCE_STATUS_ACQUIRED;
+		if (ce->status == PCE_STATUS_ENABLED) {
+			clk_disable(ce->clk);
+			ce->status = PCE_STATUS_PREPARED;
 		}
 	}
 
@@ -297,11 +295,9 @@ int pm_clk_resume(struct device *dev)
 	spin_lock_irqsave(&psd->lock, flags);
 
 	list_for_each_entry(ce, &psd->clock_list, node) {
-		if (ce->status < PCE_STATUS_ERROR) {
-			ret = __pm_clk_enable(dev, ce->clk);
-			if (!ret)
-				ce->status = PCE_STATUS_ENABLED;
-		}
+		ret = __pm_clk_enable(dev, ce->clk);
+		if (!ret)
+			ce->status = PCE_STATUS_ENABLED;
 	}
 
 	spin_unlock_irqrestore(&psd->lock, flags);
@@ -390,10 +386,9 @@ int pm_clk_suspend(struct device *dev)
 	spin_lock_irqsave(&psd->lock, flags);
 
 	list_for_each_entry_reverse(ce, &psd->clock_list, node) {
-		if (ce->status < PCE_STATUS_ERROR) {
-			if (ce->status == PCE_STATUS_ENABLED)
-				clk_disable(ce->clk);
-			ce->status = PCE_STATUS_ACQUIRED;
+		if (ce->status == PCE_STATUS_ENABLED) {
+			clk_disable(ce->clk);
+			ce->status = PCE_STATUS_PREPARED;
 		}
 	}
 
@@ -422,11 +417,9 @@ int pm_clk_resume(struct device *dev)
 	spin_lock_irqsave(&psd->lock, flags);
 
 	list_for_each_entry(ce, &psd->clock_list, node) {
-		if (ce->status < PCE_STATUS_ERROR) {
-			ret = __pm_clk_enable(dev, ce->clk);
-			if (!ret)
-				ce->status = PCE_STATUS_ENABLED;
-		}
+		ret = __pm_clk_enable(dev, ce->clk);
+		if (!ret)
+			ce->status = PCE_STATUS_ENABLED;
 	}
 
 	spin_unlock_irqrestore(&psd->lock, flags);
-- 
2.1.0.rc2.206.gedb03e5

:

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

[Ulf Hansson]

On 22 October 2014 17:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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

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

[Grygorii Strashko]

Hi Ulf,

On 10/23/2014 11:11 AM, Ulf Hansson wrote:
> On 22 October 2014 17:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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

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

[Ulf Hansson]

On 23 October 2014 16:37, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> Hi Ulf,
>
> On 10/23/2014 11:11 AM, Ulf Hansson wrote:
>> On 22 October 2014 17:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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

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

[Grygorii Strashko]

Hi Ulf,

On 10/24/2014 12:53 PM, Ulf Hansson wrote:
> On 23 October 2014 16:37, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>> On 10/23/2014 11:11 AM, Ulf Hansson wrote:
>>> On 22 October 2014 17:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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

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

[Dmitry Torokhov]

On Fri, Oct 24, 2014 at 11:53:05AM +0200, Ulf Hansson wrote:
> On 23 October 2014 16:37, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
> > Hi Ulf,
> >
> > On 10/23/2014 11:11 AM, Ulf Hansson wrote:
> >> On 22 October 2014 17:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>> On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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.

-- 
Dmitry

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

[Ulf Hansson]

On 24 October 2014 18:39, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Fri, Oct 24, 2014 at 11:53:05AM +0200, Ulf Hansson wrote:
>> On 23 October 2014 16:37, Grygorii Strashko <grygorii.strashko@ti.com> wrote:
>> > Hi Ulf,
>> >
>> > On 10/23/2014 11:11 AM, Ulf Hansson wrote:
>> >> On 22 October 2014 17:44, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> >>> On Wed, Oct 22, 2014 at 5:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >>>> On 22 October 2014 17:09, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> >>>>> On Wed, Oct 22, 2014 at 5:01 PM, Ulf Hansson <ulf.hansson@linaro.org> 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

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

[Ulf Hansson]

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