| Message ID | 20220805162844.1554247-1-mail@conchuod.ie |
|---|---|
| Headers | show |
| Series | Fix dt-validate issues on qemu dtbdumps due to dt-bindings | expand |
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 <conor.dooley@microchip.com> >> >> 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.
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, Conor.Dooley@microchip.com 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 <conor.dooley@microchip.com> >>>>> 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, Conor.Dooley@microchip.com 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, Conor.Dooley@microchip.com 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 <conor.dooley@microchip.com> > >>>>> 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 >
From: Conor Dooley <conor.dooley@microchip.com> 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/20220805155405.1504081-1-mail@conchuod.ie 1 - https://lore.kernel.org/linux-riscv/20220803170552.GA2250266-robh@kernel.org/ 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