[PATCH v5 0/3] ARM: rk3288 : Add PM Domain support

[PATCH v5 0/3] ARM: rk3288 : Add PM Domain support

[jinkun.hong]

From: "jinkun.hong" <jinkun.hong@rock-chips.com>

Add power domain drivers based on generic power domain for Rockchip platform,
and support RK3288.

https://chromium-review.googlesource.com/#/c/220253/9
This is the GPU driver, add the following information in DT,
and it can support the PMDOMAIN.

gpu: gpu@ffa30000 {
	compatible = "arm,malit764",
		     "arm,malit76x",
		     "arm,malit7xx",
		     "arm,mali-midgard";
	reg = <0xffa30000 0x10000>;
	interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
		     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
	interrupt-names = "JOB", "MMU", "GPU";
	clocks = <&cru ACLK_GPU>;
	clock-names = "aclk_gpu";
	operating-points = <
		/* KHz uV */
		100000 800000
		200000 850000
		300000 950000
		400000 1000000
		600000 1150000
	>;
	power-domains = <&gpu_power>;
	status = "disabled";
};

Based on:
- [PATCH v1 1/4] PM / clock_ops: Add pm_clk_add_clk()
  http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg735599.html

Changes in v5:
- delete idle_lock
- add timeout in rockchip_pmu_set_idle_request()

Changes in v4:
- use list storage dev

Changes in v3:
- change use pm_clk_resume() and pm_clk_suspend()
- DT structure has changed
- Decomposition power-controller, changed to multiple controller
   (gpu-power-controller, hevc-power-controller)

Changes in v2:
- remove the "pd->pd.of_node = np"
- move clocks to "optional"
- make pd_vio clocks all one entry per line and alphabetize.
- power: power-controller move back to pinctrl: pinctrl.

jinkun.hong (3):
  power-domain: add power domain drivers for Rockchip platform
  dt-bindings: add document of Rockchip power domain
  ARM: dts: add rk3288 power-domain node

 .../bindings/arm/rockchip/power_domain.txt         |   46 +++
 arch/arm/boot/dts/rk3288.dtsi                      |   24 ++
 arch/arm/mach-rockchip/Kconfig                     |    1 +
 arch/arm/mach-rockchip/Makefile                    |    1 +
 arch/arm/mach-rockchip/pm_domains.c                |  368 ++++++++++++++++++++
 5 files changed, 440 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
 create mode 100644 arch/arm/mach-rockchip/pm_domains.c

-- 
1.7.9.5

[PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[jinkun.hong]

From: "jinkun.hong" <jinkun.hong@rock-chips.com>

Add power domain drivers based on generic power domain for Rockchip platform,
and support RK3288.

Signed-off-by: Jack Dai <jack.dai@rock-chips.com>
Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>

---

Changes in v5:
- delete idle_lock
- add timeout in rockchip_pmu_set_idle_request()

Changes in v4:
- use list storage dev

Changes in v3:
- change use pm_clk_resume() and pm_clk_suspend()

Changes in v2:
- remove the "pd->pd.of_node = np"

 arch/arm/mach-rockchip/Kconfig      |    1 +
 arch/arm/mach-rockchip/Makefile     |    1 +
 arch/arm/mach-rockchip/pm_domains.c |  368 +++++++++++++++++++++++++++++++++++
 3 files changed, 370 insertions(+)
 create mode 100644 arch/arm/mach-rockchip/pm_domains.c

diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index d168669..4920a88 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -12,6 +12,7 @@ config ARCH_ROCKCHIP
 	select DW_APB_TIMER_OF
 	select ARM_GLOBAL_TIMER
 	select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
+	select PM_GENERIC_DOMAINS if PM
 	help
 	  Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs
 	  containing the RK2928, RK30xx and RK31xx series.
diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
index b29d8ea..805268d 100644
--- a/arch/arm/mach-rockchip/Makefile
+++ b/arch/arm/mach-rockchip/Makefile
@@ -2,3 +2,4 @@ CFLAGS_platsmp.o := -march=armv7-a
 
 obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
 obj-$(CONFIG_SMP) += headsmp.o platsmp.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
diff --git a/arch/arm/mach-rockchip/pm_domains.c b/arch/arm/mach-rockchip/pm_domains.c
new file mode 100644
index 0000000..8f13445
--- /dev/null
+++ b/arch/arm/mach-rockchip/pm_domains.c
@@ -0,0 +1,368 @@
+/*
+ * Rockchip Generic power domain support.
+ *
+ * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
+ * Author: Hong Jinkun <jinkun.hong@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/pm_domain.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/sched.h>
+#include <linux/clk.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/spinlock.h>
+#include <linux/pm_clock.h>
+#include <linux/delay.h>
+
+#define PWR_OFFSET		0x08
+#define STATUS_OFFSET	0x0c
+#define REQ_OFFSET		0x10
+#define IDLE_OFFSET		0x14
+#define ACK_OFFSET		0x14
+
+struct rockchip_dev_entry {
+	struct list_head node;
+	struct device *dev;
+};
+
+struct rockchip_domain {
+	struct generic_pm_domain base;
+	struct device *dev;
+	struct regmap *regmap_pmu;
+	struct list_head dev_list;
+	/* spin lock for read/write pmu reg */
+	spinlock_t pmu_lock;
+	/* spin lock for dev_list */
+	spinlock_t dev_lock;
+	u32 pwr_shift;
+	u32 status_shift;
+	u32 req_shift;
+	u32 idle_shift;
+	u32 ack_shift;
+};
+
+#define to_rockchip_pd(_gpd) container_of(_gpd, struct rockchip_domain, base)
+
+static int rockchip_pmu_set_idle_request(struct rockchip_domain *pd,
+					 bool idle)
+{
+	u32 idle_mask = BIT(pd->idle_shift);
+	u32 idle_target = idle << (pd->idle_shift);
+	u32 ack_mask = BIT(pd->ack_shift);
+	u32 ack_target = idle << (pd->ack_shift);
+	unsigned int mask = BIT(pd->req_shift);
+	unsigned int val;
+	unsigned long flags;
+	int timeout = 0;
+
+	val = (idle) ? mask : 0;
+	regmap_update_bits(pd->regmap_pmu, REQ_OFFSET, mask, val);
+	dsb();
+
+	do {
+		regmap_read(pd->regmap_pmu, ACK_OFFSET, &val);
+		udelay(1);
+		if (timeout > 10) {
+			pr_err("%s wait pmu ack timeout!\n", __func__);
+			break;
+		}
+		timeout += 1;
+	} while ((val & ack_mask) != ack_target);
+
+	timeout = 0;
+	do {
+		regmap_read(pd->regmap_pmu, IDLE_OFFSET, &val);
+		udelay(1);
+		if (timeout > 10) {
+			pr_err("%s wait pmu idle timeout!\n", __func__);
+			break;
+		}
+		timeout += 1;
+	} while ((val & idle_mask) != idle_target);
+	return 0;
+}
+
+static bool rockchip_pmu_power_domain_is_on(struct rockchip_domain *pd)
+{
+	unsigned int val;
+
+	regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val);
+
+	/* 1'b0: power on, 1'b1: power off */
+	return !(val & BIT(pd->status_shift));
+}
+
+static void rockchip_do_pmu_set_power_domain(
+		struct rockchip_domain *pd, bool on)
+{
+	unsigned int mask = BIT(pd->pwr_shift);
+	unsigned int val;
+
+	val = (on) ? 0 : mask;
+	regmap_update_bits(pd->regmap_pmu, PWR_OFFSET, mask, val);
+	dsb();
+
+	do {
+		regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val);
+	} while ((val & BIT(pd->status_shift)) == on);
+}
+
+static int rockchip_pmu_set_power_domain(struct rockchip_domain *pd,
+					 bool on)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&pd->pmu_lock, flags);
+	if (rockchip_pmu_power_domain_is_on(pd) == on)
+		goto out;
+
+	if (!on) {
+		/* FIXME: add code to save AXI_QOS */
+		/* if power down, idle request to NIU first */
+		rockchip_pmu_set_idle_request(pd, true);
+	}
+
+	rockchip_do_pmu_set_power_domain(pd, on);
+
+	if (on) {
+		/* if power up, idle request release to NIU */
+		rockchip_pmu_set_idle_request(pd, false);
+		/* FIXME: add code to restore AXI_QOS */
+	}
+out:
+	spin_unlock_irqrestore(&pd->pmu_lock, flags);
+	return 0;
+}
+
+static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
+{
+	int i = 0;
+	int ret = 0;
+	struct rockchip_dev_entry *de;
+
+	spin_lock_irq(&pd->dev_lock);
+
+	list_for_each_entry(de, &pd->dev_list, node) {
+		i += 1;
+		pm_clk_resume(pd->dev);
+	}
+
+	/* no clk, set power domain will fail */
+	if (i == 0) {
+		pr_err("%s: failed to on/off power domain!", __func__);
+		spin_unlock_irq(&pd->dev_lock);
+		return ret;
+	}
+
+	ret = rockchip_pmu_set_power_domain(pd, power_on);
+
+	list_for_each_entry(de, &pd->dev_list, node) {
+		pm_clk_suspend(pd->dev);
+	}
+
+	spin_unlock_irq(&pd->dev_lock);
+
+	return ret;
+}
+
+static int rockchip_pd_power_on(struct generic_pm_domain *domain)
+{
+	struct rockchip_domain *pd = to_rockchip_pd(domain);
+
+	return rockchip_pd_power(pd, true);
+}
+
+static int rockchip_pd_power_off(struct generic_pm_domain *domain)
+{
+	struct rockchip_domain *pd = to_rockchip_pd(domain);
+
+	return rockchip_pd_power(pd, false);
+}
+
+void rockchip_pm_domain_attach_dev(struct device *dev)
+{
+	int ret;
+	int i = 0;
+	struct clk *clk;
+	struct rockchip_domain *pd;
+	struct rockchip_dev_entry *de;
+
+	pd = (struct rockchip_domain *)dev->pm_domain;
+	ret = pm_clk_create(dev);
+	if (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;
+		};
+	}
+
+	de = devm_kcalloc(pd->dev, 1,
+			sizeof(struct rockchip_dev_entry *), GFP_KERNEL);
+	spin_lock_irq(&pd->dev_lock);
+	list_add_tail(&de->node, &pd->dev_list);
+	spin_unlock_irq(&pd->dev_lock);
+
+	return;
+clk_err:
+	pm_clk_destroy(dev);
+}
+
+void rockchip_pm_domain_detach_dev(struct device *dev)
+{
+	struct rockchip_domain *pd;
+	struct rockchip_dev_entry *de;
+
+	pd = (struct rockchip_domain *)dev->pm_domain;
+	spin_lock_irq(&pd->dev_lock);
+
+	list_for_each_entry(de, &pd->dev_list, node) {
+		if (de->dev == dev)
+			goto remove;
+	}
+	spin_unlock_irq(&pd->dev_lock);
+
+	return;
+remove:
+	list_del(&de->node);
+	spin_unlock_irq(&pd->dev_lock);
+	pm_clk_destroy(dev);
+}
+
+static const struct of_device_id rockchip_pm_domain_dt_match[];
+
+static int rockchip_pm_domain_probe(struct platform_device *pdev)
+{
+	struct device_node *node;
+	struct regmap *regmap_pmu;
+	struct rockchip_domain *pd;
+	const struct of_device_id *match;
+
+	match = of_match_node(rockchip_pm_domain_dt_match, pdev->dev.of_node);
+	pd = (struct rockchip_domain *)match->data;
+	if (!pd)
+		return -ENOMEM;
+
+	node = of_parse_phandle(pdev->dev.of_node, "rockchip,pmu", 0);
+	regmap_pmu = syscon_node_to_regmap(node);
+	of_node_put(node);
+	if (IS_ERR(regmap_pmu)) {
+		pr_err("%s: failed to get regmap_pmu", __func__);
+		return PTR_ERR(regmap_pmu);
+	}
+
+	pd->regmap_pmu = regmap_pmu;
+	pd->dev = &pdev->dev;
+
+	INIT_LIST_HEAD(&pd->dev_list);
+
+	spin_lock_init(&pd->pmu_lock);
+	spin_lock_init(&pd->dev_lock);
+
+	pm_genpd_init(&pd->base, NULL, false);
+
+	return of_genpd_add_provider_simple(pdev->dev.of_node, &pd->base);
+}
+
+static struct rockchip_domain gpu_domain = {
+	.base = {
+		.name = "pd_gpu",
+		.attach_dev = rockchip_pm_domain_attach_dev,
+		.detach_dev = rockchip_pm_domain_detach_dev,
+		.power_off = rockchip_pd_power_off,
+		.power_on = rockchip_pd_power_on,
+	},
+	.pwr_shift = 9,
+	.status_shift = 9,
+	.req_shift = 2,
+	.idle_shift = 2,
+	.ack_shift = 18,
+};
+
+static struct rockchip_domain hevc_domain = {
+	.base = {
+		.name = "pd_hevc",
+		.attach_dev = rockchip_pm_domain_attach_dev,
+		.detach_dev = rockchip_pm_domain_detach_dev,
+		.power_off = rockchip_pd_power_off,
+		.power_on = rockchip_pd_power_on,
+	},
+	.pwr_shift = 14,
+	.status_shift = 10,
+	.req_shift = 9,
+	.idle_shift = 9,
+	.ack_shift = 25,
+
+};
+
+static struct rockchip_domain video_domain = {
+	.base = {
+		.name = "pd_video",
+		.attach_dev = rockchip_pm_domain_attach_dev,
+		.detach_dev = rockchip_pm_domain_detach_dev,
+		.power_off = rockchip_pd_power_off,
+		.power_on = rockchip_pd_power_on,
+	},
+	.pwr_shift = 8,
+	.status_shift = 8,
+	.req_shift = 3,
+	.idle_shift = 3,
+	.ack_shift = 19,
+};
+
+static struct rockchip_domain vio_domain = {
+	.base = {
+		.name = "pd_vio",
+		.attach_dev = rockchip_pm_domain_attach_dev,
+		.detach_dev = rockchip_pm_domain_detach_dev,
+		.power_off = rockchip_pd_power_off,
+		.power_on = rockchip_pd_power_on,
+	},
+	.pwr_shift = 7,
+	.status_shift = 7,
+	.req_shift = 4,
+	.idle_shift = 4,
+	.ack_shift = 20,
+};
+
+static const struct of_device_id rockchip_pm_domain_dt_match[] = {
+	{ .compatible = "rockchip,rk3288-power-gpu",
+		.data = (void *)&gpu_domain},
+	{ .compatible = "rockchip,rk3288-power-hevc",
+		.data = (void *)&hevc_domain},
+	{ .compatible = "rockchip,rk3288-power-video",
+			.data = (void *)&video_domain},
+	{ .compatible = "rockchip,rk3288-power-vio",
+		.data = (void *)&vio_domain},
+	{},
+};
+MODULE_DEVICE_TABLE(of, rockchip_pm_domain_dt_match);
+
+static struct platform_driver rockchip_pm_domain_driver = {
+	.probe = rockchip_pm_domain_probe,
+	.driver = {
+		.name   = "rockchip-pm-domain",
+		.owner  = THIS_MODULE,
+		.of_match_table = rockchip_pm_domain_dt_match,
+	},
+};
+
+static int __init rockchip_pm_domain_drv_register(void)
+{
+	return platform_driver_register(&rockchip_pm_domain_driver);
+}
+postcore_initcall(rockchip_pm_domain_drv_register);
-- 
1.7.9.5

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Dmitry Torokhov]

Hi Jinkun,

On Tue, Oct 21, 2014 at 02:13:26AM -0700, jinkun.hong wrote:
> From: "jinkun.hong" <jinkun.hong@rock-chips.com>
> 
> Add power domain drivers based on generic power domain for Rockchip platform,
> and support RK3288.
> 
> Signed-off-by: Jack Dai <jack.dai@rock-chips.com>
> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
> 
> ---
> 
> Changes in v5:
> - delete idle_lock
> - add timeout in rockchip_pmu_set_idle_request()
> 
> Changes in v4:
> - use list storage dev
> 
> Changes in v3:
> - change use pm_clk_resume() and pm_clk_suspend()
> 
> Changes in v2:
> - remove the "pd->pd.of_node = np"
> 
>  arch/arm/mach-rockchip/Kconfig      |    1 +
>  arch/arm/mach-rockchip/Makefile     |    1 +
>  arch/arm/mach-rockchip/pm_domains.c |  368 +++++++++++++++++++++++++++++++++++
>  3 files changed, 370 insertions(+)
>  create mode 100644 arch/arm/mach-rockchip/pm_domains.c
> 
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index d168669..4920a88 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -12,6 +12,7 @@ config ARCH_ROCKCHIP
>  	select DW_APB_TIMER_OF
>  	select ARM_GLOBAL_TIMER
>  	select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> +	select PM_GENERIC_DOMAINS if PM
>  	help
>  	  Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs
>  	  containing the RK2928, RK30xx and RK31xx series.
> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
> index b29d8ea..805268d 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -2,3 +2,4 @@ CFLAGS_platsmp.o := -march=armv7-a
>  
>  obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
>  obj-$(CONFIG_SMP) += headsmp.o platsmp.o
> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
> diff --git a/arch/arm/mach-rockchip/pm_domains.c b/arch/arm/mach-rockchip/pm_domains.c
> new file mode 100644
> index 0000000..8f13445
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/pm_domains.c
> @@ -0,0 +1,368 @@
> +/*
> + * Rockchip Generic power domain support.
> + *
> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
> + * Author: Hong Jinkun <jinkun.hong@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/pm_domain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/sched.h>
> +#include <linux/clk.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/spinlock.h>
> +#include <linux/pm_clock.h>
> +#include <linux/delay.h>
> +
> +#define PWR_OFFSET		0x08
> +#define STATUS_OFFSET	0x0c
> +#define REQ_OFFSET		0x10
> +#define IDLE_OFFSET		0x14
> +#define ACK_OFFSET		0x14
> +
> +struct rockchip_dev_entry {
> +	struct list_head node;
> +	struct device *dev;
> +};
> +
> +struct rockchip_domain {
> +	struct generic_pm_domain base;
> +	struct device *dev;
> +	struct regmap *regmap_pmu;
> +	struct list_head dev_list;
> +	/* spin lock for read/write pmu reg */
> +	spinlock_t pmu_lock;
> +	/* spin lock for dev_list */
> +	spinlock_t dev_lock;
> +	u32 pwr_shift;
> +	u32 status_shift;
> +	u32 req_shift;
> +	u32 idle_shift;
> +	u32 ack_shift;
> +};
> +
> +#define to_rockchip_pd(_gpd) container_of(_gpd, struct rockchip_domain, base)
> +
> +static int rockchip_pmu_set_idle_request(struct rockchip_domain *pd,
> +					 bool idle)
> +{
> +	u32 idle_mask = BIT(pd->idle_shift);
> +	u32 idle_target = idle << (pd->idle_shift);
> +	u32 ack_mask = BIT(pd->ack_shift);
> +	u32 ack_target = idle << (pd->ack_shift);
> +	unsigned int mask = BIT(pd->req_shift);
> +	unsigned int val;
> +	unsigned long flags;
> +	int timeout = 0;
> +
> +	val = (idle) ? mask : 0;
> +	regmap_update_bits(pd->regmap_pmu, REQ_OFFSET, mask, val);
> +	dsb();
> +
> +	do {
> +		regmap_read(pd->regmap_pmu, ACK_OFFSET, &val);
> +		udelay(1);
> +		if (timeout > 10) {
> +			pr_err("%s wait pmu ack timeout!\n", __func__);
> +			break;
> +		}
> +		timeout += 1;
> +	} while ((val & ack_mask) != ack_target);
> +
> +	timeout = 0;
> +	do {
> +		regmap_read(pd->regmap_pmu, IDLE_OFFSET, &val);
> +		udelay(1);
> +		if (timeout > 10) {
> +			pr_err("%s wait pmu idle timeout!\n", __func__);
> +			break;
> +		}
> +		timeout += 1;
> +	} while ((val & idle_mask) != idle_target);
> +	return 0;
> +}
> +
> +static bool rockchip_pmu_power_domain_is_on(struct rockchip_domain *pd)
> +{
> +	unsigned int val;
> +
> +	regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val);
> +
> +	/* 1'b0: power on, 1'b1: power off */
> +	return !(val & BIT(pd->status_shift));
> +}
> +
> +static void rockchip_do_pmu_set_power_domain(
> +		struct rockchip_domain *pd, bool on)
> +{
> +	unsigned int mask = BIT(pd->pwr_shift);
> +	unsigned int val;
> +
> +	val = (on) ? 0 : mask;
> +	regmap_update_bits(pd->regmap_pmu, PWR_OFFSET, mask, val);
> +	dsb();
> +
> +	do {
> +		regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val);
> +	} while ((val & BIT(pd->status_shift)) == on);
> +}
> +
> +static int rockchip_pmu_set_power_domain(struct rockchip_domain *pd,
> +					 bool on)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pd->pmu_lock, flags);

