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

[<Conor.Dooley@microchip.com>]

On 25/08/2022 19:36, Heinrich Schuchardt wrote:
> 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"').

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

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.

> 
>> -      - 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
>> +
>> +    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
>>   
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv