[PATCH v1 0/4] ARM: keystone: pm: switch to use generic pm domains

[PATCH v1 0/4] 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].
Patch 4 is added to illustrate Keystone 2 PM domains configuration in DT.

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

[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 (3):
  ARM: keystone: pm: switch to use generic pm domains
  ARM: dts: keystone: add generic pm controller node
  ARM: dts: k2hk-evm: attach net, qmss and knav_dmas to keystone-gpc

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

-- 
1.9.1

[PATCH v1 1/4] 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.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 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 b99e6c0..4ac586d 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 v1 2/4] 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.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 .../devicetree/bindings/power/ti,keystone-gpc.txt  |  31 ++++++
 arch/arm/mach-keystone/Kconfig                     |   1 +
 arch/arm/mach-keystone/pm_domain.c                 | 115 ++++++++++++++-------
 3 files changed, 110 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/ti,keystone-gpc.txt

diff --git a/Documentation/devicetree/bindings/power/ti,keystone-gpc.txt b/Documentation/devicetree/bindings/power/ti,keystone-gpc.txt
new file mode 100644
index 0000000..43af1fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/ti,keystone-gpc.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-gpc"
+- #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-gpc";
+		#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 e571084..0132bde 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..3eb5257 100644
--- a/arch/arm/mach-keystone/pm_domain.c
+++ b/arch/arm/mach-keystone/pm_domain.c
@@ -12,69 +12,110 @@
  * 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 base;
+	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 = {
+	.base = {
+		.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,
+		},
+		.power_off_latency_ns = 25000,
+		.power_on_latency_ns = 2000000,
 	},
 };
 
-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->base = keystone_domain.base;
+	domain->base.of_node = pdev->dev.of_node;
+	domain->dev = &pdev->dev;
+
+	pm_genpd_init(&domain->base, NULL, false);
+	return of_genpd_add_provider_simple(pdev->dev.of_node, &domain->base);
+}
+
+static struct of_device_id keystone_pm_domain_dt_ids[] = {
+	{ .compatible = "ti,keystone-gpc" },
+	{ }
 };
 
-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-gpc",
+		.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 v1 3/4] ARM: dts: keystone: add generic pm controller node

[Grygorii Strashko]

Add TI Keystone 2 Generic PM 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 9e31fe7..4e53494 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-gpc";
+			#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 v1 4/4] ARM: dts: k2hk-evm: attach net, qmss and knav_dmas to keystone-gpc

[Grygorii Strashko]

Attach Keystone 2s nodes for NetCP, NetCPx, QMSS, KNAV-DMA devices
to the TI Keystone 2 Generic PM Controller.

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

diff --git a/arch/arm/boot/dts/k2hk-evm.dts b/arch/arm/boot/dts/k2hk-evm.dts
index 91371f7..c1ceaa2 100644
--- a/arch/arm/boot/dts/k2hk-evm.dts
+++ b/arch/arm/boot/dts/k2hk-evm.dts
@@ -58,6 +58,7 @@
 				clock-output-names = "refclk-ddr3b";
 			};
 		};
+
 		qmss: qmss at 2a40000 {
 			compatible = "ti,keystone-navigator-qmss";
 			dma-coherent;
@@ -65,6 +66,8 @@
 			#size-cells = <1>;
 			clocks = <&chipclk13>;
 			ranges;
+			power-domains = <&pm_controller>;
+
 			queue-range	= <0 0x4000>;
 			linkram0	= <0x100000 0x8000>;
 			linkram1	= <0x0 0x10000>;
@@ -198,6 +201,8 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
+			power-domains = <&pm_controller>;
+
 			ti,navigator-cloud-address = <0x23a80000 0x23a90000
 						   0x23aa0000 0x23ab0000>;
 
@@ -229,6 +234,7 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
+			power-domains = <&pm_controller>;
 
 			clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
 			dma-coherent;
@@ -307,6 +313,7 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges;
+			power-domains = <&pm_controller>;
 
 			clocks = <&clkxge>;
 			clock-names = "clk_xge";
-- 
1.9.1

[PATCH v1 2/4] ARM: keystone: pm: switch to use generic pm domains

[Geert Uytterhoeven]

Hi Grygorii,

On Mon, Sep 29, 2014 at 4:38 PM, Grygorii Strashko
<grygorii.strashko@ti.com> 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.

Thanks, this looks interesting, as today I've been digging deeper into the
!CONFIG_PM_RUNTIME case for my patch series, and ended up with something
similar...

> --- a/arch/arm/mach-keystone/pm_domain.c
> +++ b/arch/arm/mach-keystone/pm_domain.c
> @@ -12,69 +12,110 @@
>   * 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 base;
> +       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);

This is an important difference compared to the non-DT pm_clk_notify()
version for !CONFIG_PM_RUNTIME.

You do call pm_clk_create() and pm_clk_add_clk(), while
pm_clk_notify() doesn't. Hence for the latter, the clocklist is always empty.

> +               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);

As a consequence of the above, calling pm_clk_resume() here just works,
and there's no need for the separate enable_clock()/disable_clock()
functions, like in drivers/base/power/clock_ops.c.

> +               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 = {
> +       .base = {
> +               .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,

Same here: the clocks will be disabled/enabled on system suspend/resume,
which is not the case for the non-DT case.

Rafael: shouldn't pm_clk_notify() in drivers/base/power/clock_ops.c
behave the same as above?
Or is there a good reason the non-PM runtime version doesn't call pm_clk_add()?

Then the two versions (CONFIG_PM_RUNTIME enabled vs. disabled) can become
more similar, and perhaps be merged.

> +               },
> +               .power_off_latency_ns = 25000,
> +               .power_on_latency_ns = 2000000,
>         },
>  };


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 v1 2/4] ARM: keystone: pm: switch to use generic pm domains

