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

[Grygorii Strashko]

From: Geert Uytterhoeven <geert+***@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 <***@linaro.org>
Signed-off-by: Geert Uytterhoeven <geert+***@glider.be>
Signed-off-by: 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)
{
}

[Grygorii Strashko]

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

Signed-off-by: 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: ***@02530c00 {
compatible = "ns16550a";
current-speed = <115200>;
@@ -275,6 +280,7 @@
status = "disabled";
clocks = <&clkpa>;
clock-names = "fck";
+ power-domains = <&pm_controller>;
bus_freq = <2500000>;
};

[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-QSEj5FYQhm4dnm+***@public.gmane.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko-***@public.gmane.org>
---
.../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: ***@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 */

[Santosh Shilimkar]

Post by 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.
---
.../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.
+
+- compatible: Should be "ti,keystone-powerdomain"
+
+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.

Post by Grygorii Strashko
+bindings in Documentation/devicetree/bindings/power/power_domain.txt.
+
+
+ pm_controller: pm-controller {
+ compatible = "ti,keystone-powerdomain";
+ #power-domain-cells = <0>;
+ };
+
+ 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

[Grygorii Strashko]

Hi Santosh,

Post by Santosh Shilimkar

Post by 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.
---
.../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.
+
+- compatible: Should be "ti,keystone-powerdomain"
+
+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.

Post by Santosh Shilimkar

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

Can we not okkup two seperate callbacks instead of above check ?
I don't like this CONFIG check here. Its slightly better version of
ifdef in middle of the code.

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

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

regards,
-grygorii


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

[Ulf Hansson]

Post by Grygorii Strashko
Hi Santosh,

Post by Santosh Shilimkar

Post by 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.
---
.../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.
+
+- compatible: Should be "ti,keystone-powerdomain"
+
+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.

Post by Santosh Shilimkar

Post by Grygorii Strashko
+bindings in Documentation/devicetree/bindings/power/power_domain.txt.
+
+
+ pm_controller: pm-controller {
+ compatible = "ti,keystone-powerdomain";
+ #power-domain-cells = <0>;
+ };
+
+ 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

[Geert Uytterhoeven]

Hi Ulf,

Post by Ulf Hansson

Post by Grygorii Strashko

Post by Santosh Shilimkar

Post by Grygorii Strashko
+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 -- ***@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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ulf Hansson]

Post by Geert Uytterhoeven
Hi Ulf,

Post by Ulf Hansson

Post by Grygorii Strashko

Post by Santosh Shilimkar

Post by Grygorii Strashko
+void keystone_pm_domain_attach_dev(struct device *dev)
{
+ struct clk *clk;
int ret;
+ int i = 0;
dev_dbg(dev, "%s\n", __func__);
- ret = pm_generic_runtime_suspend(dev);
- if (ret)
- return ret;
-
- ret = pm_clk_suspend(dev);
+ ret = pm_clk_create(dev);
if (ret) {
- pm_generic_runtime_resume(dev);
- return ret;
+ dev_err(dev, "pm_clk_create failed %d\n", ret);
+ return;
+ };
+
+ while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
+ ret = pm_clk_add_clk(dev, clk);
+ if (ret) {
+ dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
+ goto clk_err;
+ };
}
- return 0;
+ if (!IS_ENABLED(CONFIG_PM_RUNTIME)) {

Can we not okkup two seperate callbacks instead of above check ?
I don't like this CONFIG check here. Its slightly better version of
ifdef in middle of the code.

I've found more-less similar comment on patch
"Re: [PATCH v3 1/3] power-domain: add power domain drivers for Rockchip platform"
https://lkml.org/lkml/2014/10/17/257
So, Would you like me to create patch which will enable clocks in pm_clk_add/_clk()
in case !IS_ENABLED(CONFIG_PM_RUNTIME)

I am wondering whether we actually should/could do this, no matter of
CONFIG_PM_RUNTIME.
Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
clocks through pm_clk_suspend(), will be gated once the device becomes
runtime PM suspended. Right?

Doing it unconditionally means we'll have lots of unneeded clocks running
for a short while.
Are you trying to repeat power-up-all-PM-domains-during-boot for
clocks, too? ;-)

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

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

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

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

