LinuxLists.cc - [PATCH v16 0/3] eDP/DP Phy vdda realted function

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Tue, Jul 05, 2022 at 09:29:13AM -0700, Kuogee Hsieh wrote:
> 0) rebase on https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git tree
> 1) add regulator_set_load() to eDP phy
> 2) add regulator_set_load() to DP phy
> 3) remove vdda related function out of eDP/DP controller
>
> Kuogee Hsieh (3):
> phy: qcom-edp: add regulator_set_load to edp phy
> phy: qcom-qmp: add regulator_set_load to dp phy
> drm/msm/dp: delete vdda regulator related functions from eDP/DP
> controller
>
> drivers/gpu/drm/msm/dp/dp_parser.c | 14 -----
> drivers/gpu/drm/msm/dp/dp_parser.h | 8 ---
> drivers/gpu/drm/msm/dp/dp_power.c | 95 +------------------------------
> drivers/phy/qualcomm/phy-qcom-edp.c | 12 ++++
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 41 ++++++++++---
> 5 files changed, 46 insertions(+), 124 deletions(-)

This series breaks USB and PCIe for some SC8280XP and SA540P machines
where the DP PHY regulators are shared with other PHYs whose drivers do
not request a load.

Specifically, the hard-coded vdda-phy load of 21.8 mA added by this
series, causes several RPMh regulators to now be put in low-power mode.

I found Doug's suggestion to handle situations like this in regulator
core:

https://lore.kernel.org/all/[email protected]/

but since that was rejected, how do we deal with this generally?

In the above thread Doug mentioned adding support for load requests to
further drivers and Bjorn mentioned working around it by adding
regulator-system-load properties to DT.

It seems quite likely that changes like this one affects other systems
too, and the effects may be hard to debug. So a more general solution
than playing whack-a-mole using DT would be good to have.

Johan

2022-07-21 12:08:29

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, Jul 21, 2022 at 12:31:41PM +0200, Johan Hovold wrote:

If you're copying someone into a thread that's not obviously relevant
for them it's good practice to put a note about it at the top of the
mail to reduce the chances that it just gets deleted unread - people get
copies of all sorts of random stuff for not great reasons (like getting
pulled in by checkpatch due to once having done a cleanup) and are often
quicky to delete things.

> This series breaks USB and PCIe for some SC8280XP and SA540P machines
> where the DP PHY regulators are shared with other PHYs whose drivers do
> not request a load.

> Specifically, the hard-coded vdda-phy load of 21.8 mA added by this
> series, causes several RPMh regulators to now be put in low-power mode.

> I found Doug's suggestion to handle situations like this in regulator
> core:
>
> https://lore.kernel.org/all/[email protected]/

> but since that was rejected, how do we deal with this generally?

> In the above thread Doug mentioned adding support for load requests to
> further drivers and Bjorn mentioned working around it by adding
> regulator-system-load properties to DT.

> It seems quite likely that changes like this one affects other systems
> too, and the effects may be hard to debug. So a more general solution
> than playing whack-a-mole using DT would be good to have.

You could add a way to specify constant base loads in DT on either a per
regulator or per consumer basis.

Attachments:

(No filename) (1.48 kB)
signature.asc (499.00 B)
Download all attachments

2022-07-21 12:27:41

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, Jul 21, 2022 at 12:56:37PM +0100, Mark Brown wrote:
> On Thu, Jul 21, 2022 at 12:20:31PM +0100, Mark Brown wrote:
>
> > You could add a way to specify constant base loads in DT on either a per
> > regulator or per consumer basis.
>
> ...and also note that this is only an issue if the system gives
> permission to change the mode in the constraints which is pretty rare.

Yeah, apparently only Qualcomm is using regulator-allow-set-load at the
moment, but it seems pretty common there.

We should probably just drop that from the platforms affected by this
particular regression and perhaps later add it back where it makes
sense (e.g. after making sure all consumers specify a load in some way).

Johan

Attachments:

(No filename) (731.00 B)
signature.asc (235.00 B)
Download all attachments

2022-07-21 13:00:09

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, Jul 21, 2022 at 12:20:31PM +0100, Mark Brown wrote:

