Add support in QMP PHY driver to read optional vdda-phy-max-microamp
and vdda-pll-max-microamp properties from the device tree.

These properties define the expected maximum current (in microamps) for
each supply. The driver uses this information to configure regulators
more accurately and ensure they operate in the correct mode based on
client load requirements.

If the property is not present, the driver defaults load to
`QMP_VREG_UNUSED`.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 9c69c77d10c8..6e116f7370c3 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -39,6 +39,8 @@
 #define PHY_INIT_COMPLETE_TIMEOUT		10000
 
 #define NUM_OVERLAY				2
+#define QMP_VREG_UNUSED				-1
+#define MAX_PROP_NAME				32
 
 /* set of registers with offsets different per-PHY */
 enum qphy_reg_layout {
@@ -1894,15 +1896,29 @@ static int qmp_ufs_vreg_init(struct qmp_ufs *qmp)
 {
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 	struct device *dev = qmp->dev;
+	struct device_node *np = dev->of_node;
+	char prop_name[MAX_PROP_NAME];
 	int num = cfg->num_vregs;
-	int i;
+	const char *supply;
+	int i, ret;
 
 	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];
+	for (i = 0; i < num; i++) {
+		supply = cfg->vreg_list[i];
+		qmp->vregs[i].supply = supply;
+
+		/* Defaults to unused */
+		qmp->vregs[i].init_load_uA = QMP_VREG_UNUSED;
+
+		snprintf(prop_name, sizeof(prop_name), "%s-max-microamp", supply);
+		ret = of_property_read_u32(np, prop_name, &qmp->vregs[i].init_load_uA);
+		if (ret)
+			dev_dbg(qmp->dev, "No max microamp for %s, using default %d\n",
+				supply, QMP_VREG_UNUSED);
+	}
 
 	return devm_regulator_bulk_get(dev, num, qmp->vregs);
 }
-- 
2.48.1

On Wed, Aug 06, 2025 at 09:13:40PM +0530, Nitin Rawat wrote:
> Add support in QMP PHY driver to read optional vdda-phy-max-microamp
> and vdda-pll-max-microamp properties from the device tree.
> 
> These properties define the expected maximum current (in microamps) for
> each supply. The driver uses this information to configure regulators
> more accurately and ensure they operate in the correct mode based on
> client load requirements.

What defines those load values? Are they actually dependent on the
platform? On the SoC? On the board design? On the UFS gear?

> 
> If the property is not present, the driver defaults load to
> `QMP_VREG_UNUSED`.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 

-- 
With best wishes
Dmitry

On 8/9/2025 1:00 PM, Dmitry Baryshkov wrote:
> On Wed, Aug 06, 2025 at 09:13:40PM +0530, Nitin Rawat wrote:
>> Add support in QMP PHY driver to read optional vdda-phy-max-microamp
>> and vdda-pll-max-microamp properties from the device tree.
>>
>> These properties define the expected maximum current (in microamps) for
>> each supply. The driver uses this information to configure regulators
>> more accurately and ensure they operate in the correct mode based on
>> client load requirements.
> 
> What defines those load values? Are they actually dependent on the
> platform? On the SoC? On the board design? On the UFS gear?

Hi Dmitry,

These load values are defined at the SoC level, although they may vary 
depending on the UFS gear. However, the peak value per power grid is 
determined by the maximum gear supported by the controller.

In summary, the value is SoC-specific and does not depend on board-level 
variations. Based on this, I'm comfortable hardcoding per-compatible 
data directly in the driver instead of relying on the device tree.

Thanks,
Nitin


> 
>>
>> If the property is not present, the driver defaults load to
>> `QMP_VREG_UNUSED`.
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>

