Hi Miquel, On Mon, 25 Sept 2023 at 08:47, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > > Hi Simon, > > sjg@xxxxxxxxxxxx wrote on Mon, 25 Sep 2023 06:33:14 -0600: > > > Hi Miquel, > > > > On Mon, 25 Sept 2023 at 01:21, Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > > > > > > Hi Simon, > > > > > > sjg@xxxxxxxxxxxx wrote on Fri, 22 Sep 2023 13:51:14 -0600: > > > > > > > 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"? > > > > > > I don't really understand the relationship with TF-A here. Can't we > > > just have a kind of fixed-partitions with additional properties like > > > the compression? > > > > Binman needs to know what to put in there, which is the purpose of the > > compatible string. > > But "what" should be put inside the partition is part of the input > argument, not the output. You said (maybe I got this wrong) that the > schema would apply to the output of binman. If you want to let user > know what's inside, maybe it is worth adding a label, but otherwise I > don't like the idea of a compatible for that, which for me would mean: > "here is how to handle that specific portion of the flash/here is how > the flash is organized". But I thought that the compatible string was for that purpose? See for example "brcm,bcm4908-firmware" and "brcm,bcm963xx-imagetag" and "linksys,ns-firmware". What mechanism do you suggest be used here? > > > > > > We already have some compatibles in use. We should reuse them if > > > > > possible. Not sure about TF-A though. > > > > > > > > OK. > > > > > > > > > > Also, I still don't understand the purpose of this schema. So binman > > > generates an image, you want to flash this image and you would like the > > > tool to generate the corresponding (partition) DT snippet automatically. > > > Do I get this right? I don't get why you would need new compatibles for > > > that. > > > > It is actually the other way around. The schema tells Binman how to > > build the image (what goes in there and where). Then outputs an > > updated DT which describes where everything ended up, for use by other > > tools, e.g. firmware update. It is a closed loop in that sense. See > > the references for more information. > > Maybe I fail to see why you would want these description to be > introduced here, if they are not useful to the OS. Well I was asked to send them to Linux since they apparently don't belong in dt-schema. 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. > > > [1] https://u-boot.readthedocs.io/en/latest/develop/package/index.html > > [2] https://pretalx.com/media/osfc2019/submissions/Y7EN9V/resources/Binman_-_A_data-controlled_firmware_packer_for_U-B_pFU3n2K.pdf > > [3] https://www.youtube.com/watch?v=L84ujgUXBOQ > Regards, Simon
