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

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

[Caesar Wang]

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

    Verified on url =
    https://chromium.googlesource.com/chromiumos/third_party/kernel.

    At the moment,there are mass of products are using the driver.
I believe the driver can happy work for next kernel.



Caesar Wang (3):
  dt-bindings: add document of Rockchip power domain
  power-domain: rockchip: add power domain driver
  ARM: dts: add RK3288 power-domain node

 .../bindings/arm/rockchip/power_domain.txt         |  48 ++
 arch/arm/boot/dts/rk3288.dtsi                      |  59 +++
 arch/arm/mach-rockchip/Kconfig                     |   1 +
 arch/arm/mach-rockchip/Makefile                    |   1 +
 arch/arm/mach-rockchip/pm_domains.c                | 506 +++++++++++++++++++++
 include/dt-bindings/power-domain/rk3288.h          |  11 +
 6 files changed, 626 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
 create mode 100644 arch/arm/mach-rockchip/pm_domains.c
 create mode 100644 include/dt-bindings/power-domain/rk3288.h

-- 
1.9.1

[PATCH v14 1/3] dt-bindings: add document of Rockchip power domain

[Caesar Wang]

This add the necessary binding documentation for the power domain
found on Rockchip Socs.

Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 .../bindings/arm/rockchip/power_domain.txt         | 48 ++++++++++++++++++++++
 1 file changed, 48 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..3e74e6d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
@@ -0,0 +1,48 @@
+* 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-controller - for rk3288 type power domain.
+- #power-domain-cells: Number of cells in a power-domain specifier.
+		       should be 1.
+- rockchip,pmu: phandle referencing a syscon providing the pmu registers
+- #address-cells: should be 1.
+- #size-cells: should be 0.
+
+Required properties for power domain sub nodes:
+- reg: index of the power domain, should use macros in:
+    *  include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.
+- clocks (optional): phandles to clocks which need to be enabled while power domain
+          switches state.
+
+Example:
+
+	power: power-controller {
+	       compatible = "rockchip,rk3288-power-controller";
+	       #power-domain-cells = <1>;
+	       rockchip,pmu = <&pmu>;
+               #address-cells = <1>;
+               #size-cells = <0>;
+
+	       pd_gpu {
+	               reg = <RK3288_PD_GPU>;
+	               clocks = <&cru ACLK_GPU>;
+	       };
+	};
+
+Node of a device using power domains must have a power-domains property,
+containing a phandle to the power device node and an index specifying which
+power domain to use.
+The index should use macros in:
+   * include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.
+
+Example of the node using power domain:
+
+	node {
+		/* ... */
+		power-domains = <&power RK3288_PD_GPU>;
+		/* ... */
+	};
-- 
1.9.1

[PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Caesar Wang]

In order to meet high performance and low power requirements, a power
management unit is designed or saving power when RK3288 in low power
mode.
The RK3288 PMU is dedicated for managing the power ot the whole chip.

Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 arch/arm/mach-rockchip/Kconfig      |   1 +
 arch/arm/mach-rockchip/Makefile     |   1 +
 arch/arm/mach-rockchip/pm_domains.c | 506 ++++++++++++++++++++++++++++++++++++
 3 files changed, 508 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 ae4eb7c..578206b 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
 	select ROCKCHIP_TIMER
 	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 5c3a9b2..9c902d3 100644
--- a/arch/arm/mach-rockchip/Makefile
+++ b/arch/arm/mach-rockchip/Makefile
@@ -1,5 +1,6 @@
 CFLAGS_platsmp.o := -march=armv7-a
 
 obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
 obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
 obj-$(CONFIG_SMP) += headsmp.o platsmp.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..92d2569
--- /dev/null
+++ b/arch/arm/mach-rockchip/pm_domains.c
@@ -0,0 +1,506 @@
+/*
+ * Rockchip Generic power domain support.
+ *
+ * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
+ *
+ * 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/io.h>
+#include <linux/err.h>
+#include <linux/pm_clock.h>
+#include <linux/pm_domain.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <dt-bindings/power-domain/rk3288.h>
+
+struct rockchip_domain_info {
+	int pwr_mask;
+	int status_mask;
+	int req_mask;
+	int idle_mask;
+	int ack_mask;
+};
+
+struct rockchip_pmu_info {
+	u32 pwr_offset;
+	u32 status_offset;
+	u32 req_offset;
+	u32 idle_offset;
+	u32 ack_offset;
+
+	u32 core_pwrcnt_offset;
+	u32 gpu_pwrcnt_offset;
+
+	unsigned int core_power_transition_time;
+	unsigned int gpu_power_transition_time;
+
+	int num_domains;
+	const struct rockchip_domain_info *domain_info;
+};
+
+struct rockchip_pm_domain {
+	struct generic_pm_domain genpd;
+	const struct rockchip_domain_info *info;
+	struct rockchip_pmu *pmu;
+	int num_clks;
+	struct clk *clks[];
+};
+
+struct rockchip_pmu {
+	struct device *dev;
+	struct regmap *regmap;
+	const struct rockchip_pmu_info *info;
+	struct mutex mutex; /* mutex lock for pmu */
+	struct genpd_onecell_data genpd_data;
+	struct generic_pm_domain *domains[];
+};
+
+#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
+
+#define DOMAIN(pwr, status, req, idle, ack)	\
+{						\
+	.pwr_mask = BIT(pwr),			\
+	.status_mask = BIT(status),		\
+	.req_mask = BIT(req),			\
+	.idle_mask = BIT(idle),			\
+	.ack_mask = BIT(ack),			\
+}
+
+#define DOMAIN_RK3288(pwr, status, req)		\
+	DOMAIN(pwr, status, req, req, (req) + 16)
+
+static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
+{
+	struct rockchip_pmu *pmu = pd->pmu;
+	const struct rockchip_domain_info *pd_info = pd->info;
+	unsigned int val;
+
+	regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
+	return (val & pd_info->idle_mask) == pd_info->idle_mask;
+}
+
+static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
+					 bool idle)
+{
+	const struct rockchip_domain_info *pd_info = pd->info;
+	struct rockchip_pmu *pmu = pd->pmu;
+	unsigned int val;
+
+	regmap_update_bits(pmu->regmap, pmu->info->req_offset,
+			   pd_info->req_mask, idle ? -1U : 0);
+
+	dsb();
+
+	do {
+		regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
+	} while ((val & pd_info->ack_mask) != (idle ? pd_info->ack_mask : 0));
+
+	while (rockchip_pmu_domain_is_idle(pd) != idle)
+		cpu_relax();
+
+	return 0;
+}
+
+static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
+{
+	struct rockchip_pmu *pmu = pd->pmu;
+	unsigned int val;
+
+	regmap_read(pmu->regmap, pmu->info->status_offset, &val);
+
+	/* 1'b0: power on, 1'b1: power off */
+	return !(val & pd->info->status_mask);
+}
+
+static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
+					     bool on)
+{
+	struct rockchip_pmu *pmu = pd->pmu;
+
+	regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
+			   pd->info->pwr_mask, on ? 0 : -1U);
+
+	dsb();
+
+	while (rockchip_pmu_domain_is_on(pd) != on)
+		cpu_relax();
+}
+
+static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
+{
+	int i;
+
+	mutex_lock(&pd->pmu->mutex);
+
+	if (rockchip_pmu_domain_is_on(pd) != power_on) {
+		for (i = 0; i < pd->num_clks; i++)
+			clk_enable(pd->clks[i]);
+
+		if (!power_on) {
+			/* FIXME: add code to save AXI_QOS */
+
+			/* if powering down, idle request to NIU first */
+			rockchip_pmu_set_idle_request(pd, true);
+		}
+
+		rockchip_do_pmu_set_power_domain(pd, power_on);
+
+		if (power_on) {
+			/* if powering up, leave idle mode */
+			rockchip_pmu_set_idle_request(pd, false);
+
+			/* FIXME: add code to restore AXI_QOS */
+		}
+
+		for (i = pd->num_clks - 1; i >= 0; i--)
+			clk_disable(pd->clks[i]);
+	}
+
+	mutex_unlock(&pd->pmu->mutex);
+	return 0;
+}
+
+static int rockchip_pd_power_on(struct generic_pm_domain *domain)
+{
+	struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
+
+	return rockchip_pd_power(pd, false);
+}
+
+static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
+				  struct device *dev)
+{
+	struct clk *clk;
+	int i;
+	int error;
+
+	dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
+
+	error = pm_clk_create(dev);
+	if (error) {
+		dev_err(dev, "pm_clk_create failed %d\n", error);
+		return error;
+	}
+
+	i = 0;
+	while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
+		dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
+			__clk_get_name(clk));
+		error = pm_clk_add_clk(dev, clk);
+		clk_put(clk);
+		if (error) {
+			dev_err(dev, "pm_clk_add_clk failed %d\n", error);
+			pm_clk_destroy(dev);
+			return error;
+		}
+	}
+
+	return 0;
+}
+
+static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
+				   struct device *dev)
+{
+	dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name);
+
+	pm_clk_destroy(dev);
+}
+
+static int rockchip_pd_start_dev(struct device *dev)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+
+	dev_dbg(dev, "starting device in power domain '%s'\n", genpd->name);
+
+	return pm_clk_resume(dev);
+}
+
+static int rockchip_pd_stop_dev(struct device *dev)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+
+	dev_dbg(dev, "stopping device in power domain '%s'\n", genpd->name);
+
+	return pm_clk_suspend(dev);
+}
+
+static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
+				      struct device_node *node)
+{
+	const struct rockchip_domain_info *pd_info;
+	struct rockchip_pm_domain *pd;
+	struct clk *clk;
+	int clk_cnt;
+	int i;
+	u32 id;
+	int error;
+
+	error = of_property_read_u32(node, "reg", &id);
+	if (error) {
+		dev_err(pmu->dev,
+			"%s: failed to retrieve domain id (reg): %d\n",
+			node->name, error);
+		return -EINVAL;
+	}
+
+	if (id >= pmu->info->num_domains) {
+		dev_err(pmu->dev, "%s: invalid domain id %d\n",
+			node->name, id);
+		return -EINVAL;
+	}
+
+	pd_info = &pmu->info->domain_info[id];
+	if (!pd_info) {
+		dev_err(pmu->dev, "%s: undefined domain id %d\n",
+			node->name, id);
+		return -EINVAL;
+	}
+
+	clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells");
+	pd = devm_kzalloc(pmu->dev,
+			  sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
+			  GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	pd->info = pd_info;
+	pd->pmu = pmu;
+
+	for (i = 0; i < clk_cnt; i++) {
+		clk = of_clk_get(node, i);
+		if (IS_ERR(clk)) {
+			error = PTR_ERR(clk);
+			dev_err(pmu->dev,
+				"%s: failed to get clk %s (index %d): %d\n",
+				node->name, __clk_get_name(clk), i, error);
+			goto err_out;
+		}
+
+		error = clk_prepare(clk);
+		if (error) {
+			dev_err(pmu->dev,
+				"%s: failed to prepare clk %s (index %d): %d\n",
+				node->name, __clk_get_name(clk), i, error);
+			clk_put(clk);
+			goto err_out;
+		}
+
+		pd->clks[pd->num_clks++] = clk;
+
+		dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
+			__clk_get_name(clk), node->name);
+	}
+
+	error = rockchip_pd_power(pd, true);
+	if (error) {
+		dev_err(pmu->dev,
+			"failed to power on domain '%s': %d\n",
+			node->name, error);
+		goto err_out;
+	}
+
+	pd->genpd.name = node->name;
+	pd->genpd.power_off = rockchip_pd_power_off;
+	pd->genpd.power_on = rockchip_pd_power_on;
+	pd->genpd.attach_dev = rockchip_pd_attach_dev;
+	pd->genpd.detach_dev = rockchip_pd_detach_dev;
+	pd->genpd.dev_ops.start = rockchip_pd_start_dev;
+	pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
+	pm_genpd_init(&pd->genpd, NULL, false);
+
+	pmu->genpd_data.domains[id] = &pd->genpd;
+	return 0;
+
+err_out:
+	while (--i >= 0) {
+		clk_unprepare(pd->clks[i]);
+		clk_put(pd->clks[i]);
+	}
+	return error;
+}
+
+static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
+{
+	int i;
+
+	for (i = 0; i < pd->num_clks; i++) {
+		clk_unprepare(pd->clks[i]);
+		clk_put(pd->clks[i]);
+	}
+
+	/* devm will free our memory */
+}
+
+static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
+{
+	struct generic_pm_domain *genpd;
+	struct rockchip_pm_domain *pd;
+	int i;
+
+	for (i = 0; i < pmu->genpd_data.num_domains; i++) {
+		genpd = pmu->genpd_data.domains[i];
+		if (genpd) {
+			pd = to_rockchip_pd(genpd);
+			rockchip_pm_remove_one_domain(pd);
+		}
+	}
+
+	/* devm will free our memory */
+}
+
+static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
+				      u32 domain_reg_offset,
+				      unsigned int count)
+{
+	/* First configure domain power down transition count ... */
+	regmap_write(pmu->regmap, domain_reg_offset, count);
+	/* ... and then power up count. */
+	regmap_write(pmu->regmap, domain_reg_offset + 4, count);
+}
+
+static int rockchip_pm_domain_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *node;
+	struct rockchip_pmu *pmu;
+	const struct of_device_id *match;
+	const struct rockchip_pmu_info *pmu_info;
+	int error;
+
+	if (!np) {
+		dev_err(dev, "device tree node not found\n");
+		return -ENXIO;
+	}
+
+	match = of_match_device(dev->driver->of_match_table, dev);
+	if (!match || !match->data) {
+		dev_err(dev, "missing pmu data\n");
+		return -EINVAL;
+	}
+
+	pmu_info = match->data;
+
+	pmu = devm_kzalloc(dev,
+			   sizeof(*pmu) +
+				pmu_info->num_domains * sizeof(pmu->domains[0]),
+			   GFP_KERNEL);
+	if (!pmu)
+		return -ENOMEM;
+
+	pmu->dev = &pdev->dev;
+	mutex_init(&pmu->mutex);
+
+	pmu->info = pmu_info;
+
+	pmu->genpd_data.domains = pmu->domains;
+	pmu->genpd_data.num_domains = pmu_info->num_domains;
+
+	node = of_parse_phandle(np, "rockchip,pmu", 0);
+	pmu->regmap = syscon_node_to_regmap(node);
+	of_node_put(node);
+	if (IS_ERR(pmu->regmap)) {
+		error = PTR_ERR(pmu->regmap);
+		dev_err(dev, "failed to get PMU regmap: %d\n", error);
+		return error;
+	}
+
+	/*
+	 * Configure power up and down transition delays for core
+	 * and GPU domains.
+	 */
+	rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
+				  pmu_info->core_power_transition_time);
+	rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
+				  pmu_info->gpu_power_transition_time);
+
+	error = -ENXIO;
+
+	for_each_available_child_of_node(np, node) {
+		error = rockchip_pm_add_one_domain(pmu, node);
+		if (error) {
+			dev_err(dev, "failed to handle node %s: %d\n",
+				node->name, error);
+			goto err_out;
+		}
+	}
+
+	if (error) {
+		dev_dbg(dev, "no power domains defined\n");
+		goto err_out;
+	}
+
+	of_genpd_add_provider_onecell(np, &pmu->genpd_data);
+
+	return 0;
+
+err_out:
+	rockchip_pm_domain_cleanup(pmu);
+	return error;
+}
+
+static const struct rockchip_domain_info rk3288_pm_domains[] = {
+	[RK3288_PD_GPU]		= DOMAIN_RK3288(9, 9, 2),
+	[RK3288_PD_VIO]		= DOMAIN_RK3288(7, 7, 4),
+	[RK3288_PD_VIDEO]	= DOMAIN_RK3288(8, 8, 3),
+	[RK3288_PD_HEVC]	= DOMAIN_RK3288(14, 10, 9),
+};
+
+static const struct rockchip_pmu_info rk3288_pmu = {
+	.pwr_offset = 0x08,
+	.status_offset = 0x0c,
+	.req_offset = 0x10,
+	.idle_offset = 0x14,
+	.ack_offset = 0x14,
+
+	.core_pwrcnt_offset = 0x34,
+	.gpu_pwrcnt_offset = 0x3c,
+
+	.core_power_transition_time = 24, /* 1us */
+	.gpu_power_transition_time = 24, /* 1us */
+
+	.num_domains = ARRAY_SIZE(rk3288_pm_domains),
+	.domain_info = rk3288_pm_domains,
+};
+
+static const struct of_device_id rockchip_pm_domain_dt_match[] = {
+	{
+		.compatible = "rockchip,rk3288-power-controller",
+		.data = (void *)&rk3288_pmu,
+	},
+	{ /* sentinel */ },
+};
+
+static struct platform_driver rockchip_pm_domain_driver = {
+	.probe = rockchip_pm_domain_probe,
+	.driver = {
+		.name   = "rockchip-pm-domain",
+		.of_match_table = rockchip_pm_domain_dt_match,
+		/*
+		 * We can't forcibly eject devices form power domain,
+		 * so we can't really remove power domains once they
+		 * were added.
+		 */
+		.suppress_bind_attrs = true,
+	},
+};
+
+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.9.1

[PATCH v14 3/3] ARM: dts: add RK3288 power-domain node

[Caesar Wang]

This patch add the needed clocks into power-controller.

why need we do so that?

Firstly, we always be needed turn off clocks to save power when
the system enter suspend.So we need to enumerate the clocks are needed
to switch power doamin no and off.

Secondly, RK3288 reset circuit should be syncchronous reset and
then sync revoked.so we need to enable clocks of all devices.

Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

Changes in v14:
    - does not need to set an owner,remove the "THIS_MODULE".

Changes in v13:
    - Remove essential clocks from rk3288 PD_VIO domain
    Some clocks are essential for the system health and should not
    be turned down. However there is no owner for them so if they listed
    as belonging to power domain we'll try toggling them up and down
    during power domain.
    - Device drivers expect their devices to be powered on before their
    probing code is invoked. To achieve that we should start with
    power domains powered on (we may turn them off later once all
    devices enable runtime power managment and go idle).
    - This change switches Rockchip power domain driver to use updated
    device_attach and device_detach API.
    - set the gpu/core power domain power delay time.
    - fix enumerating PM clocks for devices.
    - fix use after free
    We can't use clk after we did clk_put(clk).

Changes in v12:
    - Remove essential clocks from rk3288 PD_VIO domain,
    Some clocks are essential for the system health and should
    not be turned down. However there is no owner for them so
    if they listed as belonging to power domain we'll try toggling
    them up and down during power domain transition. As a result we
    either fail to suspend or resume the system.

Changes in v11: None

Changes in v10:
    - fix missing the #include <dt-bindings/power-domain/rk3288.h>
    - remove the notes

Changes in v9:
    - add decription for power-doamin node

Changes in v8:
    - DTS go back to v2

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             | 59 +++++++++++++++++++++++++++++++
 include/dt-bindings/power-domain/rk3288.h | 11 ++++++
 2 files changed, 70 insertions(+)
 create mode 100644 include/dt-bindings/power-domain/rk3288.h

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 5999029..f3992db 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -15,6 +15,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/rockchip.h>
 #include <dt-bindings/clock/rk3288-cru.h>
+#include <dt-bindings/power-domain/rk3288.h>
 #include <dt-bindings/thermal/thermal.h>
 #include "skeleton.dtsi"
 
@@ -1277,4 +1278,62 @@
 			};
 		};
 	};
+
+	power: power-controller {
+		compatible = "rockchip,rk3288-power-controller";
+		#power-domain-cells = <1>;
+		rockchip,pmu = <&pmu>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pd_gpu {
+			reg = <RK3288_PD_GPU>;
+			clocks = <&cru ACLK_GPU>;
+		};
+
+		pd_hevc {
+			reg = <RK3288_PD_HEVC>;
+			clocks = <&cru ACLK_HEVC>,
+				 <&cru SCLK_HEVC_CABAC>,
+				 <&cru SCLK_HEVC_CORE>,
+				 <&cru HCLK_HEVC>;
+		};
+
+		pd_vio {
+			reg = <RK3288_PD_VIO>;
+			clocks = <&cru ACLK_IEP>,
+				 <&cru ACLK_ISP>,
+				 <&cru ACLK_RGA>,
+				 <&cru ACLK_VIP>,
+				 <&cru ACLK_VOP0>,
+				 <&cru ACLK_VOP1>,
+				 <&cru DCLK_VOP0>,
+				 <&cru DCLK_VOP1>,
+				 <&cru HCLK_IEP>,
+				 <&cru HCLK_ISP>,
+				 <&cru HCLK_RGA>,
+				 <&cru HCLK_VIP>,
+				 <&cru HCLK_VOP0>,
+				 <&cru HCLK_VOP1>,
+				 <&cru PCLK_EDP_CTRL>,
+				 <&cru PCLK_HDMI_CTRL>,
+				 <&cru PCLK_LVDS_PHY>,
+				 <&cru PCLK_MIPI_CSI>,
+				 <&cru PCLK_MIPI_DSI0>,
+				 <&cru PCLK_MIPI_DSI1>,
+				 <&cru SCLK_EDP_24M>,
+				 <&cru SCLK_EDP>,
+				 <&cru SCLK_HDMI_CEC>,
+				 <&cru SCLK_HDMI_HDCP>,
+				 <&cru SCLK_ISP_JPE>,
+				 <&cru SCLK_ISP>,
+				 <&cru SCLK_RGA>;
+		};
+
+		pd_video {
+			reg = <RK3288_PD_VIDEO>;
+			clocks = <&cru ACLK_VCODEC>,
+				 <&cru HCLK_VCODEC>;
+		};
+	};
 };
diff --git a/include/dt-bindings/power-domain/rk3288.h b/include/dt-bindings/power-domain/rk3288.h
new file mode 100644
index 0000000..ca68c11
--- /dev/null
+++ b/include/dt-bindings/power-domain/rk3288.h
@@ -0,0 +1,11 @@
+#ifndef __DT_BINDINGS_POWER_DOMAIN_RK3288_H__
+#define __DT_BINDINGS_POWER_DOMAIN_RK3288_H__
+
+/* RK3288 power domain index */
+#define RK3288_PD_GPU          0
+#define RK3288_PD_VIO          1
+#define RK3288_PD_VIDEO        2
+#define RK3288_PD_HEVC         3
+#define RK3288_PD_PERI         4
+
+#endif
-- 
1.9.1

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

[Heiko Stübner]

Hi Caesar,


thanks for keeping this alive :-)

Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang:
>     Add power domain drivers based on generic power domain for
> Rockchip platform, and support RK3288.
> 
>     Verified on url =
>     https://chromium.googlesource.com/chromiumos/third_party/kernel.
> 
>     At the moment,there are mass of products are using the driver.
> I believe the driver can happy work for next kernel.

I've taken a look at the driver and here are some global remarks:

(1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks
bisectability, as the driver itself in patch 2 also includes the header and
would thus fail to compile if the later patch 3 is missing.
Ideally I think the header addition should be a separate patch itself, so that
we can possibly share it between driver and dts branches.
So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes.


(2) The dts-changes in patch 3 should also add any necessary power-domain
assignment on devices if they're still missing, so that we don't introduce
regressions. In my case my work-in-progress edp died because the powerdomain
was turned off automatically it seems.


(3) more like wondering @Kevin or so, is there some more generic place for a
power-domain driver nowadays?


(4) As Tomasz remarked previously the dts should represent the hardware and
the power-domains are part of the pmu. There is a recent addition from Linus
Walleij, called simple-mfd [a] that is supposed to get added real early for
kernel 4.2. So I'd think the power-domains should use that and the patchset
modified to include the changes shown below [b]?


(5) Keven Hilman and Tomasz had reservations about all the device clocks
being listed in the power-domains itself in the previous versions. I don't see
a comment from them yet changing that view.

Their wish was to get the clocks by reading the clocks from the device nodes,
though I see a problem on how to handle devices that do not have any bindings
at all yet.

Kevin, Tomasz any new thoughts?


Heiko


[a] https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=simple-mfd&id=fcf294c020ff7ee4e3b1e96159e4dc7a17ad59d1

[b]
Subject: [PATCH] make power-domains a child of the pmu

This should of course be distributed across the original patches and not
be a separate patch.
---
 arch/arm/boot/dts/rk3288.dtsi       | 118 ++++++++++++++++++------------------
 arch/arm/mach-rockchip/pm_domains.c |  11 +++-
 2 files changed, 67 insertions(+), 62 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 9696f51..b9ba79013 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -549,8 +549,66 @@
 	};
 
 	pmu: power-management@ff730000 {
-		compatible = "rockchip,rk3288-pmu", "syscon";
+		compatible = "rockchip,rk3288-pmu", "syscon", "simple-mfd";
 		reg = <0xff730000 0x100>;
+
+		power: power-controller {
+			compatible = "rockchip,rk3288-power-controller";
+			#power-domain-cells = <1>;
+			rockchip,pmu = <&pmu>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			pd_gpu {
+				reg = <RK3288_PD_GPU>;
+				clocks = <&cru ACLK_GPU>;
+			};
+
+			pd_hevc {
+				reg = <RK3288_PD_HEVC>;
+				clocks = <&cru ACLK_HEVC>,
+					<&cru SCLK_HEVC_CABAC>,
+					<&cru SCLK_HEVC_CORE>,
+					<&cru HCLK_HEVC>;
+			};
+
+			pd_vio {
+				reg = <RK3288_PD_VIO>;
+				clocks = <&cru ACLK_IEP>,
+					<&cru ACLK_ISP>,
+					<&cru ACLK_RGA>,
+					<&cru ACLK_VIP>,
+					<&cru ACLK_VOP0>,
+					<&cru ACLK_VOP1>,
+					<&cru DCLK_VOP0>,
+					<&cru DCLK_VOP1>,
+					<&cru HCLK_IEP>,
+					<&cru HCLK_ISP>,
+					<&cru HCLK_RGA>,
+					<&cru HCLK_VIP>,
+					<&cru HCLK_VOP0>,
+					<&cru HCLK_VOP1>,
+					<&cru PCLK_EDP_CTRL>,
+					<&cru PCLK_HDMI_CTRL>,
+					<&cru PCLK_LVDS_PHY>,
+					<&cru PCLK_MIPI_CSI>,
+					<&cru PCLK_MIPI_DSI0>,
+					<&cru PCLK_MIPI_DSI1>,
+					<&cru SCLK_EDP_24M>,
+					<&cru SCLK_EDP>,
+					<&cru SCLK_HDMI_CEC>,
+					<&cru SCLK_HDMI_HDCP>,
+					<&cru SCLK_ISP_JPE>,
+					<&cru SCLK_ISP>,
+					<&cru SCLK_RGA>;
+			};
+
+			pd_video {
+				reg = <RK3288_PD_VIDEO>;
+				clocks = <&cru ACLK_VCODEC>,
+					<&cru HCLK_VCODEC>;
+			};
+		};
 	};
 
 	sgrf: syscon@ff740000 {
@@ -1316,62 +1374,4 @@
 			};
 		};
 	};
-
-	power: power-controller {
-		compatible = "rockchip,rk3288-power-controller";
-		#power-domain-cells = <1>;
-		rockchip,pmu = <&pmu>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		pd_gpu {
-			reg = <RK3288_PD_GPU>;
-			clocks = <&cru ACLK_GPU>;
-		};
-
-		pd_hevc {
-			reg = <RK3288_PD_HEVC>;
-			clocks = <&cru ACLK_HEVC>,
-				 <&cru SCLK_HEVC_CABAC>,
-				 <&cru SCLK_HEVC_CORE>,
-				 <&cru HCLK_HEVC>;
-		};
-
-		pd_vio {
-			reg = <RK3288_PD_VIO>;
-			clocks = <&cru ACLK_IEP>,
-				 <&cru ACLK_ISP>,
-				 <&cru ACLK_RGA>,
-				 <&cru ACLK_VIP>,
-				 <&cru ACLK_VOP0>,
-				 <&cru ACLK_VOP1>,
-				 <&cru DCLK_VOP0>,
-				 <&cru DCLK_VOP1>,
-				 <&cru HCLK_IEP>,
-				 <&cru HCLK_ISP>,
-				 <&cru HCLK_RGA>,
-				 <&cru HCLK_VIP>,
-				 <&cru HCLK_VOP0>,
-				 <&cru HCLK_VOP1>,
-				 <&cru PCLK_EDP_CTRL>,
-				 <&cru PCLK_HDMI_CTRL>,
-				 <&cru PCLK_LVDS_PHY>,
-				 <&cru PCLK_MIPI_CSI>,
-				 <&cru PCLK_MIPI_DSI0>,
-				 <&cru PCLK_MIPI_DSI1>,
-				 <&cru SCLK_EDP_24M>,
-				 <&cru SCLK_EDP>,
-				 <&cru SCLK_HDMI_CEC>,
-				 <&cru SCLK_HDMI_HDCP>,
-				 <&cru SCLK_ISP_JPE>,
-				 <&cru SCLK_ISP>,
-				 <&cru SCLK_RGA>;
-		};
-
-		pd_video {
-			reg = <RK3288_PD_VIDEO>;
-			clocks = <&cru ACLK_VCODEC>,
-				 <&cru HCLK_VCODEC>;
-		};
-	};
 };
diff --git a/arch/arm/mach-rockchip/pm_domains.c b/arch/arm/mach-rockchip/pm_domains.c
index 92d2569..f3d87c21 100644
--- a/arch/arm/mach-rockchip/pm_domains.c
+++ b/arch/arm/mach-rockchip/pm_domains.c
@@ -377,6 +377,7 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct device_node *node;
+	struct device *parent;
 	struct rockchip_pmu *pmu;
 	const struct of_device_id *match;
 	const struct rockchip_pmu_info *pmu_info;
@@ -410,9 +411,13 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	pmu->genpd_data.domains = pmu->domains;
 	pmu->genpd_data.num_domains = pmu_info->num_domains;
 
-	node = of_parse_phandle(np, "rockchip,pmu", 0);
-	pmu->regmap = syscon_node_to_regmap(node);
-	of_node_put(node);
+	parent = dev->parent;
+	if (!parent) {
+		dev_err(dev, "no parent for syscon LED\n");
+		return -ENODEV;
+	}
+
+	pmu->regmap = syscon_node_to_regmap(parent->of_node);
 	if (IS_ERR(pmu->regmap)) {
 		error = PTR_ERR(pmu->regmap);
 		dev_err(dev, "failed to get PMU regmap: %d\n", error);
-- 
2.1.4

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

[Heiko Stübner]

Hi Caesar,


thanks for keeping this alive :-)

Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang:
>     Add power domain drivers based on generic power domain for
> Rockchip platform, and support RK3288.
> 
>     Verified on url =
>     https://chromium.googlesource.com/chromiumos/third_party/kernel.
> 
>     At the moment,there are mass of products are using the driver.
> I believe the driver can happy work for next kernel.

I've taken a look at the driver and here are some global remarks:

(1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks
bisectability, as the driver itself in patch 2 also includes the header and
would thus fail to compile if the later patch 3 is missing.
Ideally I think the header addition should be a separate patch itself, so that
we can possibly share it between driver and dts branches.
So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes.


(2) The dts-changes in patch 3 should also add any necessary power-domain
assignment on devices if they're still missing, so that we don't introduce
regressions. In my case my work-in-progress edp died because the powerdomain
was turned off automatically it seems.


(3) more like wondering @Kevin or so, is there some more generic place for a
power-domain driver nowadays?


(4) As Tomasz remarked previously the dts should represent the hardware and
the power-domains are part of the pmu. There is a recent addition from Linus
Walleij, called simple-mfd [a] that is supposed to get added real early for
kernel 4.2. So I'd think the power-domains should use that and the patchset
modified to include the changes shown below [b]?


(5) Keven Hilman and Tomasz had reservations about all the device clocks
being listed in the power-domains itself in the previous versions. I don't see
a comment from them yet changing that view.

Their wish was to get the clocks by reading the clocks from the device nodes,
though I see a problem on how to handle devices that do not have any bindings
at all yet.

Kevin, Tomasz any new thoughts?


Heiko


[a] https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-integrator.git/commit/?h=simple-mfd&id=fcf294c020ff7ee4e3b1e96159e4dc7a17ad59d1

[b]
Subject: [PATCH] make power-domains a child of the pmu

This should of course be distributed across the original patches and not
be a separate patch.
---
 arch/arm/boot/dts/rk3288.dtsi       | 118 ++++++++++++++++++------------------
 arch/arm/mach-rockchip/pm_domains.c |  11 +++-
 2 files changed, 67 insertions(+), 62 deletions(-)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 9696f51..b9ba79013 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -549,8 +549,66 @@
 	};
 
 	pmu: power-management at ff730000 {
-		compatible = "rockchip,rk3288-pmu", "syscon";
+		compatible = "rockchip,rk3288-pmu", "syscon", "simple-mfd";
 		reg = <0xff730000 0x100>;
+
+		power: power-controller {
+			compatible = "rockchip,rk3288-power-controller";
+			#power-domain-cells = <1>;
+			rockchip,pmu = <&pmu>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			pd_gpu {
+				reg = <RK3288_PD_GPU>;
+				clocks = <&cru ACLK_GPU>;
+			};
+
+			pd_hevc {
+				reg = <RK3288_PD_HEVC>;
+				clocks = <&cru ACLK_HEVC>,
+					<&cru SCLK_HEVC_CABAC>,
+					<&cru SCLK_HEVC_CORE>,
+					<&cru HCLK_HEVC>;
+			};
+
+			pd_vio {
+				reg = <RK3288_PD_VIO>;
+				clocks = <&cru ACLK_IEP>,
+					<&cru ACLK_ISP>,
+					<&cru ACLK_RGA>,
+					<&cru ACLK_VIP>,
+					<&cru ACLK_VOP0>,
+					<&cru ACLK_VOP1>,
+					<&cru DCLK_VOP0>,
+					<&cru DCLK_VOP1>,
+					<&cru HCLK_IEP>,
+					<&cru HCLK_ISP>,
+					<&cru HCLK_RGA>,
+					<&cru HCLK_VIP>,
+					<&cru HCLK_VOP0>,
+					<&cru HCLK_VOP1>,
+					<&cru PCLK_EDP_CTRL>,
+					<&cru PCLK_HDMI_CTRL>,
+					<&cru PCLK_LVDS_PHY>,
+					<&cru PCLK_MIPI_CSI>,
+					<&cru PCLK_MIPI_DSI0>,
+					<&cru PCLK_MIPI_DSI1>,
+					<&cru SCLK_EDP_24M>,
+					<&cru SCLK_EDP>,
+					<&cru SCLK_HDMI_CEC>,
+					<&cru SCLK_HDMI_HDCP>,
+					<&cru SCLK_ISP_JPE>,
+					<&cru SCLK_ISP>,
+					<&cru SCLK_RGA>;
+			};
+
+			pd_video {
+				reg = <RK3288_PD_VIDEO>;
+				clocks = <&cru ACLK_VCODEC>,
+					<&cru HCLK_VCODEC>;
+			};
+		};
 	};
 
 	sgrf: syscon at ff740000 {
@@ -1316,62 +1374,4 @@
 			};
 		};
 	};
-
-	power: power-controller {
-		compatible = "rockchip,rk3288-power-controller";
-		#power-domain-cells = <1>;
-		rockchip,pmu = <&pmu>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		pd_gpu {
-			reg = <RK3288_PD_GPU>;
-			clocks = <&cru ACLK_GPU>;
-		};
-
-		pd_hevc {
-			reg = <RK3288_PD_HEVC>;
-			clocks = <&cru ACLK_HEVC>,
-				 <&cru SCLK_HEVC_CABAC>,
-				 <&cru SCLK_HEVC_CORE>,
-				 <&cru HCLK_HEVC>;
-		};
-
-		pd_vio {
-			reg = <RK3288_PD_VIO>;
-			clocks = <&cru ACLK_IEP>,
-				 <&cru ACLK_ISP>,
-				 <&cru ACLK_RGA>,
-				 <&cru ACLK_VIP>,
-				 <&cru ACLK_VOP0>,
-				 <&cru ACLK_VOP1>,
-				 <&cru DCLK_VOP0>,
-				 <&cru DCLK_VOP1>,
-				 <&cru HCLK_IEP>,
-				 <&cru HCLK_ISP>,
-				 <&cru HCLK_RGA>,
-				 <&cru HCLK_VIP>,
-				 <&cru HCLK_VOP0>,
-				 <&cru HCLK_VOP1>,
-				 <&cru PCLK_EDP_CTRL>,
-				 <&cru PCLK_HDMI_CTRL>,
-				 <&cru PCLK_LVDS_PHY>,
-				 <&cru PCLK_MIPI_CSI>,
-				 <&cru PCLK_MIPI_DSI0>,
-				 <&cru PCLK_MIPI_DSI1>,
-				 <&cru SCLK_EDP_24M>,
-				 <&cru SCLK_EDP>,
-				 <&cru SCLK_HDMI_CEC>,
-				 <&cru SCLK_HDMI_HDCP>,
-				 <&cru SCLK_ISP_JPE>,
-				 <&cru SCLK_ISP>,
-				 <&cru SCLK_RGA>;
-		};
-
-		pd_video {
-			reg = <RK3288_PD_VIDEO>;
-			clocks = <&cru ACLK_VCODEC>,
-				 <&cru HCLK_VCODEC>;
-		};
-	};
 };
diff --git a/arch/arm/mach-rockchip/pm_domains.c b/arch/arm/mach-rockchip/pm_domains.c
index 92d2569..f3d87c21 100644
--- a/arch/arm/mach-rockchip/pm_domains.c
+++ b/arch/arm/mach-rockchip/pm_domains.c
@@ -377,6 +377,7 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct device_node *node;
+	struct device *parent;
 	struct rockchip_pmu *pmu;
 	const struct of_device_id *match;
 	const struct rockchip_pmu_info *pmu_info;
@@ -410,9 +411,13 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 	pmu->genpd_data.domains = pmu->domains;
 	pmu->genpd_data.num_domains = pmu_info->num_domains;
 
-	node = of_parse_phandle(np, "rockchip,pmu", 0);
-	pmu->regmap = syscon_node_to_regmap(node);
-	of_node_put(node);
+	parent = dev->parent;
+	if (!parent) {
+		dev_err(dev, "no parent for syscon LED\n");
+		return -ENODEV;
+	}
+
+	pmu->regmap = syscon_node_to_regmap(parent->of_node);
 	if (IS_ERR(pmu->regmap)) {
 		error = PTR_ERR(pmu->regmap);
 		dev_err(dev, "failed to get PMU regmap: %d\n", error);
-- 
2.1.4

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

[Kevin Hilman]

Heiko Stübner <heiko@sntech.de> writes:

> Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang:
>>     Add power domain drivers based on generic power domain for
>> Rockchip platform, and support RK3288.
>> 
>>     Verified on url =
>>     https://chromium.googlesource.com/chromiumos/third_party/kernel.
>> 
>>     At the moment,there are mass of products are using the driver.
>> I believe the driver can happy work for next kernel.
>
> I've taken a look at the driver and here are some global remarks:
>
> (1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks
> bisectability, as the driver itself in patch 2 also includes the header and
> would thus fail to compile if the later patch 3 is missing.
> Ideally I think the header addition should be a separate patch itself, so that
> we can possibly share it between driver and dts branches.
> So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes.
>
>
> (2) The dts-changes in patch 3 should also add any necessary power-domain
> assignment on devices if they're still missing, so that we don't introduce
> regressions. In my case my work-in-progress edp died because the powerdomain
> was turned off automatically it seems.
>
>
> (3) more like wondering @Kevin or so, is there some more generic place for a
> power-domain driver nowadays?

I think the preference has been to put these under drivers/soc/<vendor> for now,
so they can shared across arm32 and arm64.

> (4) As Tomasz remarked previously the dts should represent the hardware and
> the power-domains are part of the pmu. There is a recent addition from Linus
> Walleij, called simple-mfd [a] that is supposed to get added real early for
> kernel 4.2. So I'd think the power-domains should use that and the patchset
> modified to include the changes shown below [b]?
>
> (5) Keven Hilman and Tomasz had reservations about all the device clocks
> being listed in the power-domains itself in the previous versions. I don't see
> a comment from them yet changing that view.

Correct.  

> Their wish was to get the clocks by reading the clocks from the device nodes,
> though I see a problem on how to handle devices that do not have any bindings
> at all yet.
>
> Kevin, Tomasz any new thoughts?

I don't see any issues with devices that don't have bindings, as all
that would be needed would be to simple device nodes with a clock
property.  I wouldn't even matter if those devices had device drivers.

Kevin

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

[Kevin Hilman]

Heiko St?bner <heiko@sntech.de> writes:

> Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang:
>>     Add power domain drivers based on generic power domain for
>> Rockchip platform, and support RK3288.
>> 
>>     Verified on url =
>>     https://chromium.googlesource.com/chromiumos/third_party/kernel.
>> 
>>     At the moment,there are mass of products are using the driver.
>> I believe the driver can happy work for next kernel.
>
> I've taken a look at the driver and here are some global remarks:
>
> (1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks
> bisectability, as the driver itself in patch 2 also includes the header and
> would thus fail to compile if the later patch 3 is missing.
> Ideally I think the header addition should be a separate patch itself, so that
> we can possibly share it between driver and dts branches.
> So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes.
>
>
> (2) The dts-changes in patch 3 should also add any necessary power-domain
> assignment on devices if they're still missing, so that we don't introduce
> regressions. In my case my work-in-progress edp died because the powerdomain
> was turned off automatically it seems.
>
>
> (3) more like wondering @Kevin or so, is there some more generic place for a
> power-domain driver nowadays?

I think the preference has been to put these under drivers/soc/<vendor> for now,
so they can shared across arm32 and arm64.

> (4) As Tomasz remarked previously the dts should represent the hardware and
> the power-domains are part of the pmu. There is a recent addition from Linus
> Walleij, called simple-mfd [a] that is supposed to get added real early for
> kernel 4.2. So I'd think the power-domains should use that and the patchset
> modified to include the changes shown below [b]?
>
> (5) Keven Hilman and Tomasz had reservations about all the device clocks
> being listed in the power-domains itself in the previous versions. I don't see
> a comment from them yet changing that view.

Correct.  

> Their wish was to get the clocks by reading the clocks from the device nodes,
> though I see a problem on how to handle devices that do not have any bindings
> at all yet.
>
> Kevin, Tomasz any new thoughts?

I don't see any issues with devices that don't have bindings, as all
that would be needed would be to simple device nodes with a clock
property.  I wouldn't even matter if those devices had device drivers.

Kevin

Re: [PATCH v14 1/3] dt-bindings: add document of Rockchip power domain

[Ulf Hansson]

On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
> This add the necessary binding documentation for the power domain
> found on Rockchip Socs.
>
> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>  .../bindings/arm/rockchip/power_domain.txt         | 48 ++++++++++++++++++++++
>  1 file changed, 48 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..3e74e6d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
> @@ -0,0 +1,48 @@
> +* 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-controller - for rk3288 type power domain.
> +- #power-domain-cells: Number of cells in a power-domain specifier.
> +                      should be 1.
> +- rockchip,pmu: phandle referencing a syscon providing the pmu registers
> +- #address-cells: should be 1.
> +- #size-cells: should be 0.
> +
> +Required properties for power domain sub nodes:
> +- reg: index of the power domain, should use macros in:
> +    *  include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.

I can't find the above file, nor is it being adding in $subject patch.

Moreover, there are already
"include/dt-bindings/arm/ux500_pm_domains.h".  I suppose we could move
that file into your suggested path, since it seems more generic?

> +- clocks (optional): phandles to clocks which need to be enabled while power domain
> +          switches state.
> +
> +Example:
> +
> +       power: power-controller {
> +              compatible = "rockchip,rk3288-power-controller";
> +              #power-domain-cells = <1>;
> +              rockchip,pmu = <&pmu>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +              pd_gpu {
> +                      reg = <RK3288_PD_GPU>;
> +                      clocks = <&cru ACLK_GPU>;
> +              };
> +       };
> +
> +Node of a device using power domains must have a power-domains property,
> +containing a phandle to the power device node and an index specifying which
> +power domain to use.
> +The index should use macros in:
> +   * include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.

Same comment as above.

> +
> +Example of the node using power domain:
> +
> +       node {
> +               /* ... */
> +               power-domains = <&power RK3288_PD_GPU>;
> +               /* ... */
> +       };
> --
> 1.9.1
>
>

Kind regards
Uffe

Re: [PATCH v14 1/3] dt-bindings: add document of Rockchip power domain

[Ulf Hansson]

On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
> This add the necessary binding documentation for the power domain
> found on Rockchip Socs.
>
> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>  .../bindings/arm/rockchip/power_domain.txt         | 48 ++++++++++++++++++++++
>  1 file changed, 48 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..3e74e6d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
> @@ -0,0 +1,48 @@
> +* 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-controller - for rk3288 type power domain.
> +- #power-domain-cells: Number of cells in a power-domain specifier.
> +                      should be 1.
> +- rockchip,pmu: phandle referencing a syscon providing the pmu registers
> +- #address-cells: should be 1.
> +- #size-cells: should be 0.
> +
> +Required properties for power domain sub nodes:
> +- reg: index of the power domain, should use macros in:
> +    *  include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.

I can't find the above file, nor is it being adding in $subject patch.

Moreover, there are already
"include/dt-bindings/arm/ux500_pm_domains.h".  I suppose we could move
that file into your suggested path, since it seems more generic?

> +- clocks (optional): phandles to clocks which need to be enabled while power domain
> +          switches state.
> +
> +Example:
> +
> +       power: power-controller {
> +              compatible = "rockchip,rk3288-power-controller";
> +              #power-domain-cells = <1>;
> +              rockchip,pmu = <&pmu>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +              pd_gpu {
> +                      reg = <RK3288_PD_GPU>;
> +                      clocks = <&cru ACLK_GPU>;
> +              };
> +       };
> +
> +Node of a device using power domains must have a power-domains property,
> +containing a phandle to the power device node and an index specifying which
> +power domain to use.
> +The index should use macros in:
> +   * include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.

Same comment as above.

> +
> +Example of the node using power domain:
> +
> +       node {
> +               /* ... */
> +               power-domains = <&power RK3288_PD_GPU>;
> +               /* ... */
> +       };
> --
> 1.9.1
>
>

Kind regards
Uffe

[PATCH v14 1/3] dt-bindings: add document of Rockchip power domain

[Ulf Hansson]

On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
> This add the necessary binding documentation for the power domain
> found on Rockchip Socs.
>
> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>  .../bindings/arm/rockchip/power_domain.txt         | 48 ++++++++++++++++++++++
>  1 file changed, 48 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..3e74e6d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/rockchip/power_domain.txt
> @@ -0,0 +1,48 @@
> +* 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-controller - for rk3288 type power domain.
> +- #power-domain-cells: Number of cells in a power-domain specifier.
> +                      should be 1.
> +- rockchip,pmu: phandle referencing a syscon providing the pmu registers
> +- #address-cells: should be 1.
> +- #size-cells: should be 0.
> +
> +Required properties for power domain sub nodes:
> +- reg: index of the power domain, should use macros in:
> +    *  include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.

I can't find the above file, nor is it being adding in $subject patch.

Moreover, there are already
"include/dt-bindings/arm/ux500_pm_domains.h".  I suppose we could move
that file into your suggested path, since it seems more generic?

> +- clocks (optional): phandles to clocks which need to be enabled while power domain
> +          switches state.
> +
> +Example:
> +
> +       power: power-controller {
> +              compatible = "rockchip,rk3288-power-controller";
> +              #power-domain-cells = <1>;
> +              rockchip,pmu = <&pmu>;
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +              pd_gpu {
> +                      reg = <RK3288_PD_GPU>;
> +                      clocks = <&cru ACLK_GPU>;
> +              };
> +       };
> +
> +Node of a device using power domains must have a power-domains property,
> +containing a phandle to the power device node and an index specifying which
> +power domain to use.
> +The index should use macros in:
> +   * include/dt-bindings/power-domain/rk3288.h - for rk3288 type power domain.

Same comment as above.

> +
> +Example of the node using power domain:
> +
> +       node {
> +               /* ... */
> +               power-domains = <&power RK3288_PD_GPU>;
> +               /* ... */
> +       };
> --
> 1.9.1
>
>

Kind regards
Uffe

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Ulf Hansson]

On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
> In order to meet high performance and low power requirements, a power
> management unit is designed or saving power when RK3288 in low power
> mode.
> The RK3288 PMU is dedicated for managing the power ot the whole chip.
>
> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>  arch/arm/mach-rockchip/Kconfig      |   1 +
>  arch/arm/mach-rockchip/Makefile     |   1 +
>  arch/arm/mach-rockchip/pm_domains.c | 506 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 508 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 ae4eb7c..578206b 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
>         select ROCKCHIP_TIMER
>         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 5c3a9b2..9c902d3 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -1,5 +1,6 @@
>  CFLAGS_platsmp.o := -march=armv7-a
>
>  obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
>  obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
>  obj-$(CONFIG_SMP) += headsmp.o platsmp.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..92d2569
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/pm_domains.c
> @@ -0,0 +1,506 @@
> +/*
> + * Rockchip Generic power domain support.
> + *
> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
> + *
> + * 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/io.h>
> +#include <linux/err.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_domain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

clk-provider.h, why?

> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <dt-bindings/power-domain/rk3288.h>

Same comment as in patch1. I don't find this header.

> +
> +struct rockchip_domain_info {
> +       int pwr_mask;
> +       int status_mask;
> +       int req_mask;
> +       int idle_mask;
> +       int ack_mask;
> +};
> +
> +struct rockchip_pmu_info {
> +       u32 pwr_offset;
> +       u32 status_offset;
> +       u32 req_offset;
> +       u32 idle_offset;
> +       u32 ack_offset;
> +
> +       u32 core_pwrcnt_offset;
> +       u32 gpu_pwrcnt_offset;
> +
> +       unsigned int core_power_transition_time;
> +       unsigned int gpu_power_transition_time;
> +
> +       int num_domains;
> +       const struct rockchip_domain_info *domain_info;
> +};
> +
> +struct rockchip_pm_domain {
> +       struct generic_pm_domain genpd;
> +       const struct rockchip_domain_info *info;
> +       struct rockchip_pmu *pmu;
> +       int num_clks;
> +       struct clk *clks[];
> +};
> +
> +struct rockchip_pmu {
> +       struct device *dev;
> +       struct regmap *regmap;
> +       const struct rockchip_pmu_info *info;
> +       struct mutex mutex; /* mutex lock for pmu */
> +       struct genpd_onecell_data genpd_data;
> +       struct generic_pm_domain *domains[];
> +};
> +
> +#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
> +
> +#define DOMAIN(pwr, status, req, idle, ack)    \
> +{                                              \
> +       .pwr_mask = BIT(pwr),                   \
> +       .status_mask = BIT(status),             \
> +       .req_mask = BIT(req),                   \
> +       .idle_mask = BIT(idle),                 \
> +       .ack_mask = BIT(ack),                   \
> +}
> +
> +#define DOMAIN_RK3288(pwr, status, req)                \
> +       DOMAIN(pwr, status, req, req, (req) + 16)
> +
> +static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       const struct rockchip_domain_info *pd_info = pd->info;
> +       unsigned int val;
> +
> +       regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
> +       return (val & pd_info->idle_mask) == pd_info->idle_mask;
> +}
> +
> +static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
> +                                        bool idle)
> +{
> +       const struct rockchip_domain_info *pd_info = pd->info;
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       unsigned int val;
> +
> +       regmap_update_bits(pmu->regmap, pmu->info->req_offset,
> +                          pd_info->req_mask, idle ? -1U : 0);
> +
> +       dsb();
> +
> +       do {
> +               regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
> +       } while ((val & pd_info->ack_mask) != (idle ? pd_info->ack_mask : 0));
> +
> +       while (rockchip_pmu_domain_is_idle(pd) != idle)
> +               cpu_relax();
> +
> +       return 0;
> +}
> +
> +static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       unsigned int val;
> +
> +       regmap_read(pmu->regmap, pmu->info->status_offset, &val);
> +
> +       /* 1'b0: power on, 1'b1: power off */
> +       return !(val & pd->info->status_mask);
> +}
> +
> +static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> +                                            bool on)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +
> +       regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
> +                          pd->info->pwr_mask, on ? 0 : -1U);
> +
> +       dsb();
> +
> +       while (rockchip_pmu_domain_is_on(pd) != on)
> +               cpu_relax();
> +}
> +
> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> +{
> +       int i;
> +
> +       mutex_lock(&pd->pmu->mutex);

I don't quite understand why you need a lock, what are you protecting and why?

> +
> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
> +               for (i = 0; i < pd->num_clks; i++)
> +                       clk_enable(pd->clks[i]);
> +
> +               if (!power_on) {
> +                       /* FIXME: add code to save AXI_QOS */
> +
> +                       /* if powering down, idle request to NIU first */
> +                       rockchip_pmu_set_idle_request(pd, true);
> +               }
> +
> +               rockchip_do_pmu_set_power_domain(pd, power_on);
> +
> +               if (power_on) {
> +                       /* if powering up, leave idle mode */
> +                       rockchip_pmu_set_idle_request(pd, false);
> +
> +                       /* FIXME: add code to restore AXI_QOS */
> +               }
> +
> +               for (i = pd->num_clks - 1; i >= 0; i--)
> +                       clk_disable(pd->clks[i]);
> +       }
> +
> +       mutex_unlock(&pd->pmu->mutex);
> +       return 0;
> +}
> +
> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
> +{
> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
> +
> +       return rockchip_pd_power(pd, false);
> +}
> +
> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
> +                                 struct device *dev)
> +{
> +       struct clk *clk;
> +       int i;
> +       int error;
> +
> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
> +
> +       error = pm_clk_create(dev);
> +       if (error) {
> +               dev_err(dev, "pm_clk_create failed %d\n", error);
> +               return error;
> +       }
> +
> +       i = 0;

> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {

This loop adds all available device clocks to the PM clock list. I
wonder if this could be considered as a common thing and if so, we
might want to extend the pm_clk API with this.

> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
> +                       __clk_get_name(clk));
> +               error = pm_clk_add_clk(dev, clk);
> +               clk_put(clk);
> +               if (error) {
> +                       dev_err(dev, "pm_clk_add_clk failed %d\n", error);
> +                       pm_clk_destroy(dev);
> +                       return error;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
> +                                  struct device *dev)
> +{
> +       dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name);
> +
> +       pm_clk_destroy(dev);
> +}
> +
> +static int rockchip_pd_start_dev(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> +       dev_dbg(dev, "starting device in power domain '%s'\n", genpd->name);
> +
> +       return pm_clk_resume(dev);
> +}
> +
> +static int rockchip_pd_stop_dev(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> +       dev_dbg(dev, "stopping device in power domain '%s'\n", genpd->name);
> +
> +       return pm_clk_suspend(dev);
> +}
> +
> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
> +                                     struct device_node *node)
> +{
> +       const struct rockchip_domain_info *pd_info;
> +       struct rockchip_pm_domain *pd;
> +       struct clk *clk;
> +       int clk_cnt;
> +       int i;
> +       u32 id;
> +       int error;
> +
> +       error = of_property_read_u32(node, "reg", &id);
> +       if (error) {
> +               dev_err(pmu->dev,
> +                       "%s: failed to retrieve domain id (reg): %d\n",
> +                       node->name, error);
> +               return -EINVAL;
> +       }
> +
> +       if (id >= pmu->info->num_domains) {
> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
> +                       node->name, id);
> +               return -EINVAL;
> +       }
> +
> +       pd_info = &pmu->info->domain_info[id];
> +       if (!pd_info) {
> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
> +                       node->name, id);
> +               return -EINVAL;
> +       }
> +
> +       clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells");
> +       pd = devm_kzalloc(pmu->dev,
> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
> +                         GFP_KERNEL);
> +       if (!pd)
> +               return -ENOMEM;
> +
> +       pd->info = pd_info;
> +       pd->pmu = pmu;
> +
> +       for (i = 0; i < clk_cnt; i++) {

This loop is similar to the one when creates the PM clock list in the
rockchip_pd_attach_dev().

What's the reason you don't want to use pm clk for these clocks?

> +               clk = of_clk_get(node, i);
> +               if (IS_ERR(clk)) {
> +                       error = PTR_ERR(clk);
> +                       dev_err(pmu->dev,
> +                               "%s: failed to get clk %s (index %d): %d\n",
> +                               node->name, __clk_get_name(clk), i, error);
> +                       goto err_out;
> +               }
> +
> +               error = clk_prepare(clk);
> +               if (error) {
> +                       dev_err(pmu->dev,
> +                               "%s: failed to prepare clk %s (index %d): %d\n",
> +                               node->name, __clk_get_name(clk), i, error);
> +                       clk_put(clk);
> +                       goto err_out;
> +               }
> +
> +               pd->clks[pd->num_clks++] = clk;
> +
> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
> +                       __clk_get_name(clk), node->name);
> +       }
> +
> +       error = rockchip_pd_power(pd, true);
> +       if (error) {
> +               dev_err(pmu->dev,
> +                       "failed to power on domain '%s': %d\n",
> +                       node->name, error);
> +               goto err_out;
> +       }
> +
> +       pd->genpd.name = node->name;
> +       pd->genpd.power_off = rockchip_pd_power_off;
> +       pd->genpd.power_on = rockchip_pd_power_on;
> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;

