[PATCH] dt-bindings: riscv: Document cbop-block-size

[PATCH] dt-bindings: riscv: Document cbop-block-size

[Daniel Henrique Barboza]

Following the examples of cbom-block-size and cboz-block-size,
cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
The most common case is to have all cache block sizes to be the same
size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
cache operations), but there's no specification requirement for that,
and an implementation can have different cache sizes for each operation.

Cc: Rob Herring <robh@kernel.org>
Cc: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index 97e8441eda1c..1660b296f7de 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -78,6 +78,11 @@ properties:
     description:
       The blocksize in bytes for the Zicbom cache operations.
 
+  riscv,cbop-block-size:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The blocksize in bytes for the Zicbop cache operations.
+
   riscv,cboz-block-size:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:
-- 
2.41.0

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

Yo,

On Sun, Oct 29, 2023 at 09:35:00AM -0300, Daniel Henrique Barboza wrote:
> Following the examples of cbom-block-size and cboz-block-size,
> cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
> The most common case is to have all cache block sizes to be the same
> size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
> cache operations), but there's no specification requirement for that,
> and an implementation can have different cache sizes for each operation.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Firstly, odd CC list. Please CC the output of get_maintainer.pl in the
future.

IIRC, I mentioned defining this to Drew when he was add zicboz, but he
didn't want to add it - although he seems to have asked you to document
this. Drew, change of heart or am I not remembering correctly?
I think he cited some interpretation of the spec from Andrei W that
implied the Zicbop size would be the same as one of the other ones, but
I cannot find that on lore atm.

If Drew's okay with it, then I am too, so a conditional
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index 97e8441eda1c..1660b296f7de 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -78,6 +78,11 @@ properties:
>      description:
>        The blocksize in bytes for the Zicbom cache operations.
>  
> +  riscv,cbop-block-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The blocksize in bytes for the Zicbop cache operations.
> +
>    riscv,cboz-block-size:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
> -- 
> 2.41.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[Daniel Henrique Barboza]

On 10/29/23 11:53, Conor Dooley wrote:
> Yo,
> 
> On Sun, Oct 29, 2023 at 09:35:00AM -0300, Daniel Henrique Barboza wrote:
>> Following the examples of cbom-block-size and cboz-block-size,
>> cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
>> The most common case is to have all cache block sizes to be the same
>> size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
>> cache operations), but there's no specification requirement for that,
>> and an implementation can have different cache sizes for each operation.
>>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: Conor Dooley <conor.dooley@microchip.com>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> 
> Firstly, odd CC list. Please CC the output of get_maintainer.pl in the
> future.

Ops, my bad

> 
> IIRC, I mentioned defining this to Drew when he was add zicboz, but he
> didn't want to add it - although he seems to have asked you to document
> this. Drew, change of heart or am I not remembering correctly?
> I think he cited some interpretation of the spec from Andrei W that
> implied the Zicbop size would be the same as one of the other ones, but
> I cannot find that on lore atm.

The reason why I'm here is because I want to add Zicbop in QEMU riscv,isa.
I'm pushing a rva22u64 profile implementation there and Zicbop is mandatory
for it. In the process I added a riscv,cbop-block-size DT because, well,
if both Zicboz and Zicbom have their respective block-size DTs, then it's
expected that Zicbop also has one. Or so I thought.

Drew then replied in the QEMU ML [1] that riscv,cbop-block-size isn't
documented and we can't add it as it is. So here we are.

If riscv,cbop-block-size isn't needed because Zicbop will use the cache
block size of Zicboz or Zicbom, that works for me too - I'll add a note
in QEMU explaining why there's no riscv,cbop-block-size and everything
is fine. What we can't do is add stuff in the QEMU DT that's neither
documented nor acked in the DT bindings.


Thanks,


Daniel

[1] https://lore.kernel.org/qemu-riscv/20231028-2d6bf00dddc7bc4a25b32663@orel/

> 
> If Drew's okay with it, then I am too, so a conditional
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Cheers,
> Conor.
> 
>> ---
>>   Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
>> index 97e8441eda1c..1660b296f7de 100644
>> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
>> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
>> @@ -78,6 +78,11 @@ properties:
>>       description:
>>         The blocksize in bytes for the Zicbom cache operations.
>>   
>> +  riscv,cbop-block-size:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      The blocksize in bytes for the Zicbop cache operations.
>> +
>>     riscv,cboz-block-size:
>>       $ref: /schemas/types.yaml#/definitions/uint32
>>       description:
>> -- 
>> 2.41.0
>>
>>

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]

