* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
@ 2022-08-25 18:36 ` Heinrich Schuchardt
0 siblings, 0 replies; 22+ messages in thread
From: Heinrich Schuchardt @ 2022-08-25 18:36 UTC (permalink / raw)
To: Conor Dooley
Cc: Sagar Kadam, Atish Patra, Paul Walmsley, Krzysztof Kozlowski,
devicetree, linux-riscv, linux-kernel, Albert Ou, Daire McNamara,
Palmer Dabbelt, Rob Herring, Conor Dooley
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
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
2022-08-25 18:36 ` Heinrich Schuchardt
@ 2022-08-25 18:56 ` Conor.Dooley
-1 siblings, 0 replies; 22+ messages in thread
From: Conor.Dooley @ 2022-08-25 18:56 UTC (permalink / raw)
To: heinrich.schuchardt, Conor.Dooley
Cc: sagar.kadam, atishp, paul.walmsley, krzysztof.kozlowski+dt,
devicetree, linux-riscv, linux-kernel, aou, Daire.McNamara,
palmer, robh+dt
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
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
@ 2022-08-25 18:56 ` Conor.Dooley
0 siblings, 0 replies; 22+ messages in thread
From: Conor.Dooley @ 2022-08-25 18:56 UTC (permalink / raw)
To: heinrich.schuchardt, Conor.Dooley
Cc: sagar.kadam, atishp, paul.walmsley, krzysztof.kozlowski+dt,
devicetree, linux-riscv, linux-kernel, aou, Daire.McNamara,
palmer, robh+dt
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
2022-08-25 18:56 ` Conor.Dooley
@ 2022-08-25 19:49 ` Heinrich Schuchardt
-1 siblings, 0 replies; 22+ messages in thread
From: Heinrich Schuchardt @ 2022-08-25 19:49 UTC (permalink / raw)
To: Conor.Dooley
Cc: sagar.kadam, atishp, paul.walmsley, krzysztof.kozlowski+dt,
devicetree, linux-riscv, linux-kernel, aou, Daire.McNamara,
palmer, robh+dt
On 8/25/22 20:56, Conor.Dooley@microchip.com wrote:
> 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.
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
>>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
@ 2022-08-25 19:49 ` Heinrich Schuchardt
0 siblings, 0 replies; 22+ messages in thread
From: Heinrich Schuchardt @ 2022-08-25 19:49 UTC (permalink / raw)
To: Conor.Dooley
Cc: sagar.kadam, atishp, paul.walmsley, krzysztof.kozlowski+dt,
devicetree, linux-riscv, linux-kernel, aou, Daire.McNamara,
palmer, robh+dt
On 8/25/22 20:56, Conor.Dooley@microchip.com wrote:
> 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.
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
>>>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
2022-08-25 19:49 ` Heinrich Schuchardt
@ 2022-08-25 20:03 ` Conor.Dooley
-1 siblings, 0 replies; 22+ messages in thread
From: Conor.Dooley @ 2022-08-25 20:03 UTC (permalink / raw)
To: heinrich.schuchardt, Conor.Dooley
Cc: sagar.kadam, atishp, paul.walmsley, krzysztof.kozlowski+dt,
devicetree, linux-riscv, linux-kernel, aou, Daire.McNamara,
palmer, robh+dt
On 25/08/2022 20:49, Heinrich Schuchardt wrote:
> On 8/25/22 20:56, Conor.Dooley@microchip.com wrote:
>> 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>
>>>> +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'.
Outside of this diff, because of how the particular binding was
structured, there is:
interrupts:
minItems: 3
items:
- description: DirError interrupt
- description: DataError interrupt
- description: DataFail interrupt
- description: DirFail interrupt
AFAIU, "maxItems: 4" is redundant because all possible items are listed.
>
>>>> +
>>>> + 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'.
Again, this is set by the section I pasted above - although this time
explicitly.
Hope that explains things, not the easiest binding to understand from
a diff alone. Possibly I should have passed a "-U" argument while
creating the patches to get an easier-to-follow diff.
Thanks for your (prompt) reviews,
Conor.
>>>> +
>>>> + - if:
>>>> + properties:
>>>> + compatible:
>>>> + contains:
>>>> + const: sifive,fu740-c000-ccache
>>>> +
>>>> + then:
>>>> + properties:
>>>> + cache-sets:
>>>> + const: 2048
>>>> +
>>>> + else:
>>>> + properties:
>>>> + cache-sets:
>>>> + const: 1024
>>>> additionalProperties: false
>>>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
@ 2022-08-25 20:03 ` Conor.Dooley
0 siblings, 0 replies; 22+ messages in thread
From: Conor.Dooley @ 2022-08-25 20:03 UTC (permalink / raw)
To: heinrich.schuchardt, Conor.Dooley
Cc: sagar.kadam, atishp, paul.walmsley, krzysztof.kozlowski+dt,
devicetree, linux-riscv, linux-kernel, aou, Daire.McNamara,
palmer, robh+dt
On 25/08/2022 20:49, Heinrich Schuchardt wrote:
> On 8/25/22 20:56, Conor.Dooley@microchip.com wrote:
>> 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>
>>>> +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'.
Outside of this diff, because of how the particular binding was
structured, there is:
interrupts:
minItems: 3
items:
- description: DirError interrupt
- description: DataError interrupt
- description: DataFail interrupt
- description: DirFail interrupt
AFAIU, "maxItems: 4" is redundant because all possible items are listed.
>
>>>> +
>>>> + 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'.
Again, this is set by the section I pasted above - although this time
explicitly.
Hope that explains things, not the easiest binding to understand from
a diff alone. Possibly I should have passed a "-U" argument while
creating the patches to get an easier-to-follow diff.
Thanks for your (prompt) reviews,
Conor.
>>>> +
>>>> + - 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
2022-08-25 18:36 ` Heinrich Schuchardt
@ 2022-08-30 20:57 ` Rob Herring
-1 siblings, 0 replies; 22+ messages in thread
From: Rob Herring @ 2022-08-30 20:57 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Conor Dooley, Sagar Kadam, Atish Patra, Paul Walmsley,
Krzysztof Kozlowski, devicetree, linux-riscv, linux-kernel,
Albert Ou, Daire McNamara, Palmer Dabbelt, Conor Dooley
On Thu, Aug 25, 2022 at 1:36 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> 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"').
>
> 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.
Correct, but you have to account for unknown functional differences
aka errata as well. Otherwise, we need firmware updates to enable the
OS to handle errata.
> > 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?
Because it is in 2 different positions. While we can express that the
last N entries in a list are optional, there is no way in json-schema
to express entries at the beginning or in the middle are optional.
Rob
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] dt-bindings: riscv: sifive-l2: add a PolarFire SoC compatible
@ 2022-08-30 20:57 ` Rob Herring
0 siblings, 0 replies; 22+ messages in thread
From: Rob Herring @ 2022-08-30 20:57 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Conor Dooley, Sagar Kadam, Atish Patra, Paul Walmsley,
Krzysztof Kozlowski, devicetree, linux-riscv, linux-kernel,
Albert Ou, Daire McNamara, Palmer Dabbelt, Conor Dooley
On Thu, Aug 25, 2022 at 1:36 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> 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"').
>
> 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.
Correct, but you have to account for unknown functional differences
aka errata as well. Otherwise, we need firmware updates to enable the
OS to handle errata.
> > 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?
Because it is in 2 different positions. While we can express that the
last N entries in a list are optional, there is no way in json-schema
to express entries at the beginning or in the middle are optional.
Rob
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread