diff mbox series

[v3,1/2] dt-bindings: sifive: add cache-set value of 2048

Message ID 20220726170725.3245278-2-mail@conchuod.ie
State New
Headers show
Series Fix SiFive dt-schema errors | expand

Commit Message

Conor Dooley July 26, 2022, 5:07 p.m. UTC
From: Atul Khare <atulkhare@rivosinc.com>

Fixes Running device tree schema validation error messages like
'... cache-sets:0:0: 1024 was expected'.

The existing bindings had a single enumerated value of 1024, which
trips up the dt-schema checks. The ISA permits any arbitrary power
of two for the cache-sets value, but we decided to add the single
additional value of 2048 because we couldn't spot an obvious way
to express the constraint in the schema.

Signed-off-by: Atul Khare <atulkhare@rivosinc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Krzysztof Kozlowski July 26, 2022, 5:35 p.m. UTC | #1
On 26/07/2022 19:07, Conor Dooley wrote:
> From: Atul Khare <atulkhare@rivosinc.com>
> 
> Fixes Running device tree schema validation error messages like
> '... cache-sets:0:0: 1024 was expected'.
> 
> The existing bindings had a single enumerated value of 1024, which
> trips up the dt-schema checks. The ISA permits any arbitrary power
> of two for the cache-sets value, but we decided to add the single
> additional value of 2048 because we couldn't spot an obvious way
> to express the constraint in the schema.

There is no way to express "power of two" but enum for multiple values
would work. Is there a reason to limit only to 2048?

Best regards,
Krzysztof
Krzysztof Kozlowski July 27, 2022, 7:13 a.m. UTC | #2
On 26/07/2022 19:39, Conor.Dooley@microchip.com wrote:
> 
> 
> On 26/07/2022 18:35, Krzysztof Kozlowski wrote:
>> On 26/07/2022 19:07, Conor Dooley wrote:
>>> From: Atul Khare <atulkhare@rivosinc.com>
>>>
>>> Fixes Running device tree schema validation error messages like
>>> '... cache-sets:0:0: 1024 was expected'.
>>>
>>> The existing bindings had a single enumerated value of 1024, which
>>> trips up the dt-schema checks. The ISA permits any arbitrary power
>>> of two for the cache-sets value, but we decided to add the single
>>> additional value of 2048 because we couldn't spot an obvious way
>>> to express the constraint in the schema.
>>
>> There is no way to express "power of two" but enum for multiple values
>> would work. Is there a reason to limit only to 2048?
> 
> Copy pasting from the cover:
>> I don't think that there's value in speculatively adding values to this
>> enum especially as (I think at least) the scala for this cache IP has
>> been released publicly:
>> https://github.com/sifive/block-inclusivecache-sifive/blob/master/design/craft/inclusivecache/src/Parameters.scala#L32
>>
>> The two compatibles in the file match only against two specific cache
>> implemenations: the fu540's & the fu740's. I would seem to me that, it
>> would be better to lock this to a single value per compatible since the
>> 1024 applies to the fu540 & the new value of 2048 applies only to the
>> fu740.
>>
>> I have not made that change, I simply wanted to repackage this series
>> in a way that could be more easily applied & restart the discussion.
> 
> TL;DR: I would limit it to 1024 & 2048 to match the only implementations
> although not in the way this patch did it.

The explanation in cover letter is good, but it would be good to have
one sentence like this in the commit msg. Otherwise your commit is
actually confusing - you mention that you want power of two and then set
only 1k + 2k.


Best regards,
Krzysztof
Conor Dooley July 27, 2022, 7:19 a.m. UTC | #3
On 27/07/2022 08:13, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 26/07/2022 19:39, Conor.Dooley@microchip.com wrote:
>>
>>
>> On 26/07/2022 18:35, Krzysztof Kozlowski wrote:
>>> On 26/07/2022 19:07, Conor Dooley wrote:
>>>> From: Atul Khare <atulkhare@rivosinc.com>
>>>>
>>>> Fixes Running device tree schema validation error messages like
>>>> '... cache-sets:0:0: 1024 was expected'.
>>>>
>>>> The existing bindings had a single enumerated value of 1024, which
>>>> trips up the dt-schema checks. The ISA permits any arbitrary power
>>>> of two for the cache-sets value, but we decided to add the single
>>>> additional value of 2048 because we couldn't spot an obvious way
>>>> to express the constraint in the schema.
>>>
>>> There is no way to express "power of two" but enum for multiple values
>>> would work. Is there a reason to limit only to 2048?
>>
>> Copy pasting from the cover:
>>> I don't think that there's value in speculatively adding values to this
>>> enum especially as (I think at least) the scala for this cache IP has
>>> been released publicly:
>>> https://github.com/sifive/block-inclusivecache-sifive/blob/master/design/craft/inclusivecache/src/Parameters.scala#L32
>>>
>>> The two compatibles in the file match only against two specific cache
>>> implemenations: the fu540's & the fu740's. I would seem to me that, it
>>> would be better to lock this to a single value per compatible since the
>>> 1024 applies to the fu540 & the new value of 2048 applies only to the
>>> fu740.
>>>
>>> I have not made that change, I simply wanted to repackage this series
>>> in a way that could be more easily applied & restart the discussion.
>>
>> TL;DR: I would limit it to 1024 & 2048 to match the only implementations
>> although not in the way this patch did it.
> 
> The explanation in cover letter is good, but it would be good to have
> one sentence like this in the commit msg. Otherwise your commit is
> actually confusing - you mention that you want power of two and then set
> only 1k + 2k.

Yeah, I just took the commits from existing patchset as they were.
I'll rewrite the commit for the next time.
Thanks,
Conor.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
index e2d330bd4608..ab6043d9cdbe 100644
--- a/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
+++ b/Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml
@@ -46,7 +46,7 @@  properties:
     const: 2
 
   cache-sets:
-    const: 1024
+    enum: [1024, 2048]
 
   cache-size:
     const: 2097152