Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Nov 2022 06:19:11 +0100
From:   Andrew Jones <ajones@ventanamicro.com>
To:     Palmer Dabbelt <palmer@dabbelt.com>
Cc:     Conor Dooley <conor@kernel.org>, heiko@sntech.de,
        linux-riscv@lists.infradead.org,
        Conor Dooley <conor.dooley@microchip.com>, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org,
        Paul Walmsley <paul.walmsley@sifive.com>,
        aou@eecs.berkeley.edu, guoren@kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Sunil V L <sunilvl@ventanamicro.com>
Subject: Re: [PATCH 2/2] dt-bindings: riscv: fix single letter canonical order
Message-ID: <20221129051911.5xetsgfukxiljuhf@kamzik>
References: <Y4UJQYgCpnZJji9o@spud>
 <mhng-8141aa74-17c6-4692-a658-b5e4faad0c14@palmer-ri-x1c9a>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <mhng-8141aa74-17c6-4692-a658-b5e4faad0c14@palmer-ri-x1c9a>
Precedence: bulk

On Mon, Nov 28, 2022 at 03:41:19PM -0800, Palmer Dabbelt wrote:
> On Mon, 28 Nov 2022 11:17:21 PST (-0800), Conor Dooley wrote:
> > On Mon, Nov 28, 2022 at 10:12:17AM -0800, Palmer Dabbelt wrote:
> > > On Mon, 28 Nov 2022 10:08:05 PST (-0800), Conor Dooley wrote:
> > > > On Mon, Nov 28, 2022 at 09:41:03AM -0800, Palmer Dabbelt wrote:
> > > > > On Thu, 24 Nov 2022 05:42:20 PST (-0800), heiko@sntech.de wrote:
> > > > > > Am Donnerstag, 24. November 2022, 14:04:41 CET schrieb Conor Dooley:
> > > > > > > I used the wikipedia table for ordering extensions when updating the
> > > > > > > pattern here in foo.
> > > > > >
> > > > > > 	    ^ foo? :-)
> > > > > >
> > > > > > > Unfortunately that table did not match canonical order, as defined by
> > > > > > > the RISC-V ISA Manual, which defines extension ordering in (what is
> > > > > > > currently) Table 41, "Standard ISA extension names". Fix things up by
> > > > > > > re-sorting v (vector) and adding p (packed-simd) & j (dynamic
> > > > > > > languages). The e (reduced integer) and g (general) extensions are still
> > > > > > > intentionally left out.
> > > > > > >
> > > > > > > Link: https://github.com/riscv/riscv-isa-manual/releases/tag/riscv-unpriv-pdf-from-asciidoc-15112022 # Chapter 29.5
> > > > > > > Fixes: 299824e68bd0 ("dt-bindings: riscv: add new riscv,isa strings for emulators")
> > > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > >
> > > > > > So I have compared the new pattern to the isa manual,
> > > > > > and it looks like the order checks out, so
> > > > >
> > > > > Which ISA manual?
> > > >
> > > > For me, isa manual is the above github repo.
> > > 
> > > Which commit, though?
> > 
> > mutt won't let me paste a clown face emoticon.
> > 
> > > > > There have been many mutually incompatible ISA string
> > > > > encoding rules, at least one of them was a change to the extension ordering.
> > > > > It's not entirely clear what the right answer is here, as we can't really
> > > > > parse ISA strings without also knowing the version of the ISA manual we're
> > > > > meant to parse them against.  Maybe we just accept everything?
> > > >
> > > > I don't think accepting everything is the right thing to do. A minimal
> > > > amount of validation is still needed here, but I think we can deprecate
> > > > the DT property entirely & make it optional if a new-and-improved way of
> > > > encoding the in DT is used.
> > > 
> > > Sorry, by "everything" I meant "everything that's even been allowed by the
> > > ISA manual".  Just accetping anything would be bad ;)
> > > 
> > > > > IMO it's time to just stop using the ISA string.  It's not a stable
> > > > > interface, trying to treat it as such just leads to headaches.  We should
> > > > > just come up with some DT-specific way of encoding whatever HW features are
> > > > > in question.  Sure it'll be a bit of work to write that all down in the DT
> > > > > bindings, but it's going to be way less work than trying to keep around all
> > > > > this ISA string parsing code.
> > > >
> > > > I'm a glutton for punishment, I'll try and come up with some sort of
> > > > other way to encode this information in DT that requires less parsing
> > > > and validation. As I said on IRC, something that more resembles:
> > > > if (of_property_wahtever("riscv,isa-foo")) { do_enable_foo() }
> > > 
> > > That seems way simpler to me, thanks!  We'll still need to support whatever
> > > was here as a legacy format, but at least we won't need to add a bunch of
> > > new stuff to it -- that's where the parsing starts to get really
> > > complicated.