You only call rockchip_pmu_set_power_domain() with pd->dev_lock held,
why do you need this spinlock?

> +	if (rockchip_pmu_power_domain_is_on(pd) == on)
> +		goto out;
> +
> +	if (!on) {
> +		/* FIXME: add code to save AXI_QOS */
> +		/* if power down, idle request to NIU first */
> +		rockchip_pmu_set_idle_request(pd, true);
> +	}
> +
> +	rockchip_do_pmu_set_power_domain(pd, on);
> +
> +	if (on) {
> +		/* if power up, idle request release to NIU */
> +		rockchip_pmu_set_idle_request(pd, false);
> +		/* FIXME: add code to restore AXI_QOS */
> +	}
> +out:
> +	spin_unlock_irqrestore(&pd->pmu_lock, flags);
> +	return 0;
> +}
> +
> +static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
> +{
> +	int i = 0;
> +	int ret = 0;
> +	struct rockchip_dev_entry *de;
> +
> +	spin_lock_irq(&pd->dev_lock);

Do you really need to run here with interrupts off? Maybe a mutex would
be better here?

> +
> +	list_for_each_entry(de, &pd->dev_list, node) {
> +		i += 1;
> +		pm_clk_resume(pd->dev);

Do you really need to call pm_clk_resume() number of times that there
are devices in power domain? Did you want it to be

		pm_clk_resume(de->dev);

by any chance?

> +	}
> +
> +	/* no clk, set power domain will fail */
> +	if (i == 0) {
> +		pr_err("%s: failed to on/off power domain!", __func__);
> +		spin_unlock_irq(&pd->dev_lock);
> +		return ret;
> +	}

Instead of counting I'd do

	if (list_empty(&pd->dev_list)) {
		pr_waen("%s: no devices in power domain\n", __func__);
		goto out;
	}

in the beginning of the function.

> +
> +	ret = rockchip_pmu_set_power_domain(pd, power_on);
> +
> +	list_for_each_entry(de, &pd->dev_list, node) {
> +		pm_clk_suspend(pd->dev);

Same here?

> +	}
> +
> +	spin_unlock_irq(&pd->dev_lock);
> +
> +	return ret;
> +}
> +
> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
> +{
> +	struct rockchip_domain *pd = to_rockchip_pd(domain);
> +
> +	return rockchip_pd_power(pd, true);
> +}
> +
> +static int rockchip_pd_power_off(struct generic_pm_domain *domain)
> +{
> +	struct rockchip_domain *pd = to_rockchip_pd(domain);
> +
> +	return rockchip_pd_power(pd, false);
> +}
> +
> +void rockchip_pm_domain_attach_dev(struct device *dev)
> +{
> +	int ret;
> +	int i = 0;
> +	struct clk *clk;
> +	struct rockchip_domain *pd;
> +	struct rockchip_dev_entry *de;
> +
> +	pd = (struct rockchip_domain *)dev->pm_domain;
> +	ret = pm_clk_create(dev);
> +	if (ret) {
> +		dev_err(dev, "pm_clk_create failed %d\n", ret);
> +		return;
> +	};

Stray semicolon.

> +
> +	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;
> +		};
> +	}
> +
> +	de = devm_kcalloc(pd->dev, 1,
> +			sizeof(struct rockchip_dev_entry *), GFP_KERNEL);

Why devm_calloc for a single element and not devm_kzalloc? Also, I am a
bit concerned about using devm_* API here. They are better reserved fir
driver's ->probe() paths whereas we are called from
dev_pm_domain_attach() which is more general API (yes, currently it is
used by buses probing code, but that might change in the future).

Also, where is OOM error handling?

