[PATCH v2 0/6] rockchip: power-domain: fix pm domain for support RK3399 SoC

fix some idle handling
support sub-power domain
add rk3399-power.h
modify power domain for RK3399 SoC
modify document for RK3399 Soc

Elaine Zhang (6):
  rockchip: power-domain: make idle handling optional
  rockchip: power-domain: allow domains only handling idle requests
  rockchip: power-domain: add support for sub-power domains
  dt/bindings: power: add RK3399 SoCs header for power-domain
  rockchip: power-domain: Modify power domain driver for rk3399
  dt/bindings: rockchip: modify document of Rockchip power domains

 .../bindings/soc/rockchip/power_domain.txt         |  21 ++++
 drivers/soc/rockchip/pm_domains.c                  | 131 ++++++++++++++++++++-
 include/dt-bindings/power/rk3399-power.h           |  53 +++++++++
 3 files changed, 200 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/power/rk3399-power.h

-- 
1.9.1

Not all new socs need to handle idle states on domain state changes,
so add the possibility to make them optional.

Signed-off-by: Elaine Zhang <redacted>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/soc/rockchip/pm_domains.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 6cdffb1..3dcc611 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c

@@ -63,14 +63,16 @@ struct rockchip_pmu {
 };
 
 #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
+#define NULL_BIT		-32
+#define OVERFLOW_MASK		32
 
 #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),			\
+	.req_mask = (req >= 0) ? BIT(req) : OVERFLOW_MASK,		\
+	.idle_mask = (idle >= 0) ? BIT(idle) : OVERFLOW_MASK,	\
+	.ack_mask = (ack >= 0) ? BIT(ack) : OVERFLOW_MASK,		\
 }
 
 #define DOMAIN_RK3288(pwr, status, req)		\

@@ -96,6 +98,9 @@ static int rockchip_pmu_set_idle_request(struct rockchip_pm_domain *pd,
 	struct rockchip_pmu *pmu = pd->pmu;
 	unsigned int val;
 
+	if (pd_info->req_mask >= OVERFLOW_MASK)
+		return 0;
+
 	regmap_update_bits(pmu->regmap, pmu->info->req_offset,
 			   pd_info->req_mask, idle ? -1U : 0);
 

-- 
1.9.1

Am Donnerstag, 18. Februar 2016, 11:07:13 schrieb Elaine Zhang:

Not all new socs need to handle idle states on domain state changes,
so add the possibility to make them optional.

Signed-off-by: Elaine Zhang <redacted>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>

What's up with the Signed-off-bys? I remember creating the draft of this 
change, so either my authorship of the patch should be retained or the 
Signed-off-by with my name removed :-)

git send-email will keep patch authorship nicely.

I don't really understand why you need this NULL_BIT / OVERFLOW_MASK.
Defining the unset things to -1 should work nicely as well and is already 
regularly used elsewhere - so people will already know this scheme. That way 
you also don't need to introduce two new constants someone will have to look 
up later.

According to a description from TRM, add all the power domains

Signed-off-by: Elaine Zhang <redacted>
---
 include/dt-bindings/power/rk3399-power.h | 53 ++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 include/dt-bindings/power/rk3399-power.h

diff --git a/include/dt-bindings/power/rk3399-power.h b/include/dt-bindings/power/rk3399-power.h
new file mode 100644
index 0000000..69fbd67
--- /dev/null
+++ b/include/dt-bindings/power/rk3399-power.h

@@ -0,0 +1,53 @@
+#ifndef __DT_BINDINGS_POWER_RK3399_POWER_H__
+#define __DT_BINDINGS_POWER_RK3399_POWER_H__
+
+/* VD_CORE_L */
+#define RK3399_PD_A53_L0	0
+#define RK3399_PD_A53_L1	1
+#define RK3399_PD_A53_L2	2
+#define RK3399_PD_A53_L3	3
+#define RK3399_PD_SCU_L		4
+
+/* VD_CORE_B */
+#define RK3399_PD_A72_B0	5
+#define RK3399_PD_A72_B1	6
+#define RK3399_PD_SCU_B		7
+
+/* VD_CENTER */
+#define RK3399_PD_CENTER	8
+#define RK3399_PD_VCODEC	9
+#define RK3399_PD_RGA		10
+#define RK3399_PD_IEP		11
+#define RK3399_PD_VDU		12
+
+/* VD_LOGIC */
+#define RK3399_PD_PERILP	13
+#define RK3399_PD_PERIHP	14
+#define RK3399_PD_VIO		15
+#define RK3399_PD_VO		16
+#define RK3399_PD_VOPB		17
+#define RK3399_PD_VOPL		18
+#define RK3399_PD_ISP0		19
+#define RK3399_PD_ISP1		20
+#define RK3399_PD_HDCP		21
+#define RK3399_PD_TCPD0		22
+#define RK3399_PD_TCPD1		23
+#define RK3399_PD_GIC		24
+#define RK3399_PD_ALIVE		25
+#define RK3399_PD_USB3		26
+#define RK3399_PD_SD		27
+#define RK3399_PD_CCI		28
+#define RK3399_PD_CCI0		29
+#define RK3399_PD_CCI1		30
+#define RK3399_PD_GMAC		31
+#define RK3399_PD_EMMC		32
+#define RK3399_PD_EDP		33
+#define RK3399_PD_SDIOAUDIO	34
+
+/* VD_GPU */
+#define RK3399_PD_GPU		35
+
+/* VD_PMU */
+#define RK3399_PD_PMU		36
+
+#endif

