diff mbox series

[13/13] dt-bindings: cpufreq: qcom-hw: Add bindings for 8998

Message ID 20201126184559.3052375-14-angelogioacchino.delregno@somainline.org
State Changes Requested
Headers show
Series Enable CPRh/3/4, CPU Scaling on various QCOM SoCs | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

AngeloGioacchino Del Regno Nov. 26, 2020, 6:45 p.m. UTC
The OSM programming addition has been done under the
qcom,cpufreq-hw-8998 compatible name: specify the requirement
of two additional register spaces for this functionality.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
 .../bindings/cpufreq/qcom,cpufreq-hw.yaml     | 31 ++++++++++++++++---
 1 file changed, 27 insertions(+), 4 deletions(-)

Comments

Rob Herring Dec. 8, 2020, 6:11 p.m. UTC | #1
On Thu, Nov 26, 2020 at 07:45:59PM +0100, AngeloGioacchino Del Regno wrote:
> The OSM programming addition has been done under the
> qcom,cpufreq-hw-8998 compatible name: specify the requirement
> of two additional register spaces for this functionality.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
>  .../bindings/cpufreq/qcom,cpufreq-hw.yaml     | 31 ++++++++++++++++---
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
> index 94a56317b14b..f64cea73037e 100644
> --- a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
> +++ b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
> @@ -23,17 +23,21 @@ properties:
>            - qcom,cpufreq-epss
>  
>    reg:
> +    description: Base address and size of the RBCPR register region

That doesn't make sense given you have 2 regions.

>      minItems: 2
>      maxItems: 2

maxItems: 4

>  
>    reg-names:
>      description:
> -      Frequency domain register region for each domain.
> -    items:
> -      - const: "freq-domain0"
> -      - const: "freq-domain1"
> +      Frequency domain register region for each domain. If OSM programming
> +      does not happen in the bootloader and has to be done in this driver,
> +      then also the OSM domain region osm-domain[0-1] has to be provided.

Don't write free form text for what can be expressed as schema.

> +    minItems: 2
> +    maxItems: 2

You obviously haven't tried this change with 8998. It will fail with 
more than 2. What you need here is:

minItems: 2
maxItems: 4

items:
  - const: "freq-domain0"
  - const: "freq-domain1"
  - const: "osm-domain0"
  - const: "osm-domain1"

And then...

>  
>    clock-names:
> +    minItems: 2
> +    maxItems: 2
>      - const: xo
>      - const: ref
>  
> @@ -53,9 +57,28 @@ properties:
>        property with phandle to a cpufreq_hw followed by the Domain ID(0/1)
>        in the CPU DT node.
>  
> +allOf:
> + - if:
> +     properties:
> +       reg-names:
> +         contains:
> +           const: qcom,cpufreq-hw-8998
> +   then:
> +     properties:
> +       reg:
> +         minItems: 4
> +         maxItems: 4
> +       reg-names:

...here just:

minItems: 4

And you'll need an 'else' clause with 'maxItems: 2' for reg and 
reg-names.

> +         items:
> +           - const: "freq-domain0"
> +           - const: "freq-domain1"
> +           - const: "osm-domain0"
> +           - const: "osm-domain1"
> +
>  required:
>    - compatible
>    - reg
> +  - reg-names

You can't make something that was optional now required. (Unless it was 
a mistake and all existing users always had 'reg-names'.)

>    - clock-names
>    - clocks
>    - "#freq-domain-cells"
> -- 
> 2.29.2
>
AngeloGioacchino Del Regno Dec. 22, 2020, 9:11 p.m. UTC | #2
Il 08/12/20 19:11, Rob Herring ha scritto:

Hello! Replying very late seem to be obligatory for me nowadays
so for this and for any other late replies: I'm sorry!

> On Thu, Nov 26, 2020 at 07:45:59PM +0100, AngeloGioacchino Del Regno wrote:
>> The OSM programming addition has been done under the
>> qcom,cpufreq-hw-8998 compatible name: specify the requirement
>> of two additional register spaces for this functionality.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   .../bindings/cpufreq/qcom,cpufreq-hw.yaml     | 31 ++++++++++++++++---
>>   1 file changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
>> index 94a56317b14b..f64cea73037e 100644
>> --- a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
>> +++ b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
>> @@ -23,17 +23,21 @@ properties:
>>             - qcom,cpufreq-epss
>>   
>>     reg:
>> +    description: Base address and size of the RBCPR register region
> 
> That doesn't make sense given you have 2 regions.
> 
>>       minItems: 2
>>       maxItems: 2
> 
> maxItems: 4
> 
Indeed it doesn't make sense.