Instead of assigning the ->stop|start() callbacks, which do
pm_clk_suspend|resume(), just set the genpd->flags to
GENPD_FLAG_PM_CLK, and genpd will fix this for you.

> +       pm_genpd_init(&pd->genpd, NULL, false);
> +
> +       pmu->genpd_data.domains[id] = &pd->genpd;
> +       return 0;
> +
> +err_out:
> +       while (--i >= 0) {
> +               clk_unprepare(pd->clks[i]);
> +               clk_put(pd->clks[i]);
> +       }
> +       return error;
> +}
> +
> +static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
> +{
> +       int i;
> +
> +       for (i = 0; i < pd->num_clks; i++) {
> +               clk_unprepare(pd->clks[i]);

If you converted these clocks to be dealt with via the PM clk API,
pm_clk_destoy() would have replaced this loop.

> +               clk_put(pd->clks[i]);
> +       }
> +
> +       /* devm will free our memory */
> +}
> +
> +static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
> +{
> +       struct generic_pm_domain *genpd;
> +       struct rockchip_pm_domain *pd;
> +       int i;
> +
> +       for (i = 0; i < pmu->genpd_data.num_domains; i++) {
> +               genpd = pmu->genpd_data.domains[i];
> +               if (genpd) {
> +                       pd = to_rockchip_pd(genpd);
> +                       rockchip_pm_remove_one_domain(pd);
> +               }
> +       }
> +
> +       /* devm will free our memory */
> +}
> +
> +static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
> +                                     u32 domain_reg_offset,
> +                                     unsigned int count)
> +{
> +       /* First configure domain power down transition count ... */
> +       regmap_write(pmu->regmap, domain_reg_offset, count);
> +       /* ... and then power up count. */
> +       regmap_write(pmu->regmap, domain_reg_offset + 4, count);
> +}
> +
> +static int rockchip_pm_domain_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct device_node *node;
> +       struct rockchip_pmu *pmu;
> +       const struct of_device_id *match;
> +       const struct rockchip_pmu_info *pmu_info;
> +       int error;
> +
> +       if (!np) {
> +               dev_err(dev, "device tree node not found\n");
> +               return -ENXIO;/

Nitpick:
It's more common to return -ENODEV here, you might want to change.

> +       }
> +
> +       match = of_match_device(dev->driver->of_match_table, dev);
> +       if (!match || !match->data) {
> +               dev_err(dev, "missing pmu data\n");
> +               return -EINVAL;
> +       }
> +
> +       pmu_info = match->data;
> +
> +       pmu = devm_kzalloc(dev,
> +                          sizeof(*pmu) +
> +                               pmu_info->num_domains * sizeof(pmu->domains[0]),
> +                          GFP_KERNEL);
> +       if (!pmu)
> +               return -ENOMEM;
> +
> +       pmu->dev = &pdev->dev;
> +       mutex_init(&pmu->mutex);
> +
> +       pmu->info = pmu_info;
> +
> +       pmu->genpd_data.domains = pmu->domains;
> +       pmu->genpd_data.num_domains = pmu_info->num_domains;
> +
> +       node = of_parse_phandle(np, "rockchip,pmu", 0);
> +       pmu->regmap = syscon_node_to_regmap(node);
> +       of_node_put(node);
> +       if (IS_ERR(pmu->regmap)) {
> +               error = PTR_ERR(pmu->regmap);
> +               dev_err(dev, "failed to get PMU regmap: %d\n", error);
> +               return error;
> +       }
> +
> +       /*
> +        * Configure power up and down transition delays for core
> +        * and GPU domains.
> +        */
> +       rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
> +                                 pmu_info->core_power_transition_time);
> +       rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
> +                                 pmu_info->gpu_power_transition_time);
> +
> +       error = -ENXIO;
> +
> +       for_each_available_child_of_node(np, node) {
> +               error = rockchip_pm_add_one_domain(pmu, node);
> +               if (error) {
> +                       dev_err(dev, "failed to handle node %s: %d\n",
> +                               node->name, error);
> +                       goto err_out;
> +               }
> +       }
> +
> +       if (error) {
> +               dev_dbg(dev, "no power domains defined\n");
> +               goto err_out;
> +       }
> +
> +       of_genpd_add_provider_onecell(np, &pmu->genpd_data);
> +
> +       return 0;
> +
> +err_out:
> +       rockchip_pm_domain_cleanup(pmu);
> +       return error;
> +}
> +
> +static const struct rockchip_domain_info rk3288_pm_domains[] = {
> +       [RK3288_PD_GPU]         = DOMAIN_RK3288(9, 9, 2),
> +       [RK3288_PD_VIO]         = DOMAIN_RK3288(7, 7, 4),
> +       [RK3288_PD_VIDEO]       = DOMAIN_RK3288(8, 8, 3),
> +       [RK3288_PD_HEVC]        = DOMAIN_RK3288(14, 10, 9),
> +};
> +
> +static const struct rockchip_pmu_info rk3288_pmu = {
> +       .pwr_offset = 0x08,
> +       .status_offset = 0x0c,
> +       .req_offset = 0x10,
> +       .idle_offset = 0x14,
> +       .ack_offset = 0x14,
> +
> +       .core_pwrcnt_offset = 0x34,
> +       .gpu_pwrcnt_offset = 0x3c,
> +
> +       .core_power_transition_time = 24, /* 1us */
> +       .gpu_power_transition_time = 24, /* 1us */
> +
> +       .num_domains = ARRAY_SIZE(rk3288_pm_domains),
> +       .domain_info = rk3288_pm_domains,
> +};
> +
> +static const struct of_device_id rockchip_pm_domain_dt_match[] = {
> +       {
> +               .compatible = "rockchip,rk3288-power-controller",
> +               .data = (void *)&rk3288_pmu,
> +       },
> +       { /* sentinel */ },
> +};
> +
> +static struct platform_driver rockchip_pm_domain_driver = {
> +       .probe = rockchip_pm_domain_probe,
> +       .driver = {
> +               .name   = "rockchip-pm-domain",
> +               .of_match_table = rockchip_pm_domain_dt_match,
> +               /*
> +                * We can't forcibly eject devices form power domain,
> +                * so we can't really remove power domains once they
> +                * were added.
> +                */
> +               .suppress_bind_attrs = true,
> +       },
> +};
> +
> +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.9.1
>

Kind regards
Uffe

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Ulf Hansson]

On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
> In order to meet high performance and low power requirements, a power
> management unit is designed or saving power when RK3288 in low power
> mode.
> The RK3288 PMU is dedicated for managing the power ot the whole chip.
>
> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>  arch/arm/mach-rockchip/Kconfig      |   1 +
>  arch/arm/mach-rockchip/Makefile     |   1 +
>  arch/arm/mach-rockchip/pm_domains.c | 506 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 508 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 ae4eb7c..578206b 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
>         select ROCKCHIP_TIMER
>         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 5c3a9b2..9c902d3 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -1,5 +1,6 @@
>  CFLAGS_platsmp.o := -march=armv7-a
>
>  obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
>  obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
>  obj-$(CONFIG_SMP) += headsmp.o platsmp.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..92d2569
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/pm_domains.c
> @@ -0,0 +1,506 @@
> +/*
> + * Rockchip Generic power domain support.
> + *
> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
> + *
> + * 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/io.h>
> +#include <linux/err.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_domain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

clk-provider.h, why?

> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <dt-bindings/power-domain/rk3288.h>

Same comment as in patch1. I don't find this header.

> +
> +struct rockchip_domain_info {
> +       int pwr_mask;
> +       int status_mask;
> +       int req_mask;
> +       int idle_mask;
> +       int ack_mask;
> +};
> +
> +struct rockchip_pmu_info {
> +       u32 pwr_offset;
> +       u32 status_offset;
> +       u32 req_offset;
> +       u32 idle_offset;
> +       u32 ack_offset;
> +
> +       u32 core_pwrcnt_offset;
> +       u32 gpu_pwrcnt_offset;
> +
> +       unsigned int core_power_transition_time;
> +       unsigned int gpu_power_transition_time;
> +
> +       int num_domains;
> +       const struct rockchip_domain_info *domain_info;
> +};
> +
> +struct rockchip_pm_domain {
> +       struct generic_pm_domain genpd;
> +       const struct rockchip_domain_info *info;
> +       struct rockchip_pmu *pmu;
> +       int num_clks;
> +       struct clk *clks[];
> +};
> +
> +struct rockchip_pmu {
> +       struct device *dev;
> +       struct regmap *regmap;
> +       const struct rockchip_pmu_info *info;
> +       struct mutex mutex; /* mutex lock for pmu */
> +       struct genpd_onecell_data genpd_data;
> +       struct generic_pm_domain *domains[];
> +};
> +
> +#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
> +
> +#define DOMAIN(pwr, status, req, idle, ack)    \
> +{                                              \
> +       .pwr_mask = BIT(pwr),                   \
> +       .status_mask = BIT(status),             \
> +       .req_mask = BIT(req),                   \
> +       .idle_mask = BIT(idle),                 \
> +       .ack_mask = BIT(ack),                   \
> +}
> +
> +#define DOMAIN_RK3288(pwr, status, req)                \
> +       DOMAIN(pwr, status, req, req, (req) + 16)
> +
> +static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       const struct rockchip_domain_info *pd_info = pd->info;
> +       unsigned int val;
> +
> +       regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
> +       return (val & pd_info->idle_mask) == pd_info->idle_mask;
> +}
> +
> +static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
> +                                        bool idle)
> +{
> +       const struct rockchip_domain_info *pd_info = pd->info;
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       unsigned int val;
> +
> +       regmap_update_bits(pmu->regmap, pmu->info->req_offset,
> +                          pd_info->req_mask, idle ? -1U : 0);
> +
> +       dsb();
> +
> +       do {
> +               regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
> +       } while ((val & pd_info->ack_mask) != (idle ? pd_info->ack_mask : 0));
> +
> +       while (rockchip_pmu_domain_is_idle(pd) != idle)
> +               cpu_relax();
> +
> +       return 0;
> +}
> +
> +static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       unsigned int val;
> +
> +       regmap_read(pmu->regmap, pmu->info->status_offset, &val);
> +
> +       /* 1'b0: power on, 1'b1: power off */
> +       return !(val & pd->info->status_mask);
> +}
> +
> +static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> +                                            bool on)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +
> +       regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
> +                          pd->info->pwr_mask, on ? 0 : -1U);
> +
> +       dsb();
> +
> +       while (rockchip_pmu_domain_is_on(pd) != on)
> +               cpu_relax();
> +}
> +
> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> +{
> +       int i;
> +
> +       mutex_lock(&pd->pmu->mutex);

I don't quite understand why you need a lock, what are you protecting and why?

> +
> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
> +               for (i = 0; i < pd->num_clks; i++)
> +                       clk_enable(pd->clks[i]);
> +
> +               if (!power_on) {
> +                       /* FIXME: add code to save AXI_QOS */
> +
> +                       /* if powering down, idle request to NIU first */
> +                       rockchip_pmu_set_idle_request(pd, true);
> +               }
> +
> +               rockchip_do_pmu_set_power_domain(pd, power_on);
> +
> +               if (power_on) {
> +                       /* if powering up, leave idle mode */
> +                       rockchip_pmu_set_idle_request(pd, false);
> +
> +                       /* FIXME: add code to restore AXI_QOS */
> +               }
> +
> +               for (i = pd->num_clks - 1; i >= 0; i--)
> +                       clk_disable(pd->clks[i]);
> +       }
> +
> +       mutex_unlock(&pd->pmu->mutex);
> +       return 0;
> +}
> +
> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
> +{
> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
> +
> +       return rockchip_pd_power(pd, false);
> +}
> +
> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
> +                                 struct device *dev)
> +{
> +       struct clk *clk;
> +       int i;
> +       int error;
> +
> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
> +
> +       error = pm_clk_create(dev);
> +       if (error) {
> +               dev_err(dev, "pm_clk_create failed %d\n", error);
> +               return error;
> +       }
> +
> +       i = 0;

> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {

This loop adds all available device clocks to the PM clock list. I
wonder if this could be considered as a common thing and if so, we
might want to extend the pm_clk API with this.

> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
> +                       __clk_get_name(clk));
> +               error = pm_clk_add_clk(dev, clk);
> +               clk_put(clk);
> +               if (error) {
> +                       dev_err(dev, "pm_clk_add_clk failed %d\n", error);
> +                       pm_clk_destroy(dev);
> +                       return error;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
> +                                  struct device *dev)
> +{
> +       dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name);
> +
> +       pm_clk_destroy(dev);
> +}
> +
> +static int rockchip_pd_start_dev(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> +       dev_dbg(dev, "starting device in power domain '%s'\n", genpd->name);
> +
> +       return pm_clk_resume(dev);
> +}
> +
> +static int rockchip_pd_stop_dev(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> +       dev_dbg(dev, "stopping device in power domain '%s'\n", genpd->name);
> +
> +       return pm_clk_suspend(dev);
> +}
> +
> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
> +                                     struct device_node *node)
> +{
> +       const struct rockchip_domain_info *pd_info;
> +       struct rockchip_pm_domain *pd;
> +       struct clk *clk;
> +       int clk_cnt;
> +       int i;
> +       u32 id;
> +       int error;
> +
> +       error = of_property_read_u32(node, "reg", &id);
> +       if (error) {
> +               dev_err(pmu->dev,
> +                       "%s: failed to retrieve domain id (reg): %d\n",
> +                       node->name, error);
> +               return -EINVAL;
> +       }
> +
> +       if (id >= pmu->info->num_domains) {
> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
> +                       node->name, id);
> +               return -EINVAL;
> +       }
> +
> +       pd_info = &pmu->info->domain_info[id];
> +       if (!pd_info) {
> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
> +                       node->name, id);
> +               return -EINVAL;
> +       }
> +
> +       clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells");
> +       pd = devm_kzalloc(pmu->dev,
> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
> +                         GFP_KERNEL);
> +       if (!pd)
> +               return -ENOMEM;
> +
> +       pd->info = pd_info;
> +       pd->pmu = pmu;
> +
> +       for (i = 0; i < clk_cnt; i++) {

This loop is similar to the one when creates the PM clock list in the
rockchip_pd_attach_dev().

What's the reason you don't want to use pm clk for these clocks?

> +               clk = of_clk_get(node, i);
> +               if (IS_ERR(clk)) {
> +                       error = PTR_ERR(clk);
> +                       dev_err(pmu->dev,
> +                               "%s: failed to get clk %s (index %d): %d\n",
> +                               node->name, __clk_get_name(clk), i, error);
> +                       goto err_out;
> +               }
> +
> +               error = clk_prepare(clk);
> +               if (error) {
> +                       dev_err(pmu->dev,
> +                               "%s: failed to prepare clk %s (index %d): %d\n",
> +                               node->name, __clk_get_name(clk), i, error);
> +                       clk_put(clk);
> +                       goto err_out;
> +               }
> +
> +               pd->clks[pd->num_clks++] = clk;
> +
> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
> +                       __clk_get_name(clk), node->name);
> +       }
> +
> +       error = rockchip_pd_power(pd, true);
> +       if (error) {
> +               dev_err(pmu->dev,
> +                       "failed to power on domain '%s': %d\n",
> +                       node->name, error);
> +               goto err_out;
> +       }
> +
> +       pd->genpd.name = node->name;
> +       pd->genpd.power_off = rockchip_pd_power_off;
> +       pd->genpd.power_on = rockchip_pd_power_on;
> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;

Instead of assigning the ->stop|start() callbacks, which do
pm_clk_suspend|resume(), just set the genpd->flags to
GENPD_FLAG_PM_CLK, and genpd will fix this for you.

> +       pm_genpd_init(&pd->genpd, NULL, false);
> +
> +       pmu->genpd_data.domains[id] = &pd->genpd;
> +       return 0;
> +
> +err_out:
> +       while (--i >= 0) {
> +               clk_unprepare(pd->clks[i]);
> +               clk_put(pd->clks[i]);
> +       }
> +       return error;
> +}
> +
> +static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
> +{
> +       int i;
> +
> +       for (i = 0; i < pd->num_clks; i++) {
> +               clk_unprepare(pd->clks[i]);

If you converted these clocks to be dealt with via the PM clk API,
pm_clk_destoy() would have replaced this loop.

> +               clk_put(pd->clks[i]);
> +       }
> +
> +       /* devm will free our memory */
> +}
> +
> +static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
> +{
> +       struct generic_pm_domain *genpd;
> +       struct rockchip_pm_domain *pd;
> +       int i;
> +
> +       for (i = 0; i < pmu->genpd_data.num_domains; i++) {
> +               genpd = pmu->genpd_data.domains[i];
> +               if (genpd) {
> +                       pd = to_rockchip_pd(genpd);
> +                       rockchip_pm_remove_one_domain(pd);
> +               }
> +       }
> +
> +       /* devm will free our memory */
> +}
> +
> +static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
> +                                     u32 domain_reg_offset,
> +                                     unsigned int count)
> +{
> +       /* First configure domain power down transition count ... */
> +       regmap_write(pmu->regmap, domain_reg_offset, count);
> +       /* ... and then power up count. */
> +       regmap_write(pmu->regmap, domain_reg_offset + 4, count);
> +}
> +
> +static int rockchip_pm_domain_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct device_node *node;
> +       struct rockchip_pmu *pmu;
> +       const struct of_device_id *match;
> +       const struct rockchip_pmu_info *pmu_info;
> +       int error;
> +
> +       if (!np) {
> +               dev_err(dev, "device tree node not found\n");
> +               return -ENXIO;/

Nitpick:
It's more common to return -ENODEV here, you might want to change.

> +       }
> +
> +       match = of_match_device(dev->driver->of_match_table, dev);
> +       if (!match || !match->data) {
> +               dev_err(dev, "missing pmu data\n");
> +               return -EINVAL;
> +       }
> +
> +       pmu_info = match->data;
> +
> +       pmu = devm_kzalloc(dev,
> +                          sizeof(*pmu) +
> +                               pmu_info->num_domains * sizeof(pmu->domains[0]),
> +                          GFP_KERNEL);
> +       if (!pmu)
> +               return -ENOMEM;
> +
> +       pmu->dev = &pdev->dev;
> +       mutex_init(&pmu->mutex);
> +
> +       pmu->info = pmu_info;
> +
> +       pmu->genpd_data.domains = pmu->domains;
> +       pmu->genpd_data.num_domains = pmu_info->num_domains;
> +
> +       node = of_parse_phandle(np, "rockchip,pmu", 0);
> +       pmu->regmap = syscon_node_to_regmap(node);
> +       of_node_put(node);
> +       if (IS_ERR(pmu->regmap)) {
> +               error = PTR_ERR(pmu->regmap);
> +               dev_err(dev, "failed to get PMU regmap: %d\n", error);
> +               return error;
> +       }
> +
> +       /*
> +        * Configure power up and down transition delays for core
> +        * and GPU domains.
> +        */
> +       rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
> +                                 pmu_info->core_power_transition_time);
> +       rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
> +                                 pmu_info->gpu_power_transition_time);
> +
> +       error = -ENXIO;
> +
> +       for_each_available_child_of_node(np, node) {
> +               error = rockchip_pm_add_one_domain(pmu, node);
> +               if (error) {
> +                       dev_err(dev, "failed to handle node %s: %d\n",
> +                               node->name, error);
> +                       goto err_out;
> +               }
> +       }
> +
> +       if (error) {
> +               dev_dbg(dev, "no power domains defined\n");
> +               goto err_out;
> +       }
> +
> +       of_genpd_add_provider_onecell(np, &pmu->genpd_data);
> +
> +       return 0;
> +
> +err_out:
> +       rockchip_pm_domain_cleanup(pmu);
> +       return error;
> +}
> +
> +static const struct rockchip_domain_info rk3288_pm_domains[] = {
> +       [RK3288_PD_GPU]         = DOMAIN_RK3288(9, 9, 2),
> +       [RK3288_PD_VIO]         = DOMAIN_RK3288(7, 7, 4),
> +       [RK3288_PD_VIDEO]       = DOMAIN_RK3288(8, 8, 3),
> +       [RK3288_PD_HEVC]        = DOMAIN_RK3288(14, 10, 9),
> +};
> +
> +static const struct rockchip_pmu_info rk3288_pmu = {
> +       .pwr_offset = 0x08,
> +       .status_offset = 0x0c,
> +       .req_offset = 0x10,
> +       .idle_offset = 0x14,
> +       .ack_offset = 0x14,
> +
> +       .core_pwrcnt_offset = 0x34,
> +       .gpu_pwrcnt_offset = 0x3c,
> +
> +       .core_power_transition_time = 24, /* 1us */
> +       .gpu_power_transition_time = 24, /* 1us */
> +
> +       .num_domains = ARRAY_SIZE(rk3288_pm_domains),
> +       .domain_info = rk3288_pm_domains,
> +};
> +
> +static const struct of_device_id rockchip_pm_domain_dt_match[] = {
> +       {
> +               .compatible = "rockchip,rk3288-power-controller",
> +               .data = (void *)&rk3288_pmu,
> +       },
> +       { /* sentinel */ },
> +};
> +
> +static struct platform_driver rockchip_pm_domain_driver = {
> +       .probe = rockchip_pm_domain_probe,
> +       .driver = {
> +               .name   = "rockchip-pm-domain",
> +               .of_match_table = rockchip_pm_domain_dt_match,
> +               /*
> +                * We can't forcibly eject devices form power domain,
> +                * so we can't really remove power domains once they
> +                * were added.
> +                */
> +               .suppress_bind_attrs = true,
> +       },
> +};
> +
> +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.9.1
>