-- 
1.9.1

This patch adds support for making one power domain a sub-domain of
other domain. This is useful for modeling power dependences,
which needs to have more than one power domain enabled to be operational.

Signed-off-by: Elaine Zhang <redacted>
---
 drivers/soc/rockchip/pm_domains.c | 54 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 350527b..8cdf1b2 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c

@@ -372,6 +372,54 @@ static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
 	regmap_write(pmu->regmap, domain_reg_offset + 4, count);
 }
 
+static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
+				     struct device_node *parent)
+{
+	struct device_node *np;
+	int error;
+
+	for_each_child_of_node(parent, np) {
+		struct generic_pm_domain *child_domain, *parent_domain;
+		u32 idx = ~0;
+
+		if (of_property_read_u32(parent, "reg", &idx)) {
+			dev_err(pmu->dev,
+				"%s: failed to retrieve domain id (reg)\n",
+				parent->name);
+			of_node_put(parent);
+			return -EINVAL;
+		}
+		parent_domain = pmu->genpd_data.domains[idx];
+
+		error = rockchip_pm_add_one_domain(pmu, np);
+		if (error) {
+			dev_err(pmu->dev, "failed to handle node %s: %d\n",
+				np->name, error);
+			of_node_put(np);
+			return -ENODEV;
+		}
+
+		if (of_property_read_u32(np, "reg", &idx)) {
+			dev_err(pmu->dev,
+				"%s: failed to retrieve domain id (reg)\n",
+				np->name);
+			of_node_put(np);
+			return -EINVAL;
+		}
+		child_domain = pmu->genpd_data.domains[idx];
+
+		if (pm_genpd_add_subdomain(parent_domain, child_domain))
+			pr_warn("%s failed to add subdomain: %s\n",
+				parent_domain->name, child_domain->name);
+		else
+			pr_warn("%s add subdomain: %s\n",
+				parent_domain->name, child_domain->name);
+
+		rockchip_pm_add_subdomain(pmu, np);
+	}
+	return 0;
+}
+
 static int rockchip_pm_domain_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;

@@ -437,6 +485,12 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 				node->name, error);
 			goto err_out;
 		}
+		error = rockchip_pm_add_subdomain(pmu, node);
+		if (error < 0) {
+			dev_err(dev, "failed to handle subdomain node %s: %d\n",
+				node->name, error);
+			goto err_out;
+		}
 	}
 
 	if (error) {

-- 
1.9.1

Hi Elaine,

Am Donnerstag, 18. Februar 2016, 11:07:15 schrieb Elaine Zhang:

should probably be dev_err here

+		else
+			pr_warn("%s add subdomain: %s\n",
+				parent_domain->name, child_domain->name);

you don't need pr_warn / dev_warn on sucess

+
+		rockchip_pm_add_subdomain(pmu, np);
+	}
+	return 0;
+}
+

In general the code looks like you're going to group the domains 
hirarchically, like

       power: power-controller {
               compatible = "rockchip,rk3399-power-controller";

               pd_cci {
                       reg = <RK3399_PD_CCI>;
                       clocks = ...;

			pd_cci0 {
				reg = <RK3399_PD_CCI0>;
				clocks = ...;
			};
               };
       };

This isn't documented in the dt-binding and also isn't really improving
reading these declarations. Instead as I said before, I think I'd like more 
get inspiration from how Exynos is doing that already (arch/arm/mach-
exynos/pm_domains.c at "Assign the child power domains to their parents") 
simply declaring an additional power-domains property like:

       power: power-controller {
               compatible = "rockchip,rk3399-power-controller";

               pd_cci {
                       reg = <RK3399_PD_CCI>;
                       clocks = ...;
               };

		pd_cci0 {
			reg = <RK3399_PD_CCI0>;
			clocks = ...;
			power-domains = <&power RK3399_PD_CCI>;
		};
       };

blank line here

