Re: [PATCH v2 2/6] dt-bindings: riscv: Document cboz-block-size

Re: [PATCH v2 2/6] dt-bindings: riscv: Document cboz-block-size

[Andrew Jones]

On Mon, Jan 23, 2023 at 08:10:35AM +0000, Conor Dooley wrote:
> Hey,
> 
> On Sun, Jan 22, 2023 at 08:13:24PM +0100, Andrew Jones wrote:
> > The Zicboz operates on a block-size defined for the cpu-core,
> > which does not necessarily match other cache-sizes used.
> > 
> > So add the necessary property for the system to know the core's
> > block-size.
> > 
> > Cc: Rob Herring <robh@kernel.org>
> 
> FYI bindings need to be CC Krzysztof & the devicetree list too.

Thanks, hopefully CC'ing them now is OK (I just added them). If not,
I can repost.

> 
> > Signed-off-by: Andrew Jones <ajones@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 c6720764e765..f4ee70f8e1cf 100644
> > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > @@ -72,6 +72,11 @@ properties:
> >      description:
> >        The blocksize in bytes for the Zicbom cache operations.
> >  
> > +  riscv,cboz-block-size:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      The blocksize in bytes for the Zicboz cache operations.
> 
> Do you think hardware that has different Zicboz versus Zicbom block
> sizes is going to be common, or would we benefit from just defaulting
> the Zicboz size to the Zicbom one?

I'm not sure. If it turns out the block size will be the same in most
cases, then we could add another property named cbo-block-size, which,
when present, would be parsed first and used to populate all CBO-related
block sizes. Then, these specific properties would only serve to override
that general size for their respective block sizes when they're present.

> 
> Either way,
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
drew

> 
> Thanks,
> Conor.
> 

Re: [PATCH v2 2/6] dt-bindings: riscv: Document cboz-block-size

[Krzysztof Kozlowski]

On 23/01/2023 10:44, Andrew Jones wrote:
> On Mon, Jan 23, 2023 at 08:10:35AM +0000, Conor Dooley wrote:
>> Hey,
>>
>> On Sun, Jan 22, 2023 at 08:13:24PM +0100, Andrew Jones wrote:
>>> The Zicboz operates on a block-size defined for the cpu-core,
>>> which does not necessarily match other cache-sizes used.
>>>
>>> So add the necessary property for the system to know the core's
>>> block-size.
>>>
>>> Cc: Rob Herring <robh@kernel.org>
>>
>> FYI bindings need to be CC Krzysztof & the devicetree list too.
> 
> Thanks, hopefully CC'ing them now is OK (I just added them). If not,
> I can repost.
> 

It does not work like this. I don't have the patch in my inbox. How it
should magically appear there?

Also how do you expect the bot to get it?

You need to use get_maintainers.pl *always*. Please resend.

Best regards,
Krzysztof

Re: [PATCH v2 2/6] dt-bindings: riscv: Document cboz-block-size

[Andrew Jones]

On Mon, Jan 23, 2023 at 08:33:56AM -0600, Rob Herring wrote:
> On Sun, Jan 22, 2023 at 1:13 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > The Zicboz operates on a block-size defined for the cpu-core,
> > which does not necessarily match other cache-sizes used.
> 
> Please use get_maintainers.pl and send patches to the correct lists.

Yup, Conor also pointed out that I forgot to update the CC list when
adding this patch to the series.

> 
> I have no idea what Zicboz is. How does it relate to Zicbom for which
> we already have a block size property? I really hate one by one
> property additions because they lead to poorly designed bindings. So
> what's next? What other information might be needed?

Zicbom and Zicboz are both RISC-V ISA extensions for cache-block operation
(CBO) instructions. Zicbom defines the instructions cbo.inval, cbo.clean,
and cbo.flush while Zicboz only defines cbo.zero. While it's probably
likely that the cache block sizes which Zicbom and Zicboz use will be
the same when both are implemented, the specification does not require it.
With that in mind, it makes sense to me that Zicbom has its own property
and that Zicboz could follow Zicbom's pattern with its own property as
well.

That said, having a generic block size property which is used in the
absence of the per-extension block size properties would allow DTs to only
specify the size once when they're the same. In my reply to Conor, I
suggested introducing a cbo-block-size property for this purpose, but Anup
suggests we just expand the purpose of cbom-block-size. Expanding cbom-
block-size's purpose would allow its size to be used with cbo.zero in the
absence of a cboz-block-size property. Additionally, we could defer the
introduction of the cboz-block-size property until some system needs it,
which may be never.

