[PATCH V1 0/4] Add support to configure regulator load for UFS QMP PHY

[PATCH V1 0/4] Add support to configure regulator load for UFS QMP PHY

[Nitin Rawat]

Add support in the Qualcomm QMP UFS PHY driver to configure regulator
load using max-microamp values from the device tree.

Patch introduces two optional properties in the SC8280XP QMP UFS
PHY binding.

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.

This change is tested on m8550-qrd, m8750-mtp and sm8650-mtp.

Nitin Rawat (4):
  dt-bindings: phy: Add max-microamp properties for PHY and PLL supplies
  arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL
    supplies
  arm64: dts: qcom: sm8650: add max-microamp for UFS PHY and PLL
    supplies
  phy: qcom-qmp-ufs: read max-microamp values from device tree

 .../phy/qcom,sc8280xp-qmp-ufs-phy.yaml        | 10 +++++++++
 arch/arm64/boot/dts/qcom/sm8650-hdk.dts       |  2 ++
 arch/arm64/boot/dts/qcom/sm8650-mtp.dts       |  3 ++-
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts       |  2 ++
 arch/arm64/boot/dts/qcom/sm8750-mtp.dts       |  2 ++
 arch/arm64/boot/dts/qcom/sm8750-qrd.dts       |  2 ++
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c       | 22 ++++++++++++++++---
 7 files changed, 39 insertions(+), 4 deletions(-)

--
2.48.1

[PATCH V1 1/4] dt-bindings: phy: Add max-microamp properties for PHY and PLL supplies

[Nitin Rawat]

Add two new optional properties to the SC8280XP QMP UFS PHY
device tree binding:

- `vdda-phy-max-microamp`: Specifies the maximum current (in microamps)
			   that can be drawn from the PHY supply.
- `vdda-pll-max-microamp`: Specifies the maximum current (in microamps)
			   that can be drawn from the PLL supply.

These additions help define power requirements more precisely for
regulators supplying the PHY and PLL blocks and ensuring the regulators
is kept in correct mode based on the client load requirements.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 .../bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml        | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
index a58370a6a5d3..4648642dc974 100644
--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
@@ -71,6 +71,16 @@ properties:
 
   vdda-pll-supply: true
 
+  vdda-phy-max-microamp:
+    maxItems: 1
+    description:
+      Specifies max. load that can be drawn from phy supply.
+
+  vdda-pll-max-microamp:
+    maxItems: 1
+    description:
+      Specifies max. load that can be drawn from pll supply.
+
   "#clock-cells":
     const: 1
 
-- 
2.48.1

Re: [PATCH V1 1/4] dt-bindings: phy: Add max-microamp properties for PHY and PLL supplies

[Krzysztof Kozlowski]

On Wed, Aug 06, 2025 at 09:13:37PM +0530, Nitin Rawat wrote:
> Add two new optional properties to the SC8280XP QMP UFS PHY
> device tree binding:
> 
> - `vdda-phy-max-microamp`: Specifies the maximum current (in microamps)
> 			   that can be drawn from the PHY supply.
> - `vdda-pll-max-microamp`: Specifies the maximum current (in microamps)
> 			   that can be drawn from the PLL supply.

How much you can draw from some supply is not a property of the device,
but that supply, so to me this is really oddly named.

Anyway, discussion is going in driver patch.

Best regards,
Krzysztof

Re: [PATCH V1 1/4] dt-bindings: phy: Add max-microamp properties for PHY and PLL supplies

[Bjorn Andersson]

On Wed, Aug 06, 2025 at 09:13:37PM +0530, Nitin Rawat wrote:
> Add two new optional properties to the SC8280XP QMP UFS PHY
> device tree binding:

Please start your commit message with a description of the problem
you're trying to solve.

> 
> - `vdda-phy-max-microamp`: Specifies the maximum current (in microamps)
> 			   that can be drawn from the PHY supply.
> - `vdda-pll-max-microamp`: Specifies the maximum current (in microamps)
> 			   that can be drawn from the PLL supply.

That's literally the content of the patch...use the commit message to
describe the problem you're solving and a technical description of the
solution to your problem (and in this case, why that solution goes in
the DeviceTree).

Regards,
Bjorn

> 
> These additions help define power requirements more precisely for
> regulators supplying the PHY and PLL blocks and ensuring the regulators
> is kept in correct mode based on the client load requirements.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  .../bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml        | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
> index a58370a6a5d3..4648642dc974 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml
> @@ -71,6 +71,16 @@ properties:
>  
>    vdda-pll-supply: true
>  
> +  vdda-phy-max-microamp:
> +    maxItems: 1
> +    description:
> +      Specifies max. load that can be drawn from phy supply.
> +
> +  vdda-pll-max-microamp:
> +    maxItems: 1
> +    description:
> +      Specifies max. load that can be drawn from pll supply.
> +
>    "#clock-cells":
>      const: 1
>  
> -- 
> 2.48.1
> 

[PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Nitin Rawat]

Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
the UFS PHY node in the device tree.

These properties define the maximum current (in microamps) expected
from the PHY and PLL regulators. This allows the PHY driver to
configure regulator load accurately and ensure proper regulator
mode based on load requirements.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8750-mtp.dts | 2 ++
 arch/arm64/boot/dts/qcom/sm8750-qrd.dts | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8750-mtp.dts b/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