On Sun, Oct 29, 2023 at 04:49:30PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 10/29/23 11:53, Conor Dooley wrote:
> > Yo,
> > 
> > On Sun, Oct 29, 2023 at 09:35:00AM -0300, Daniel Henrique Barboza wrote:
> > > Following the examples of cbom-block-size and cboz-block-size,
> > > cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
> > > The most common case is to have all cache block sizes to be the same
> > > size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
> > > cache operations), but there's no specification requirement for that,
> > > and an implementation can have different cache sizes for each operation.
> > > 
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Conor Dooley <conor.dooley@microchip.com>
> > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > 
> > Firstly, odd CC list. Please CC the output of get_maintainer.pl in the
> > future.
> 
> Ops, my bad
> 
> > 
> > IIRC, I mentioned defining this to Drew when he was add zicboz, but he
> > didn't want to add it - although he seems to have asked you to document
> > this. Drew, change of heart or am I not remembering correctly?
> > I think he cited some interpretation of the spec from Andrei W that
> > implied the Zicbop size would be the same as one of the other ones, but
> > I cannot find that on lore atm.
> 
> The reason why I'm here is because I want to add Zicbop in QEMU riscv,isa.
> I'm pushing a rva22u64 profile implementation there and Zicbop is mandatory
> for it. In the process I added a riscv,cbop-block-size DT because, well,
> if both Zicboz and Zicbom have their respective block-size DTs, then it's
> expected that Zicbop also has one. Or so I thought.
> 
> Drew then replied in the QEMU ML [1] that riscv,cbop-block-size isn't
> documented and we can't add it as it is. So here we are.

Yeah, I did read that.

> If riscv,cbop-block-size isn't needed because Zicbop will use the cache
> block size of Zicboz or Zicbom, that works for me too - I'll add a note
> in QEMU explaining why there's no riscv,cbop-block-size and everything
> is fine.

I just wanted to remind Drew why we didn't add this in the first place,
given I had seen that he suggested that you add it in the QEMU thread.
And in the hopes that he would be able to dig the link back up to
Andrei's comments, given I wasn't able to find it/couldnt remember
recall where it had come from.

> What we can't do is add stuff in the QEMU DT that's neither
> documented nor acked in the DT bindings.

That's a welcome change.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[Andrew Jones]

On Sun, Oct 29, 2023 at 10:21:55PM +0000, Conor Dooley wrote:
> On Sun, Oct 29, 2023 at 04:49:30PM -0300, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 10/29/23 11:53, Conor Dooley wrote:
> > > Yo,
> > > 
> > > On Sun, Oct 29, 2023 at 09:35:00AM -0300, Daniel Henrique Barboza wrote:
> > > > Following the examples of cbom-block-size and cboz-block-size,
> > > > cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
> > > > The most common case is to have all cache block sizes to be the same
> > > > size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
> > > > cache operations), but there's no specification requirement for that,
> > > > and an implementation can have different cache sizes for each operation.
> > > > 
> > > > Cc: Rob Herring <robh@kernel.org>
> > > > Cc: Conor Dooley <conor.dooley@microchip.com>
> > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > > 
> > > Firstly, odd CC list. Please CC the output of get_maintainer.pl in the
> > > future.
> > 
> > Ops, my bad
> > 
> > > 
> > > IIRC, I mentioned defining this to Drew when he was add zicboz, but he
> > > didn't want to add it - although he seems to have asked you to document
> > > this. Drew, change of heart or am I not remembering correctly?
> > > I think he cited some interpretation of the spec from Andrei W that
> > > implied the Zicbop size would be the same as one of the other ones, but
> > > I cannot find that on lore atm.
> > 
> > The reason why I'm here is because I want to add Zicbop in QEMU riscv,isa.
> > I'm pushing a rva22u64 profile implementation there and Zicbop is mandatory
> > for it. In the process I added a riscv,cbop-block-size DT because, well,
> > if both Zicboz and Zicbom have their respective block-size DTs, then it's
> > expected that Zicbop also has one. Or so I thought.
> > 
> > Drew then replied in the QEMU ML [1] that riscv,cbop-block-size isn't
> > documented and we can't add it as it is. So here we are.
> 
> Yeah, I did read that.
> 
> > If riscv,cbop-block-size isn't needed because Zicbop will use the cache
> > block size of Zicboz or Zicbom, that works for me too - I'll add a note
> > in QEMU explaining why there's no riscv,cbop-block-size and everything
> > is fine.
> 
> I just wanted to remind Drew why we didn't add this in the first place,
> given I had seen that he suggested that you add it in the QEMU thread.
> And in the hopes that he would be able to dig the link back up to
> Andrei's comments, given I wasn't able to find it/couldnt remember
> recall where it had come from.

