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.
