Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible

[Heinrich Schuchardt]

On 8/25/22 20:56, Conor.Dooley@xxxxxxxxxxxxx wrote:
> On 25/08/2022 19:36, Heinrich Schuchardt wrote:
> > On 8/25/22 20:04, Conor Dooley wrote:
> > > From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> > >
> > > The l2 cache on PolarFire SoC is cross between that of the fu540 and
> > > the fu740. It has the extra interrupt from the fu740 but the lower
> > > number of cache-sets. Add a specific compatible to avoid the likes
> > > of:
> > >
> > > mpfs-polarberry.dtb: cache-controller@2010000: interrupts: [[1], [3], [4], [2]] is too long
> >
> > Where is such a message written? I couldn't find the string in
> > next-20220825 (git grep -n 'is too long"').
>
> dtbs_check on next-20220825 (with dt-schema v22.08 FWIW):
> mpfs-polarberry.dtb: cache-controller@2010000: interrupts: [[1], [3], [4], [2]] is too long
> mpfs-icicle-kit.dtb: cache-controller@2010000: interrupts: [[1], [3], [4], [2]] is too long
>
> I should have caught this before applying, but I got distracted
> by the unusable system.
>
> > Why should a different number of cache sets require an extra
> > compatible string. cache-size is simply a parameter going with> the existing compatible strings.
>
> s/cache sets/interrupts
> Because the correct value for the fu540 is 3 & this is regulated by
> the binding. The alternative would be relaxing the binding to not
> regulate the number of interrupts.
>
> > I would assume that you only need an extra compatible string if
> > there is a functional difference that can not be expressed with
> > the existing parameters.
> >
> > > Fixes: 34fc9cc3aebe ("riscv: dts: microchip: correct L2 cache interrupts")
> > > Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> > > ---
> > >    .../bindings/riscv/sifive-l2-cache.yaml       | 79 ++++++++++++-------
> > >    1 file changed, 49 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
> > > index 69cdab18d629..ca3b9be58058 100644
> > > --- a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
> > > +++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
> > > @@ -17,9 +17,6 @@ description:
> > >      acts as directory-based coherency manager.
> > >      All the properties in ePAPR/DeviceTree specification applies for this platform.
> > >    -allOf:
> > > -  - $ref: /schemas/cache-controller.yaml#
> > > -
> > >    select:
> > >      properties:
> > >        compatible:
> > > @@ -33,11 +30,16 @@ select:
> > >      properties:
> > >      compatible:
> > > -    items:
> > > -      - enum:
> > > -          - sifive,fu540-c000-ccache
> > > -          - sifive,fu740-c000-ccache
> >
> > Why can't you simply add microchip,mpfs-ccache here?
>
> I *could* but I opted not to because the fu540 supports a compatible
> subset of the features & keeping the compatible for it allows systems
> with a newer dts to work with an older kernel.

That makes it clearer.

> > > -      - const: cache
> > > +    oneOf:
> > > +      - items:
> > > +          - enum:
> > > +              - sifive,fu540-c000-ccache
> > > +              - sifive,fu740-c000-ccache
> > > +          - const: cache
> > > +      - items:
> > > +          - const: microchip,mpfs-ccache
> > > +          - const: sifive,fu540-c000-ccache
> >
> > Why do we need 'sifive,fu540-c000-ccache' twice?
>
> Is there a better way to write it given the above caveat?
>
> Thanks,
> Conor.
>
>
> > > +          - const: cache
> > >        cache-block-size:
> > >        const: 64
> > > @@ -72,29 +74,46 @@ properties:
> > >          The reference to the reserved-memory for the L2 Loosely Integrated Memory region.
> > >          The reserved memory node should be defined as per the bindings in reserved-memory.txt.
> > >    -if:
> > > -  properties:
> > > -    compatible:
> > > -      contains:
> > > -        const: sifive,fu540-c000-ccache
> > > +allOf:
> > > +  - $ref: /schemas/cache-controller.yaml#
> > >    -then:
> > > -  properties:
> > > -    interrupts:
> > > -      description: |
> > > -        Must contain entries for DirError, DataError and DataFail signals.
> > > -      maxItems: 3
> > > -    cache-sets:
> > > -      const: 1024
> > > -
> > > -else:
> > > -  properties:
> > > -    interrupts:
> > > -      description: |
> > > -        Must contain entries for DirError, DataError, DataFail, DirFail signals.
> > > -      minItems: 4
> > > -    cache-sets:
> > > -      const: 2048
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            enum:
> > > +              - sifive,fu740-c000-ccache
> > > +              - microchip,mpfs-ccache
> > > +
> > > +    then:
> > > +      properties:
> > > +        interrupts:
> > > +          description: |
> > > +            Must contain entries for DirError, DataError, DataFail, DirFail signals.
> > > +          minItems: 4

Above you indicated that you want strict limits for the interrupt count. 
You expect exactly 4 items here. Having 5 entries would not be correct.
Please, add 'maxItems: 4'.

> > > +
> > > +    else:
> > > +      properties:
> > > +        interrupts:
> > > +          description: |
> > > +            Must contain entries for DirError, DataError and DataFail signals.
> > > +          maxItems: 3

The item count should be exactly 3. Having 2 entries would not be correct.
Please, add 'minItems: 3'.

Best regards

Heinrich

> > > +
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: sifive,fu740-c000-ccache
> > > +
> > > +    then:
> > > +      properties:
> > > +        cache-sets:
> > > +          const: 2048
> > > +
> > > +    else:
> > > +      properties:
> > > +        cache-sets:
> > > +          const: 1024
> > >      additionalProperties: false
> > >