LinuxLists.cc - [RESEND PATCH v16 0/4] ARM: rk3288: Add PM Domain support

[permalink] [raw]

Subject: [RESEND PATCH v16 1/4] dt-bindings: add document of Rockchip power domain

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

Signed-off-by: jinkun.hong <[email protected]>
Signed-off-by: Caesar Wang <[email protected]>

---

Changes in v16:
- remove the pmu node.

Changes in v15: None
Changes in v14: None
Changes in v13: None
Changes in v12: None
Changes in v11: None
Changes in v10: None
Changes in v9:
- add document decription.

Changes in v8:
- document go back to v2.

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

Changes in v2:
- move clocks to "optional".

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

diff --git a/Documentation/devicetree/bindings/soc/rockchip/power_domain.txt b/Documentation/devicetree/bindings/soc/rockchip/power_domain.txt
new file mode 100644
index 0000000..138754d
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/rockchip/power_domain.txt
@@ -0,0 +1,46 @@
+* Rockchip Power Domains
+
+Rockchip processors include support for multiple power domains which can be
+powered up/down by software based on different application scenes to save power.
+
+Required properties for power domain controller:
+- compatible: Should be one of the following.
+ "rockchip,rk3288-power-controller" - for RK3288 SoCs.
+- #power-domain-cells: Number of cells in a power-domain specifier.
+ Should be 1 for multiple PM domains.
+- #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>;
+ #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

2015-08-25 07:25:54

[permalink] [raw]

Subject: [RESEND PATCH v16 2/4] ARM: power-domain: rockchip: add all the domain type on RK3288 SoCs

According to a description from TRM, the following table lists
all the power domains.

------------------------ -------------------------
|VD_CORE | |VD_LOGIC | PD_VIO |
|xxxxxxxxxxxxxxxxxxxx | |xxxxxxxx -----------|
|xxxxxxxxxxxxxxxxxxxx | |xxxxxxxx |
|xxxxxxxxxxxxxxxxxxxx | | |
|xxxxxxxxxxxxxxxxxxxx | |--------- -----------|
| | |PD_PERI | | PD_VIDEO |
| | |--------- -----------|
| | |--------- -----------|
| | |PD_ALIVE | | PD_HEVC |
|---------- ---------- | |--------- -----------|
||PD_A17_1| |PD_A17_2| | -------------------------
|---------- ---------- | |VD_GPU |PD_GPU |
|---------- ---------- | -------------------------
||PD_A17_3| |PD_DEBUG| | -------------------------
|---------- ---------- | |VD_PMU |PD_PMU |
------------------------ -------------------------

VD_* : voltage domain
PD_* : power domain

At the moment, we can support some power-domain type on RK3288.
We can add more types on RK3288 in the future, that's need to do.

Signed-off-by: Caesar Wang <[email protected]>

---

Changes in v16:
- Add more domain decription.

Changes in v15:
- change the comment.

Changes in v14: None
Changes in v13: None
Changes in v12: None
Changes in v11: None
Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

include/dt-bindings/power-domain/rk3288.h | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 include/dt-bindings/power-domain/rk3288.h

diff --git a/include/dt-bindings/power-domain/rk3288.h b/include/dt-bindings/power-domain/rk3288.h
new file mode 100644
index 0000000..db5e810
--- /dev/null
+++ b/include/dt-bindings/power-domain/rk3288.h
@@ -0,0 +1,31 @@
+#ifndef __DT_BINDINGS_POWER_DOMAIN_RK3288_H__
+#define __DT_BINDINGS_POWER_DOMAIN_RK3288_H__
+
+/**
+ * RK3288 Power Domain and Voltage Domain Summary.
+ */
+
+/* VD_CORE */
+#define RK3288_PD_A17_0 0
+#define RK3288_PD_A17_1 1
+#define RK3288_PD_A17_2 2
+#define RK3288_PD_A17_3 3
+#define RK3288_PD_SCU 4
+#define RK3288_PD_DEBUG 5
+#define RK3288_PD_MEM 6
+
+/* VD_LOGIC */
+#define RK3288_PD_BUS 7
+#define RK3288_PD_PERI 8
+#define RK3288_PD_VIO 9
+#define RK3288_PD_ALIVE 10
+#define RK3288_PD_HEVC 11
+#define RK3288_PD_VIDEO 12
+
+/* VD_GPU */
+#define RK3288_PD_GPU 13
+
+/* VD_PMU */
+#define RK3288_PD_PMU 14
+
+#endif
--
1.9.1

2015-08-25 07:26:05

[permalink] [raw]

Subject: [RESEND PATCH v16 3/4] soc: rockchip: power-domain: Add power domain driver

This driver is found on RK3288 SoCs.

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 of the whole chip.

PMU can work in the Low Power Mode by setting bit[0] of PMU_PWRMODE_CON
register. After setting the register, PMU would enter the Low Power mode.
In the low power mode, pmu will auto power on/off the specified power
domain, send idle req to specified power domain, shut down/up pll and
so on. All of above are configurable by setting corresponding registers.

Signed-off-by: jinkun.hong <[email protected]>
Signed-off-by: Caesar Wang <[email protected]>

---

Changes in v16:
- the driver type from tristate to bool.
- Letter misspelled.
- As Ulf suggestion, remove #include <linux/clk-provider.h>,
use "%pC" as the formatting string for the dev_dbg().
- As the Ulf suggestion, set the genpd->flags to assign the
->stop|start() callbacks.

Changes in v15:
- As the kevin suggestion, put the power-domain driver into driver/soc/vendor.
- As Heiko suggestion, Patch 1: binding doc, 2: binding-header, 3: driver,
4: dts-changes.
- return -ENXIO --> return -ENODEV.

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 powermanagment 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:
- fix the title doamin->domain.
- updated device_attach and device_detach API,otherwise it will
compile fail on next kernel.

Changes in v11:
- fix pm_genpd_init(&pd->genpd, NULL, false).

Changes in v10:
- this switches over domain infos to use masks instead of recomputing
them each time and also gets rid of custom domain translator and
uses standard onecell on.

