On 19/04/2024 17:49, Conor Dooley wrote: > On Thu, Apr 18, 2024 at 02:42:26PM +0200, Clément Léger wrote: >> As stated by Zc* spec: >> >> "As C defines the same instructions as Zca, Zcf and Zcd, the rule is that: >> - C always implies Zca >> - C+F implies Zcf (RV32 only) >> - C+D implies Zcd" >> >> Add additionnal validation rules to enforce this in dts. > > I'll get it out of the way: NAK, and the dts patch is the perfect > example of why. I don't want us to have to continually update > devicetrees. If these are implied due to being subsets of other > extensions, then software should be able to enable them when that > other extension is present. Acked. > > My fear is that, and a quick look at the "add probing" commit seemed to > confirm it, new subsets would require updates to the dts, even though > the existing extension is perfectly sufficient to determine presence. > > I definitely want to avoid continual updates to the devicetree for churn > reasons whenever subsets are added, but not turning on the likes of Zca > when C is present because "the bindings were updated to enforce this" > is a complete blocker. I do concede that having two parents makes that > more difficult and will likely require some changes to how we probe - do > we need to have a "second round" type thing? Yeah, I understand. At first, I actually did the modifications in the ISA probing loop with some dependency probing (ie loop while we don't have a stable extension state). But I thought that it was not actually our problem but rather the ISA string provider. For instance, Qemu provides them. > Taking Zcf as an example, maybe something like making both of C and F into > "standard" supersets and adding a case to riscv_isa_extension_check() > that would mandate that Zca and F are enabled before enabling it, and we > would ensure that C implies Zca before it implies Zcf? I'm afraid that riscv_isa_extension_check() will become a rat nest so rather than going that way, I would be in favor of adding a validation callback for the extensions if needed. > > Given we'd be relying on ordering, we have to perform the same implication > for both F and C and make sure that the "implies" struct has Zca before Zcf. > I don't really like that suggestion, hopefully there's a nicer way of doing > that, but I don't like the dt stuff here. I guess the "cleanest" way would be to have some "defered-like" mechanism in ISA probing which would allow to handle ordering as well as dependencies/implies for extensions. For Zca, Zcf, we actually do not have ordering problems but I think it would be a bit broken not to support that as well. I can actually revive the work mentioned above to handle that and see if it works ok. Clément > > Thanks, > Conor. > >> >> Signed-off-by: Clément Léger <cleger@xxxxxxxxxxxx> >> --- >> .../devicetree/bindings/riscv/cpus.yaml | 8 +++-- >> .../devicetree/bindings/riscv/extensions.yaml | 34 +++++++++++++++++++ >> 2 files changed, 39 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml >> index d87dd50f1a4b..c4e2c65437b1 100644 >> --- a/Documentation/devicetree/bindings/riscv/cpus.yaml >> +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml >> @@ -168,7 +168,7 @@ examples: >> i-cache-size = <16384>; >> reg = <0>; >> riscv,isa-base = "rv64i"; >> - riscv,isa-extensions = "i", "m", "a", "c"; >> + riscv,isa-extensions = "i", "m", "a", "c", "zca"; >> >> cpu_intc0: interrupt-controller { >> #interrupt-cells = <1>; >> @@ -194,7 +194,8 @@ examples: >> reg = <1>; >> tlb-split; >> riscv,isa-base = "rv64i"; >> - riscv,isa-extensions = "i", "m", "a", "f", "d", "c"; >> + riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca", >> + "zcd"; >> >> cpu_intc1: interrupt-controller { >> #interrupt-cells = <1>; >> @@ -215,7 +216,8 @@ examples: >> compatible = "riscv"; >> mmu-type = "riscv,sv48"; >> riscv,isa-base = "rv64i"; >> - riscv,isa-extensions = "i", "m", "a", "f", "d", "c"; >> + riscv,isa-extensions = "i", "m", "a", "f", "d", "c", "zca", >> + "zcd"; >> >> interrupt-controller { >> #interrupt-cells = <1>; >> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml >> index db7daf22b863..0172cbaa13ca 100644 >> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml >> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml >> @@ -549,6 +549,23 @@ properties: >> const: zca >> - contains: >> const: f >> + # C extension implies Zca >> + - if: >> + contains: >> + const: c >> + then: >> + contains: >> + const: zca >> + # C extension implies Zcd if d >> + - if: >> + allOf: >> + - contains: >> + const: c >> + - contains: >> + const: d >> + then: >> + contains: >> + const: zcd >> >> allOf: >> # Zcf extension does not exists on rv64 >> @@ -566,6 +583,23 @@ allOf: >> not: >> contains: >> const: zcf >> + # C extension implies Zcf if f on rv32 only >> + - if: >> + properties: >> + riscv,isa-extensions: >> + allOf: >> + - contains: >> + const: c >> + - contains: >> + const: f >> + riscv,isa-base: >> + contains: >> + const: rv32i >> + then: >> + properties: >> + riscv,isa-extensions: >> + contains: >> + const: zcf >> >> additionalProperties: true >> ... >> -- >> 2.43.0 >>