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

[PATCH v7 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 v7:
- Delete unused variables

Changes in v6:
- delete pmu_lock
- modify dev_lock using mutex
- pm_clk_resume(pd->dev) change to pm_clk_resume(ed->dev)
- pm_clk_suspend(pd->dev) change to pm_clk_suspend(ed->dev)
- add devm_kfree(pd->dev, de) in rockchip_pm_domain_detach_dev

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                |  355 ++++++++++++++++++++
 5 files changed, 427 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 v7 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 v7:
- Delete unused variables

Changes in v6:
- delete pmu_lock
- modify dev_lock using mutex
- pm_clk_resume(pd->dev) change to pm_clk_resume(ed->dev)
- pm_clk_suspend(pd->dev) change to pm_clk_suspend(ed->dev)
- add devm_kfree(pd->dev, de) in rockchip_pm_domain_detach_dev

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 |  355 +++++++++++++++++++++++++++++++++++
 3 files changed, 357 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..f6d2f79
--- /dev/null
+++ b/arch/arm/mach-rockchip/pm_domains.c
@@ -0,0 +1,355 @@
+/*
+ * 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
+#define PMU_TIMEOUT_MAX		10000
+
+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;
+	/* mutex lock for dev_list */
+	struct mutex  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;
+	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 > PMU_TIMEOUT_MAX) {
+			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 > PMU_TIMEOUT_MAX) {
+			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)
+{
+	if (rockchip_pmu_power_domain_is_on(pd) == on)
+		return 0;
+
+	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 */
+	}
+
+	return 0;
+}
+
+static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
+{
+	int ret = 0;
+	struct rockchip_dev_entry *de;
+
+	mutex_lock(&pd->dev_lock);
+	/* no clk, set power domain will fail */
+	if (list_empty(&pd->dev_list)) {
+		pr_err("%s: no devices in %s power domain\n", __func__,
+			pd->base.name);
+		goto out;
+	}
+
+	list_for_each_entry(de, &pd->dev_list, node) {
+		pm_clk_resume(de->dev);
+	}
+
+	ret = rockchip_pmu_set_power_domain(pd, power_on);
+
+	list_for_each_entry(de, &pd->dev_list, node) {
+		pm_clk_suspend(de->dev);
+	}
+out:
+	mutex_unlock(&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);
+			pm_clk_destroy(dev);
+			return;
+		}
+	}
+
+	de = devm_kzalloc(pd->dev,
+			sizeof(struct rockchip_dev_entry *), GFP_KERNEL);
+	de->dev = dev;
+	mutex_lock(&pd->dev_lock);
+	list_add_tail(&de->node, &pd->dev_list);
+	mutex_unlock(&pd->dev_lock);
+
+	return;
+}
+
+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;
+	mutex_lock(&pd->dev_lock);
+
+	list_for_each_entry(de, &pd->dev_list, node) {
+		if (de->dev == dev) {
+			list_del(&de->node);
+			pm_clk_destroy(dev);
+			devm_kfree(pd->dev, de);
+		}
+	}
+
+	mutex_unlock(&pd->dev_lock);
+}
+
+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);
+	mutex_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

[PATCH v7 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 v7: None
Changes in v6: None
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 v7 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 v7: None
Changes in v6: None
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

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

[Dmitry Torokhov]

Hi Jinkun,

On Fri, Oct 24, 2014 at 12:29:19AM -0700, jinkun.hong wrote:
> +static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
> +{
> +	int ret = 0;
> +	struct rockchip_dev_entry *de;
> +
> +	mutex_lock(&pd->dev_lock);
> +	/* no clk, set power domain will fail */
> +	if (list_empty(&pd->dev_list)) {
> +		pr_err("%s: no devices in %s power domain\n", __func__,
> +			pd->base.name);
> +		goto out;
> +	}
> +
> +	list_for_each_entry(de, &pd->dev_list, node) {
> +		pm_clk_resume(de->dev);
> +	}
> +
> +	ret = rockchip_pmu_set_power_domain(pd, power_on);
> +
> +	list_for_each_entry(de, &pd->dev_list, node) {
> +		pm_clk_suspend(de->dev);
> +	}
> +out:
> +	mutex_unlock(&pd->dev_lock);
> +	return ret;
> +}

For my education, could you please tell me why we need to enable all
clocks of all devices in a power domain when we are talking to the PMU
and then turn them immediately off? Would not we want to enable clocks
in start()) callbacks, like Ulf mentioned?

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Fri, Oct 24, 2014 at 09:44:31AM -0700, Dmitry Torokhov wrote:
> Hi Jinkun,
> 
> On Fri, Oct 24, 2014 at 12:29:19AM -0700, jinkun.hong wrote:
> > +static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
> > +{
> > +	int ret = 0;
> > +	struct rockchip_dev_entry *de;
> > +
> > +	mutex_lock(&pd->dev_lock);
> > +	/* no clk, set power domain will fail */
> > +	if (list_empty(&pd->dev_list)) {
> > +		pr_err("%s: no devices in %s power domain\n", __func__,
> > +			pd->base.name);
> > +		goto out;
> > +	}
> > +
> > +	list_for_each_entry(de, &pd->dev_list, node) {
> > +		pm_clk_resume(de->dev);
> > +	}
> > +
> > +	ret = rockchip_pmu_set_power_domain(pd, power_on);
> > +
> > +	list_for_each_entry(de, &pd->dev_list, node) {
> > +		pm_clk_suspend(de->dev);
> > +	}
> > +out:
> > +	mutex_unlock(&pd->dev_lock);
> > +	return ret;
> > +}
> 
> For my education, could you please tell me why we need to enable all
> clocks of all devices in a power domain when we are talking to the PMU
> and then turn them immediately off? Would not we want to enable clocks
> in start()) callbacks, like Ulf mentioned?

Jinkun, any update on this question?

Thanks.

-- 
Dmitry

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

[Hong jinkun]

在 2014/10/28 7:39, Dmitry Torokhov 写道:
> On Fri, Oct 24, 2014 at 09:44:31AM -0700, Dmitry Torokhov wrote:
>> Hi Jinkun,
>>
>> On Fri, Oct 24, 2014 at 12:29:19AM -0700, jinkun.hong wrote:
>>> +static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
>>> +{
>>> +	int ret = 0;
>>> +	struct rockchip_dev_entry *de;
>>> +
>>> +	mutex_lock(&pd->dev_lock);
>>> +	/* no clk, set power domain will fail */
>>> +	if (list_empty(&pd->dev_list)) {
>>> +		pr_err("%s: no devices in %s power domain\n", __func__,
>>> +			pd->base.name);
>>> +		goto out;
>>> +	}
>>> +
>>> +	list_for_each_entry(de, &pd->dev_list, node) {
>>> +		pm_clk_resume(de->dev);
>>> +	}
>>> +
>>> +	ret = rockchip_pmu_set_power_domain(pd, power_on);
>>> +
>>> +	list_for_each_entry(de, &pd->dev_list, node) {
>>> +		pm_clk_suspend(de->dev);
>>> +	}
>>> +out:
>>> +	mutex_unlock(&pd->dev_lock);
>>> +	return ret;
>>> +}
>> For my education, could you please tell me why we need to enable all
>> clocks of all devices in a power domain when we are talking to the PMU
>> and then turn them immediately off?
Sorry for late reply.RK3288 reset circuit are first asynchronous reset 
and then sync revoked.
All reset revocation actions are needed to synchronize the clocks.
If there is no clock open, power_up reset will cause the corresponding 
reset can not be undone, there will be risks.
>> Would not we want to enable clocks
>> in start()) callbacks, like Ulf mentioned?
Can you tell me which one patch it. Thank you!
> Jinkun, any update on this question?
>
> Thanks.
>

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

[Eddie Cai(蔡枫)]

