Message-ID: <ljythvl2yfilcnmgdwt2cyyefxmgl54osll5e76qn7njadhgqq@rwrl3dy6ykt3>
Date: Tue, 12 Aug 2025 13:52:08 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Nitin Rawat <quic_nitirawa@...cinc.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 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

Powered by blists - more mailing lists