Re: [PATCH] dt-bindings: mtd: Add a schema for binman

[Simon Glass]

Hi Rob,

On Fri, 22 Sept 2023 at 13:43, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Fri, Sep 22, 2023 at 1:12 PM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> >
> > Hi Rob,
> >
> > On Fri, 22 Sept 2023 at 11:46, Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Sep 22, 2023 at 11:01:18AM -0600, Simon Glass wrote:
> > > > Hi Rob,
> > > >
> > > > On Fri, 22 Sept 2023 at 10:00, Rob Herring <robh@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass <sjg@xxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > Binman[1] is a tool for creating firmware images. It allows you to
> > > > > > combine various binaries and place them in an output file.
> > > > > >
> > > > > > Binman uses a DT schema to describe an image, in enough detail that
> > > > > > it can be automatically built from component parts, disassembled,
> > > > > > replaced, listed, etc.
> > > > > >
> > > > > > Images are typically stored in flash, which is why this binding is
> > > > > > targeted at mtd. Previous discussion is at [2] [3].
> > > > > >
> > > > > > [1] https://u-boot.readthedocs.io/en/stable/develop/package/binman.html
> > > > > > [2] https://lore.kernel.org/u-boot/20230821180220.2724080-3-sjg@xxxxxxxxxxxx/
> > > > > > [3] https://www.spinics.net/lists/devicetree/msg626149.html
> > > > >
> > > > > You missed:
> > > > >
> > > > > https://github.com/devicetree-org/dt-schema/pull/110
> > > > >
> > > > > where I said: We certainly shouldn't duplicate the existing partitions
> > > > > bindings. What's missing from them (I assume we're mostly talking
> > > > > about "fixed-partitions" which has been around forever I think (before
> > > > > me))?
> > > > >
> > > > > To repeat, unless there is some reason binman partitions conflict with
> > > > > fixed-partitions, you need to start there and extend it. From what's
> > > > > posted here, it neither conflicts nor needs extending.
> > > >
> > > > I think at this point I am just hopelessly confused. Have you taken a
> > > > look at the binman schema? [1]
> > >
> > > Why do I need to? That's used for some tool and has nothing to do with a
> > > device's DTB. However, I thought somewhere in this discussion you showed
> > > it under a flash device node.
> >
> > Yes, that is the intent (under a flash node).
> >
> > > Then I care because then it overlaps with
> > > what we already have for partitions. If I misunderstood that, then just
> > > put your schema with your tool. Only users of the tool should care about
> > > the tool's schema.
> >
> > OK. I believe that binman will fit into both camps, since its input is
> > not necessarily fully formed. E.g. if you don't specify the offset of
> > an entry, then it will be packed automatically. But the output is
> > fully formed, in that Binman now knows the offset so can write it to
> > the DT.
>
> I suppose it could take its own format as input and then write out
> something different for the "on the device" format (i.e.
> fixed-partitions). At least for the dynamic offsets, we may need
> something allowed for binman input, but not allowed on device. In
> general, there is support for partitions without addresses/offsets,
> but only for partitions that have some other way to figure that out
> (on disk partition info).
>
> There's also the image filename which doesn't really belong in the on
> device partitions. So maybe the input and output schemas should be
> separate.

OK, I'll focus on the output schema for now. I suspect this will be a
grey area though.

As an example, if you replace a binary in the firmware, Binman can
repack the firmware to make room, respecting the alignment and size
constraints. So these need to be in the output schema somehow.

>
> > > > I saw this file, which seems to extend a partition.
> > > >
> > > > Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml
> > >
> > > IIRC, that's a different type where partition locations are stored in
> > > the flash, so we don't need location and size in DT.
> >
> > OK.
> >
> > >
> > > >
> > > > I was assuming that I should create a top-level compatible = "binman"
> > > > node, with subnodes like compatible = "binman,bl31-atf", for example.
> > > > I should use the compatible string to indicate the contents, right?
> > >
> > > Yes for subnodes, and we already have some somewhat standard ones for
> > > "u-boot" and "u-boot-env". Though historically, "label" was used.
> >
> > Binman has common properties for all entries, including "compress"
> > which sets the compression algorithm.
>
> I see no issue with adding that. It seems useful and something missing
> in the existing partition schemas.

OK I sent a patch with that.

>
> > So perhaps I should start by defining a new binman,bl31-atf which has
> > common properties from an "binman,entry" definition?
>
> I don't understand the binman prefix. The contents are ATF (or TF-A
> now). Who wrote it to the flash image is not relevant.

Are you suggesting just "atf-bl31", or "arm,atf-bl31" ? Or should we
change it to "tfa-bl31"?

>
> We already have some compatibles in use. We should reuse them if
> possible. Not sure about TF-A though.

OK.

Regards,
Simon