lkml.org 
[lkml]   [2020]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 13/13] dt-bindings: cpufreq: qcom-hw: Add bindings for 8998
From
Date
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

\
 
 \ /
  Last update: 2020-12-22 22:13    [W:1.048 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site