On Mon, Apr 15, 2024 at 11:10:24AM +0200, Clément Léger wrote:
On 11/04/2024 13:53, Conor Dooley wrote:
On Thu, Apr 11, 2024 at 11:08:21AM +0200, Clément Léger wrote:
If we consider to have potentially broken isa string (ie extensions dependencies not correctly handled), then we'll need some way to validate this within the kernel.
No, the DT passed to the kernel should be correct and we by and large we should not have to do validation of it. What I meant above was writing the binding so that something invalid will not pass dtbs_check.
Acked, I was mainly answering Deepak question about dependencies wrt to using __RISCV_ISA_EXT_SUPERSET() which does not seems to be relevant since we expect a correct isa string to be passed.
Ahh, okay.
But as you stated, DT validation clearly make sense. I think a lot of extensions strings would benefit such support (All the Zv* depends on V, etc).
I think it is actually as simple something like this, which makes it invalid to have "d" without "f":
| diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml | index 468c646247aa..594828700cbe 100644 | --- a/Documentation/devicetree/bindings/riscv/extensions.yaml | +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml | @@ -484,5 +484,20 @@ properties: | Registers in the AX45MP datasheet. | https://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.... | | +allOf: | + - if: | + properties: | + riscv,isa-extensions: | + contains: | + const: "d" | + not: | + contains: | + const: "f" | + then: | + properties: | + riscv,isa-extensions: | + false | + | + | additionalProperties: true | ...
If you do have d without f, the checker will say: cpu@2: riscv,isa-extensions: False schema does not allow ['i', 'm', 'a', 'd', 'c']
At least that's readable, even though not clear about what to do. I wish
That looks really readable indeed but the messages that result from errors are not so informative.
It tried playing with various constructs and found this one to yield a comprehensive message:
+allOf:
- if:
properties:riscv,isa-extensions:contains:const: zcfnot:contains:const: zca- then:
properties:riscv,isa-extensions:items:anyOf:- const: zcaarch/riscv/boot/dts/allwinner/sun20i-d1-dongshan-nezha-stu.dtb: cpu@0: riscv,isa-extensions:10: 'anyOf' conditional failed, one must be fixed: 'zca' was expected from schema $id: http://devicetree.org/schemas/riscv/extensions.yaml
Even though dt-bindings-check passed, not sure if this is totally a valid construct though...
I asked Rob about this yesterday, he suggested adding: riscv,isa-extensions: if: contains: const: zcf then: contains: const: zca to the existing property, not in an allOf. I think that is by far the most readable version in terms of what goes into the binding. The output would look like: cpu@0: riscv,isa-extensions: ['i', 'm', 'a', 'd', 'c'] does not contain items matching the given schema (for d requiring f cos I am lazy)
Also, his comment about your one that gives the nice message was that it would wrong as the anyOf was pointless and it says all items must be "zca". I didn't try it, but I have a feeling your nice output will be rather less nice if several different deps are unmet - but hey, probably will still be better than having an undocumented extension!