[PATCH 0/4] soc: rockchip: power-domain: Add regulator support

[PATCH 0/4] soc: rockchip: power-domain: Add regulator support

[Sascha Hauer]

This series adds the possibility to add supplies to the rockchip power
domains. This is a bit more complicated then it should be as we can
get a regulator from a device node only when a device is attached to
it, so this series starts by moving the power domains into a device.
Overall this series makes the clock handling nicer because we no longer
have to use of_clk_get(), but can use a plain devm_clk_bulk_get_all()
to retrieve the domain clocks.

All this is needed for the RK3568-EVB board where the power domain of
the GPU is supplied by an external regulator.

Sascha Hauer (4):
  soc: rockchip: power-domain: register device for each domain
  soc: rockchip: power-domain: Use devm_clk_bulk_get_all()
  soc: rockchip: power-domain: Add regulator support
  dt-bindings: power: rockchip: Add power-supply to domain nodes

 .../power/rockchip,power-controller.yaml      |   6 +
 drivers/soc/rockchip/pm_domains.c             | 322 ++++++++----------
 2 files changed, 157 insertions(+), 171 deletions(-)

-- 
2.30.2


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

[PATCH 1/4] soc: rockchip: power-domain: register device for each domain

[Sascha Hauer]

This patch prepares the rockchip power domain driver for regulator
support. When a switchable regulator supplies a power domain the logical
place to put the regulator is into the device node of that domain. In
Linux we can get a regulator from a device node only when a device is
attached to it. With this patch we register a device for each domain.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/soc/rockchip/pm_domains.c | 275 ++++++++++++++----------------
 1 file changed, 127 insertions(+), 148 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 0868b7d406fba..d2f71437c73a9 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -29,6 +29,8 @@
 #include <dt-bindings/power/rk3399-power.h>
 #include <dt-bindings/power/rk3568-power.h>
 
+struct rockchip_pmu;
+
 struct rockchip_domain_info {
 	const char *name;
 	int pwr_mask;
@@ -39,6 +41,10 @@ struct rockchip_domain_info {
 	bool active_wakeup;
 	int pwr_w_mask;
 	int req_w_mask;
+
+	struct rockchip_pmu *pmu;
+	struct generic_pm_domain *parent_domain;
+	int id;
 };
 
 struct rockchip_pmu_info {
@@ -81,6 +87,7 @@ struct rockchip_pmu {
 	struct regmap *regmap;
 	const struct rockchip_pmu_info *info;
 	struct mutex mutex; /* mutex lock for pmu */
+	atomic_t missing;
 	struct genpd_onecell_data genpd_data;
 	struct generic_pm_domain *domains[];
 };
@@ -387,12 +394,11 @@ static void rockchip_pd_detach_dev(struct generic_pm_domain *genpd,
 }
 
 static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
-				      struct device_node *node)
+				      struct device_node *node,
+				      struct generic_pm_domain *parent_domain)
 {
-	const struct rockchip_domain_info *pd_info;
-	struct rockchip_pm_domain *pd;
-	struct device_node *qos_node;
-	int i, j;
+	struct platform_device *pd_pdev;
+	struct rockchip_domain_info *domain_info;
 	u32 id;
 	int error;
 
@@ -410,28 +416,98 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 		return -EINVAL;
 	}
 
