| Message ID | 20211217130919.3035788-4-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: rockchip: power-domain: Add regulator support | expand |
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
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.
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.
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.
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
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.
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.
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.
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.
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
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 >
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.
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 --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;
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(-)