All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Fix SiFive dt-schema errors
@ 2022-07-26 17:07 ` Conor Dooley
  0 siblings, 0 replies; 16+ messages in thread
From: Conor Dooley @ 2022-07-26 17:07 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

From: Conor Dooley <conor.dooley@microchip.com>

Hi all,
As these are the last remaining dtbs_check warnings for the default
riscv defconfig I am reviving the series. Atul has been hit by some
hardware issues so has not had a chance to send an applicable version
of these patches. All I have done here is rebase on linux-next, so by
the time the merge window reopens it will hopefully apply..

On Atul's V2, Rob had a comment about changing the cache-sets in the
patch 1/2:
There is not any way to express power of 2, so you have to list values.
Rather than just adding 1 more value, I would add at least a few more so
we're not adding these one by one. This is for a specific cache
implementation, so it can't really be *any* power of 2. Designs have
some limits or physics does. /endquote

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.

Thanks,
Conor.

Atul Khare (2):
  dt-bindings: sifive: add cache-set value of 2048
  dt-bindings: sifive: add gpio-line-names

 Documentation/devicetree/bindings/gpio/sifive,gpio.yaml      | 4 ++++
 Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)


base-commit: 058affafc65a74cf54499fb578b66ad0b18f939b
-- 
2.37.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 0/2] Fix SiFive dt-schema errors
@ 2022-07-26 17:07 ` Conor Dooley
  0 siblings, 0 replies; 16+ messages in thread
From: Conor Dooley @ 2022-07-26 17:07 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

From: Conor Dooley <conor.dooley@microchip.com>

Hi all,
As these are the last remaining dtbs_check warnings for the default
riscv defconfig I am reviving the series. Atul has been hit by some
hardware issues so has not had a chance to send an applicable version
of these patches. All I have done here is rebase on linux-next, so by
the time the merge window reopens it will hopefully apply..

On Atul's V2, Rob had a comment about changing the cache-sets in the
patch 1/2:
There is not any way to express power of 2, so you have to list values.
Rather than just adding 1 more value, I would add at least a few more so
we're not adding these one by one. This is for a specific cache
implementation, so it can't really be *any* power of 2. Designs have
some limits or physics does. /endquote

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.

Thanks,
Conor.

Atul Khare (2):
  dt-bindings: sifive: add cache-set value of 2048
  dt-bindings: sifive: add gpio-line-names

 Documentation/devicetree/bindings/gpio/sifive,gpio.yaml      | 4 ++++
 Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)


base-commit: 058affafc65a74cf54499fb578b66ad0b18f939b
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
  2022-07-26 17:07 ` Conor Dooley
@ 2022-07-26 17:07   ` Conor Dooley
  -1 siblings, 0 replies; 16+ messages in thread
From: Conor Dooley @ 2022-07-26 17:07 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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(-)

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
@ 2022-07-26 17:07   ` Conor Dooley
  0 siblings, 0 replies; 16+ messages in thread
From: Conor Dooley @ 2022-07-26 17:07 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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(-)

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 2/2] dt-bindings: sifive: add gpio-line-names
  2022-07-26 17:07 ` Conor Dooley
@ 2022-07-26 17:07   ` Conor Dooley
  -1 siblings, 0 replies; 16+ messages in thread
From: Conor Dooley @ 2022-07-26 17:07 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

From: Atul Khare <atulkhare@rivosinc.com>

Fix device tree schema validation messages like 'gpio-line-names'
does not match any of the regexes: 'pinctrl-[0-9]+' From schema: ...
sifive,gpio.yaml'.

The bindings were missing the gpio-line-names element, which was
causing the dt-schema checker to trip-up.

Signed-off-by: Atul Khare <atulkhare@rivosinc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/gpio/sifive,gpio.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
index 939e31c48081..fc095646adea 100644
--- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
@@ -46,6 +46,10 @@ properties:
     maximum: 32
     default: 16
 
+  gpio-line-names:
+    minItems: 1
+    maxItems: 32
+
   gpio-controller: true
 
 required:
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 2/2] dt-bindings: sifive: add gpio-line-names
@ 2022-07-26 17:07   ` Conor Dooley
  0 siblings, 0 replies; 16+ messages in thread