[Santosh Shilimkar]

On Monday 29 September 2014 10:38 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.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  .../devicetree/bindings/power/ti,keystone-gpc.txt  |  31 ++++++
>  arch/arm/mach-keystone/Kconfig                     |   1 +
>  arch/arm/mach-keystone/pm_domain.c                 | 115 ++++++++++++++-------
>  3 files changed, 110 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/power/ti,keystone-gpc.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/ti,keystone-gpc.txt b/Documentation/devicetree/bindings/power/ti,keystone-gpc.txt
> new file mode 100644
> index 0000000..43af1fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/ti,keystone-gpc.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-gpc"
> +- #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-gpc";
'gpc' doesn't make it clear. May be '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 e571084..0132bde 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..3eb5257 100644
> --- a/arch/arm/mach-keystone/pm_domain.c
> +++ b/arch/arm/mach-keystone/pm_domain.c
> @@ -12,69 +12,110 @@
>   * 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 base;
> +	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 = {
> +	.base = {
> +		.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,
> +		},
> +		.power_off_latency_ns = 25000,
> +		.power_on_latency_ns = 2000000,
Did you cook up these numbers ?

Other than that patch looks fine to me. If Kevin is happy
with overall approach then we can proceed with this.

Regards,
Santosh

[PATCH v1 4/4] ARM: dts: k2hk-evm: attach net, qmss and knav_dmas to keystone-gpc

[Santosh Shilimkar]

On Monday 29 September 2014 10:38 AM, Grygorii Strashko wrote:
> Attach Keystone 2s nodes for NetCP, NetCPx, QMSS, KNAV-DMA devices
> to the TI Keystone 2 Generic PM Controller.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  arch/arm/boot/dts/k2hk-evm.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/k2hk-evm.dts b/arch/arm/boot/dts/k2hk-evm.dts
> index 91371f7..c1ceaa2 100644
> --- a/arch/arm/boot/dts/k2hk-evm.dts
> +++ b/arch/arm/boot/dts/k2hk-evm.dts
> @@ -58,6 +58,7 @@
>  				clock-output-names = "refclk-ddr3b";
>  			};
>  		};
> +
Stray change ?

>  		qmss: qmss at 2a40000 {
>  			compatible = "ti,keystone-navigator-qmss";
>  			dma-coherent;
> @@ -65,6 +66,8 @@
>  			#size-cells = <1>;
>  			clocks = <&chipclk13>;
>  			ranges;
> +			power-domains = <&pm_controller>;
> +
>  			queue-range	= <0 0x4000>;
>  			linkram0	= <0x100000 0x8000>;
>  			linkram1	= <0x0 0x10000>;
> @@ -198,6 +201,8 @@
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			ranges;
> +			power-domains = <&pm_controller>;
> +
>  			ti,navigator-cloud-address = <0x23a80000 0x23a90000
>  						   0x23aa0000 0x23ab0000>;
>  
> @@ -229,6 +234,7 @@
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			ranges;
> +			power-domains = <&pm_controller>;
>  
>  			clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
>  			dma-coherent;
> @@ -307,6 +313,7 @@
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			ranges;
> +			power-domains = <&pm_controller>;
>  
>  			clocks = <&clkxge>;
>  			clock-names = "clk_xge";
> 

[PATCH v1 2/4] ARM: keystone: pm: switch to use generic pm domains

[Kevin Hilman]

Hi Grygorii,

On 9/29/14 7:38 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.
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

IMO, this approach is much better.

One minor nit below...

> diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
> index ca79dda..3eb5257 100644
> --- a/arch/arm/mach-keystone/pm_domain.c
> +++ b/arch/arm/mach-keystone/pm_domain.c
> @@ -12,69 +12,110 @@
>   * 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 base;
> + struct device *dev;
> +};

I think the name 'base' for this field leads to confusion later in the
code, since base usually means something else in drivers.  How about
'genpd'?

Kevin

[PATCH v1 2/4] ARM: keystone: pm: switch to use generic pm domains

[Kevin Hilman]

On 10/2/14 12:17 PM, Santosh Shilimkar wrote:
> On Monday 29 September 2014 10:38 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.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---

[...]

>> Other than that patch looks fine to me. If Kevin is happy
>> with overall approach then we can proceed with this.

Looks good to me:

Reviewed-by: Kevin Hilman <khilman@linaro.org>

[PATCH v1 2/4] ARM: keystone: pm: switch to use generic pm domains

[Grygorii Strashko]

On 10/12/2014 03:56 PM, Kevin Hilman wrote:
> Hi Grygorii,
>
> On 9/29/14 7:38 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.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>
> IMO, this approach is much better.
>
> One minor nit below...
>
>> diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
>> index ca79dda..3eb5257 100644
>> --- a/arch/arm/mach-keystone/pm_domain.c
>> +++ b/arch/arm/mach-keystone/pm_domain.c
>> @@ -12,69 +12,110 @@
>>    * 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 base;
>> + struct device *dev;
>> +};
>
> I think the name 'base' for this field leads to confusion later in the
> code, since base usually means something else in drivers.  How about
> 'genpd'?

Agree. I'll change it.

Thanks for your comments.

Regards,
-grygorii