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

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

[Simon Glass]

Add a compatible string for binman, so we can extend fixed-partitions
in various ways.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Drop mention of 'enhanced features' in fixed-partitions.yaml
- Mention Binman input and output properties
- Use plain partition@xxx for the node name

 .../bindings/mtd/partitions/binman.yaml       | 61 +++++++++++++++++++
 .../mtd/partitions/fixed-partitions.yaml      |  3 +
 .../bindings/mtd/partitions/partitions.yaml   |  1 +
 MAINTAINERS                                   |  5 ++
 4 files changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman.yaml

diff --git a/Documentation/devicetree/bindings/mtd/partitions/binman.yaml b/Documentation/devicetree/bindings/mtd/partitions/binman.yaml
new file mode 100644
index 000000000000..844f241feebf
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/partitions/binman.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.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binman firmware layout
+
+maintainers:
+  - Simon Glass <sjg@chromium.org>
+
+select: false
+
+description: |
+  The binman node provides a layout for firmware, used when packaging firmware
+  from multiple projects. It is based on fixed-partitions, with some
+  extensions.
+
+  Binman supports properties used as inputs to the firmware-packaging process,
+  such as those which control alignment of partitions. This binding addresses
+  these 'input' properties. For example, it is common for the 'reg' property
+  (an 'output' property) to be set by Binman, based on the alignment requested
+  in the input.
+
+  Once processing is complete, input properties have mostly served their
+  purpose, at least until the firmware is repacked later, e.g. due to a
+  firmware update. The base 'fixed-partitions' binding should provide enough
+  information to read the firmware at runtime, including decompression if
+  needed.
+
+  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
+
+allOf:
+  - $ref: /schemas/mtd/partitions/fixed-partitions.yaml#
+
+properties:
+  compatible:
+    const: binman
+
+additionalProperties: false
+
+examples:
+  - |
+    partitions {
+        compatible = "binman", "fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@100000 {
+            label = "u-boot";
+            reg = <0x100000 0xf00000>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
index 331e564f29dc..85aef1572967 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
@@ -24,6 +24,9 @@ properties:
       - items:
           - const: sercomm,sc-partitions
           - const: fixed-partitions
+      - items:
+          - const: binman
+          - const: fixed-partitions
 
   "#address-cells": true
 
diff --git a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
index 1dda2c80747b..849fd15d085c 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 5f18c6ba3c3c..367c843ec348 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3517,6 +3517,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>
-- 
2.42.0.582.g8ccd20d70d-goog

[PATCH v2 2/3] dt-bindings: mtd: binman-partition: Add binman labels

[Simon Glass]

Add two labels for binman entries, as a starting point for the schema.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Use plain partition@xxx for the node name

 .../mtd/partitions/binman-partition.yaml      | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml

diff --git a/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml b/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
new file mode 100644
index 000000000000..406a8997d3e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
@@ -0,0 +1,48 @@
+# 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-partition.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binman partition
+
+maintainers:
+  - Simon Glass <sjg@chromium.org>
+
+select: false
+
+description: |
+  This corresponds to a binman 'entry'. It is a single partition which holds
+  data of a defined type.
+
+allOf:
+  - $ref: /schemas/mtd/partitions/partition.yaml#
+
+properties:
+  label:
+    items:
+      enum:
+        - u-boot       # u-boot.bin from U-Boot projec6t
+        - atf-bl31     # bl31.bin or bl31.elf from TF-A project
+
+additionalProperties: false
+
+examples:
+  - |
+    partitions {
+        compatible = "binman", "fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        partition@100000 {
+            label = "u-boot";
+            reg = <0x100000 0xf00000>;
+        };
+
+        partition@200000 {
+            label = "atf-bl31";
+            reg = <0x200000 0x100000>;
+        };
+    };
-- 
2.42.0.582.g8ccd20d70d-goog

[PATCH v2 3/3] dt-bindings: mtd: binman-partitions: Add alignment properties

[Simon Glass]