-	pd_info = &pmu->info->domain_info[id];
-	if (!pd_info) {
-		dev_err(pmu->dev, "%pOFn: undefined domain id %d\n",
-			node, id);
-		return -EINVAL;
+	pd_pdev = platform_device_alloc("rk-power-domain", id);
+	if (!pd_pdev) {
+		dev_err(pmu->dev, "Failed to allocate platform device\n");
+		return -ENOMEM;
 	}
 
-	pd = devm_kzalloc(pmu->dev, sizeof(*pd), GFP_KERNEL);
+	error = platform_device_add_data(pd_pdev,
+				         &pmu->info->domain_info[id],
+				         sizeof(pmu->info->domain_info[id]));
+	if (error)
+		goto err_put;
+
+	domain_info = pd_pdev->dev.platform_data;
+	domain_info->parent_domain = parent_domain;
+	domain_info->pmu = pmu;
+	domain_info->id = id;
+
+	pd_pdev->dev.parent = pmu->dev;
+	pd_pdev->dev.of_node = node;
+
+	atomic_inc(&pmu->missing);
+
+	error = platform_device_add(pd_pdev);
+	if (error)
+		goto err_put;
+
+	return 0;
+
+err_put:
+	platform_device_put(pd_pdev);
+
+	return error;
+}
+
+static void rockchip_pm_add_domains(struct rockchip_pmu *pmu,
+				    struct device_node *parent,
+				    struct generic_pm_domain *parent_domain)
+{
+	struct device_node *np;
+	int error;
+
+	/*
+	 * We may only register the genpd provider when we have registered all
+	 * domains that are specified in the device tree. We count the missing
+	 * domains in rockchip_pmu::missing.
+	 * The rockchip pm_domains may have subdomains which means we can be
+	 * called here recursively. Give it one extra count here to prevent
+	 * the counter dropping to zero when we are called recursively.
+	 */
+	atomic_inc(&pmu->missing);
+
+	for_each_child_of_node(parent, np) {
+		error = rockchip_pm_add_one_domain(pmu, np, parent_domain);
+		if (error)
+			dev_err(pmu->dev, "failed to handle node %pOFn: %d\n",
+				np, error);
+	}
+
+	if (!atomic_dec_and_test(&pmu->missing))
+		return;
+
+	error = of_genpd_add_provider_onecell(pmu->dev->of_node, &pmu->genpd_data);
+	if (error)
+		dev_err(pmu->dev, "failed to add provider: %d\n", error);
+}
+
+static int rockchip_domain_probe(struct platform_device *pdev)
+{
+	struct rockchip_domain_info *pd_info = pdev->dev.platform_data;
+	struct rockchip_pm_domain *pd;
+	struct device_node *node = pdev->dev.of_node;
+	struct device_node *qos_node;
+	struct rockchip_pmu *pmu = pd_info->pmu;
+	struct generic_pm_domain *parent_domain;
+	int i, j;
+	int error;
+
+	pd = devm_kzalloc(&pdev->dev, sizeof(*pd), GFP_KERNEL);
 	if (!pd)
 		return -ENOMEM;
 
 	pd->info = pd_info;
-	pd->pmu = pmu;
+	pd->pmu = pd_info->pmu;
 
 	pd->num_clks = of_clk_get_parent_count(node);
 	if (pd->num_clks > 0) {
-		pd->clks = devm_kcalloc(pmu->dev, pd->num_clks,
+		pd->clks = devm_kcalloc(&pdev->dev, pd->num_clks,
 					sizeof(*pd->clks), GFP_KERNEL);
 		if (!pd->clks)
 			return -ENOMEM;
 	} else {
-		dev_dbg(pmu->dev, "%pOFn: doesn't have clocks: %d\n",
+		dev_dbg(&pdev->dev, "%pOFn: doesn't have clocks: %d\n",
 			node, pd->num_clks);
 		pd->num_clks = 0;
 	}
@@ -440,7 +516,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 		pd->clks[i].clk = of_clk_get(node, i);
 		if (IS_ERR(pd->clks[i].clk)) {
 			error = PTR_ERR(pd->clks[i].clk);
-			dev_err(pmu->dev,
+			dev_err(&pdev->dev,
 				"%pOFn: failed to get clk at index %d: %d\n",
 				node, i, error);
 			return error;
@@ -455,7 +531,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 						 NULL);
 
 	if (pd->num_qos > 0) {
-		pd->qos_regmap = devm_kcalloc(pmu->dev, pd->num_qos,
+		pd->qos_regmap = devm_kcalloc(&pdev->dev, pd->num_qos,
 					      sizeof(*pd->qos_regmap),
 					      GFP_KERNEL);
 		if (!pd->qos_regmap) {
@@ -464,7 +540,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 		}
 
 		for (j = 0; j < MAX_QOS_REGS_NUM; j++) {
-			pd->qos_save_regs[j] = devm_kcalloc(pmu->dev,
+			pd->qos_save_regs[j] = devm_kcalloc(&pdev->dev,
 							    pd->num_qos,
 							    sizeof(u32),
 							    GFP_KERNEL);
@@ -490,7 +566,7 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 		}
 	}
 
-	error = rockchip_pd_power(pd, true);
+	error = rockchip_pd_power_on(&pd->genpd);
 	if (error) {
 		dev_err(pmu->dev,
 			"failed to power on domain '%pOFn': %d\n",
@@ -507,13 +583,33 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 	pd->genpd.attach_dev = rockchip_pd_attach_dev;
 	pd->genpd.detach_dev = rockchip_pd_detach_dev;
 	pd->genpd.flags = GENPD_FLAG_PM_CLK;
-	if (pd_info->active_wakeup)
+	if (pd->info->active_wakeup)
 		pd->genpd.flags |= GENPD_FLAG_ACTIVE_WAKEUP;
 	pm_genpd_init(&pd->genpd, NULL, false);
 
-	pmu->genpd_data.domains[id] = &pd->genpd;
+	parent_domain = pd_info->parent_domain;
+	if (parent_domain) {
+		error = pm_genpd_add_subdomain(parent_domain, &pd->genpd);
+		if (error) {
+			dev_err(pmu->dev, "%s failed to add subdomain %s: %d\n",
+				parent_domain->name, pd->genpd.name, error);
+			goto out_genpd_remove;
+		} else {
+			dev_dbg(pmu->dev, "%s add subdomain: %s\n",
+				parent_domain->name, pd->genpd.name);
+		}
+	}
+
+	pmu->genpd_data.domains[pd->info->id] = &pd->genpd;
+
+	atomic_dec(&pmu->missing);
+
+	rockchip_pm_add_domains(pmu, node, &pd->genpd);
+
 	return 0;
 
+out_genpd_remove:
+	pm_genpd_remove(&pd->genpd);
 err_unprepare_clocks:
 	clk_bulk_unprepare(pd->num_clks, pd->clks);
 err_put_clocks:
@@ -521,46 +617,19 @@ static int rockchip_pm_add_one_domain(struct rockchip_pmu *pmu,
 	return error;
 }
 
-static void rockchip_pm_remove_one_domain(struct rockchip_pm_domain *pd)
+static int rockchip_domain_remove(struct platform_device *pdev)
 {
-	int ret;
-
-	/*
-	 * We're in the error cleanup already, so we only complain,
-	 * but won't emit another error on top of the original one.
-	 */
-	ret = pm_genpd_remove(&pd->genpd);
-	if (ret < 0)
-		dev_err(pd->pmu->dev, "failed to remove domain '%s' : %d - state may be inconsistent\n",
-			pd->genpd.name, ret);
-
-	clk_bulk_unprepare(pd->num_clks, pd->clks);
-	clk_bulk_put(pd->num_clks, pd->clks);
-
-	/* protect the zeroing of pm->num_clks */
-	mutex_lock(&pd->pmu->mutex);
-	pd->num_clks = 0;
-	mutex_unlock(&pd->pmu->mutex);
-
-	/* devm will free our memory */
+	return 0;
 }
 
-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 struct platform_driver rockchip_domain_driver = {
+	.driver = {
+		.name = "rk-power-domain",
+	},
+	.probe    = rockchip_domain_probe,
+	.remove   = rockchip_domain_remove,
+};
+builtin_platform_driver(rockchip_domain_driver)
 
 static void rockchip_configure_pd_cnt(struct rockchip_pmu *pmu,
 				      u32 domain_reg_offset,
@@ -572,71 +641,14 @@ 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;
-	struct generic_pm_domain *child_domain, *parent_domain;
-	int error;
-
-	for_each_child_of_node(parent, np) {
-		u32 idx;
-
-		error = of_property_read_u32(parent, "reg", &idx);
-		if (error) {
-			dev_err(pmu->dev,
-				"%pOFn: failed to retrieve domain id (reg): %d\n",
-				parent, error);
-			goto err_out;
-		}
-		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 %pOFn: %d\n",
-				np, error);
-			goto err_out;
-		}
-
-		error = of_property_read_u32(np, "reg", &idx);
-		if (error) {
-			dev_err(pmu->dev,
-				"%pOFn: failed to retrieve domain id (reg): %d\n",
-				np, error);
-			goto err_out;
-		}
-		child_domain = pmu->genpd_data.domains[idx];
-
-		error = pm_genpd_add_subdomain(parent_domain, child_domain);
-		if (error) {
-			dev_err(pmu->dev, "%s failed to add subdomain %s: %d\n",
-				parent_domain->name, child_domain->name, error);
-			goto err_out;
-		} else {
-			dev_dbg(pmu->dev, "%s add subdomain: %s\n",
-				parent_domain->name, child_domain->name);
-		}
-
-		rockchip_pm_add_subdomain(pmu, np);
-	}
-
-	return 0;
-
-err_out:
-	of_node_put(np);
-	return error;
-}
-
 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");
@@ -688,42 +700,9 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 		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 %pOFn: %d\n",
-				node, error);
-			of_node_put(node);
-			goto err_out;
-		}
-
-		error = rockchip_pm_add_subdomain(pmu, node);
-		if (error < 0) {
-			dev_err(dev, "failed to handle subdomain node %pOFn: %d\n",
-				node, error);
-			of_node_put(node);
-			goto err_out;
-		}
-	}
-
-	if (error) {
-		dev_dbg(dev, "no power domains defined\n");
-		goto err_out;
-	}
-
-	error = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
-	if (error) {
-		dev_err(dev, "failed to add provider: %d\n", error);
-		goto err_out;
-	}
+	rockchip_pm_add_domains(pmu, np, NULL);
 
 	return 0;
-
-err_out:
-	rockchip_pm_domain_cleanup(pmu);
-	return error;
 }
 
 static const struct rockchip_domain_info px30_pm_domains[] = {
-- 
2.30.2


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

[PATCH 2/4] soc: rockchip: power-domain: Use devm_clk_bulk_get_all()

[Sascha Hauer]

Now that each power domain has its own device we no longer have to use
of_clk_get(), but can instead simplify retrieving the clocks by using
devm_clk_bulk_get_all().

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/soc/rockchip/pm_domains.c | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index d2f71437c73a9..dcfd3db649f58 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -490,7 +490,7 @@ static int rockchip_domain_probe(struct platform_device *pdev)
 	struct device_node *qos_node;
 	struct rockchip_pmu *pmu = pd_info->pmu;
 	struct generic_pm_domain *parent_domain;
-	int i, j;
+	int j;
 	int error;
 
 	pd = devm_kzalloc(&pdev->dev, sizeof(*pd), GFP_KERNEL);
@@ -500,32 +500,13 @@ static int rockchip_domain_probe(struct platform_device *pdev)
 	pd->info = pd_info;
 	pd->pmu = pd_info->pmu;
 
-	pd->num_clks = of_clk_get_parent_count(node);
-	if (pd->num_clks > 0) {
-		pd->clks = devm_kcalloc(&pdev->dev, pd->num_clks,
-					sizeof(*pd->clks), GFP_KERNEL);
-		if (!pd->clks)
-			return -ENOMEM;
-	} else {
-		dev_dbg(&pdev->dev, "%pOFn: doesn't have clocks: %d\n",
-			node, pd->num_clks);
-		pd->num_clks = 0;
-	}
-
-	for (i = 0; i < pd->num_clks; i++) {
-		pd->clks[i].clk = of_clk_get(node, i);
-		if (IS_ERR(pd->clks[i].clk)) {
-			error = PTR_ERR(pd->clks[i].clk);
-			dev_err(&pdev->dev,
-				"%pOFn: failed to get clk at index %d: %d\n",
-				node, i, error);
-			return error;
-		}
-	}
+	pd->num_clks = devm_clk_bulk_get_all(&pdev->dev, &pd->clks);
+	if (pd->num_clks < 0)
+		return pd->num_clks;
 
 	error = clk_bulk_prepare(pd->num_clks, pd->clks);
 	if (error)
-		goto err_put_clocks;
+		return error;
 
 	pd->num_qos = of_count_phandle_with_args(node, "pm_qos",
 						 NULL);
@@ -612,8 +593,6 @@ static int rockchip_domain_probe(struct platform_device *pdev)
 	pm_genpd_remove(&pd->genpd);
 err_unprepare_clocks:
 	clk_bulk_unprepare(pd->num_clks, pd->clks);
-err_put_clocks:
-	clk_bulk_put(pd->num_clks, pd->clks);
 	return error;
 }
 
-- 
2.30.2


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

[PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Sascha Hauer]

This patch allows to let a domain be supplied by a regulator which
is needed for the GPU on the rk3568-EVB board.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/soc/rockchip/pm_domains.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index dcfd3db649f58..e7db888cc226c 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -15,6 +15,7 @@
 #include <linux/of_platform.h>
 #include <linux/clk.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/mfd/syscon.h>
 #include <dt-bindings/power/px30-power.h>
 #include <dt-bindings/power/rk3036-power.h>
@@ -80,6 +81,7 @@ struct rockchip_pm_domain {
 	u32 *qos_save_regs[MAX_QOS_REGS_NUM];
 	int num_clks;
 	struct clk_bulk_data *clks;
+	struct regulator *regulator;
 };
 
 struct rockchip_pmu {
@@ -344,6 +346,14 @@ static int rockchip_pd_power(struct rockchip_pm_domain *pd, bool power_on)
 static int rockchip_pd_power_on(struct generic_pm_domain *domain)
 {
 	struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
+	struct rockchip_pmu *pmu = pd->pmu;
+	int ret;
+
+	ret = regulator_enable(pd->regulator);
+	if (ret) {
+		dev_err(pmu->dev, "failed to enable regulator: %d\n", ret);
+		return ret;
+	}
 
 	return rockchip_pd_power(pd, true);
 }
@@ -351,8 +361,15 @@ static int rockchip_pd_power_on(struct generic_pm_domain *domain)
 static int rockchip_pd_power_off(struct generic_pm_domain *domain)
 {
 	struct rockchip_pm_domain *pd = to_rockchip_pd(domain);
+	int ret;
 
-	return rockchip_pd_power(pd, false);
+	ret = rockchip_pd_power(pd, false);
+	if (ret)
+		return ret;
+
+	regulator_disable(pd->regulator);
+
+	return 0;
 }
 
 static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
@@ -500,6 +517,11 @@ static int rockchip_domain_probe(struct platform_device *pdev)
 	pd->info = pd_info;
 	pd->pmu = pd_info->pmu;
 
+	pd->regulator = devm_regulator_get(&pdev->dev, "power");
+
+	if (IS_ERR(pd->regulator))
+		return PTR_ERR(pd->regulator);
+
 	pd->num_clks = devm_clk_bulk_get_all(&pdev->dev, &pd->clks);
 	if (pd->num_clks < 0)
 		return pd->num_clks;
-- 
2.30.2


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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Sascha Hauer]

On Fri, Dec 17, 2021 at 02:09:18PM +0100, Sascha Hauer wrote:
> This patch allows to let a domain be supplied by a regulator which
> is needed for the GPU on the rk3568-EVB board.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/soc/rockchip/pm_domains.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> +
> +	regulator_disable(pd->regulator);
> +
> +	return 0;
>  }
>  
>  static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
> @@ -500,6 +517,11 @@ static int rockchip_domain_probe(struct platform_device *pdev)
>  	pd->info = pd_info;
>  	pd->pmu = pd_info->pmu;
>  
> +	pd->regulator = devm_regulator_get(&pdev->dev, "power");

I was told that I should use this function instead of
devm_regulator_get_optional() when the regulator is not optional and
also I can drop the if (pd->regulator) dance when enabling the regulator
because I get a dummy regulator here when using devm_regulator_get().

Well, all true and on one specific board the regulator is indeed not
optional. However, on all other power domains that don't need a
regulator and all other boards and all other SoCs this driver is used we
now get:

[    0.185588] rk-power-domain rk-power-domain.8: supply power not found, using dummy regulator
[    0.186036] rk-power-domain rk-power-domain.9: supply power not found, using dummy regulator
[    0.186459] rk-power-domain rk-power-domain.10: supply power not found, using dummy regulator
[    0.187039] rk-power-domain rk-power-domain.11: supply power not found, using dummy regulator
[    0.187333] rk-power-domain rk-power-domain.13: supply power not found, using dummy regulator
[    0.187644] rk-power-domain rk-power-domain.14: supply power not found, using dummy regulator
[    0.188042] rk-power-domain rk-power-domain.15: supply power not found, using dummy regulator

I wonder if devm_regulator_get() is really the right function here. Or
should the message be dropped?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Robin Murphy]

