Re: [PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/10/2024 21:13, Akhil P Oommen wrote:
> On Mon, Oct 14, 2024 at 09:39:01AM +0200, Krzysztof Kozlowski wrote:
>> On Sat, Oct 12, 2024 at 01:59:29AM +0530, Akhil P Oommen wrote:
>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>> for each opp needs to be shared to GMU during runtime.
>>>
>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>> ---
>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 84 ++++++++++++++++++++++
>>>  1 file changed, 84 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>> new file mode 100644
>>> index 000000000000..9fb828e9da86
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>> @@ -0,0 +1,84 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm Adreno compatible OPP supply
>>> +
>>> +description:
>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>> +  ACD related information tailored for the specific chipset. This binding
>>> +  provides the information needed to describe such a hardware value.
>>> +
>>> +maintainers:
>>> +  - Rob Clark <robdclark@gmail.com>
>>> +
>>> +allOf:
>>> +  - $ref: opp-v2-base.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: operating-points-v2-adreno
>>> +
>>> +patternProperties:
>>> +  '^opp-?[0-9]+$':
>>> +    type: object
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      opp-hz: true
>>> +
>>> +      opp-level: true
>>> +
>>> +      opp-peak-kBps: true
>>> +
>>> +      opp-supported-hw: true
>>> +
>>> +      qcom,opp-acd-level:
>>> +        description: |
>>> +          A positive value representing the acd level associated with this
>>
>> What is acd?
> 
> Adaptive Clock Distribution, a fancy name for clock throttling during voltage
> droop. I will update the description to capture this.
> 
>>
>>> +          OPP node. This value is shared to GMU during GPU wake up. It may
>>
>> What is GMU?
> 
> A co-processor which does power management for Adreno GPU.

Everything, except obvious GPU, should be explained. GMU is not really
that obvious:
https://en.wikipedia.org/wiki/GMU

> 
>>
>>> +          not be present for some OPPs and GMU will disable ACD while
>>
>> acd or ACD?
> 
> should be uppercase everywhere in description.
> 
>>
>>> +          transitioning to that OPP.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>> +    required:
>>> +      - opp-hz
>>> +      - opp-level
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +
>>
>> Drop blank line
>>
>>> +    #include <dt-bindings/power/qcom-rpmpd.h>
>>> +
>>> +    gpu_opp_table: opp-table {
>>> +        compatible = "operating-points-v2-adreno";
>>> +
>>> +        opp-550000000 {
>>> +                opp-hz = /bits/ 64 <550000000>;
>>> +                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
>>> +                opp-peak-kBps = <6074219>;
>>> +                qcom,opp-acd-level = <0xc0285ffd>;
>>> +        };
>>> +
>>> +        opp-390000000 {
>>> +                opp-hz = /bits/ 64 <390000000>;
>>> +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>>> +                opp-peak-kBps = <3000000>;
>>> +                qcom,opp-acd-level = <0xc0285ffd>;
>>
>> That's the same value used everywhere. What's the point? Just encode it
>> in the driver.
> 
> I will update this to keep a different value. In a real implmentation,
> these values may vary between OPPs. For eg:, please check the DT patch
> in this series:
> 
> https://patchwork.freedesktop.org/patch/619413/

OK. I still have concerns that it is just some magic hex value. Which
looks exactly how downstream code. No explanation, no meaning: neither
in property description nor in actual value (at least I could not spot it).

And why this is hex? Unit of "level" is either some logical meaning,
like "high" or "low", or some unit, e.g. Hertz or kBps. None of them are
hex values in real world.

Best regards,
Krzysztof