Hi Conor,

Thanks for the reminder. I had forgotten my own opinion on this :-)

I found the messages. In [1], I advocate for the block size DT property,
but then, in [2], I reply to myself saying we could probably wait until
we have a prefetch block size that differs from the management block
size, due to the "reasonable" interpretation of the spec that management
and prefetch block sizes are the same.

I think I could go either way. The nice thing about adding the node is
that it's self-documenting. While we could document that Zicbop will use
cbom's block size (and if we ever added a cbop-block-size, then we'd
change the documentation to state that cbom's block size is Zicbop's
fallback block size), it might be better for things like hwprobe to just
have them separate from the start. FWIW, ACPI already has a separate table
entry for Zicbop's block size.

I guess after letting this ping-pong back and forth in my brain a few
times the ball is currently resting on the "let's add cbop-block-size"
side.

[1] https://lore.kernel.org/all/20230914-892327a75b4b86badac5de02@orel/
[2] https://lore.kernel.org/all/20230914-74d0cf00633c199758ee3450@orel/

Thanks,
drew

> 
> > What we can't do is add stuff in the QEMU DT that's neither
> > documented nor acked in the DT bindings.
> 
> That's a welcome change.
> 
> Cheers,
> Conor.

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[Daniel Henrique Barboza]

On 10/30/23 05:18, Andrew Jones wrote:
> On Sun, Oct 29, 2023 at 10:21:55PM +0000, Conor Dooley wrote:
>> On Sun, Oct 29, 2023 at 04:49:30PM -0300, Daniel Henrique Barboza wrote:
>>>
>>>
>>> On 10/29/23 11:53, Conor Dooley wrote:
>>>> Yo,
>>>>
>>>> On Sun, Oct 29, 2023 at 09:35:00AM -0300, Daniel Henrique Barboza wrote:
>>>>> Following the examples of cbom-block-size and cboz-block-size,
>>>>> cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
>>>>> The most common case is to have all cache block sizes to be the same
>>>>> size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
>>>>> cache operations), but there's no specification requirement for that,
>>>>> and an implementation can have different cache sizes for each operation.
>>>>>
>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>> Cc: Conor Dooley <conor.dooley@microchip.com>
>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>
>>>> Firstly, odd CC list. Please CC the output of get_maintainer.pl in the
>>>> future.
>>>
>>> Ops, my bad
>>>
>>>>
>>>> IIRC, I mentioned defining this to Drew when he was add zicboz, but he
>>>> didn't want to add it - although he seems to have asked you to document
>>>> this. Drew, change of heart or am I not remembering correctly?
>>>> I think he cited some interpretation of the spec from Andrei W that
>>>> implied the Zicbop size would be the same as one of the other ones, but
>>>> I cannot find that on lore atm.
>>>
>>> The reason why I'm here is because I want to add Zicbop in QEMU riscv,isa.
>>> I'm pushing a rva22u64 profile implementation there and Zicbop is mandatory
>>> for it. In the process I added a riscv,cbop-block-size DT because, well,
>>> if both Zicboz and Zicbom have their respective block-size DTs, then it's
>>> expected that Zicbop also has one. Or so I thought.
>>>
>>> Drew then replied in the QEMU ML [1] that riscv,cbop-block-size isn't
>>> documented and we can't add it as it is. So here we are.
>>
>> Yeah, I did read that.
>>
>>> If riscv,cbop-block-size isn't needed because Zicbop will use the cache
>>> block size of Zicboz or Zicbom, that works for me too - I'll add a note
>>> in QEMU explaining why there's no riscv,cbop-block-size and everything
>>> is fine.
>>
>> I just wanted to remind Drew why we didn't add this in the first place,
>> given I had seen that he suggested that you add it in the QEMU thread.
>> And in the hopes that he would be able to dig the link back up to
>> Andrei's comments, given I wasn't able to find it/couldnt remember
>> recall where it had come from.
> 
> Hi Conor,
> 
> Thanks for the reminder. I had forgotten my own opinion on this :-)
> 
> I found the messages. In [1], I advocate for the block size DT property,
> but then, in [2], I reply to myself saying we could probably wait until
> we have a prefetch block size that differs from the management block
> size, due to the "reasonable" interpretation of the spec that management
> and prefetch block sizes are the same.
> 
> I think I could go either way. The nice thing about adding the node is
> that it's self-documenting. While we could document that Zicbop will use
> cbom's block size (and if we ever added a cbop-block-size, then we'd
> change the documentation to state that cbom's block size is Zicbop's
> fallback block size), it might be better for things like hwprobe to just
> have them separate from the start. FWIW, ACPI already has a separate table
> entry for Zicbop's block size.
> 
> I guess after letting this ping-pong back and forth in my brain a few
> times the ball is currently resting on the "let's add cbop-block-size"
> side.