> +	spin_lock_irq(&pd->dev_lock);
> +	list_add_tail(&de->node, &pd->dev_list);
> +	spin_unlock_irq(&pd->dev_lock);
> +
> +	return;
> +clk_err:
> +	pm_clk_destroy(dev);
> +}
> +
> +void rockchip_pm_domain_detach_dev(struct device *dev)
> +{
> +	struct rockchip_domain *pd;
> +	struct rockchip_dev_entry *de;
> +
> +	pd = (struct rockchip_domain *)dev->pm_domain;
> +	spin_lock_irq(&pd->dev_lock);
> +
> +	list_for_each_entry(de, &pd->dev_list, node) {
> +		if (de->dev == dev)
> +			goto remove;

If you use mutex instead of spinlock I think you'll be able to do
list_del/pm_clk_destroy() right here. I'd free memory here as well.

Thanks.

-- 
Dmitry

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Hong jinkun]

On 2014/10/22 3:58, Dmitry Torokhov wrote:
> Hi Jinkun,
>
> On Tue, Oct 21, 2014 at 02:13:26AM -0700, jinkun.hong wrote:
>> From: "jinkun.hong" <jinkun.hong@rock-chips.com>
>>
>> Add power domain drivers based on generic power domain for Rockchip platform,
>> and support RK3288.
>>
>> Signed-off-by: Jack Dai <jack.dai@rock-chips.com>
>> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
>>
>> ---
>>
>> Changes in v5:
>> - delete idle_lock
>> - add timeout in rockchip_pmu_set_idle_request()
>>
>> Changes in v4:
>> - use list storage dev
>>
>> Changes in v3:
>> - change use pm_clk_resume() and pm_clk_suspend()
>>
>> Changes in v2:
>> - remove the "pd->pd.of_node = np"
>>
>>   arch/arm/mach-rockchip/Kconfig      |    1 +
>>   arch/arm/mach-rockchip/Makefile     |    1 +
>>   arch/arm/mach-rockchip/pm_domains.c |  368 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 370 insertions(+)
>>   create mode 100644 arch/arm/mach-rockchip/pm_domains.c
>>
>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
>> index d168669..4920a88 100644
>> --- a/arch/arm/mach-rockchip/Kconfig
>> +++ b/arch/arm/mach-rockchip/Kconfig
>> @@ -12,6 +12,7 @@ config ARCH_ROCKCHIP
>>   	select DW_APB_TIMER_OF
>>   	select ARM_GLOBAL_TIMER
>>   	select CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>> +	select PM_GENERIC_DOMAINS if PM
>>   	help
>>   	  Support for Rockchip's Cortex-A9 Single-to-Quad-Core-SoCs
>>   	  containing the RK2928, RK30xx and RK31xx series.
>> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile
>> index b29d8ea..805268d 100644
>> --- a/arch/arm/mach-rockchip/Makefile
>> +++ b/arch/arm/mach-rockchip/Makefile
>> @@ -2,3 +2,4 @@ CFLAGS_platsmp.o := -march=armv7-a
>>   
>>   obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
>>   obj-$(CONFIG_SMP) += headsmp.o platsmp.o
>> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
>> diff --git a/arch/arm/mach-rockchip/pm_domains.c b/arch/arm/mach-rockchip/pm_domains.c
>> new file mode 100644
>> index 0000000..8f13445
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/pm_domains.c
>> @@ -0,0 +1,368 @@
>> +/*
>> + * Rockchip Generic power domain support.
>> + *
>> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
>> + * Author: Hong Jinkun <jinkun.hong@rock-chips.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/sched.h>
>> +#include <linux/clk.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/delay.h>
>> +
>> +#define PWR_OFFSET		0x08
>> +#define STATUS_OFFSET	0x0c
>> +#define REQ_OFFSET		0x10
>> +#define IDLE_OFFSET		0x14
>> +#define ACK_OFFSET		0x14
>> +
>> +struct rockchip_dev_entry {
>> +	struct list_head node;
>> +	struct device *dev;
>> +};
>> +
>> +struct rockchip_domain {
>> +	struct generic_pm_domain base;
>> +	struct device *dev;
>> +	struct regmap *regmap_pmu;
>> +	struct list_head dev_list;
>> +	/* spin lock for read/write pmu reg */
>> +	spinlock_t pmu_lock;
>> +	/* spin lock for dev_list */
>> +	spinlock_t dev_lock;
>> +	u32 pwr_shift;
>> +	u32 status_shift;
>> +	u32 req_shift;
>> +	u32 idle_shift;
>> +	u32 ack_shift;
>> +};
>> +
>> +#define to_rockchip_pd(_gpd) container_of(_gpd, struct rockchip_domain, base)
>> +
>> +static int rockchip_pmu_set_idle_request(struct rockchip_domain *pd,
>> +					 bool idle)
>> +{
>> +	u32 idle_mask = BIT(pd->idle_shift);
>> +	u32 idle_target = idle << (pd->idle_shift);
>> +	u32 ack_mask = BIT(pd->ack_shift);
>> +	u32 ack_target = idle << (pd->ack_shift);
>> +	unsigned int mask = BIT(pd->req_shift);
>> +	unsigned int val;
>> +	unsigned long flags;
>> +	int timeout = 0;
>> +
>> +	val = (idle) ? mask : 0;
>> +	regmap_update_bits(pd->regmap_pmu, REQ_OFFSET, mask, val);
>> +	dsb();
>> +
>> +	do {
>> +		regmap_read(pd->regmap_pmu, ACK_OFFSET, &val);
>> +		udelay(1);
>> +		if (timeout > 10) {
>> +			pr_err("%s wait pmu ack timeout!\n", __func__);
>> +			break;
>> +		}
>> +		timeout += 1;
>> +	} while ((val & ack_mask) != ack_target);
>> +
>> +	timeout = 0;
>> +	do {
>> +		regmap_read(pd->regmap_pmu, IDLE_OFFSET, &val);
>> +		udelay(1);
>> +		if (timeout > 10) {
>> +			pr_err("%s wait pmu idle timeout!\n", __func__);
>> +			break;
>> +		}
>> +		timeout += 1;
>> +	} while ((val & idle_mask) != idle_target);
>> +	return 0;
>> +}
>> +
>> +static bool rockchip_pmu_power_domain_is_on(struct rockchip_domain *pd)
>> +{
>> +	unsigned int val;
>> +
>> +	regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val);
>> +
>> +	/* 1'b0: power on, 1'b1: power off */
>> +	return !(val & BIT(pd->status_shift));
>> +}
>> +
>> +static void rockchip_do_pmu_set_power_domain(
>> +		struct rockchip_domain *pd, bool on)
>> +{
>> +	unsigned int mask = BIT(pd->pwr_shift);
>> +	unsigned int val;
>> +
>> +	val = (on) ? 0 : mask;
>> +	regmap_update_bits(pd->regmap_pmu, PWR_OFFSET, mask, val);
>> +	dsb();
>> +
>> +	do {
>> +		regmap_read(pd->regmap_pmu, STATUS_OFFSET, &val);
>> +	} while ((val & BIT(pd->status_shift)) == on);
>> +}
>> +
>> +static int rockchip_pmu_set_power_domain(struct rockchip_domain *pd,
>> +					 bool on)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&pd->pmu_lock, flags);
> You only call rockchip_pmu_set_power_domain() with pd->dev_lock held,
> why do you need this spinlock?
I will remove the pmu_lock.
>> +	if (rockchip_pmu_power_domain_is_on(pd) == on)
>> +		goto out;
>> +
>> +	if (!on) {
>> +		/* FIXME: add code to save AXI_QOS */
>> +		/* if power down, idle request to NIU first */
>> +		rockchip_pmu_set_idle_request(pd, true);
>> +	}
>> +
>> +	rockchip_do_pmu_set_power_domain(pd, on);
>> +
>> +	if (on) {
>> +		/* if power up, idle request release to NIU */
>> +		rockchip_pmu_set_idle_request(pd, false);
>> +		/* FIXME: add code to restore AXI_QOS */
>> +	}
>> +out:
>> +	spin_unlock_irqrestore(&pd->pmu_lock, flags);
>> +	return 0;
>> +}
>> +
>> +static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
>> +{
>> +	int i = 0;
>> +	int ret = 0;
>> +	struct rockchip_dev_entry *de;
>> +
>> +	spin_lock_irq(&pd->dev_lock);
> Do you really need to run here with interrupts off? Maybe a mutex would
> be better here?
Ok,I use mutex.
>> +
>> +	list_for_each_entry(de, &pd->dev_list, node) {
>> +		i += 1;
>> +		pm_clk_resume(pd->dev);
> Do you really need to call pm_clk_resume() number of times that there
> are devices in power domain? Did you want it to be
>
> 		pm_clk_resume(de->dev);
>
> by any chance?
You are right.I will modify in the next version.
>> +	}
>> +
>> +	/* no clk, set power domain will fail */
>> +	if (i == 0) {
>> +		pr_err("%s: failed to on/off power domain!", __func__);
>> +		spin_unlock_irq(&pd->dev_lock);
>> +		return ret;
>> +	}
> Instead of counting I'd do
>
> 	if (list_empty(&pd->dev_list)) {
> 		pr_waen("%s: no devices in power domain\n", __func__);
> 		goto out;
> 	}
>
> in the beginning of the function.
This is a good idea.
>> +
>> +	ret = rockchip_pmu_set_power_domain(pd, power_on);
>> +
>> +	list_for_each_entry(de, &pd->dev_list, node) {
>> +		pm_clk_suspend(pd->dev);
> Same here?
>
>> +	}
>> +
>> +	spin_unlock_irq(&pd->dev_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>> +{
>> +	struct rockchip_domain *pd = to_rockchip_pd(domain);
>> +
>> +	return rockchip_pd_power(pd, true);
>> +}
>> +
>> +static int rockchip_pd_power_off(struct generic_pm_domain *domain)
>> +{
>> +	struct rockchip_domain *pd = to_rockchip_pd(domain);
>> +
>> +	return rockchip_pd_power(pd, false);
>> +}
>> +
>> +void rockchip_pm_domain_attach_dev(struct device *dev)
>> +{
>> +	int ret;
>> +	int i = 0;
>> +	struct clk *clk;
>> +	struct rockchip_domain *pd;
>> +	struct rockchip_dev_entry *de;
>> +
>> +	pd = (struct rockchip_domain *)dev->pm_domain;
>> +	ret = pm_clk_create(dev);
>> +	if (ret) {
>> +		dev_err(dev, "pm_clk_create failed %d\n", ret);
>> +		return;
>> +	};
> Stray semicolon.
>> +
>> +	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;
>> +		};
>> +	}
>> +
>> +	de = devm_kcalloc(pd->dev, 1,
>> +			sizeof(struct rockchip_dev_entry *), GFP_KERNEL);
> Why devm_calloc for a single element and not devm_kzalloc? Also, I am a
> bit concerned about using devm_* API here. They are better reserved fir
> driver's ->probe() paths whereas we are called from
> dev_pm_domain_attach() which is more general API (yes, currently it is
> used by buses probing code, but that might change in the future).
>
> Also, where is OOM error handling?
Ok,I will change the use  devm_kzalloc.
Register to pm domain devices, the number is not a lot.
>> +	spin_lock_irq(&pd->dev_lock);
>> +	list_add_tail(&de->node, &pd->dev_list);
>> +	spin_unlock_irq(&pd->dev_lock);
>> +
>> +	return;
>> +clk_err:
>> +	pm_clk_destroy(dev);
>> +}
>> +
>> +void rockchip_pm_domain_detach_dev(struct device *dev)
>> +{
>> +	struct rockchip_domain *pd;
>> +	struct rockchip_dev_entry *de;
>> +
>> +	pd = (struct rockchip_domain *)dev->pm_domain;
>> +	spin_lock_irq(&pd->dev_lock);
>> +
>> +	list_for_each_entry(de, &pd->dev_list, node) {
>> +		if (de->dev == dev)
>> +			goto remove;
> If you use mutex instead of spinlock I think you'll be able to do
> list_del/pm_clk_destroy() right here. I'd free memory here as well.
OK
> Thanks.
>

Thank you for your review.

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Ulf Hansson]

[...]

>>>
>>> +
>>> +       list_for_each_entry(de, &pd->dev_list, node) {
>>> +               i += 1;
>>> +               pm_clk_resume(pd->dev);
>>
>> Do you really need to call pm_clk_resume() number of times that there
>> are devices in power domain? Did you want it to be
>>
>>                 pm_clk_resume(de->dev);
>>
>> by any chance?

I was just about to ask the similar question as Dmitry did. :-)

>
> You are right.I will modify in the next version.

Now, does that also mean you would like to assign the ->start|stop()
callbacks in the struct gpd_dev_ops to pm_clk_suspend|resume()? Or do
you intend to handle that from each driver instead?

>>>
>>> +       }
>>> +
>>> +       /* no clk, set power domain will fail */
>>> +       if (i == 0) {
>>> +               pr_err("%s: failed to on/off power domain!", __func__);
>>> +               spin_unlock_irq(&pd->dev_lock);
>>> +               return ret;
>>> +       }
>>
>> Instead of counting I'd do
>>
>>         if (list_empty(&pd->dev_list)) {
>>                 pr_waen("%s: no devices in power domain\n", __func__);
>>                 goto out;
>>         }
>>
>> in the beginning of the function.
>
> This is a good idea.
>
>>> +
>>> +       ret = rockchip_pmu_set_power_domain(pd, power_on);
>>> +
>>> +       list_for_each_entry(de, &pd->dev_list, node) {
>>> +               pm_clk_suspend(pd->dev);
>>
>> Same here?
>>
>>> +       }
>>> +
>>> +       spin_unlock_irq(&pd->dev_lock);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>> +{
>>> +       struct rockchip_domain *pd = to_rockchip_pd(domain);
>>> +
>>> +       return rockchip_pd_power(pd, true);
>>> +}
>>> +
>>> +static int rockchip_pd_power_off(struct generic_pm_domain *domain)
>>> +{
>>> +       struct rockchip_domain *pd = to_rockchip_pd(domain);
>>> +
>>> +       return rockchip_pd_power(pd, false);
>>> +}
>>> +
>>> +void rockchip_pm_domain_attach_dev(struct device *dev)
>>> +{
>>> +       int ret;
>>> +       int i = 0;
>>> +       struct clk *clk;
>>> +       struct rockchip_domain *pd;
>>> +       struct rockchip_dev_entry *de;
>>> +
>>> +       pd = (struct rockchip_domain *)dev->pm_domain;
>>> +       ret = pm_clk_create(dev);
>>> +       if (ret) {
>>> +               dev_err(dev, "pm_clk_create failed %d\n", ret);
>>> +               return;
>>> +       };
>>
>> Stray semicolon.
>>>
>>> +
>>> +       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;
>>> +               };
>>> +       }
>>> +
>>> +       de = devm_kcalloc(pd->dev, 1,
>>> +                       sizeof(struct rockchip_dev_entry *), GFP_KERNEL);
>>
>> Why devm_calloc for a single element and not devm_kzalloc? Also, I am a
>> bit concerned about using devm_* API here. They are better reserved fir
>> driver's ->probe() paths whereas we are called from
>> dev_pm_domain_attach() which is more general API (yes, currently it is
>> used by buses probing code, but that might change in the future).