Kind regards
Uffe

[PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Ulf Hansson]

On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
> In order to meet high performance and low power requirements, a power
> management unit is designed or saving power when RK3288 in low power
> mode.
> The RK3288 PMU is dedicated for managing the power ot the whole chip.
>
> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>  arch/arm/mach-rockchip/Kconfig      |   1 +
>  arch/arm/mach-rockchip/Makefile     |   1 +
>  arch/arm/mach-rockchip/pm_domains.c | 506 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 508 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 ae4eb7c..578206b 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
>         select ROCKCHIP_TIMER
>         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 5c3a9b2..9c902d3 100644
> --- a/arch/arm/mach-rockchip/Makefile
> +++ b/arch/arm/mach-rockchip/Makefile
> @@ -1,5 +1,6 @@
>  CFLAGS_platsmp.o := -march=armv7-a
>
>  obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
>  obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
>  obj-$(CONFIG_SMP) += headsmp.o platsmp.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..92d2569
> --- /dev/null
> +++ b/arch/arm/mach-rockchip/pm_domains.c
> @@ -0,0 +1,506 @@
> +/*
> + * Rockchip Generic power domain support.
> + *
> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
> + *
> + * 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/io.h>
> +#include <linux/err.h>
> +#include <linux/pm_clock.h>
> +#include <linux/pm_domain.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

clk-provider.h, why?

> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +#include <dt-bindings/power-domain/rk3288.h>

Same comment as in patch1. I don't find this header.

> +
> +struct rockchip_domain_info {
> +       int pwr_mask;
> +       int status_mask;
> +       int req_mask;
> +       int idle_mask;
> +       int ack_mask;
> +};
> +
> +struct rockchip_pmu_info {
> +       u32 pwr_offset;
> +       u32 status_offset;
> +       u32 req_offset;
> +       u32 idle_offset;
> +       u32 ack_offset;
> +
> +       u32 core_pwrcnt_offset;
> +       u32 gpu_pwrcnt_offset;
> +
> +       unsigned int core_power_transition_time;
> +       unsigned int gpu_power_transition_time;
> +
> +       int num_domains;
> +       const struct rockchip_domain_info *domain_info;
> +};
> +
> +struct rockchip_pm_domain {
> +       struct generic_pm_domain genpd;
> +       const struct rockchip_domain_info *info;
> +       struct rockchip_pmu *pmu;
> +       int num_clks;
> +       struct clk *clks[];
> +};
> +
> +struct rockchip_pmu {
> +       struct device *dev;
> +       struct regmap *regmap;
> +       const struct rockchip_pmu_info *info;
> +       struct mutex mutex; /* mutex lock for pmu */
> +       struct genpd_onecell_data genpd_data;
> +       struct generic_pm_domain *domains[];
> +};
> +
> +#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
> +
> +#define DOMAIN(pwr, status, req, idle, ack)    \
> +{                                              \
> +       .pwr_mask = BIT(pwr),                   \
> +       .status_mask = BIT(status),             \
> +       .req_mask = BIT(req),                   \
> +       .idle_mask = BIT(idle),                 \
> +       .ack_mask = BIT(ack),                   \
> +}
> +
> +#define DOMAIN_RK3288(pwr, status, req)                \
> +       DOMAIN(pwr, status, req, req, (req) + 16)
> +
> +static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       const struct rockchip_domain_info *pd_info = pd->info;
> +       unsigned int val;
> +
> +       regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
> +       return (val & pd_info->idle_mask) == pd_info->idle_mask;
> +}
> +
> +static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
> +                                        bool idle)
> +{
> +       const struct rockchip_domain_info *pd_info = pd->info;
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       unsigned int val;
> +
> +       regmap_update_bits(pmu->regmap, pmu->info->req_offset,
> +                          pd_info->req_mask, idle ? -1U : 0);
> +
> +       dsb();
> +
> +       do {
> +               regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
> +       } while ((val & pd_info->ack_mask) != (idle ? pd_info->ack_mask : 0));
> +
> +       while (rockchip_pmu_domain_is_idle(pd) != idle)
> +               cpu_relax();
> +
> +       return 0;
> +}
> +
> +static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +       unsigned int val;
> +
> +       regmap_read(pmu->regmap, pmu->info->status_offset, &val);
> +
> +       /* 1'b0: power on, 1'b1: power off */
> +       return !(val & pd->info->status_mask);
> +}
> +
> +static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
> +                                            bool on)
> +{
> +       struct rockchip_pmu *pmu = pd->pmu;
> +
> +       regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
> +                          pd->info->pwr_mask, on ? 0 : -1U);
> +
> +       dsb();
> +
> +       while (rockchip_pmu_domain_is_on(pd) != on)
> +               cpu_relax();
> +}
> +
> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
> +{
> +       int i;
> +
> +       mutex_lock(&pd->pmu->mutex);

I don't quite understand why you need a lock, what are you protecting and why?

> +
> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
> +               for (i = 0; i < pd->num_clks; i++)
> +                       clk_enable(pd->clks[i]);
> +
> +               if (!power_on) {
> +                       /* FIXME: add code to save AXI_QOS */
> +
> +                       /* if powering down, idle request to NIU first */
> +                       rockchip_pmu_set_idle_request(pd, true);
> +               }
> +
> +               rockchip_do_pmu_set_power_domain(pd, power_on);
> +
> +               if (power_on) {
> +                       /* if powering up, leave idle mode */
> +                       rockchip_pmu_set_idle_request(pd, false);
> +
> +                       /* FIXME: add code to restore AXI_QOS */
> +               }
> +
> +               for (i = pd->num_clks - 1; i >= 0; i--)
> +                       clk_disable(pd->clks[i]);
> +       }
> +
> +       mutex_unlock(&pd->pmu->mutex);
> +       return 0;
> +}
> +
> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
> +{
> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
> +
> +       return rockchip_pd_power(pd, false);
> +}
> +
> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
> +                                 struct device *dev)
> +{
> +       struct clk *clk;
> +       int i;
> +       int error;
> +
> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
> +
> +       error = pm_clk_create(dev);
> +       if (error) {
> +               dev_err(dev, "pm_clk_create failed %d\n", error);
> +               return error;
> +       }
> +
> +       i = 0;

> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {

This loop adds all available device clocks to the PM clock list. I
wonder if this could be considered as a common thing and if so, we
might want to extend the pm_clk API with this.

> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
> +                       __clk_get_name(clk));
> +               error = pm_clk_add_clk(dev, clk);
> +               clk_put(clk);
> +               if (error) {
> +                       dev_err(dev, "pm_clk_add_clk failed %d\n", error);
> +                       pm_clk_destroy(dev);
> +                       return error;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
> +                                  struct device *dev)
> +{
> +       dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name);
> +
> +       pm_clk_destroy(dev);
> +}
> +
> +static int rockchip_pd_start_dev(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> +       dev_dbg(dev, "starting device in power domain '%s'\n", genpd->name);
> +
> +       return pm_clk_resume(dev);
> +}
> +
> +static int rockchip_pd_stop_dev(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> +       dev_dbg(dev, "stopping device in power domain '%s'\n", genpd->name);
> +
> +       return pm_clk_suspend(dev);
> +}
> +
> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
> +                                     struct device_node *node)
> +{
> +       const struct rockchip_domain_info *pd_info;
> +       struct rockchip_pm_domain *pd;
> +       struct clk *clk;
> +       int clk_cnt;
> +       int i;
> +       u32 id;
> +       int error;
> +
> +       error = of_property_read_u32(node, "reg", &id);
> +       if (error) {
> +               dev_err(pmu->dev,
> +                       "%s: failed to retrieve domain id (reg): %d\n",
> +                       node->name, error);
> +               return -EINVAL;
> +       }
> +
> +       if (id >= pmu->info->num_domains) {
> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
> +                       node->name, id);
> +               return -EINVAL;
> +       }
> +
> +       pd_info = &pmu->info->domain_info[id];
> +       if (!pd_info) {
> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
> +                       node->name, id);
> +               return -EINVAL;
> +       }
> +
> +       clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells");
> +       pd = devm_kzalloc(pmu->dev,
> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
> +                         GFP_KERNEL);
> +       if (!pd)
> +               return -ENOMEM;
> +
> +       pd->info = pd_info;
> +       pd->pmu = pmu;
> +
> +       for (i = 0; i < clk_cnt; i++) {

This loop is similar to the one when creates the PM clock list in the
rockchip_pd_attach_dev().

What's the reason you don't want to use pm clk for these clocks?

> +               clk = of_clk_get(node, i);
> +               if (IS_ERR(clk)) {
> +                       error = PTR_ERR(clk);
> +                       dev_err(pmu->dev,
> +                               "%s: failed to get clk %s (index %d): %d\n",
> +                               node->name, __clk_get_name(clk), i, error);
> +                       goto err_out;
> +               }
> +
> +               error = clk_prepare(clk);
> +               if (error) {
> +                       dev_err(pmu->dev,
> +                               "%s: failed to prepare clk %s (index %d): %d\n",
> +                               node->name, __clk_get_name(clk), i, error);
> +                       clk_put(clk);
> +                       goto err_out;
> +               }
> +
> +               pd->clks[pd->num_clks++] = clk;
> +
> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
> +                       __clk_get_name(clk), node->name);
> +       }
> +
> +       error = rockchip_pd_power(pd, true);
> +       if (error) {
> +               dev_err(pmu->dev,
> +                       "failed to power on domain '%s': %d\n",
> +                       node->name, error);
> +               goto err_out;
> +       }
> +
> +       pd->genpd.name = node->name;
> +       pd->genpd.power_off = rockchip_pd_power_off;
> +       pd->genpd.power_on = rockchip_pd_power_on;
> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;

Instead of assigning the ->stop|start() callbacks, which do
pm_clk_suspend|resume(), just set the genpd->flags to
GENPD_FLAG_PM_CLK, and genpd will fix this for you.

> +       pm_genpd_init(&pd->genpd, NULL, false);
> +
> +       pmu->genpd_data.domains[id] = &pd->genpd;
> +       return 0;
> +
> +err_out:
> +       while (--i >= 0) {
> +               clk_unprepare(pd->clks[i]);
> +               clk_put(pd->clks[i]);
> +       }
> +       return error;
> +}
> +
> +static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
> +{
> +       int i;
> +
> +       for (i = 0; i < pd->num_clks; i++) {
> +               clk_unprepare(pd->clks[i]);

If you converted these clocks to be dealt with via the PM clk API,
pm_clk_destoy() would have replaced this loop.

> +               clk_put(pd->clks[i]);
> +       }
> +
> +       /* devm will free our memory */
> +}
> +
> +static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
> +{
> +       struct generic_pm_domain *genpd;
> +       struct rockchip_pm_domain *pd;
> +       int i;
> +
> +       for (i = 0; i < pmu->genpd_data.num_domains; i++) {
> +               genpd = pmu->genpd_data.domains[i];
> +               if (genpd) {
> +                       pd = to_rockchip_pd(genpd);
> +                       rockchip_pm_remove_one_domain(pd);
> +               }
> +       }
> +
> +       /* devm will free our memory */
> +}
> +
> +static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
> +                                     u32 domain_reg_offset,
> +                                     unsigned int count)
> +{
> +       /* First configure domain power down transition count ... */
> +       regmap_write(pmu->regmap, domain_reg_offset, count);
> +       /* ... and then power up count. */
> +       regmap_write(pmu->regmap, domain_reg_offset + 4, count);
> +}
> +
> +static int rockchip_pm_domain_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct device_node *node;
> +       struct rockchip_pmu *pmu;
> +       const struct of_device_id *match;
> +       const struct rockchip_pmu_info *pmu_info;
> +       int error;
> +
> +       if (!np) {
> +               dev_err(dev, "device tree node not found\n");
> +               return -ENXIO;/

Nitpick:
It's more common to return -ENODEV here, you might want to change.

> +       }
> +
> +       match = of_match_device(dev->driver->of_match_table, dev);
> +       if (!match || !match->data) {
> +               dev_err(dev, "missing pmu data\n");
> +               return -EINVAL;
> +       }
> +
> +       pmu_info = match->data;
> +
> +       pmu = devm_kzalloc(dev,
> +                          sizeof(*pmu) +
> +                               pmu_info->num_domains * sizeof(pmu->domains[0]),
> +                          GFP_KERNEL);
> +       if (!pmu)
> +               return -ENOMEM;
> +
> +       pmu->dev = &pdev->dev;
> +       mutex_init(&pmu->mutex);
> +
> +       pmu->info = pmu_info;
> +
> +       pmu->genpd_data.domains = pmu->domains;
> +       pmu->genpd_data.num_domains = pmu_info->num_domains;
> +
> +       node = of_parse_phandle(np, "rockchip,pmu", 0);
> +       pmu->regmap = syscon_node_to_regmap(node);
> +       of_node_put(node);
> +       if (IS_ERR(pmu->regmap)) {
> +               error = PTR_ERR(pmu->regmap);
> +               dev_err(dev, "failed to get PMU regmap: %d\n", error);
> +               return error;
> +       }
> +
> +       /*
> +        * Configure power up and down transition delays for core
> +        * and GPU domains.
> +        */
> +       rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
> +                                 pmu_info->core_power_transition_time);
> +       rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
> +                                 pmu_info->gpu_power_transition_time);
> +
> +       error = -ENXIO;
> +
> +       for_each_available_child_of_node(np, node) {
> +               error = rockchip_pm_add_one_domain(pmu, node);
> +               if (error) {
> +                       dev_err(dev, "failed to handle node %s: %d\n",
> +                               node->name, error);
> +                       goto err_out;
> +               }
> +       }
> +
> +       if (error) {
> +               dev_dbg(dev, "no power domains defined\n");
> +               goto err_out;
> +       }
> +
> +       of_genpd_add_provider_onecell(np, &pmu->genpd_data);
> +
> +       return 0;
> +
> +err_out:
> +       rockchip_pm_domain_cleanup(pmu);
> +       return error;
> +}
> +
> +static const struct rockchip_domain_info rk3288_pm_domains[] = {
> +       [RK3288_PD_GPU]         = DOMAIN_RK3288(9, 9, 2),
> +       [RK3288_PD_VIO]         = DOMAIN_RK3288(7, 7, 4),
> +       [RK3288_PD_VIDEO]       = DOMAIN_RK3288(8, 8, 3),
> +       [RK3288_PD_HEVC]        = DOMAIN_RK3288(14, 10, 9),
> +};
> +
> +static const struct rockchip_pmu_info rk3288_pmu = {
> +       .pwr_offset = 0x08,
> +       .status_offset = 0x0c,
> +       .req_offset = 0x10,
> +       .idle_offset = 0x14,
> +       .ack_offset = 0x14,
> +
> +       .core_pwrcnt_offset = 0x34,
> +       .gpu_pwrcnt_offset = 0x3c,
> +
> +       .core_power_transition_time = 24, /* 1us */
> +       .gpu_power_transition_time = 24, /* 1us */
> +
> +       .num_domains = ARRAY_SIZE(rk3288_pm_domains),
> +       .domain_info = rk3288_pm_domains,
> +};
> +
> +static const struct of_device_id rockchip_pm_domain_dt_match[] = {
> +       {
> +               .compatible = "rockchip,rk3288-power-controller",
> +               .data = (void *)&rk3288_pmu,
> +       },
> +       { /* sentinel */ },
> +};
> +
> +static struct platform_driver rockchip_pm_domain_driver = {
> +       .probe = rockchip_pm_domain_probe,
> +       .driver = {
> +               .name   = "rockchip-pm-domain",
> +               .of_match_table = rockchip_pm_domain_dt_match,
> +               /*
> +                * We can't forcibly eject devices form power domain,
> +                * so we can't really remove power domains once they
> +                * were added.
> +                */
> +               .suppress_bind_attrs = true,
> +       },
> +};
> +
> +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.9.1
>

Kind regards
Uffe

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

[Caesar Wang]

Hi Kevin, Heiko

Thanks for your comments.
Sorry for delay reply.

在 2015年04月28日 02:28, Kevin Hilman 写道:
> Heiko Stübner <heiko@sntech.de> writes:
>
>> Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang:
>>>      Add power domain drivers based on generic power domain for
>>> Rockchip platform, and support RK3288.
>>>
>>>      Verified on url =
>>>      https://chromium.googlesource.com/chromiumos/third_party/kernel.
>>>
>>>      At the moment,there are mass of products are using the driver.
>>> I believe the driver can happy work for next kernel.
>> I've taken a look at the driver and here are some global remarks:
>>
>> (1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks
>> bisectability, as the driver itself in patch 2 also includes the header and
>> would thus fail to compile if the later patch 3 is missing.
>> Ideally I think the header addition should be a separate patch itself, so that
>> we can possibly share it between driver and dts branches.
>> So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes.
OK, done.

>>
>> (2) The dts-changes in patch 3 should also add any necessary power-domain
>> assignment on devices if they're still missing, so that we don't introduce
>> regressions. In my case my work-in-progress edp died because the powerdomain
>> was turned off automatically it seems.
OK, I will list that devices.
At the moment, I don't find the EDP driver for rockchip. (I think the 
EDP driver hasn't a upstream).

Anyway, I will test it on 
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14,
Meanwhile work on next-kernel.


>>
>> (3) more like wondering @Kevin or so, is there some more generic place for a
>> power-domain driver nowadays?
> I think the preference has been to put these under drivers/soc/<vendor> for now,
> so they can shared across arm32 and arm64.
>

Interesting. Do you want to put the domain driver into /driver/soc/rockchip?
I guess the efuse driver ...is also do that.

Perhaps, it's a good select in the future.


>> (4) As Tomasz remarked previously the dts should represent the hardware and
>> the power-domains are part of the pmu. There is a recent addition from Linus
>> Walleij, called simple-mfd [a] that is supposed to get added real early for
>> kernel 4.2. So I'd think the power-domains should use that and the patchset
>> modified to include the changes shown below [b]?
>>
>> (5) Keven Hilman and Tomasz had reservations about all the device clocks
>> being listed in the power-domains itself in the previous versions. I don't see
>> a comment from them yet changing that view.
> Correct.

How about this patch?

https://patchwork.kernel.org/patch/5145241/

I will do that.

Maybe, do you have more suggestions?


>> Their wish was to get the clocks by reading the clocks from the device nodes,
>> though I see a problem on how to handle devices that do not have any bindings
>> at all yet.
>>
>> Kevin, Tomasz any new thoughts?
> I don't see any issues with devices that don't have bindings, as all
> that would be needed would be to simple device nodes with a clock
> property.  I wouldn't even matter if those devices had device drivers.
>
> Kevin
>
>
>

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

[Caesar Wang]

Hi Kevin, Heiko

Thanks for your comments.
Sorry for delay reply.

? 2015?04?28? 02:28, Kevin Hilman ??:
> Heiko St?bner <heiko@sntech.de> writes:
>
>> Am Freitag, 24. April 2015, 16:07:45 schrieb Caesar Wang:
>>>      Add power domain drivers based on generic power domain for
>>> Rockchip platform, and support RK3288.
>>>
>>>      Verified on url =
>>>      https://chromium.googlesource.com/chromiumos/third_party/kernel.
>>>
>>>      At the moment,there are mass of products are using the driver.
>>> I believe the driver can happy work for next kernel.
>> I've taken a look at the driver and here are some global remarks:
>>
>> (1) You provide dt-bindings/power-domain/rk3288.h in patch 3. This breaks
>> bisectability, as the driver itself in patch 2 also includes the header and
>> would thus fail to compile if the later patch 3 is missing.
>> Ideally I think the header addition should be a separate patch itself, so that
>> we can possibly share it between driver and dts branches.
>> So 1: binding doc, 2: binding-header, 3: driver, 4: dts-changes.
OK, done.

>>
>> (2) The dts-changes in patch 3 should also add any necessary power-domain
>> assignment on devices if they're still missing, so that we don't introduce
>> regressions. In my case my work-in-progress edp died because the powerdomain
>> was turned off automatically it seems.
OK, I will list that devices.
At the moment, I don't find the EDP driver for rockchip. (I think the 
EDP driver hasn't a upstream).

Anyway, I will test it on 
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14,
Meanwhile work on next-kernel.


>>
>> (3) more like wondering @Kevin or so, is there some more generic place for a
>> power-domain driver nowadays?
> I think the preference has been to put these under drivers/soc/<vendor> for now,
> so they can shared across arm32 and arm64.
>

Interesting. Do you want to put the domain driver into /driver/soc/rockchip?
I guess the efuse driver ...is also do that.

Perhaps, it's a good select in the future.


>> (4) As Tomasz remarked previously the dts should represent the hardware and
>> the power-domains are part of the pmu. There is a recent addition from Linus
>> Walleij, called simple-mfd [a] that is supposed to get added real early for
>> kernel 4.2. So I'd think the power-domains should use that and the patchset
>> modified to include the changes shown below [b]?
>>
>> (5) Keven Hilman and Tomasz had reservations about all the device clocks
>> being listed in the power-domains itself in the previous versions. I don't see
>> a comment from them yet changing that view.
> Correct.

How about this patch?

https://patchwork.kernel.org/patch/5145241/

I will do that.

Maybe, do you have more suggestions?


>> Their wish was to get the clocks by reading the clocks from the device nodes,
>> though I see a problem on how to handle devices that do not have any bindings
>> at all yet.
>>
>> Kevin, Tomasz any new thoughts?
> I don't see any issues with devices that don't have bindings, as all
> that would be needed would be to simple device nodes with a clock
> property.  I wouldn't even matter if those devices had device drivers.
>
> Kevin
>
>
>

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Caesar Wang]

Ulf,

Thanks for your comments.:-)