I'll go ahead and understand that it is ok for QEMU to add a riscv,cbop-block-size DT
then.

If Linux wants to handle cbop-block-size as an alias of cbom-block-size that's
Linux prerrogative and it's perfectly fine. QEMU and other emulators/VMMs deals
with non-Linux OSes that uses the devicetree bindings though (like FreeBSD), so
we'd rather error to the side of having redundant info than baking in Linux
assumptions in the DT.


Thanks,

Daniel

> 
> [1] https://lore.kernel.org/all/20230914-892327a75b4b86badac5de02@orel/
> [2] https://lore.kernel.org/all/20230914-74d0cf00633c199758ee3450@orel/
> 
> Thanks,
> drew
> 
>>
>>> What we can't do is add stuff in the QEMU DT that's neither
>>> documented nor acked in the DT bindings.
>>
>> That's a welcome change.
>>
>> Cheers,
>> Conor.
> 
> 

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On Mon, Oct 30, 2023 at 06:14:45AM -0300, Daniel Henrique Barboza wrote:
> On 10/30/23 05:18, Andrew Jones wrote:

> > I guess after letting this ping-pong back and forth in my brain a few
> > times the ball is currently resting on the "let's add cbop-block-size"
> > side.
> 
> I'll go ahead and understand that it is ok for QEMU to add a riscv,cbop-block-size DT
> then.

Aye. And in case it was not clear, here's an unqualified ack.

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Can you pick this up Palmer?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[Andrew Jones]

On Sun, Oct 29, 2023 at 09:35:00AM -0300, Daniel Henrique Barboza wrote:
> Following the examples of cbom-block-size and cboz-block-size,
> cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
> The most common case is to have all cache block sizes to be the same
> size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
> cache operations), but there's no specification requirement for that,
> and an implementation can have different cache sizes for each operation.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  Documentation/devicetree/bindings/riscv/cpus.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index 97e8441eda1c..1660b296f7de 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -78,6 +78,11 @@ properties:
>      description:
>        The blocksize in bytes for the Zicbom cache operations.
>  
> +  riscv,cbop-block-size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The blocksize in bytes for the Zicbop cache operations.
> +
>    riscv,cboz-block-size:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
> -- 
> 2.41.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH] dt-bindings: riscv: Document cbop-block-size

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Sun, 29 Oct 2023 09:35:00 -0300 you wrote:
> Following the examples of cbom-block-size and cboz-block-size,
> cbop-block-size is the cache size of Zicbop (cbo.prefetch) operations.
> The most common case is to have all cache block sizes to be the same
> size (e.g. profiles such as rva22u64 mandates a 64 bytes size for all
> cache operations), but there's no specification requirement for that,
> and an implementation can have different cache sizes for each operation.
> 
> [...]

Here is the summary with links:
  - dt-bindings: riscv: Document cbop-block-size
    https://git.kernel.org/riscv/c/d3e591a38c98

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html