index 75cfbb510be5..2ae5915fe38d 100644
--- a/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
@@ -1032,7 +1032,9 @@ wcd_default: wcd-reset-n-active-state {
 
 &ufs_mem_phy {
 	vdda-phy-supply = <&vreg_l1j_0p91>;
+	vdda-phy-max-microamp = <213000>;
 	vdda-pll-supply = <&vreg_l3g_1p2>;
+	vdda-pll-max-microamp = <18300>;
 
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sm8750-qrd.dts b/arch/arm64/boot/dts/qcom/sm8750-qrd.dts
index 13c7b9664c89..e9a41d34e2d6 100644
--- a/arch/arm64/boot/dts/qcom/sm8750-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8750-qrd.dts
@@ -1039,7 +1039,9 @@ &uart7 {
 
 &ufs_mem_phy {
 	vdda-phy-supply = <&vreg_l1j_0p91>;
+	vdda-phy-max-microamp = <213000>;
 	vdda-pll-supply = <&vreg_l3g_1p2>;
+	vdda-pll-max-microamp = <18300>;
 
 	status = "okay";
 };
-- 
2.48.1

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Krzysztof Kozlowski]

On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
> Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
> the UFS PHY node in the device tree.
> 
> These properties define the maximum current (in microamps) expected
> from the PHY and PLL regulators. This allows the PHY driver to
> configure regulator load accurately and ensure proper regulator
> mode based on load requirements.

That's not the property of phy, but regulator.

Also reasoning is here incomplete - you just post downstream code. :/

Best regards,
Krzysztof

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Konrad Dybcio]

On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
> On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
>> Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
>> the UFS PHY node in the device tree.
>>
>> These properties define the maximum current (in microamps) expected
>> from the PHY and PLL regulators. This allows the PHY driver to
>> configure regulator load accurately and ensure proper regulator
>> mode based on load requirements.
> 
> That's not the property of phy, but regulator.
> 
> Also reasoning is here incomplete - you just post downstream code. :/

The reason for this change is good, but perhaps not explained clearly

All of these values refer to the maximum current draw that needs to be
allocated on a shared voltage supply for this peripheral (because the
supply's capabilities change depending on the maximum potential load
at any given time, which the regulator driver must be aware of)

This is a property of a regulator *consumer*, i.e. if we had a chain
of LEDs hanging off of this supply, we'd need to specify NUM_LEDS * 
MAX_CURR under the "led chain" device, to make sure that if the
aggregated current requirements go over a certain threshold (which is
unknown to Linux and hidden in RPMh fw), the regulator can be
reconfigured to allow for a higher current draw (likely at some
downgrade to efficiency)

Konrad

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Krzysztof Kozlowski]

On 08/08/2025 10:58, Konrad Dybcio wrote:
> On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
>> On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
>>> Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
>>> the UFS PHY node in the device tree.
>>>
>>> These properties define the maximum current (in microamps) expected
>>> from the PHY and PLL regulators. This allows the PHY driver to
>>> configure regulator load accurately and ensure proper regulator
>>> mode based on load requirements.
>>
>> That's not the property of phy, but regulator.
>>
>> Also reasoning is here incomplete - you just post downstream code. :/
> 
> The reason for this change is good, but perhaps not explained clearly
> 
> All of these values refer to the maximum current draw that needs to be
> allocated on a shared voltage supply for this peripheral (because the


It sounds very different than how much it can be drawn. How much can be
drawn is the property of the regulator. The regulator knows how much
current it can support.


> supply's capabilities change depending on the maximum potential load
> at any given time, which the regulator driver must be aware of)
> 
> This is a property of a regulator *consumer*, i.e. if we had a chain
> of LEDs hanging off of this supply, we'd need to specify NUM_LEDS * 
> MAX_CURR under the "led chain" device, to make sure that if the
> aggregated current requirements go over a certain threshold (which is
> unknown to Linux and hidden in RPMh fw), the regulator can be
> reconfigured to allow for a higher current draw (likely at some
> downgrade to efficiency)


The problem is that rationale is downstream. Instead I want to see some
reason: e.g. datasheets, spec, type of UFS device (that was the argument
in the driver patch discussion).

The only argument here for given value is: downstream has it.

Best regards,
Krzysztof

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Nitin Rawat]