On 2021-12-20 09:44, Sascha Hauer wrote:
> On Fri, Dec 17, 2021 at 02:09:18PM +0100, Sascha Hauer wrote:
>> This patch allows to let a domain be supplied by a regulator which
>> is needed for the GPU on the rk3568-EVB board.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>   drivers/soc/rockchip/pm_domains.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> +
>> +	regulator_disable(pd->regulator);
>> +
>> +	return 0;
>>   }
>>   
>>   static int rockchip_pd_attach_dev(struct generic_pm_domain *genpd,
>> @@ -500,6 +517,11 @@ static int rockchip_domain_probe(struct platform_device *pdev)
>>   	pd->info = pd_info;
>>   	pd->pmu = pd_info->pmu;
>>   
>> +	pd->regulator = devm_regulator_get(&pdev->dev, "power");
> 
> I was told that I should use this function instead of
> devm_regulator_get_optional() when the regulator is not optional and
> also I can drop the if (pd->regulator) dance when enabling the regulator
> because I get a dummy regulator here when using devm_regulator_get().
> 
> Well, all true and on one specific board the regulator is indeed not
> optional. However, on all other power domains that don't need a
> regulator and all other boards and all other SoCs this driver is used we
> now get:
> 
> [    0.185588] rk-power-domain rk-power-domain.8: supply power not found, using dummy regulator
> [    0.186036] rk-power-domain rk-power-domain.9: supply power not found, using dummy regulator
> [    0.186459] rk-power-domain rk-power-domain.10: supply power not found, using dummy regulator
> [    0.187039] rk-power-domain rk-power-domain.11: supply power not found, using dummy regulator
> [    0.187333] rk-power-domain rk-power-domain.13: supply power not found, using dummy regulator
> [    0.187644] rk-power-domain rk-power-domain.14: supply power not found, using dummy regulator
> [    0.188042] rk-power-domain rk-power-domain.15: supply power not found, using dummy regulator
> 
> I wonder if devm_regulator_get() is really the right function here. Or
> should the message be dropped?

Since the power domains are hierarchical and none of them really 
represent the physical owner of a supply, I'm not sure there's really a 
correct answer to the regulator question either way in this context. 
FWIW I reckon it would make sense to model things properly and teach the 
driver about the voltage domains that actually own the input supplies 
(maybe with a binding more like I/O domains where we just have 
explicitly-named supply properties for each one on the power 
controller?) - it's a little more work up-front, but seems like it 
should be relatively straightforward to fit into the genpd hierarchy, 
and be more robust in the long term.

Robin.

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 833 bytes --]