Using the devm_*API is supposed to work from here. I have kept this in
mind, while we added the new dev_pm_domain_attach|detach() API. The
buses also handles -EPROBE_DEFER.

Now, I just realized that while Geert added attach|detach_dev()
callbacks for the generic PM domain, those are both "void" callbacks.
It means the deferred probe error handling is broken for these
callbacks. We should convert the attach_dev() callback into an int, I
will cook a patch immediately.

>>
>> Also, where is OOM error handling?
>
> Ok,I will change the use  devm_kzalloc.
> Register to pm domain devices, the number is not a lot.

[...]

Kind regards
Uffe

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Geert Uytterhoeven]

On Wed, Oct 22, 2014 at 9:58 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Now, I just realized that while Geert added attach|detach_dev()
> callbacks for the generic PM domain, those are both "void" callbacks.
> It means the deferred probe error handling is broken for these
> callbacks. We should convert the attach_dev() callback into an int, I
> will cook a patch immediately.

That was changed in v3:
  - Move calling of the callbacks to a point where dev->pm_domain is
    valid, so the domain parameter is no longer needed,
  - Make callbacks return void.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Ulf Hansson]

On 22 October 2014 10:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Oct 22, 2014 at 9:58 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Now, I just realized that while Geert added attach|detach_dev()
>> callbacks for the generic PM domain, those are both "void" callbacks.
>> It means the deferred probe error handling is broken for these
>> callbacks. We should convert the attach_dev() callback into an int, I
>> will cook a patch immediately.
>
> That was changed in v3:
>   - Move calling of the callbacks to a point where dev->pm_domain is
>     valid, so the domain parameter is no longer needed,

