diff mbox series

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

Message ID 20211217130919.3035788-4-s.hauer@pengutronix.de (mailing list archive)
State New
Headers show
Series soc: rockchip: power-domain: Add regulator support | expand

Commit Message

s.hauer@pengutronix.de Dec. 17, 2021, 1:09 p.m. UTC
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(-)

Comments

s.hauer@pengutronix.de Dec. 20, 2021, 9:44 a.m. UTC | #1
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
Robin Murphy Dec. 20, 2021, 10:46 a.m. UTC | #2
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.
Mark Brown Dec. 20, 2021, 12:53 p.m. UTC | #3
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.
Mark Brown Dec. 20, 2021, 12:56 p.m. UTC | #4
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.
s.hauer@pengutronix.de Dec. 22, 2021, 10:40 a.m. UTC | #5
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
Robin Murphy Dec. 22, 2021, 12:54 p.m. UTC | #6
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.
Mark Brown Dec. 22, 2021, 1 p.m. UTC | #7
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.
Robin Murphy Dec. 22, 2021, 1:19 p.m. UTC | #8
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.
Mark Brown Dec. 22, 2021, 1:25 p.m. UTC | #9
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.
Michael Riesch Dec. 22, 2021, 1:29 p.m. UTC | #10
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
Michael Riesch Dec. 22, 2021, 1:37 p.m. UTC | #11
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
>
Mark Brown Dec. 22, 2021, 1:40 p.m. UTC | #12
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.
Michael Riesch Feb. 23, 2022, 8:51 a.m. UTC | #13
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
diff mbox series

Patch

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;