On Mon, Dec 20, 2021 at 10:46:34AM +0000, Robin Murphy wrote:

> to the regulator question either way in this context. FWIW I reckon it would
> make sense to model things properly and teach the driver about the voltage
> domains that actually own the input supplies (maybe with a binding more like
> I/O domains where we just have explicitly-named supply properties for each
> one on the power controller?) - it's a little more work up-front, but seems
> like it should be relatively straightforward to fit into the genpd
> hierarchy, and be more robust in the long term.

This is what I would expect too, I don't see how it is possible to
implement sensible and robust usage of the regulator API (or other
provider APIs like the clock API for that matter) if the consumer is
unaware of what resources it is supposed to be managing.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1369 bytes --]

On Mon, Dec 20, 2021 at 10:44:35AM +0100, Sascha Hauer wrote:

> Well, all true and on one specific board the regulator is indeed not
> optional. However, on all other power domains that don't need a
> regulator and all other boards and all other SoCs this driver is used we
> now get:

This seems unlikely to be board specific, if the chip requires power the
chip requires power.  If there are power domains that don't take
external supplies then they shouldn't be requesting any regulators and
should be fixed.

> [    0.185588] rk-power-domain rk-power-domain.8: supply power not found, using dummy regulator

It seems vanishingly unlikely that the SoC takes a single supply called
"power" shared by everything in the SoC but that is what the code
appears to be requesting - the power domains should be requesting the
supplies they actually use, and as ever the supplies should be named
such that someone looking at the schematic can hook them up.  The
general recommendation is to use the names used in the datasheet.