Add three properties for controlling alignment of partitions, aka
'entries' in binman.

For now there is no explicit mention of hierarchy, so a 'section' is
just the 'fixed-partitions' node.

These new properties are inputs to the packaging process, but are also
needed if the firmware is repacked, to ensure that alignment
constraints are not violated. Therefore they are provided as part of
the schema.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Fix 'a' typo in commit message

 .../mtd/partitions/binman-partition.yaml      | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml b/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
index 406a8997d3e4..be7273a4c9ac 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
@@ -27,6 +27,42 @@ properties:
         - u-boot       # u-boot.bin from U-Boot projec6t
         - atf-bl31     # bl31.bin or bl31.elf from TF-A project
 
+  align:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      This sets the alignment of the entry. The entry offset is adjusted
+      so that the entry starts on an aligned boundary within the containing
+      section or image. For example ‘align = <16>’ means that the entry will
+      start on a 16-byte boundary. This may mean that padding is added before
+      the entry. The padding is part of the containing section but is not
+      included in the entry, meaning that an empty space may be created before
+      the entry starts. Alignment should be a power of 2. If ‘align’ is not
+      provided, no alignment is performed.
+
+  align-size:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      This sets the alignment of the entry size. For example, to ensure
+      that the size of an entry is a multiple of 64 bytes, set this to 64.
+      While this does not affect the contents of the entry within binman
+      itself (the padding is performed only when its parent section is
+      assembled), the end result is that the entry ends with the padding
+      bytes, so may grow. If ‘align-size’ is not provided, no alignment is
+      performed.
+
+  align-end:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      This sets the alignment of the end of an entry with respect to the
+      containing section. Some entries require that they end on an alignment
+      boundary, regardless of where they start. This does not move the start
+      of the entry, so the contents of the entry will still start at the
+      beginning. But there may be padding at the end. While this does not
+      affect the contents of the entry within binman itself (the padding is
+      performed only when its parent section is assembled), the end result is
+      that the entry ends with the padding bytes, so may grow. If ‘align-end’
+      is not provided, no alignment is performed.
+
 additionalProperties: false
 
 examples:
@@ -39,10 +75,13 @@ examples:
         partition@100000 {
             label = "u-boot";
             reg = <0x100000 0xf00000>;
+            align-size = <0x1000>;
+            align-end = <0x10000>;
         };
 
         partition@200000 {
             label = "atf-bl31";
             reg = <0x200000 0x100000>;
+            align = <0x4000>;
         };
     };
-- 
2.42.0.582.g8ccd20d70d-goog

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

[Miquel Raynal]

Hi Simon,

sjg@chromium.org wrote on Mon,  2 Oct 2023 11:49:40 -0600:

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

> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---

[...]

> +properties:
> +  compatible:
> +    const: binman

Right now this does not fit (I believe) the example. But if we no
longer extend fixed-partitions but just create binman.yaml, this will
probably be enough.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    partitions {
> +        compatible = "binman", "fixed-partitions";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        partition@100000 {
> +            label = "u-boot";
> +            reg = <0x100000 0xf00000>;
> +        };
> +    };

Thanks,
Miquèl

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

[Michael Walle]

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?

Maybe (a more complete) example would be helpful.

-michael

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

[Simon Glass]

Hi Michael,

On Wed, 4 Oct 2023 at 05:35, Michael Walle <mwalle@kernel.org> 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.

>
> Maybe (a more complete) example would be helpful.

Can you please be a bit more specific? What is missing from the example?

Regards,
Simon

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

[Michael Walle]

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.

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

What if a board uses eMMC to store the firmware binaries? Will that then
be a subnode to the eMMC device?

-michael

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

[Simon Glass]

Hi Michael,

On Wed, 4 Oct 2023 at 11:17, Michael Walle <mwalle@kernel.org> 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?

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

Regards,
Simon

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

[Michael Walle]

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. 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"?


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

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

-michael

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

[Simon Glass]

Hi Michael,

