Re: [PATCH v2 1/3] dt-bindings: mtd: fixed-partitions: Add binman compatible

[Simon Glass]

Hi Michael,

On Thu, 5 Oct 2023 at 07:28, Simon Glass <sjg@xxxxxxxxxxxx> wrote:
>
> Hi Michael,
>
> On Thu, 5 Oct 2023 at 02:54, Michael Walle <mwalle@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > >> >> >> Add a compatible string for binman, so we can extend fixed-partitions
> > >> >> >> in various ways.
> > >> >> >
> > >> >> > I've been thinking at the proper way to describe the binman partitions.
> > >> >> > I am wondering if we should really extend the fixed-partitions
> > >> >> > schema. This description is really basic and kind of supposed to remain
> > >> >> > like that. Instead, I wonder if we should not just keep the binman
> > >> >> > compatible alone, like many others already. This way it would be very clear
> > >> >> > what is expected and allowed in both cases. I am thinking about
> > >> >> > something like that:
> > >> >> >
> > >> >> >       Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml
> > >> >> >
> > >> >> > this file is also referenced there (but this patch does the same, which
> > >> >> > is what I'd expect):
> > >> >> >
> > >> >> >       Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > >> >> >
> > >> >> > I'll let the binding maintainers judge whether they think it's
> > >> >> > relevant, it's not a strong opposition.
> > >> >>
> > >> >> What is the overall goal here? To replace the current binman node
> > >> >> which is
> > >> >> usually contained in the -u-boot.dtsi files? If one is using binman to
> > >> >> create an image, is it expected that one needs to adapt the DT in
> > >> >> linux?
> > >> >> Or will it still be a seperate -u-boot.dtsi? > Because in the latter
> > >> >> case
> > >> >> I see that there will be conflicts because you have to overwrite the
> > >> >> flash node. Or will it be a seperate node with all the information
> > >> >> duplicated?
> > >> >
> > >> > The goal is simply to have a full binding for firmware layout, such
> > >> > that firmware images can be created, examined and updated. The
> > >> > -u-boot.dtsi files are a stopgap while we sort out a real binding.
> > >> > They should eventually go away.
> > >>
> > >> You haven't answered whether this node should be a seperate binman
> > >> node - or if you'll reuse the existing flash (partitions) node(s) and
> > >> add any missing property there. If it's the latter, I don't think
> > >> compatible = "binman", "fixed-partitions"; is correct.
> > >
> > > My intent is to make it compatible, so wouldn't it make sense to have
> > > binman as the first compatible, then falling back to fixed-partitions
> > > as the second?
> >
> > As far as I know, the compatibles should get more specific with each
> > string.
>
> That's the opposite to what I understood.
>
> > But "binman" seems to be used as a kind of tag which could be
> > added to any compatible under the flash node. What if one wants to build
> > an image which isn't compatible = "fixed-partitions"? E.g.
> > "linksys,ns-partitions", will it then have
> > compatible = "binman", "linksys,ns-partitions"?
>
> I suppose so.
>
> >
> >
> > >> >> Maybe (a more complete) example would be helpful.
> > >> >
> > >> > Can you please be a bit more specific? What is missing from the
> > >> > example?
> > >>
> > >> Like a complete (stripped) DTS. Right now I just see how the
> > >> individual
> > >> node looks like. But with a complete example DTS, my question from
> > >> above
> > >> would have been answered.
> >
> > So to give an example myself, please correct it if it's wrong. From
> > our board (kontron-sl28):
> >
> > &fspi {
> >          status = "okay";
> >
> >          flash@0 {
> >                  compatible = "jedec,spi-nor";
> >                  m25p,fast-read;
> >                  spi-max-frequency = <133000000>;
> >                  reg = <0>;
> >                  /* The following setting enables 1-1-2 (CMD-ADDR-DATA)
> > mode */
> >                  spi-rx-bus-width = <2>; /* 2 SPI Rx lines */
> >                  spi-tx-bus-width = <1>; /* 1 SPI Tx line */
> >
> >                  partitions {
> >                          compatible = "fixed-partitions";
> >                          #address-cells = <1>;
> >                          #size-cells = <1>;
> >
> >                          partition@0 {
> >                                  reg = <0x000000 0x010000>;
> >                                  label = "rcw";
> >                                  read-only;
> >                          };
> >
> >                          partition@10000 {
> >                                  reg = <0x010000 0x1d0000>;
> >                                  label = "failsafe bootloader";
> >                                  read-only;
> >                          };
> >
> >                          partition@200000 {
> >                                  reg = <0x200000 0x010000>;
> >                                  label = "configuration store";
> >                          };
> >
> >                          partition@210000 {
> >                                  reg = <0x210000 0x1d0000>;
> >                                  label = "bootloader";
> >                          };
> >
> >                          partition@3e0000 {
> >                                  reg = <0x3e0000 0x020000>;
> >                                  label = "bootloader environment";
> >                          };
> >                  };
> >          };
> > };
> >
> > In u-boot we use binman, see
> > arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi
> > in the u-boot repository.
> >
> > Now to use the new method, am I expected to adapt the dts in the
> > linux kernel? As far as I understand that is the case. So that node
> > from above would look something like the following:
> >
> > &fspi {
> >          status = "okay";
> >
> >          flash@0 {
> >                  compatible = "jedec,spi-nor";
> >                  m25p,fast-read;
> >                  spi-max-frequency = <133000000>;
> >                  reg = <0>;
> >                  /* The following setting enables 1-1-2 (CMD-ADDR-DATA)
> > mode */
> >                  spi-rx-bus-width = <2>; /* 2 SPI Rx lines */
> >                  spi-tx-bus-width = <1>; /* 1 SPI Tx line */
> >
> >                  partitions {
> >                          compatible = "binman", "fixed-partitions";
> >                          #address-cells = <1>;
> >                          #size-cells = <1>;
> > [..]
> >                          partition@210000 {
> >                                  reg = <0x210000 0x1d0000>;
> >                                  label = "u-boot"; /* or "u-boot+atf" ?
> > */
> >                          };
> >
> >                          partition@3e0000 {
> >                                  reg = <0x3e0000 0x020000>;
> >                                  label = "bootloader environment";
> >                          };
> >                  };
> >          };
> > };
> >
> > I'm still not sure why that compatible is needed. Also I'd need to
> > change
> > the label which might break user space apps looking for that specific
> > name.
> >
> > Also, our board might have u-boot/spl or u-boot/spl/bl31/bl32, right now
> > that's something which depends on an u-boot configuration variable,
> > which
> > then enables or disables binman nodes in the -u-boot.dtsi. So in linux
> > we only have that "bootloader" partition, but there might be either
> > u-boot+spl or u-boot+spl+bl31+bl32.
> >
> > Honestly, I'm really not sure this should go into a device tree.
>
> I think we might be getting a bit ahead of ourselves here. I thought
> that the decision was that the label should indicate the contents. If
> you have multiple things in a partition then it would become a
> 'section' in Binman's terminology. Either the label programmatically
> describes what is inside or it doesn't. We can't have it both ways.
> What do you suggest?
>
> At present it seems you have the image described in two places - one
> is the binman node and the other is the partitions node. I would like
> to unify these.

I should also mention that I originally proposed a binman in the
/firmware node, but Rob indicated that the /firmware node is not for
that sort of purpose.

>
> What does user space do with the partition labels?
>
> >
> > >> What if a board uses eMMC to store the firmware binaries? Will that
> > >> then
> > >> be a subnode to the eMMC device?
> > >
> > > I thought there was a way to link the partition nodes and the device
> > > using a property, without having the partition info as a subnode of
> > > the device. But I may have imagined it as I cannot find it now. So
> > > yes, it will be a subnode of the eMMC device.
> >
> > Not sure if that will fly.
>
> I can't find it anyway. There is somelike like that in
> simple-framebuffer with the 'display' property.

Regards,
Simon