> I wonder if devm_regulator_get() is really the right function here. Or
> should the message be dropped?

No, the issue is that the client driver is badly written and needs to be
fixed.  In general it's probably better to have error handling than not.
If you're getting lots of warnings about problems it's probably due to
there being problems.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Sascha Hauer]

On Mon, Dec 20, 2021 at 12:53:58PM +0000, Mark Brown wrote:
> On Mon, Dec 20, 2021 at 10:44:35AM +0100, Sascha Hauer wrote:
> 
> > Well, all true and on one specific board the regulator is indeed not
> > optional. However, on all other power domains that don't need a
> > regulator and all other boards and all other SoCs this driver is used we
> > now get:
> 
> This seems unlikely to be board specific, if the chip requires power the
> chip requires power.  If there are power domains that don't take
> external supplies then they shouldn't be requesting any regulators and
> should be fixed.
> 
> > [    0.185588] rk-power-domain rk-power-domain.8: supply power not found, using dummy regulator
> 
> It seems vanishingly unlikely that the SoC takes a single supply called
> "power" shared by everything in the SoC but that is what the code
> appears to be requesting - the power domains should be requesting the
> supplies they actually use, and as ever the supplies should be named
> such that someone looking at the schematic can hook them up.  The
> general recommendation is to use the names used in the datasheet.

Ok. I'll change the patch in a way that only for the GPU power domain on
rk3568 a supply is requested. That's the one power domain I know that a
regulator is needed. I'm sure there are more, if not on rk3568 then
probably on other SoCs the driver handles. Once we notice that other
domains need a supply we'll have to add the supply name to driver data
for that domain.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Robin Murphy]

On 2021-12-22 10:40, Sascha Hauer wrote:
> On Mon, Dec 20, 2021 at 12:53:58PM +0000, Mark Brown wrote:
>> On Mon, Dec 20, 2021 at 10:44:35AM +0100, Sascha Hauer wrote:
>>
>>> Well, all true and on one specific board the regulator is indeed not
>>> optional. However, on all other power domains that don't need a
>>> regulator and all other boards and all other SoCs this driver is used we
>>> now get:
>>
>> This seems unlikely to be board specific, if the chip requires power the
>> chip requires power.  If there are power domains that don't take
>> external supplies then they shouldn't be requesting any regulators and
>> should be fixed.
>>
>>> [    0.185588] rk-power-domain rk-power-domain.8: supply power not found, using dummy regulator
>>
>> It seems vanishingly unlikely that the SoC takes a single supply called
>> "power" shared by everything in the SoC but that is what the code
>> appears to be requesting - the power domains should be requesting the
>> supplies they actually use, and as ever the supplies should be named
>> such that someone looking at the schematic can hook them up.  The
>> general recommendation is to use the names used in the datasheet.
> 
> Ok. I'll change the patch in a way that only for the GPU power domain on
> rk3568 a supply is requested. That's the one power domain I know that a
> regulator is needed. I'm sure there are more, if not on rk3568 then
> probably on other SoCs the driver handles. Once we notice that other
> domains need a supply we'll have to add the supply name to driver data
> for that domain.

Certainly RK3399 (and I guess RK3288 too) suffers from the same 
priority-inversion issue of being unaware that VD_GPU needs power before 
PD_GPU can be successfully turned on to probe the GPU to claim and 
enable the regulator (via "mali-supply" for DVFS purposes) that needed 
to be on in the first place. Currently all the boards are bodging around 
this with "regulator-always-on" (e.g. commit 06b2818678d9).

Thanks,
Robin.

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 807 bytes --]

On Wed, Dec 22, 2021 at 12:54:06PM +0000, Robin Murphy wrote:

> Certainly RK3399 (and I guess RK3288 too) suffers from the same
> priority-inversion issue of being unaware that VD_GPU needs power before
> PD_GPU can be successfully turned on to probe the GPU to claim and enable
> the regulator (via "mali-supply" for DVFS purposes) that needed to be on in
> the first place. Currently all the boards are bodging around this with
> "regulator-always-on" (e.g. commit 06b2818678d9).

Does the SoC actually support the supply being completely off in
operation?  A lot of devices want everything powered even if idle during
full operation since keeping voltage differentials smaller makes it a
lot easier to design to avoid leakage current type issues at the points
where the different power domains connect.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Robin Murphy]

On 2021-12-22 13:00, Mark Brown wrote:
> On Wed, Dec 22, 2021 at 12:54:06PM +0000, Robin Murphy wrote:
> 
>> Certainly RK3399 (and I guess RK3288 too) suffers from the same
>> priority-inversion issue of being unaware that VD_GPU needs power before
>> PD_GPU can be successfully turned on to probe the GPU to claim and enable
>> the regulator (via "mali-supply" for DVFS purposes) that needed to be on in
>> the first place. Currently all the boards are bodging around this with
>> "regulator-always-on" (e.g. commit 06b2818678d9).
> 
> Does the SoC actually support the supply being completely off in
> operation?  A lot of devices want everything powered even if idle during
> full operation since keeping voltage differentials smaller makes it a
> lot easier to design to avoid leakage current type issues at the points
> where the different power domains connect.

I don't know TBH - the available documentation doesn't seem to go into 
quite that much detail.

Robin.

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 804 bytes --]

On Wed, Dec 22, 2021 at 01:19:23PM +0000, Robin Murphy wrote:
> On 2021-12-22 13:00, Mark Brown wrote:

> > Does the SoC actually support the supply being completely off in
> > operation?  A lot of devices want everything powered even if idle during
> > full operation since keeping voltage differentials smaller makes it a
> > lot easier to design to avoid leakage current type issues at the points
> > where the different power domains connect.

> I don't know TBH - the available documentation doesn't seem to go into quite
> that much detail.

In that case I would tend to assume that unless otherwise stated it's
safer to keep the supply enabled when everything else is enabled.  It's
the default thing and so is more likely to be what was designed for and
less likely to result in nasty surprises.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Michael Riesch]

Hi Robin and Mark,

On 12/22/21 2:25 PM, Mark Brown wrote:
> On Wed, Dec 22, 2021 at 01:19:23PM +0000, Robin Murphy wrote:
>> On 2021-12-22 13:00, Mark Brown wrote:
> 
>>> Does the SoC actually support the supply being completely off in
>>> operation?  A lot of devices want everything powered even if idle during
>>> full operation since keeping voltage differentials smaller makes it a
>>> lot easier to design to avoid leakage current type issues at the points
>>> where the different power domains connect.
> 
>> I don't know TBH - the available documentation doesn't seem to go into quite
>> that much detail.
> 
> In that case I would tend to assume that unless otherwise stated it's
> safer to keep the supply enabled when everything else is enabled.  It's
> the default thing and so is more likely to be what was designed for and
> less likely to result in nasty surprises.