> You could add a way to specify constant base loads in DT on either a per
> regulator or per consumer basis.

...and also note that this is only an issue if the system gives
permission to change the mode in the constraints which is pretty rare.

Attachments:

(No filename) (314.00 B)
signature.asc (499.00 B)
Download all attachments

2022-07-21 13:37:06

by Dmitry Baryshkov

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, 21 Jul 2022 at 13:31, Johan Hovold <[email protected]> wrote:
>
> On Tue, Jul 05, 2022 at 09:29:13AM -0700, Kuogee Hsieh wrote:
> > 0) rebase on https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git tree
> > 1) add regulator_set_load() to eDP phy
> > 2) add regulator_set_load() to DP phy
> > 3) remove vdda related function out of eDP/DP controller
> >
> > Kuogee Hsieh (3):
> > phy: qcom-edp: add regulator_set_load to edp phy
> > phy: qcom-qmp: add regulator_set_load to dp phy
> > drm/msm/dp: delete vdda regulator related functions from eDP/DP
> > controller
> >
> > drivers/gpu/drm/msm/dp/dp_parser.c | 14 -----
> > drivers/gpu/drm/msm/dp/dp_parser.h | 8 ---
> > drivers/gpu/drm/msm/dp/dp_power.c | 95 +------------------------------
> > drivers/phy/qualcomm/phy-qcom-edp.c | 12 ++++
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 41 ++++++++++---
> > 5 files changed, 46 insertions(+), 124 deletions(-)
>
> This series breaks USB and PCIe for some SC8280XP and SA540P machines
> where the DP PHY regulators are shared with other PHYs whose drivers do
> not request a load.

I'm trying to understand, why does this series cause the regression.
Previously it would be the DP controller voting on the regulators
load, now it has been moved to the DP/eDP PHYs.

> Specifically, the hard-coded vdda-phy load of 21.8 mA added by this
> series, causes several RPMh regulators to now be put in low-power mode.
>
> I found Doug's suggestion to handle situations like this in regulator
> core:
>
> https://lore.kernel.org/all/[email protected]/
>
> but since that was rejected, how do we deal with this generally?
>
> In the above thread Doug mentioned adding support for load requests to
> further drivers and Bjorn mentioned working around it by adding
> regulator-system-load properties to DT.
>
> It seems quite likely that changes like this one affects other systems
> too, and the effects may be hard to debug. So a more general solution
> than playing whack-a-mole using DT would be good to have.

I think enabling a regulator which supports set_load() and without
load being set should cause a warning, at least with
CONFIG_REGULATOR_DEBUG. WDYT?

--
With best wishes
Dmitry

2022-07-21 14:46:08

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

Hi,

On Thu, Jul 21, 2022 at 4:20 AM Mark Brown <[email protected]> wrote:
>
> On Thu, Jul 21, 2022 at 12:31:41PM +0200, Johan Hovold wrote:
>
> If you're copying someone into a thread that's not obviously relevant
> for them it's good practice to put a note about it at the top of the
> mail to reduce the chances that it just gets deleted unread - people get
> copies of all sorts of random stuff for not great reasons (like getting
> pulled in by checkpatch due to once having done a cleanup) and are often
> quicky to delete things.
>
> > This series breaks USB and PCIe for some SC8280XP and SA540P machines
> > where the DP PHY regulators are shared with other PHYs whose drivers do
> > not request a load.
>
> > Specifically, the hard-coded vdda-phy load of 21.8 mA added by this
> > series, causes several RPMh regulators to now be put in low-power mode.
>
> > I found Doug's suggestion to handle situations like this in regulator
> > core:
> >
> > https://lore.kernel.org/all/[email protected]/
>
> > but since that was rejected, how do we deal with this generally?
>
> > In the above thread Doug mentioned adding support for load requests to
> > further drivers and Bjorn mentioned working around it by adding
> > regulator-system-load properties to DT.
>
> > It seems quite likely that changes like this one affects other systems
> > too, and the effects may be hard to debug. So a more general solution
> > than playing whack-a-mole using DT would be good to have.
>
> You could add a way to specify constant base loads in DT on either a per
> regulator or per consumer basis.

