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