On Thu, 5 Oct 2023 at 02:54, Michael Walle <mwalle@kernel.org> 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.

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

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@chromium.org> wrote:
>
> Hi Michael,
>
> On Thu, 5 Oct 2023 at 02:54, Michael Walle <mwalle@kernel.org> 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

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

[Michael Walle]

Hi,

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

As Rob pointed out earlier, it's just a user-facing string. I'm a bit
reluctant to use it programatically.
Taking my example again, the string "bootloader" is sufficient for a
user. He doesn't care if it's u-boot with spl or u-boot with tfa, or
even coreboot. It just says, "in this partition is the bootloader".
If you have an "bootloader" image you can flash it there.

If it has a label "u-boot" and I want to switch to coreboot, will
it have to change to "coreboot"? I really don't think this is practical,
you are really putting software configuration into the device tree.

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

And I'm not sure that will work for all the corner cases :/

If you keep the binman section seperate from the flash partition
definition you don't have any of these problems, although there is
some redundancy:
  - you only have compatible = "binman", "fixed-partition", no further
    compatibles are required
  - you don't have any conflicts with the current partition descriptions
  - you could even use the labels, because binman is the (only?) user

But of course you need to find a place where to put your node.

> What does user space do with the partition labels?

I'm not sure. Also I'm not sure if it really matters, I just wanted to
point out, that you'll force users to change it.

-michael

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

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

[Simon Glass]

Hi Michael,

On Fri, 6 Oct 2023 at 02:37, Michael Walle <mwalle@kernel.org> wrote:
>
> Hi,
>
> >> 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?
>
> As Rob pointed out earlier, it's just a user-facing string. I'm a bit
> reluctant to use it programatically.
> Taking my example again, the string "bootloader" is sufficient for a
> user. He doesn't care if it's u-boot with spl or u-boot with tfa, or
> even coreboot. It just says, "in this partition is the bootloader".
> If you have an "bootloader" image you can flash it there.
>
> If it has a label "u-boot" and I want to switch to coreboot, will
> it have to change to "coreboot"? I really don't think this is practical,
> you are really putting software configuration into the device tree.

I thought Rob changed his mind on that?

I agree that compatible would make things easier. We could then
continue to use 'label' for whatever it currently has.

Note that some kernel drivers or user space will want to look at what
is there, e.g. to provide a way to extract, check or update a
particular part of the firmware.

>
> > 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.
>
> And I'm not sure that will work for all the corner cases :/
>
> If you keep the binman section seperate from the flash partition
> definition you don't have any of these problems, although there is
> some redundancy:
>   - you only have compatible = "binman", "fixed-partition", no further
>     compatibles are required
>   - you don't have any conflicts with the current partition descriptions
>   - you could even use the labels, because binman is the (only?) user
>
> But of course you need to find a place where to put your node.

Sure, but I was pointed to partitions as the place where this should live.

>
> > What does user space do with the partition labels?
>
> I'm not sure. Also I'm not sure if it really matters, I just wanted to
> point out, that you'll force users to change it.

OK I'll send a version that uses compatible strings and see if that
makes any progress.

Regards,
Simon

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

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

[Rob Herring]

On Fri, Oct 06, 2023 at 10:37:41AM +0200, Michael Walle wrote:
> Hi,
> 
> > > 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?
> 
> As Rob pointed out earlier, it's just a user-facing string. I'm a bit
> reluctant to use it programatically.

