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

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

[Kuogee Hsieh]

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(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v16 1/3] phy: qcom-edp: add regulator_set_load to edp phy

[Kuogee Hsieh]

This patch add regulator_set_load() before enable regulator at
eDP phy driver.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/phy/qualcomm/phy-qcom-edp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
index cacd32f..7e357078 100644
--- a/drivers/phy/qualcomm/phy-qcom-edp.c
+++ b/drivers/phy/qualcomm/phy-qcom-edp.c
@@ -639,6 +639,18 @@ 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;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v16 2/3] phy: qcom-qmp: add regulator_set_load to dp phy

[Kuogee Hsieh]

This patch add regulator_set_load() before enable regulator at
DP phy driver.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 41 ++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 893b5a4..22046cf 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -604,6 +604,18 @@ static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[] = {
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_EMP_POST1_LVL, 0x20),
 };
 
+
+/* list of regulators */
+struct qmp_regulator_data {
+	const char *name;
+	unsigned int enable_load;
+};
+
+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 */
@@ -646,7 +658,7 @@ struct qmp_phy_cfg {
 	const char * const *reset_list;
 	int num_resets;
 	/* regulators to be requested */
-	const char * const *vreg_list;
+	const struct qmp_regulator_data *vreg_list;
 	int num_vregs;
 
 	/* array of registers with different offsets */
@@ -809,11 +821,6 @@ 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,
@@ -1969,16 +1976,32 @@ 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 i;
+	int ret, 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];
+		qmp->vregs[i].supply = cfg->vreg_list[i].name;
+
+	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 devm_regulator_bulk_get(dev, num, qmp->vregs);
+	Return 0;
 }
 
 static int qcom_qmp_phy_combo_reset_init(struct device *dev, const struct qmp_phy_cfg *cfg)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v16 3/3] drm/msm/dp: delete vdda regulator related functions from eDP/DP controller

[Kuogee Hsieh]

Vdda regulators are related to both eDP and DP phy so that it should be
managed at eDP and DP phy driver instead of controller. This patch removes
vdda regulators related functions out of eDP/DP controller.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 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 +-------------------------------------
 3 files changed, 2 insertions(+), 115 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 8f9fed9..4ef2130 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -22,14 +22,6 @@
 #define DP_DEFAULT_P0_OFFSET	0x1000
 #define DP_DEFAULT_P0_SIZE	0x0400
 
-static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
-	.num = 2,
-	.regs = {
-		{"vdda-1p2", 21800, 4 },	/* 1.2 V */
-		{"vdda-0p9", 36000, 32 },	/* 0.9 V */
-	},
-};
-
 static void __iomem *dp_ioremap(struct platform_device *pdev, int idx, size_t *len)
 {
 	struct resource *res;
@@ -298,12 +290,6 @@ static int dp_parser_parse(struct dp_parser *parser)
 	if (rc)
 		return rc;
 
-	/* Map the corresponding regulator information according to
-	 * version. Currently, since we only have one supported platform,
-	 * mapping the regulator directly.
-	 */
-	parser->regulator_cfg = &sdm845_dp_reg_cfg;
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 3a4d797..47430e3 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -92,8 +92,6 @@ struct dp_pinctrl {
 	struct pinctrl_state *state_suspend;
 };
 
-#define DP_DEV_REGULATOR_MAX	4
-
 /* Regulators for DP devices */
 struct dp_reg_entry {
 	char name[32];
@@ -101,11 +99,6 @@ struct dp_reg_entry {
 	int disable_load;
 };
 
-struct dp_regulator_cfg {
-	int num;
-	struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
-};
-
 /**
  * struct dp_parser - DP parser's data exposed to clients
  *
@@ -121,7 +114,6 @@ struct dp_parser {
 	struct dp_pinctrl pinctrl;
 	struct dp_io io;
 	struct dp_display_data disp_data;
-	const struct dp_regulator_cfg *regulator_cfg;
 	u32 max_dp_lanes;
 	struct drm_bridge *next_bridge;
 
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index d9e0117..b52ac1d 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -20,82 +20,10 @@ struct dp_power_private {
 	struct clk *link_clk_src;
 	struct clk *pixel_provider;
 	struct clk *link_provider;
-	struct regulator_bulk_data supplies[DP_DEV_REGULATOR_MAX];
 
 	struct dp_power dp_power;
 };
 
-static void dp_power_regulator_disable(struct dp_power_private *power)
-{
-	struct regulator_bulk_data *s = power->supplies;
-	const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
-	int num = power->parser->regulator_cfg->num;
-	int i;
-
-	DBG("");
-	for (i = num - 1; i >= 0; i--)
-		if (regs[i].disable_load >= 0)
-			regulator_set_load(s[i].consumer,
-					   regs[i].disable_load);
-
-	regulator_bulk_disable(num, s);
-}
-
-static int dp_power_regulator_enable(struct dp_power_private *power)
-{
-	struct regulator_bulk_data *s = power->supplies;
-	const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
-	int num = power->parser->regulator_cfg->num;
-	int ret, i;
-
-	DBG("");
-	for (i = 0; i < num; i++) {
-		if (regs[i].enable_load >= 0) {
-			ret = regulator_set_load(s[i].consumer,
-						 regs[i].enable_load);
-			if (ret < 0) {
-				pr_err("regulator %d set op mode failed, %d\n",
-					i, ret);
-				goto fail;
-			}
-		}
-	}
-
-	ret = regulator_bulk_enable(num, s);
-	if (ret < 0) {
-		pr_err("regulator enable failed, %d\n", ret);
-		goto fail;
-	}
-
-	return 0;
-
-fail:
-	for (i--; i >= 0; i--)
-		regulator_set_load(s[i].consumer, regs[i].disable_load);
-	return ret;
-}
-
-static int dp_power_regulator_init(struct dp_power_private *power)
-{
-	struct regulator_bulk_data *s = power->supplies;
-	const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
-	struct platform_device *pdev = power->pdev;
-	int num = power->parser->regulator_cfg->num;
-	int i, ret;
-
-	for (i = 0; i < num; i++)
-		s[i].supply = regs[i].name;
-
-	ret = devm_regulator_bulk_get(&pdev->dev, num, s);
-	if (ret < 0) {
-		pr_err("%s: failed to init regulator, ret=%d\n",
-						__func__, ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 static int dp_power_clk_init(struct dp_power_private *power)
 {
 	int rc = 0;
@@ -318,21 +246,10 @@ int dp_power_client_init(struct dp_power *dp_power)
 
 	pm_runtime_enable(&power->pdev->dev);
 
-	rc = dp_power_regulator_init(power);
-	if (rc) {
-		DRM_ERROR("failed to init regulators %d\n", rc);
-		goto error;
-	}
-
 	rc = dp_power_clk_init(power);
-	if (rc) {
+	if (rc)
 		DRM_ERROR("failed to init clocks %d\n", rc);
-		goto error;
-	}
-	return 0;
 
-error:
-	pm_runtime_disable(&power->pdev->dev);
 	return rc;
 }
 
@@ -365,22 +282,15 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	pm_runtime_get_sync(&power->pdev->dev);
-	rc = dp_power_regulator_enable(power);
-	if (rc) {
-		DRM_ERROR("failed to enable regulators, %d\n", rc);
-		goto exit;
-	}
 
 	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
 	if (rc) {
 		DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
-		goto err_clk;
+		goto exit;
 	}
 
 	return 0;
 
-err_clk:
-	dp_power_regulator_disable(power);
 exit:
 	pm_runtime_put_sync(&power->pdev->dev);
 	return rc;
@@ -393,7 +303,6 @@ int dp_power_deinit(struct dp_power *dp_power)
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
-	dp_power_regulator_disable(power);
 	pm_runtime_put_sync(&power->pdev->dev);
 	return 0;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Vinod Koul]

On 05-07-22, 09:29, 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

Applied, thanks

-- 
~Vinod

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

[Johan Hovold]

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/20180814170617.100087-1-dianders@chromium.org/

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

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/20180814170617.100087-1-dianders@chromium.org/

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 4:20 AM Mark Brown <broonie@kernel.org> 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/20180814170617.100087-1-dianders@chromium.org/
>
> > 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

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

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 7:39 AM Doug Anderson <dianders@chromium.org> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

On Thu, Jul 21, 2022 at 07:52:01AM -0700, Doug Anderson wrote:
> On Thu, Jul 21, 2022 at 7:39 AM Doug Anderson <dianders@chromium.org> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 7:52 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jul 21, 2022 at 7:39 AM Doug Anderson <dianders@chromium.org> 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

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

[Dmitry Baryshkov]

On Thu, 21 Jul 2022 at 13:31, Johan Hovold <johan@kernel.org> 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/20180814170617.100087-1-dianders@chromium.org/
>
> 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

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

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 6:25 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Doug Anderson]

Hi,

On Thu, Jul 21, 2022 at 8:06 AM Mark Brown <broonie@kernel.org> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Dmitry Baryshkov]

On Thu, 21 Jul 2022 at 17:50, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jul 21, 2022 at 6:25 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> 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

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

[Johan Hovold]

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
> <dmitry.baryshkov@linaro.org> 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]

On Fri, Jul 29, 2022 at 03:07:47PM +0100, Mark Brown wrote:
> 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?

Good point.

> > 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.

Right, but some of that could be captured in DT with
'regulator-system-load'.

> 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.

That sounds like it could be useful.
 
> > 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.

Ok, thanks for confirming. Perhaps we should drop the set_loads() added
to the PHY driver by this series then.
 
> 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?

I tend to agree. Thanks again for your input!

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]