diff mbox series

[1/3] dt-bindings: cpufreq: qcom-cpufreq-nvmem: make cpr bindings optional

Message ID 20230121000146.7809-1-ansuelsmth@gmail.com
State New
Headers
Series [1/3] dt-bindings: cpufreq: qcom-cpufreq-nvmem: make cpr bindings optional |

Commit Message

Christian Marangi Jan. 21, 2023, 12:01 a.m. UTC 
  The qcom-cpufreq-nvmem driver also supports legacy devices pre-cpr that
doesn't have power-domains. When the schema was introduced, it was
wrongly set to always require these binding but this is not the case for
legacy device that base everything on nvmem cells and multiple microvolt
binding providing values based on speedbin, psv and version.

Make the power-domain optional and set them required only for qcs404
based devices.

Fixes: ec24d1d55469 ("dt-bindings: opp: Convert qcom-nvmem-cpufreq to DT schema")
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../bindings/cpufreq/qcom-cpufreq-nvmem.yaml  | 78 +++++++++++++------
 1 file changed, 54 insertions(+), 24 deletions(-)

Comments

Krzysztof Kozlowski Jan. 22, 2023, 1:57 p.m. UTC | #1 
On 21/01/2023 01:01, Christian Marangi wrote:
> The qcom-cpufreq-nvmem driver also supports legacy devices pre-cpr that
> doesn't have power-domains. When the schema was introduced, it was
> wrongly set to always require these binding but this is not the case for
> legacy device that base everything on nvmem cells and multiple microvolt

What is a "legacy device"? Why do you adjust bindings to legacy device?
Can't you just fix the DTS on these devices?

> binding providing values based on speedbin, psv and version.
> 
> Make the power-domain optional and set them required only for qcs404
> based devices.
> 
> Fixes: ec24d1d55469 ("dt-bindings: opp: Convert qcom-nvmem-cpufreq to DT schema")
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../bindings/cpufreq/qcom-cpufreq-nvmem.yaml  | 78 +++++++++++++------
>  1 file changed, 54 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml b/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
> index 9c086eac6ca7..04aac634d7fc 100644
> --- a/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
> +++ b/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
> @@ -17,6 +17,9 @@ description: |
>    on the CPU OPP in use. The CPUFreq driver sets the CPR power domain level
>    according to the required OPPs defined in the CPU OPP tables.
>  
> +  For old implementation efuses are parsed to select the correct opp table and
> +  voltage and CPR is not supported/used.
> +
>  select:
>    properties:
>      compatible:
> @@ -33,37 +36,64 @@ select:
>    required:
>      - compatible
>  
> -properties:
> -  cpus:
> -    type: object
> +if:

Missing allOf. Fix also the placement - this block goes before
additionalPropertes:false.

> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - qcom,qcs404
>  
> -    patternProperties:
> -      '^cpu@[0-9a-f]+$':
> -        type: object
> +then:
> +  properties:
> +    cpus:
> +      type: object
>  
> -        properties:
> -          power-domains:

Best regards,
Krzysztof
Christian Marangi Jan. 22, 2023, 2:05 p.m. UTC | #2 
On Sun, Jan 22, 2023 at 02:57:07PM +0100, Krzysztof Kozlowski wrote:
> On 21/01/2023 01:01, Christian Marangi wrote:
> > The qcom-cpufreq-nvmem driver also supports legacy devices pre-cpr that
> > doesn't have power-domains. When the schema was introduced, it was
> > wrongly set to always require these binding but this is not the case for
> > legacy device that base everything on nvmem cells and multiple microvolt
> 
> What is a "legacy device"? Why do you adjust bindings to legacy device?
> Can't you just fix the DTS on these devices?
>

With legacy I mean device where cpr (core power reduction) wasn't a
thing and qcom used a different way to select the microvolt for the opp.

There is nothing in the related DTS to fix since they are not broken.
The driver doesn't enforce cpr presence and supports both new and old
implementation...

Setting the cpr as a required binding was wrong from the start. It was
probably done when qcs404 was introduced and they had this bright idea
of creating the schema and ignoring the other kind of configuration the
driver supports.

Since now we want to send opp for ipq8064 that use the old
implementation this fixup is required.

Probably I should drop the legacy term and just say that the driver
supports 2 different configuration and the following schema permits only
one?