Yes, this please! ...on a per consumer basis. :-) It's been on my
wishlist for a while and would eliminate a massive amount of code /
tables in the drivers.

We could debate syntax, but I guess you'd either do it w/ two cells

vdda-phy-supply = <&vdda_mipi_dsi0_1p2 21800>;

...or with matching properties:

vdda-phy-supply = <&vdda_mipi_dsi0_1p2>;
vdda-phy-supply-base-load = <21800>;

-Doug

2022-07-21 15:09:55

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

Hi,

On Thu, Jul 21, 2022 at 6:25 AM Dmitry Baryshkov
<[email protected]> wrote:
>
> > This series breaks USB and PCIe for some SC8280XP and SA540P machines
> > where the DP PHY regulators are shared with other PHYs whose drivers do
> > not request a load.
>
> I'm trying to understand, why does this series cause the regression.
> Previously it would be the DP controller voting on the regulators
> load, now it has been moved to the DP/eDP PHYs.

I think in the past not many device trees actually hooked up the
regulators to the DP/eDP but did hook up the regulators to the PHYs?
That means they didn't used to get a regulator_set_load() on them and
now they do?

It should also be noted that we're now setting the load for a bunch of
USB PHYs that we didn't used to set a load on...

> > It seems quite likely that changes like this one affects other systems
> > too, and the effects may be hard to debug. So a more general solution
> > than playing whack-a-mole using DT would be good to have.
>
> I think enabling a regulator which supports set_load() and without
> load being set should cause a warning, at least with
> CONFIG_REGULATOR_DEBUG. WDYT?

I'm not a total fan. I'd love to see evidence to the contrary, but I'm
a believer that the amount of extra cruft involved with all these
regulator_set_load() calls is overkill for most cases. All the extra
code / per-SoC tables added to drivers isn't ideal.

Every single LDO on Qualcomm's PMICs seems to be able to be set in
"high power mode" and "low power mode", but I think the majority of
clients only really care about two things: on and in high power mode
vs. off. I think the amount of stuff peripherals can do in low power
mode is super limited, so you have to be _really_ sure that the
peripheral won't draw too much current without you having a chance to
reconfigure the regulator.

Of course, if the load could be easily specified in the device tree
and we could get rid of all the cruft in the drivers then maybe that
would be OK.

-Doug

2022-07-21 15:26:13

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

Hi,

On Thu, Jul 21, 2022 at 7:39 AM Doug Anderson <[email protected]> wrote:
>
> > You could add a way to specify constant base loads in DT on either a per
> > regulator or per consumer basis.
>
> Yes, this please! ...on a per consumer basis. :-) It's been on my
> wishlist for a while and would eliminate a massive amount of code /
> tables in the drivers.
>
> We could debate syntax, but I guess you'd either do it w/ two cells
>
> vdda-phy-supply = <&vdda_mipi_dsi0_1p2 21800>;
>
> ...or with matching properties:
>
> vdda-phy-supply = <&vdda_mipi_dsi0_1p2>;
> vdda-phy-supply-base-load = <21800>;

Ah, sorry to respond to my own thread so quickly, but I just thought
of a reason for the "matching properties" solution instead of the two
cells. It would allow the SoC "dtsi" file to specify a base load while
the board "dts" file can then specify the supply. That feels like it
could be a nice solution.

-Doug

2022-07-21 15:33:19

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, Jul 21, 2022 at 07:49:55AM -0700, Doug Anderson wrote:

> Every single LDO on Qualcomm's PMICs seems to be able to be set in
> "high power mode" and "low power mode", but I think the majority of
> clients only really care about two things: on and in high power mode
> vs. off. I think the amount of stuff peripherals can do in low power
> mode is super limited, so you have to be _really_ sure that the
> peripheral won't draw too much current without you having a chance to
> reconfigure the regulator.

*Generally* a low power mode would be mainly useful for low power
retention type states, not active use.

Attachments:

(No filename) (630.00 B)
signature.asc (499.00 B)
Download all attachments

2022-07-21 16:04:49

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

Hi,

