diff mbox series

[01/18] dt-bindings: opp: opp-v2-kryo-cpu: support Qualcomm Krait SoCs

Message ID 20230612053922.3284394-2-dmitry.baryshkov@linaro.org
State Superseded
Headers show
Series ARM: qcom: apq8064: support CPU frequency scaling | expand

Commit Message

Dmitry Baryshkov June 12, 2023, 5:39 a.m. UTC
Exted the opp-v2-kryo-cpu.yaml to support defining OPP tables for the
previous generation of Qualcomm CPUs, 32-bit Krait-based platforms.

It makes no sense to use 'operating-points-v2-kryo-cpu' compatibility
node for the Krait cores. Add support for the Krait-specific
'operating-points-v2-krait-cpu' compatibility string and the relevant
opp-microvolt subclasses properties.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../devicetree/bindings/opp/opp-v2-kryo-cpu.yaml      | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Krzysztof Kozlowski June 14, 2023, 4:01 p.m. UTC | #1
On 12/06/2023 07:39, Dmitry Baryshkov wrote:
> Exted the opp-v2-kryo-cpu.yaml to support defining OPP tables for the
> previous generation of Qualcomm CPUs, 32-bit Krait-based platforms.
> 
> It makes no sense to use 'operating-points-v2-kryo-cpu' compatibility
> node for the Krait cores. Add support for the Krait-specific
> 'operating-points-v2-krait-cpu' compatibility string and the relevant
> opp-microvolt subclasses properties.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../devicetree/bindings/opp/opp-v2-kryo-cpu.yaml      | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
> index bbbad31ae4ca..93ec778bf333 100644
> --- a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
> @@ -26,7 +26,9 @@ description: |
>  
>  properties:
>    compatible:
> -    const: operating-points-v2-kryo-cpu
> +    enum:
> +      - operating-points-v2-krait-cpu
> +      - operating-points-v2-kryo-cpu
>  
>    nvmem-cells:
>      description: |
> @@ -63,14 +65,15 @@ patternProperties:
>            5:  MSM8996SG, speedbin 1
>            6:  MSM8996SG, speedbin 2
>            7-31:  unused
> -        enum: [0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
> -               0x9, 0xd, 0xe, 0xf,
> -               0x10, 0x20, 0x30, 0x70]

Why?

> +        $ref: /schemas/types.yaml#/definitions/uint32

You are changing the type. No. It should be fixed instead (enum applies
to items).

>  
>        clock-latency-ns: true
>  
>        required-opps: true
>  
> +    patternProperties:
> +      '^opp-microvolt-speed[0-9]+-pvs[0-9]+$': true

I don't think it is a common property, so it needs description and
specific type. Specifically "pvs[0-9]" something entirely new.



Best regards,
Krzysztof
Dmitry Baryshkov June 14, 2023, 8:11 p.m. UTC | #2
On 14/06/2023 19:01, Krzysztof Kozlowski wrote:
> On 12/06/2023 07:39, Dmitry Baryshkov wrote:
>> Exted the opp-v2-kryo-cpu.yaml to support defining OPP tables for the
>> previous generation of Qualcomm CPUs, 32-bit Krait-based platforms.
>>
>> It makes no sense to use 'operating-points-v2-kryo-cpu' compatibility
>> node for the Krait cores. Add support for the Krait-specific
>> 'operating-points-v2-krait-cpu' compatibility string and the relevant
>> opp-microvolt subclasses properties.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   .../devicetree/bindings/opp/opp-v2-kryo-cpu.yaml      | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
>> index bbbad31ae4ca..93ec778bf333 100644
>> --- a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
>> @@ -26,7 +26,9 @@ description: |
>>   
>>   properties:
>>     compatible:
>> -    const: operating-points-v2-kryo-cpu
>> +    enum:
>> +      - operating-points-v2-krait-cpu
>> +      - operating-points-v2-kryo-cpu
>>   
>>     nvmem-cells:
>>       description: |
>> @@ -63,14 +65,15 @@ patternProperties:
>>             5:  MSM8996SG, speedbin 1
>>             6:  MSM8996SG, speedbin 2
>>             7-31:  unused
>> -        enum: [0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
>> -               0x9, 0xd, 0xe, 0xf,
>> -               0x10, 0x20, 0x30, 0x70]
> 
> Why?
> 
>> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> You are changing the type. No. It should be fixed instead (enum applies
> to items).

Currenlty this bindings are only usable for msm8996/msm8996pro. As such 
we listed opp-supported-hw values that are applicable to this platform. 
This series adds support for apq8064 platform, which will add new items 
to this enum. I think it is not very sensible to list all of them here.

However granted there is already a good enough base type definition, I 
think it would be better to drop the $ref, drop the enum, add ': true' 
(is it necessary if we have a description already?) and expand 
documentation.

> 
>>   
>>         clock-latency-ns: true
>>   
>>         required-opps: true
>>   
>> +    patternProperties:
>> +      '^opp-microvolt-speed[0-9]+-pvs[0-9]+$': true
> 
> I don't think it is a common property, so it needs description and
> specific type. Specifically "pvs[0-9]" something entirely new.

Ack.

> 
> 
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski June 21, 2023, 8:51 a.m. UTC | #3
On 14/06/2023 22:11, Dmitry Baryshkov wrote:

>> Why?
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>
>> You are changing the type. No. It should be fixed instead (enum applies
>> to items).
> 
> Currenlty this bindings are only usable for msm8996/msm8996pro. As such 
> we listed opp-supported-hw values that are applicable to this platform. 
> This series adds support for apq8064 platform, which will add new items 
> to this enum. I think it is not very sensible to list all of them here.

Sure, but this is uint32-matrix, so don't change the type to something else.

> 
> However granted there is already a good enough base type definition, I 
> think it would be better to drop the $ref, drop the enum, add ': true' 
> (is it necessary if we have a description already?) and expand 
> documentation.

Probably this should be constrained to only one value with:
  items:
    - items:
       - description: foo bar




Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
index bbbad31ae4ca..93ec778bf333 100644
--- a/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
+++ b/Documentation/devicetree/bindings/opp/opp-v2-kryo-cpu.yaml
@@ -26,7 +26,9 @@  description: |
 
 properties:
   compatible:
-    const: operating-points-v2-kryo-cpu
+    enum:
+      - operating-points-v2-krait-cpu
+      - operating-points-v2-kryo-cpu
 
   nvmem-cells:
     description: |
@@ -63,14 +65,15 @@  patternProperties:
           5:  MSM8996SG, speedbin 1
           6:  MSM8996SG, speedbin 2
           7-31:  unused
-        enum: [0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7,
-               0x9, 0xd, 0xe, 0xf,
-               0x10, 0x20, 0x30, 0x70]
+        $ref: /schemas/types.yaml#/definitions/uint32
 
       clock-latency-ns: true
 
       required-opps: true
 
+    patternProperties:
+      '^opp-microvolt-speed[0-9]+-pvs[0-9]+$': true
+
     required:
       - opp-hz