While it's easy for Linux to add new DT nodes when new extension support is
added, it's not so easy to add new ACPI objects. The current plan for ACPI
is to use the ISA string[1]. With that in mind, I think Linux should
continue to parse the string for DT as well.

[1] https://docs.google.com/document/d/1LlCefO_0GQ_7Tf3lzfMPETEfMlGo2FfLRJ09IMqJKEk/edit

> > 
> > Yah, and "deprecated" in dt-schema doesn't actually do anything at the
> > moment other than let humans know not to use something. Just gonna have
> > to do some sort of "feature-wise AND" between the existing things we
> > parse from the isa string & whatever riscv,isa-foo stuff later on.
> 
> I suppose this is more of a Rob question, but could we just make the DT
> bindings match the current ISA manual's rules and then have the kernel's
> "riscv,isa" string parser accept more orderings to remain compatible?
> 
> Sort of a API vs ABI stability question, but for DTB and bindngs.
> 
> > > FWIW, there's a similar dicussion going on in GCC land right now.
> > > 
> > > > > I know I've said the opposite before, but there's just been way too many
> > > > > breakages here to assume they're going to stop.
> > > >
> > > > :upside_down_face:
> > > >
> > > > Either way, I think these two patches are worth taking in the mean time.
> > > 
> > > Yep, just as long as it doesn't break any of the strings that were valid
> > > according to previous versions of the ISA manual I'm fine with it.
> > 
> > I don't think so. I had been looking around for a supposed order for
> > where to actually put H, which had been dropped - and the only place I
> > recall seeing that was Wikipedia - which now seems like an awful
> > decision since the order there looks kinda off anything I see in dozen
> > or so spec PDFs I have downloaded. But that's where I got the K & V
> > ordering from that I now think is wrong (and doesn't match any PDF I
> > have). The other changes relax rules and add letters so they should be
> > okay too.
> 
> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> as it sounds like there aren't even any fixed rules, so I guess none of this
> even matters?

IMHO, we should try to get the spec changed to say that the only order
which matters is that single letter extensions come first (in any order)
and then multi-letter extensions come after (in any order). Parsing
becomes simple and we can publish the string in proc in any order too,
maybe preferring alphabetical order for neatness.

Thanks,
drew

> 
> > 
> > > > > > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > > > > >
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/riscv/cpus.yaml | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/riscv/cpus.yaml b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > > > > > > index e80c967a4fa4..b7462ea2dbe4 100644
> > > > > > > --- a/Documentation/devicetree/bindings/riscv/cpus.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/riscv/cpus.yaml
> > > > > > > @@ -80,7 +80,7 @@ properties:
> > > > > > >        insensitive, letters in the riscv,isa string must be all
> > > > > > >        lowercase to simplify parsing.
> > > > > > >      $ref: "/schemas/types.yaml#/definitions/string"
> > > > > > > -    pattern: ^rv(?:64|32)imaf?d?q?c?b?v?k?h?(?:z(?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$
> > > > > > > +    pattern: ^rv(?:64|32)imaf?d?q?c?b?k?j?p?v?h?(?:z(?:[a-z])+)?(?:_[hsxz](?:[a-z])+)*$
> > > > > > >
> > > > > > >    # RISC-V requires 'timebase-frequency' in /cpus, so disallow it here
> > > > > > >    timebase-frequency: false
> > > > > > >