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

[Conor.Dooley]

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@xxxxxxxxxxxxx wrote:
>>
>>
>> On 26/07/2022 18:35, Krzysztof Kozlowski wrote:
>>> On 26/07/2022 19:07, Conor Dooley wrote:
>>>> From: Atul Khare <atulkhare@xxxxxxxxxxxx>
>>>>
>>>> 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.