在 2015年05月28日 18:38, Ulf Hansson 写道:
> On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
>> In order to meet high performance and low power requirements, a power
>> management unit is designed or saving power when RK3288 in low power
>> mode.
>> The RK3288 PMU is dedicated for managing the power ot the whole chip.
>>
>> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
>>
>>   arch/arm/mach-rockchip/Kconfig      |   1 +
>>   arch/arm/mach-rockchip/Makefile     |   1 +
>>   arch/arm/mach-rockchip/pm_domains.c | 506 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 508 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 ae4eb7c..578206b 100644
>> --- a/arch/arm/mach-rockchip/Kconfig
>> +++ b/arch/arm/mach-rockchip/Kconfig
>> @@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
>>          select ROCKCHIP_TIMER
>>          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 5c3a9b2..9c902d3 100644
>> --- a/arch/arm/mach-rockchip/Makefile
>> +++ b/arch/arm/mach-rockchip/Makefile
>> @@ -1,5 +1,6 @@
>>   CFLAGS_platsmp.o := -march=armv7-a
>>
>>   obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
>> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
>>   obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
>>   obj-$(CONFIG_SMP) += headsmp.o platsmp.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..92d2569
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/pm_domains.c
>> @@ -0,0 +1,506 @@
>> +/*
>> + * Rockchip Generic power domain support.
>> + *
>> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
>> + *
>> + * 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/io.h>
>> +#include <linux/err.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
> clk-provider.h, why?

The following is needed.

_clk_get_name(clk)


>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <dt-bindings/power-domain/rk3288.h>
> Same comment as in patch1. I don't find this header.

OK.
Perhaps, Making a mistake. the missing the patch in 
here.(https://patchwork.kernel.org/patch/6266881/)
>> +
>> +struct rockchip_domain_info {
>> +       int pwr_mask;
>> +       int status_mask;
>> +       int req_mask;
>> +       int idle_mask;
>> +       int ack_mask;
>> +};
>> +
>> +struct rockchip_pmu_info {
>> +       u32 pwr_offset;
>> +       u32 status_offset;
>> +       u32 req_offset;
>> +       u32 idle_offset;
>> +       u32 ack_offset;
>> +
>> +       u32 core_pwrcnt_offset;
>> +       u32 gpu_pwrcnt_offset;
>> +
>> +       unsigned int core_power_transition_time;
>> +       unsigned int gpu_power_transition_time;
>> +
>> +       int num_domains;
>> +       const struct rockchip_domain_info *domain_info;
>> +};
>> +
>> +struct rockchip_pm_domain {
>> +       struct generic_pm_domain genpd;
>> +       const struct rockchip_domain_info *info;
>> +       struct rockchip_pmu *pmu;
>> +       int num_clks;
>> +       struct clk *clks[];
>> +};
>> +
>> +struct rockchip_pmu {
>> +       struct device *dev;
>> +       struct regmap *regmap;
>> +       const struct rockchip_pmu_info *info;
>> +       struct mutex mutex; /* mutex lock for pmu */
>> +       struct genpd_onecell_data genpd_data;
>> +       struct generic_pm_domain *domains[];
>> +};
>> +
>> +#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
>> +
>> +#define DOMAIN(pwr, status, req, idle, ack)    \
>> +{                                              \
>> +       .pwr_mask = BIT(pwr),                   \
>> +       .status_mask = BIT(status),             \
>> +       .req_mask = BIT(req),                   \
>> +       .idle_mask = BIT(idle),                 \
>> +       .ack_mask = BIT(ack),                   \
>> +}
>> +
>> +#define DOMAIN_RK3288(pwr, status, req)                \
>> +       DOMAIN(pwr, status, req, req, (req) + 16)
>> +
>> +static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       const struct rockchip_domain_info *pd_info = pd->info;
>> +       unsigned int val;
>> +
>> +       regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
>> +       return (val & pd_info->idle_mask) == pd_info->idle_mask;
>> +}
>> +
>> +static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
>> +                                        bool idle)
>> +{
>> +       const struct rockchip_domain_info *pd_info = pd->info;
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       unsigned int val;
>> +
>> +       regmap_update_bits(pmu->regmap, pmu->info->req_offset,
>> +                          pd_info->req_mask, idle ? -1U : 0);
>> +
>> +       dsb();
>> +
>> +       do {
>> +               regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
>> +       } while ((val & pd_info->ack_mask) != (idle ? pd_info->ack_mask : 0));
>> +
>> +       while (rockchip_pmu_domain_is_idle(pd) != idle)
>> +               cpu_relax();
>> +
>> +       return 0;
>> +}
>> +
>> +static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       unsigned int val;
>> +
>> +       regmap_read(pmu->regmap, pmu->info->status_offset, &val);
>> +
>> +       /* 1'b0: power on, 1'b1: power off */
>> +       return !(val & pd->info->status_mask);
>> +}
>> +
>> +static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>> +                                            bool on)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +
>> +       regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
>> +                          pd->info->pwr_mask, on ? 0 : -1U);
>> +
>> +       dsb();
>> +
>> +       while (rockchip_pmu_domain_is_on(pd) != on)
>> +               cpu_relax();
>> +}
>> +
>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>> +{
>> +       int i;
>> +
>> +       mutex_lock(&pd->pmu->mutex);
> I don't quite understand why you need a lock, what are you protecting and why?

Says:
[ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3551.919730] rockchip_pd_power:139: mutex_lock
[ 3551.924130] rockchip_pd_power:167,mutex_unlock
[ 3563.827295] rockchip_pd_power:139: mutex_lock
[ 3563.831753] rockchip_pd_power:167,mutex_unlock
[ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3564.079047] rockchip_pd_power:139: mutex_lock
[ 3564.083426] rockchip_pd_power:167,mutex_unlock
[ 3611.665726] rockchip_pd_power:139: mutex_lock
[ 3611.670206] rockchip_pd_power:167,mutex_unlock
[ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3611.919689] rockchip_pd_power:139: mutex_lock
[ 3611.924090] rockchip_pd_power:167,mutex_unlock
[ 3623.848296] rockchip_pd_power:139: mutex_lock
[ 3623.852752] rockchip_pd_power:167,mutex_unlock



>> +
>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>> +               for (i = 0; i < pd->num_clks; i++)
>> +                       clk_enable(pd->clks[i]);
>> +
>> +               if (!power_on) {
>> +                       /* FIXME: add code to save AXI_QOS */
>> +
>> +                       /* if powering down, idle request to NIU first */
>> +                       rockchip_pmu_set_idle_request(pd, true);
>> +               }
>> +
>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>> +
>> +               if (power_on) {
>> +                       /* if powering up, leave idle mode */
>> +                       rockchip_pmu_set_idle_request(pd, false);
>> +
>> +                       /* FIXME: add code to restore AXI_QOS */
>> +               }
>> +
>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>> +                       clk_disable(pd->clks[i]);
>> +       }
>> +
>> +       mutex_unlock(&pd->pmu->mutex);
>> +       return 0;
>> +}
>> +
>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>> +{
>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>> +
>> +       return rockchip_pd_power(pd, false);
>> +}
>> +
>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>> +                                 struct device *dev)
>> +{
>> +       struct clk *clk;
>> +       int i;
>> +       int error;
>> +
>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>> +
>> +       error = pm_clk_create(dev);
>> +       if (error) {
>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>> +               return error;
>> +       }
>> +
>> +       i = 0;
>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
> This loop adds all available device clocks to the PM clock list. I
> wonder if this could be considered as a common thing and if so, we
> might want to extend the pm_clk API with this.

There are several reasons as follows:

     Firstly, the clocks need be turned off to save power when
     the system enter the suspend state. So we need to enumerate the clocks
     in the dts. In order to power domain can turn on and off.

     Secondly, the reset-circuit should reset be synchronous on rk3288, 
then sync revoked.
     So we need to enable clocks of all devices.
>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>> +                       __clk_get_name(clk));
>> +               error = pm_clk_add_clk(dev, clk);
>> +               clk_put(clk);
>> +               if (error) {
>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n", error);
>> +                       pm_clk_destroy(dev);
>> +                       return error;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
>> +                                  struct device *dev)
>> +{
>> +       dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name);
>> +
>> +       pm_clk_destroy(dev);
>> +}
>> +
>> +static int rockchip_pd_start_dev(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +
>> +       dev_dbg(dev, "starting device in power domain '%s'\n", genpd->name);
>> +
>> +       return pm_clk_resume(dev);
>> +}
>> +
>> +static int rockchip_pd_stop_dev(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +
>> +       dev_dbg(dev, "stopping device in power domain '%s'\n", genpd->name);
>> +
>> +       return pm_clk_suspend(dev);
>> +}
>> +
>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>> +                                     struct device_node *node)
>> +{
>> +       const struct rockchip_domain_info *pd_info;
>> +       struct rockchip_pm_domain *pd;
>> +       struct clk *clk;
>> +       int clk_cnt;
>> +       int i;
>> +       u32 id;
>> +       int error;
>> +
>> +       error = of_property_read_u32(node, "reg", &id);
>> +       if (error) {
>> +               dev_err(pmu->dev,
>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>> +                       node->name, error);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (id >= pmu->info->num_domains) {
>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>> +                       node->name, id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       pd_info = &pmu->info->domain_info[id];
>> +       if (!pd_info) {
>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>> +                       node->name, id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells");
>> +       pd = devm_kzalloc(pmu->dev,
>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>> +                         GFP_KERNEL);
>> +       if (!pd)
>> +               return -ENOMEM;
>> +
>> +       pd->info = pd_info;
>> +       pd->pmu = pmu;
>> +
>> +       for (i = 0; i < clk_cnt; i++) {
> This loop is similar to the one when creates the PM clock list in the
> rockchip_pd_attach_dev().
>
> What's the reason you don't want to use pm clk for these clocks?
>

Ditto.
>> +               clk = of_clk_get(node, i);
>> +               if (IS_ERR(clk)) {
>> +                       error = PTR_ERR(clk);
>> +                       dev_err(pmu->dev,
>> +                               "%s: failed to get clk %s (index %d): %d\n",
>> +                               node->name, __clk_get_name(clk), i, error);
>> +                       goto err_out;
>> +               }
>> +
>> +               error = clk_prepare(clk);
>> +               if (error) {
>> +                       dev_err(pmu->dev,
>> +                               "%s: failed to prepare clk %s (index %d): %d\n",
>> +                               node->name, __clk_get_name(clk), i, error);
>> +                       clk_put(clk);
>> +                       goto err_out;
>> +               }
>> +
>> +               pd->clks[pd->num_clks++] = clk;
>> +
>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>> +                       __clk_get_name(clk), node->name);
>> +       }
>> +
>> +       error = rockchip_pd_power(pd, true);
>> +       if (error) {
>> +               dev_err(pmu->dev,
>> +                       "failed to power on domain '%s': %d\n",
>> +                       node->name, error);
>> +               goto err_out;
>> +       }
>> +
>> +       pd->genpd.name = node->name;
>> +       pd->genpd.power_off = rockchip_pd_power_off;
>> +       pd->genpd.power_on = rockchip_pd_power_on;
>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
> Instead of assigning the ->stop|start() callbacks, which do
> pm_clk_suspend|resume(), just set the genpd->flags to
> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>
Ditto.

>> +       pm_genpd_init(&pd->genpd, NULL, false);
>> +
>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>> +       return 0;
>> +
>> +err_out:
>> +       while (--i >= 0) {
>> +               clk_unprepare(pd->clks[i]);
>> +               clk_put(pd->clks[i]);
>> +       }
>> +       return error;
>> +}
>> +
>> +static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < pd->num_clks; i++) {
>> +               clk_unprepare(pd->clks[i]);
> If you converted these clocks to be dealt with via the PM clk API,
> pm_clk_destoy() would have replaced this loop.
>
>> +               clk_put(pd->clks[i]);
>> +       }
>> +
>> +       /* devm will free our memory */
>> +}
>> +
>> +static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
>> +{
>> +       struct generic_pm_domain *genpd;
>> +       struct rockchip_pm_domain *pd;
>> +       int i;
>> +
>> +       for (i = 0; i < pmu->genpd_data.num_domains; i++) {
>> +               genpd = pmu->genpd_data.domains[i];
>> +               if (genpd) {
>> +                       pd = to_rockchip_pd(genpd);
>> +                       rockchip_pm_remove_one_domain(pd);
>> +               }
>> +       }
>> +
>> +       /* devm will free our memory */
>> +}
>> +
>> +static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
>> +                                     u32 domain_reg_offset,
>> +                                     unsigned int count)
>> +{
>> +       /* First configure domain power down transition count ... */
>> +       regmap_write(pmu->regmap, domain_reg_offset, count);
>> +       /* ... and then power up count. */
>> +       regmap_write(pmu->regmap, domain_reg_offset + 4, count);
>> +}
>> +
>> +static int rockchip_pm_domain_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct device_node *node;
>> +       struct rockchip_pmu *pmu;
>> +       const struct of_device_id *match;
>> +       const struct rockchip_pmu_info *pmu_info;
>> +       int error;
>> +
>> +       if (!np) {
>> +               dev_err(dev, "device tree node not found\n");
>> +               return -ENXIO;/
> Nitpick:
> It's more common to return -ENODEV here, you might want to change.

OK.

Thanks,
Caesar
>> +       }
>> +
>> +       match = of_match_device(dev->driver->of_match_table, dev);
>> +       if (!match || !match->data) {
>> +               dev_err(dev, "missing pmu data\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       pmu_info = match->data;
>> +
>> +       pmu = devm_kzalloc(dev,
>> +                          sizeof(*pmu) +
>> +                               pmu_info->num_domains * sizeof(pmu->domains[0]),
>> +                          GFP_KERNEL);
>> +       if (!pmu)
>> +               return -ENOMEM;
>> +
>> +       pmu->dev = &pdev->dev;
>> +       mutex_init(&pmu->mutex);
>> +
>> +       pmu->info = pmu_info;
>> +
>> +       pmu->genpd_data.domains = pmu->domains;
>> +       pmu->genpd_data.num_domains = pmu_info->num_domains;
>> +
>> +       node = of_parse_phandle(np, "rockchip,pmu", 0);
>> +       pmu->regmap = syscon_node_to_regmap(node);
>> +       of_node_put(node);
>> +       if (IS_ERR(pmu->regmap)) {
>> +               error = PTR_ERR(pmu->regmap);
>> +               dev_err(dev, "failed to get PMU regmap: %d\n", error);
>> +               return error;
>> +       }
>> +
>> +       /*
>> +        * Configure power up and down transition delays for core
>> +        * and GPU domains.
>> +        */
>> +       rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
>> +                                 pmu_info->core_power_transition_time);
>> +       rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
>> +                                 pmu_info->gpu_power_transition_time);
>> +
>> +       error = -ENXIO;
>> +
>> +       for_each_available_child_of_node(np, node) {
>> +               error = rockchip_pm_add_one_domain(pmu, node);
>> +               if (error) {
>> +                       dev_err(dev, "failed to handle node %s: %d\n",
>> +                               node->name, error);
>> +                       goto err_out;
>> +               }
>> +       }
>> +
>> +       if (error) {
>> +               dev_dbg(dev, "no power domains defined\n");
>> +               goto err_out;
>> +       }
>> +
>> +       of_genpd_add_provider_onecell(np, &pmu->genpd_data);
>> +
>> +       return 0;
>> +
>> +err_out:
>> +       rockchip_pm_domain_cleanup(pmu);
>> +       return error;
>> +}
>> +
>> +static const struct rockchip_domain_info rk3288_pm_domains[] = {
>> +       [RK3288_PD_GPU]         = DOMAIN_RK3288(9, 9, 2),
>> +       [RK3288_PD_VIO]         = DOMAIN_RK3288(7, 7, 4),
>> +       [RK3288_PD_VIDEO]       = DOMAIN_RK3288(8, 8, 3),
>> +       [RK3288_PD_HEVC]        = DOMAIN_RK3288(14, 10, 9),
>> +};
>> +
>> +static const struct rockchip_pmu_info rk3288_pmu = {
>> +       .pwr_offset = 0x08,
>> +       .status_offset = 0x0c,
>> +       .req_offset = 0x10,
>> +       .idle_offset = 0x14,
>> +       .ack_offset = 0x14,
>> +
>> +       .core_pwrcnt_offset = 0x34,
>> +       .gpu_pwrcnt_offset = 0x3c,
>> +
>> +       .core_power_transition_time = 24, /* 1us */
>> +       .gpu_power_transition_time = 24, /* 1us */
>> +
>> +       .num_domains = ARRAY_SIZE(rk3288_pm_domains),
>> +       .domain_info = rk3288_pm_domains,
>> +};
>> +
>> +static const struct of_device_id rockchip_pm_domain_dt_match[] = {
>> +       {
>> +               .compatible = "rockchip,rk3288-power-controller",
>> +               .data = (void *)&rk3288_pmu,
>> +       },
>> +       { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver rockchip_pm_domain_driver = {
>> +       .probe = rockchip_pm_domain_probe,
>> +       .driver = {
>> +               .name   = "rockchip-pm-domain",
>> +               .of_match_table = rockchip_pm_domain_dt_match,
>> +               /*
>> +                * We can't forcibly eject devices form power domain,
>> +                * so we can't really remove power domains once they
>> +                * were added.
>> +                */
>> +               .suppress_bind_attrs = true,
>> +       },
>> +};
>> +
>> +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.9.1
>>
> Kind regards
> Uffe
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Caesar Wang]

Ulf,

Thanks for your comments.:-)



在 2015年05月28日 18:38, Ulf Hansson 写道:
> On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
>> In order to meet high performance and low power requirements, a power
>> management unit is designed or saving power when RK3288 in low power
>> mode.
>> The RK3288 PMU is dedicated for managing the power ot the whole chip.
>>
>> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
>>
>>   arch/arm/mach-rockchip/Kconfig      |   1 +
>>   arch/arm/mach-rockchip/Makefile     |   1 +
>>   arch/arm/mach-rockchip/pm_domains.c | 506 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 508 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 ae4eb7c..578206b 100644
>> --- a/arch/arm/mach-rockchip/Kconfig
>> +++ b/arch/arm/mach-rockchip/Kconfig
>> @@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
>>          select ROCKCHIP_TIMER
>>          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 5c3a9b2..9c902d3 100644
>> --- a/arch/arm/mach-rockchip/Makefile
>> +++ b/arch/arm/mach-rockchip/Makefile
>> @@ -1,5 +1,6 @@
>>   CFLAGS_platsmp.o := -march=armv7-a
>>
>>   obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
>> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
>>   obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
>>   obj-$(CONFIG_SMP) += headsmp.o platsmp.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..92d2569
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/pm_domains.c
>> @@ -0,0 +1,506 @@
>> +/*
>> + * Rockchip Generic power domain support.
>> + *
>> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
>> + *
>> + * 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/io.h>
>> +#include <linux/err.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
> clk-provider.h, why?

The following is needed.

_clk_get_name(clk)


>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <dt-bindings/power-domain/rk3288.h>
> Same comment as in patch1. I don't find this header.

OK.
Perhaps, Making a mistake. the missing the patch in 
here.(https://patchwork.kernel.org/patch/6266881/)
>> +
>> +struct rockchip_domain_info {
>> +       int pwr_mask;
>> +       int status_mask;
>> +       int req_mask;
>> +       int idle_mask;
>> +       int ack_mask;
>> +};
>> +
>> +struct rockchip_pmu_info {
>> +       u32 pwr_offset;
>> +       u32 status_offset;
>> +       u32 req_offset;
>> +       u32 idle_offset;
>> +       u32 ack_offset;
>> +
>> +       u32 core_pwrcnt_offset;
>> +       u32 gpu_pwrcnt_offset;
>> +
>> +       unsigned int core_power_transition_time;
>> +       unsigned int gpu_power_transition_time;
>> +
>> +       int num_domains;
>> +       const struct rockchip_domain_info *domain_info;
>> +};
>> +
>> +struct rockchip_pm_domain {
>> +       struct generic_pm_domain genpd;
>> +       const struct rockchip_domain_info *info;
>> +       struct rockchip_pmu *pmu;
>> +       int num_clks;
>> +       struct clk *clks[];
>> +};
>> +
>> +struct rockchip_pmu {
>> +       struct device *dev;
>> +       struct regmap *regmap;
>> +       const struct rockchip_pmu_info *info;
>> +       struct mutex mutex; /* mutex lock for pmu */
>> +       struct genpd_onecell_data genpd_data;
>> +       struct generic_pm_domain *domains[];
>> +};
>> +
>> +#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
>> +
>> +#define DOMAIN(pwr, status, req, idle, ack)    \
>> +{                                              \
>> +       .pwr_mask = BIT(pwr),                   \
>> +       .status_mask = BIT(status),             \
>> +       .req_mask = BIT(req),                   \
>> +       .idle_mask = BIT(idle),                 \
>> +       .ack_mask = BIT(ack),                   \
>> +}
>> +
>> +#define DOMAIN_RK3288(pwr, status, req)                \
>> +       DOMAIN(pwr, status, req, req, (req) + 16)
>> +
>> +static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       const struct rockchip_domain_info *pd_info = pd->info;
>> +       unsigned int val;
>> +
>> +       regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
>> +       return (val & pd_info->idle_mask) == pd_info->idle_mask;
>> +}
>> +
>> +static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
>> +                                        bool idle)
>> +{
>> +       const struct rockchip_domain_info *pd_info = pd->info;
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       unsigned int val;
>> +
>> +       regmap_update_bits(pmu->regmap, pmu->info->req_offset,
>> +                          pd_info->req_mask, idle ? -1U : 0);
>> +
>> +       dsb();
>> +
>> +       do {
>> +               regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
>> +       } while ((val & pd_info->ack_mask) != (idle ? pd_info->ack_mask : 0));
>> +
>> +       while (rockchip_pmu_domain_is_idle(pd) != idle)
>> +               cpu_relax();
>> +
>> +       return 0;
>> +}
>> +
>> +static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       unsigned int val;
>> +
>> +       regmap_read(pmu->regmap, pmu->info->status_offset, &val);
>> +
>> +       /* 1'b0: power on, 1'b1: power off */
>> +       return !(val & pd->info->status_mask);
>> +}
>> +
>> +static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>> +                                            bool on)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +
>> +       regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
>> +                          pd->info->pwr_mask, on ? 0 : -1U);
>> +
>> +       dsb();
>> +
>> +       while (rockchip_pmu_domain_is_on(pd) != on)
>> +               cpu_relax();
>> +}
>> +
>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>> +{
>> +       int i;
>> +
>> +       mutex_lock(&pd->pmu->mutex);
> I don't quite understand why you need a lock, what are you protecting and why?

Says:
[ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3551.919730] rockchip_pd_power:139: mutex_lock
[ 3551.924130] rockchip_pd_power:167,mutex_unlock
[ 3563.827295] rockchip_pd_power:139: mutex_lock
[ 3563.831753] rockchip_pd_power:167,mutex_unlock
[ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3564.079047] rockchip_pd_power:139: mutex_lock
[ 3564.083426] rockchip_pd_power:167,mutex_unlock
[ 3611.665726] rockchip_pd_power:139: mutex_lock
[ 3611.670206] rockchip_pd_power:167,mutex_unlock
[ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3611.919689] rockchip_pd_power:139: mutex_lock
[ 3611.924090] rockchip_pd_power:167,mutex_unlock
[ 3623.848296] rockchip_pd_power:139: mutex_lock
[ 3623.852752] rockchip_pd_power:167,mutex_unlock



>> +
>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>> +               for (i = 0; i < pd->num_clks; i++)
>> +                       clk_enable(pd->clks[i]);
>> +
>> +               if (!power_on) {
>> +                       /* FIXME: add code to save AXI_QOS */
>> +
>> +                       /* if powering down, idle request to NIU first */
>> +                       rockchip_pmu_set_idle_request(pd, true);
>> +               }
>> +
>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>> +
>> +               if (power_on) {
>> +                       /* if powering up, leave idle mode */
>> +                       rockchip_pmu_set_idle_request(pd, false);
>> +
>> +                       /* FIXME: add code to restore AXI_QOS */
>> +               }
>> +
>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>> +                       clk_disable(pd->clks[i]);
>> +       }
>> +
>> +       mutex_unlock(&pd->pmu->mutex);
>> +       return 0;
>> +}
>> +
>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>> +{
>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>> +
>> +       return rockchip_pd_power(pd, false);
>> +}
>> +
>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>> +                                 struct device *dev)
>> +{
>> +       struct clk *clk;
>> +       int i;
>> +       int error;
>> +
>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>> +
>> +       error = pm_clk_create(dev);
>> +       if (error) {
>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>> +               return error;
>> +       }
>> +
>> +       i = 0;
>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
> This loop adds all available device clocks to the PM clock list. I
> wonder if this could be considered as a common thing and if so, we
> might want to extend the pm_clk API with this.

There are several reasons as follows:

     Firstly, the clocks need be turned off to save power when
     the system enter the suspend state. So we need to enumerate the clocks
     in the dts. In order to power domain can turn on and off.

     Secondly, the reset-circuit should reset be synchronous on rk3288, 
then sync revoked.
     So we need to enable clocks of all devices.
>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>> +                       __clk_get_name(clk));
>> +               error = pm_clk_add_clk(dev, clk);
>> +               clk_put(clk);
>> +               if (error) {
>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n", error);
>> +                       pm_clk_destroy(dev);
>> +                       return error;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
>> +                                  struct device *dev)
>> +{
>> +       dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name);
>> +
>> +       pm_clk_destroy(dev);
>> +}
>> +
>> +static int rockchip_pd_start_dev(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +
>> +       dev_dbg(dev, "starting device in power domain '%s'\n", genpd->name);
>> +
>> +       return pm_clk_resume(dev);
>> +}
>> +
>> +static int rockchip_pd_stop_dev(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +
>> +       dev_dbg(dev, "stopping device in power domain '%s'\n", genpd->name);
>> +
>> +       return pm_clk_suspend(dev);
>> +}
>> +
>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>> +                                     struct device_node *node)
>> +{
>> +       const struct rockchip_domain_info *pd_info;
>> +       struct rockchip_pm_domain *pd;
>> +       struct clk *clk;
>> +       int clk_cnt;
>> +       int i;
>> +       u32 id;
>> +       int error;
>> +
>> +       error = of_property_read_u32(node, "reg", &id);
>> +       if (error) {
>> +               dev_err(pmu->dev,
>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>> +                       node->name, error);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (id >= pmu->info->num_domains) {
>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>> +                       node->name, id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       pd_info = &pmu->info->domain_info[id];
>> +       if (!pd_info) {
>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>> +                       node->name, id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells");
>> +       pd = devm_kzalloc(pmu->dev,
>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>> +                         GFP_KERNEL);
>> +       if (!pd)
>> +               return -ENOMEM;
>> +
>> +       pd->info = pd_info;
>> +       pd->pmu = pmu;
>> +
>> +       for (i = 0; i < clk_cnt; i++) {
> This loop is similar to the one when creates the PM clock list in the
> rockchip_pd_attach_dev().
>
> What's the reason you don't want to use pm clk for these clocks?
>

Ditto.
>> +               clk = of_clk_get(node, i);
>> +               if (IS_ERR(clk)) {
>> +                       error = PTR_ERR(clk);
>> +                       dev_err(pmu->dev,
>> +                               "%s: failed to get clk %s (index %d): %d\n",
>> +                               node->name, __clk_get_name(clk), i, error);
>> +                       goto err_out;
>> +               }
>> +
>> +               error = clk_prepare(clk);
>> +               if (error) {
>> +                       dev_err(pmu->dev,
>> +                               "%s: failed to prepare clk %s (index %d): %d\n",
>> +                               node->name, __clk_get_name(clk), i, error);
>> +                       clk_put(clk);
>> +                       goto err_out;
>> +               }
>> +
>> +               pd->clks[pd->num_clks++] = clk;
>> +
>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>> +                       __clk_get_name(clk), node->name);
>> +       }
>> +
>> +       error = rockchip_pd_power(pd, true);
>> +       if (error) {
>> +               dev_err(pmu->dev,
>> +                       "failed to power on domain '%s': %d\n",
>> +                       node->name, error);
>> +               goto err_out;
>> +       }
>> +
>> +       pd->genpd.name = node->name;
>> +       pd->genpd.power_off = rockchip_pd_power_off;
>> +       pd->genpd.power_on = rockchip_pd_power_on;
>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
> Instead of assigning the ->stop|start() callbacks, which do
> pm_clk_suspend|resume(), just set the genpd->flags to
> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>
Ditto.

>> +       pm_genpd_init(&pd->genpd, NULL, false);
>> +
>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>> +       return 0;
>> +
>> +err_out:
>> +       while (--i >= 0) {
>> +               clk_unprepare(pd->clks[i]);
>> +               clk_put(pd->clks[i]);
>> +       }
>> +       return error;
>> +}
>> +
>> +static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < pd->num_clks; i++) {
>> +               clk_unprepare(pd->clks[i]);
> If you converted these clocks to be dealt with via the PM clk API,
> pm_clk_destoy() would have replaced this loop.
>
>> +               clk_put(pd->clks[i]);
>> +       }
>> +
>> +       /* devm will free our memory */
>> +}
>> +
>> +static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
>> +{
>> +       struct generic_pm_domain *genpd;
>> +       struct rockchip_pm_domain *pd;
>> +       int i;
>> +
>> +       for (i = 0; i < pmu->genpd_data.num_domains; i++) {
>> +               genpd = pmu->genpd_data.domains[i];
>> +               if (genpd) {
>> +                       pd = to_rockchip_pd(genpd);
>> +                       rockchip_pm_remove_one_domain(pd);
>> +               }
>> +       }
>> +
>> +       /* devm will free our memory */
>> +}
>> +
>> +static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
>> +                                     u32 domain_reg_offset,
>> +                                     unsigned int count)
>> +{
>> +       /* First configure domain power down transition count ... */
>> +       regmap_write(pmu->regmap, domain_reg_offset, count);
>> +       /* ... and then power up count. */
>> +       regmap_write(pmu->regmap, domain_reg_offset + 4, count);
>> +}
>> +
>> +static int rockchip_pm_domain_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct device_node *node;
>> +       struct rockchip_pmu *pmu;
>> +       const struct of_device_id *match;
>> +       const struct rockchip_pmu_info *pmu_info;
>> +       int error;
>> +
>> +       if (!np) {
>> +               dev_err(dev, "device tree node not found\n");
>> +               return -ENXIO;/
> Nitpick:
> It's more common to return -ENODEV here, you might want to change.

OK.

Thanks,
Caesar
>> +       }
>> +
>> +       match = of_match_device(dev->driver->of_match_table, dev);
>> +       if (!match || !match->data) {
>> +               dev_err(dev, "missing pmu data\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       pmu_info = match->data;
>> +
>> +       pmu = devm_kzalloc(dev,
>> +                          sizeof(*pmu) +
>> +                               pmu_info->num_domains * sizeof(pmu->domains[0]),
>> +                          GFP_KERNEL);
>> +       if (!pmu)
>> +               return -ENOMEM;
>> +
>> +       pmu->dev = &pdev->dev;
>> +       mutex_init(&pmu->mutex);
>> +
>> +       pmu->info = pmu_info;
>> +
>> +       pmu->genpd_data.domains = pmu->domains;
>> +       pmu->genpd_data.num_domains = pmu_info->num_domains;
>> +
>> +       node = of_parse_phandle(np, "rockchip,pmu", 0);
>> +       pmu->regmap = syscon_node_to_regmap(node);
>> +       of_node_put(node);
>> +       if (IS_ERR(pmu->regmap)) {
>> +               error = PTR_ERR(pmu->regmap);
>> +               dev_err(dev, "failed to get PMU regmap: %d\n", error);
>> +               return error;
>> +       }
>> +
>> +       /*
>> +        * Configure power up and down transition delays for core
>> +        * and GPU domains.
>> +        */
>> +       rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
>> +                                 pmu_info->core_power_transition_time);
>> +       rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
>> +                                 pmu_info->gpu_power_transition_time);
>> +
>> +       error = -ENXIO;
>> +
>> +       for_each_available_child_of_node(np, node) {
>> +               error = rockchip_pm_add_one_domain(pmu, node);
>> +               if (error) {
>> +                       dev_err(dev, "failed to handle node %s: %d\n",
>> +                               node->name, error);
>> +                       goto err_out;
>> +               }
>> +       }
>> +
>> +       if (error) {
>> +               dev_dbg(dev, "no power domains defined\n");
>> +               goto err_out;
>> +       }
>> +
>> +       of_genpd_add_provider_onecell(np, &pmu->genpd_data);
>> +
>> +       return 0;
>> +
>> +err_out:
>> +       rockchip_pm_domain_cleanup(pmu);
>> +       return error;
>> +}
>> +
>> +static const struct rockchip_domain_info rk3288_pm_domains[] = {
>> +       [RK3288_PD_GPU]         = DOMAIN_RK3288(9, 9, 2),
>> +       [RK3288_PD_VIO]         = DOMAIN_RK3288(7, 7, 4),
>> +       [RK3288_PD_VIDEO]       = DOMAIN_RK3288(8, 8, 3),
>> +       [RK3288_PD_HEVC]        = DOMAIN_RK3288(14, 10, 9),
>> +};
>> +
>> +static const struct rockchip_pmu_info rk3288_pmu = {
>> +       .pwr_offset = 0x08,
>> +       .status_offset = 0x0c,
>> +       .req_offset = 0x10,
>> +       .idle_offset = 0x14,
>> +       .ack_offset = 0x14,
>> +
>> +       .core_pwrcnt_offset = 0x34,
>> +       .gpu_pwrcnt_offset = 0x3c,
>> +
>> +       .core_power_transition_time = 24, /* 1us */
>> +       .gpu_power_transition_time = 24, /* 1us */
>> +
>> +       .num_domains = ARRAY_SIZE(rk3288_pm_domains),
>> +       .domain_info = rk3288_pm_domains,
>> +};
>> +
>> +static const struct of_device_id rockchip_pm_domain_dt_match[] = {
>> +       {
>> +               .compatible = "rockchip,rk3288-power-controller",
>> +               .data = (void *)&rk3288_pmu,
>> +       },
>> +       { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver rockchip_pm_domain_driver = {
>> +       .probe = rockchip_pm_domain_probe,
>> +       .driver = {
>> +               .name   = "rockchip-pm-domain",
>> +               .of_match_table = rockchip_pm_domain_dt_match,
>> +               /*
>> +                * We can't forcibly eject devices form power domain,
>> +                * so we can't really remove power domains once they
>> +                * were added.
>> +                */
>> +               .suppress_bind_attrs = true,
>> +       },
>> +};
>> +
>> +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.9.1
>>
> Kind regards
> Uffe
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Caesar Wang]

Ulf,

Thanks for your comments.:-)