On 8/8/2025 3:09 PM, Krzysztof Kozlowski wrote:
> On 08/08/2025 10:58, Konrad Dybcio wrote:
>> On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
>>> On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
>>>> Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
>>>> the UFS PHY node in the device tree.
>>>>
>>>> These properties define the maximum current (in microamps) expected
>>>> from the PHY and PLL regulators. This allows the PHY driver to
>>>> configure regulator load accurately and ensure proper regulator
>>>> mode based on load requirements.
>>>
>>> That's not the property of phy, but regulator.
>>>
>>> Also reasoning is here incomplete - you just post downstream code. :/
>>
>> The reason for this change is good, but perhaps not explained clearly
>>
>> All of these values refer to the maximum current draw that needs to be
>> allocated on a shared voltage supply for this peripheral (because the
> 
> 
> It sounds very different than how much it can be drawn. How much can be
> drawn is the property of the regulator. The regulator knows how much
> current it can support.

Consumers are aware of their dynamic load requirements, which can vary 
at runtime—this awareness is not reciprocal. The power grid is designed 
based on the collective load requirements of all clients sharing the 
same power rail.

Since regulators can operate in multiple modes for power optimization, 
each consumer is expected to vote for its runtime power needs. These 
votes help the regulator framework maintain the regulator in the 
appropriate mode, ensuring stable and efficient operation across all 
clients.


Stability issues can arise if each consumer does not vote for its own 
load requirement.
For example, consider a scenario where a single regulator is shared by 
two consumers.

If the first client requests low-power mode by voting for zero or a 
minimal load to regulator framework during its driver's LPM sequence, 
and the second client (e.g., UFS PHY) has not voted for its own load 
requirement through the regulator framework, the regulator may 
transition to low-power mode. This can lead to issues for the second 
client, which expects a higher power state to operate correctly.


> 
> 
>> supply's capabilities change depending on the maximum potential load
>> at any given time, which the regulator driver must be aware of)
>>
>> This is a property of a regulator *consumer*, i.e. if we had a chain
>> of LEDs hanging off of this supply, we'd need to specify NUM_LEDS *
>> MAX_CURR under the "led chain" device, to make sure that if the
>> aggregated current requirements go over a certain threshold (which is
>> unknown to Linux and hidden in RPMh fw), the regulator can be
>> reconfigured to allow for a higher current draw (likely at some
>> downgrade to efficiency)
> 
> 
> The problem is that rationale is downstream. Instead I want to see some
> reason: e.g. datasheets, spec, type of UFS device (that was the argument
> in the driver patch discussion).


The PHY load requirements for consumers such as UFS, USB, PCIe are 
defined by Qualcomm’s PHY IP and are well-documented in Qualcomm’s 
datasheets and power grid documentation. These values can depending on 
the process or technology node, board design, and even the chip foundry 
used.

As a result, the load values can differ across SoCs or may be even 
board(unlikely though) due to variations in any of these parameters.


Regards,
Nitin



> 
> The only argument here for given value is: downstream has it.
> 
> Best regards,
> Krzysztof

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Manivannan Sadhasivam]

On Fri, Aug 08, 2025 at 08:49:45PM GMT, Nitin Rawat wrote:
> 
> 
> On 8/8/2025 3:09 PM, Krzysztof Kozlowski wrote:
> > On 08/08/2025 10:58, Konrad Dybcio wrote:
> > > On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
> > > > On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
> > > > > Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
> > > > > the UFS PHY node in the device tree.
> > > > > 
> > > > > These properties define the maximum current (in microamps) expected
> > > > > from the PHY and PLL regulators. This allows the PHY driver to
> > > > > configure regulator load accurately and ensure proper regulator
> > > > > mode based on load requirements.
> > > > 
> > > > That's not the property of phy, but regulator.
> > > > 
> > > > Also reasoning is here incomplete - you just post downstream code. :/
> > > 
> > > The reason for this change is good, but perhaps not explained clearly
> > > 
> > > All of these values refer to the maximum current draw that needs to be
> > > allocated on a shared voltage supply for this peripheral (because the
> > 
> > 
> > It sounds very different than how much it can be drawn. How much can be
> > drawn is the property of the regulator. The regulator knows how much
> > current it can support.
> 
> Consumers are aware of their dynamic load requirements, which can vary at
> runtime—this awareness is not reciprocal. The power grid is designed based
> on the collective load requirements of all clients sharing the same power
> rail.
> 
> Since regulators can operate in multiple modes for power optimization, each
> consumer is expected to vote for its runtime power needs. These votes help
> the regulator framework maintain the regulator in the appropriate mode,
> ensuring stable and efficient operation across all clients.
> 
> 
> Stability issues can arise if each consumer does not vote for its own load
> requirement.
> For example, consider a scenario where a single regulator is shared by two
> consumers.
> 
> If the first client requests low-power mode by voting for zero or a minimal
> load to regulator framework during its driver's LPM sequence, and the second
> client (e.g., UFS PHY) has not voted for its own load requirement through
> the regulator framework, the regulator may transition to low-power mode.
> This can lead to issues for the second client, which expects a higher power
> state to operate correctly.
> 

I think we all agree on consumers setting the load for shared regulators, but
the naming and description of the DT property is what causing confusion here.
There is no way the consumers can set the *max* current draw for a shared
regulator. They can only request load as per their requirement. But the max
current draw is a regulator constraint.

> 
> > 
> > 
> > > supply's capabilities change depending on the maximum potential load
> > > at any given time, which the regulator driver must be aware of)
> > > 
> > > This is a property of a regulator *consumer*, i.e. if we had a chain
> > > of LEDs hanging off of this supply, we'd need to specify NUM_LEDS *
> > > MAX_CURR under the "led chain" device, to make sure that if the
> > > aggregated current requirements go over a certain threshold (which is
> > > unknown to Linux and hidden in RPMh fw), the regulator can be
> > > reconfigured to allow for a higher current draw (likely at some
> > > downgrade to efficiency)
> > 
> > 
> > The problem is that rationale is downstream. Instead I want to see some
> > reason: e.g. datasheets, spec, type of UFS device (that was the argument
> > in the driver patch discussion).
> 
> 
> The PHY load requirements for consumers such as UFS, USB, PCIe are defined
> by Qualcomm’s PHY IP and are well-documented in Qualcomm’s datasheets and
> power grid documentation. These values can depending on the process or
> technology node, board design, and even the chip foundry used.
> 
> As a result, the load values can differ across SoCs or may be even
> board(unlikely though) due to variations in any of these parameters.
> 