Changes in v9:
- fix v8 changes as follows:
- This reconciles the v2 and v7 code so that we power domain have lists of clocks
they trigger on and off during power transitions and independently from power
domains clocks. We attach clocks to devices comprising power domain and prepare
them so they are turn on and off by runtime PM.
- add rockchip_pm_add_one_domain() to control domains.
- add pd_start/pd_stop interface to control clocks.

Changes in v8:
- This reconciles the v2 and v7 code so that we power domain have
lists of clocks they toggle on and off during power transitions
and independently from power domains clocks we attach clocks to
devices comprising power domain and prepare them so they are
turn on and off by runtime PM.

Changes in v7:
- Delete unused variables

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

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

Changes in v4:
- use list storage dev.

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

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

drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/rockchip/Kconfig | 13 +
drivers/soc/rockchip/Makefile | 4 +
drivers/soc/rockchip/pm_domains.c | 485 ++++++++++++++++++++++++++++++++++++++
5 files changed, 504 insertions(+)
create mode 100644 drivers/soc/rockchip/Kconfig
create mode 100644 drivers/soc/rockchip/Makefile
create mode 100644 drivers/soc/rockchip/pm_domains.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 96ddecb..ecb1a6c 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -2,6 +2,7 @@ menu "SOC (System On Chip) specific Drivers"

source "drivers/soc/mediatek/Kconfig"
source "drivers/soc/qcom/Kconfig"
+source "drivers/soc/rockchip/Kconfig"
source "drivers/soc/sunxi/Kconfig"
source "drivers/soc/ti/Kconfig"
source "drivers/soc/versatile/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 7dc7c0d..ff8bbfb 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -4,6 +4,7 @@