? 2015?05?28? 18:38, Ulf Hansson ??:
> On 24 April 2015 at 10:07, Caesar Wang <wxt@rock-chips.com> wrote:
>> In order to meet high performance and low power requirements, a power
>> management unit is designed or saving power when RK3288 in low power
>> mode.
>> The RK3288 PMU is dedicated for managing the power ot the whole chip.
>>
>> Signed-off-by: jinkun.hong <jinkun.hong@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
>>
>>   arch/arm/mach-rockchip/Kconfig      |   1 +
>>   arch/arm/mach-rockchip/Makefile     |   1 +
>>   arch/arm/mach-rockchip/pm_domains.c | 506 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 508 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 ae4eb7c..578206b 100644
>> --- a/arch/arm/mach-rockchip/Kconfig
>> +++ b/arch/arm/mach-rockchip/Kconfig
>> @@ -15,6 +15,7 @@ config ARCH_ROCKCHIP
>>          select ROCKCHIP_TIMER
>>          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 5c3a9b2..9c902d3 100644
>> --- a/arch/arm/mach-rockchip/Makefile
>> +++ b/arch/arm/mach-rockchip/Makefile
>> @@ -1,5 +1,6 @@
>>   CFLAGS_platsmp.o := -march=armv7-a
>>
>>   obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o
>> +obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
>>   obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o
>>   obj-$(CONFIG_SMP) += headsmp.o platsmp.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..92d2569
>> --- /dev/null
>> +++ b/arch/arm/mach-rockchip/pm_domains.c
>> @@ -0,0 +1,506 @@
>> +/*
>> + * Rockchip Generic power domain support.
>> + *
>> + * Copyright (c) 2014 ROCKCHIP, Co. Ltd.
>> + *
>> + * 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/io.h>
>> +#include <linux/err.h>
>> +#include <linux/pm_clock.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
> clk-provider.h, why?

The following is needed.

_clk_get_name(clk)


>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <dt-bindings/power-domain/rk3288.h>
> Same comment as in patch1. I don't find this header.

OK.
Perhaps, Making a mistake. the missing the patch in 
here.(https://patchwork.kernel.org/patch/6266881/)
>> +
>> +struct rockchip_domain_info {
>> +       int pwr_mask;
>> +       int status_mask;
>> +       int req_mask;
>> +       int idle_mask;
>> +       int ack_mask;
>> +};
>> +
>> +struct rockchip_pmu_info {
>> +       u32 pwr_offset;
>> +       u32 status_offset;
>> +       u32 req_offset;
>> +       u32 idle_offset;
>> +       u32 ack_offset;
>> +
>> +       u32 core_pwrcnt_offset;
>> +       u32 gpu_pwrcnt_offset;
>> +
>> +       unsigned int core_power_transition_time;
>> +       unsigned int gpu_power_transition_time;
>> +
>> +       int num_domains;
>> +       const struct rockchip_domain_info *domain_info;
>> +};
>> +
>> +struct rockchip_pm_domain {
>> +       struct generic_pm_domain genpd;
>> +       const struct rockchip_domain_info *info;
>> +       struct rockchip_pmu *pmu;
>> +       int num_clks;
>> +       struct clk *clks[];
>> +};
>> +
>> +struct rockchip_pmu {
>> +       struct device *dev;
>> +       struct regmap *regmap;
>> +       const struct rockchip_pmu_info *info;
>> +       struct mutex mutex; /* mutex lock for pmu */
>> +       struct genpd_onecell_data genpd_data;
>> +       struct generic_pm_domain *domains[];
>> +};
>> +
>> +#define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
>> +
>> +#define DOMAIN(pwr, status, req, idle, ack)    \
>> +{                                              \
>> +       .pwr_mask = BIT(pwr),                   \
>> +       .status_mask = BIT(status),             \
>> +       .req_mask = BIT(req),                   \
>> +       .idle_mask = BIT(idle),                 \
>> +       .ack_mask = BIT(ack),                   \
>> +}
>> +
>> +#define DOMAIN_RK3288(pwr, status, req)                \
>> +       DOMAIN(pwr, status, req, req, (req) + 16)
>> +
>> +static bool rockchip_pmu_domain_is_idle(struct rockchip_pm_domain *pd)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       const struct rockchip_domain_info *pd_info = pd->info;
>> +       unsigned int val;
>> +
>> +       regmap_read(pmu->regmap, pmu->info->idle_offset, &val);
>> +       return (val & pd_info->idle_mask) == pd_info->idle_mask;
>> +}
>> +
>> +static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
>> +                                        bool idle)
>> +{
>> +       const struct rockchip_domain_info *pd_info = pd->info;
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       unsigned int val;
>> +
>> +       regmap_update_bits(pmu->regmap, pmu->info->req_offset,
>> +                          pd_info->req_mask, idle ? -1U : 0);
>> +
>> +       dsb();
>> +
>> +       do {
>> +               regmap_read(pmu->regmap, pmu->info->ack_offset, &val);
>> +       } while ((val & pd_info->ack_mask) != (idle ? pd_info->ack_mask : 0));
>> +
>> +       while (rockchip_pmu_domain_is_idle(pd) != idle)
>> +               cpu_relax();
>> +
>> +       return 0;
>> +}
>> +
>> +static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +       unsigned int val;
>> +
>> +       regmap_read(pmu->regmap, pmu->info->status_offset, &val);
>> +
>> +       /* 1'b0: power on, 1'b1: power off */
>> +       return !(val & pd->info->status_mask);
>> +}
>> +
>> +static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
>> +                                            bool on)
>> +{
>> +       struct rockchip_pmu *pmu = pd->pmu;
>> +
>> +       regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
>> +                          pd->info->pwr_mask, on ? 0 : -1U);
>> +
>> +       dsb();
>> +
>> +       while (rockchip_pmu_domain_is_on(pd) != on)
>> +               cpu_relax();
>> +}
>> +
>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
>> +{
>> +       int i;
>> +
>> +       mutex_lock(&pd->pmu->mutex);
> I don't quite understand why you need a lock, what are you protecting and why?

Says:
[ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3551.919730] rockchip_pd_power:139: mutex_lock
[ 3551.924130] rockchip_pd_power:167,mutex_unlock
[ 3563.827295] rockchip_pd_power:139: mutex_lock
[ 3563.831753] rockchip_pd_power:167,mutex_unlock
[ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3564.079047] rockchip_pd_power:139: mutex_lock
[ 3564.083426] rockchip_pd_power:167,mutex_unlock
[ 3611.665726] rockchip_pd_power:139: mutex_lock
[ 3611.670206] rockchip_pd_power:167,mutex_unlock
[ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
[ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
[ 3611.919689] rockchip_pd_power:139: mutex_lock
[ 3611.924090] rockchip_pd_power:167,mutex_unlock
[ 3623.848296] rockchip_pd_power:139: mutex_lock
[ 3623.852752] rockchip_pd_power:167,mutex_unlock



>> +
>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>> +               for (i = 0; i < pd->num_clks; i++)
>> +                       clk_enable(pd->clks[i]);
>> +
>> +               if (!power_on) {
>> +                       /* FIXME: add code to save AXI_QOS */
>> +
>> +                       /* if powering down, idle request to NIU first */
>> +                       rockchip_pmu_set_idle_request(pd, true);
>> +               }
>> +
>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>> +
>> +               if (power_on) {
>> +                       /* if powering up, leave idle mode */
>> +                       rockchip_pmu_set_idle_request(pd, false);
>> +
>> +                       /* FIXME: add code to restore AXI_QOS */
>> +               }
>> +
>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>> +                       clk_disable(pd->clks[i]);
>> +       }
>> +
>> +       mutex_unlock(&pd->pmu->mutex);
>> +       return 0;
>> +}
>> +
>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>> +{
>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>> +
>> +       return rockchip_pd_power(pd, false);
>> +}
>> +
>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>> +                                 struct device *dev)
>> +{
>> +       struct clk *clk;
>> +       int i;
>> +       int error;
>> +
>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>> +
>> +       error = pm_clk_create(dev);
>> +       if (error) {
>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>> +               return error;
>> +       }
>> +
>> +       i = 0;
>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
> This loop adds all available device clocks to the PM clock list. I
> wonder if this could be considered as a common thing and if so, we
> might want to extend the pm_clk API with this.

There are several reasons as follows:

     Firstly, the clocks need be turned off to save power when
     the system enter the suspend state. So we need to enumerate the clocks
     in the dts. In order to power domain can turn on and off.

     Secondly, the reset-circuit should reset be synchronous on rk3288, 
then sync revoked.
     So we need to enable clocks of all devices.
>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>> +                       __clk_get_name(clk));
>> +               error = pm_clk_add_clk(dev, clk);
>> +               clk_put(clk);
>> +               if (error) {
>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n", error);
>> +                       pm_clk_destroy(dev);
>> +                       return error;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
>> +                                  struct device *dev)
>> +{
>> +       dev_dbg(dev, "detaching from power domain '%s'\n", genpd->name);
>> +
>> +       pm_clk_destroy(dev);
>> +}
>> +
>> +static int rockchip_pd_start_dev(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +
>> +       dev_dbg(dev, "starting device in power domain '%s'\n", genpd->name);
>> +
>> +       return pm_clk_resume(dev);
>> +}
>> +
>> +static int rockchip_pd_stop_dev(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +
>> +       dev_dbg(dev, "stopping device in power domain '%s'\n", genpd->name);
>> +
>> +       return pm_clk_suspend(dev);
>> +}
>> +
>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>> +                                     struct device_node *node)
>> +{
>> +       const struct rockchip_domain_info *pd_info;
>> +       struct rockchip_pm_domain *pd;
>> +       struct clk *clk;
>> +       int clk_cnt;
>> +       int i;
>> +       u32 id;
>> +       int error;
>> +
>> +       error = of_property_read_u32(node, "reg", &id);
>> +       if (error) {
>> +               dev_err(pmu->dev,
>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>> +                       node->name, error);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (id >= pmu->info->num_domains) {
>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>> +                       node->name, id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       pd_info = &pmu->info->domain_info[id];
>> +       if (!pd_info) {
>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>> +                       node->name, id);
>> +               return -EINVAL;
>> +       }
>> +
>> +       clk_cnt = of_count_phandle_with_args(node, "clocks", "#clock-cells");
>> +       pd = devm_kzalloc(pmu->dev,
>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>> +                         GFP_KERNEL);
>> +       if (!pd)
>> +               return -ENOMEM;
>> +
>> +       pd->info = pd_info;
>> +       pd->pmu = pmu;
>> +
>> +       for (i = 0; i < clk_cnt; i++) {
> This loop is similar to the one when creates the PM clock list in the
> rockchip_pd_attach_dev().
>
> What's the reason you don't want to use pm clk for these clocks?
>

Ditto.
>> +               clk = of_clk_get(node, i);
>> +               if (IS_ERR(clk)) {
>> +                       error = PTR_ERR(clk);
>> +                       dev_err(pmu->dev,
>> +                               "%s: failed to get clk %s (index %d): %d\n",
>> +                               node->name, __clk_get_name(clk), i, error);
>> +                       goto err_out;
>> +               }
>> +
>> +               error = clk_prepare(clk);
>> +               if (error) {
>> +                       dev_err(pmu->dev,
>> +                               "%s: failed to prepare clk %s (index %d): %d\n",
>> +                               node->name, __clk_get_name(clk), i, error);
>> +                       clk_put(clk);
>> +                       goto err_out;
>> +               }
>> +
>> +               pd->clks[pd->num_clks++] = clk;
>> +
>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>> +                       __clk_get_name(clk), node->name);
>> +       }
>> +
>> +       error = rockchip_pd_power(pd, true);
>> +       if (error) {
>> +               dev_err(pmu->dev,
>> +                       "failed to power on domain '%s': %d\n",
>> +                       node->name, error);
>> +               goto err_out;
>> +       }
>> +
>> +       pd->genpd.name = node->name;
>> +       pd->genpd.power_off = rockchip_pd_power_off;
>> +       pd->genpd.power_on = rockchip_pd_power_on;
>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
> Instead of assigning the ->stop|start() callbacks, which do
> pm_clk_suspend|resume(), just set the genpd->flags to
> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>
Ditto.

>> +       pm_genpd_init(&pd->genpd, NULL, false);
>> +
>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>> +       return 0;
>> +
>> +err_out:
>> +       while (--i >= 0) {
>> +               clk_unprepare(pd->clks[i]);
>> +               clk_put(pd->clks[i]);
>> +       }
>> +       return error;
>> +}
>> +
>> +static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < pd->num_clks; i++) {
>> +               clk_unprepare(pd->clks[i]);
> If you converted these clocks to be dealt with via the PM clk API,
> pm_clk_destoy() would have replaced this loop.
>
>> +               clk_put(pd->clks[i]);
>> +       }
>> +
>> +       /* devm will free our memory */
>> +}
>> +
>> +static void rockchip_pm_domain_cleanup(struct rockchip_pmu *pmu)
>> +{
>> +       struct generic_pm_domain *genpd;
>> +       struct rockchip_pm_domain *pd;
>> +       int i;
>> +
>> +       for (i = 0; i < pmu->genpd_data.num_domains; i++) {
>> +               genpd = pmu->genpd_data.domains[i];
>> +               if (genpd) {
>> +                       pd = to_rockchip_pd(genpd);
>> +                       rockchip_pm_remove_one_domain(pd);
>> +               }
>> +       }
>> +
>> +       /* devm will free our memory */
>> +}
>> +
>> +static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
>> +                                     u32 domain_reg_offset,
>> +                                     unsigned int count)
>> +{
>> +       /* First configure domain power down transition count ... */
>> +       regmap_write(pmu->regmap, domain_reg_offset, count);
>> +       /* ... and then power up count. */
>> +       regmap_write(pmu->regmap, domain_reg_offset + 4, count);
>> +}
>> +
>> +static int rockchip_pm_domain_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct device_node *node;
>> +       struct rockchip_pmu *pmu;
>> +       const struct of_device_id *match;
>> +       const struct rockchip_pmu_info *pmu_info;
>> +       int error;
>> +
>> +       if (!np) {
>> +               dev_err(dev, "device tree node not found\n");
>> +               return -ENXIO;/
> Nitpick:
> It's more common to return -ENODEV here, you might want to change.

OK.

Thanks,
Caesar
>> +       }
>> +
>> +       match = of_match_device(dev->driver->of_match_table, dev);
>> +       if (!match || !match->data) {
>> +               dev_err(dev, "missing pmu data\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       pmu_info = match->data;
>> +
>> +       pmu = devm_kzalloc(dev,
>> +                          sizeof(*pmu) +
>> +                               pmu_info->num_domains * sizeof(pmu->domains[0]),
>> +                          GFP_KERNEL);
>> +       if (!pmu)
>> +               return -ENOMEM;
>> +
>> +       pmu->dev = &pdev->dev;
>> +       mutex_init(&pmu->mutex);
>> +
>> +       pmu->info = pmu_info;
>> +
>> +       pmu->genpd_data.domains = pmu->domains;
>> +       pmu->genpd_data.num_domains = pmu_info->num_domains;
>> +
>> +       node = of_parse_phandle(np, "rockchip,pmu", 0);
>> +       pmu->regmap = syscon_node_to_regmap(node);
>> +       of_node_put(node);
>> +       if (IS_ERR(pmu->regmap)) {
>> +               error = PTR_ERR(pmu->regmap);
>> +               dev_err(dev, "failed to get PMU regmap: %d\n", error);
>> +               return error;
>> +       }
>> +
>> +       /*
>> +        * Configure power up and down transition delays for core
>> +        * and GPU domains.
>> +        */
>> +       rockchip_configure_pd_cnt(pmu, pmu_info->core_pwrcnt_offset,
>> +                                 pmu_info->core_power_transition_time);
>> +       rockchip_configure_pd_cnt(pmu, pmu_info->gpu_pwrcnt_offset,
>> +                                 pmu_info->gpu_power_transition_time);
>> +
>> +       error = -ENXIO;
>> +
>> +       for_each_available_child_of_node(np, node) {
>> +               error = rockchip_pm_add_one_domain(pmu, node);
>> +               if (error) {
>> +                       dev_err(dev, "failed to handle node %s: %d\n",
>> +                               node->name, error);
>> +                       goto err_out;
>> +               }
>> +       }
>> +
>> +       if (error) {
>> +               dev_dbg(dev, "no power domains defined\n");
>> +               goto err_out;
>> +       }
>> +
>> +       of_genpd_add_provider_onecell(np, &pmu->genpd_data);
>> +
>> +       return 0;
>> +
>> +err_out:
>> +       rockchip_pm_domain_cleanup(pmu);
>> +       return error;
>> +}
>> +
>> +static const struct rockchip_domain_info rk3288_pm_domains[] = {
>> +       [RK3288_PD_GPU]         = DOMAIN_RK3288(9, 9, 2),
>> +       [RK3288_PD_VIO]         = DOMAIN_RK3288(7, 7, 4),
>> +       [RK3288_PD_VIDEO]       = DOMAIN_RK3288(8, 8, 3),
>> +       [RK3288_PD_HEVC]        = DOMAIN_RK3288(14, 10, 9),
>> +};
>> +
>> +static const struct rockchip_pmu_info rk3288_pmu = {
>> +       .pwr_offset = 0x08,
>> +       .status_offset = 0x0c,
>> +       .req_offset = 0x10,
>> +       .idle_offset = 0x14,
>> +       .ack_offset = 0x14,
>> +
>> +       .core_pwrcnt_offset = 0x34,
>> +       .gpu_pwrcnt_offset = 0x3c,
>> +
>> +       .core_power_transition_time = 24, /* 1us */
>> +       .gpu_power_transition_time = 24, /* 1us */
>> +
>> +       .num_domains = ARRAY_SIZE(rk3288_pm_domains),
>> +       .domain_info = rk3288_pm_domains,
>> +};
>> +
>> +static const struct of_device_id rockchip_pm_domain_dt_match[] = {
>> +       {
>> +               .compatible = "rockchip,rk3288-power-controller",
>> +               .data = (void *)&rk3288_pmu,
>> +       },
>> +       { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver rockchip_pm_domain_driver = {
>> +       .probe = rockchip_pm_domain_probe,
>> +       .driver = {
>> +               .name   = "rockchip-pm-domain",
>> +               .of_match_table = rockchip_pm_domain_dt_match,
>> +               /*
>> +                * We can't forcibly eject devices form power domain,
>> +                * so we can't really remove power domains once they
>> +                * were added.
>> +                */
>> +               .suppress_bind_attrs = true,
>> +       },
>> +};
>> +
>> +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.9.1
>>
> Kind regards
> Uffe
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Ulf Hansson]

[...]

>>> +#include <linux/clk-provider.h>
>>
>> clk-provider.h, why?
>
>
> The following is needed.
>
> _clk_get_name(clk)

I see, you need it for for the dev_dbg().

I think you shall use "%pC" as the formatting string for the dev_dbg()
message, since that will take care of printing the clock name for you.

[...]

>>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool
>>> power_on)
>>> +{
>>> +       int i;
>>> +
>>> +       mutex_lock(&pd->pmu->mutex);
>>
>> I don't quite understand why you need a lock, what are you protecting and
>> why?
>
>
> Says:
> [ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.919730] rockchip_pd_power:139: mutex_lock
> [ 3551.924130] rockchip_pd_power:167,mutex_unlock
> [ 3563.827295] rockchip_pd_power:139: mutex_lock
> [ 3563.831753] rockchip_pd_power:167,mutex_unlock
> [ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.079047] rockchip_pd_power:139: mutex_lock
> [ 3564.083426] rockchip_pd_power:167,mutex_unlock
> [ 3611.665726] rockchip_pd_power:139: mutex_lock
> [ 3611.670206] rockchip_pd_power:167,mutex_unlock
> [ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.919689] rockchip_pd_power:139: mutex_lock
> [ 3611.924090] rockchip_pd_power:167,mutex_unlock
> [ 3623.848296] rockchip_pd_power:139: mutex_lock
> [ 3623.852752] rockchip_pd_power:167,mutex_unlock
>
>
>
>
>>> +
>>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>>> +               for (i = 0; i < pd->num_clks; i++)
>>> +                       clk_enable(pd->clks[i]);
>>> +
>>> +               if (!power_on) {
>>> +                       /* FIXME: add code to save AXI_QOS */
>>> +
>>> +                       /* if powering down, idle request to NIU first */
>>> +                       rockchip_pmu_set_idle_request(pd, true);
>>> +               }
>>> +
>>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>>> +
>>> +               if (power_on) {
>>> +                       /* if powering up, leave idle mode */
>>> +                       rockchip_pmu_set_idle_request(pd, false);
>>> +
>>> +                       /* FIXME: add code to restore AXI_QOS */
>>> +               }
>>> +
>>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>>> +                       clk_disable(pd->clks[i]);
>>> +       }
>>> +
>>> +       mutex_unlock(&pd->pmu->mutex);
>>> +       return 0;
>>> +}
>>> +
>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>> +{
>>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>>> +
>>> +       return rockchip_pd_power(pd, false);
>>> +}
>>> +
>>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>>> +                                 struct device *dev)
>>> +{
>>> +       struct clk *clk;
>>> +       int i;
>>> +       int error;
>>> +
>>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>>> +
>>> +       error = pm_clk_create(dev);
>>> +       if (error) {
>>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>>> +               return error;
>>> +       }
>>> +
>>> +       i = 0;
>>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>
>> This loop adds all available device clocks to the PM clock list. I
>> wonder if this could be considered as a common thing and if so, we
>> might want to extend the pm_clk API with this.
>
>
> There are several reasons as follows:
>
>     Firstly, the clocks need be turned off to save power when
>     the system enter the suspend state. So we need to enumerate the clocks
>     in the dts. In order to power domain can turn on and off.
>
>     Secondly, the reset-circuit should reset be synchronous on rk3288, then
> sync revoked.
>     So we need to enable clocks of all devices.

I think you misinterpreted my previous answer. Instead of fetching all
clocks for a device and manually create the pm_clk list as you do
here, I was suggesting to make this a part of the pm clk API.

I would happily support such an API, but I can't tell what the other
maintainers think.

>
>>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>>> +                       __clk_get_name(clk));
>>> +               error = pm_clk_add_clk(dev, clk);
>>> +               clk_put(clk);
>>> +               if (error) {
>>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n",
>>> error);
>>> +                       pm_clk_destroy(dev);
>>> +                       return error;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +

[...]

>>> +
>>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>>> +                                     struct device_node *node)
>>> +{
>>> +       const struct rockchip_domain_info *pd_info;
>>> +       struct rockchip_pm_domain *pd;
>>> +       struct clk *clk;
>>> +       int clk_cnt;
>>> +       int i;
>>> +       u32 id;
>>> +       int error;
>>> +
>>> +       error = of_property_read_u32(node, "reg", &id);
>>> +       if (error) {
>>> +               dev_err(pmu->dev,
>>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>>> +                       node->name, error);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       if (id >= pmu->info->num_domains) {
>>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>>> +                       node->name, id);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       pd_info = &pmu->info->domain_info[id];
>>> +       if (!pd_info) {
>>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>>> +                       node->name, id);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       clk_cnt = of_count_phandle_with_args(node, "clocks",
>>> "#clock-cells");
>>> +       pd = devm_kzalloc(pmu->dev,
>>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>>> +                         GFP_KERNEL);
>>> +       if (!pd)
>>> +               return -ENOMEM;
>>> +
>>> +       pd->info = pd_info;
>>> +       pd->pmu = pmu;
>>> +
>>> +       for (i = 0; i < clk_cnt; i++) {
>>
>> This loop is similar to the one when creates the PM clock list in the
>> rockchip_pd_attach_dev().
>>
>> What's the reason you don't want to use pm clk for these clocks?
>>
>
> Ditto.

I don't follow. Can't you do the exact same thing via the pm clk API,
can you please elaborate!?

>
>>> +               clk = of_clk_get(node, i);
>>> +               if (IS_ERR(clk)) {
>>> +                       error = PTR_ERR(clk);
>>> +                       dev_err(pmu->dev,
>>> +                               "%s: failed to get clk %s (index %d):
>>> %d\n",
>>> +                               node->name, __clk_get_name(clk), i,
>>> error);
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               error = clk_prepare(clk);
>>> +               if (error) {
>>> +                       dev_err(pmu->dev,
>>> +                               "%s: failed to prepare clk %s (index %d):
>>> %d\n",
>>> +                               node->name, __clk_get_name(clk), i,
>>> error);
>>> +                       clk_put(clk);
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               pd->clks[pd->num_clks++] = clk;
>>> +
>>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>>> +                       __clk_get_name(clk), node->name);
>>> +       }
>>> +
>>> +       error = rockchip_pd_power(pd, true);
>>> +       if (error) {
>>> +               dev_err(pmu->dev,
>>> +                       "failed to power on domain '%s': %d\n",
>>> +                       node->name, error);
>>> +               goto err_out;
>>> +       }
>>> +
>>> +       pd->genpd.name = node->name;
>>> +       pd->genpd.power_off = rockchip_pd_power_off;
>>> +       pd->genpd.power_on = rockchip_pd_power_on;
>>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>>
>> Instead of assigning the ->stop|start() callbacks, which do
>> pm_clk_suspend|resume(), just set the genpd->flags to
>> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>>
> Ditto.

Again, I don't follow. Here is what goes on:

rockchip_pd_start_dev() calls pm_clk_resume() and
rockchip_pd_stop_dev() calls pm_clk_suspend().
Instead of assigning the below callbacks...

pd->genpd.dev_ops.start = rockchip_pd_start_dev;
pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;

... just use GENPD_FLAG_PM_CLK and let genpd deal with the calls to
pm_clk_suspend|resume().

>
>
>>> +       pm_genpd_init(&pd->genpd, NULL, false);
>>> +
>>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>>> +       return 0;
>>> +
>>> +err_out:
>>> +       while (--i >= 0) {
>>> +               clk_unprepare(pd->clks[i]);
>>> +               clk_put(pd->clks[i]);
>>> +       }
>>> +       return error;
>>> +}
>>> +

[...]

Kind regards
Uffe

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Ulf Hansson]

[...]

>>> +#include <linux/clk-provider.h>
>>
>> clk-provider.h, why?
>
>
> The following is needed.
>
> _clk_get_name(clk)

I see, you need it for for the dev_dbg().

I think you shall use "%pC" as the formatting string for the dev_dbg()
message, since that will take care of printing the clock name for you.

[...]

>>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool
>>> power_on)
>>> +{
>>> +       int i;
>>> +
>>> +       mutex_lock(&pd->pmu->mutex);
>>
>> I don't quite understand why you need a lock, what are you protecting and
>> why?
>
>
> Says:
> [ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.919730] rockchip_pd_power:139: mutex_lock
> [ 3551.924130] rockchip_pd_power:167,mutex_unlock
> [ 3563.827295] rockchip_pd_power:139: mutex_lock
> [ 3563.831753] rockchip_pd_power:167,mutex_unlock
> [ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.079047] rockchip_pd_power:139: mutex_lock
> [ 3564.083426] rockchip_pd_power:167,mutex_unlock
> [ 3611.665726] rockchip_pd_power:139: mutex_lock
> [ 3611.670206] rockchip_pd_power:167,mutex_unlock
> [ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.919689] rockchip_pd_power:139: mutex_lock
> [ 3611.924090] rockchip_pd_power:167,mutex_unlock
> [ 3623.848296] rockchip_pd_power:139: mutex_lock
> [ 3623.852752] rockchip_pd_power:167,mutex_unlock
>
>
>
>
>>> +
>>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>>> +               for (i = 0; i < pd->num_clks; i++)
>>> +                       clk_enable(pd->clks[i]);
>>> +
>>> +               if (!power_on) {
>>> +                       /* FIXME: add code to save AXI_QOS */
>>> +
>>> +                       /* if powering down, idle request to NIU first */
>>> +                       rockchip_pmu_set_idle_request(pd, true);
>>> +               }
>>> +
>>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>>> +
>>> +               if (power_on) {
>>> +                       /* if powering up, leave idle mode */
>>> +                       rockchip_pmu_set_idle_request(pd, false);
>>> +
>>> +                       /* FIXME: add code to restore AXI_QOS */
>>> +               }
>>> +
>>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>>> +                       clk_disable(pd->clks[i]);
>>> +       }
>>> +
>>> +       mutex_unlock(&pd->pmu->mutex);
>>> +       return 0;
>>> +}
>>> +
>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>> +{
>>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>>> +
>>> +       return rockchip_pd_power(pd, false);
>>> +}
>>> +
>>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>>> +                                 struct device *dev)
>>> +{
>>> +       struct clk *clk;
>>> +       int i;
>>> +       int error;
>>> +
>>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>>> +
>>> +       error = pm_clk_create(dev);
>>> +       if (error) {
>>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>>> +               return error;
>>> +       }
>>> +
>>> +       i = 0;
>>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>
>> This loop adds all available device clocks to the PM clock list. I
>> wonder if this could be considered as a common thing and if so, we
>> might want to extend the pm_clk API with this.
>
>
> There are several reasons as follows:
>
>     Firstly, the clocks need be turned off to save power when
>     the system enter the suspend state. So we need to enumerate the clocks
>     in the dts. In order to power domain can turn on and off.
>
>     Secondly, the reset-circuit should reset be synchronous on rk3288, then
> sync revoked.
>     So we need to enable clocks of all devices.

I think you misinterpreted my previous answer. Instead of fetching all
clocks for a device and manually create the pm_clk list as you do
here, I was suggesting to make this a part of the pm clk API.

I would happily support such an API, but I can't tell what the other
maintainers think.

>
>>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>>> +                       __clk_get_name(clk));
>>> +               error = pm_clk_add_clk(dev, clk);
>>> +               clk_put(clk);
>>> +               if (error) {
>>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n",
>>> error);
>>> +                       pm_clk_destroy(dev);
>>> +                       return error;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +

[...]

>>> +
>>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>>> +                                     struct device_node *node)
>>> +{
>>> +       const struct rockchip_domain_info *pd_info;
>>> +       struct rockchip_pm_domain *pd;
>>> +       struct clk *clk;
>>> +       int clk_cnt;
>>> +       int i;
>>> +       u32 id;
>>> +       int error;
>>> +
>>> +       error = of_property_read_u32(node, "reg", &id);
>>> +       if (error) {
>>> +               dev_err(pmu->dev,
>>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>>> +                       node->name, error);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       if (id >= pmu->info->num_domains) {
>>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>>> +                       node->name, id);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       pd_info = &pmu->info->domain_info[id];
>>> +       if (!pd_info) {
>>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>>> +                       node->name, id);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       clk_cnt = of_count_phandle_with_args(node, "clocks",
>>> "#clock-cells");
>>> +       pd = devm_kzalloc(pmu->dev,
>>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>>> +                         GFP_KERNEL);
>>> +       if (!pd)
>>> +               return -ENOMEM;
>>> +
>>> +       pd->info = pd_info;
>>> +       pd->pmu = pmu;
>>> +
>>> +       for (i = 0; i < clk_cnt; i++) {
>>
>> This loop is similar to the one when creates the PM clock list in the
>> rockchip_pd_attach_dev().
>>
>> What's the reason you don't want to use pm clk for these clocks?
>>
>
> Ditto.

I don't follow. Can't you do the exact same thing via the pm clk API,
can you please elaborate!?

>
>>> +               clk = of_clk_get(node, i);
>>> +               if (IS_ERR(clk)) {
>>> +                       error = PTR_ERR(clk);
>>> +                       dev_err(pmu->dev,
>>> +                               "%s: failed to get clk %s (index %d):
>>> %d\n",
>>> +                               node->name, __clk_get_name(clk), i,
>>> error);
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               error = clk_prepare(clk);
>>> +               if (error) {
>>> +                       dev_err(pmu->dev,
>>> +                               "%s: failed to prepare clk %s (index %d):
>>> %d\n",
>>> +                               node->name, __clk_get_name(clk), i,
>>> error);
>>> +                       clk_put(clk);
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               pd->clks[pd->num_clks++] = clk;
>>> +
>>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>>> +                       __clk_get_name(clk), node->name);
>>> +       }
>>> +
>>> +       error = rockchip_pd_power(pd, true);
>>> +       if (error) {
>>> +               dev_err(pmu->dev,
>>> +                       "failed to power on domain '%s': %d\n",
>>> +                       node->name, error);
>>> +               goto err_out;
>>> +       }
>>> +
>>> +       pd->genpd.name = node->name;
>>> +       pd->genpd.power_off = rockchip_pd_power_off;
>>> +       pd->genpd.power_on = rockchip_pd_power_on;
>>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>>
>> Instead of assigning the ->stop|start() callbacks, which do
>> pm_clk_suspend|resume(), just set the genpd->flags to
>> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>>
> Ditto.

Again, I don't follow. Here is what goes on:

rockchip_pd_start_dev() calls pm_clk_resume() and
rockchip_pd_stop_dev() calls pm_clk_suspend().
Instead of assigning the below callbacks...

pd->genpd.dev_ops.start = rockchip_pd_start_dev;
pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;

... just use GENPD_FLAG_PM_CLK and let genpd deal with the calls to
pm_clk_suspend|resume().

>
>
>>> +       pm_genpd_init(&pd->genpd, NULL, false);
>>> +
>>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>>> +       return 0;
>>> +
>>> +err_out:
>>> +       while (--i >= 0) {
>>> +               clk_unprepare(pd->clks[i]);
>>> +               clk_put(pd->clks[i]);
>>> +       }
>>> +       return error;
>>> +}
>>> +

[...]

Kind regards
Uffe

[PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Ulf Hansson]

[...]

>>> +#include <linux/clk-provider.h>
>>
>> clk-provider.h, why?
>
>
> The following is needed.
>
> _clk_get_name(clk)

I see, you need it for for the dev_dbg().

I think you shall use "%pC" as the formatting string for the dev_dbg()
message, since that will take care of printing the clock name for you.

[...]

>>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool
>>> power_on)
>>> +{
>>> +       int i;
>>> +
>>> +       mutex_lock(&pd->pmu->mutex);
>>
>> I don't quite understand why you need a lock, what are you protecting and
>> why?
>
>
> Says:
> [ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3551.919730] rockchip_pd_power:139: mutex_lock
> [ 3551.924130] rockchip_pd_power:167,mutex_unlock
> [ 3563.827295] rockchip_pd_power:139: mutex_lock
> [ 3563.831753] rockchip_pd_power:167,mutex_unlock
> [ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3564.079047] rockchip_pd_power:139: mutex_lock
> [ 3564.083426] rockchip_pd_power:167,mutex_unlock
> [ 3611.665726] rockchip_pd_power:139: mutex_lock
> [ 3611.670206] rockchip_pd_power:167,mutex_unlock
> [ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
> [ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
> [ 3611.919689] rockchip_pd_power:139: mutex_lock
> [ 3611.924090] rockchip_pd_power:167,mutex_unlock
> [ 3623.848296] rockchip_pd_power:139: mutex_lock
> [ 3623.852752] rockchip_pd_power:167,mutex_unlock
>
>
>
>
>>> +
>>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>>> +               for (i = 0; i < pd->num_clks; i++)
>>> +                       clk_enable(pd->clks[i]);
>>> +
>>> +               if (!power_on) {
>>> +                       /* FIXME: add code to save AXI_QOS */
>>> +
>>> +                       /* if powering down, idle request to NIU first */
>>> +                       rockchip_pmu_set_idle_request(pd, true);
>>> +               }
>>> +
>>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>>> +
>>> +               if (power_on) {
>>> +                       /* if powering up, leave idle mode */
>>> +                       rockchip_pmu_set_idle_request(pd, false);
>>> +
>>> +                       /* FIXME: add code to restore AXI_QOS */
>>> +               }
>>> +
>>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>>> +                       clk_disable(pd->clks[i]);
>>> +       }
>>> +
>>> +       mutex_unlock(&pd->pmu->mutex);
>>> +       return 0;
>>> +}
>>> +
>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>> +{
>>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>>> +
>>> +       return rockchip_pd_power(pd, false);
>>> +}
>>> +
>>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>>> +                                 struct device *dev)
>>> +{
>>> +       struct clk *clk;
>>> +       int i;
>>> +       int error;
>>> +
>>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>>> +
>>> +       error = pm_clk_create(dev);
>>> +       if (error) {
>>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>>> +               return error;
>>> +       }
>>> +
>>> +       i = 0;
>>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>
>> This loop adds all available device clocks to the PM clock list. I
>> wonder if this could be considered as a common thing and if so, we
>> might want to extend the pm_clk API with this.
>
>
> There are several reasons as follows:
>
>     Firstly, the clocks need be turned off to save power when
>     the system enter the suspend state. So we need to enumerate the clocks
>     in the dts. In order to power domain can turn on and off.
>
>     Secondly, the reset-circuit should reset be synchronous on rk3288, then
> sync revoked.
>     So we need to enable clocks of all devices.

I think you misinterpreted my previous answer. Instead of fetching all
clocks for a device and manually create the pm_clk list as you do
here, I was suggesting to make this a part of the pm clk API.

I would happily support such an API, but I can't tell what the other
maintainers think.

>
>>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>>> +                       __clk_get_name(clk));
>>> +               error = pm_clk_add_clk(dev, clk);
>>> +               clk_put(clk);
>>> +               if (error) {
>>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n",
>>> error);
>>> +                       pm_clk_destroy(dev);
>>> +                       return error;
>>> +               }
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +

[...]

>>> +
>>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>>> +                                     struct device_node *node)
>>> +{
>>> +       const struct rockchip_domain_info *pd_info;
>>> +       struct rockchip_pm_domain *pd;
>>> +       struct clk *clk;
>>> +       int clk_cnt;
>>> +       int i;
>>> +       u32 id;
>>> +       int error;
>>> +
>>> +       error = of_property_read_u32(node, "reg", &id);
>>> +       if (error) {
>>> +               dev_err(pmu->dev,
>>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>>> +                       node->name, error);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       if (id >= pmu->info->num_domains) {
>>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>>> +                       node->name, id);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       pd_info = &pmu->info->domain_info[id];
>>> +       if (!pd_info) {
>>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>>> +                       node->name, id);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       clk_cnt = of_count_phandle_with_args(node, "clocks",
>>> "#clock-cells");
>>> +       pd = devm_kzalloc(pmu->dev,
>>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>>> +                         GFP_KERNEL);
>>> +       if (!pd)
>>> +               return -ENOMEM;
>>> +
>>> +       pd->info = pd_info;
>>> +       pd->pmu = pmu;
>>> +
>>> +       for (i = 0; i < clk_cnt; i++) {
>>
>> This loop is similar to the one when creates the PM clock list in the
>> rockchip_pd_attach_dev().
>>
>> What's the reason you don't want to use pm clk for these clocks?
>>
>
> Ditto.

I don't follow. Can't you do the exact same thing via the pm clk API,
can you please elaborate!?

>
>>> +               clk = of_clk_get(node, i);
>>> +               if (IS_ERR(clk)) {
>>> +                       error = PTR_ERR(clk);
>>> +                       dev_err(pmu->dev,
>>> +                               "%s: failed to get clk %s (index %d):
>>> %d\n",
>>> +                               node->name, __clk_get_name(clk), i,
>>> error);
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               error = clk_prepare(clk);
>>> +               if (error) {
>>> +                       dev_err(pmu->dev,
>>> +                               "%s: failed to prepare clk %s (index %d):
>>> %d\n",
>>> +                               node->name, __clk_get_name(clk), i,
>>> error);
>>> +                       clk_put(clk);
>>> +                       goto err_out;
>>> +               }
>>> +
>>> +               pd->clks[pd->num_clks++] = clk;
>>> +
>>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>>> +                       __clk_get_name(clk), node->name);
>>> +       }
>>> +
>>> +       error = rockchip_pd_power(pd, true);
>>> +       if (error) {
>>> +               dev_err(pmu->dev,
>>> +                       "failed to power on domain '%s': %d\n",
>>> +                       node->name, error);
>>> +               goto err_out;
>>> +       }
>>> +
>>> +       pd->genpd.name = node->name;
>>> +       pd->genpd.power_off = rockchip_pd_power_off;
>>> +       pd->genpd.power_on = rockchip_pd_power_on;
>>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>>
>> Instead of assigning the ->stop|start() callbacks, which do
>> pm_clk_suspend|resume(), just set the genpd->flags to
>> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>>
> Ditto.

Again, I don't follow. Here is what goes on:

rockchip_pd_start_dev() calls pm_clk_resume() and
rockchip_pd_stop_dev() calls pm_clk_suspend().
Instead of assigning the below callbacks...

pd->genpd.dev_ops.start = rockchip_pd_start_dev;
pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;

... just use GENPD_FLAG_PM_CLK and let genpd deal with the calls to
pm_clk_suspend|resume().

>
>
>>> +       pm_genpd_init(&pd->genpd, NULL, false);
>>> +
>>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>>> +       return 0;
>>> +
>>> +err_out:
>>> +       while (--i >= 0) {
>>> +               clk_unprepare(pd->clks[i]);
>>> +               clk_put(pd->clks[i]);
>>> +       }
>>> +       return error;
>>> +}
>>> +

[...]

Kind regards
Uffe

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Caesar Wang]

Hi Uffe,

在 2015年06月25日 23:33, Ulf Hansson 写道:
> [...]
>
>>>> +#include <linux/clk-provider.h>
>>> clk-provider.h, why?
>>
>> The following is needed.
>>
>> _clk_get_name(clk)
> I see, you need it for for the dev_dbg().
>
> I think you shall use "%pC" as the formatting string for the dev_dbg()
> message, since that will take care of printing the clock name for you.

Yes, the "%pC" can do the similar thing.
Done, fixed in next patch.