On Thu, Jul 21, 2022 at 8:06 AM Mark Brown <[email protected]> wrote:
>
> On Thu, Jul 21, 2022 at 07:49:55AM -0700, Doug Anderson wrote:
>
> > Every single LDO on Qualcomm's PMICs seems to be able to be set in
> > "high power mode" and "low power mode", but I think the majority of
> > clients only really care about two things: on and in high power mode
> > vs. off. I think the amount of stuff peripherals can do in low power
> > mode is super limited, so you have to be _really_ sure that the
> > peripheral won't draw too much current without you having a chance to
> > reconfigure the regulator.
>
> *Generally* a low power mode would be mainly useful for low power
> retention type states, not active use.

Right. Certainly the case I've seen where it is most useful is in S3
where we need to keep a device powered just enough to detect a wakeup,
but it can definitely also be useful for mostly idle devices that we
need to keep powered to retain memory so they can start up again
quickly.

I guess I'd put it this way, though: how many drivers in Linux today
have _two_ calls to regulator_set_load(): one for the "active" state
and one for the retention state. Looks like UFS maybe. Any others? For
most devices the pattern is:

* get all of our regulators.
* for each regulator, set the load to something that will trigger HPM
when we're using the regulator.
* turn regulators on when we need power and off when we don't.

All the extra scaffolding and tables to pass something to
regulator_set_load() is just a lot of noise to add for drivers that
don't have any concept of "retention" mode and don't need it.

-Doug

2022-07-21 16:11:50

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, Jul 21, 2022 at 08:43:48AM -0700, Doug Anderson wrote:

> I guess I'd put it this way, though: how many drivers in Linux today
> have _two_ calls to regulator_set_load(): one for the "active" state
> and one for the retention state. Looks like UFS maybe. Any others? For
> most devices the pattern is:

Oh, I'm not saying there's sensible implementations in drivers. In
general I'd say that as with voltage if a driver is not actively
managing the load during runtime it should not be setting it at all, one
time configuration should be left to the constraints if it's needed.

Attachments:

(No filename) (597.00 B)
signature.asc (499.00 B)
Download all attachments

2022-07-21 16:31:43

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, Jul 21, 2022 at 07:52:01AM -0700, Doug Anderson wrote:
> On Thu, Jul 21, 2022 at 7:39 AM Doug Anderson <[email protected]> wrote:

> > vdda-phy-supply = <&vdda_mipi_dsi0_1p2>;
> > vdda-phy-supply-base-load = <21800>;

> Ah, sorry to respond to my own thread so quickly, but I just thought
> of a reason for the "matching properties" solution instead of the two
> cells. It would allow the SoC "dtsi" file to specify a base load while
> the board "dts" file can then specify the supply. That feels like it
> could be a nice solution.

For a per device thing it probably shouldn't be called a "base load",
it's just the load. I would question how useful this would be, aside
from the concerns vendors will likely have with disclosing actual power
consumptions if the number never changes then it is immaterial which
device contributes which milliamp of load, you just want to configure a
fixed mode on the regulator and have done with it.

Attachments:

(No filename) (968.00 B)
signature.asc (499.00 B)
Download all attachments

2022-07-21 18:56:47

by Dmitry Baryshkov

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Thu, 21 Jul 2022 at 17:50, Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Thu, Jul 21, 2022 at 6:25 AM Dmitry Baryshkov
> <[email protected]> wrote:
> >
> > > This series breaks USB and PCIe for some SC8280XP and SA540P machines
> > > where the DP PHY regulators are shared with other PHYs whose drivers do
> > > not request a load.
> >
> > I'm trying to understand, why does this series cause the regression.
> > Previously it would be the DP controller voting on the regulators
> > load, now it has been moved to the DP/eDP PHYs.
>
> I think in the past not many device trees actually hooked up the
> regulators to the DP/eDP but did hook up the regulators to the PHYs?
> That means they didn't used to get a regulator_set_load() on them and
> now they do?
>
> It should also be noted that we're now setting the load for a bunch of
> USB PHYs that we didn't used to set a load on...

Might be the case, yes.

