Message-ID: <CAPnjgZ3TJLaRzv_cXyLdpRBtc2-FBYQ=gk-_MhEDH47HvO71FQ@mail.gmail.com>
Date:   Wed, 27 Sep 2023 10:43:45 -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 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?
>
> > > 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?

>
>
> > 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