linux-kernel - Re: [PATCH v10 2/6] dt-bindings: opp: v2-qcom-level: Document CPR3 open/closed loop volt adjustment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c2bfa6b0-edee-b492-d40e-cf43291b90d4@linaro.org>
Date:   Sat, 18 Feb 2023 01:26:32 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Rob Herring <robh@...nel.org>
Cc:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        Niklas Cassel <nks@...wful.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Robert Marko <robimarko@...il.com>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v10 2/6] dt-bindings: opp: v2-qcom-level: Document CPR3
 open/closed loop volt adjustment



On 18.02.2023 00:13, Rob Herring wrote:
> On Fri, Feb 17, 2023 at 12:08:25PM +0100, Konrad Dybcio wrote:
>> CPR3 and newer can be fed per-OPP voltage adjustment values for both
>> open- and closed-loop paths to make better decisions about settling
>> on the final voltage offset target. Document these properties.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>> ---
>>  .../devicetree/bindings/opp/opp-v2-qcom-level.yaml         | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml
>> index a30ef93213c0..93cc88434dfe 100644
>> --- a/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml
>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-level.yaml
>> @@ -34,6 +34,20 @@ patternProperties:
>>          minItems: 1
>>          maxItems: 2
>>  
>> +      qcom,opp-cloop-vadj:
>> +        description: |
>> +          A value representing the closed-loop voltage adjustment value
> 
> A value?
> 
>> +          associated with this OPP node.
>> +        $ref: /schemas/types.yaml#/definitions/int32-array
>> +        maxItems: 2
> 
> Or 2 values?
Right, this description doesn't make any sense if you're just
looking at the documentation without looking at the driver..

Generally, each CPR3 instance can have multiple "threads"
(each one of which regulates voltage for some on-SoC IP or
part of it). The nth entry in the qcom,opp-[co]loop-vadj
array corresponds to a voltage offset for the nth thread.

If the nth entry in the array is missing, the driver assumes
the arr[0] one is "global" to this CPR3 instance at this OPP
level and applies it to all threads. ...and looking at it
again, this is sorta just bad design, especially if you
take into account that there's no known user of CPR3 that
employs more than 2 threads.

I'll remove that from the driver and make the description clearer.


Also, only noticed now.. "qcom,sdm630-cprh" was not documented,
so that's to be fixed for the next submission as well!


Konrad
> 
>> +
>> +      qcom,opp-oloop-vadj:
>> +        description: |
>> +          A value representing the open-loop voltage adjustment value
>> +          associated with this OPP node.
>> +        $ref: /schemas/types.yaml#/definitions/int32-array
>> +        maxItems: 2
>> +
>>      required:
>>        - opp-level
>>        - qcom,opp-fuse-level
>>
>> -- 
>> 2.39.1
>>