[-- Attachment #1: Type: text/plain, Size: 2783 bytes --]

Hi Jinkun

Please refer to https://lkml.org/lkml/2014/10/22/95

2014-10-29 9:00 GMT+08:00 Hong jinkun <jinkun.hong@rock-chips.com>:

>
> 在 2014/10/28 7:39, Dmitry Torokhov 写道:
>
>  On Fri, Oct 24, 2014 at 09:44:31AM -0700, Dmitry Torokhov wrote:
>>
>>> Hi Jinkun,
>>>
>>> On Fri, Oct 24, 2014 at 12:29:19AM -0700, jinkun.hong wrote:
>>>
>>>> +static int rockchip_pd_power(struct rockchip_domain *pd, bool power_on)
>>>> +{
>>>> +       int ret = 0;
>>>> +       struct rockchip_dev_entry *de;
>>>> +
>>>> +       mutex_lock(&pd->dev_lock);
>>>> +       /* no clk, set power domain will fail */
>>>> +       if (list_empty(&pd->dev_list)) {
>>>> +               pr_err("%s: no devices in %s power domain\n", __func__,
>>>> +                       pd->base.name);
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       list_for_each_entry(de, &pd->dev_list, node) {
>>>> +               pm_clk_resume(de->dev);
>>>> +       }
>>>> +
>>>> +       ret = rockchip_pmu_set_power_domain(pd, power_on);
>>>> +
>>>> +       list_for_each_entry(de, &pd->dev_list, node) {
>>>> +               pm_clk_suspend(de->dev);
>>>> +       }
>>>> +out:
>>>> +       mutex_unlock(&pd->dev_lock);
>>>> +       return ret;
>>>> +}
>>>>
>>> For my education, could you please tell me why we need to enable all
>>> clocks of all devices in a power domain when we are talking to the PMU
>>> and then turn them immediately off?
>>>
>> Sorry for late reply.RK3288 reset circuit are first asynchronous reset
> and then sync revoked.
> All reset revocation actions are needed to synchronize the clocks.
> If there is no clock open, power_up reset will cause the corresponding
> reset can not be undone, there will be risks.
>
>> Would not we want to enable clocks
>>> in start()) callbacks, like Ulf mentioned?
>>>
>> Can you tell me which one patch it. Thank you!
>
>> Jinkun, any update on this question?
>>
>> Thanks.
>>
>>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>



-- 

Eddie.Cai
蔡枫
Fuzhou Rockchip Electronics Co.Ltd
福州瑞芯微电子有限公司
No. 18 Building, A District, No.89,software Boulevard Fuzhou,Fujian,PR
China(350003)
福建省福州市铜盘路软件大道 89 号软件园 A 区 18 号楼 (350003)
cell phone:+86 13459443244
phone of Fuzhou ： 0591 － 83991906/07 -8108
21th Floor, Malata Building, Ke ji zhong yi Avenue, Nanshan District,
Shenzheng, Guangdong, PR China (518000)
深圳办公室地址：深圳市南山区科技中一路万利达大厦21层  (518000)
phone of Shenzhen ： 0755 － 86690899
mail: cf@rock-chips.com

[-- Attachment #2: Type: text/html, Size: 4709 bytes --]

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

[Doug Anderson]

Jinkun,

On Fri, Oct 24, 2014 at 12:29 AM, jinkun.hong
<jinkun.hong@rock-chips.com> wrote:
> 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 v7:
> - Delete unused variables
>
> Changes in v6:
> - delete pmu_lock
> - modify dev_lock using mutex
> - pm_clk_resume(pd->dev) change to pm_clk_resume(ed->dev)
> - pm_clk_suspend(pd->dev) change to pm_clk_suspend(ed->dev)
> - add devm_kfree(pd->dev, de) in rockchip_pm_domain_detach_dev
>
> 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                |  355 ++++++++++++++++++++
>  5 files changed, 427 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
>  create mode 100644 arch/arm/mach-rockchip/pm_domains.c

I haven't been following all of the changes here, but I'll say that I
just spent a bunch of time figuring out why my system wasn't properly
going into suspend using your patchset after I picked up Kever's patch
to disable unused clocks
(https://patchwork.kernel.org/patch/5202291/).

It turns out that if I go back to patch v2 of your series that suspend
works great.  ...but not with v7.

I got to this point because I started bisecting clocks.  I realized
that I needed to leave on "aclk_vepu", "aclk_vdpu", "aclk_rga_pre",
"sclk_rga", "aclk_hevc", ..., ...  As I kept finding more clocks they
kept looking more and more like your list from the v2 dtsi and it
became obvious.


I guess that things are not properly being turned off properly due to
the reason you stated in <https://lkml.org/lkml/2014/10/28/1279>.
Specifically we need all the relevant clocks on in order to power
things on and off.


I'll let you guys hash out how you want to make this work, but I
figured I'd at least point out what I was seeing.  ;)


NOTE: I'll upload something before EOD California today into our
chromeos-3.14 tree that shows how I am testing on rk3288-pinky.  I
know that's not terribly useful to everyone upstream, but not all
patches for S2R have landed upstream so that's the best I can do.  I'm
currently testing "deep" suspend (with SDRAM in self refresh mode)
which we have no way to resume from ATM.  I'm measuring "success" by
looking at total system power as reported by the battery.  With v2 I
get down to very low power.  With v7 I don't.



-Doug

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

[Dmitry Torokhov]

On Fri, Oct 31, 2014 at 11:03:46AM -0700, Doug Anderson wrote:
> Jinkun,
> 
> On Fri, Oct 24, 2014 at 12:29 AM, jinkun.hong
> <jinkun.hong@rock-chips.com> wrote:
> > 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 v7:
> > - Delete unused variables
> >
> > Changes in v6:
> > - delete pmu_lock
> > - modify dev_lock using mutex
> > - pm_clk_resume(pd->dev) change to pm_clk_resume(ed->dev)
> > - pm_clk_suspend(pd->dev) change to pm_clk_suspend(ed->dev)
> > - add devm_kfree(pd->dev, de) in rockchip_pm_domain_detach_dev
> >
> > 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                |  355 ++++++++++++++++++++
> >  5 files changed, 427 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
> >  create mode 100644 arch/arm/mach-rockchip/pm_domains.c
> 
> I haven't been following all of the changes here, but I'll say that I
> just spent a bunch of time figuring out why my system wasn't properly
> going into suspend using your patchset after I picked up Kever's patch
> to disable unused clocks
> (https://patchwork.kernel.org/patch/5202291/).
> 
> It turns out that if I go back to patch v2 of your series that suspend
> works great.  ...but not with v7.
> 
> I got to this point because I started bisecting clocks.  I realized
> that I needed to leave on "aclk_vepu", "aclk_vdpu", "aclk_rga_pre",
> "sclk_rga", "aclk_hevc", ..., ...  As I kept finding more clocks they
> kept looking more and more like your list from the v2 dtsi and it
> became obvious.
> 
> 
> I guess that things are not properly being turned off properly due to
> the reason you stated in <https://lkml.org/lkml/2014/10/28/1279>.
> Specifically we need all the relevant clocks on in order to power
> things on and off.

I guess since the platform requirement is to have all clocks on during
power domain power transitions the easiest way is indeed to list all
relevant clocks in power domain description instead of trying to fetch
them from devices that compose power domain. I believe it is more
correct because:

1. Placing a device into power domain is done at probe time so
inherently not all clocks are enumerated when first device is being
probed and we trun the power to the domain.

2. We may not have drivers for all devices enabled on a given product
and so not all device will not be bound to a driver which cause them be
detached from their power domain.

Thanks.

-- 
Dmitry

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

[Kever Yang]

[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]

Hi Dmitry,

On 11/01/2014 02:32 AM, Dmitry Torokhov wrote:
>> >I haven't been following all of the changes here, but I'll say that I
>> >just spent a bunch of time figuring out why my system wasn't properly
>> >going into suspend using your patchset after I picked up Kever's patch
>> >to disable unused clocks
>> >(https://patchwork.kernel.org/patch/5202291/).
>> >
>> >It turns out that if I go back to patch v2 of your series that suspend
>> >works great.  ...but not with v7.
>> >
>> >I got to this point because I started bisecting clocks.  I realized
>> >that I needed to leave on "aclk_vepu", "aclk_vdpu", "aclk_rga_pre",
>> >"sclk_rga", "aclk_hevc", ..., ...  As I kept finding more clocks they
>> >kept looking more and more like your list from the v2 dtsi and it
>> >became obvious.
>> >
>> >
>> >I guess that things are not properly being turned off properly due to
>> >the reason you stated in<https://lkml.org/lkml/2014/10/28/1279>.
>> >Specifically we need all the relevant clocks on in order to power
>> >things on and off.
> I guess since the platform requirement is to have all clocks on during
> power domain power transitions
That's true, I'm not sure if Jingkun have make himself clear, but
it is recommend to have all clocks on during power on/off a
power domain.

According to rk3288 trm 4.6.1, obviously we need clock for  power
domain switch, although I'm not sure we need _all _clock on,
but it is safe to have all clock on. And we won't get risk with power
lost of clock management when we turn on all the clocks for
power domain switch, because we always operate the clocks
in pairs and disable the clock after domain power off.
>   the easiest way is indeed to list all
> relevant clocks in power domain description instead of trying to fetch
> them from devices that compose power domain. I believe it is more
> correct because:
>
> 1. Placing a device into power domain is done at probe time so
> inherently not all clocks are enumerated when first device is being
> probed and we trun the power to the domain.
>
> 2. We may not have drivers for all devices enabled on a given product
> and so not all device will not be bound to a driver which cause them be
> detached from their power domain.
Do you mean we should get the clock management for powerdomain
in V2 back?

Hi Greet, Ulf, Kevin,
      do you agree?

- Kever


[-- Attachment #2: Type: text/html, Size: 4070 bytes --]