| Message ID | 20230921124459.1.I91ddcfacf9b234af5cc3eabea4b62edb31153317@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | dt-bindings: mtd: Add a schema for binman | expand |
On 2023-09-21 21:45 +03:00, Simon Glass 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@chromium.org/ > [3] https://www.spinics.net/lists/devicetree/msg626149.html > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > .../bindings/mtd/partitions/binman.yaml | 50 +++++++++++++++ > .../bindings/mtd/partitions/binman/entry.yaml | 61 +++++++++++++++++++ > .../bindings/mtd/partitions/partitions.yaml | 1 + > MAINTAINERS | 5 ++ > 4 files changed, 117 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman.yaml > create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman/entry.yaml This doesn't match the schema in [2], but seems more like v1 of that. Is that intentional?
Hi Alper, On Fri, 22 Sept 2023 at 07:57, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote: > > On 2023-09-21 21:45 +03:00, Simon Glass 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@chromium.org/ > > [3] https://www.spinics.net/lists/devicetree/msg626149.html > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > .../bindings/mtd/partitions/binman.yaml | 50 +++++++++++++++ > > .../bindings/mtd/partitions/binman/entry.yaml | 61 +++++++++++++++++++ > > .../bindings/mtd/partitions/partitions.yaml | 1 + > > MAINTAINERS | 5 ++ > > 4 files changed, 117 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman.yaml > > create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman/entry.yaml > > This doesn't match the schema in [2], but seems more like v1 of that. Is > that intentional? Yes. Based on discussions with Rob, the idea of setting a general format seems to be too ambitious, at least for now. Regards, Simon
On Thu, Sep 21, 2023 at 1:45 PM Simon Glass <sjg@chromium.org> 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@chromium.org/ > [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 did a bit more research. "fixed-partitions" as a compatible has "only" been around since 2015. Prior to that, it was implicit with just partition nodes with addresses (i.e. reg) and that dates back to 2007. Looks like u-boot only supports the newer form and since 2021. Rob
Hi Rob, On Fri, 22 Sept 2023 at 11:46, Rob Herring <robh@kernel.org> 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@kernel.org> wrote: > > > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass <sjg@chromium.org> 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@chromium.org/ > > > > [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 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. So perhaps I should start by defining a new binman,bl31-atf which has common properties from an "binman,entry" definition? > > Top-level, meaning the root of the DT? That sound like just something > for the tool, so I don't care, but it doesn't belong in the DTB. Sorry, I mean 'top-level' with respect to the partitions. > > > > > Re extending, what is the minimum I can do? Are you looking for > > something like a "compress" property that indicates that the entry is > > compressed? > > > > I'm really just a bit lost. > > Me too. Regards, Simon
On Fri, Sep 22, 2023 at 1:12 PM Simon Glass <sjg@chromium.org> wrote: > > Hi Rob, > > On Fri, 22 Sept 2023 at 11:46, Rob Herring <robh@kernel.org> 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@kernel.org> wrote: > > > > > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass <sjg@chromium.org> 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@chromium.org/ > > > > > [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. > > > 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. > 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. We already have some compatibles in use. We should reuse them if possible. Not sure about TF-A though. Rob
Hi Rob, On Fri, 22 Sept 2023 at 13:43, Rob Herring <robh@kernel.org> wrote: > > On Fri, Sep 22, 2023 at 1:12 PM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Rob, > > > > On Fri, 22 Sept 2023 at 11:46, Rob Herring <robh@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass <sjg@chromium.org> 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@chromium.org/ > > > > > > [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
Hi Simon, sjg@chromium.org wrote on Fri, 22 Sep 2023 13:51:14 -0600: > Hi Rob, > > On Fri, 22 Sept 2023 at 13:43, Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Sep 22, 2023 at 1:12 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Rob, > > > > > > On Fri, 22 Sept 2023 at 11:46, Rob Herring <robh@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass <sjg@chromium.org> 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@chromium.org/ > > > > > > > [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? > > 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. Thanks, Miquèl
Hi Simon, sjg@chromium.org wrote on Mon, 25 Sep 2023 06:33:14 -0600: > Hi Miquel, > > On Mon, 25 Sept 2023 at 01:21, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Simon, > > > > sjg@chromium.org wrote on Fri, 22 Sep 2023 13:51:14 -0600: > > > > > Hi Rob, > > > > > > On Fri, 22 Sept 2023 at 13:43, Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Fri, Sep 22, 2023 at 1:12 PM Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > Hi Rob, > > > > > > > > > > On Fri, 22 Sept 2023 at 11:46, Rob Herring <robh@kernel.org> 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@kernel.org> wrote: > > > > > > > > > > > > > > > > On Thu, Sep 21, 2023 at 1:45 PM Simon Glass <sjg@chromium.org> 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@chromium.org/ > > > > > > > > > [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". > > > > 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. > [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 Thanks, Miquèl
Hi Miquel, On Mon, 25 Sept 2023 at 09:24, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Simon, > > > > > > > > > > > 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". > > These three examples apparently need specific handling, the partitions > contain meta-data that a parser needs to check or something like that. > And finally it looks like partition names are set depending on the > content that was discovered, so yes, the partition name is likely the > good location to tell users/OSes what's inside. > > > > > > 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. > > 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. So you are suggesting 'label' for the contents. Rob suggested 'compatible' [1], so what should I do? With this schema, would every node be called 'partition@...' or is there flexibility to use other names? One other point to note is that some entry types will eventually need other properties, which vary depending on the type. For example a signature entry will need to hold the algorithm name used to generate (and therefore at runtime check) the signature. > > > > > [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 Regards, Simon [1] https://github.com/devicetree-org/dt-schema/pull/110
On Mon, Sep 25, 2023 at 11:25 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Miquel, > > On Mon, 25 Sept 2023 at 09:24, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Simon, > > > > > > > > > > > > > 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". > > > > These three examples apparently need specific handling, the partitions > > contain meta-data that a parser needs to check or something like that. > > And finally it looks like partition names are set depending on the > > content that was discovered, so yes, the partition name is likely the > > good location to tell users/OSes what's inside. > > > > > > > > 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. That is not what I said. I said fixed-partitions should be extended. I prefer they are extended in-place before moving them rather than the other way around. > > > 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". > 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. > With this schema, would every node be called 'partition@...' or is > there flexibility to use other names? The preference is to use generic names. Do you mean without a unit-address or different from "partition"? The need for the input side of binman to have dynamic addresses seems like the biggest issue. That's allowed in other cases with "partition-N" or "partition-foo" IIRC. I don't think we want to allow that for "fixed-partitions" at least in the DTB (i.e. the output side of binman). Rob
Hi Rob, On Mon, 25 Sept 2023 at 12:49, Rob Herring <robh@kernel.org> wrote: > > On Mon, Sep 25, 2023 at 11:25 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Miquel, > > > > On Mon, 25 Sept 2023 at 09:24, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Simon, > > > > > > > > > > > > > > > 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". > > > > > > These three examples apparently need specific handling, the partitions > > > contain meta-data that a parser needs to check or something like that. > > > And finally it looks like partition names are set depending on the > > > content that was discovered, so yes, the partition name is likely the > > > good location to tell users/OSes what's inside. > > > > > > > > > > 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. > > That is not what I said. I said fixed-partitions should be extended. I > prefer they are extended in-place before moving them rather than the > other way around. OK. > > > > > 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. 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. 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. > > > > 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'. > > > With this schema, would every node be called 'partition@...' or is > > there flexibility to use other names? > > The preference is to use generic names. Do you mean without a > unit-address or different from "partition"? The need for the input > side of binman to have dynamic addresses seems like the biggest issue. > That's allowed in other cases with "partition-N" or "partition-foo" > IIRC. I don't think we want to allow that for "fixed-partitions" at > least in the DTB (i.e. the output side of binman). OK I suppose this is the problem with starting small. I was hoping to build up the schema piece by piece but now I am wondering whether every little detail will get redirected and I'll end up with something that Binman cannot use. So far all I have is that I can add a 'compress' property and a 'compatible' which describes the contents. I suppose it is a start. Regards, Simon
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. > 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. > > 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. > > > > > > > > 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. 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"; I don't see why, for the binman schema, we could not constrain the labels? > > > With this schema, would every node be called 'partition@...' or is > > > there flexibility to use other names? > > > > The preference is to use generic names. Do you mean without a > > unit-address or different from "partition"? The need for the input > > side of binman to have dynamic addresses seems like the biggest issue. > > That's allowed in other cases with "partition-N" or "partition-foo" > > IIRC. I don't think we want to allow that for "fixed-partitions" at > > least in the DTB (i.e. the output side of binman). > > OK I suppose this is the problem with starting small. I was hoping to > build up the schema piece by piece but now I am wondering whether > every little detail will get redirected and I'll end up with something > that Binman cannot use. > > So far all I have is that I can add a 'compress' property and a > 'compatible' which describes the contents. I suppose it is a start. I guess defining all you need in one go would be better. At least showing a full and typical example might help. But some items like encoding if you have TF-A or U-Boot in the compatible, I'm far from convinced... Thanks, Miquèl
On Tue, Sep 26, 2023 at 2:48 AM Miquel Raynal <miquel.raynal@bootlin.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. > 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. Rob
Hi again Rob, On Wed, 27 Sept 2023 at 10:43, Simon Glass <sjg@chromium.org> wrote: > > Hi Rob, > > On Tue, 26 Sept 2023 at 11:29, Rob Herring <robh@kernel.org> wrote: > > > > On Tue, Sep 26, 2023 at 2:48 AM Miquel Raynal <miquel.raynal@bootlin.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
diff --git a/Documentation/devicetree/bindings/mtd/partitions/binman.yaml b/Documentation/devicetree/bindings/mtd/partitions/binman.yaml new file mode 100644 index 00000000000000..c792d5a37b700a --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/partitions/binman.yaml @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2023 Google LLC + +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mtd/partitions/binman.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Binman firmware layout + +maintainers: + - Simon Glass <sjg@chromium.org> + +description: | + The binman node provides a layout for firmware, used when packaging firmware + from multiple projects. For now it just supports a very simple set of + features, as a starting point for discussion. + + Documentation for Binman is available at: + + https://u-boot.readthedocs.io/en/latest/develop/package/binman.html + + with the current image-description format at: + + https://u-boot.readthedocs.io/en/latest/develop/package/binman.html#image-description-format + +properties: + compatible: + const: u-boot,binman + +required: + - compatible + +additionalProperties: false + +examples: + - | + firmware { + binman { + compatible = "u-boot,binman"; + + u-boot { + size = <0xa0000>; + }; + + atf-bl31 { + offset = <0x100000>; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/mtd/partitions/binman/entry.yaml b/Documentation/devicetree/bindings/mtd/partitions/binman/entry.yaml new file mode 100644 index 00000000000000..8003eb4f1a994f --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/partitions/binman/entry.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2023 Google LLC + +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mtd/partitions/binman/entry.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Binman entry + +maintainers: + - Simon Glass <sjg@chromium.org> + +description: | + The entry node specifies a single entry in the firmware. + + Entries have a specific type, such as "u-boot" or "atf-bl31". If the type + is missing, the name is used as the type. + + Note: This definition is intended to be hierarchical, so that entries can + appear in other entries. Schema for that is TBD. + +properties: + $nodename: + pattern: "^[-a-z]+(-[0-9]+)?$" + + type: + $ref: /schemas/types.yaml#/definitions/string + + offset: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Provides the offset of this entry from the start of its parent section. + If this is omitted, Binman will determine this by packing the enclosing + section according to alignment rules, etc. + + size: + $ref: /schemas/types.yaml#/definitions/uint32 + description: | + Provides the size of this entry in bytes. If this is omitted, Binman will + use the content size, along with any alignment information, to determine + the size of the entry. + +additionalProperties: false + +examples: + - | + firmware { + binman { + compatible = "u-boot,binman"; + + u-boot { + size = <0xa0000>; + }; + + second-area { + type = "atf-bl31"; + offset = <0x100000>; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml index 1dda2c80747bd7..849fd15d085ccc 100644 --- a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml +++ b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml @@ -15,6 +15,7 @@ maintainers: oneOf: - $ref: arm,arm-firmware-suite.yaml + - $ref: binman.yaml - $ref: brcm,bcm4908-partitions.yaml - $ref: brcm,bcm947xx-cfe-partitions.yaml - $ref: fixed-partitions.yaml diff --git a/MAINTAINERS b/MAINTAINERS index a4c30221eb305d..ebcbfb4292e8dc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3505,6 +3505,11 @@ F: Documentation/filesystems/bfs.rst F: fs/bfs/ F: include/uapi/linux/bfs_fs.h +BINMAN +M: Simon Glass <sjg@chromium.org> +S: Supported +F: Documentation/devicetree/bindings/mtd/partitions/binman* + BITMAP API M: Yury Norov <yury.norov@gmail.com> R: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
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@chromium.org/ [3] https://www.spinics.net/lists/devicetree/msg626149.html Signed-off-by: Simon Glass <sjg@chromium.org> --- .../bindings/mtd/partitions/binman.yaml | 50 +++++++++++++++ .../bindings/mtd/partitions/binman/entry.yaml | 61 +++++++++++++++++++ .../bindings/mtd/partitions/partitions.yaml | 1 + MAINTAINERS | 5 ++ 4 files changed, 117 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman.yaml create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman/entry.yaml