obj-$(CONFIG_ARCH_MEDIATEK) += mediatek/
obj-$(CONFIG_ARCH_QCOM) += qcom/
+obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip/
obj-$(CONFIG_ARCH_SUNXI) += sunxi/
obj-$(CONFIG_ARCH_TEGRA) += tegra/
obj-$(CONFIG_SOC_TI) += ti/
diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
new file mode 100644
index 0000000..87a09c3
--- /dev/null
+++ b/drivers/soc/rockchip/Kconfig
@@ -0,0 +1,13 @@
+#
+# Rockchip Soc drivers
+#
+config PM_GENERIC_DOMAINS
+ bool "Rockchip generic power domain"
+ depends on PM
+ help
+ Say y here to enable power domain support.
+ 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 of the whole chip.
+
+ If unsure, say N.
diff --git a/drivers/soc/rockchip/Makefile b/drivers/soc/rockchip/Makefile
new file mode 100644
index 0000000..1c0fd91
--- /dev/null
+++ b/drivers/soc/rockchip/Makefile
@@ -0,0 +1,4 @@
+#
+# Rockchip Soc drivers
+#
+obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
new file mode 100644
index 0000000..fd772d2
--- /dev/null
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -0,0 +1,485 @@
+/*
+ * Rockchip Generic power domain support.
+ *
+ * Copyright (c) 2015 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/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 '%pC' to list of PM clocks\n", 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_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 %pC (index %d): %d\n",
+ node->name, clk, i, error);
+ goto err_out;
+ }
+
+ error = clk_prepare(clk);
+ if (error) {
+ dev_err(pmu->dev,
+ "%s: failed to prepare clk %pC (index %d): %d\n",
+ node->name, clk, i, error);
+ clk_put(clk);
+ goto err_out;
+ }
+
+ pd->clks[pd->num_clks++] = clk;
+
+ dev_dbg(pmu->dev, "added clock '%pC' to domain '%s'\n",
+ 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.flags = GENPD_FLAG_PM_CLK;
+ 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 device *parent;
+ 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 -ENODEV;
+ }
+
+ 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;
+
+ parent = dev->parent;
+ if (!parent) {
+ dev_err(dev, "no parent for syscon devices\n");
+ return -ENODEV;
+ }
+
+ pmu->regmap = syscon_node_to_regmap(parent->of_node);
+
+ /*
+ * 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 = -ENODEV;
+
+ 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

2015-08-25 07:26:17

[permalink] [raw]

Subject: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

We can add more domains node in the future.
This patch add the needed clocks into power-controller.
As the discuess about all the device clocks being listed in
the power-domains itself.

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.
In other words, we have to enable the clocks before you operate them
if all the device clocks are included in someone domians.

Someone wish was to get the clocks by reading the clocks from the
device nodes, We can do that but we can solve the above issues.

Anyway, the best ideas we can fix it in the future SoCs.

Signed-off-by: jinkun.hong <[email protected]>
Signed-off-by: Caesar Wang <[email protected]>

---

Changes in v16:
- Manually copy the problem in patch v15.
- rebase the description.

Changes in v15:
- As Tomasz remarked previously the dts should represent the hardware
and the power-domains are part of the pmu.

Changes in v14:
- 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 v13: None
Changes in v12: None
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 | 60 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 22316d0..161931d 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -44,6 +44,7 @@
#include <dt-bindings/pinctrl/rockchip.h>
#include <dt-bindings/clock/rk3288-cru.h>
#include <dt-bindings/thermal/thermal.h>
+#include <dt-bindings/power-domain/rk3288.h>
#include "skeleton.dtsi"

/ {
@@ -590,8 +591,65 @@
};

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>;
+ #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 {
--
1.9.1

2015-08-25 20:57:21

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 3/4] soc: rockchip: power-domain: Add power domain driver

Caesar Wang <[email protected]> writes:

> This driver is found on RK3288 SoCs.
>
> 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 of the whole chip.
>
> PMU can work in the Low Power Mode by setting bit[0] of PMU_PWRMODE_CON
> register. After setting the register, PMU would enter the Low Power mode.
> In the low power mode, pmu will auto power on/off the specified power
> domain, send idle req to specified power domain, shut down/up pll and
> so on. All of above are configurable by setting corresponding registers.
>
> Signed-off-by: jinkun.hong <[email protected]>
> Signed-off-by: Caesar Wang <[email protected]>

[...]

> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
> new file mode 100644
> index 0000000..87a09c3
> --- /dev/null
> +++ b/drivers/soc/rockchip/Kconfig
> @@ -0,0 +1,13 @@
> +#
> +# Rockchip Soc drivers
> +#
> +config PM_GENERIC_DOMAINS

Why are you (re)defining this config option? This name is already used in kernel/pm/Kconfig.

Kevin

2015-08-25 21:07:10

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Caesar Wang <[email protected]> writes:

> We can add more domains node in the future.
> This patch add the needed clocks into power-controller.
> As the discuess about all the device clocks being listed in
> the power-domains itself.
>
> 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.

Yes, but this is the job of device drivers which are runtime PM adapted
to gate their own clocks. I agree these clocks need to be enumerated in
the DTS, but they should be in the device nodes.

> Secondly, the reset-circuit should reset be synchronous on RK3288,
> then sync revoked. So we need to enable clocks of all devices.
> In other words, we have to enable the clocks before you operate them
> if all the device clocks are included in someone domians.

Yes, this is pretty common for reset.

> Someone wish was to get the clocks by reading the clocks from the
> device nodes, We can do that but we can solve the above issues.

I don't follow this sentence. Are you saying doing that will not solve
the above issues? Why not? Please explain.

If there are non-device clocks that also need to be enabled before
asserting reset, then those are candidates for the power-domain node,
but not device clocks.

> Anyway, the best ideas we can fix it in the future SoCs.

I don't think this is an SoC design issue as this is needed when you
have synchronous reset. My concern is primarily around how to describe
this in the DT.

Kevin

2015-08-25 21:48:44

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Kevin,

On Tue, Aug 25, 2015 at 2:07 PM, Kevin Hilman <[email protected]> wrote:
> Caesar Wang <[email protected]> writes:
>
>> We can add more domains node in the future.
>> This patch add the needed clocks into power-controller.
>> As the discuess about all the device clocks being listed in
>> the power-domains itself.
>>
>> 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.
>
> Yes, but this is the job of device drivers which are runtime PM adapted
> to gate their own clocks. I agree these clocks need to be enumerated in
> the DTS, but they should be in the device nodes.

I _think_ what Caesar means is that the alternative to this patch is
to leave the clocks on all the time as they were during the early days
of Rockchip (AKA last year). If the clocks are on all the time then
the power domain patches can work fine. However, once you start
letting clocks turn off then you need to make sure that the power
domain code turns the back on temporarily.

>> Secondly, the reset-circuit should reset be synchronous on RK3288,
>> then sync revoked. So we need to enable clocks of all devices.
>> In other words, we have to enable the clocks before you operate them
>> if all the device clocks are included in someone domians.
>
> Yes, this is pretty common for reset.
>
>> Someone wish was to get the clocks by reading the clocks from the
>> device nodes, We can do that but we can solve the above issues.
>
> I don't follow this sentence. Are you saying doing that will not solve
> the above issues? Why not? Please explain.
>
> If there are non-device clocks that also need to be enabled before
> asserting reset, then those are candidates for the power-domain node,
> but not device clocks.

It's been a long time and I don't know that I've reviewed every
revision of this series, but I think there was a proposal that we
shouldn't list clocks here. Instead we should search through and find
all devices that refer to this power domain, reach in and find their
clocks, and turn them on. Did I get that right? To put things in a
concrete way, for pd_vio we'd go through the entire device tree
ourselves and find all properties that look like "power-domains =
<&power RK3288_PD_VIO>;". We'd then find the parent of those
properties and look for a property named "clocks". We'd then iterate
over all those clocks and turn those on. Did I get that right?

The above doesn't seem like a terribly great idea to me for a number
of reasons, including:

1. If I remember correctly, it's important to turn on clocks for
devices even if they're not something you're using / have a driver
for. If you don't then the device won't get reset properly and this
can affect things like suspend/resume because the hardware in the SoC
will query all devices at suspend time to make sure they're ready. If
a device is wedged because its clock wasn't on at the right them then
it will cause problems.

2. If we absolutely need to turn all clocks and we get clocks from
device tree nodes on then it means we need device tree nodes for every
device in the domain. These would be needed even if there are no
accepted bindings for this device yet. So we'd need to do one of: A)
Block power domain patches on feature complete bindings for all
drivers; B) Make up non-approved compatible strings for all devices
and throw them into the DTS; C) Add nodes in the DTS without a
compatible string just to satisfy the power domain requirements. None
of these seem terribly appealing.

3. It is entirely possible that there are clocks that will be listed
in the individual devices that aren't needed for powering on the power
domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock)
doesn't really need to be turned on when adjusting the "VIO" power
domain. Right now Caesar has it listed, but it probably isn't needed
(Caesar: can you confirm?).

4. It seems just slightly brittle to be reaching into other device
nodes and making assumptions about their properties. Yeah, it's
probably safe to assume that "clocks" has a list of clocks and
"power-domains" will point to something whose first entry is a
phandle, but it still seems just a tad bit like violating an
abstraction barrier.

Anyway, perhaps I'm misunderstanding, or perhaps my concerns are
simply not for important things. If so feel free to yell at me. ;)

>> Anyway, the best ideas we can fix it in the future SoCs.
>
> I don't think this is an SoC design issue as this is needed when you
> have synchronous reset. My concern is primarily around how to describe
> this in the DT.

I suppose the SoC could override things and make sure clocks are on in
this case? ...or if a clock is off it could defer powering it up
somehow until the clock came on? ...dunno how this actually looks in
hardware. In any case letting devices get wedged doesn't seem
ideal...

-Doug

2015-08-25 22:46:00

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Doug Anderson <[email protected]> writes:

> Kevin,
>
> On Tue, Aug 25, 2015 at 2:07 PM, Kevin Hilman <[email protected]> wrote:
>> Caesar Wang <[email protected]> writes:
>>
>>> We can add more domains node in the future.
>>> This patch add the needed clocks into power-controller.
>>> As the discuess about all the device clocks being listed in
>>> the power-domains itself.
>>>
>>> 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.
>>
>> Yes, but this is the job of device drivers which are runtime PM adapted
>> to gate their own clocks. I agree these clocks need to be enumerated in
>> the DTS, but they should be in the device nodes.
>
> I _think_ what Caesar means is that the alternative to this patch is
> to leave the clocks on all the time as they were during the early days
> of Rockchip (AKA last year). If the clocks are on all the time then
> the power domain patches can work fine. However, once you start
> letting clocks turn off then you need to make sure that the power
> domain code turns the back on temporarily.

Yup, I understand that part, and many SoCs have this same "feature".

>>> Secondly, the reset-circuit should reset be synchronous on RK3288,
>>> then sync revoked. So we need to enable clocks of all devices.
>>> In other words, we have to enable the clocks before you operate them
>>> if all the device clocks are included in someone domians.
>>
>> Yes, this is pretty common for reset.
>>
>>> Someone wish was to get the clocks by reading the clocks from the
>>> device nodes, We can do that but we can solve the above issues.
>>
>> I don't follow this sentence. Are you saying doing that will not solve
>> the above issues? Why not? Please explain.
>>
>> If there are non-device clocks that also need to be enabled before
>> asserting reset, then those are candidates for the power-domain node,
>> but not device clocks.
>
> It's been a long time and I don't know that I've reviewed every
> revision of this series, but I think there was a proposal that we
> shouldn't list clocks here. Instead we should search through and find
> all devices that refer to this power domain, reach in and find their
> clocks, and turn them on. Did I get that right?

Yes...

> To put things in a
> concrete way, for pd_vio we'd go through the entire device tree
> ourselves and find all properties that look like "power-domains =
> <&power RK3288_PD_VIO>;". We'd then find the parent of those
> properties and look for a property named "clocks". We'd then iterate
> over all those clocks and turn those on. Did I get that right?

... but you make it sound like more work than it is. The genpd already
keeps a list of devices that refer to the power domain. In fact, the
genpd 'attach' method can be platform-specific, so could be used to keep
track of a list (or a subset) of clocks which are needed for reset.

> The above doesn't seem like a terribly great idea to me for a number
> of reasons, including:
>
> 1. If I remember correctly, it's important to turn on clocks for
> devices even if they're not something you're using / have a driver
> for. If you don't then the device won't get reset properly and this
> can affect things like suspend/resume because the hardware in the SoC
> will query all devices at suspend time to make sure they're ready.

Correct. This condition also exists in the clock framework when unused
clocks are disabled, or if you have drivers but PM_RUNTIME is disabled
(which can happen from userspace on a per-device basis) so it needs to
be understood and managed already.

> If
> a device is wedged because its clock wasn't on at the right them then
> it will cause problems.

Right. I'm not arguing that the power domain doesn't have to deal with
device clocks. It has to for sync reset. The objection I have have is
where these clocks are described.

> 2. If we absolutely need to turn all clocks and we get clocks from
> device tree nodes on then it means we need device tree nodes for every
> device in the domain.

Isn't that the end goal?

> These would be needed even if there are no
> accepted bindings for this device yet. So we'd need to do one of: A)
> Block power domain patches on feature complete bindings for all
> drivers; B) Make up non-approved compatible strings for all devices
> and throw them into the DTS; C) Add nodes in the DTS without a
> compatible string just to satisfy the power domain requirements. None
> of these seem terribly appealing.

well, I think we can be slightly more accomodating than that and go for
somewhere in between:

D) In the power-domain DTS, clearly describe why each clock is needed by
the power-domain. In particular list out clocks that are not device
clocks (and why they need to be asserted for reset) and separate those
from device clocks which are only listed in the power-domain because
there is not *yet* a driver/binding for that device node.

Doing it that way also makes it clear that when a new driver/binding is
added, the clocks should be removed from the power-domain node and put
into the device node.

Also, this addresses my primary concern that the DTS acurately describes
the hardware. IOW, in hardware, most of these clocks are are properties
of devices, not power-domains, and the DT should reflect that.

IMO, if it's not describing the hardware, and is a placeholder until a
driver/binding is in place, it should be properly documented.

> 3. It is entirely possible that there are clocks that will be listed
> in the individual devices that aren't needed for powering on the power
> domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock)
> doesn't really need to be turned on when adjusting the "VIO" power
> domain. Right now Caesar has it listed, but it probably isn't needed
> (Caesar: can you confirm?).

Yes, I suspect there are several of those, which is also why I'd like to
see the clocks in the power-domain node described in detail, and exacly
why they're needed to be enabled.

Also, in the current proposed DTS, clocks that are listed in both device
nodes and the power domain are also suspicous, especially when the
device node doesn't have a power-domain property. (c.f. vop[bl] nodes
and ACLK_VOP[01] clocks.) For that matter, this series doesn't add any
devices to the power domains, which makes it even more confusing about
how this is meant to work. IOW, with no devices belonging to
power-domains, how are the power-domain power_on/power_off callbacks
being called?

> 4. It seems just slightly brittle to be reaching into other device
> nodes and making assumptions about their properties. Yeah, it's
> probably safe to assume that "clocks" has a list of clocks and
> "power-domains" will point to something whose first entry is a
> phandle, but it still seems just a tad bit like violating an
> abstraction barrier.

shmobile is already doing this with platform-specific genpd attach
callbacks, and using the pm_clk API. I don't see any issues with that.

> Anyway, perhaps I'm misunderstanding, or perhaps my concerns are
> simply not for important things. If so feel free to yell at me. ;)

No need for me to yell. Your concerns are perfectly valid, and it's not
my style anyways. ;)

Thanks for the feedback,

Kevin

2015-08-25 23:48:00

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

On Tue, Aug 25, 2015 at 3:45 PM, Kevin Hilman <[email protected]> wrote:
> Doug Anderson <[email protected]> writes:
>
>> To put things in a
>> concrete way, for pd_vio we'd go through the entire device tree
>> ourselves and find all properties that look like "power-domains =
>> <&power RK3288_PD_VIO>;". We'd then find the parent of those
>> properties and look for a property named "clocks". We'd then iterate
>> over all those clocks and turn those on. Did I get that right?
>
> ... but you make it sound like more work than it is. The genpd already
> keeps a list of devices that refer to the power domain. In fact, the
> genpd 'attach' method can be platform-specific, so could be used to keep
> track of a list (or a subset) of clocks which are needed for reset.

That is not really workable: the attach and detach happen in
probe/remove path; if you do not have driver for the device you will
miss the clocks for it. And the quirk of this SoC is that we need to
turn all clocks during domain transition, regardless of whether there
is a driver for all devices.

Thanks.

--
Dmitry

2015-08-25 23:55:45

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Kevin,

On Tue, Aug 25, 2015 at 3:45 PM, Kevin Hilman <[email protected]> wrote:
>> To put things in a
>> concrete way, for pd_vio we'd go through the entire device tree
>> ourselves and find all properties that look like "power-domains =
>> <&power RK3288_PD_VIO>;". We'd then find the parent of those
>> properties and look for a property named "clocks". We'd then iterate
>> over all those clocks and turn those on. Did I get that right?
>
> ... but you make it sound like more work than it is. The genpd already
> keeps a list of devices that refer to the power domain. In fact, the
> genpd 'attach' method can be platform-specific, so could be used to keep
> track of a list (or a subset) of clocks which are needed for reset.

OK. I'll need to dig through and figure out how this works. So this
list will include devices whose drivers are compiled out and also
devices that are marked as 'status = "disabled";' in the device tree?
It would need to do this in case someone had a board that didn't use
one of these peripherals (since we still need to clock it as we make
the domain go up and down).

I took a quick gander and didn't see code that would do this. Can you
give me a quick pointer?

We also need to really make sure that there are no probe order issues...

>> 2. If we absolutely need to turn all clocks and we get clocks from
>> device tree nodes on then it means we need device tree nodes for every
>> device in the domain.
>
> Isn't that the end goal?

In the ideal world, yes. ...but it's possibly years before all
drivers are added, or possibly they never will be. To name a concrete
example, unless I'm mistaken, the "hardware crypto" device present in
exynos5250 still isn't anywhere in upstream despite several years
having passed. There are just so many devices in these SoCs that
aren't used.

>> These would be needed even if there are no
>> accepted bindings for this device yet. So we'd need to do one of: A)
>> Block power domain patches on feature complete bindings for all
>> drivers; B) Make up non-approved compatible strings for all devices
>> and throw them into the DTS; C) Add nodes in the DTS without a
>> compatible string just to satisfy the power domain requirements. None
>> of these seem terribly appealing.
>
> well, I think we can be slightly more accomodating than that and go for
> somewhere in between:
>
> D) In the power-domain DTS, clearly describe why each clock is needed by
> the power-domain. In particular list out clocks that are not device
> clocks (and why they need to be asserted for reset) and separate those
> from device clocks which are only listed in the power-domain because
> there is not *yet* a driver/binding for that device node.
>
> Doing it that way also makes it clear that when a new driver/binding is
> added, the clocks should be removed from the power-domain node and put
> into the device node.
>
> Also, this addresses my primary concern that the DTS acurately describes
> the hardware. IOW, in hardware, most of these clocks are are properties
> of devices, not power-domains, and the DT should reflect that.
>
> IMO, if it's not describing the hardware, and is a placeholder until a
> driver/binding is in place, it should be properly documented.

Agreed that more documentation about why each clock is needed would go
a long way to helping.

I'd say that the clocks are properties of the device, but I'd again
assert that not all clocks that are owned by the device are relevant
to turning power domains on and off. If you really wanted to be
correct, you'd add a property like this to devices:

power-on-clocks = <..>;

...this would be a list of clocks needed to power on the device
properly. If such a property was added that would erase my objection
to that. Note that you could say that if "power-on-clocks" wasn't
present then you could fall back to "clocks". I do worry a little bit
about over-engineering, so I guess this could just be a future
improvement...

>> 3. It is entirely possible that there are clocks that will be listed
>> in the individual devices that aren't needed for powering on the power
>> domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock)
>> doesn't really need to be turned on when adjusting the "VIO" power
>> domain. Right now Caesar has it listed, but it probably isn't needed
>> (Caesar: can you confirm?).
>
> Yes, I suspect there are several of those, which is also why I'd like to
> see the clocks in the power-domain node described in detail, and exacly
> why they're needed to be enabled.

I guess my point was that this was a reason for doing something like
"power-on-clocks" because it would let you avoid turning on
PCLK_EDP_CTRL (which is listed in the "clocks" of the EDP device but
maybe not needed while powering on/off the domain). ...or just have
the list as part of the power domain. :-P

> Also, in the current proposed DTS, clocks that are listed in both device
> nodes and the power domain are also suspicous, especially when the
> device node doesn't have a power-domain property. (c.f. vop[bl] nodes
> and ACLK_VOP[01] clocks.) For that matter, this series doesn't add any
> devices to the power domains, which makes it even more confusing about
> how this is meant to work. IOW, with no devices belonging to
> power-domains, how are the power-domain power_on/power_off callbacks
> being called?

I totally believe that our current hardware definition isn't so
correct (though it does seem to work). I'd be that devices that never
turn off often don't have a power domain defined (though they
technically have one) because that power domain always happens to be
on and no amount of specifying will allow it to turn off. An
advantage of what you're pushing for is that it really forces us to
think about exactly what clocks are needed and forces us to specify
power domains more correctly.

>> 4. It seems just slightly brittle to be reaching into other device
>> nodes and making assumptions about their properties. Yeah, it's
>> probably safe to assume that "clocks" has a list of clocks and
>> "power-domains" will point to something whose first entry is a
>> phandle, but it still seems just a tad bit like violating an
>> abstraction barrier.
>
> shmobile is already doing this with platform-specific genpd attach
> callbacks, and using the pm_clk API. I don't see any issues with that.

I can see it working. It's more of a feeling of unease. ...but
that's not a good enough reason to reject something...

-Doug

2015-08-26 09:25:21

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

在 2015年08月26日 06:45, Kevin Hilman 写道:
> Doug Anderson <[email protected]> writes:
>
>> Kevin,
>>
>> On Tue, Aug 25, 2015 at 2:07 PM, Kevin Hilman <[email protected]> wrote:
>>> Caesar Wang <[email protected]> writes:
>>>
>>>> We can add more domains node in the future.
>>>> This patch add the needed clocks into power-controller.
>>>> As the discuess about all the device clocks being listed in
>>>> the power-domains itself.
>>>>
>>>> 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.
>>> Yes, but this is the job of device drivers which are runtime PM adapted
>>> to gate their own clocks. I agree these clocks need to be enumerated in
>>> the DTS, but they should be in the device nodes.
>> I _think_ what Caesar means is that the alternative to this patch is
>> to leave the clocks on all the time as they were during the early days
>> of Rockchip (AKA last year). If the clocks are on all the time then
>> the power domain patches can work fine. However, once you start
>> letting clocks turn off then you need to make sure that the power
>> domain code turns the back on temporarily.
> Yup, I understand that part, and many SoCs have this same "feature".
>
>>>> Secondly, the reset-circuit should reset be synchronous on RK3288,
>>>> then sync revoked. So we need to enable clocks of all devices.
>>>> In other words, we have to enable the clocks before you operate them
>>>> if all the device clocks are included in someone domians.
>>> Yes, this is pretty common for reset.
>>>
>>>> Someone wish was to get the clocks by reading the clocks from the
>>>> device nodes, We can do that but we can solve the above issues.
>>> I don't follow this sentence. Are you saying doing that will not solve
>>> the above issues? Why not? Please explain.
>>>
>>> If there are non-device clocks that also need to be enabled before
>>> asserting reset, then those are candidates for the power-domain node,
>>> but not device clocks.
>> It's been a long time and I don't know that I've reviewed every
>> revision of this series, but I think there was a proposal that we
>> shouldn't list clocks here. Instead we should search through and find
>> all devices that refer to this power domain, reach in and find their
>> clocks, and turn them on. Did I get that right?
> Yes...

Sounds resonable, the domain(e.g. "pd_vio"...) idle will be abnormal but
you have registered all devices.
Why?
AFAIK, you need turn on the noc/ip clocks if we are operating the
"pd_vio" domain to enter the idle status.
if the noc is same clock with ip side. We need turn on the IP side
clocks. Otherwise we need turn on the noc clocks.

>> To put things in a
>> concrete way, for pd_vio we'd go through the entire device tree
>> ourselves and find all properties that look like "power-domains =
>> <&power RK3288_PD_VIO>;". We'd then find the parent of those
>> properties and look for a property named "clocks". We'd then iterate
>> over all those clocks and turn those on. Did I get that right?
> ... but you make it sound like more work than it is. The genpd already
> keeps a list of devices that refer to the power domain. In fact, the
> genpd 'attach' method can be platform-specific, so could be used to keep
> track of a list (or a subset) of clocks which are needed for reset.
>
>> The above doesn't seem like a terribly great idea to me for a number
>> of reasons, including:
>>
>> 1. If I remember correctly, it's important to turn on clocks for
>> devices even if they're not something you're using / have a driver
>> for. If you don't then the device won't get reset properly and this
>> can affect things like suspend/resume because the hardware in the SoC
>> will query all devices at suspend time to make sure they're ready.
> Correct. This condition also exists in the clock framework when unused
> clocks are disabled, or if you have drivers but PM_RUNTIME is disabled
> (which can happen from userspace on a per-device basis) so it needs to
> be understood and managed already.
>
>> If
>> a device is wedged because its clock wasn't on at the right them then
>> it will cause problems.
> Right. I'm not arguing that the power domain doesn't have to deal with
> device clocks. It has to for sync reset. The objection I have have is
> where these clocks are described.
>
>> 2. If we absolutely need to turn all clocks and we get clocks from
>> device tree nodes on then it means we need device tree nodes for every
>> device in the domain.
> Isn't that the end goal?
>
>> These would be needed even if there are no
>> accepted bindings for this device yet. So we'd need to do one of: A)
>> Block power domain patches on feature complete bindings for all
>> drivers; B) Make up non-approved compatible strings for all devices
>> and throw them into the DTS; C) Add nodes in the DTS without a
>> compatible string just to satisfy the power domain requirements. None
>> of these seem terribly appealing.
> well, I think we can be slightly more accomodating than that and go for
> somewhere in between:
>
> D) In the power-domain DTS, clearly describe why each clock is needed by
> the power-domain. In particular list out clocks that are not device
> clocks (and why they need to be asserted for reset) and separate those
> from device clocks which are only listed in the power-domain because
> there is not *yet* a driver/binding for that device node.
>
> Doing it that way also makes it clear that when a new driver/binding is
> added, the clocks should be removed from the power-domain node and put
> into the device node.
>
> Also, this addresses my primary concern that the DTS acurately describes
> the hardware. IOW, in hardware, most of these clocks are are properties
> of devices, not power-domains, and the DT should reflect that.
>
> IMO, if it's not describing the hardware, and is a placeholder until a
> driver/binding is in place, it should be properly documented.
>
>> 3. It is entirely possible that there are clocks that will be listed
>> in the individual devices that aren't needed for powering on the power
>> domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock)
>> doesn't really need to be turned on when adjusting the "VIO" power
>> domain. Right now Caesar has it listed, but it probably isn't needed
>> (Caesar: can you confirm?).
> Yes, I suspect there are several of those, which is also why I'd like to
> see the clocks in the power-domain node described in detail, and exacly
> why they're needed to be enabled.
>
> Also, in the current proposed DTS, clocks that are listed in both device
> nodes and the power domain are also suspicous, especially when the
> device node doesn't have a power-domain property. (c.f. vop[bl] nodes
> and ACLK_VOP[01] clocks.) For that matter, this series doesn't add any
> devices to the power domains, which makes it even more confusing about
> how this is meant to work. IOW, with no devices belonging to
> power-domains, how are the power-domain power_on/power_off callbacks
> being called?

Okay, As the cover-letter lists.
We can add the EDP/VOP domain node to test in next-kernel.

Also, the chromium-3.14 has some devices to complete it.

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/v3.14

----
Thanks,
Caesar

>> 4. It seems just slightly brittle to be reaching into other device
>> nodes and making assumptions about their properties. Yeah, it's
>> probably safe to assume that "clocks" has a list of clocks and
>> "power-domains" will point to something whose first entry is a
>> phandle, but it still seems just a tad bit like violating an
>> abstraction barrier.
> shmobile is already doing this with platform-specific genpd attach
> callbacks, and using the pm_clk API. I don't see any issues with that.
>
>> Anyway, perhaps I'm misunderstanding, or perhaps my concerns are
>> simply not for important things. If so feel free to yell at me. ;)
> No need for me to yell. Your concerns are perfectly valid, and it's not
> my style anyways. ;)
>
> Thanks for the feedback,
>
> Kevin
>
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

2015-08-26 09:29:08

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 3/4] soc: rockchip: power-domain: Add power domain driver

在 2015年08月26日 04:57, Kevin Hilman 写道:
> Caesar Wang <[email protected]> writes:
>
>> This driver is found on RK3288 SoCs.
>>
>> 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 of the whole chip.
>>
>> PMU can work in the Low Power Mode by setting bit[0] of PMU_PWRMODE_CON
>> register. After setting the register, PMU would enter the Low Power mode.
>> In the low power mode, pmu will auto power on/off the specified power
>> domain, send idle req to specified power domain, shut down/up pll and
>> so on. All of above are configurable by setting corresponding registers.
>>
>> Signed-off-by: jinkun.hong <[email protected]>
>> Signed-off-by: Caesar Wang <[email protected]>
> [...]
>
>> diff --git a/drivers/soc/rockchip/Kconfig b/drivers/soc/rockchip/Kconfig
>> new file mode 100644
>> index 0000000..87a09c3
>> --- /dev/null
>> +++ b/drivers/soc/rockchip/Kconfig
>> @@ -0,0 +1,13 @@
>> +#
>> +# Rockchip Soc drivers
>> +#
>> +config PM_GENERIC_DOMAINS
> Why are you (re)defining this config option? This name is already used in kernel/pm/Kconfig.

Fixed.

----
Thanks,
Caesar

> Kevin
>
> _______________________________________________
> Linux-rockchip mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

2015-08-28 00:24:20

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Dmitry Torokhov <[email protected]> writes:

> On Tue, Aug 25, 2015 at 3:45 PM, Kevin Hilman <[email protected]> wrote:
>> Doug Anderson <[email protected]> writes:
>>
>>> To put things in a
>>> concrete way, for pd_vio we'd go through the entire device tree
>>> ourselves and find all properties that look like "power-domains =
>>> <&power RK3288_PD_VIO>;". We'd then find the parent of those
>>> properties and look for a property named "clocks". We'd then iterate
>>> over all those clocks and turn those on. Did I get that right?
>>
>> ... but you make it sound like more work than it is. The genpd already
>> keeps a list of devices that refer to the power domain. In fact, the
>> genpd 'attach' method can be platform-specific, so could be used to keep
>> track of a list (or a subset) of clocks which are needed for reset.
>
> That is not really workable: the attach and detach happen in
> probe/remove path; if you do not have driver for the device you will
> miss the clocks for it.

And in my proposal, I suggested that clocks without drivers are
good candidates to list in the domain, with the caveat that the be
called out (documented) as being device clocks that are missing a
driver, so when a driver shows up they can be moved accordingly, and in
a way that actually describes the hardware.

> And the quirk of this SoC is that we need to
> turn all clocks during domain transition, regardless of whether there
> is a driver for all devices.

I understand. And that "quirk" is not unique to rockchip socs.

Kevin

2015-08-28 02:03:23

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Kevin,

On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <[email protected]> wrote:
>> That is not really workable: the attach and detach happen in
>> probe/remove path; if you do not have driver for the device you will
>> miss the clocks for it.
>
> And in my proposal, I suggested that clocks without drivers are
> good candidates to list in the domain, with the caveat that the be
> called out (documented) as being device clocks that are missing a
> driver, so when a driver shows up they can be moved accordingly, and in
> a way that actually describes the hardware.

What happens if someone disables the driver using the CONFIG subsystem?

What happens if this is a device that someone has set to 'status =
"disabled";' in the device tree?

Even if the device is disabled in one of those two ways, we still need
the clocks to be turned on. ...so if we turn on/off the VIO domain we
need to turn on the EDP clock even if there's no EDP in the current
board / config. We might turn on/off VIO for one of the other devices
in the VIO domain for one of the other devices in VIO that we are
using.

-Doug

2015-08-28 20:02:45

by Mike Turquette

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Hi Doug,

Quoting Doug Anderson (2015-08-27 19:03:20)
> Kevin,
>
> On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <[email protected]> wrote:
> >> That is not really workable: the attach and detach happen in
> >> probe/remove path; if you do not have driver for the device you will
> >> miss the clocks for it.
> >
> > And in my proposal, I suggested that clocks without drivers are
> > good candidates to list in the domain, with the caveat that the be
> > called out (documented) as being device clocks that are missing a
> > driver, so when a driver shows up they can be moved accordingly, and in
> > a way that actually describes the hardware.
>
> What happens if someone disables the driver using the CONFIG subsystem?

Kevin asked me to chime in on this thread, as I have a half-baked idea
that might solve the problem posed by your question above.

One thing I have been considering for a while is a fallback compatible
string that can be used for an IP block when either there is no driver
loaded or no driver exists at all. Something like "generic-ip-block".

The purpose of this compatible string is to allow us to model resource
consumption in dts accurately, regardless of whether or not a proper
driver has been written in Linux. This idea was born out of the
simple-fb binding/driver discussion last year[0].

Obviously such a binding would not enable any of the logic or function
of that IP block; that would require a proper driver. But it would allow
us to properly link system-wide resources that are consumed: the
generic-ip could consume clocks and regulators, it could belong to power
domains, etc. For this reason I have also thought that
"generic-resource-consumer" is an accurate compatible string.

This spares us from having to encode nasty details into the power domain
binding, which is exactly what would happen if you needed a dedicated
list of clocks in the power domain node that were not claimed by device
nodes/drivers.

Note that a real driver might exist for an IP block, but if that driver
is disabled in Kconfig AND the corresponding dt node has this fallback
compatible string, then we could be OK, from the perspective of the
power domain problem.

>
> What happens if this is a device that someone has set to 'status =
> "disabled";' in the device tree?

If someone does that, then I think we should let that break power domain
transitions.

>
> Even if the device is disabled in one of those two ways, we still need
> the clocks to be turned on. ...so if we turn on/off the VIO domain we
> need to turn on the EDP clock even if there's no EDP in the current
> board / config. We might turn on/off VIO for one of the other devices
> in the VIO domain for one of the other devices in VIO that we are
> using.

I'm hesitant to mention this but I am working on a patch series to
implement a clock "handoff" mechanism (also inspired by the simplefb
discussion). This allows us to set a per-clock flag that tells the
framework to enable that clock at registration time, and then the first
clock consumer driver to come along and claim that clock inherits that
clock enable reference count.

I'm working on v2 that lets us set this flag from DT, but I really only
plan to do this for special cases. For the normal case the flag should
be set in the Linux clock provider driver. In the mean time v1 is under
discussion[1].

[0] http://lkml.kernel.org/r/<[email protected]>
[1] http://lkml.kernel.org/r/<[email protected]>

Regards,
Mike

>
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2015-08-28 21:08:56

[permalink] [raw]

Subject: Re: [RESEND PATCH v16 4/4] ARM: dts: add the support power-domain node on RK3288 SoCs

Mike,

On Fri, Aug 28, 2015 at 1:02 PM, Michael Turquette
<[email protected]> wrote:
> Hi Doug,
>
> Quoting Doug Anderson (2015-08-27 19:03:20)
>> Kevin,
>>
>> On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <[email protected]> wrote:
>> >> That is not really workable: the attach and detach happen in
>> >> probe/remove path; if you do not have driver for the device you will
>> >> miss the clocks for it.
>> >
>> > And in my proposal, I suggested that clocks without drivers are
>> > good candidates to list in the domain, with the caveat that the be
>> > called out (documented) as being device clocks that are missing a
>> > driver, so when a driver shows up they can be moved accordingly, and in
>> > a way that actually describes the hardware.
>>
>> What happens if someone disables the driver using the CONFIG subsystem?
>
> Kevin asked me to chime in on this thread, as I have a half-baked idea
> that might solve the problem posed by your question above.
>
> One thing I have been considering for a while is a fallback compatible
> string that can be used for an IP block when either there is no driver
> loaded or no driver exists at all. Something like "generic-ip-block".
>
> The purpose of this compatible string is to allow us to model resource
> consumption in dts accurately, regardless of whether or not a proper
> driver has been written in Linux. This idea was born out of the
> simple-fb binding/driver discussion last year[0].
>
> Obviously such a binding would not enable any of the logic or function
> of that IP block; that would require a proper driver. But it would allow
> us to properly link system-wide resources that are consumed: the
> generic-ip could consume clocks and regulators, it could belong to power
> domains, etc. For this reason I have also thought that
> "generic-resource-consumer" is an accurate compatible string.
>
> This spares us from having to encode nasty details into the power domain
> binding, which is exactly what would happen if you needed a dedicated
> list of clocks in the power domain node that were not claimed by device
> nodes/drivers.
>
> Note that a real driver might exist for an IP block, but if that driver
> is disabled in Kconfig AND the corresponding dt node has this fallback
> compatible string, then we could be OK, from the perspective of the
> power domain problem.

OK, so that could solve the "Kconfig" problem.

>> What happens if this is a device that someone has set to 'status =
>> "disabled";' in the device tree?
>
> If someone does that, then I think we should let that break power domain
> transitions.

So if you're on a board that doesn't use EDP but uses LVDS then their
suspend/resume should be broken?

Specifically, it's my understanding that on this SoC:

1. There's a single "video IO" power domain that contains things like
the video output processor, EDP, LVDS, HDMI, etc.

2. When you turn on/off the power domain it's important to clock all
devices in the domain during the reset.

3. If you don't clock all devices during the reset they get left in a
"wedged" state.

4. If you try to do things like suspend/resume it will query all
devices. If they've been left in the wedged state then suspend/resume
will be blocked.

...so this problem is still not solved.

>> Even if the device is disabled in one of those two ways, we still need
>> the clocks to be turned on. ...so if we turn on/off the VIO domain we
>> need to turn on the EDP clock even if there's no EDP in the current
>> board / config. We might turn on/off VIO for one of the other devices
>> in the VIO domain for one of the other devices in VIO that we are
>> using.
>
> I'm hesitant to mention this but I am working on a patch series to
> implement a clock "handoff" mechanism (also inspired by the simplefb
> discussion). This allows us to set a per-clock flag that tells the
> framework to enable that clock at registration time, and then the first
> clock consumer driver to come along and claim that clock inherits that
> clock enable reference count.
>
> I'm working on v2 that lets us set this flag from DT, but I really only
> plan to do this for special cases. For the normal case the flag should
> be set in the Linux clock provider driver. In the mean time v1 is under
> discussion[1].

In this case if we're using LVDS and HDMI on a system, we don't want
the EDP clock always on (that just wastes power). We just need to
clock it for the power domain transition.

Note that nothing in the above solves the problem that not all clocks
for a given device need to be turned on during power domain
transitions. If we're trying to describe hardware perfectly, we'd
really need to add to each device:

clocks = <clk1>, <clk2>, <clk3>, ...
clock-names = "name1", "name2", "name3", ...
clocks-for-power-domain-transition = "name2", "name3";

...since perhaps the clock named "name1" isn't required for power
domain transitions and so shouldn't be turned on. Note that once
we're going through and adding a special list, it doesn't seem that
different than just adding the list in the power domain node instead
(other than being more complex).

2015-08-28 21:28:16