> > binding providing values based on speedbin, psv and version.
> > 
> > Make the power-domain optional and set them required only for qcs404
> > based devices.
> > 
> > Fixes: ec24d1d55469 ("dt-bindings: opp: Convert qcom-nvmem-cpufreq to DT schema")
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../bindings/cpufreq/qcom-cpufreq-nvmem.yaml  | 78 +++++++++++++------
> >  1 file changed, 54 insertions(+), 24 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml b/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
> > index 9c086eac6ca7..04aac634d7fc 100644
> > --- a/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
> > +++ b/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
> > @@ -17,6 +17,9 @@ description: |
> >    on the CPU OPP in use. The CPUFreq driver sets the CPR power domain level
> >    according to the required OPPs defined in the CPU OPP tables.
> >  
> > +  For old implementation efuses are parsed to select the correct opp table and
> > +  voltage and CPR is not supported/used.
> > +
> >  select:
> >    properties:
> >      compatible:
> > @@ -33,37 +36,64 @@ select:
> >    required:
> >      - compatible
> >  
> > -properties:
> > -  cpus:
> > -    type: object
> > +if:
> 
> Missing allOf. Fix also the placement - this block goes before
> additionalPropertes:false.
> 

Thanks for the review! Will fix in v2.

> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - qcom,qcs404
> >  
> > -    patternProperties:
> > -      '^cpu@[0-9a-f]+$':
> > -        type: object
> > +then:
> > +  properties:
> > +    cpus:
> > +      type: object
> >  
> > -        properties:
> > -          power-domains:
> 
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Jan. 22, 2023, 2:13 p.m. UTC | #3 
On 22/01/2023 15:05, Christian Marangi wrote:
> On Sun, Jan 22, 2023 at 02:57:07PM +0100, Krzysztof Kozlowski wrote:
>> On 21/01/2023 01:01, Christian Marangi wrote:
>>> The qcom-cpufreq-nvmem driver also supports legacy devices pre-cpr that
>>> doesn't have power-domains. When the schema was introduced, it was
>>> wrongly set to always require these binding but this is not the case for
>>> legacy device that base everything on nvmem cells and multiple microvolt
>>
>> What is a "legacy device"? Why do you adjust bindings to legacy device?
>> Can't you just fix the DTS on these devices?
>>
> 
> With legacy I mean device where cpr (core power reduction) wasn't a
> thing and qcom used a different way to select the microvolt for the opp.
> 
> There is nothing in the related DTS to fix since they are not broken.
> The driver doesn't enforce cpr presence and supports both new and old
> implementation...
> 
> Setting the cpr as a required binding was wrong from the start. It was
> probably done when qcs404 was introduced and they had this bright idea
> of creating the schema and ignoring the other kind of configuration the
> driver supports.
> 
> Since now we want to send opp for ipq8064 that use the old
> implementation this fixup is required.
> 
> Probably I should drop the legacy term and just say that the driver
> supports 2 different configuration and the following schema permits only
> one?

Yes, it would be clearer.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml b/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
index 9c086eac6ca7..04aac634d7fc 100644
--- a/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
+++ b/Documentation/devicetree/bindings/cpufreq/qcom-cpufreq-nvmem.yaml
@@ -17,6 +17,9 @@  description: |
   on the CPU OPP in use. The CPUFreq driver sets the CPR power domain level
   according to the required OPPs defined in the CPU OPP tables.
 
+  For old implementation efuses are parsed to select the correct opp table and
+  voltage and CPR is not supported/used.
+
 select:
   properties:
     compatible:
@@ -33,37 +36,64 @@  select:
   required:
     - compatible
 
-properties:
-  cpus:
-    type: object
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - qcom,qcs404
 
-    patternProperties:
-      '^cpu@[0-9a-f]+$':
-        type: object
+then:
+  properties:
+    cpus:
+      type: object
 
-        properties:
-          power-domains:
-            maxItems: 1
+      patternProperties:
+        '^cpu@[0-9a-f]+$':
+          type: object
+
+          properties:
+            power-domains:
+              maxItems: 1
 
-          power-domain-names:
-            items:
-              - const: cpr
+            power-domain-names:
+              items:
+                - const: cpr
 
-        required:
-          - power-domains
-          - power-domain-names
+          required:
+            - power-domains
+            - power-domain-names
+
+  patternProperties:
+    '^opp-table(-[a-z0-9]+)?$':
+      if:
+        properties:
+          compatible:
+            const: operating-points-v2-kryo-cpu
+      then:
+        patternProperties:
+          '^opp-?[0-9]+$':
+            required:
+              - required-opps
 
 patternProperties:
   '^opp-table(-[a-z0-9]+)?$':
-    if:
-      properties:
-        compatible:
-          const: operating-points-v2-kryo-cpu
-    then:
-      patternProperties:
-        '^opp-?[0-9]+$':
-          required:
-            - required-opps
+    allOf:
+      - if:
+          properties:
+            compatible:
+              const: operating-points-v2-kryo-cpu
+        then:
+          $ref: /schemas/opp/opp-v2-kryo-cpu.yaml#
+
+      - if:
+          properties:
+            compatible:
+              const: operating-points-v2-qcom-level
+        then:
+          $ref: /schemas/opp/opp-v2-qcom-level.yaml#
+
+    unevaluatedProperties: false
 
 additionalProperties: true