> > > It seems quite likely that changes like this one affects other systems
> > > too, and the effects may be hard to debug. So a more general solution
> > > than playing whack-a-mole using DT would be good to have.
> >
> > I think enabling a regulator which supports set_load() and without
> > load being set should cause a warning, at least with
> > CONFIG_REGULATOR_DEBUG. WDYT?
>
> I'm not a total fan. I'd love to see evidence to the contrary, but I'm
> a believer that the amount of extra cruft involved with all these
> regulator_set_load() calls is overkill for most cases. All the extra
> code / per-SoC tables added to drivers isn't ideal.

I'm fine with the load being specified in the DT (and that sounds like
a good idea for me too).
What I'd like to achieve is catching load-enabled regulators for which
we did not set the load (via an API call or via the DT).

--
With best wishes
Dmitry

2022-07-22 01:42:12

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

Hi,

On Thu, Jul 21, 2022 at 7:52 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Thu, Jul 21, 2022 at 7:39 AM Doug Anderson <[email protected]> wrote:
> >
> > > You could add a way to specify constant base loads in DT on either a per
> > > regulator or per consumer basis.
> >
> > Yes, this please! ...on a per consumer basis. :-) It's been on my
> > wishlist for a while and would eliminate a massive amount of code /
> > tables in the drivers.
> >
> > We could debate syntax, but I guess you'd either do it w/ two cells
> >
> > vdda-phy-supply = <&vdda_mipi_dsi0_1p2 21800>;
> >
> > ...or with matching properties:
> >
> > vdda-phy-supply = <&vdda_mipi_dsi0_1p2>;
> > vdda-phy-supply-base-load = <21800>;
>
> Ah, sorry to respond to my own thread so quickly, but I just thought
> of a reason for the "matching properties" solution instead of the two
> cells. It would allow the SoC "dtsi" file to specify a base load while
> the board "dts" file can then specify the supply. That feels like it
> could be a nice solution.

This seemed easy, so I whipped up a quick prototype. Forewarned that I
did very little detailed testing. I didn't CC everyone on this thread,
but hopefully folks can find it if they are interested...

https://lore.kernel.org/r/20220721182622.RFC.1.I8a64b707169cfd73d9309c5eaf5d43b8bc4db988@changeid

-Doug

2022-07-29 13:41:49

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

Sorry about the late follow-up on this. Has been one of those
constant-preemption weeks.

And thanks Doug and Mark for the insightful comments in this and the
dt-load RFC thread.

On Thu, Jul 21, 2022 at 07:49:55AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, Jul 21, 2022 at 6:25 AM Dmitry Baryshkov
> <[email protected]> wrote:
> >
> > > This series breaks USB and PCIe for some SC8280XP and SA540P machines
> > > where the DP PHY regulators are shared with other PHYs whose drivers do
> > > not request a load.
> >
> > I'm trying to understand, why does this series cause the regression.
> > Previously it would be the DP controller voting on the regulators
> > load, now it has been moved to the DP/eDP PHYs.
>
> I think in the past not many device trees actually hooked up the
> regulators to the DP/eDP but did hook up the regulators to the PHYs?
> That means they didn't used to get a regulator_set_load() on them and
> now they do?

This was the case for us, but this could probably also partly be blamed
on the DP stuff being work-in-progress and the regulators were only ever
specified in the PHY nodes.

> It should also be noted that we're now setting the load for a bunch of
> USB PHYs that we didn't used to set a load on...

That was my concern. But after skimming the mainline dts it seems that
no users of these drivers are currently allowing the regulator mode to
be changed for those particular regulators.

Well, except for the sc8280xp (derivate) machines in -next that I'm
working on and that regressed of course.

I guess we just need to drop all those regulator-allow-set-load
properties for now even if using DT for power-management configuration
this way does seem to run against the whole DT-as-hardware-description
idea (e.g. we may want to add them back when/if active- and idle loads
are specified by the corresponding Linux drivers).

