Message-ID: <4e996a17-c996-4194-b57d-128e7d05e8ad@quicinc.com>
Date: Thu, 14 Aug 2025 02:17:04 +0530
From: Nitin Rawat <quic_nitirawa@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
CC: Manivannan Sadhasivam <mani@...nel.org>,
        Krzysztof Kozlowski
	<krzk@...nel.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>, <vkoul@...nel.org>,
        <kishon@...nel.org>, <conor+dt@...nel.org>, <bvanassche@....org>,
        <andersson@...nel.org>, <neil.armstrong@...aro.org>,
        <konradybcio@...nel.org>, <krzk+dt@...nel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-phy@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH V1 2/4] arm64: dts: qcom: sm8750: add max-microamp for UFS
 PHY and PLL supplies



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

Powered by blists - more mailing lists