Message-ID: <8c7f8cfc-2090-449e-b6ec-688a0021bac4@oss.qualcomm.com>
Date: Thu, 7 Aug 2025 19:43:15 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Mark Brown <broonie@...nel.org>, Nitin Rawat <quic_nitirawa@...cinc.com>
Cc: vkoul@...nel.org, kishon@...nel.org, mani@...nel.org, conor+dt@...nel.org,
        bvanassche@....org, andersson@...nel.org, neil.armstrong@...aro.org,
        dmitry.baryshkov@....qualcomm.com, 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 4/4] phy: qcom-qmp-ufs: read max-microamp values from
 device tree

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 

Powered by blists - more mailing lists