[Geert Uytterhoeven]

Post by Ulf Hansson

Post by Geert Uytterhoeven

Post by Ulf Hansson

Post by Grygorii Strashko

Post by Santosh Shilimkar

Post by Grygorii Strashko
+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 -- ***@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

[Ulf Hansson]

Post by Geert Uytterhoeven

Post by Ulf Hansson

Post by Grygorii Strashko

Post by Santosh Shilimkar

Post by Grygorii Strashko
+void keystone_pm_domain_attach_dev(struct device *dev)
{
+ struct clk *clk;
int ret;
+ int i =3D 0;
dev_dbg(dev, "%s\n", __func__);
- ret =3D pm_generic_runtime_suspend(dev);
- if (ret)
- return ret;
-
- ret =3D pm_clk_suspend(dev);
+ ret =3D 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 =3D of_clk_get(dev->of_node, i++)) && !IS_ERR(=

clk)) {

Post by Geert Uytterhoeven

Post by Ulf Hansson

Post by Grygorii Strashko

Post by Santosh Shilimkar

Post by Grygorii Strashko
+ ret =3D 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

Post by Geert Uytterhoeven

Post by Ulf Hansson

Post by Grygorii Strashko

Post by Santosh Shilimkar
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 Ro=

ckchip platform"

Post by Geert Uytterhoeven

Post by Ulf Hansson

Post by Grygorii Strashko
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()

Post by Geert Uytterhoeven

Post by Ulf Hansson

Post by Grygorii Strashko
in case !IS_ENABLED(CONFIG_PM_RUNTIME)

I am wondering whether we actually should/could do this, no matter=

of

Post by Geert Uytterhoeven

Post by Ulf Hansson

CONFIG_PM_RUNTIME.
Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
clocks through pm_clk_suspend(), will be gated once the device bec=

omes

Post by Geert Uytterhoeven

Post by Ulf Hansson

runtime PM suspended. Right?

Doing it unconditionally means we'll have lots of unneeded clocks r=

unning

Post by Geert Uytterhoeven

Post by Ulf Hansson

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=E2=80=99t 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=3Dlinux-pm&m=3D141327095713390&w=3D2

Kind regards
Uffe

[Grygorii Strashko]

Hi Ulf,

Post by Ulf Hansson

Post by Geert Uytterhoeven

Post by Grygorii Strashko

Post by Grygorii Strashko
+void keystone_pm_domain_attach_dev(struct device *dev)
{
+ struct clk *clk;
int ret;
+ int i =3D 0;
dev_dbg(dev, "%s\n", __func__);
- ret =3D pm_generic_runtime_suspend(dev);
- if (ret)
- return ret;
-
- ret =3D pm_clk_suspend(dev);
+ ret =3D 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 =3D of_clk_get(dev->of_node, i++)) && !IS_ERR=

(clk)) {

Post by Ulf Hansson

Post by Geert Uytterhoeven

Post by Grygorii Strashko

Post by Grygorii Strashko
+ ret =3D 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 =

?

Post by Ulf Hansson

Post by Geert Uytterhoeven

Post by Grygorii Strashko

I don't like this CONFIG check here. Its slightly better versio=

n of

Post by Ulf Hansson

Post by Geert Uytterhoeven

Post by Grygorii Strashko

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 R=

ockchip platform"

Post by Ulf Hansson

Post by Geert Uytterhoeven

Post by Grygorii Strashko
https://lkml.org/lkml/2014/10/17/257
So, Would you like me to create patch which will enable clocks i=

n pm_clk_add/_clk()

Post by Ulf Hansson

Post by Geert Uytterhoeven

Post by Grygorii Strashko
in case !IS_ENABLED(CONFIG_PM_RUNTIME)

I am wondering whether we actually should/could do this, no matte=

r of

Post by Ulf Hansson

Post by Geert Uytterhoeven

CONFIG_PM_RUNTIME.
Typically, for configurations that uses CONFIG_PM_RUNTIME, the PM
clocks through pm_clk_suspend(), will be gated once the device be=

comes

Post by Ulf Hansson

Post by Geert Uytterhoeven

runtime PM suspended. Right?

Doing it unconditionally means we'll have lots of unneeded clocks =

running

Post by Ulf Hansson

Post by Geert Uytterhoeven

for a short while.

As long as the pm_clk_add() is being invoked from the ->attach_dev(=

)

Post by Ulf Hansson

Post by Geert Uytterhoeven

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.

=20
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=20
be ok to call enable & get after that - these functions will only=20
adjust counters.

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

3) if CONFIG_PM_RUNTIME=3Dn the pm_runtime_enable/disable() will
be empty - and disable_depth =3D=3D 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=3Dy/n.

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

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.