+		error = rockchip_pm_add_subdomain(pmu, node);
+		if (error < 0) {
+			dev_err(dev, "failed to handle subdomain node %s: %d\n",
+				node->name, error);
+			goto err_out;
+		}
 	}

 	if (error) {

On Thu, Feb 18, 2016 at 4:20 PM, Heiko Stuebner [off-list ref] wrote:

In general the code looks like you're going to group the domains
hirarchically, like

       power: power-controller {
               compatible = "rockchip,rk3399-power-controller";

               pd_cci {
                       reg = <RK3399_PD_CCI>;
                       clocks = ...;

                        pd_cci0 {
                                reg = <RK3399_PD_CCI0>;
                                clocks = ...;
                        };
               };
       };

This isn't documented in the dt-binding and also isn't really improving
reading these declarations. Instead as I said before, I think I'd like more
get inspiration from how Exynos is doing that already

Heiko and I had a chat offline about this, and I mentioned that the
Exynos way is not the preferred way.  The nesting that is being done
here (and in v4) is the better way, and is fully supported by the
genpd code.

Kevin

On some Rockchip SoC there exist child-domains only handling their
idle state with the actual power-state handled by a parent-domain.

So allow such types of domains. For them, we can determine their
state (on/of) by checking the inverse idle-state instead.

There exist one special case if both idle as well power handling
were set as not present, but as the domain-data is defined in the
code itself, we can expect the reasonable developer to define them

So allow such types of domains. For them, we can determine their
state (on/of) by checking the inverse idle-state instead.

There exist one special case if both idle as well power handling
were set as not present, but as the domain-data is defined in the
code itself, we can expect the reasonable developer to define them
in a correct, without adding more checks.

Signed-off-by: Elaine Zhang <redacted>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/soc/rockchip/pm_domains.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 3dcc611..350527b 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c

@@ -68,8 +68,8 @@ struct rockchip_pmu {
 
 #define DOMAIN(pwr, status, req, idle, ack)	\
 {						\
-	.pwr_mask = BIT(pwr),			\
-	.status_mask = BIT(status),		\
+	.pwr_mask = (pwr >= 0) ? BIT(pwr) : OVERFLOW_MASK,		\
+	.status_mask = (status >= 0) ? BIT(status) : OVERFLOW_MASK,\
 	.req_mask = (req >= 0) ? BIT(req) : OVERFLOW_MASK,		\
 	.idle_mask = (idle >= 0) ? BIT(idle) : OVERFLOW_MASK,	\
 	.ack_mask = (ack >= 0) ? BIT(ack) : OVERFLOW_MASK,		\

@@ -121,6 +121,10 @@ static bool rockchip_pmu_domain_is_on(struct rockchip_pm_domain *pd)
 	struct rockchip_pmu *pmu = pd->pmu;
 	unsigned int val;
 
+	/* check idle status for idle-only domains */
+	if (pd->info->status_mask >= OVERFLOW_MASK)
+		return !rockchip_pmu_domain_is_idle(pd);
+
 	regmap_read(pmu->regmap, pmu->info->status_offset, &val);
 
 	/* 1'b0: power on, 1'b1: power off */

@@ -132,6 +136,9 @@ static void rockchip_do_pmu_set_power_domain(struct rockchip_pm_domain *pd,
 {
 	struct rockchip_pmu *pmu = pd->pmu;
 
+	if (pd->info->pwr_mask >= OVERFLOW_MASK)
+		return;
+
 	regmap_update_bits(pmu->regmap, pmu->info->pwr_offset,
 			   pd->info->pwr_mask, on ? 0 : -1U);
 

-- 
1.9.1

Hi Elaine,

your sending mechanism could use some improvements :-)
I always get patches 1-4 correctly as replies to the cover-letter while 
patches 5+6 always come separately (missing in-reply-to?).

I guess using git send-email might help with that.

Also, Kevin Hilman was very involved in reviewing the original Rockchip 
power-domain driver, so it might be nice to include him in further versions.


Heiko

Am Donnerstag, 18. Februar 2016, 11:07:12 schrieb Elaine Zhang:

fix some idle handling
support sub-power domain
add rk3399-power.h
modify power domain for RK3399 SoC
modify document for RK3399 Soc

Elaine Zhang (6):
  rockchip: power-domain: make idle handling optional
  rockchip: power-domain: allow domains only handling idle requests
  rockchip: power-domain: add support for sub-power domains
  dt/bindings: power: add RK3399 SoCs header for power-domain
  rockchip: power-domain: Modify power domain driver for rk3399
  dt/bindings: rockchip: modify document of Rockchip power domains

 .../bindings/soc/rockchip/power_domain.txt         |  21 ++++
 drivers/soc/rockchip/pm_domains.c                  | 131
++++++++++++++++++++- include/dt-bindings/power/rk3399-power.h          
|  53 +++++++++ 3 files changed, 200 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/power/rk3399-power.h