That's good.

>   - Make callbacks return void.
>

But not this. :-) Sorry, for being too quick in reviewing that time.

Kind regards
Uffe

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Dmitry Torokhov]

On Wed, Oct 22, 2014 at 09:58:34AM +0200, Ulf Hansson wrote:
> 
> Using the devm_*API is supposed to work from here. I have kept this in
> mind, while we added the new dev_pm_domain_attach|detach() API. The
> buses also handles -EPROBE_DEFER.

How do you ensure this? You have no control over from where API is
called. If I happen to call it form anywhere but bus probe code path I
will be leaking memory at least until the device is unbound completely
(which would be in this case an unrelated event).

Thanks.

-- 
Dmitry

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Ulf Hansson]

On 22 October 2014 19:45, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Wed, Oct 22, 2014 at 09:58:34AM +0200, Ulf Hansson wrote:
>>
>> Using the devm_*API is supposed to work from here. I have kept this in
>> mind, while we added the new dev_pm_domain_attach|detach() API. The
>> buses also handles -EPROBE_DEFER.
>
> How do you ensure this? You have no control over from where API is
> called. If I happen to call it form anywhere but bus probe code path I
> will be leaking memory at least until the device is unbound completely
> (which would be in this case an unrelated event).

You are right, devm_*API is currently not a good idea! I didn't
realize that the ->attach_dev() callback was invoked from
__pm_genpd_add_device(). Sorry for the noise.

Anyway,  I am working on adding the option to return -EPROBE_DEFER
from the ->attach_dev() callback, since that could be useful to
support.

Kind regards
Uffe

Re: [PATCH v5 1/3] power-domain: add power domain drivers for Rockchip platform

[Hong jinkun]

On 2014/10/22 15:58, Ulf Hansson wrote:
> [...]
>
>>>> +
>>>> +       list_for_each_entry(de, &pd->dev_list, node) {
>>>> +               i += 1;
>>>> +               pm_clk_resume(pd->dev);
>>> Do you really need to call pm_clk_resume() number of times that there
>>> are devices in power domain? Did you want it to be
>>>
>>>                  pm_clk_resume(de->dev);
>>>
>>> by any chance?
> I was just about to ask the similar question as Dmitry did. :-)
>
>> You are right.I will modify in the next version.
> Now, does that also mean you would like to assign the ->start|stop()
> callbacks in the struct gpd_dev_ops to pm_clk_suspend|resume()? Or do
> you intend to handle that from each driver instead?
If it can call dev_ops.start before calling power_on and power_off is 
the best.But I found dev_ops.start not called.Is not I did add some patch?
>>>> +       }
>>>> +
>>>> +       /* no clk, set power domain will fail */
>>>> +       if (i == 0) {
>>>> +               pr_err("%s: failed to on/off power domain!", __func__);
>>>> +               spin_unlock_irq(&pd->dev_lock);
>>>> +               return ret;
>>>> +       }
>>> Instead of counting I'd do
>>>
>>>          if (list_empty(&pd->dev_list)) {
>>>                  pr_waen("%s: no devices in power domain\n", __func__);
>>>                  goto out;
>>>          }
>>>
>>> in the beginning of the function.
>> This is a good idea.
>>
>>>> +
>>>> +       ret = rockchip_pmu_set_power_domain(pd, power_on);
>>>> +
>>>> +       list_for_each_entry(de, &pd->dev_list, node) {
>>>> +               pm_clk_suspend(pd->dev);
>>> Same here?
>>>
>>>> +       }
>>>> +
>>>> +       spin_unlock_irq(&pd->dev_lock);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>>> +{
>>>> +       struct rockchip_domain *pd = to_rockchip_pd(domain);
>>>> +
>>>> +       return rockchip_pd_power(pd, true);
>>>> +}
>>>> +
>>>> +static int rockchip_pd_power_off(struct generic_pm_domain *domain)
>>>> +{
>>>> +       struct rockchip_domain *pd = to_rockchip_pd(domain);
>>>> +
>>>> +       return rockchip_pd_power(pd, false);
>>>> +}
>>>> +
>>>> +void rockchip_pm_domain_attach_dev(struct device *dev)
>>>> +{
>>>> +       int ret;
>>>> +       int i = 0;
>>>> +       struct clk *clk;
>>>> +       struct rockchip_domain *pd;
>>>> +       struct rockchip_dev_entry *de;
>>>> +
>>>> +       pd = (struct rockchip_domain *)dev->pm_domain;
>>>> +       ret = pm_clk_create(dev);
>>>> +       if (ret) {
>>>> +               dev_err(dev, "pm_clk_create failed %d\n", ret);
>>>> +               return;
>>>> +       };
>>> Stray semicolon.
>>>> +
>>>> +       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;
>>>> +               };
>>>> +       }
>>>> +
>>>> +       de = devm_kcalloc(pd->dev, 1,
>>>> +                       sizeof(struct rockchip_dev_entry *), GFP_KERNEL);
>>> Why devm_calloc for a single element and not devm_kzalloc? Also, I am a
>>> bit concerned about using devm_* API here. They are better reserved fir
>>> driver's ->probe() paths whereas we are called from
>>> dev_pm_domain_attach() which is more general API (yes, currently it is
>>> used by buses probing code, but that might change in the future).
> Using the devm_*API is supposed to work from here. I have kept this in
> mind, while we added the new dev_pm_domain_attach|detach() API. The
> buses also handles -EPROBE_DEFER.
>
> Now, I just realized that while Geert added attach|detach_dev()
> callbacks for the generic PM domain, those are both "void" callbacks.
> It means the deferred probe error handling is broken for these
> callbacks. We should convert the attach_dev() callback into an int, I
> will cook a patch immediately.
>
>>> Also, where is OOM error handling?
>> Ok,I will change the use  devm_kzalloc.
>> Register to pm domain devices, the number is not a lot.
> [...]
>
> Kind regards
> Uffe
>
>
>

[PATCH v5 2/3] dt-bindings: add document of Rockchip power domain

[jinkun.hong]

From: "jinkun.hong" <jinkun.hong@rock-chips.com>

Signed-off-by: Jack Dai <jack.dai@rock-chips.com>
Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>

---

Changes in v5: None
Changes in v4: None
Changes in v3:
- DT structure has changed

Changes in v2:
- move clocks to "optional"

 .../bindings/arm/rockchip/power_domain.txt         |   46 ++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/power_domain.txt

diff --git a/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
new file mode 100644
index 0000000..f8357b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
@@ -0,0 +1,46 @@
+* Rockchip Power Domains
+
+Rockchip processors include support for multiple power domains which can be
+powered up/down by software based on different application scenes to save power.
+
+Required properties for power domain controller:
+- compatible: should be one of the following.
+    * rockchip,rk3288-power-gpu - for rk3288 type gpu power domain.
+    * rockchip,rk3288-power-hevc - for rk3288 type hevc power domain.
+    * rockchip,rk3288-power-video - for rk3288 type video power domain.
+    * rockchip,rk3288-power-vio - for rk3288 type vio power domain.
+- rockchip,pmu: phandle referencing a syscon providing the pmu registers
+- #power-domain-cells: Number of cells in a power-domain specifier.
+		       should be 0.
+
+Example:
+
+	gpu_power: gpu-power-controller {
+		compatible = "rockchip,rk3288-power-gpu";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
+
+	hevc_power: hevc-power-controller {
+		compatible = "rockchip,rk3288-power-hevc";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
+
+	video_power: video-power-controller {
+		compatible = "rockchip,rk3288-power-video";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
+
+	vio_power: vio-power-controller {
+		compatible = "rockchip,rk3288-power-vio";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
+
+	gpu: gpu@0xffa30000 {
+		...
+		power-domains = <&gpu_power>;
+		...
+	};
-- 
1.7.9.5

[PATCH v5 3/3] ARM: dts: add rk3288 power-domain node

[jinkun.hong]

From: "jinkun.hong" <jinkun.hong@rock-chips.com>

Signed-off-by: Jack Dai <jack.dai@rock-chips.com>
Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>

---

Changes in v5: None
Changes in v4: None
Changes in v3:
- Decomposition power-controller, changed to multiple controller
   (gpu-power-controller, hevc-power-controller)

Changes in v2:
- make pd_vio clocks all one entry per line and alphabetize.
- power: power-controller move back to pinctrl: pinctrl.

 arch/arm/boot/dts/rk3288.dtsi |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 5950b0a..6a9b313 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -592,4 +592,28 @@
 			};
 		};
 	};
+
+	gpu_power: gpu-power-controller {
+		compatible = "rockchip,rk3288-power-gpu";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
+
+	hevc_power: hevc-power-controller {
+		compatible = "rockchip,rk3288-power-hevc";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
+
+	video_power: video-power-controller {
+		compatible = "rockchip,rk3288-power-video";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
+
+	vio_power: vio-power-controller {
+		compatible = "rockchip,rk3288-power-vio";
+		rockchip,pmu = <&pmu>;
+		#power-domain-cells = <0>;
+	};
 };
-- 
1.7.9.5