Post by Ulf Hansson
=20
That should also be a common good practice for most drivers, otherwis=

e

Post by Ulf Hansson
they wouldn=E2=80=99t work unless CONFIG_PM_RUNTIME is enabled.
=20
Please have a look at the following patchset, which is fixing up one
driver to behave better.
http://marc.info/?l=3Dlinux-pm&m=3D141327095713390&w=3D2

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

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


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

- and similar thing in remove, before pm_runtime_disable
=20
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 =3D=3D RPM_SUSPENDED always in this case!
So, there may be some side-effects.

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

Oh, You definitely will be enjoyed ;)

regards,
-grygorii

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

[Kevin Hilman]

Post by Grygorii Strashko
Hi Santosh,

Post by Santosh Shilimkar

Post by 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.
---
.../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.
+
+- compatible: Should be "ti,keystone-powerdomain"
+
+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.

Post by Santosh Shilimkar

Post by Grygorii Strashko
+bindings in Documentation/devicetree/bindings/power/power_domain.txt.
+
+
+ pm_controller: pm-controller {
+ compatible = "ti,keystone-powerdomain";
+ #power-domain-cells = <0>;
+ };
+
+ 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

[Santosh Shilimkar]

Post by Kevin Hilman

Post by Grygorii Strashko
Hi Santosh,

Post by Santosh Shilimkar

Post by 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.
---
.../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

[..]

Post by Kevin Hilman

Post by Grygorii Strashko

Post by Santosh Shilimkar

Post by Grygorii Strashko
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

[Santosh Shilimkar]

Kevin, Rafael,

Post by Grygorii Strashko
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.
---
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

[Dmitry Torokhov]

Post by Grygorii Strashko
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.
---
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.
- *
- */
-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.
+ *
+ */
+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.

[Grygorii Strashko]

Post by Dmitry Torokhov

Post by Grygorii Strashko
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.
---
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.
- *
- */
-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.
+ *
+ */
+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
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

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

[Dmitry Torokhov]

Post by Grygorii Strashko

Post by Dmitry Torokhov

Post by Grygorii Strashko
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.
---
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.
- *
- */
-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.
+ *
+ */
+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
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);
}

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 Torokhov]

Post by Dmitry Torokhov

Post by Grygorii Strashko

Post by Dmitry Torokhov

Post by Grygorii Strashko
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.
---
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.
- *
- */
-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()?

Post by Dmitry Torokhov

Post by Grygorii Strashko

Post by Dmitry Torokhov

Post by Grygorii Strashko
}
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.
+ *
+ */
+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
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);
}

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 Torokhov]

Post by Dmitry Torokhov

Post by Dmitry Torokhov

Post by Grygorii Strashko

Post by Dmitry Torokhov

Post by Grygorii Strashko
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.
---
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.
- *
- */
-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()?

Post by Dmitry Torokhov

Post by Grygorii Strashko

Post by Dmitry Torokhov

Post by Grygorii Strashko
}
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.
+ *
+ */
+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
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);
}

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 <***@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);