Message-ID: <4f722a25-65f3-49cd-9c45-8710e8242248@kernel.org>
Date: Wed, 11 Dec 2024 11:34:56 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Ziyue Zhang <quic_ziyuzhan@...cinc.com>, vkoul@...nel.org,
 kishon@...nel.org, dmitry.baryshkov@...aro.org, abel.vesa@...aro.org,
 neil.armstrong@...aro.org, andersson@...nel.org, konradybcio@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+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 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current
 load properties

On 11/12/2024 10:52, Krzysztof Kozlowski wrote:
> On 11/12/2024 09:24, Manivannan Sadhasivam wrote:
>> On Wed, Dec 11, 2024 at 09:09:18AM +0100, Krzysztof Kozlowski wrote:
>>> On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
>>>> On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
>>>>> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
>>>>>> On some platforms, the power supply for PCIe PHY is not able to provide
>>>>>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
>>>>>> set current load to vote the regulator to HPM mode.
>>>>>>
>>>>>> Document the current load as properties for each power supply PCIe PHY
>>>>>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
>>>>>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
>>>>>> set appropriate current load during PHY power on.
>>>>>>
>>>>>> This three properties are optional and not mandatory for those platforms
>>>>>> that PCIe PHY can still work with power supply.
>>>>>
>>>>>
>>>>> Uh uh, so the downstream comes finally!
>>>>>
>>>>> No sorry guys, use existing regulator bindings for this.
>>>>>
>>>>
>>>> Maybe they got inspired by upstream UFS bindings?
>>>> Documentation/devicetree/bindings/ufs/ufs-common.yaml:
>>>>
>>>> vcc-max-microamp
>>>> vccq-max-microamp
>>>> vccq2-max-microamp
>>>
>>> And it is already an ABI, so we cannot do anything about it.
>>>
>>>>
>>>> Regulator binding only describes the min/max load for the regulators and not
>>>
>>> No, it exactly describes min/max consumers can use. Let's quote:
>>> "largest current consumers may set"
>>> It is all about consumers.
>>>
>>>> consumers. What if the consumers need to set variable load per platform? Should
>>>
>>> Then each platform uses regulator API or regulator bindings to set it? I
>>> don't see the problem here.
>>>
>>>> they hardcode the load in driver? (even so, the load should not vary for each
>>>> board).
>>>
>>> The load must vary per board, because regulators vary per board. Of
>>> course in practice most designs could be the same, but regulators and
>>> their limits are always properties of the board, not the SoC.
>>>
>>
>> How the consumer drivers are supposed to know the optimum load?
>>
>> I don't see how the consumer drivers can set the load without hardcoding the
>> values. And I could see from UFS properties that each board has different
>> values.
> 
> 
> Drivers do not need to know, it's not the driver's responsibility. If
> these are constraints per board, then regulator properties apply and
> there is no difference between this "vdd-max-microamp = 10" and
> "regulator-max-microamp".
> 
> If this varies runtime, then your property is already not suitable and
> very limited and you should use OPP table.
> 
Plus let's recap the commit msg which is supposed to fully explain the
hardware:
"the power supply for PCIe PHY is not able to provide
enough current"

Well, that's 100% property of power supply - so the regulator node in
PMIC, not the UFS device (consumer).

With that commit msg it is clear the properties are placed wrongly.
However maybe just commit msg is wrong, but then how could we possibly
know what is the real hardware, right? :)


Best regards,
Krzysztof

Powered by blists - more mailing lists