From: Conor Dooley <[email protected]>
The device trees produced automatically for the virt and spike machines
fail dt-validate on several grounds. Some of these need to be fixed in
the linux kernel's dt-bindings, but others are caused by bugs in QEMU.
Patches been sent that fix the QEMU issues [0], but a couple of them
need to be fixed in the kernel's dt-bindings. The first patches add
compatibles for "riscv,{clint,plic}0" which are present in drivers and
the auto generated QEMU dtbs. The final patch adds some new ISA strings
which needs scruitiny from someone with more knowledge about what ISA
extension strings should be reported in a dt than I have.
Thanks to Rob Herring for reporting these issues [1],
Conor.
To reproduce the errors:
./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
dt-validate -p /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json qemu.dtb
(The processed schema needs to be generated first)
0 - https://lore.kernel.org/linux-riscv/[email protected]
1 - https://lore.kernel.org/linux-riscv/[email protected]/
Conor Dooley (3):
dt-bindings: timer: sifive,clint: add legacy riscv compatible
dt-bindings: interrupt-controller: sifive,plic: add legacy riscv
compatible
dt-bindings: riscv: add new riscv,isa strings for emulators
.../sifive,plic-1.0.0.yaml | 5 +++++
.../devicetree/bindings/riscv/cpus.yaml | 2 ++
.../bindings/timer/sifive,clint.yaml | 18 ++++++++++++------
3 files changed, 19 insertions(+), 6 deletions(-)
base-commit: 42d670bda02fdba0f3944c92f545984501e5788d
--
2.37.1
From: Conor Dooley <[email protected]>
The QEMU virt and spike machines currently export a riscv,isa string of
"rv64imafdcsuh", but this obviously has illegal extensions in it.
The presense of "su" is a QEMU bug, so add an entry for the valid
portion of the isa string.
Reported-by: Rob Herring <[email protected]>
Link: https://lore.kernel.org/linux-riscv/[email protected]/
Signed-off-by: Conor Dooley <[email protected]>
---
Although the commit message says "a" string, I have added more than one
isa string. My patched version of QEMU emits the full string with the
multi letter extensions and I am not sure what the policy is for
including them in the binding. Obviously I am more than willing to
change the patch text if needed.
---
Documentation/devicetree/bindings/riscv/cpus.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index d632ac76532e..59b942c5b9aa 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -77,6 +77,8 @@ properties:
enum:
- rv64imac
- rv64imafdc
+ - rv64imafdch
+ - rv64imafdch_zicsr_zifencei_zba_zbb_zbc_zbs
# RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
timebase-frequency: false
--
2.37.1
From: Conor Dooley <[email protected]>
While "real" hardware might not use the compatible string "riscv,clint0"
it is present in the driver & QEMU uses it for automatically generated
virt machine dtbs. To avoid dt-validate problems with QEMU produced
dtbs, such as the following, add it to the binding.
riscv-virt.dtb: clint@2000000: compatible:0: 'sifive,clint0' is not one of ['sifive,fu540-c000-clint', 'starfive,jh7100-clint', 'canaan,k210-clint']
Reported-by: Rob Herring <[email protected]>
Link: https://lore.kernel.org/linux-riscv/[email protected]/
Signed-off-by: Conor Dooley <[email protected]>
---
.../bindings/timer/sifive,clint.yaml | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
index e64f46339079..9fcf20942582 100644
--- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
+++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
@@ -22,12 +22,18 @@ description:
properties:
compatible:
- items:
- - enum:
- - sifive,fu540-c000-clint
- - starfive,jh7100-clint
- - canaan,k210-clint
- - const: sifive,clint0
+ oneOf:
+ - items:
+ - enum:
+ - sifive,fu540-c000-clint
+ - starfive,jh7100-clint
+ - canaan,k210-clint
+ - const: sifive,clint0
+ - items:
+ - const: sifive,clint0
+ - const: riscv,clint0
+ deprecated: true
+ description: For legacy systems & the qemu virt machine only
description:
Should be "<vendor>,<chip>-clint" and "sifive,clint<version>".
--
2.37.1
From: Conor Dooley <[email protected]>
While "real" hardware might not use the compatible string "riscv,plic0"
it is present in the driver & QEMU uses it for automatically generated
virt machine dtbs. To avoid dt-validate problems with QEMU produced
dtbs, such as the following, add it to the binding.
riscv-virt.dtb: plic@c000000: compatible: 'oneOf' conditional failed, one must be fixed:
'sifive,plic-1.0.0' is not one of ['sifive,fu540-c000-plic', 'starfive,jh7100-plic', 'canaan,k210-plic']
'sifive,plic-1.0.0' is not one of ['allwinner,sun20i-d1-plic']
'sifive,plic-1.0.0' was expected
'thead,c900-plic' was expected
riscv-virt.dtb: plic@c000000: '#address-cells' is a required property
Reported-by: Rob Herring <[email protected]>
Link: https://lore.kernel.org/linux-riscv/[email protected]/
Signed-off-by: Conor Dooley <[email protected]>
---
.../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 92e0f8c3eff2..eb07c4f1a201 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -66,6 +66,11 @@ properties:
- enum:
- allwinner,sun20i-d1-plic
- const: thead,c900-plic
+ - items:
+ - const: sifive,plic-1.0.0
+ - const: riscv,plic0
+ deprecated: true
+ description: For legacy systems & the qemu virt machine only
reg:
maxItems: 1
--
2.37.1
On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> The device trees produced automatically for the virt and spike machines
> fail dt-validate on several grounds. Some of these need to be fixed in
> the linux kernel's dt-bindings, but others are caused by bugs in QEMU.
>
> Patches been sent that fix the QEMU issues [0], but a couple of them
> need to be fixed in the kernel's dt-bindings. The first patches add
> compatibles for "riscv,{clint,plic}0" which are present in drivers and
> the auto generated QEMU dtbs.
IMO the correct thing is to have QEMU use a qemu,plicX rather than to
weaken the requirement that a non-generic compatible be used. Otherwise
you end up with QEMU using something that's marked as deprecated and
either the warning remains and annoys people still or it becomes too
weak and people ignore it when creating real hardware.
> The final patch adds some new ISA strings
> which needs scruitiny from someone with more knowledge about what ISA
> extension strings should be reported in a dt than I have.
Listing every possible ISA string supported by the Linux kernel really
is not going to scale...
Jess
> Thanks to Rob Herring for reporting these issues [1],
> Conor.
>
> To reproduce the errors:
> ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb
> dt-validate -p /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json qemu.dtb
> (The processed schema needs to be generated first)
>
> 0 - https://lore.kernel.org/linux-riscv/[email protected]
> 1 - https://lore.kernel.org/linux-riscv/[email protected]/
>
> Conor Dooley (3):
> dt-bindings: timer: sifive,clint: add legacy riscv compatible
> dt-bindings: interrupt-controller: sifive,plic: add legacy riscv
> compatible
> dt-bindings: riscv: add new riscv,isa strings for emulators
>
> .../sifive,plic-1.0.0.yaml | 5 +++++
> .../devicetree/bindings/riscv/cpus.yaml | 2 ++
> .../bindings/timer/sifive,clint.yaml | 18 ++++++++++++------
> 3 files changed, 19 insertions(+), 6 deletions(-)
>
>
> base-commit: 42d670bda02fdba0f3944c92f545984501e5788d
> --
> 2.37.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On 08/08/2022 22:34, Jessica Clarke wrote:
> On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
>> From: Conor Dooley <[email protected]>
>>
>> The device trees produced automatically for the virt and spike machines
>> fail dt-validate on several grounds. Some of these need to be fixed in
>> the linux kernel's dt-bindings, but others are caused by bugs in QEMU.
>>
>> Patches been sent that fix the QEMU issues [0], but a couple of them
>> need to be fixed in the kernel's dt-bindings. The first patches add
>> compatibles for "riscv,{clint,plic}0" which are present in drivers and
>> the auto generated QEMU dtbs.
>
> IMO the correct thing is to have QEMU use a qemu,plicX rather than to
> weaken the requirement that a non-generic compatible be used. Otherwise
> you end up with QEMU using something that's marked as deprecated and
> either the warning remains and annoys people still or it becomes too
> weak and people ignore it when creating real hardware.
It's already in a driver so I figure it should be in the bindings too.
In arm's virt.c they use the generic gic compatible & I don't see any
evidence of other archs using "qemu,foo" bindings. I suppose there's
always the option of just removing the "riscv,plic0" from the riscv's
virt.c
>
>> The final patch adds some new ISA strings
>> which needs scruitiny from someone with more knowledge about what ISA
>> extension strings should be reported in a dt than I have.
>
> Listing every possible ISA string supported by the Linux kernel really
> is not going to scale...
Yeah, totally correct there. Case for adding a regex I suppose, but I
am not sure how to go about handling the multi-letter extensions or
if parsing them is required from a binding compliance point of view.
Hoping for some input from Palmer really.
Thanks,
Conor.
On Mon, Aug 08, 2022 at 10:01:11PM +0000, [email protected] wrote:
> On 08/08/2022 22:34, Jessica Clarke wrote:
> > On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
> >> From: Conor Dooley <[email protected]>
> >>
> >> The device trees produced automatically for the virt and spike machines
> >> fail dt-validate on several grounds. Some of these need to be fixed in
> >> the linux kernel's dt-bindings, but others are caused by bugs in QEMU.
> >>
> >> Patches been sent that fix the QEMU issues [0], but a couple of them
> >> need to be fixed in the kernel's dt-bindings. The first patches add
> >> compatibles for "riscv,{clint,plic}0" which are present in drivers and
> >> the auto generated QEMU dtbs.
> >
> > IMO the correct thing is to have QEMU use a qemu,plicX rather than to
> > weaken the requirement that a non-generic compatible be used. Otherwise
> > you end up with QEMU using something that's marked as deprecated and
> > either the warning remains and annoys people still or it becomes too
> > weak and people ignore it when creating real hardware.
>
> It's already in a driver so I figure it should be in the bindings too.
>
> In arm's virt.c they use the generic gic compatible & I don't see any
> evidence of other archs using "qemu,foo" bindings. I suppose there's
> always the option of just removing the "riscv,plic0" from the riscv's
> virt.c
I think we're pretty much stuck with what's in use already.
I'm on the fence whether to mark it deprecated though if there is no
plan to 'fix' it. Doesn't really matter until the tools can selectively
remove deprecated properties from validation.
> >> The final patch adds some new ISA strings
> >> which needs scruitiny from someone with more knowledge about what ISA
> >> extension strings should be reported in a dt than I have.
> >
> > Listing every possible ISA string supported by the Linux kernel really
> > is not going to scale...
How does the kernel scale? (No need to answer)
> Yeah, totally correct there. Case for adding a regex I suppose, but I
> am not sure how to go about handling the multi-letter extensions or
> if parsing them is required from a binding compliance point of view.
> Hoping for some input from Palmer really.
Yeah, looks like a regex pattern is needed.
Rob
On Fri, Aug 05, 2022 at 05:28:43PM +0100, Conor Dooley wrote:
> From: Conor Dooley <[email protected]>
>
> While "real" hardware might not use the compatible string "riscv,clint0"
> it is present in the driver & QEMU uses it for automatically generated
> virt machine dtbs. To avoid dt-validate problems with QEMU produced
> dtbs, such as the following, add it to the binding.
>
> riscv-virt.dtb: clint@2000000: compatible:0: 'sifive,clint0' is not one of ['sifive,fu540-c000-clint', 'starfive,jh7100-clint', 'canaan,k210-clint']
>
> Reported-by: Rob Herring <[email protected]>
> Link: https://lore.kernel.org/linux-riscv/[email protected]/
> Signed-off-by: Conor Dooley <[email protected]>
> ---
> .../bindings/timer/sifive,clint.yaml | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> index e64f46339079..9fcf20942582 100644
> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
> @@ -22,12 +22,18 @@ description:
>
> properties:
> compatible:
> - items:
> - - enum:
> - - sifive,fu540-c000-clint
> - - starfive,jh7100-clint
> - - canaan,k210-clint
> - - const: sifive,clint0
> + oneOf:
> + - items:
> + - enum:
> + - sifive,fu540-c000-clint
> + - starfive,jh7100-clint
> + - canaan,k210-clint
> + - const: sifive,clint0
> + - items:
> + - const: sifive,clint0
> + - const: riscv,clint0
> + deprecated: true
> + description: For legacy systems & the qemu virt machine only
I would drop 'legacy systems'.
>
> description:
> Should be "<vendor>,<chip>-clint" and "sifive,clint<version>".
> --
> 2.37.1
>
>
On 09/08/2022 15:14, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Mon, Aug 08, 2022 at 10:01:11PM +0000, [email protected] wrote:
>> On 08/08/2022 22:34, Jessica Clarke wrote:
>>> On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
>>>> From: Conor Dooley <[email protected]>
>>>>
>>>> The device trees produced automatically for the virt and spike machines
>>>> fail dt-validate on several grounds. Some of these need to be fixed in
>>>> the linux kernel's dt-bindings, but others are caused by bugs in QEMU.
>>>>
>>>> Patches been sent that fix the QEMU issues [0], but a couple of them
>>>> need to be fixed in the kernel's dt-bindings. The first patches add
>>>> compatibles for "riscv,{clint,plic}0" which are present in drivers and
>>>> the auto generated QEMU dtbs.
>>>
>>> IMO the correct thing is to have QEMU use a qemu,plicX rather than to
>>> weaken the requirement that a non-generic compatible be used. Otherwise
>>> you end up with QEMU using something that's marked as deprecated and
>>> either the warning remains and annoys people still or it becomes too
>>> weak and people ignore it when creating real hardware.
>>
>> It's already in a driver so I figure it should be in the bindings too.
>>
>> In arm's virt.c they use the generic gic compatible & I don't see any
>> evidence of other archs using "qemu,foo" bindings. I suppose there's
>> always the option of just removing the "riscv,plic0" from the riscv's
>> virt.c
>
> I think we're pretty much stuck with what's in use already.
>
> I'm on the fence whether to mark it deprecated though if there is no
> plan to 'fix' it. Doesn't really matter until the tools can selectively
> remove deprecated properties from validation.
>
I guess I had considered "deprecated" to mean "don't use it in new
device trees" rather than "don't use it at all". I am not sure how it
could be "fixed" if it is potentially used by who-tf-knows-what.
I do think that adding the deprecated flag adds information in the
absence of tooling that responds to the property.
Thanks,
Conor.
On 09/08/2022 15:16, Rob Herring wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, Aug 05, 2022 at 05:28:43PM +0100, Conor Dooley wrote:
>> From: Conor Dooley <[email protected]>
>>
>> While "real" hardware might not use the compatible string "riscv,clint0"
>> it is present in the driver & QEMU uses it for automatically generated
>> virt machine dtbs. To avoid dt-validate problems with QEMU produced
>> dtbs, such as the following, add it to the binding.
>>
>> riscv-virt.dtb: clint@2000000: compatible:0: 'sifive,clint0' is not one of ['sifive,fu540-c000-clint', 'starfive,jh7100-clint', 'canaan,k210-clint']
>>
>> Reported-by: Rob Herring <[email protected]>
>> Link: https://lore.kernel.org/linux-riscv/[email protected]/
>> Signed-off-by: Conor Dooley <[email protected]>
>> ---
>> .../bindings/timer/sifive,clint.yaml | 18 ++++++++++++------
>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> index e64f46339079..9fcf20942582 100644
>> --- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> +++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
>> @@ -22,12 +22,18 @@ description:
>>
>> properties:
>> compatible:
>> - items:
>> - - enum:
>> - - sifive,fu540-c000-clint
>> - - starfive,jh7100-clint
>> - - canaan,k210-clint
>> - - const: sifive,clint0
>> + oneOf:
>> + - items:
>> + - enum:
>> + - sifive,fu540-c000-clint
>> + - starfive,jh7100-clint
>> + - canaan,k210-clint
>> + - const: sifive,clint0
>> + - items:
>> + - const: sifive,clint0
>> + - const: riscv,clint0
>> + deprecated: true
>> + description: For legacy systems & the qemu virt machine only
>
> I would drop 'legacy systems'.
I took this from a comment in the driver against "riscv,plic0". Thought
it applied to both plic and clint bindings so put it in here. Happy to
drop them for v2 :)
Thanks,
Conor.
On 09/08/2022 15:14, Rob Herring wrote:
> On Mon, Aug 08, 2022 at 10:01:11PM +0000, [email protected] wrote:
>> On 08/08/2022 22:34, Jessica Clarke wrote:
>>> On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
>>>> From: Conor Dooley <[email protected]>
>>>> The final patch adds some new ISA strings
>>>> which needs scruitiny from someone with more knowledge about what ISA
>>>> extension strings should be reported in a dt than I have.
>>>
>>> Listing every possible ISA string supported by the Linux kernel really
>>> is not going to scale...
>
> How does the kernel scale? (No need to answer)
>
>> Yeah, totally correct there. Case for adding a regex I suppose, but I
>> am not sure how to go about handling the multi-letter extensions or
>> if parsing them is required from a binding compliance point of view.
>> Hoping for some input from Palmer really.
>
> Yeah, looks like a regex pattern is needed.
I started pottering away at this but I have arrived at:
rv64imaf?d?c?h?(_z[imafdqcbvkh]([a-z])*)*$
I suspect that before "h?" there should be more single letter
extensions added for completeness sake. So then it'd bloat out to:
rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
I checked a couple different "bad" isa strings against it and
nothing went up in flames but my regex skills are far from great
so I'm sure there's better ways to represent this.
Anyways, this pattern is based on my understanding that:
- the single letter order is fixed & we don't care about things that
can't even do "ima"
- the multi letter extensions are all in a "_z<foo>" format where the
first letter of <foo> is a valid single letter extension
- we don't care about the e extension from an OS PoV (this could be a
very flawed take...)
- after the first two chars, the extension name could be an english
word (ifencei anyone?) so it's not worth restricting the charset
- that attempting to validate the contents of the multiletter extensions
with dt-validate beyond the formatting is a futile, massively verbose
or unwieldy exercise at best
Some or all of those assumptions could be very very wrong so if {someone,
anyone} wants to correct me - feel ***more*** than free..
Thanks,
Conor.
patch would then look like:
diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
index d632ac76532e..1e54e7746190 100644
--- a/Documentation/devicetree/bindings/riscv/cpus.yaml
+++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
@@ -74,9 +74,7 @@ properties:
insensitive, letters in the riscv,isa string must be all
lowercase to simplify parsing.
$ref: "/schemas/types.yaml#/definitions/string"
- enum:
- - rv64imac
- - rv64imafdc
+ pattern: rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
# RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
timebase-frequency: false
Any takers on trashing my regex? Otherwise I'll just submit
a v2 with the regex and it can be shat on there instead :)
On 09/08/2022 19:36, Conor Dooley wrote:
> On 09/08/2022 15:14, Rob Herring wrote:
>> On Mon, Aug 08, 2022 at 10:01:11PM +0000, [email protected] wrote:
>>> On 08/08/2022 22:34, Jessica Clarke wrote:
>>>> On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
>>>>> From: Conor Dooley <[email protected]>
>>>>> The final patch adds some new ISA strings
>>>>> which needs scruitiny from someone with more knowledge about what ISA
>>>>> extension strings should be reported in a dt than I have.
>>>>
>>>> Listing every possible ISA string supported by the Linux kernel really
>>>> is not going to scale...
>>
>> How does the kernel scale? (No need to answer)
>>
>>> Yeah, totally correct there. Case for adding a regex I suppose, but I
>>> am not sure how to go about handling the multi-letter extensions or
>>> if parsing them is required from a binding compliance point of view.
>>> Hoping for some input from Palmer really.
>>
>> Yeah, looks like a regex pattern is needed.
>
> I started pottering away at this but I have arrived at:
> rv64imaf?d?c?h?(_z[imafdqcbvkh]([a-z])*)*$
>
> I suspect that before "h?" there should be more single letter
> extensions added for completeness sake. So then it'd bloat out to:
> rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
>
> I checked a couple different "bad" isa strings against it and
> nothing went up in flames but my regex skills are far from great
> so I'm sure there's better ways to represent this.
>
> Anyways, this pattern is based on my understanding that:
> - the single letter order is fixed & we don't care about things that
> can't even do "ima"
> - the multi letter extensions are all in a "_z<foo>" format where the
> first letter of <foo> is a valid single letter extension
> - we don't care about the e extension from an OS PoV (this could be a
> very flawed take...)
> - after the first two chars, the extension name could be an english
> word (ifencei anyone?) so it's not worth restricting the charset
> - that attempting to validate the contents of the multiletter extensions
> with dt-validate beyond the formatting is a futile, massively verbose
> or unwieldy exercise at best
>
> Some or all of those assumptions could be very very wrong so if {someone,
> anyone} wants to correct me - feel ***more*** than free..
>
> Thanks,
> Conor.
>
> patch would then look like:
>
> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> index d632ac76532e..1e54e7746190 100644
> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> @@ -74,9 +74,7 @@ properties:
> insensitive, letters in the riscv,isa string must be all
> lowercase to simplify parsing.
> $ref: "/schemas/types.yaml#/definitions/string"
> - enum:
> - - rv64imac
> - - rv64imafdc
> + pattern: rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
>
> # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
> timebase-frequency: false
On Mon, Aug 15, 2022 at 07:18:02PM +0000, [email protected] wrote:
> Any takers on trashing my regex? Otherwise I'll just submit
> a v2 with the regex and it can be shat on there instead :)
>
> On 09/08/2022 19:36, Conor Dooley wrote:
> > On 09/08/2022 15:14, Rob Herring wrote:
> >> On Mon, Aug 08, 2022 at 10:01:11PM +0000, [email protected] wrote:
> >>> On 08/08/2022 22:34, Jessica Clarke wrote:
> >>>> On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
> >>>>> From: Conor Dooley <[email protected]>
> >>>>> The final patch adds some new ISA strings
> >>>>> which needs scruitiny from someone with more knowledge about what ISA
> >>>>> extension strings should be reported in a dt than I have.
> >>>>
> >>>> Listing every possible ISA string supported by the Linux kernel really
> >>>> is not going to scale...
> >>
> >> How does the kernel scale? (No need to answer)
> >>
> >>> Yeah, totally correct there. Case for adding a regex I suppose, but I
> >>> am not sure how to go about handling the multi-letter extensions or
> >>> if parsing them is required from a binding compliance point of view.
> >>> Hoping for some input from Palmer really.
> >>
> >> Yeah, looks like a regex pattern is needed.
> >
> > I started pottering away at this but I have arrived at:
> > rv64imaf?d?c?h?(_z[imafdqcbvkh]([a-z])*)*$
Don't forget the ^ at the start.
Do we need to worry about optional major and minor version numbers?
Or check that Z names have at least one character following the category
character? Actually, the first letter after Z being a category is only a
convention. Maybe we don't want to enforce that. What about X extensions?
Thanks,
drew
> >
> > I suspect that before "h?" there should be more single letter
> > extensions added for completeness sake. So then it'd bloat out to:
> > rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
> >
> > I checked a couple different "bad" isa strings against it and
> > nothing went up in flames but my regex skills are far from great
> > so I'm sure there's better ways to represent this.
> >
> > Anyways, this pattern is based on my understanding that:
> > - the single letter order is fixed & we don't care about things that
> > can't even do "ima"
> > - the multi letter extensions are all in a "_z<foo>" format where the
> > first letter of <foo> is a valid single letter extension
> > - we don't care about the e extension from an OS PoV (this could be a
> > very flawed take...)
> > - after the first two chars, the extension name could be an english
> > word (ifencei anyone?) so it's not worth restricting the charset
> > - that attempting to validate the contents of the multiletter extensions
> > with dt-validate beyond the formatting is a futile, massively verbose
> > or unwieldy exercise at best
> >
> > Some or all of those assumptions could be very very wrong so if {someone,
> > anyone} wants to correct me - feel ***more*** than free..
> >
> > Thanks,
> > Conor.
> >
> > patch would then look like:
> >
> > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > index d632ac76532e..1e54e7746190 100644
> > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > @@ -74,9 +74,7 @@ properties:
> > insensitive, letters in the riscv,isa string must be all
> > lowercase to simplify parsing.
> > $ref: "/schemas/types.yaml#/definitions/string"
> > - enum:
> > - - rv64imac
> > - - rv64imafdc
> > + pattern: rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
> >
> > # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
> > timebase-frequency: false
>
Hey Drew,
Thanks for piping up.
On 16/08/2022 15:06, Andrew Jones wrote:
> [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Mon, Aug 15, 2022 at 07:18:02PM +0000, [email protected] wrote:
>> Any takers on trashing my regex? Otherwise I'll just submit
>> a v2 with the regex and it can be shat on there instead :)
>>
>> On 09/08/2022 19:36, Conor Dooley wrote:
>>> On 09/08/2022 15:14, Rob Herring wrote:
>>>> On Mon, Aug 08, 2022 at 10:01:11PM +0000, [email protected] wrote:
>>>>> On 08/08/2022 22:34, Jessica Clarke wrote:
>>>>>> On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
>>>>>>> From: Conor Dooley <[email protected]>
>>>>>>> The final patch adds some new ISA strings
>>>>>>> which needs scruitiny from someone with more knowledge about what ISA
>>>>>>> extension strings should be reported in a dt than I have.
>>>>>>
>>>>>> Listing every possible ISA string supported by the Linux kernel really
>>>>>> is not going to scale...
>>>>
>>>> How does the kernel scale? (No need to answer)
>>>>
>>>>> Yeah, totally correct there. Case for adding a regex I suppose, but I
>>>>> am not sure how to go about handling the multi-letter extensions or
>>>>> if parsing them is required from a binding compliance point of view.
>>>>> Hoping for some input from Palmer really.
>>>>
>>>> Yeah, looks like a regex pattern is needed.
>>>
>>> I started pottering away at this but I have arrived at:
>>> rv64imaf?d?c?h?(_z[imafdqcbvkh]([a-z])*)*$
>
> Don't forget the ^ at the start.
>
> Do we need to worry about optional major and minor version numbers?
> Or check that Z names have at least one character following the category
> character? Actually, the first letter after Z being a category is only a
> convention. Maybe we don't want to enforce that. What about X extensions?
For the character after Z, I think we could operate on the assumption
that that's a convention until things change. The regex isnt set in
stone forever.
With x, it becomes - which to me makes bad worse:
^rv64imaf?d?q?c?b?v?k?h?(?:(?:_z[imafdqcbvkh]|_x)(?:[a-z])*)*$
and then for the version numbers it becomes completely awful.
I'd argue that if we are going to support those, then we should
do that as another regex. We are already forcing lower case in
these ISA strings - is there an actual benefit in adding the
numbers, or might we want to "encourage" removing those too?
I hope I am missing something, as my regex foo isn't that good, to
enforce the ordering & the numbers - even for the simple case of the
major number only, we'd need to convert "f?" to "(?:f\d+)?" and so
on for every single extension. I don't think we reduce that either
as we want to enforce the ordering.
For the minor versions it goes to "(?:f\d+p\d+)?". At that point I
don't think we are adding any value but w/e, who am I to decide.
That ballooned out to 194 characters for me. I then decided to have
a bit of fun, and just do both number sets as a oneliner, using
some named match groups. That was about 255 characters. ????
Anyway, dt-schema had a panic attack at something I was doing
so I think that /may/ be a bad idea.
I vote for allow the x extensions, keep the convention for standard
extensions & revisit this in the future if needed...
Thanks,
Conor.
>
> Thanks,
> drew
>
>>>
>>> I suspect that before "h?" there should be more single letter
>>> extensions added for completeness sake. So then it'd bloat out to:
>>> rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
>>>
>>> I checked a couple different "bad" isa strings against it and
>>> nothing went up in flames but my regex skills are far from great
>>> so I'm sure there's better ways to represent this.
>>>
>>> Anyways, this pattern is based on my understanding that:
>>> - the single letter order is fixed & we don't care about things that
>>> can't even do "ima"
>>> - the multi letter extensions are all in a "_z<foo>" format where the
>>> first letter of <foo> is a valid single letter extension
>>> - we don't care about the e extension from an OS PoV (this could be a
>>> very flawed take...)
>>> - after the first two chars, the extension name could be an english
>>> word (ifencei anyone?) so it's not worth restricting the charset
>>> - that attempting to validate the contents of the multiletter extensions
>>> with dt-validate beyond the formatting is a futile, massively verbose
>>> or unwieldy exercise at best
>>>
>>> Some or all of those assumptions could be very very wrong so if {someone,
>>> anyone} wants to correct me - feel ***more*** than free..
>>>
>>> Thanks,
>>> Conor.
>>>
>>> patch would then look like:
>>>
>>> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
>>> index d632ac76532e..1e54e7746190 100644
>>> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
>>> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
>>> @@ -74,9 +74,7 @@ properties:
>>> insensitive, letters in the riscv,isa string must be all
>>> lowercase to simplify parsing.
>>> $ref: "/schemas/types.yaml#/definitions/string"
>>> - enum:
>>> - - rv64imac
>>> - - rv64imafdc
>>> + pattern: rv64imaf?d?q?c?b?v?k?h?(_z[imafdqcbvkh]([a-z])*)*$
>>>
>>> # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
>>> timebase-frequency: false
>>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Aug 16, 2022 at 10:53:45PM +0000, [email protected] wrote:
> Hey Drew,
> Thanks for piping up.
>
> On 16/08/2022 15:06, Andrew Jones wrote:
> > [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > On Mon, Aug 15, 2022 at 07:18:02PM +0000, [email protected] wrote:
> >> Any takers on trashing my regex? Otherwise I'll just submit
> >> a v2 with the regex and it can be shat on there instead :)
> >>
> >> On 09/08/2022 19:36, Conor Dooley wrote:
> >>> On 09/08/2022 15:14, Rob Herring wrote:
> >>>> On Mon, Aug 08, 2022 at 10:01:11PM +0000, [email protected] wrote:
> >>>>> On 08/08/2022 22:34, Jessica Clarke wrote:
> >>>>>> On Fri, Aug 05, 2022 at 05:28:42PM +0100, Conor Dooley wrote:
> >>>>>>> From: Conor Dooley <[email protected]>
> >>>>>>> The final patch adds some new ISA strings
> >>>>>>> which needs scruitiny from someone with more knowledge about what ISA
> >>>>>>> extension strings should be reported in a dt than I have.
> >>>>>>
> >>>>>> Listing every possible ISA string supported by the Linux kernel really
> >>>>>> is not going to scale...
> >>>>
> >>>> How does the kernel scale? (No need to answer)
> >>>>
> >>>>> Yeah, totally correct there. Case for adding a regex I suppose, but I
> >>>>> am not sure how to go about handling the multi-letter extensions or
> >>>>> if parsing them is required from a binding compliance point of view.
> >>>>> Hoping for some input from Palmer really.
> >>>>
> >>>> Yeah, looks like a regex pattern is needed.
> >>>
> >>> I started pottering away at this but I have arrived at:
> >>> rv64imaf?d?c?h?(_z[imafdqcbvkh]([a-z])*)*$
> >
> > Don't forget the ^ at the start.
> >
> > Do we need to worry about optional major and minor version numbers?
> > Or check that Z names have at least one character following the category
> > character? Actually, the first letter after Z being a category is only a
> > convention. Maybe we don't want to enforce that. What about X extensions?
>
> For the character after Z, I think we could operate on the assumption
> that that's a convention until things change. The regex isnt set in
> stone forever.
> With x, it becomes - which to me makes bad worse:
> ^rv64imaf?d?q?c?b?v?k?h?(?:(?:_z[imafdqcbvkh]|_x)(?:[a-z])*)*$
I think we should change the ([a-z]*) to ([a-z]+).
>
> and then for the version numbers it becomes completely awful.
> I'd argue that if we are going to support those, then we should
> do that as another regex. We are already forcing lower case in
> these ISA strings - is there an actual benefit in adding the
> numbers, or might we want to "encourage" removing those too?
>
> I hope I am missing something, as my regex foo isn't that good, to
> enforce the ordering & the numbers - even for the simple case of the
> major number only, we'd need to convert "f?" to "(?:f\d+)?" and so
> on for every single extension. I don't think we reduce that either
> as we want to enforce the ordering.
>
> For the minor versions it goes to "(?:f\d+p\d+)?". At that point I
> don't think we are adding any value but w/e, who am I to decide.
> That ballooned out to 194 characters for me. I then decided to have
> a bit of fun, and just do both number sets as a oneliner, using
> some named match groups. That was about 255 characters. ????
> Anyway, dt-schema had a panic attack at something I was doing
> so I think that /may/ be a bad idea.
I presume if a version is used it means one cannot rely on the default
version. So we can't always encourage them to be removed. To simplify
things we can always require minor numbers. We can also always require
underscores. Something like
^rv64_i(\d+p\d+)?_m\1?_a\1?(_f\1?)?(_d\1?)? ... ((_z[imafdqcbvkh]|_x)[a-z]+\1?)*$
>
> I vote for allow the x extensions, keep the convention for standard
> extensions & revisit this in the future if needed...
Sounds good. We can also easily add _s|_h|_zxm to the OR if we want. But,
there is a problem with the OR. By using it we don't enforce order. To be
pedantic we should ensure _z comes before _s, then _h, then _zxm, then _x.
Thanks,
drew