>>   
>>     reg-names:
>>       description:
>> -      Frequency domain register region for each domain.
>> -    items:
>> -      - const: "freq-domain0"
>> -      - const: "freq-domain1"
>> +      Frequency domain register region for each domain. If OSM programming
>> +      does not happen in the bootloader and has to be done in this driver,
>> +      then also the OSM domain region osm-domain[0-1] has to be provided.
> 
> Don't write free form text for what can be expressed as schema.
> 
I guess the later 'if' for 8998 is sufficient to express that, then...
right?

>> +    minItems: 2
>> +    maxItems: 2
> 
> You obviously haven't tried this change with 8998. It will fail with
> more than 2. What you need here is:
> 
My testing methodology must be flawed. Or perhaps I just need some more 
sleep... probably the latter.

> minItems: 2
> maxItems: 4
> 
> items:
>    - const: "freq-domain0"
>    - const: "freq-domain1"
>    - const: "osm-domain0"
>    - const: "osm-domain1"
> 
> And then...
> 
>>   
>>     clock-names:
>> +    minItems: 2
>> +    maxItems: 2
>>       - const: xo
>>       - const: ref
>>   
>> @@ -53,9 +57,28 @@ properties:
>>         property with phandle to a cpufreq_hw followed by the Domain ID(0/1)
>>         in the CPU DT node.
>>   
>> +allOf:
>> + - if:
>> +     properties:
>> +       reg-names:
>> +         contains:
>> +           const: qcom,cpufreq-hw-8998
>> +   then:
>> +     properties:
>> +       reg:
>> +         minItems: 4
>> +         maxItems: 4
>> +       reg-names:
> 
> ...here just:
> 
> minItems: 4
> 
> And you'll need an 'else' clause with 'maxItems: 2' for reg and
> reg-names.
> 
Big thank you for that!!!

>> +         items:
>> +           - const: "freq-domain0"
>> +           - const: "freq-domain1"
>> +           - const: "osm-domain0"
>> +           - const: "osm-domain1"
>> +
>>   required:
>>     - compatible
>>     - reg
>> +  - reg-names
> 
> You can't make something that was optional now required. (Unless it was
> a mistake and all existing users always had 'reg-names'.)
> 
Well, yes. All existing users are already declaring reg-names, no DT 
changes to do for them.

>>     - clock-names
>>     - clocks
>>     - "#freq-domain-cells"
>> -- 
>> 2.29.2
>>

Thanks for the review.
A V2 of the entire series will come soon!

-- Angelo
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
index 94a56317b14b..f64cea73037e 100644
--- a/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
+++ b/Documentation/devicetree/bindings/cpufreq/qcom,cpufreq-hw.yaml
@@ -23,17 +23,21 @@  properties:
           - qcom,cpufreq-epss
 
   reg:
+    description: Base address and size of the RBCPR register region
     minItems: 2
     maxItems: 2
 
   reg-names:
     description:
-      Frequency domain register region for each domain.
-    items:
-      - const: "freq-domain0"
-      - const: "freq-domain1"
+      Frequency domain register region for each domain. If OSM programming
+      does not happen in the bootloader and has to be done in this driver,
+      then also the OSM domain region osm-domain[0-1] has to be provided.
+    minItems: 2
+    maxItems: 2
 
   clock-names:
+    minItems: 2
+    maxItems: 2
     - const: xo
     - const: ref
 
@@ -53,9 +57,28 @@  properties:
       property with phandle to a cpufreq_hw followed by the Domain ID(0/1)
       in the CPU DT node.
 
+allOf:
+ - if:
+     properties:
+       reg-names:
+         contains:
+           const: qcom,cpufreq-hw-8998
+   then:
+     properties:
+       reg:
+         minItems: 4
+         maxItems: 4
+       reg-names:
+         items:
+           - const: "freq-domain0"
+           - const: "freq-domain1"
+           - const: "osm-domain0"
+           - const: "osm-domain1"
+
 required:
   - compatible
   - reg
+  - reg-names
   - clock-names
   - clocks
   - "#freq-domain-cells"