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

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 8/25/22 20:04, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> 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"').

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.

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@microchip.com>
> ---
>   .../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?

> -      - 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?

Best regards

Heinrich

> +          - 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
> +
> +    else:
> +      properties:
> +        interrupts:
> +          description: |
> +            Must contain entries for DirError, DataError and DataFail signals.
> +          maxItems: 3
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: sifive,fu740-c000-ccache
> +
> +    then:
> +      properties:
> +        cache-sets:
> +          const: 2048
> +
> +    else:
> +      properties:
> +        cache-sets:
> +          const: 1024
>   
>   additionalProperties: false
>