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

[AngeloGioacchino Del Regno]

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@xxxxxxxxxxxxxx>
> > ---
> >   .../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