> [...]
>
>>>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool
>>>> power_on)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       mutex_lock(&pd->pmu->mutex);
>>> I don't quite understand why you need a lock, what are you protecting and
>>> why?
>>
>> Says:
>> [ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3551.919730] rockchip_pd_power:139: mutex_lock
>> [ 3551.924130] rockchip_pd_power:167,mutex_unlock
>> [ 3563.827295] rockchip_pd_power:139: mutex_lock
>> [ 3563.831753] rockchip_pd_power:167,mutex_unlock
>> [ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3564.079047] rockchip_pd_power:139: mutex_lock
>> [ 3564.083426] rockchip_pd_power:167,mutex_unlock
>> [ 3611.665726] rockchip_pd_power:139: mutex_lock
>> [ 3611.670206] rockchip_pd_power:167,mutex_unlock
>> [ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3611.919689] rockchip_pd_power:139: mutex_lock
>> [ 3611.924090] rockchip_pd_power:167,mutex_unlock
>> [ 3623.848296] rockchip_pd_power:139: mutex_lock
>> [ 3623.852752] rockchip_pd_power:167,mutex_unlock
>>
>>
>>
>>
>>>> +
>>>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>>>> +               for (i = 0; i < pd->num_clks; i++)
>>>> +                       clk_enable(pd->clks[i]);
>>>> +
>>>> +               if (!power_on) {
>>>> +                       /* FIXME: add code to save AXI_QOS */
>>>> +
>>>> +                       /* if powering down, idle request to NIU first */
>>>> +                       rockchip_pmu_set_idle_request(pd, true);
>>>> +               }
>>>> +
>>>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>>>> +
>>>> +               if (power_on) {
>>>> +                       /* if powering up, leave idle mode */
>>>> +                       rockchip_pmu_set_idle_request(pd, false);
>>>> +
>>>> +                       /* FIXME: add code to restore AXI_QOS */
>>>> +               }
>>>> +
>>>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>>>> +                       clk_disable(pd->clks[i]);
>>>> +       }
>>>> +
>>>> +       mutex_unlock(&pd->pmu->mutex);
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>>> +{
>>>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>>>> +
>>>> +       return rockchip_pd_power(pd, false);
>>>> +}
>>>> +
>>>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>>>> +                                 struct device *dev)
>>>> +{
>>>> +       struct clk *clk;
>>>> +       int i;
>>>> +       int error;
>>>> +
>>>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>>>> +
>>>> +       error = pm_clk_create(dev);
>>>> +       if (error) {
>>>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>>>> +               return error;
>>>> +       }
>>>> +
>>>> +       i = 0;
>>>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>> This loop adds all available device clocks to the PM clock list. I
>>> wonder if this could be considered as a common thing and if so, we
>>> might want to extend the pm_clk API with this.
>>
>> There are several reasons as follows:
>>
>>      Firstly, the clocks need be turned off to save power when
>>      the system enter the suspend state. So we need to enumerate the clocks
>>      in the dts. In order to power domain can turn on and off.
>>
>>      Secondly, the reset-circuit should reset be synchronous on rk3288, then
>> sync revoked.
>>      So we need to enable clocks of all devices.
> I think you misinterpreted my previous answer. Instead of fetching all
> clocks for a device and manually create the pm_clk list as you do
> here, I was suggesting to make this a part of the pm clk API.
>
> I would happily support such an API, but I can't tell what the other
> maintainers think.

Sorry, this is my misunderstanding.

Here, I agree your point. I will glad to use it if you support such an API.
I will send patch v16 with it if you have the implementation code.

>>>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>>>> +                       __clk_get_name(clk));
>>>> +               error = pm_clk_add_clk(dev, clk);
>>>> +               clk_put(clk);
>>>> +               if (error) {
>>>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n",
>>>> error);
>>>> +                       pm_clk_destroy(dev);
>>>> +                       return error;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
> [...]
>
>>>> +
>>>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>>>> +                                     struct device_node *node)
>>>> +{
>>>> +       const struct rockchip_domain_info *pd_info;
>>>> +       struct rockchip_pm_domain *pd;
>>>> +       struct clk *clk;
>>>> +       int clk_cnt;
>>>> +       int i;
>>>> +       u32 id;
>>>> +       int error;
>>>> +
>>>> +       error = of_property_read_u32(node, "reg", &id);
>>>> +       if (error) {
>>>> +               dev_err(pmu->dev,
>>>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>>>> +                       node->name, error);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       if (id >= pmu->info->num_domains) {
>>>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>>>> +                       node->name, id);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       pd_info = &pmu->info->domain_info[id];
>>>> +       if (!pd_info) {
>>>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>>>> +                       node->name, id);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       clk_cnt = of_count_phandle_with_args(node, "clocks",
>>>> "#clock-cells");
>>>> +       pd = devm_kzalloc(pmu->dev,
>>>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>>>> +                         GFP_KERNEL);
>>>> +       if (!pd)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       pd->info = pd_info;
>>>> +       pd->pmu = pmu;
>>>> +
>>>> +       for (i = 0; i < clk_cnt; i++) {
>>> This loop is similar to the one when creates the PM clock list in the
>>> rockchip_pd_attach_dev().
>>>
>>> What's the reason you don't want to use pm clk for these clocks?
>>>
>> Ditto.
> I don't follow. Can't you do the exact same thing via the pm clk API,
> can you please elaborate!?
>

Although I'm glad to use the pm clk API, something to prevent that.

As perivous versions told about:

At the moment, all the device clocks being listed in the power-domains 
itself,
I know someone wish was to get the clocks by reading the clocks from the 
device nodes,

I think reading the clocks from the devices if we via the pm clk API.
I concerned that we can't ensure the consumption enough good during the 
suspend state.

Meanwhile, as the pervious patch v7 said:

     the clocks need be turned off to save power when the system enter 
the suspend state.
So we need to enumerate the clocks in the dts. In order to power domain 
can turn on and off.

     the reset-circuit should reset be synchronous on rk3288, then sync 
revoked.
So we need to enable clocks of all devices.

>>>> +               clk = of_clk_get(node, i);
>>>> +               if (IS_ERR(clk)) {
>>>> +                       error = PTR_ERR(clk);
>>>> +                       dev_err(pmu->dev,
>>>> +                               "%s: failed to get clk %s (index %d):
>>>> %d\n",
>>>> +                               node->name, __clk_get_name(clk), i,
>>>> error);
>>>> +                       goto err_out;
>>>> +               }
>>>> +
>>>> +               error = clk_prepare(clk);
>>>> +               if (error) {
>>>> +                       dev_err(pmu->dev,
>>>> +                               "%s: failed to prepare clk %s (index %d):
>>>> %d\n",
>>>> +                               node->name, __clk_get_name(clk), i,
>>>> error);
>>>> +                       clk_put(clk);
>>>> +                       goto err_out;
>>>> +               }
>>>> +
>>>> +               pd->clks[pd->num_clks++] = clk;
>>>> +
>>>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>>>> +                       __clk_get_name(clk), node->name);
>>>> +       }
>>>> +
>>>> +       error = rockchip_pd_power(pd, true);
>>>> +       if (error) {
>>>> +               dev_err(pmu->dev,
>>>> +                       "failed to power on domain '%s': %d\n",
>>>> +                       node->name, error);
>>>> +               goto err_out;
>>>> +       }
>>>> +
>>>> +       pd->genpd.name = node->name;
>>>> +       pd->genpd.power_off = rockchip_pd_power_off;
>>>> +       pd->genpd.power_on = rockchip_pd_power_on;
>>>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>>>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>>>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>>>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>>> Instead of assigning the ->stop|start() callbacks, which do
>>> pm_clk_suspend|resume(), just set the genpd->flags to
>>> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>>>
>> Ditto.
> Again, I don't follow. Here is what goes on:
>
> rockchip_pd_start_dev() calls pm_clk_resume() and
> rockchip_pd_stop_dev() calls pm_clk_suspend().
> Instead of assigning the below callbacks...
>
> pd->genpd.dev_ops.start = rockchip_pd_start_dev;
> pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>
> ... just use GENPD_FLAG_PM_CLK and let genpd deal with the calls to
> pm_clk_suspend|resume().

Yes, you are right.
Done, fixed in next patch v16.

Thanks!

Caesar
>>
>>>> +       pm_genpd_init(&pd->genpd, NULL, false);
>>>> +
>>>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>>>> +       return 0;
>>>> +
>>>> +err_out:
>>>> +       while (--i >= 0) {
>>>> +               clk_unprepare(pd->clks[i]);
>>>> +               clk_put(pd->clks[i]);
>>>> +       }
>>>> +       return error;
>>>> +}
>>>> +
> [...]
>
> Kind regards
> Uffe
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Caesar Wang]

Hi Uffe,

在 2015年06月25日 23:33, Ulf Hansson 写道:
> [...]
>
>>>> +#include <linux/clk-provider.h>
>>> clk-provider.h, why?
>>
>> The following is needed.
>>
>> _clk_get_name(clk)
> I see, you need it for for the dev_dbg().
>
> I think you shall use "%pC" as the formatting string for the dev_dbg()
> message, since that will take care of printing the clock name for you.

Yes, the "%pC" can do the similar thing.
Done, fixed in next patch.

> [...]
>
>>>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool
>>>> power_on)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       mutex_lock(&pd->pmu->mutex);
>>> I don't quite understand why you need a lock, what are you protecting and
>>> why?
>>
>> Says:
>> [ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3551.919730] rockchip_pd_power:139: mutex_lock
>> [ 3551.924130] rockchip_pd_power:167,mutex_unlock
>> [ 3563.827295] rockchip_pd_power:139: mutex_lock
>> [ 3563.831753] rockchip_pd_power:167,mutex_unlock
>> [ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3564.079047] rockchip_pd_power:139: mutex_lock
>> [ 3564.083426] rockchip_pd_power:167,mutex_unlock
>> [ 3611.665726] rockchip_pd_power:139: mutex_lock
>> [ 3611.670206] rockchip_pd_power:167,mutex_unlock
>> [ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3611.919689] rockchip_pd_power:139: mutex_lock
>> [ 3611.924090] rockchip_pd_power:167,mutex_unlock
>> [ 3623.848296] rockchip_pd_power:139: mutex_lock
>> [ 3623.852752] rockchip_pd_power:167,mutex_unlock
>>
>>
>>
>>
>>>> +
>>>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>>>> +               for (i = 0; i < pd->num_clks; i++)
>>>> +                       clk_enable(pd->clks[i]);
>>>> +
>>>> +               if (!power_on) {
>>>> +                       /* FIXME: add code to save AXI_QOS */
>>>> +
>>>> +                       /* if powering down, idle request to NIU first */
>>>> +                       rockchip_pmu_set_idle_request(pd, true);
>>>> +               }
>>>> +
>>>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>>>> +
>>>> +               if (power_on) {
>>>> +                       /* if powering up, leave idle mode */
>>>> +                       rockchip_pmu_set_idle_request(pd, false);
>>>> +
>>>> +                       /* FIXME: add code to restore AXI_QOS */
>>>> +               }
>>>> +
>>>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>>>> +                       clk_disable(pd->clks[i]);
>>>> +       }
>>>> +
>>>> +       mutex_unlock(&pd->pmu->mutex);
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>>> +{
>>>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>>>> +
>>>> +       return rockchip_pd_power(pd, false);
>>>> +}
>>>> +
>>>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>>>> +                                 struct device *dev)
>>>> +{
>>>> +       struct clk *clk;
>>>> +       int i;
>>>> +       int error;
>>>> +
>>>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>>>> +
>>>> +       error = pm_clk_create(dev);
>>>> +       if (error) {
>>>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>>>> +               return error;
>>>> +       }
>>>> +
>>>> +       i = 0;
>>>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>> This loop adds all available device clocks to the PM clock list. I
>>> wonder if this could be considered as a common thing and if so, we
>>> might want to extend the pm_clk API with this.
>>
>> There are several reasons as follows:
>>
>>      Firstly, the clocks need be turned off to save power when
>>      the system enter the suspend state. So we need to enumerate the clocks
>>      in the dts. In order to power domain can turn on and off.
>>
>>      Secondly, the reset-circuit should reset be synchronous on rk3288, then
>> sync revoked.
>>      So we need to enable clocks of all devices.
> I think you misinterpreted my previous answer. Instead of fetching all
> clocks for a device and manually create the pm_clk list as you do
> here, I was suggesting to make this a part of the pm clk API.
>
> I would happily support such an API, but I can't tell what the other
> maintainers think.

Sorry, this is my misunderstanding.

Here, I agree your point. I will glad to use it if you support such an API.
I will send patch v16 with it if you have the implementation code.

>>>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>>>> +                       __clk_get_name(clk));
>>>> +               error = pm_clk_add_clk(dev, clk);
>>>> +               clk_put(clk);
>>>> +               if (error) {
>>>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n",
>>>> error);
>>>> +                       pm_clk_destroy(dev);
>>>> +                       return error;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
> [...]
>
>>>> +
>>>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>>>> +                                     struct device_node *node)
>>>> +{
>>>> +       const struct rockchip_domain_info *pd_info;
>>>> +       struct rockchip_pm_domain *pd;
>>>> +       struct clk *clk;
>>>> +       int clk_cnt;
>>>> +       int i;
>>>> +       u32 id;
>>>> +       int error;
>>>> +
>>>> +       error = of_property_read_u32(node, "reg", &id);
>>>> +       if (error) {
>>>> +               dev_err(pmu->dev,
>>>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>>>> +                       node->name, error);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       if (id >= pmu->info->num_domains) {
>>>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>>>> +                       node->name, id);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       pd_info = &pmu->info->domain_info[id];
>>>> +       if (!pd_info) {
>>>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>>>> +                       node->name, id);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       clk_cnt = of_count_phandle_with_args(node, "clocks",
>>>> "#clock-cells");
>>>> +       pd = devm_kzalloc(pmu->dev,
>>>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>>>> +                         GFP_KERNEL);
>>>> +       if (!pd)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       pd->info = pd_info;
>>>> +       pd->pmu = pmu;
>>>> +
>>>> +       for (i = 0; i < clk_cnt; i++) {
>>> This loop is similar to the one when creates the PM clock list in the
>>> rockchip_pd_attach_dev().
>>>
>>> What's the reason you don't want to use pm clk for these clocks?
>>>
>> Ditto.
> I don't follow. Can't you do the exact same thing via the pm clk API,
> can you please elaborate!?
>

Although I'm glad to use the pm clk API, something to prevent that.

As perivous versions told about:

At the moment, all the device clocks being listed in the power-domains 
itself,
I know someone wish was to get the clocks by reading the clocks from the 
device nodes,

I think reading the clocks from the devices if we via the pm clk API.
I concerned that we can't ensure the consumption enough good during the 
suspend state.

Meanwhile, as the pervious patch v7 said:

     the clocks need be turned off to save power when the system enter 
the suspend state.
So we need to enumerate the clocks in the dts. In order to power domain 
can turn on and off.

     the reset-circuit should reset be synchronous on rk3288, then sync 
revoked.
So we need to enable clocks of all devices.

>>>> +               clk = of_clk_get(node, i);
>>>> +               if (IS_ERR(clk)) {
>>>> +                       error = PTR_ERR(clk);
>>>> +                       dev_err(pmu->dev,
>>>> +                               "%s: failed to get clk %s (index %d):
>>>> %d\n",
>>>> +                               node->name, __clk_get_name(clk), i,
>>>> error);
>>>> +                       goto err_out;
>>>> +               }
>>>> +
>>>> +               error = clk_prepare(clk);
>>>> +               if (error) {
>>>> +                       dev_err(pmu->dev,
>>>> +                               "%s: failed to prepare clk %s (index %d):
>>>> %d\n",
>>>> +                               node->name, __clk_get_name(clk), i,
>>>> error);
>>>> +                       clk_put(clk);
>>>> +                       goto err_out;
>>>> +               }
>>>> +
>>>> +               pd->clks[pd->num_clks++] = clk;
>>>> +
>>>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>>>> +                       __clk_get_name(clk), node->name);
>>>> +       }
>>>> +
>>>> +       error = rockchip_pd_power(pd, true);
>>>> +       if (error) {
>>>> +               dev_err(pmu->dev,
>>>> +                       "failed to power on domain '%s': %d\n",
>>>> +                       node->name, error);
>>>> +               goto err_out;
>>>> +       }
>>>> +
>>>> +       pd->genpd.name = node->name;
>>>> +       pd->genpd.power_off = rockchip_pd_power_off;
>>>> +       pd->genpd.power_on = rockchip_pd_power_on;
>>>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>>>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>>>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>>>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>>> Instead of assigning the ->stop|start() callbacks, which do
>>> pm_clk_suspend|resume(), just set the genpd->flags to
>>> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>>>
>> Ditto.
> Again, I don't follow. Here is what goes on:
>
> rockchip_pd_start_dev() calls pm_clk_resume() and
> rockchip_pd_stop_dev() calls pm_clk_suspend().
> Instead of assigning the below callbacks...
>
> pd->genpd.dev_ops.start = rockchip_pd_start_dev;
> pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>
> ... just use GENPD_FLAG_PM_CLK and let genpd deal with the calls to
> pm_clk_suspend|resume().

Yes, you are right.
Done, fixed in next patch v16.

Thanks!

Caesar
>>
>>>> +       pm_genpd_init(&pd->genpd, NULL, false);
>>>> +
>>>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>>>> +       return 0;
>>>> +
>>>> +err_out:
>>>> +       while (--i >= 0) {
>>>> +               clk_unprepare(pd->clks[i]);
>>>> +               clk_put(pd->clks[i]);
>>>> +       }
>>>> +       return error;
>>>> +}
>>>> +
> [...]
>
> Kind regards
> Uffe
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v14 2/3] power-domain: rockchip: add power domain driver

[Caesar Wang]

Hi Uffe,

? 2015?06?25? 23:33, Ulf Hansson ??:
> [...]
>
>>>> +#include <linux/clk-provider.h>
>>> clk-provider.h, why?
>>
>> The following is needed.
>>
>> _clk_get_name(clk)
> I see, you need it for for the dev_dbg().
>
> I think you shall use "%pC" as the formatting string for the dev_dbg()
> message, since that will take care of printing the clock name for you.

Yes, the "%pC" can do the similar thing.
Done, fixed in next patch.

> [...]
>
>>>> +static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool
>>>> power_on)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       mutex_lock(&pd->pmu->mutex);
>>> I don't quite understand why you need a lock, what are you protecting and
>>> why?
>>
>> Says:
>> [ 3551.678762] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3551.899180] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3551.905958] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3551.912832] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3551.919730] rockchip_pd_power:139: mutex_lock
>> [ 3551.924130] rockchip_pd_power:167,mutex_unlock
>> [ 3563.827295] rockchip_pd_power:139: mutex_lock
>> [ 3563.831753] rockchip_pd_power:167,mutex_unlock
>> [ 3563.836216] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3564.058989] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3564.065659] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3564.072354] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3564.079047] rockchip_pd_power:139: mutex_lock
>> [ 3564.083426] rockchip_pd_power:167,mutex_unlock
>> [ 3611.665726] rockchip_pd_power:139: mutex_lock
>> [ 3611.670206] rockchip_pd_power:167,mutex_unlock
>> [ 3611.674692] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3611.899160] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3611.905938] mali ffa30000.gpu: starting device in power domain 'pd_gpu'
>> [ 3611.912818] mali ffa30000.gpu: stopping device in power domain 'pd_gpu'
>> [ 3611.919689] rockchip_pd_power:139: mutex_lock
>> [ 3611.924090] rockchip_pd_power:167,mutex_unlock
>> [ 3623.848296] rockchip_pd_power:139: mutex_lock
>> [ 3623.852752] rockchip_pd_power:167,mutex_unlock
>>
>>
>>
>>
>>>> +
>>>> +       if (rockchip_pmu_domain_is_on(pd) != power_on) {
>>>> +               for (i = 0; i < pd->num_clks; i++)
>>>> +                       clk_enable(pd->clks[i]);
>>>> +
>>>> +               if (!power_on) {
>>>> +                       /* FIXME: add code to save AXI_QOS */
>>>> +
>>>> +                       /* if powering down, idle request to NIU first */
>>>> +                       rockchip_pmu_set_idle_request(pd, true);
>>>> +               }
>>>> +
>>>> +               rockchip_do_pmu_set_power_domain(pd, power_on);
>>>> +
>>>> +               if (power_on) {
>>>> +                       /* if powering up, leave idle mode */
>>>> +                       rockchip_pmu_set_idle_request(pd, false);
>>>> +
>>>> +                       /* FIXME: add code to restore AXI_QOS */
>>>> +               }
>>>> +
>>>> +               for (i = pd->num_clks - 1; i >= 0; i--)
>>>> +                       clk_disable(pd->clks[i]);
>>>> +       }
>>>> +
>>>> +       mutex_unlock(&pd->pmu->mutex);
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_pd_power_on(struct generic_pm_domain *domain)
>>>> +{
>>>> +       struct rockchip_pm_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_pm_domain *pd = to_rockchip_pd(domain);
>>>> +
>>>> +       return rockchip_pd_power(pd, false);
>>>> +}
>>>> +
>>>> +static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>>>> +                                 struct device *dev)
>>>> +{
>>>> +       struct clk *clk;
>>>> +       int i;
>>>> +       int error;
>>>> +
>>>> +       dev_dbg(dev, "attaching to power domain '%s'\n", genpd->name);
>>>> +
>>>> +       error = pm_clk_create(dev);
>>>> +       if (error) {
>>>> +               dev_err(dev, "pm_clk_create failed %d\n", error);
>>>> +               return error;
>>>> +       }
>>>> +
>>>> +       i = 0;
>>>> +       while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
>>> This loop adds all available device clocks to the PM clock list. I
>>> wonder if this could be considered as a common thing and if so, we
>>> might want to extend the pm_clk API with this.
>>
>> There are several reasons as follows:
>>
>>      Firstly, the clocks need be turned off to save power when
>>      the system enter the suspend state. So we need to enumerate the clocks
>>      in the dts. In order to power domain can turn on and off.
>>
>>      Secondly, the reset-circuit should reset be synchronous on rk3288, then
>> sync revoked.
>>      So we need to enable clocks of all devices.
> I think you misinterpreted my previous answer. Instead of fetching all
> clocks for a device and manually create the pm_clk list as you do
> here, I was suggesting to make this a part of the pm clk API.
>
> I would happily support such an API, but I can't tell what the other
> maintainers think.

Sorry, this is my misunderstanding.

Here, I agree your point. I will glad to use it if you support such an API.
I will send patch v16 with it if you have the implementation code.

>>>> +               dev_dbg(dev, "adding clock '%s' to list of PM clocks\n",
>>>> +                       __clk_get_name(clk));
>>>> +               error = pm_clk_add_clk(dev, clk);
>>>> +               clk_put(clk);
>>>> +               if (error) {
>>>> +                       dev_err(dev, "pm_clk_add_clk failed %d\n",
>>>> error);
>>>> +                       pm_clk_destroy(dev);
>>>> +                       return error;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
> [...]
>
>>>> +
>>>> +static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
>>>> +                                     struct device_node *node)
>>>> +{
>>>> +       const struct rockchip_domain_info *pd_info;
>>>> +       struct rockchip_pm_domain *pd;
>>>> +       struct clk *clk;
>>>> +       int clk_cnt;
>>>> +       int i;
>>>> +       u32 id;
>>>> +       int error;
>>>> +
>>>> +       error = of_property_read_u32(node, "reg", &id);
>>>> +       if (error) {
>>>> +               dev_err(pmu->dev,
>>>> +                       "%s: failed to retrieve domain id (reg): %d\n",
>>>> +                       node->name, error);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       if (id >= pmu->info->num_domains) {
>>>> +               dev_err(pmu->dev, "%s: invalid domain id %d\n",
>>>> +                       node->name, id);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       pd_info = &pmu->info->domain_info[id];
>>>> +       if (!pd_info) {
>>>> +               dev_err(pmu->dev, "%s: undefined domain id %d\n",
>>>> +                       node->name, id);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       clk_cnt = of_count_phandle_with_args(node, "clocks",
>>>> "#clock-cells");
>>>> +       pd = devm_kzalloc(pmu->dev,
>>>> +                         sizeof(*pd) + clk_cnt * sizeof(pd->clks[0]),
>>>> +                         GFP_KERNEL);
>>>> +       if (!pd)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       pd->info = pd_info;
>>>> +       pd->pmu = pmu;
>>>> +
>>>> +       for (i = 0; i < clk_cnt; i++) {
>>> This loop is similar to the one when creates the PM clock list in the
>>> rockchip_pd_attach_dev().
>>>
>>> What's the reason you don't want to use pm clk for these clocks?
>>>
>> Ditto.
> I don't follow. Can't you do the exact same thing via the pm clk API,
> can you please elaborate!?
>

Although I'm glad to use the pm clk API, something to prevent that.

As perivous versions told about:

At the moment, all the device clocks being listed in the power-domains 
itself,
I know someone wish was to get the clocks by reading the clocks from the 
device nodes,

I think reading the clocks from the devices if we via the pm clk API.
I concerned that we can't ensure the consumption enough good during the 
suspend state.

Meanwhile, as the pervious patch v7 said:

     the clocks need be turned off to save power when the system enter 
the suspend state.
So we need to enumerate the clocks in the dts. In order to power domain 
can turn on and off.

     the reset-circuit should reset be synchronous on rk3288, then sync 
revoked.
So we need to enable clocks of all devices.

>>>> +               clk = of_clk_get(node, i);
>>>> +               if (IS_ERR(clk)) {
>>>> +                       error = PTR_ERR(clk);
>>>> +                       dev_err(pmu->dev,
>>>> +                               "%s: failed to get clk %s (index %d):
>>>> %d\n",
>>>> +                               node->name, __clk_get_name(clk), i,
>>>> error);
>>>> +                       goto err_out;
>>>> +               }
>>>> +
>>>> +               error = clk_prepare(clk);
>>>> +               if (error) {
>>>> +                       dev_err(pmu->dev,
>>>> +                               "%s: failed to prepare clk %s (index %d):
>>>> %d\n",
>>>> +                               node->name, __clk_get_name(clk), i,
>>>> error);
>>>> +                       clk_put(clk);
>>>> +                       goto err_out;
>>>> +               }
>>>> +
>>>> +               pd->clks[pd->num_clks++] = clk;
>>>> +
>>>> +               dev_dbg(pmu->dev, "added clock '%s' to domain '%s'\n",
>>>> +                       __clk_get_name(clk), node->name);
>>>> +       }
>>>> +
>>>> +       error = rockchip_pd_power(pd, true);
>>>> +       if (error) {
>>>> +               dev_err(pmu->dev,
>>>> +                       "failed to power on domain '%s': %d\n",
>>>> +                       node->name, error);
>>>> +               goto err_out;
>>>> +       }
>>>> +
>>>> +       pd->genpd.name = node->name;
>>>> +       pd->genpd.power_off = rockchip_pd_power_off;
>>>> +       pd->genpd.power_on = rockchip_pd_power_on;
>>>> +       pd->genpd.attach_dev = rockchip_pd_attach_dev;
>>>> +       pd->genpd.detach_dev = rockchip_pd_detach_dev;
>>>> +       pd->genpd.dev_ops.start = rockchip_pd_start_dev;
>>>> +       pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>>> Instead of assigning the ->stop|start() callbacks, which do
>>> pm_clk_suspend|resume(), just set the genpd->flags to
>>> GENPD_FLAG_PM_CLK, and genpd will fix this for you.
>>>
>> Ditto.
> Again, I don't follow. Here is what goes on:
>
> rockchip_pd_start_dev() calls pm_clk_resume() and
> rockchip_pd_stop_dev() calls pm_clk_suspend().
> Instead of assigning the below callbacks...
>
> pd->genpd.dev_ops.start = rockchip_pd_start_dev;
> pd->genpd.dev_ops.stop = rockchip_pd_stop_dev;
>
> ... just use GENPD_FLAG_PM_CLK and let genpd deal with the calls to
> pm_clk_suspend|resume().

Yes, you are right.
Done, fixed in next patch v16.

Thanks!

Caesar
>>
>>>> +       pm_genpd_init(&pd->genpd, NULL, false);
>>>> +
>>>> +       pmu->genpd_data.domains[id] = &pd->genpd;
>>>> +       return 0;
>>>> +
>>>> +err_out:
>>>> +       while (--i >= 0) {
>>>> +               clk_unprepare(pd->clks[i]);
>>>> +               clk_put(pd->clks[i]);
>>>> +       }
>>>> +       return error;
>>>> +}
>>>> +
> [...]
>
> Kind regards
> Uffe
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip