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

[Konrad Dybcio]

On 20.02.2023 12:27, AngeloGioacchino Del Regno wrote:
> Il 18/02/23 01:26, Konrad Dybcio ha scritto:
>>
>>
>> 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@xxxxxxxxxx>
>>>> ---
>>>>   .../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.
>>
> 
> description:
>   Represents the closed-loop voltage adjustment associated with
>   this OPP node.
> 
> P.S.: Drop '|' here and on oloop!
> 
> This binding is intended to support either single or multiple CPR threads;
> the driver's behavior is unimportant as bindings describe the hardware,
> not the driver.
Correct, but specifying just one value regardless of the number of threads
is not in the spirit of representing things clearly. These properties do
not describe the hardware. They let us pass configuration values that are
specific to the SoC hosting the CPR, not to the CPR itself.

Konrad
> 
> Regards,
> Angelo
> 
>>
>> 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
>>>>
> 
> 
>