From: Conor Dooley @ 2022-07-26 17:07 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

From: Atul Khare <atulkhare@rivosinc.com>

Fix device tree schema validation messages like 'gpio-line-names'
does not match any of the regexes: 'pinctrl-[0-9]+' From schema: ...
sifive,gpio.yaml'.

The bindings were missing the gpio-line-names element, which was
causing the dt-schema checker to trip-up.

Signed-off-by: Atul Khare <atulkhare@rivosinc.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/gpio/sifive,gpio.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
index 939e31c48081..fc095646adea 100644
--- a/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/sifive,gpio.yaml
@@ -46,6 +46,10 @@ properties:
     maximum: 32
     default: 16
 
+  gpio-line-names:
+    minItems: 1
+    maxItems: 32
+
   gpio-controller: true
 
 required:
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
  2022-07-26 17:07   ` Conor Dooley
@ 2022-07-26 17:35     ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 16+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-26 17:35 UTC (permalink / raw)
  To: Conor Dooley, Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
@ 2022-07-26 17:35     ` Krzysztof Kozlowski
  0 siblings, 0 replies; 16+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-26 17:35 UTC (permalink / raw)
  To: Conor Dooley, Linus Walleij, Bartosz Golaszewski, Rob Herring,
	Krzysztof Kozlowski, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Atul Khare, Sagar Kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
  2022-07-26 17:35     ` Krzysztof Kozlowski
@ 2022-07-26 17:39       ` Conor.Dooley
  -1 siblings, 0 replies; 16+ messages in thread
From: Conor.Dooley @ 2022-07-26 17:39 UTC (permalink / raw)
  To: krzysztof.kozlowski, linus.walleij, brgl, robh+dt,
	krzysztof.kozlowski+dt, palmer, paul.walmsley, aou, Conor.Dooley,
	atulkhare, sagar.kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel



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.

Hope that helps,
Conor.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
@ 2022-07-26 17:39       ` Conor.Dooley
  0 siblings, 0 replies; 16+ messages in thread
From: Conor.Dooley @ 2022-07-26 17:39 UTC (permalink / raw)
  To: krzysztof.kozlowski, linus.walleij, brgl, robh+dt,
	krzysztof.kozlowski+dt, palmer, paul.walmsley, aou, Conor.Dooley,
	atulkhare, sagar.kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel



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.

Hope that helps,
Conor.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
  2022-07-26 17:39       ` Conor.Dooley
@ 2022-07-27  7:13         ` Krzysztof Kozlowski
  -1 siblings, 0 replies; 16+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-27  7:13 UTC (permalink / raw)
  To: Conor.Dooley, linus.walleij, brgl, robh+dt,
	krzysztof.kozlowski+dt, palmer, paul.walmsley, aou, atulkhare,
	sagar.kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
@ 2022-07-27  7:13         ` Krzysztof Kozlowski
  0 siblings, 0 replies; 16+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-27  7:13 UTC (permalink / raw)
  To: Conor.Dooley, linus.walleij, brgl, robh+dt,
	krzysztof.kozlowski+dt, palmer, paul.walmsley, aou, atulkhare,
	sagar.kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
  2022-07-27  7:13         ` Krzysztof Kozlowski
@ 2022-07-27  7:19           ` Conor.Dooley
  -1 siblings, 0 replies; 16+ messages in thread
From: Conor.Dooley @ 2022-07-27  7:19 UTC (permalink / raw)
  To: krzysztof.kozlowski, linus.walleij, brgl, robh+dt,
	krzysztof.kozlowski+dt, palmer, paul.walmsley, aou, atulkhare,
	sagar.kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048