Okay. This goes into the commit message and possibly some part of it to property
description also.

- Mani

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

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Nitin Rawat]

On 8/9/2025 4:37 PM, Manivannan Sadhasivam wrote:
> On Fri, Aug 08, 2025 at 08:49:45PM GMT, Nitin Rawat wrote:
>>
>>
>> On 8/8/2025 3:09 PM, Krzysztof Kozlowski wrote:
>>> On 08/08/2025 10:58, Konrad Dybcio wrote:
>>>> On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
>>>>> On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
>>>>>> Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
>>>>>> the UFS PHY node in the device tree.
>>>>>>
>>>>>> These properties define the maximum current (in microamps) expected
>>>>>> from the PHY and PLL regulators. This allows the PHY driver to
>>>>>> configure regulator load accurately and ensure proper regulator
>>>>>> mode based on load requirements.
>>>>>
>>>>> That's not the property of phy, but regulator.
>>>>>
>>>>> Also reasoning is here incomplete - you just post downstream code. :/
>>>>
>>>> The reason for this change is good, but perhaps not explained clearly
>>>>
>>>> All of these values refer to the maximum current draw that needs to be
>>>> allocated on a shared voltage supply for this peripheral (because the
>>>
>>>
>>> It sounds very different than how much it can be drawn. How much can be
>>> drawn is the property of the regulator. The regulator knows how much
>>> current it can support.
>>
>> Consumers are aware of their dynamic load requirements, which can vary at
>> runtime—this awareness is not reciprocal. The power grid is designed based
>> on the collective load requirements of all clients sharing the same power
>> rail.
>>
>> Since regulators can operate in multiple modes for power optimization, each
>> consumer is expected to vote for its runtime power needs. These votes help
>> the regulator framework maintain the regulator in the appropriate mode,
>> ensuring stable and efficient operation across all clients.
>>
>>
>> Stability issues can arise if each consumer does not vote for its own load
>> requirement.
>> For example, consider a scenario where a single regulator is shared by two
>> consumers.
>>
>> If the first client requests low-power mode by voting for zero or a minimal
>> load to regulator framework during its driver's LPM sequence, and the second
>> client (e.g., UFS PHY) has not voted for its own load requirement through
>> the regulator framework, the regulator may transition to low-power mode.
>> This can lead to issues for the second client, which expects a higher power
>> state to operate correctly.
>>
> 
> I think we all agree on consumers setting the load for shared regulators, but
> the naming and description of the DT property is what causing confusion here.
> There is no way the consumers can set the *max* current draw for a shared
> regulator. They can only request load as per their requirement. But the max
> current draw is a regulator constraint.

To avoid confusion with regulator-level constraints, I'm open to 
renaming the property vdda-phy-max-microamp to something more 
descriptive, such as vdda-phy-client-peak-load-microamp or 
vdda-phy-peak-load-microamp. Along with updating the description, this 
would better reflect the property's actual intent: to specify the 
maximum current a client may draw during peak operation, rather than 
implying it defines the regulator’s maximum capability.


Having said that, 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 does not change across 
different boards(atleast for now). Therefore, I can remove from device 
tree and replaced with hardcoded, per-compatible data in the driver.

> 
>>
>>>
>>>
>>>> supply's capabilities change depending on the maximum potential load
>>>> at any given time, which the regulator driver must be aware of)
>>>>
>>>> This is a property of a regulator *consumer*, i.e. if we had a chain
>>>> of LEDs hanging off of this supply, we'd need to specify NUM_LEDS *
>>>> MAX_CURR under the "led chain" device, to make sure that if the
>>>> aggregated current requirements go over a certain threshold (which is
>>>> unknown to Linux and hidden in RPMh fw), the regulator can be
>>>> reconfigured to allow for a higher current draw (likely at some
>>>> downgrade to efficiency)
>>>
>>>
>>> The problem is that rationale is downstream. Instead I want to see some
>>> reason: e.g. datasheets, spec, type of UFS device (that was the argument
>>> in the driver patch discussion).
>>
>>
>> The PHY load requirements for consumers such as UFS, USB, PCIe are defined
>> by Qualcomm’s PHY IP and are well-documented in Qualcomm’s datasheets and
>> power grid documentation. These values can depending on the process or
>> technology node, board design, and even the chip foundry used.
>>
>> As a result, the load values can differ across SoCs or may be even
>> board(unlikely though) due to variations in any of these parameters.
>>
> 
> Okay. This goes into the commit message and possibly some part of it to property
> description also.




> 
> - Mani
> 

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Dmitry Baryshkov]

