2022-08-03 12:58:49

by Johan Hovold

[permalink] [raw]
Subject: [PATCH 0/2] phy: qcom: drop regulator loads

Unless a driver implements an idle mode, there's generally no point in
specifying an active-mode regulator load.

Drop the regulator loads that were recently added to the Qualcomm QMP
combo and edp PHY drivers.

For a background discussion on this matter, see the following thread:

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

Johan


Johan Hovold (2):
phy: qcom-qmp-combo: drop regulator loads
phy: qcom-edp: drop regulator loads

drivers/phy/qualcomm/phy-qcom-edp.c | 12 -------
drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 40 +++++------------------
2 files changed, 9 insertions(+), 43 deletions(-)

--
2.35.1



2022-08-03 13:17:59

by Johan Hovold

[permalink] [raw]
Subject: [PATCH 2/2] phy: qcom-edp: drop regulator loads

Drivers should not be specifying active-mode regulator loads unless
supporting an idle mode where the load is reduced during runtime.

This effectively reverts commit a4888b2005d1 ("phy: qcom-edp: add
regulator_set_load to edp phy")

Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/phy/qualcomm/phy-qcom-edp.c | 12 ------------
1 file changed, 12 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
index 7e3570789845..cacd32f6e0cc 100644
--- a/drivers/phy/qualcomm/phy-qcom-edp.c
+++ b/drivers/phy/qualcomm/phy-qcom-edp.c
@@ -639,18 +639,6 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
if (ret)
return ret;

- ret = regulator_set_load(edp->supplies[0].consumer, 21800); /* 1.2 V vdda-phy */
- if (ret) {
- dev_err(dev, "failed to set load at %s\n", edp->supplies[0].supply);
- return ret;
- }
-
- ret = regulator_set_load(edp->supplies[1].consumer, 36000); /* 0.9 V vdda-pll */
- if (ret) {
- dev_err(dev, "failed to set load at %s\n", edp->supplies[1].supply);
- return ret;
- }
-
ret = qcom_edp_clks_register(edp, pdev->dev.of_node);
if (ret)
return ret;
--
2.35.1


2022-08-03 15:35:11

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH 0/2] phy: qcom: drop regulator loads

Hi,

On Wed, Aug 3, 2022 at 5:34 AM Johan Hovold <[email protected]> wrote:
>
> Unless a driver implements an idle mode, there's generally no point in
> specifying an active-mode regulator load.
>
> Drop the regulator loads that were recently added to the Qualcomm QMP
> combo and edp PHY drivers.
>
> For a background discussion on this matter, see the following thread:
>
> https://lore.kernel.org/r/[email protected]
>
> Johan
>
>
> Johan Hovold (2):
> phy: qcom-qmp-combo: drop regulator loads
> phy: qcom-edp: drop regulator loads
>
> drivers/phy/qualcomm/phy-qcom-edp.c | 12 -------
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 40 +++++------------------
> 2 files changed, 9 insertions(+), 43 deletions(-)

It's really hard to evaluate this based on the information I have
available to me. :( I'm all for getting rid of all this complexity if
it was added for no reason and I could definitely believe that on most
boards there is no reason for it at all as talked about in other
threads.

I guess I worry that there is some use case where LPM mode is actually
usable to power these devices when they're active. It seems like
_maybe_ it could be but only if nothing else is pulling power from the
same LDO? Some LDOs on the board I have seem to be able to do LPM up
to 30 mA and some of the rails are being specified as ~22mA.

The problem with regulator loads is that using them is kinda an "all
or nothing". Either all the consumers need to specify something or
none of them can. :( This means that once the first user comes in and
is able to run the device in LPM (maybe only if they're the only
consumer?) that everything will break. I honestly have no idea if this
will ever happen, though... Mark said the phrase "actively managing
loads it's probably not doing anything useful" and I think "probably"
is an important word there. If that word was "never" then it would
definitely be OK to remove load management like this, but with
"probably" it becomes a lot harder.

If we needed a hack, I'd somewhat prefer a hack that just bumps the
"mA" value here up to something higher. That would force it to HPM...
...although maybe it still won't work? Then the regulator will still
go down to LPM for other consumers if the PHY ever turns off. In that
case I guess there's no getting around other consumers requesting the
load or finding some way to say that on your board this regulator can
only ever be in HPM mode.


-Doug