@ 2022-07-27  7:19           ` Conor.Dooley
  0 siblings, 0 replies; 16+ messages in thread
From: Conor.Dooley @ 2022-07-27  7:19 UTC (permalink / raw)
  To: krzysztof.kozlowski, linus.walleij, brgl, robh+dt,
	krzysztof.kozlowski+dt, palmer, paul.walmsley, aou, atulkhare,
	sagar.kadam
  Cc: linux-gpio, devicetree, linux-riscv, linux-kernel

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] dt-bindings: sifive: add gpio-line-names
  2022-07-26 17:07   ` Conor Dooley
@ 2022-07-27 15:23     ` Rob Herring
  -1 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2022-07-27 15:23 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Atul Khare, Albert Ou, Conor Dooley, Bartosz Golaszewski,
	Linus Walleij, Rob Herring, linux-gpio, linux-kernel,
	Krzysztof Kozlowski, Sagar Kadam, Palmer Dabbelt, Paul Walmsley,
	linux-riscv, devicetree

On Tue, 26 Jul 2022 18:07:26 +0100, Conor Dooley wrote:
> From: Atul Khare <atulkhare@rivosinc.com>
> 
> Fix device tree schema validation messages like 'gpio-line-names'
> does not match any of the regexes: 'pinctrl-[0-9]+' From schema: ...
> sifive,gpio.yaml'.
> 
> The bindings were missing the gpio-line-names element, which was
> causing the dt-schema checker to trip-up.
> 
> Signed-off-by: Atul Khare <atulkhare@rivosinc.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  Documentation/devicetree/bindings/gpio/sifive,gpio.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 2/2] dt-bindings: sifive: add gpio-line-names
@ 2022-07-27 15:23     ` Rob Herring
  0 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2022-07-27 15:23 UTC (permalink / raw)
  To: Conor Dooley
  Cc: Atul Khare, Albert Ou, Conor Dooley, Bartosz Golaszewski,
	Linus Walleij, Rob Herring, linux-gpio, linux-kernel,
	Krzysztof Kozlowski, Sagar Kadam, Palmer Dabbelt, Paul Walmsley,
	linux-riscv, devicetree

On Tue, 26 Jul 2022 18:07:26 +0100, Conor Dooley wrote:
> From: Atul Khare <atulkhare@rivosinc.com>
> 
> Fix device tree schema validation messages like 'gpio-line-names'
> does not match any of the regexes: 'pinctrl-[0-9]+' From schema: ...
> sifive,gpio.yaml'.
> 
> The bindings were missing the gpio-line-names element, which was
> causing the dt-schema checker to trip-up.
> 
> Signed-off-by: Atul Khare <atulkhare@rivosinc.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  Documentation/devicetree/bindings/gpio/sifive,gpio.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2022-07-27 15:23 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-26 17:07 [PATCH v3 0/2] Fix SiFive dt-schema errors Conor Dooley
2022-07-26 17:07 ` Conor Dooley
2022-07-26 17:07 ` [PATCH v3 1/2] dt-bindings: sifive: add cache-set value of 2048 Conor Dooley
2022-07-26 17:07   ` Conor Dooley
2022-07-26 17:35   ` Krzysztof Kozlowski
2022-07-26 17:35     ` Krzysztof Kozlowski
2022-07-26 17:39     ` Conor.Dooley
2022-07-26 17:39       ` Conor.Dooley
2022-07-27  7:13       ` Krzysztof Kozlowski
2022-07-27  7:13         ` Krzysztof Kozlowski
2022-07-27  7:19         ` Conor.Dooley
2022-07-27  7:19           ` Conor.Dooley
2022-07-26 17:07 ` [PATCH v3 2/2] dt-bindings: sifive: add gpio-line-names Conor Dooley
2022-07-26 17:07   ` Conor Dooley
2022-07-27 15:23   ` Rob Herring
2022-07-27 15:23     ` Rob Herring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.