On Wed, Aug 06, 2025 at 09:13:40PM GMT, Nitin Rawat wrote:
> Add support in QMP PHY driver to read optional vdda-phy-max-microamp
> and vdda-pll-max-microamp properties from the device tree.
> 
> These properties define the expected maximum current (in microamps) for
> each supply. The driver uses this information to configure regulators
> more accurately and ensure they operate in the correct mode based on
> client load requirements.
> 
> If the property is not present, the driver defaults load to
> `QMP_VREG_UNUSED`.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 9c69c77d10c8..6e116f7370c3 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -39,6 +39,8 @@
>  #define PHY_INIT_COMPLETE_TIMEOUT		10000
>  
>  #define NUM_OVERLAY				2
> +#define QMP_VREG_UNUSED				-1
> +#define MAX_PROP_NAME				32
>  
>  /* set of registers with offsets different per-PHY */
>  enum qphy_reg_layout {
> @@ -1894,15 +1896,29 @@ static int qmp_ufs_vreg_init(struct qmp_ufs *qmp)
>  {
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  	struct device *dev = qmp->dev;
> +	struct device_node *np = dev->of_node;
> +	char prop_name[MAX_PROP_NAME];
>  	int num = cfg->num_vregs;
> -	int i;
> +	const char *supply;
> +	int i, ret;
>  
>  	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];
> +	for (i = 0; i < num; i++) {
> +		supply = cfg->vreg_list[i];
> +		qmp->vregs[i].supply = supply;
> +
> +		/* Defaults to unused */
> +		qmp->vregs[i].init_load_uA = QMP_VREG_UNUSED;

You don't need to initialize the load. It will be zero initialized at this point
and if the property parsing fails below, it's value still be zero. And the
regulator core will apply the load only if it is > 0.

> +
> +		snprintf(prop_name, sizeof(prop_name), "%s-max-microamp", supply);
> +		ret = of_property_read_u32(np, prop_name, &qmp->vregs[i].init_load_uA);
> +		if (ret)
> +			dev_dbg(qmp->dev, "No max microamp for %s, using default %d\n",
> +				supply, QMP_VREG_UNUSED);

This print doesn't make sense as it will print the default value of -1. Please
drop it.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On 8/6/25 5:43 PM, Nitin Rawat wrote:
> Add support in QMP PHY driver to read optional vdda-phy-max-microamp
> and vdda-pll-max-microamp properties from the device tree.
> 
> These properties define the expected maximum current (in microamps) for
> each supply. The driver uses this information to configure regulators
> more accurately and ensure they operate in the correct mode based on
> client load requirements.
> 
> If the property is not present, the driver defaults load to
> `QMP_VREG_UNUSED`.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 9c69c77d10c8..6e116f7370c3 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -39,6 +39,8 @@
>  #define PHY_INIT_COMPLETE_TIMEOUT		10000
>  
>  #define NUM_OVERLAY				2
> +#define QMP_VREG_UNUSED				-1
> +#define MAX_PROP_NAME				32
>  
>  /* set of registers with offsets different per-PHY */
>  enum qphy_reg_layout {
> @@ -1894,15 +1896,29 @@ static int qmp_ufs_vreg_init(struct qmp_ufs *qmp)
>  {
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  	struct device *dev = qmp->dev;
> +	struct device_node *np = dev->of_node;
> +	char prop_name[MAX_PROP_NAME];
>  	int num = cfg->num_vregs;
> -	int i;
> +	const char *supply;
> +	int i, ret;
>  
>  	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];
> +	for (i = 0; i < num; i++) {
> +		supply = cfg->vreg_list[i];
> +		qmp->vregs[i].supply = supply;
> +
> +		/* Defaults to unused */
> +		qmp->vregs[i].init_load_uA = QMP_VREG_UNUSED;
> +
> +		snprintf(prop_name, sizeof(prop_name), "%s-max-microamp", supply);
> +		ret = of_property_read_u32(np, prop_name, &qmp->vregs[i].init_load_uA);
> +		if (ret)
> +			dev_dbg(qmp->dev, "No max microamp for %s, using default %d\n",
> +				supply, QMP_VREG_UNUSED);

+Mark

do you think having this in regulator core would make sense?

Konrad

On Wed, Aug 06, 2025 at 05:58:30PM +0200, Konrad Dybcio wrote:
> On 8/6/25 5:43 PM, Nitin Rawat wrote:

> > Add support in QMP PHY driver to read optional vdda-phy-max-microamp
> > and vdda-pll-max-microamp properties from the device tree.

> > These properties define the expected maximum current (in microamps) for
> > each supply. The driver uses this information to configure regulators
> > more accurately and ensure they operate in the correct mode based on
> > client load requirements.

> > If the property is not present, the driver defaults load to
> > `QMP_VREG_UNUSED`.

> do you think having this in regulator core would make sense?

I'm not clear why the driver is trying to do this at all, the driver is
AFAICT making no other effort to manage the load at all.  We already
impose any constraints that are defined for a regulator while initially
parsing them so it's not clear to me what this is supposed to
accomplish, and it'll be broken if the supply is ever shared since it'll
set the load from this individual consumer to the maximum that's
permitted for the regulator as a whole.

On 8/6/25 6:51 PM, Mark Brown wrote:
> On Wed, Aug 06, 2025 at 05:58:30PM +0200, Konrad Dybcio wrote:
>> On 8/6/25 5:43 PM, Nitin Rawat wrote:
> 
>>> Add support in QMP PHY driver to read optional vdda-phy-max-microamp
>>> and vdda-pll-max-microamp properties from the device tree.
> 
>>> These properties define the expected maximum current (in microamps) for
>>> each supply. The driver uses this information to configure regulators
>>> more accurately and ensure they operate in the correct mode based on
>>> client load requirements.
> 
>>> If the property is not present, the driver defaults load to
>>> `QMP_VREG_UNUSED`.
> 
>> do you think having this in regulator core would make sense?
> 
> I'm not clear why the driver is trying to do this at all, the driver is
> AFAICT making no other effort to manage the load at all.  We already
> impose any constraints that are defined for a regulator while initially
> parsing them so it's not clear to me what this is supposed to
> accomplish, and it'll be broken if the supply is ever shared since it'll
> set the load from this individual consumer to the maximum that's
> permitted for the regulator as a whole.

Qualcomm regulators feature a low- and a high-power mode. As one may
imagine, low- is preferred, and high- needs to be engaged if we go
over a current threshold.

The specific regulator instances in question are often shared between
a couple PHYs (UFS, PCIe, USB..) and we need to convey to the
framework how much each consumer requires (and consumers can of course
go on/off at runtime). The current value varies between platforms, so
we want to read from DT.
The intended use is to set the load requirement and then only en/disable
the consumer, so that the current load is updated in core (like in the
kerneldoc of _regulator_handle_consumer_enable())

My question was about moving the custom parsing of
$supplyname-max-micromap introduced in this patch into the regulator
core, as this seems like a rather common problem.

Unless you meant to object to the "QMP_VREG_UNUSED" part specifically?

Konrad

On Thu, Aug 07, 2025 at 03:06:01PM +0200, Konrad Dybcio wrote:
> On 8/6/25 6:51 PM, Mark Brown wrote:

> > I'm not clear why the driver is trying to do this at all, the driver is
> > AFAICT making no other effort to manage the load at all.  We already
> > impose any constraints that are defined for a regulator while initially
> > parsing them so it's not clear to me what this is supposed to
> > accomplish, and it'll be broken if the supply is ever shared since it'll
> > set the load from this individual consumer to the maximum that's
> > permitted for the regulator as a whole.

> Qualcomm regulators feature a low- and a high-power mode. As one may
> imagine, low- is preferred, and high- needs to be engaged if we go
> over a current threshold.

Sure, but the driver is like I say doing nothing to actively manage the
current reporting.  It's just pulling a random number not specific to
the device (the max-microamp configuration is part of the constraints
which apply to the regualtor as a whole) out of the DT and throwing it
at the framework.

> The specific regulator instances in question are often shared between
> a couple PHYs (UFS, PCIe, USB..) and we need to convey to the
> framework how much each consumer requires (and consumers can of course
> go on/off at runtime). The current value varies between platforms, so
> we want to read from DT.

In that case this will definitely encounter the bug I mentioned above
where it's trying to read the maximum load permitted for the regulator
as a whole and report it as the load from this one specific device.

> The intended use is to set the load requirement and then only en/disable
> the consumer, so that the current load is updated in core (like in the
> kerneldoc of _regulator_handle_consumer_enable())

> My question was about moving the custom parsing of
> $supplyname-max-micromap introduced in this patch into the regulator
> core, as this seems like a rather common problem.

Wait, is this supposed to be some new property that you want to
standardise?  I didn't see a proposal for that, it's not something that
currently exists - the only standard properties that currently exist are
for the regulator as a whole.  

I'm not super convinced this is a particularly common issue, most
devices perform pretty much the same regardless of the board design so
the driver should just know their peak consumption and it's becoming
less and less common for regulators to need software help to adapt to
loads.  The main use case these days seems to be for safety (eg,
constraining how much can be drawn via a USB port) which seems like it'd
either be for the full supply or just known by the driver.

On 8/7/2025 7:14 PM, Mark Brown wrote:
> On Thu, Aug 07, 2025 at 03:06:01PM +0200, Konrad Dybcio wrote:
>> On 8/6/25 6:51 PM, Mark Brown wrote:
> 
>>> I'm not clear why the driver is trying to do this at all, the driver is
>>> AFAICT making no other effort to manage the load at all.  We already
>>> impose any constraints that are defined for a regulator while initially
>>> parsing them so it's not clear to me what this is supposed to
>>> accomplish, and it'll be broken if the supply is ever shared since it'll
>>> set the load from this individual consumer to the maximum that's
>>> permitted for the regulator as a whole.
> 
>> Qualcomm regulators feature a low- and a high-power mode. As one may
>> imagine, low- is preferred, and high- needs to be engaged if we go
>> over a current threshold.
> 
> Sure, but the driver is like I say doing nothing to actively manage the
> current reporting.  It's just pulling a random number not specific to
> the device (the max-microamp configuration is part of the constraints
> which apply to the regualtor as a whole) out of the DT and throwing it
> at the framework.
> 
>> The specific regulator instances in question are often shared between
>> a couple PHYs (UFS, PCIe, USB..) and we need to convey to the
>> framework how much each consumer requires (and consumers can of course
>> go on/off at runtime). The current value varies between platforms, so
>> we want to read from DT.
> 
> In that case this will definitely encounter the bug I mentioned above
> where it's trying to read the maximum load permitted for the regulator
> as a whole and report it as the load from this one specific device.
> 
>> The intended use is to set the load requirement and then only en/disable
>> the consumer, so that the current load is updated in core (like in the
>> kerneldoc of _regulator_handle_consumer_enable())
> 
>> My question was about moving the custom parsing of
>> $supplyname-max-micromap introduced in this patch into the regulator
>> core, as this seems like a rather common problem.
> 
> Wait, is this supposed to be some new property that you want to
> standardise?  I didn't see a proposal for that, it's not something that
> currently exists - the only standard properties that currently exist are
> for the regulator as a whole.


Hi Mark,

The UFS QMP PHY driver is not the first client to use regulator_set_load 
or alternatively set load requirements and invoke enable/disable or 
alternatively

Similar to other PHY drivers (such as USB, Display, and Combo PHYs),
as well as various subsystem drivers like UFS, SDHCI, Bluetooth, and 
others, the QMP UFS PHY driver is communicating/setting its load 
requirements.

These drivers also define corresponding binding properties, as seen in 
the UFS instances documented here:

UFS Common DT Binding ((link - 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807) 


Relevant properties include:

vcc-max-microamp: Specifies the maximum load that can be drawn from the 
VCC supply
vccq-max-microamp: Specifies the maximum load that can be drawn from the 
VCCQ supply
vccq2-max-microamp: Specifies the maximum load that can be drawn from 
the VCCQ2 supply

There was a previous effort to introduce similar properties 
(vdda-phy-max-microamp and vdda-pll-max-microamp) in the device tree 
bindings.
Link - (link- 
https://patchwork.kernel.org/project/linux-arm-msm/patch/20220418205509.1102109-3-bhupesh.sharma@linaro.org/#24820481)

However, at that time, the driver-side implementation for aggregating 
load requests was not in place, which led to the patch not being merged:
Patch Reference

Currently, the regulator framework does support automatic aggregation of 
load requests from multiple client drivers. Therefore, it is reasonable 
and necessary for each client to individually communicate its expected 
runtime load to the regulator framework to put the regulators in current
operation mode.

Regards,
Nitin

> 
> I'm not super convinced this is a particularly common issue, most
> devices perform pretty much the same regardless of the board design so
> the driver should just know their peak consumption and it's becoming
> less and less common for regulators to need software help to adapt to
> loads.  The main use case these days seems to be for safety (eg,
> constraining how much can be drawn via a USB port) which seems like it'd
> either be for the full supply or just known by the driver.

On Thu, Aug 07, 2025 at 09:12:53PM +0530, Nitin Rawat wrote:
> On 8/7/2025 7:14 PM, Mark Brown wrote:

> > > The intended use is to set the load requirement and then only en/disable
> > > the consumer, so that the current load is updated in core (like in the
> > > kerneldoc of _regulator_handle_consumer_enable())

> > > My question was about moving the custom parsing of
> > > $supplyname-max-micromap introduced in this patch into the regulator
> > > core, as this seems like a rather common problem.

> > Wait, is this supposed to be some new property that you want to
> > standardise?  I didn't see a proposal for that, it's not something that
> > currently exists - the only standard properties that currently exist are
> > for the regulator as a whole.

> The UFS QMP PHY driver is not the first client to use regulator_set_load or
> alternatively set load requirements and invoke enable/disable or
> alternatively

The issue isn't using regulator_set_load(), that's perfectly fine and
expected.  The issue is either reading the value to use from the
constraint information (which is just buggy) or adding a generic
property for this (which I'm not convinced is a good idea, I'd expect a
large propoprtion of drivers should just know what their requirements
are and that a generic property would just get abused).

> These drivers also define corresponding binding properties, as seen in the
> UFS instances documented here:

> UFS Common DT Binding ((link - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807)

Note that that's specifying OPPs which is different...

> There was a previous effort to introduce similar properties
> (vdda-phy-max-microamp and vdda-pll-max-microamp) in the device tree
> bindings.
> Link - (link- https://patchwork.kernel.org/project/linux-arm-msm/patch/20220418205509.1102109-3-bhupesh.sharma@linaro.org/#24820481)

That patch also fails to supply any rationale for making this board
specific or generally putting them in the DT, AFAICT it's one of these
things just pulled out of the vendor tree without really thinking about
it.  The changelog just says the properties are in downstream DTs.

> Currently, the regulator framework does support automatic aggregation of
> load requests from multiple client drivers. Therefore, it is reasonable and
> necessary for each client to individually communicate its expected runtime
> load to the regulator framework to put the regulators in current
> operation mode.

That doesn't mean that it's a good idea to put that information in the
DT, nor if it is sensible to put in DT does it mean that it's a good
idea to define a generic property that applies to all regulator
consumers which is what I now think Konrad is proposing.

On 8/7/25 7:26 PM, Mark Brown wrote:
> On Thu, Aug 07, 2025 at 09:12:53PM +0530, Nitin Rawat wrote:
>> On 8/7/2025 7:14 PM, Mark Brown wrote:
> 
>>>> The intended use is to set the load requirement and then only en/disable
>>>> the consumer, so that the current load is updated in core (like in the
>>>> kerneldoc of _regulator_handle_consumer_enable())
> 
>>>> My question was about moving the custom parsing of
>>>> $supplyname-max-micromap introduced in this patch into the regulator
>>>> core, as this seems like a rather common problem.
> 
>>> Wait, is this supposed to be some new property that you want to
>>> standardise?  I didn't see a proposal for that, it's not something that
>>> currently exists - the only standard properties that currently exist are
>>> for the regulator as a whole.
> 
>> The UFS QMP PHY driver is not the first client to use regulator_set_load or
>> alternatively set load requirements and invoke enable/disable or
>> alternatively
> 
> The issue isn't using regulator_set_load(), that's perfectly fine and
> expected.  The issue is either reading the value to use from the
> constraint information (which is just buggy) or adding a generic
> property for this (which I'm not convinced is a good idea, I'd expect a
> large propoprtion of drivers should just know what their requirements
> are and that a generic property would just get abused).
> 
>> These drivers also define corresponding binding properties, as seen in the
>> UFS instances documented here:
> 
>> UFS Common DT Binding ((link - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807)
> 
> Note that that's specifying OPPs which is different...

The microamp properties are in the top-level, not under OPP if
that's what you meant

Or are you perhaps suggesting that any device requiring explicit
current requirement settings, should do so through an OPP table
(perhaps a degenerated one with just a single entry detailling
the single requirement most of the time)?

>> There was a previous effort to introduce similar properties
>> (vdda-phy-max-microamp and vdda-pll-max-microamp) in the device tree
>> bindings.
>> Link - (link- https://patchwork.kernel.org/project/linux-arm-msm/patch/20220418205509.1102109-3-bhupesh.sharma@linaro.org/#24820481)
> 
> That patch also fails to supply any rationale for making this board
> specific or generally putting them in the DT, AFAICT it's one of these
> things just pulled out of the vendor tree without really thinking about
> it.  The changelog just says the properties are in downstream DTs.
> 
>> Currently, the regulator framework does support automatic aggregation of
>> load requests from multiple client drivers. Therefore, it is reasonable and
>> necessary for each client to individually communicate its expected runtime
>> load to the regulator framework to put the regulators in current
>> operation mode.
> 
> That doesn't mean that it's a good idea to put that information in the
> DT, nor if it is sensible to put in DT does it mean that it's a good
> idea to define a generic property that applies to all regulator
> consumers which is what I now think Konrad is proposing.

Yeah, that's what I had in mind

I was never able to get a reliable source for those numbers myselfe
either.. At least some of them are prooooobably? chosen based on the
used regulator type, to ensure it's always in HPM..

That said, our drivers cover a wide variety of hardware, built on a
wide variety of process nodes, with different configurations, etc.,
so it's either polluting the DT, or polluting the driver with
per-compatible hardcoded data (and additional compatibles because
fallbacks wouldn't work most of the time)

Konrad

On Thu, Aug 07, 2025 at 07:43:15PM +0200, Konrad Dybcio wrote:
> On 8/7/25 7:26 PM, Mark Brown wrote:

> > Note that that's specifying OPPs which is different...

> The microamp properties are in the top-level, not under OPP if
> that's what you meant

I mean the OPPs use case is an existing well known one for dumping stuff
into DT.

> > That doesn't mean that it's a good idea to put that information in the
> > DT, nor if it is sensible to put in DT does it mean that it's a good
> > idea to define a generic property that applies to all regulator
> > consumers which is what I now think Konrad is proposing.

> Yeah, that's what I had in mind

> I was never able to get a reliable source for those numbers myselfe
> either.. At least some of them are prooooobably? chosen based on the
> used regulator type, to ensure it's always in HPM..

That's what set_mode() is for.  Like I say it's becoming less and less
relevant though.

> That said, our drivers cover a wide variety of hardware, built on a
> wide variety of process nodes, with different configurations, etc.,
> so it's either polluting the DT, or polluting the driver with
> per-compatible hardcoded data (and additional compatibles because
> fallbacks wouldn't work most of the time)

That's really not a persuasive argument for adding a genric property
that applies to all regulator consumers...

My instinct with this stuff is generally to avoid putting it in the DT,
we see far too many instances where someone's typed some numbers in
wrongly or discovers the ability to drive the hardware harder and needs
to tune the numbers - once something is ABI you're stuck just trusting
the numbers.  That said I'm not going to stop you putting something
specific to this driver in there, I just don't think this is a good idea
as a generic property.

On Thu, Aug 07, 2025 at 08:09:56PM +0100, Mark Brown wrote:
> On Thu, Aug 07, 2025 at 07:43:15PM +0200, Konrad Dybcio wrote:
> > On 8/7/25 7:26 PM, Mark Brown wrote:
> 
> > > Note that that's specifying OPPs which is different...
> 
> > The microamp properties are in the top-level, not under OPP if
> > that's what you meant
> 
> I mean the OPPs use case is an existing well known one for dumping stuff
> into DT.
> 
> > > That doesn't mean that it's a good idea to put that information in the
> > > DT, nor if it is sensible to put in DT does it mean that it's a good
> > > idea to define a generic property that applies to all regulator
> > > consumers which is what I now think Konrad is proposing.
> 
> > Yeah, that's what I had in mind
> 
> > I was never able to get a reliable source for those numbers myselfe
> > either.. At least some of them are prooooobably? chosen based on the
> > used regulator type, to ensure it's always in HPM..
> 
> That's what set_mode() is for.  Like I say it's becoming less and less
> relevant though.
> 

set_mode() just applies the mode to the regulator_dev, so in cases where
you have multiple consumers of a regulator_dev things would break.

Further, there are numerous cases where we have multiple consumers each
needing a "low" mode, but their combined load requires a "high" mode.

set_load() and its aggregation of the inputs deals with both of these
issues.


Whether mode setting is becoming less relevant in our hardware, that I
don't have the definitive answer to.

> > That said, our drivers cover a wide variety of hardware, built on a
> > wide variety of process nodes, with different configurations, etc.,
> > so it's either polluting the DT, or polluting the driver with
> > per-compatible hardcoded data (and additional compatibles because
> > fallbacks wouldn't work most of the time)

If this is our reason for putting it in DeviceTree, then we should write
that in the commit message :)

> 
> That's really not a persuasive argument for adding a genric property
> that applies to all regulator consumers...
> 

I agree, even if we determine that this belongs in DT, because it needs
to be tweaked on a per-board basis, it's still only applicable to a
fraction of our device nodes.

Regards,
Bjorn

> My instinct with this stuff is generally to avoid putting it in the DT,
> we see far too many instances where someone's typed some numbers in
> wrongly or discovers the ability to drive the hardware harder and needs
> to tune the numbers - once something is ABI you're stuck just trusting
> the numbers.  That said I'm not going to stop you putting something
> specific to this driver in there, I just don't think this is a good idea
> as a generic property.

On 8/11/2025 9:20 PM, Bjorn Andersson wrote:
> On Thu, Aug 07, 2025 at 08:09:56PM +0100, Mark Brown wrote:
>> On Thu, Aug 07, 2025 at 07:43:15PM +0200, Konrad Dybcio wrote:
>>> On 8/7/25 7:26 PM, Mark Brown wrote:
>>
>>>> Note that that's specifying OPPs which is different...
>>
>>> The microamp properties are in the top-level, not under OPP if
>>> that's what you meant
>>
>> I mean the OPPs use case is an existing well known one for dumping stuff
>> into DT.
>>
>>>> That doesn't mean that it's a good idea to put that information in the
>>>> DT, nor if it is sensible to put in DT does it mean that it's a good
>>>> idea to define a generic property that applies to all regulator
>>>> consumers which is what I now think Konrad is proposing.
>>
>>> Yeah, that's what I had in mind
>>
>>> I was never able to get a reliable source for those numbers myselfe
>>> either.. At least some of them are prooooobably? chosen based on the
>>> used regulator type, to ensure it's always in HPM..
>>
>> That's what set_mode() is for.  Like I say it's becoming less and less
>> relevant though.
>>
> 
> set_mode() just applies the mode to the regulator_dev, so in cases where
> you have multiple consumers of a regulator_dev things would break.
> 
> Further, there are numerous cases where we have multiple consumers each
> needing a "low" mode, but their combined load requires a "high" mode.
> 
> set_load() and its aggregation of the inputs deals with both of these
> issues.
> 
> 
> Whether mode setting is becoming less relevant in our hardware, that I
> don't have the definitive answer to.
> 
>>> That said, our drivers cover a wide variety of hardware, built on a
>>> wide variety of process nodes, with different configurations, etc.,
>>> so it's either polluting the DT, or polluting the driver with
>>> per-compatible hardcoded data (and additional compatibles because
>>> fallbacks wouldn't work most of the time)
> 
> If this is our reason for putting it in DeviceTree, then we should write
> that in the commit message :)
> 
>>
>> That's really not a persuasive argument for adding a genric property
>> that applies to all regulator consumers...
>>
> 
> I agree, even if we determine that this belongs in DT, because it needs
> to be tweaked on a per-board basis, it's still only applicable to a
> fraction of our device nodes.

Hi Bjorn & Mark,

I had a follow-up discussion with the PHY designer to confirm whether 
this value could vary at the board level. Based on their response, it's 
a fixed value for the SoC and remains consistent across different 
boards. Therefore, I'm comfortable removing it from the device tree and 
using hardcoded, per-compatible data in the driver.

The only concern is that this approach may lead to driver bloat over 
time, as more SoCs are added and each requires its own hardcoded 
configuration.

Regards,
Nitin






> 
> Regards,
> Bjorn
> 
>> My instinct with this stuff is generally to avoid putting it in the DT,
>> we see far too many instances where someone's typed some numbers in
>> wrongly or discovers the ability to drive the hardware harder and needs
>> to tune the numbers - once something is ABI you're stuck just trusting
>> the numbers.  That said I'm not going to stop you putting something
>> specific to this driver in there, I just don't think this is a good idea
>> as a generic property.
>

On 8/12/2025 1:18 AM, Nitin Rawat wrote:
> 
> 
> On 8/11/2025 9:20 PM, Bjorn Andersson wrote:
>> On Thu, Aug 07, 2025 at 08:09:56PM +0100, Mark Brown wrote:
>>> On Thu, Aug 07, 2025 at 07:43:15PM +0200, Konrad Dybcio wrote:
>>>> On 8/7/25 7:26 PM, Mark Brown wrote:
>>>
>>>>> Note that that's specifying OPPs which is different...
>>>
>>>> The microamp properties are in the top-level, not under OPP if
>>>> that's what you meant
>>>
>>> I mean the OPPs use case is an existing well known one for dumping stuff
>>> into DT.
>>>
>>>>> That doesn't mean that it's a good idea to put that information in the
>>>>> DT, nor if it is sensible to put in DT does it mean that it's a good
>>>>> idea to define a generic property that applies to all regulator
>>>>> consumers which is what I now think Konrad is proposing.
>>>
>>>> Yeah, that's what I had in mind
>>>
>>>> I was never able to get a reliable source for those numbers myselfe
>>>> either.. At least some of them are prooooobably? chosen based on the
>>>> used regulator type, to ensure it's always in HPM..
>>>
>>> That's what set_mode() is for.  Like I say it's becoming less and less
>>> relevant though.
>>>
>>
>> set_mode() just applies the mode to the regulator_dev, so in cases where
>> you have multiple consumers of a regulator_dev things would break.
>>
>> Further, there are numerous cases where we have multiple consumers each
>> needing a "low" mode, but their combined load requires a "high" mode.
>>
>> set_load() and its aggregation of the inputs deals with both of these
>> issues.
>>
>>
>> Whether mode setting is becoming less relevant in our hardware, that I
>> don't have the definitive answer to.
>>
>>>> That said, our drivers cover a wide variety of hardware, built on a
>>>> wide variety of process nodes, with different configurations, etc.,
>>>> so it's either polluting the DT, or polluting the driver with
>>>> per-compatible hardcoded data (and additional compatibles because
>>>> fallbacks wouldn't work most of the time)
>>
>> If this is our reason for putting it in DeviceTree, then we should write
>> that in the commit message :)
>>
>>>
>>> That's really not a persuasive argument for adding a genric property
>>> that applies to all regulator consumers...
>>>
>>
>> I agree, even if we determine that this belongs in DT, because it needs
>> to be tweaked on a per-board basis, it's still only applicable to a
>> fraction of our device nodes.
> 
> Hi Bjorn & Mark,
> 
> I had a follow-up discussion with the PHY designer to confirm whether 
> this value could vary at the board level. Based on their response, it's 
> a fixed value for the SoC and remains consistent across different 
> boards. Therefore, I'm comfortable removing it from the device tree and 
> using hardcoded, per-compatible data in the driver.
> 
> The only concern is that this approach may lead to driver bloat over 
> time, as more SoCs are added and each requires its own hardcoded 
> configuration.
> 
> Regards,
> Nitin
> 
> 
Based on the current understanding, I plan to post a revised patch that 
uses hardcoded, per-compatible data within the driver, rather than 
specifying it in the device tree. Please let me know if anyone has any 
concerns with this approach.

Thanks,
Nitin


> 
> 
> 
> 
>>
>> Regards,
>> Bjorn
>>
>>> My instinct with this stuff is generally to avoid putting it in the DT,
>>> we see far too many instances where someone's typed some numbers in
>>> wrongly or discovers the ability to drive the hardware harder and needs
>>> to tune the numbers - once something is ABI you're stuck just trusting
>>> the numbers.  That said I'm not going to stop you putting something
>>> specific to this driver in there, I just don't think this is a good idea
>>> as a generic property.
>>
> 

On Mon, Aug 11, 2025 at 10:50:27AM -0500, Bjorn Andersson wrote:
> On Thu, Aug 07, 2025 at 08:09:56PM +0100, Mark Brown wrote:
> > On Thu, Aug 07, 2025 at 07:43:15PM +0200, Konrad Dybcio wrote:

> > > I was never able to get a reliable source for those numbers myselfe
> > > either.. At least some of them are prooooobably? chosen based on the
> > > used regulator type, to ensure it's always in HPM..

> > That's what set_mode() is for.  Like I say it's becoming less and less
> > relevant though.

> set_mode() just applies the mode to the regulator_dev, so in cases where
> you have multiple consumers of a regulator_dev things would break.

> Further, there are numerous cases where we have multiple consumers each
> needing a "low" mode, but their combined load requires a "high" mode.

> set_load() and its aggregation of the inputs deals with both of these
> issues.

That sort of active mode management is not the suggestion above that all
this stuff is just intended to force the regulator to always be in high
power mode.  If that's the goal then like I say just use set_mode() and
directly express it.

> Whether mode setting is becoming less relevant in our hardware, that I
> don't have the definitive answer to.

I rather get the impression that nobody understands what any of this
stuff is actually trying to accomplish in these systems and is just
copying things around from older code or BSPs, I'm not entirely
convinced it's actually doing anything useful in modern systems.

On 8/7/2025 11:13 PM, Konrad Dybcio wrote:
> On 8/7/25 7:26 PM, Mark Brown wrote:
>> On Thu, Aug 07, 2025 at 09:12:53PM +0530, Nitin Rawat wrote:
>>> On 8/7/2025 7:14 PM, Mark Brown wrote:
>>
>>>>> The intended use is to set the load requirement and then only en/disable
>>>>> the consumer, so that the current load is updated in core (like in the
>>>>> kerneldoc of _regulator_handle_consumer_enable())
>>
>>>>> My question was about moving the custom parsing of
>>>>> $supplyname-max-micromap introduced in this patch into the regulator
>>>>> core, as this seems like a rather common problem.
>>
>>>> Wait, is this supposed to be some new property that you want to
>>>> standardise?  I didn't see a proposal for that, it's not something that
>>>> currently exists - the only standard properties that currently exist are
>>>> for the regulator as a whole.
>>
>>> The UFS QMP PHY driver is not the first client to use regulator_set_load or
>>> alternatively set load requirements and invoke enable/disable or
>>> alternatively
>>
>> The issue isn't using regulator_set_load(), that's perfectly fine and
>> expected.  The issue is either reading the value to use from the
>> constraint information (which is just buggy) or adding a generic
>> property for this (which I'm not convinced is a good idea, I'd expect a
>> large propoprtion of drivers should just know what their requirements
>> are and that a generic property would just get abused).
>>
>>> These drivers also define corresponding binding properties, as seen in the
>>> UFS instances documented here:
>>
>>> UFS Common DT Binding ((link - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807)
>>
>> Note that that's specifying OPPs which is different...
> 
> The microamp properties are in the top-level, not under OPP if
> that's what you meant

Thanks for pointing that out, Konrad


> 
> Or are you perhaps suggesting that any device requiring explicit
> current requirement settings, should do so through an OPP table
> (perhaps a degenerated one with just a single entry detailling
> the single requirement most of the time)?
> 
>>> There was a previous effort to introduce similar properties
>>> (vdda-phy-max-microamp and vdda-pll-max-microamp) in the device tree
>>> bindings.
>>> Link - (link- https://patchwork.kernel.org/project/linux-arm-msm/patch/20220418205509.1102109-3-bhupesh.sharma@linaro.org/#24820481)
>>
>> That patch also fails to supply any rationale for making this board
>> specific or generally putting them in the DT, AFAICT it's one of these
>> things just pulled out of the vendor tree without really thinking about
>> it.  The changelog just says the properties are in downstream DTs.
>>
>>> Currently, the regulator framework does support automatic aggregation of
>>> load requests from multiple client drivers. Therefore, it is reasonable and
>>> necessary for each client to individually communicate its expected runtime
>>> load to the regulator framework to put the regulators in current
>>> operation mode.
>>
>> That doesn't mean that it's a good idea to put that information in the
>> DT, nor if it is sensible to put in DT does it mean that it's a good
>> idea to define a generic property that applies to all regulator
>> consumers which is what I now think Konrad is proposing.
> 
> Yeah, that's what I had in mind
> 
> I was never able to get a reliable source for those numbers myselfe
> either.. At least some of them are prooooobably? chosen based on the
> used regulator type, to ensure it's always in HPM..
> 
> That said, our drivers cover a wide variety of hardware, built on a
> wide variety of process nodes, with different configurations, etc.,
> so it's either polluting the DT, or polluting the driver with
> per-compatible hardcoded data (and additional compatibles because
> fallbacks wouldn't work most of the time)

I missed to read this before replying to my last reply.
Thanks for the explaining this. I too had mention similar thing in reply 
to mark's query in my last reply.



> 
> Konrad

On 8/7/2025 10:56 PM, Mark Brown wrote:
> On Thu, Aug 07, 2025 at 09:12:53PM +0530, Nitin Rawat wrote:
>> On 8/7/2025 7:14 PM, Mark Brown wrote:
> 
>>>> The intended use is to set the load requirement and then only en/disable
>>>> the consumer, so that the current load is updated in core (like in the
>>>> kerneldoc of _regulator_handle_consumer_enable())
> 
>>>> My question was about moving the custom parsing of
>>>> $supplyname-max-micromap introduced in this patch into the regulator
>>>> core, as this seems like a rather common problem.
> 
>>> Wait, is this supposed to be some new property that you want to
>>> standardise?  I didn't see a proposal for that, it's not something that
>>> currently exists - the only standard properties that currently exist are
>>> for the regulator as a whole.
> 
>> The UFS QMP PHY driver is not the first client to use regulator_set_load or
>> alternatively set load requirements and invoke enable/disable or
>> alternatively
> 
> The issue isn't using regulator_set_load(), that's perfectly fine and
> expected.  The issue is either reading the value to use from the
> constraint information (which is just buggy) or adding a generic
> property for this (which I'm not convinced is a good idea, I'd expect a
> large propoprtion of drivers should just know what their requirements
> are and that a generic property would just get abused).
> 
>> These drivers also define corresponding binding properties, as seen in the
>> UFS instances documented here:
> 
>> UFS Common DT Binding ((link - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807)
> 
> Note that that's specifying OPPs which is different...

Sorry for the confusion .Instead, I meant the following three properties 
defined in the link to ufs-common.yaml binding, which specify the 
maximum load that can be drawn from the respective power supplies.

   vcc-max-microamp:
     description:
       Specifies max. load that can be drawn from VCC supply.

   vccq-max-microamp:
     description:
       Specifies max. load that can be drawn from VCCQ supply.

   vccq2-max-microamp:
     description:
       Specifies max. load that can be drawn from VCCQ2 supply.


> 
>> There was a previous effort to introduce similar properties
>> (vdda-phy-max-microamp and vdda-pll-max-microamp) in the device tree
>> bindings.
>> Link - (link- https://patchwork.kernel.org/project/linux-arm-msm/patch/20220418205509.1102109-3-bhupesh.sharma@linaro.org/#24820481)
> 
> That patch also fails to supply any rationale for making this board
> specific or generally putting them in the DT, AFAICT it's one of these
> things just pulled out of the vendor tree without really thinking about
> it.  The changelog just says the properties are in downstream DTs.
> 
>> Currently, the regulator framework does support automatic aggregation of
>> load requests from multiple client drivers. Therefore, it is reasonable and
>> necessary for each client to individually communicate its expected runtime
>> load to the regulator framework to put the regulators in current
>> operation mode.
> 
> That doesn't mean that it's a good idea to put that information in the
> DT, nor if it is sensible to put in DT does it mean that it's a good
> idea to define a generic property that applies to all regulator
> consumers which is what I now think Konrad is proposing.

On Thu, Aug 07, 2025 at 11:05:08PM +0530, Nitin Rawat wrote:
> On 8/7/2025 10:56 PM, Mark Brown wrote:

> > The issue isn't using regulator_set_load(), that's perfectly fine and
> > expected.  The issue is either reading the value to use from the
> > constraint information (which is just buggy) or adding a generic
> > property for this (which I'm not convinced is a good idea, I'd expect a
> > large propoprtion of drivers should just know what their requirements
> > are and that a generic property would just get abused).

> > > These drivers also define corresponding binding properties, as seen in the
> > > UFS instances documented here:

> > > UFS Common DT Binding ((link - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807)

> > Note that that's specifying OPPs which is different...

> Sorry for the confusion .Instead, I meant the following three properties
> defined in the link to ufs-common.yaml binding, which specify the maximum
> load that can be drawn from the respective power supplies.

>   vcc-max-microamp:
>     description:
>       Specifies max. load that can be drawn from VCC supply.
> 
>   vccq-max-microamp:
>     description:
>       Specifies max. load that can be drawn from VCCQ supply.
> 
>   vccq2-max-microamp:
>     description:
>       Specifies max. load that can be drawn from VCCQ2 supply.

OK, but that's still not motivating putting things in DT (the properties
are there but don't explain why) and having this for some devices
doesn't really address the why make it a generic rather than device
specific part of things like Konrad was suggesting.  Perhaps there's
enough board specific variation that it does make sense to put something
specific to this consumer in DT, but it'd be another step to make it a
generic thing that applies to all regulators.

On 8/7/2025 11:13 PM, Mark Brown wrote:
> On Thu, Aug 07, 2025 at 11:05:08PM +0530, Nitin Rawat wrote:
>> On 8/7/2025 10:56 PM, Mark Brown wrote:
> 
>>> The issue isn't using regulator_set_load(), that's perfectly fine and
>>> expected.  The issue is either reading the value to use from the
>>> constraint information (which is just buggy) or adding a generic
>>> property for this (which I'm not convinced is a good idea, I'd expect a
>>> large propoprtion of drivers should just know what their requirements
>>> are and that a generic property would just get abused).
> 
>>>> These drivers also define corresponding binding properties, as seen in the
>>>> UFS instances documented here:
> 
>>>> UFS Common DT Binding ((link - https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/ufs/ufs-common.yaml?h=next-20250807)
> 
>>> Note that that's specifying OPPs which is different...
> 
>> Sorry for the confusion .Instead, I meant the following three properties
>> defined in the link to ufs-common.yaml binding, which specify the maximum
>> load that can be drawn from the respective power supplies.
> 
>>    vcc-max-microamp:
>>      description:
>>        Specifies max. load that can be drawn from VCC supply.
>>
>>    vccq-max-microamp:
>>      description:
>>        Specifies max. load that can be drawn from VCCQ supply.
>>
>>    vccq2-max-microamp:
>>      description:
>>        Specifies max. load that can be drawn from VCCQ2 supply.
> 
> OK, but that's still not motivating putting things in DT (the properties
> are there but don't explain why) and having this for some devices
> doesn't really address the why make it a generic rather than device
> specific part of things like Konrad was suggesting.  Perhaps there's
> enough board specific variation that it does make sense to put something
> specific to this consumer in DT, but it'd be another step to make it a
> generic thing that applies to all regulators.

Hi Mark,

Thanks for your review and feedback.
To address your question about why these properties should be included 
in the device tree:

1. Regulator and PMIC configurations are board-specific, meaning they 
can vary significantly across different platforms. For example, some 
boards may use different generations of UFS devices — such as UFS 2.x — 
which come with distinct power and load requirements and some with 
UFS3.x which has it own power/load requirement.

2. UFS PHY load and PMIC requirements also varies across targets, 
depending on the underlying technology node and the specific PHY 
capabilities. These differences can be influenced by the MIPI version or 
other implementation details.

Given this variability, expressing these requirements in the device tree 
allows for a flexible and accurate way to describe board-specific 
constraints without hardcoding them into the driver.

Thanks,
Nitin

On Thu, Aug 07, 2025 at 11:26:17PM +0530, Nitin Rawat wrote:

> 1. Regulator and PMIC configurations are board-specific, meaning they can
> vary significantly across different platforms. For example, some boards may
> use different generations of UFS devices — such as UFS 2.x — which come with
> distinct power and load requirements and some with UFS3.x which has it own
> power/load requirement.

Requirements from generations of UFS devices presumably come from the
UFS spec and should just be known though?

> 2. UFS PHY load and PMIC requirements also varies across targets, depending
> on the underlying technology node and the specific PHY capabilities. These
> differences can be influenced by the MIPI version or other implementation
> details.

If you've got non-enumerable PHYs that have a big impact that's a much
clearer use case for putting things in DT.

> Given this variability, expressing these requirements in the device tree
> allows for a flexible and accurate way to describe board-specific
> constraints without hardcoding them into the driver.

There's still the issue with making this a thing for all regulators, not
just for this specific device.

On 8/8/2025 12:15 AM, Mark Brown wrote:
> On Thu, Aug 07, 2025 at 11:26:17PM +0530, Nitin Rawat wrote:
> 
>> 1. Regulator and PMIC configurations are board-specific, meaning they can
>> vary significantly across different platforms. For example, some boards may
>> use different generations of UFS devices — such as UFS 2.x — which come with
>> distinct power and load requirements and some with UFS3.x which has it own
>> power/load requirement.
> 
> Requirements from generations of UFS devices presumably come from the
> UFS spec and should just be known though?
> 
>> 2. UFS PHY load and PMIC requirements also varies across targets, depending
>> on the underlying technology node and the specific PHY capabilities. These
>> differences can be influenced by the MIPI version or other implementation
>> details.
> 
> If you've got non-enumerable PHYs that have a big impact that's a much
> clearer use case for putting things in DT.

What I meant is that different boards may use different UFS parts, and 
the associated PHY load requirements are not governed by the UFS 
specification itself. Instead, these requirements depend on our specific 
PHY design and MIPI, which can vary across platforms.

Because these characteristics — such as load requirements — are not 
enumerable or automatically detectable, it makes sense to describe them 
explicitly in the device tree. This approach ensures that board-specific 
variations are accurately captured without hardcoding them into the driver.

> 
>> Given this variability, expressing these requirements in the device tree
>> allows for a flexible and accurate way to describe board-specific
>> constraints without hardcoding them into the driver.
> 
> There's still the issue with making this a thing for all regulators, not
> just for this specific device.


I see your point. Just to clarify — my patch is specifically scoped to 
UFS PHY and PLL regulators. The device tree binding defines these 
properties as optional, and they are only enabled for targets where load 
voting is required.

So, this isn’t intended to be a generic change for all regulators, but 
rather a targeted solution for specific consumers that need it.

That’s why I’ve chosen to keep the regulator parsing logic and 
corresponding load voting support within the UFS PHY driver, rather than 
extending it to the core regulator framework.


Thanks,
Nitin

On Fri, Aug 08, 2025 at 02:15:31AM +0530, Nitin Rawat wrote:
> 
> 
> On 8/8/2025 12:15 AM, Mark Brown wrote:
> > On Thu, Aug 07, 2025 at 11:26:17PM +0530, Nitin Rawat wrote:
> > 
> > > 1. Regulator and PMIC configurations are board-specific, meaning they can
> > > vary significantly across different platforms. For example, some boards may
> > > use different generations of UFS devices — such as UFS 2.x — which come with
> > > distinct power and load requirements and some with UFS3.x which has it own
> > > power/load requirement.
> > 
> > Requirements from generations of UFS devices presumably come from the
> > UFS spec and should just be known though?
> > 
> > > 2. UFS PHY load and PMIC requirements also varies across targets, depending
> > > on the underlying technology node and the specific PHY capabilities. These
> > > differences can be influenced by the MIPI version or other implementation
> > > details.
> > 
> > If you've got non-enumerable PHYs that have a big impact that's a much
> > clearer use case for putting things in DT.
> 
> What I meant is that different boards may use different UFS parts, and the
> associated PHY load requirements are not governed by the UFS specification
> itself. Instead, these requirements depend on our specific PHY design and
> MIPI, which can vary across platforms.

The UFS controller knows which device it has attached - 2.x or 3.x - so
power considerations are known to the driver.

> 
> Because these characteristics — such as load requirements — are not
> enumerable or automatically detectable, it makes sense to describe them
> explicitly in the device tree. This approach ensures that board-specific
> variations are accurately captured without hardcoding them into the driver.

But you never came with rationale why given board has that value.

All this is done ONLY because downstream has it.

Best regards,
Krzysztof