As far as to what's coming next, I'm not aware of a plan for more of these
types of properties at this time, but the CMO spec also describes prefetch
instructions, which are defined under the Zicbop extension. If Zicbop
support is added, then it should follow the same pattern as we agree for
Zicboz, which is either

 a. Add cboz-block-size and require it (as this series currently does)
 b. Add cboz-block-size, expand the function of cbom-block-size to be
    a fallback, and fallback to cbom-block-size when cboz-block-size is
    absent
 c. Don't add cboz-block-size, only expand the function of cbom-block-size
    and use it. If a need arises for cboz-block-size some day, then it
    can be added at that time.
 d. ??
  
I'm not aware of any additional information needed for these extensions
beyond the block sizes.

Thanks,
drew

Re: [PATCH v2 2/6] dt-bindings: riscv: Document cboz-block-size

[Conor Dooley]

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

Hey Drew, Rob,

On Mon, Jan 23, 2023 at 04:54:04PM +0100, Andrew Jones wrote:
> On Mon, Jan 23, 2023 at 08:33:56AM -0600, Rob Herring wrote:
> > On Sun, Jan 22, 2023 at 1:13 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > The Zicboz operates on a block-size defined for the cpu-core,
> > > which does not necessarily match other cache-sizes used.
> > 
> > Please use get_maintainers.pl and send patches to the correct lists.
> 
> Yup, Conor also pointed out that I forgot to update the CC list when
> adding this patch to the series.
> 
> > 
> > I have no idea what Zicboz is. How does it relate to Zicbom for which
> > we already have a block size property? I really hate one by one
> > property additions because they lead to poorly designed bindings. So
> > what's next? What other information might be needed?
> 
> Zicbom and Zicboz are both RISC-V ISA extensions for cache-block operation
> (CBO) instructions. Zicbom defines the instructions cbo.inval, cbo.clean,
> and cbo.flush while Zicboz only defines cbo.zero. While it's probably
> likely that the cache block sizes which Zicbom and Zicboz use will be
> the same when both are implemented, the specification does not require it.
> With that in mind, it makes sense to me that Zicbom has its own property
> and that Zicboz could follow Zicbom's pattern with its own property as
> well.
> 
> That said, having a generic block size property which is used in the
> absence of the per-extension block size properties would allow DTs to only
> specify the size once when they're the same. In my reply to Conor, I
> suggested introducing a cbo-block-size property for this purpose, but Anup
> suggests we just expand the purpose of cbom-block-size. Expanding cbom-
> block-size's purpose would allow its size to be used with cbo.zero in the
> absence of a cboz-block-size property. Additionally, we could defer the
> introduction of the cboz-block-size property until some system needs it,
> which may be never.
> 
> As far as to what's coming next, I'm not aware of a plan for more of these
> types of properties at this time, but the CMO spec also describes prefetch
> instructions, which are defined under the Zicbop extension. If Zicbop
> support is added, then it should follow the same pattern as we agree for
> Zicboz, which is either
> 
>  a. Add cboz-block-size and require it (as this series currently does)

heh, be careful with the word "require", in dt-binding terms it's not
required - we just get a pr_err() and the feature is disabled, right?

This is most clear of the options though, even if it will likely be
superfluous most of the time...

>  c. Don't add cboz-block-size, only expand the function of cbom-block-size
>     and use it. If a need arises for cboz-block-size some day, then it
>     can be added at that time.

I don't really think that this one makes much sense. It'd be perfectly
okay to have Zicboz without Zicbom, even if that scenario may be
unlikely.
Having a system with Zicboz in the isa string, a cbom-block-size just
sounds like a source of confusion.
IMO, there's enough potential crud that "random" extensions may bring
going forward that I'd rather not make decisions here that complicate
matters more.

>  b. Add cboz-block-size, expand the function of cbom-block-size to be
>     a fallback, and fallback to cbom-block-size when cboz-block-size is
>     absent

I personally think that this one is pretty fair. I won't even try to
claim knowledge of what decisions hardware folk will make, but I think
it is likely that cbo.zero will share its block size with the other
three cbo instructions that we already support.

idk if you can refer to other properties in a binding, but effectively I
am suggesting:
  riscv,cboz-block-size:
    $ref: /schemas/types.yaml#/definitions/uint32
    default: riscv,cbom-block-size
    description:
      The blocksize in bytes for the Zicboz cache operations.

>  d. ??

Have both properties and default them to the regular old cache block
sizes, thereby allowing Zicbom/Zicboz in the ISA string without either
property at all?
Or is that one an ABI break? And if it is, do we care since there
doesn't appear to be a linux-capable, Zicbo* compatible SoC yet?

Thanks,
Conor.


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

Re: [PATCH v2 2/6] dt-bindings: riscv: Document cboz-block-size

[Andrew Jones]

