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

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

[Johan Hovold]

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/YuPps+cvVAMugWmy@sirena.org.uk

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

[PATCH 1/2] phy: qcom-qmp-combo: drop regulator loads

[Johan Hovold]

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 85936d4f3815 ("phy: qcom-qmp: add
regulator_set_load to dp phy").

Link: https://lore.kernel.org/r/YuPps+cvVAMugWmy@sirena.org.uk
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 40 +++++------------------
 1 file changed, 9 insertions(+), 31 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 4b1828976104..3663252622be 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -607,17 +607,6 @@ static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[] = {
 };
 
 
-/* list of regulators */
-struct qmp_regulator_data {
-	const char *name;
-	unsigned int enable_load;
-};
-
-static struct qmp_regulator_data qmp_phy_vreg_l[] = {
-	{ .name = "vdda-phy", .enable_load = 21800 },
-	{ .name = "vdda-pll", .enable_load = 36000 },
-};
-
 struct qmp_phy;
 
 /* struct qmp_phy_cfg - per-PHY initialization config */
@@ -662,7 +651,7 @@ struct qmp_phy_cfg {
 	const char * const *reset_list;
 	int num_resets;
 	/* regulators to be requested */
-	const struct qmp_regulator_data *vreg_list;
+	const char * const *vreg_list;
 	int num_vregs;
 
 	/* array of registers with different offsets */
@@ -831,6 +820,11 @@ static const char * const sc7180_usb3phy_reset_l[] = {
 	"phy",
 };
 
+/* list of regulators */
+static const char * const qmp_phy_vreg_l[] = {
+	"vdda-phy", "vdda-pll",
+};
+
 static const struct qmp_phy_cfg sc7180_usb3phy_cfg = {
 	.type			= PHY_TYPE_USB3,
 	.nlanes			= 1,
@@ -1992,32 +1986,16 @@ static int qcom_qmp_phy_combo_vreg_init(struct device *dev, const struct qmp_phy
 {
 	struct qcom_qmp *qmp = dev_get_drvdata(dev);
 	int num = cfg->num_vregs;
-	int ret, i;
+	int i;
 
 	qmp->vregs = devm_kcalloc(dev, num, sizeof(*qmp->vregs), GFP_KERNEL);
 	if (!qmp->vregs)
 		return -ENOMEM;
 
 	for (i = 0; i < num; i++)
-		qmp->vregs[i].supply = cfg->vreg_list[i].name;
+		qmp->vregs[i].supply = cfg->vreg_list[i];
 
-	ret = devm_regulator_bulk_get(dev, num, qmp->vregs);
-	if (ret) {
-		dev_err(dev, "failed at devm_regulator_bulk_get\n");
-		return ret;
-	}
-
-	for (i = 0; i < num; i++) {
-		ret = regulator_set_load(qmp->vregs[i].consumer,
-					cfg->vreg_list[i].enable_load);
-		if (ret) {
-			dev_err(dev, "failed to set load at %s\n",
-				qmp->vregs[i].supply);
-			return ret;
-		}
-	}
-
-	return 0;
+	return devm_regulator_bulk_get(dev, num, qmp->vregs);
 }
 
 static int qcom_qmp_phy_combo_reset_init(struct device *dev, const struct qmp_phy_cfg *cfg)
-- 
2.35.1

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

[Johan Hovold]

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/YuPps+cvVAMugWmy@sirena.org.uk
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 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

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

[Doug Anderson]

Hi,

On Wed, Aug 3, 2022 at 5:34 AM Johan Hovold <johan+linaro@kernel.org> 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/YuPps+cvVAMugWmy@sirena.org.uk
>
> 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