Based on my experience with the RK3568 EVB1 the vdd_gpu supply turns
after startup if it is unused and the SoC works fine. Let's take the
energy efficient route here :-)

Best regards,
Michael

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Michael Riesch]

On 12/22/21 2:29 PM, Michael Riesch wrote:
> Hi Robin and Mark,
> 
> On 12/22/21 2:25 PM, Mark Brown wrote:
>> On Wed, Dec 22, 2021 at 01:19:23PM +0000, Robin Murphy wrote:
>>> On 2021-12-22 13:00, Mark Brown wrote:
>>
>>>> Does the SoC actually support the supply being completely off in
>>>> operation?  A lot of devices want everything powered even if idle during
>>>> full operation since keeping voltage differentials smaller makes it a
>>>> lot easier to design to avoid leakage current type issues at the points
>>>> where the different power domains connect.
>>
>>> I don't know TBH - the available documentation doesn't seem to go into quite
>>> that much detail.
>>
>> In that case I would tend to assume that unless otherwise stated it's
>> safer to keep the supply enabled when everything else is enabled.  It's
>> the default thing and so is more likely to be what was designed for and
>> less likely to result in nasty surprises.
> 
> Based on my experience with the RK3568 EVB1 the vdd_gpu supply turns

[edit] turns off, of course

Best regards,
Michael

> after startup if it is unused and the SoC works fine. Let's take the
> energy efficient route here :-)
> 
> Best regards,
> Michael
> 
> _______________________________________________
> 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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 821 bytes --]

On Wed, Dec 22, 2021 at 02:29:23PM +0100, Michael Riesch wrote:
> On 12/22/21 2:25 PM, Mark Brown wrote:

> > In that case I would tend to assume that unless otherwise stated it's
> > safer to keep the supply enabled when everything else is enabled.  It's
> > the default thing and so is more likely to be what was designed for and
> > less likely to result in nasty surprises.

> Based on my experience with the RK3568 EVB1 the vdd_gpu supply turns
> after startup if it is unused and the SoC works fine. Let's take the
> energy efficient route here :-)

One of the issues is that it may not actually be energy efficient if it
ends up leaking current through the SoC from the other supplies (and
long term that leakage probably won't do any good for hardware).  A very
lightly loaded regulator can be the better option.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

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

Re: [PATCH 3/4] soc: rockchip: power-domain: Add regulator support

[Michael Riesch]

Hi Mark,

Sorry for the long pause!

On 12/22/21 14:40, Mark Brown wrote:
> On Wed, Dec 22, 2021 at 02:29:23PM +0100, Michael Riesch wrote:
>> On 12/22/21 2:25 PM, Mark Brown wrote:
> 
>>> In that case I would tend to assume that unless otherwise stated it's
>>> safer to keep the supply enabled when everything else is enabled.  It's
>>> the default thing and so is more likely to be what was designed for and
>>> less likely to result in nasty surprises.
> 
>> Based on my experience with the RK3568 EVB1 the vdd_gpu supply turns
>> after startup if it is unused and the SoC works fine. Let's take the
>> energy efficient route here :-)
> 
> One of the issues is that it may not actually be energy efficient if it
> ends up leaking current through the SoC from the other supplies (and
> long term that leakage probably won't do any good for hardware).  A very
> lightly loaded regulator can be the better option.

OK, I see. So in summary we don't know the behavior of the SoC and
should go for the safe route (set the GPU regulator to always on)? I'll
submit a patch for the RK3568 EVB1 -- the other boards enable this
regulator always as well.

Best regards,
Michael

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

[PATCH 4/4] dt-bindings: power: rockchip: Add power-supply to domain nodes

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../bindings/power/rockchip,power-controller.yaml           | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml b/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
index 9b9d710874663..f7e4b95fb6feb 100644
--- a/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
+++ b/Documentation/devicetree/bindings/power/rockchip,power-controller.yaml
@@ -71,6 +71,8 @@ patternProperties:
       "#size-cells":
         const: 0
 
+      power-supply: true
+
     patternProperties:
       "^power-domain@[0-9a-f]+$":
 
@@ -85,6 +87,8 @@ patternProperties:
           "#size-cells":
             const: 0
 
+          power-supply: true
+
         patternProperties:
           "^power-domain@[0-9a-f]+$":
 
@@ -96,6 +100,8 @@ patternProperties:
               "#power-domain-cells":
                 const: 0
 
+            power-supply: true
+
 $defs:
   pd-node:
     type: object
-- 
2.30.2


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