On Mon, Jan 23, 2023 at 06:25:22PM +0000, Conor Dooley wrote:
> Hey Drew, Rob,
> 
> On Mon, Jan 23, 2023 at 04:54:04PM +0100, Andrew Jones wrote:
> > On Mon, Jan 23, 2023 at 08:33:56AM -0600, Rob Herring wrote:
> > > On Sun, Jan 22, 2023 at 1:13 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > >
> > > > The Zicboz operates on a block-size defined for the cpu-core,
> > > > which does not necessarily match other cache-sizes used.
> > > 
> > > Please use get_maintainers.pl and send patches to the correct lists.
> > 
> > Yup, Conor also pointed out that I forgot to update the CC list when
> > adding this patch to the series.
> > 
> > > 
> > > I have no idea what Zicboz is. How does it relate to Zicbom for which
> > > we already have a block size property? I really hate one by one
> > > property additions because they lead to poorly designed bindings. So
> > > what's next? What other information might be needed?
> > 
> > Zicbom and Zicboz are both RISC-V ISA extensions for cache-block operation
> > (CBO) instructions. Zicbom defines the instructions cbo.inval, cbo.clean,
> > and cbo.flush while Zicboz only defines cbo.zero. While it's probably
> > likely that the cache block sizes which Zicbom and Zicboz use will be
> > the same when both are implemented, the specification does not require it.
> > With that in mind, it makes sense to me that Zicbom has its own property
> > and that Zicboz could follow Zicbom's pattern with its own property as
> > well.
> > 
> > That said, having a generic block size property which is used in the
> > absence of the per-extension block size properties would allow DTs to only
> > specify the size once when they're the same. In my reply to Conor, I
> > suggested introducing a cbo-block-size property for this purpose, but Anup
> > suggests we just expand the purpose of cbom-block-size. Expanding cbom-
> > block-size's purpose would allow its size to be used with cbo.zero in the
> > absence of a cboz-block-size property. Additionally, we could defer the
> > introduction of the cboz-block-size property until some system needs it,
> > which may be never.
> > 
> > As far as to what's coming next, I'm not aware of a plan for more of these
> > types of properties at this time, but the CMO spec also describes prefetch
> > instructions, which are defined under the Zicbop extension. If Zicbop
> > support is added, then it should follow the same pattern as we agree for
> > Zicboz, which is either
> > 
> >  a. Add cboz-block-size and require it (as this series currently does)
> 
> heh, be careful with the word "require", in dt-binding terms it's not
> required - we just get a pr_err() and the feature is disabled, right?

Correct. Here "require" means that without this property Zicboz won't
work, not that the DT won't validate. I'll use "need" for this purpose
to avoid confusion below :-)

> 
> This is most clear of the options though, even if it will likely be
> superfluous most of the time...

I'm leaning more towards this approach (and not just because it's
already done). While this property may potentially be redundant with
cbom-block-size and cache-block-size, one should be sure they know
Zicboz's cache block size before they use it. Having to explicitly
assign it to a property in the DT to get Zicboz to work should ensure
they've double checked their manuals. Otherwise, potentially difficult
to debug issues may emerge.

> 
> >  c. Don't add cboz-block-size, only expand the function of cbom-block-size
> >     and use it. If a need arises for cboz-block-size some day, then it
> >     can be added at that time.
> 
> I don't really think that this one makes much sense. It'd be perfectly
> okay to have Zicboz without Zicbom, even if that scenario may be
> unlikely.
> Having a system with Zicboz in the isa string, a cbom-block-size just
> sounds like a source of confusion.
> IMO, there's enough potential crud that "random" extensions may bring
> going forward that I'd rather not make decisions here that complicate
> matters more.
> 
> >  b. Add cboz-block-size, expand the function of cbom-block-size to be
> >     a fallback, and fallback to cbom-block-size when cboz-block-size is
> >     absent
> 
> I personally think that this one is pretty fair. I won't even try to
> claim knowledge of what decisions hardware folk will make, but I think
> it is likely that cbo.zero will share its block size with the other
> three cbo instructions that we already support.

While I think we're all in agreement on the likeliness of these block
sizes being the same, I think the fact that we have to use the word
'likely' implies we should just stick to explicit properties.

> 
> idk if you can refer to other properties in a binding, but effectively I
> am suggesting:
>   riscv,cboz-block-size:
>     $ref: /schemas/types.yaml#/definitions/uint32
>     default: riscv,cbom-block-size
>     description:
>       The blocksize in bytes for the Zicboz cache operations.
> 
> >  d. ??
> 
> Have both properties and default them to the regular old cache block
> sizes, thereby allowing Zicbom/Zicboz in the ISA string without either
> property at all?
> Or is that one an ABI break? And if it is, do we care since there
> doesn't appear to be a linux-capable, Zicbo* compatible SoC yet?

I don't think it would be ABI breakage unless we need to preserve
failures in the absence of cbom-block-size and/or cannot expand
the scope of cache-block-size to include Zicbom and Zicboz. IMO,
those should be safe changes, but I'm still leaning towards keeping
all these sizes explicit and needed for their respective extensions,
particularly if we believe we should add the properties anyway.

Thanks,
drew