Message-ID: <CAPnjgZ2eUcgevpS7Ak5p=7bhm0aHW3taLO8Fq_EcpDDQidJFVA@mail.gmail.com>
Date:   Thu, 28 Sep 2023 09:20:05 -0600
From:   Simon Glass <sjg@...omium.org>
To:     Rob Herring <robh@...nel.org>
Cc:     Miquel Raynal <miquel.raynal@...tlin.com>,
        devicetree@...r.kernel.org,
        U-Boot Mailing List <u-boot@...ts.denx.de>,
        linux-mtd@...ts.infradead.org, Tom Rini <trini@...sulko.com>,
        Conor Dooley <conor+dt@...nel.org>,
        Dhruva Gole <d-gole@...com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rafał Miłecki <rafal@...ecki.pl>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: mtd: Add a schema for binman

Hi again Rob,

On Wed, 27 Sept 2023 at 10:43, Simon Glass <sjg@...omium.org> wrote:
>
> Hi Rob,
>
> On Tue, 26 Sept 2023 at 11:29, Rob Herring <robh@...nel.org> wrote:
> >
> > On Tue, Sep 26, 2023 at 2:48 AM Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > >
> > > Hello,
> > >
> > > > > > > > These are firmware bindings, as indicated, but I
> > > > > > > > took them out of the /firmware node since that is for a different
> > > > > > > > purpose. Rob suggested that partitions was a good place. We have fwupd
> > > > > > > > using DT to hold the firmware-update information, so I expect it will
> > > > > > > > move to use these bindings too.
> > > > > > >
> > > > > > > I would definitely use fixed partitions as that's what you need then:
> > > > > > > registering where everything starts and ends. If you have "in-band"
> > > > > > > meta data you might require a compatible, but I don't think you
> > > > > > > do, in this case you should probably carry the content through a label
> > > > > > > (which will become the partition name) and we can discuss additional
> > > > > > > properties if needed.
> > > > > >
> > > > > > I believe I am going to need a compatible string at the 'partitions'
> > > > > > level to indicate that this is the binman scheme. But we can leave
> > > > > > that until later.
> > > > >
> > > > > Perhaps:
> > > > >
> > > > > compatible = "binman", "fixed-partitions";
> > > > >
> > > > > Though I don't understand why binman couldn't just understand what
> > > > > "fixed-partitions" means rather than "binman".
> > > >
> > > > Well so long as we don't add any binman things in here, you are right.
> > > >
> > > > But the eventual goal is parity with current Binman functionality,
> > > > which writes the entire (augmented) description to the DT, allowing
> > > > tools to rebuild / repack / replace pieces later, maintaining the same
> > > > alignment constraints, etc. I am assuming that properties like 'align
> > > > = <16>' would not fit with fixed-partitions.
> > >
> > > I am personally not bothered by this kind of properties. But if we plan
> > > on adding too much properties, I will advise to indeed use another name
> > > than fixed-partitions (or add the "binman" secondary compatible)
> > > otherwise it's gonna be hard to support in the code while still
> > > restraining as much as we can the other partition schema.
> >
> > Agreed. It's a trade off. I think we need enough to understand the
> > problem (not just presented with a solution), agree on the general
> > solution/direction, and then discuss specific additions.
> >
> > > > But if we don't preserve
> > > > these properties then Binman cannot do repacking reliably. Perhaps for
> > > > now I could put the augmented DT in its own section somewhere, but I
> > > > am just not sure if that will work in a real system. E.g. with VBE the
> > > > goal is to use the DT to figure out how to access the firmware, update
> > > > it, etc.
> >
> > VBE?

Verified Boot for Embedded, an EFI alternative with no callbacks.

> >
> > > > Is it not possible to have my own node with whatever things Binman
> > > > needs in it (subject to review of course)? i.e. could we discuss how
> > > > to encode it, but argue less about whether things are needed? I
> > > > kind-of feel I know what is needed, since I wrote the tool.
> >
> > What we don't need is the same information in 2 places for the DTB
> > used at runtime. If the binman node is removed, do whatever you want.
> > If you want to keep it at runtime, then it's got to extend what we
> > already have.
> >
> > I don't think anyone is disagreeing about whether specific information
> > is needed or not.
> >
> > > > > > So you are suggesting 'label' for the contents. Rob suggested
> > > > > > 'compatible' [1], so what should I do?
> > > > >
> > > > > "label" is for consumption by humans, not tools/software. Compatible
> > > > > values are documented, label values are not. Though the partition
> > > > > stuff started out using label long ago and it's evolved to preferring
> > > > > compatible.
> > > >
> > > > OK so we are agreed that we are going with 'compatible'.
> > >
> > > Still strongly disagree here.
> >
> > Miquel is right. I was confused here. "label" is still pretty much
> > used for what the image is. Though we do have "u-boot,env" for both it
> > seems.
> >
> > My position on "label" stands. To the extent we have images for common
> > components, I think we should standardize the names. Certainly if
> > tools rely on the names, then they should be documented.
>
> OK thanks for clearing that up.
>
> But at present 'label' is free-form text. If I change it to an enum,
> won't that break things? If not, how do I actually do it?
>
> There is a u-boot.yaml but it doesn't actually have a "u-boot" label
> in the schema. In fact it seems that the label is not validated at
> all?

It looks like I can just add it to a separate schema file, which is
what I did in the latest version.

>
> >
> >
> > > My understanding is that a compatible carries how the content is
> > > organized, and how this maybe specific (like you have in-band meta data
> > > data that needs to be parsed in a specific way or in your case
> > > additional specific properties in the DT which give more context about
> > > how the data is stored). But the real content of the partition, ie. if
> > > it contains a firmware, the kernel or some user data does not belong to
> > > the compatible.
> > >
> > > I.e:
> > > - The first byte of my partition gives the compression algorithm:
> > >   -> compatible = "compressed-partition-foo";
> > >      or
> > >   -> compatible = "fixed-partitions" + compression-algorithm = "foo";
> > > - The partition contains a picture of my dog:
> > >   -> label = "my dog is beautiful"
> > >   but certainly not
> > >   -> compatible = "my-dog";
> >
> > IMO, compatible in this case should convey "JPEG image" or similar.
> >
> > > I don't see why, for the binman schema, we could not constrain the
> > > labels?
> >
> > Yes, but those should follow what we already have. "u-boot" for
> > example rather than "data,u-boot" which I think Simon had in some
> > version of this.
>
> Yes, don't worry, I had some feedback from Alper but have given up on
> that approach.

Regards,
Simon

Powered by blists - more mailing lists