> > > It seems quite likely that changes like this one affects other systems
> > > too, and the effects may be hard to debug. So a more general solution
> > > than playing whack-a-mole using DT would be good to have.
> >
> > I think enabling a regulator which supports set_load() and without
> > load being set should cause a warning, at least with
> > CONFIG_REGULATOR_DEBUG. WDYT?
>
> I'm not a total fan. I'd love to see evidence to the contrary, but I'm
> a believer that the amount of extra cruft involved with all these
> regulator_set_load() calls is overkill for most cases. All the extra
> code / per-SoC tables added to drivers isn't ideal.
>
> Every single LDO on Qualcomm's PMICs seems to be able to be set in
> "high power mode" and "low power mode", but I think the majority of
> clients only really care about two things: on and in high power mode
> vs. off. I think the amount of stuff peripherals can do in low power
> mode is super limited, so you have to be _really_ sure that the
> peripheral won't draw too much current without you having a chance to
> reconfigure the regulator.
>
> Of course, if the load could be easily specified in the device tree
> and we could get rid of all the cruft in the drivers then maybe that
> would be OK.

It seems a decision was made against this (which I tend to agree with),
and that instead it is now easier for drivers to specify loads using
your updated bulk-regulator implementation.

But that doesn't address the problem that was trying to highlight here,
and that you had noticed years ago, namely that using set_load only
works reliably if *all* consumers use it.

Otherwise we may end up with a couple of drivers adding some 10 mA each to
the load, while other driver do not specify a load so that while the
actual load is really above the 30 mA HPM threshold, we still end up in
LPM.

Shouldn't an enabled regulator from a consumer that didn't specify a
load somehow result in HPM always being selected (e.g. count as INT_MAX
load as Doug suggested some years ago)?

Note that LPM may be ok until that last consumer that didn't specify a
load pushes it over the HPM threshold (so specifying a regulator base
load in DT would also prevent ever selecting LPM).

This case of mixing consumers that specify and don't specify loads might
at least warrant a warning otherwise to avoid ending up in a
hard-to-debug situation with failing USB and PCIe due to a seemingly
unrelated DP PHY update.

At some point in the discussion I thought Mark suggested removing
set_load from drivers that don't actually manage active and idle loads.
That would also work, at least until the day one of the drivers adds
support for idle loads.

Johan

2022-07-29 14:38:59

[permalink] [raw]

Subject: Re: [PATCH v16 0/3] eDP/DP Phy vdda realted function

On Fri, Jul 29, 2022 at 03:35:33PM +0200, Johan Hovold wrote:

> I guess we just need to drop all those regulator-allow-set-load
> properties for now even if using DT for power-management configuration
> this way does seem to run against the whole DT-as-hardware-description
> idea (e.g. we may want to add them back when/if active- and idle loads
> are specified by the corresponding Linux drivers).

Well, there's also a question of if the hardware can usefully make use
of the facility - is there any non-suspend state where the regulator
needs to be on but is drawing so little current that it's worth trying
to select a lower power mode?

> But that doesn't address the problem that was trying to highlight here,
> and that you had noticed years ago, namely that using set_load only
> works reliably if *all* consumers use it.

> Shouldn't an enabled regulator from a consumer that didn't specify a
> load somehow result in HPM always being selected (e.g. count as INT_MAX
> load as Doug suggested some years ago)?

Possibly, but note that as well as the consumers with software drivers
you also have to consider any passive consumers on the board which may
not have any representation in DT so the actual numbers may well be off
even if every consumer is trying to keep things up to date. You also
come back to the "let's just shove a random number in here" problem.

For ultimate saftey we probably want a command line option to gate the
feature which people can set to say they've audited their full
software/hardware integration stack.

> At some point in the discussion I thought Mark suggested removing
> set_load from drivers that don't actually manage active and idle loads.
> That would also work, at least until the day one of the drivers adds
> support for idle loads.

Yes, if the driver isn't actively managing loads it's probably not doing
anything useful.

The difficulties with this sort of system integration question is an
unfortunate consequence of DT, having to describe what's safe for an
unknown software stack is fundamentally hard. I do question how much
effort it's worth putting into enabling this, especially in cases where
the regulator is shared - how much power is actually saved in the grand
scheme of things given that this is only taking effect when the system
is out of suspend and we tend to be talking about some percentage of the
power being drawn on something which is presumably already consuming
very little power for this to be at all relevant?

Attachments:

(No filename) (2.48 kB)
signature.asc (499.00 B)
Download all attachments

2022-08-03 08:57:57