In general, yes, but the partition stuff has long (and still) uses 
label. As long as the values the tools understand are documented (which 
we don't normally do for label), I don't care so much. That's my 
opinion as long as this is shared with fixed-partitions. If it is not 
and there's little reason to use label, then absolutely, I think 
'compatible' makes more sense. 

> Taking my example again, the string "bootloader" is sufficient for a
> user. He doesn't care if it's u-boot with spl or u-boot with tfa, or
> even coreboot. It just says, "in this partition is the bootloader".
> If you have an "bootloader" image you can flash it there.

These days, there's generally not just 1 bootloader in the boot flow. 
Maybe there's 1 image, maybe not. Being more specific is hardly ever a 
bad thing. Only when the number of specific things becomes multiple 10s 
or 100s of them does it become a problem.


> If it has a label "u-boot" and I want to switch to coreboot, will
> it have to change to "coreboot"? I really don't think this is practical,
> you are really putting software configuration into the device tree.

On the input side (to binman), yes it is config, but on the output side 
(to the running system) we are saying what's there.


> > 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.
> 
> And I'm not sure that will work for all the corner cases :/
> 
> If you keep the binman section seperate from the flash partition
> definition you don't have any of these problems, although there is
> some redundancy:
>  - you only have compatible = "binman", "fixed-partition", no further
>    compatibles are required
>  - you don't have any conflicts with the current partition descriptions
>  - you could even use the labels, because binman is the (only?) user
> 
> But of course you need to find a place where to put your node.

And remove it. We don't need 2 sources of truth in the DTB.

Rob

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

[Simon Glass]

Hi Rob,

On Fri, 6 Oct 2023 at 10:11, Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 06, 2023 at 10:37:41AM +0200, Michael Walle wrote:
> > Hi,
> >
> > > > 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?
> >
> > As Rob pointed out earlier, it's just a user-facing string. I'm a bit
> > reluctant to use it programatically.
>
> In general, yes, but the partition stuff has long (and still) uses
> label. As long as the values the tools understand are documented (which
> we don't normally do for label), I don't care so much. That's my
> opinion as long as this is shared with fixed-partitions. If it is not
> and there's little reason to use label, then absolutely, I think
> 'compatible' makes more sense.

OK I will try to drop the sharing with fixed-partitions

>
> > Taking my example again, the string "bootloader" is sufficient for a
> > user. He doesn't care if it's u-boot with spl or u-boot with tfa, or
> > even coreboot. It just says, "in this partition is the bootloader".
> > If you have an "bootloader" image you can flash it there.
>
> These days, there's generally not just 1 bootloader in the boot flow.
> Maybe there's 1 image, maybe not. Being more specific is hardly ever a
> bad thing. Only when the number of specific things becomes multiple 10s
> or 100s of them does it become a problem.
>
>
> > If it has a label "u-boot" and I want to switch to coreboot, will
> > it have to change to "coreboot"? I really don't think this is practical,
> > you are really putting software configuration into the device tree.
>
> On the input side (to binman), yes it is config, but on the output side
> (to the running system) we are saying what's there.
>
>
> > > 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.
> >
> > And I'm not sure that will work for all the corner cases :/
> >
> > If you keep the binman section seperate from the flash partition
> > definition you don't have any of these problems, although there is
> > some redundancy:
> >  - you only have compatible = "binman", "fixed-partition", no further
> >    compatibles are required
> >  - you don't have any conflicts with the current partition descriptions
> >  - you could even use the labels, because binman is the (only?) user
> >
> > But of course you need to find a place where to put your node.
>
> And remove it. We don't need 2 sources of truth in the DTB.

I would certainly prefer to have one...I think using 'label' for the
existing case and 'compatible' for the new one could be a reasonable
compromise, so I will send v3.

Regards,
Simon

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

[Simon Glass]

Hi Miquel,

On Wed, 4 Oct 2023 at 01:36, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Simon,
>
> sjg@chromium.org wrote on Mon,  2 Oct 2023 11:49:40 -0600:
>
> > 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.

OK, yes I can do that. I suppose they would still remain backwards
compatible, due to the use of '$ref: partition.yaml#'

>
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
>
> [...]
>
> > +properties:
> > +  compatible:
> > +    const: binman
>
> Right now this does not fit (I believe) the example. But if we no
> longer extend fixed-partitions but just create binman.yaml, this will
> probably be enough.

OK

>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    partitions {
> > +        compatible = "binman", "fixed-partitions";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        partition@100000 {
> > +            label = "u-boot";
> > +            reg = <0x100000 0xf00000>;
> > +        };
> > +    };

Regards,
Simon