On Tue, Aug 12, 2025 at 01:25:02AM +0530, Nitin Rawat wrote:
> 
> 
> On 8/9/2025 4:37 PM, Manivannan Sadhasivam wrote:
> > On Fri, Aug 08, 2025 at 08:49:45PM GMT, Nitin Rawat wrote:
> > > 
> > > 
> > > On 8/8/2025 3:09 PM, Krzysztof Kozlowski wrote:
> > > > On 08/08/2025 10:58, Konrad Dybcio wrote:
> > > > > On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
> > > > > > On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
> > > > > > > Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
> > > > > > > the UFS PHY node in the device tree.
> > > > > > > 
> > > > > > > These properties define the maximum current (in microamps) expected
> > > > > > > from the PHY and PLL regulators. This allows the PHY driver to
> > > > > > > configure regulator load accurately and ensure proper regulator
> > > > > > > mode based on load requirements.
> > > > > > 
> > > > > > That's not the property of phy, but regulator.
> > > > > > 
> > > > > > Also reasoning is here incomplete - you just post downstream code. :/
> > > > > 
> > > > > The reason for this change is good, but perhaps not explained clearly
> > > > > 
> > > > > All of these values refer to the maximum current draw that needs to be
> > > > > allocated on a shared voltage supply for this peripheral (because the
> > > > 
> > > > 
> > > > It sounds very different than how much it can be drawn. How much can be
> > > > drawn is the property of the regulator. The regulator knows how much
> > > > current it can support.
> > > 
> > > Consumers are aware of their dynamic load requirements, which can vary at
> > > runtime—this awareness is not reciprocal. The power grid is designed based
> > > on the collective load requirements of all clients sharing the same power
> > > rail.
> > > 
> > > Since regulators can operate in multiple modes for power optimization, each
> > > consumer is expected to vote for its runtime power needs. These votes help
> > > the regulator framework maintain the regulator in the appropriate mode,
> > > ensuring stable and efficient operation across all clients.
> > > 
> > > 
> > > Stability issues can arise if each consumer does not vote for its own load
> > > requirement.
> > > For example, consider a scenario where a single regulator is shared by two
> > > consumers.
> > > 
> > > If the first client requests low-power mode by voting for zero or a minimal
> > > load to regulator framework during its driver's LPM sequence, and the second
> > > client (e.g., UFS PHY) has not voted for its own load requirement through
> > > the regulator framework, the regulator may transition to low-power mode.
> > > This can lead to issues for the second client, which expects a higher power
> > > state to operate correctly.
> > > 
> > 
> > I think we all agree on consumers setting the load for shared regulators, but
> > the naming and description of the DT property is what causing confusion here.
> > There is no way the consumers can set the *max* current draw for a shared
> > regulator. They can only request load as per their requirement. But the max
> > current draw is a regulator constraint.
> 
> To avoid confusion with regulator-level constraints, I'm open to renaming
> the property vdda-phy-max-microamp to something more descriptive, such as
> vdda-phy-client-peak-load-microamp or vdda-phy-peak-load-microamp. Along
> with updating the description, this would better reflect the property's
> actual intent: to specify the maximum current a client may draw during peak
> operation, rather than implying it defines the regulator’s maximum
> capability.

Move them into the driver please.

> 
> 
> Having said that, 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 does not change across
> different boards(atleast for now). Therefore, I can remove from device tree
> and replaced with hardcoded, per-compatible data in the driver.
> 
> > 
> > > 
> > > > 
> > > > 
> > > > > supply's capabilities change depending on the maximum potential load
> > > > > at any given time, which the regulator driver must be aware of)
> > > > > 
> > > > > This is a property of a regulator *consumer*, i.e. if we had a chain
> > > > > of LEDs hanging off of this supply, we'd need to specify NUM_LEDS *
> > > > > MAX_CURR under the "led chain" device, to make sure that if the
> > > > > aggregated current requirements go over a certain threshold (which is
> > > > > unknown to Linux and hidden in RPMh fw), the regulator can be
> > > > > reconfigured to allow for a higher current draw (likely at some
> > > > > downgrade to efficiency)
> > > > 
> > > > 
> > > > The problem is that rationale is downstream. Instead I want to see some
> > > > reason: e.g. datasheets, spec, type of UFS device (that was the argument
> > > > in the driver patch discussion).
> > > 
> > > 
> > > The PHY load requirements for consumers such as UFS, USB, PCIe are defined
> > > by Qualcomm’s PHY IP and are well-documented in Qualcomm’s datasheets and
> > > power grid documentation. These values can depending on the process or
> > > technology node, board design, and even the chip foundry used.
> > > 
> > > As a result, the load values can differ across SoCs or may be even
> > > board(unlikely though) due to variations in any of these parameters.
> > > 
> > 
> > Okay. This goes into the commit message and possibly some part of it to property
> > description also.
> 
> 
> 
> 
> > 
> > - Mani
> > 
> 

-- 
With best wishes
Dmitry

回复: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[yizhijiao2025]

-----邮件原件-----
发件人: linux-arm-msm+bounces-68747-yizhijiao2025=163.com@vger.kernel.org <linux-arm-msm+bounces-68747-yizhijiao2025=163.com@vger.kernel.org> 代表 Dmitry Baryshkov
发送时间: 2025年8月12日 18:52
收件人: Nitin Rawat <quic_nitirawa@quicinc.com>
抄送: Manivannan Sadhasivam <mani@kernel.org>; Krzysztof Kozlowski <krzk@kernel.org>; Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>; vkoul@kernel.org; kishon@kernel.org; conor+dt@kernel.org; bvanassche@acm.org; andersson@kernel.org; neil.armstrong@linaro.org; konradybcio@kernel.org; krzk+dt@kernel.org; linux-arm-msm@vger.kernel.org; linux-phy@lists.infradead.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
主题: Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

On Tue, Aug 12, 2025 at 01:25:02AM +0530, Nitin Rawat wrote:
> 
> 
> On 8/9/2025 4:37 PM, Manivannan Sadhasivam wrote:
> > On Fri, Aug 08, 2025 at 08:49:45PM GMT, Nitin Rawat wrote:
> > > 
> > > 
> > > On 8/8/2025 3:09 PM, Krzysztof Kozlowski wrote:
> > > > On 08/08/2025 10:58, Konrad Dybcio wrote:
> > > > > On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
> > > > > > On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
> > > > > > > Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` 
> > > > > > > properties to the UFS PHY node in the device tree.
> > > > > > > 
> > > > > > > These properties define the maximum current (in microamps) 
> > > > > > > expected from the PHY and PLL regulators. This allows the 
> > > > > > > PHY driver to configure regulator load accurately and 
> > > > > > > ensure proper regulator mode based on load requirements.
> > > > > > 
> > > > > > That's not the property of phy, but regulator.
> > > > > > 
> > > > > > Also reasoning is here incomplete - you just post downstream 
> > > > > > code. :/
> > > > > 
> > > > > The reason for this change is good, but perhaps not explained 
> > > > > clearly
> > > > > 
> > > > > All of these values refer to the maximum current draw that 
> > > > > needs to be allocated on a shared voltage supply for this 
> > > > > peripheral (because the
> > > > 
> > > > 
> > > > It sounds very different than how much it can be drawn. How much 
> > > > can be drawn is the property of the regulator. The regulator 
> > > > knows how much current it can support.
> > > 
> > > Consumers are aware of their dynamic load requirements, which can 
> > > vary at runtime—this awareness is not reciprocal. The power grid 
> > > is designed based on the collective load requirements of all 
> > > clients sharing the same power rail.
> > > 
> > > Since regulators can operate in multiple modes for power 
> > > optimization, each consumer is expected to vote for its runtime 
> > > power needs. These votes help the regulator framework maintain the 
> > > regulator in the appropriate mode, ensuring stable and efficient operation across all clients.
> > > 
> > > 
> > > Stability issues can arise if each consumer does not vote for its 
> > > own load requirement.
> > > For example, consider a scenario where a single regulator is 
> > > shared by two consumers.
> > > 
> > > If the first client requests low-power mode by voting for zero or 
> > > a minimal load to regulator framework during its driver's LPM 
> > > sequence, and the second client (e.g., UFS PHY) has not voted for 
> > > its own load requirement through the regulator framework, the regulator may transition to low-power mode.
> > > This can lead to issues for the second client, which expects a 
> > > higher power state to operate correctly.
> > > 
> > 
> > I think we all agree on consumers setting the load for shared 
> > regulators, but the naming and description of the DT property is what causing confusion here.
> > There is no way the consumers can set the *max* current draw for a 
> > shared regulator. They can only request load as per their 
> > requirement. But the max current draw is a regulator constraint.
> 
> To avoid confusion with regulator-level constraints, I'm open to 
> renaming the property vdda-phy-max-microamp to something more 
> descriptive, such as vdda-phy-client-peak-load-microamp or 
> vdda-phy-peak-load-microamp. Along with updating the description, this 
> would better reflect the property's actual intent: to specify the 
> maximum current a client may draw during peak operation, rather than 
> implying it defines the regulator’s maximum capability.

Move them into the driver please.

> 
> 
> Having said that, 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 does not change 
> across different boards(atleast for now). Therefore, I can remove from 
> device tree and replaced with hardcoded, per-compatible data in the driver.
> 
> > 
> > > 
> > > > 
> > > > 
> > > > > supply's capabilities change depending on the maximum 
> > > > > potential load at any given time, which the regulator driver 
> > > > > must be aware of)
> > > > > 
> > > > > This is a property of a regulator *consumer*, i.e. if we had a 
> > > > > chain of LEDs hanging off of this supply, we'd need to specify 
> > > > > NUM_LEDS * MAX_CURR under the "led chain" device, to make sure 
> > > > > that if the aggregated current requirements go over a certain 
> > > > > threshold (which is unknown to Linux and hidden in RPMh fw), 
> > > > > the regulator can be reconfigured to allow for a higher 
> > > > > current draw (likely at some downgrade to efficiency)
> > > > 
> > > > 
> > > > The problem is that rationale is downstream. Instead I want to 
> > > > see some
> > > > reason: e.g. datasheets, spec, type of UFS device (that was the 
> > > > argument in the driver patch discussion).
> > > 
> > > 
> > > The PHY load requirements for consumers such as UFS, USB, PCIe are 
> > > defined by Qualcomm’s PHY IP and are well-documented in Qualcomm’s 
> > > datasheets and power grid documentation. These values can 
> > > depending on the process or technology node, board design, and even the chip foundry used.
> > > 
> > > As a result, the load values can differ across SoCs or may be even 
> > > board(unlikely though) due to variations in any of these parameters.
> > > 
> > 
> > Okay. This goes into the commit message and possibly some part of it 
> > to property description also.
> 
> 
> 
> 
> > 
> > - Mani
> > 
> 

--
With best wishes
Dmitry

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Nitin Rawat]

On 8/12/2025 4:22 PM, Dmitry Baryshkov wrote:
> On Tue, Aug 12, 2025 at 01:25:02AM +0530, Nitin Rawat wrote:
>>
>>
>> On 8/9/2025 4:37 PM, Manivannan Sadhasivam wrote:
>>> On Fri, Aug 08, 2025 at 08:49:45PM GMT, Nitin Rawat wrote:
>>>>
>>>>
>>>> On 8/8/2025 3:09 PM, Krzysztof Kozlowski wrote:
>>>>> On 08/08/2025 10:58, Konrad Dybcio wrote:
>>>>>> On 8/8/25 9:29 AM, Krzysztof Kozlowski wrote:
>>>>>>> On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
>>>>>>>> Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
>>>>>>>> the UFS PHY node in the device tree.
>>>>>>>>
>>>>>>>> These properties define the maximum current (in microamps) expected
>>>>>>>> from the PHY and PLL regulators. This allows the PHY driver to
>>>>>>>> configure regulator load accurately and ensure proper regulator
>>>>>>>> mode based on load requirements.
>>>>>>>
>>>>>>> That's not the property of phy, but regulator.
>>>>>>>
>>>>>>> Also reasoning is here incomplete - you just post downstream code. :/
>>>>>>
>>>>>> The reason for this change is good, but perhaps not explained clearly
>>>>>>
>>>>>> All of these values refer to the maximum current draw that needs to be
>>>>>> allocated on a shared voltage supply for this peripheral (because the
>>>>>
>>>>>
>>>>> It sounds very different than how much it can be drawn. How much can be
>>>>> drawn is the property of the regulator. The regulator knows how much
>>>>> current it can support.
>>>>
>>>> Consumers are aware of their dynamic load requirements, which can vary at
>>>> runtime—this awareness is not reciprocal. The power grid is designed based
>>>> on the collective load requirements of all clients sharing the same power
>>>> rail.
>>>>
>>>> Since regulators can operate in multiple modes for power optimization, each
>>>> consumer is expected to vote for its runtime power needs. These votes help
>>>> the regulator framework maintain the regulator in the appropriate mode,
>>>> ensuring stable and efficient operation across all clients.
>>>>
>>>>
>>>> Stability issues can arise if each consumer does not vote for its own load
>>>> requirement.
>>>> For example, consider a scenario where a single regulator is shared by two
>>>> consumers.
>>>>
>>>> If the first client requests low-power mode by voting for zero or a minimal
>>>> load to regulator framework during its driver's LPM sequence, and the second
>>>> client (e.g., UFS PHY) has not voted for its own load requirement through
>>>> the regulator framework, the regulator may transition to low-power mode.
>>>> This can lead to issues for the second client, which expects a higher power
>>>> state to operate correctly.
>>>>
>>>
>>> I think we all agree on consumers setting the load for shared regulators, but
>>> the naming and description of the DT property is what causing confusion here.
>>> There is no way the consumers can set the *max* current draw for a shared
>>> regulator. They can only request load as per their requirement. But the max
>>> current draw is a regulator constraint.
>>
>> To avoid confusion with regulator-level constraints, I'm open to renaming
>> the property vdda-phy-max-microamp to something more descriptive, such as
>> vdda-phy-client-peak-load-microamp or vdda-phy-peak-load-microamp. Along
>> with updating the description, this would better reflect the property's
>> actual intent: to specify the maximum current a client may draw during peak
>> operation, rather than implying it defines the regulator’s maximum
>> capability.
> 
> Move them into the driver please.

Sure Dmitry. I’ll will take care of this in next patchset.

Thanks,
Nitin


> 
>>
>>
>> Having said that, 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 does not change across
>> different boards(atleast for now). Therefore, I can remove from device tree
>> and replaced with hardcoded, per-compatible data in the driver.
>>
>>>
>>>>
>>>>>
>>>>>
>>>>>> supply's capabilities change depending on the maximum potential load
>>>>>> at any given time, which the regulator driver must be aware of)
>>>>>>
>>>>>> This is a property of a regulator *consumer*, i.e. if we had a chain
>>>>>> of LEDs hanging off of this supply, we'd need to specify NUM_LEDS *
>>>>>> MAX_CURR under the "led chain" device, to make sure that if the
>>>>>> aggregated current requirements go over a certain threshold (which is
>>>>>> unknown to Linux and hidden in RPMh fw), the regulator can be
>>>>>> reconfigured to allow for a higher current draw (likely at some
>>>>>> downgrade to efficiency)
>>>>>
>>>>>
>>>>> The problem is that rationale is downstream. Instead I want to see some
>>>>> reason: e.g. datasheets, spec, type of UFS device (that was the argument
>>>>> in the driver patch discussion).
>>>>
>>>>
>>>> The PHY load requirements for consumers such as UFS, USB, PCIe are defined
>>>> by Qualcomm’s PHY IP and are well-documented in Qualcomm’s datasheets and
>>>> power grid documentation. These values can depending on the process or
>>>> technology node, board design, and even the chip foundry used.
>>>>
>>>> As a result, the load values can differ across SoCs or may be even
>>>> board(unlikely though) due to variations in any of these parameters.
>>>>
>>>
>>> Okay. This goes into the commit message and possibly some part of it to property
>>> description also.
>>
>>
>>
>>
>>>
>>> - Mani
>>>
>>
> 

Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS PHY and PLL supplies

[Bjorn Andersson]

On Wed, Aug 06, 2025 at 09:13:38PM +0530, Nitin Rawat wrote:
> Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
> the UFS PHY node in the device tree.
> 
> These properties define the maximum current (in microamps) expected
> from the PHY and PLL regulators. This allows the PHY driver to
> configure regulator load accurately and ensure proper regulator
> mode based on load requirements.
> 

But doesn't this imply that these values are fixed for a given SoC?
Perhaps even for a given generation of the PHY/process?

Is there any case where these values need to be tweaked on a per-board
basis?


If not, I think these should be constants in the driver.

Regards,
Bjorn

> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8750-mtp.dts | 2 ++
>  arch/arm64/boot/dts/qcom/sm8750-qrd.dts | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8750-mtp.dts b/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
> index 75cfbb510be5..2ae5915fe38d 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8750-mtp.dts
> @@ -1032,7 +1032,9 @@ wcd_default: wcd-reset-n-active-state {
>  
>  &ufs_mem_phy {
>  	vdda-phy-supply = <&vreg_l1j_0p91>;
> +	vdda-phy-max-microamp = <213000>;
>  	vdda-pll-supply = <&vreg_l3g_1p2>;
> +	vdda-pll-max-microamp = <18300>;
>  
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sm8750-qrd.dts b/arch/arm64/boot/dts/qcom/sm8750-qrd.dts
> index 13c7b9664c89..e9a41d34e2d6 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750-qrd.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8750-qrd.dts
> @@ -1039,7 +1039,9 @@ &uart7 {
>  
>  &ufs_mem_phy {
>  	vdda-phy-supply = <&vreg_l1j_0p91>;
> +	vdda-phy-max-microamp = <213000>;
>  	vdda-pll-supply = <&vreg_l3g_1p2>;
> +	vdda-pll-max-microamp = <18300>;
>  
>  	status = "okay";
>  };
> -- 
> 2.48.1
> 

[PATCH V1 3/4] arm64: dts: qcom: sm8650: add max-microamp for UFS PHY and PLL supplies

[Nitin Rawat]

Add `vdda-phy-max-microamp` and `vdda-pll-max-microamp` properties to
the UFS PHY node in the device tree.

These properties define the maximum current (in microamps) expected
from the PHY and PLL regulators. This allows the PHY driver to
configure regulator load accurately and ensure proper regulator mode
based on load requirements.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8650-hdk.dts | 2 ++
 arch/arm64/boot/dts/qcom/sm8650-mtp.dts | 3 ++-
 arch/arm64/boot/dts/qcom/sm8650-qrd.dts | 2 ++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
index d0912735b54e..356254b11906 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-hdk.dts
@@ -1294,7 +1294,9 @@ &ufs_mem_hc {
 
 &ufs_mem_phy {
 	vdda-phy-supply = <&vreg_l1d_0p88>;
+	vdda-phy-max-microamp = <211000>;
 	vdda-pll-supply = <&vreg_l3i_1p2>;
+	vdda-pll-max-microamp = <18300>;
 
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
index 76ef43c10f77..f47f62e0b0d9 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-mtp.dts
@@ -841,8 +841,9 @@ &ufs_mem_hc {
 
 &ufs_mem_phy {
 	vdda-phy-supply = <&vreg_l1d_0p88>;
+	vdda-phy-max-microamp = <211000>;
 	vdda-pll-supply = <&vreg_l3i_1p2>;
-
+	vdda-pll-max-microamp = <18300>;
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
index 71033fba21b5..c4359f8033e3 100644
--- a/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8650-qrd.dts
@@ -1277,7 +1277,9 @@ &ufs_mem_hc {
 
 &ufs_mem_phy {
 	vdda-phy-supply = <&vreg_l1d_0p88>;
+	vdda-phy-max-microamp = <211000>;
 	vdda-pll-supply = <&vreg_l3i_1p2>;
+	vdda-pll-max-microamp = <18300>;
 
 	status = "okay";
 };
-- 
2.48.1

[PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Konrad Dybcio]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Mark Brown]

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

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.

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Konrad Dybcio]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Mark Brown]

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

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.

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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.

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Mark Brown]

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

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.

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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.

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Mark Brown]

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

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.

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Mark Brown]

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

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.

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Krzysztof Kozlowski]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Konrad Dybcio]

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 

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Mark Brown]

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

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.

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Bjorn Andersson]

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.

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Mark Brown]

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

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.

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Manivannan Sadhasivam]

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

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Dmitry Baryshkov]

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

Re: [PATCH V1 4/4] phy: qcom-qmp-ufs: read max-microamp values from device tree

[Nitin Rawat]

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