[PATCH 2/2] schemas: Add a schema for binman

[Alper Nebi Yasak]

Hi Simon,

I know I haven't been able to look at binman stuff for a long time, but
I've been occasionally thinking about it through the course of a year
and think binman severely needs a design-wise review before things get
entrenched, even in the most fundamental parts. I do see the
cross-project collaboration intention here, but still...

There's also the issue of binman having multiple different device-trees:
its input, the ones in fdtmaps per image, the ones injected to U-Boot
dtb files per image. I'd say each has different needs, and those
differences have to be figured out before specified upstream. I can only
guess this is about the one that'll be in a u-boot.dtb.

I want to go through binman more thoroughly, but a lot of changes
happened since I last looked at it and I'm a bit slow at writing things,
so won't exactly be soon. That being said, here's some ideas off the top
of my head, for inspiration on both this schema and binman itself.

On 2023-07-12 00:18 +03:00, Simon Glass wrote:
> I am unsure whether to add this with a generic name, such as 'layout',
> but for now am using /firmware/binman to avoid conflicts with any other
> firmware-layout schema that others might be working on.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---

I've been thinking of compatible = "data,<type>" for entries, so
proposing 'data' here. A big ask, but it might be the one schema to
unify them all if we look at things as "description of data" instead of
"firmware layout".

Also consider data layouts in-memory. For example it could be an
alternative to /chosen linux,initrd-start to specify initramfs location.
Or things like keys or logs received from previous stages / other parts?
Weak examples, but maybe might connect better into firmware handoff
things. (Sorry, I don't know much about those.)

I'm also thinking about things like JPEG/BMP files for ACPI BGRT-like
boot splash, unique firmware/configuration/calibration data for drivers,
but they don't exactly need to be in-memory I guess.

> 
>  dtschema/schemas/firmware/binman.yaml       | 51 ++++++++++++++++++
>  dtschema/schemas/firmware/binman/entry.yaml | 57 +++++++++++++++++++++
>  2 files changed, 108 insertions(+)
>  create mode 100644 dtschema/schemas/firmware/binman.yaml
>  create mode 100644 dtschema/schemas/firmware/binman/entry.yaml
> 
> diff --git a/dtschema/schemas/firmware/binman.yaml b/dtschema/schemas/firmware/binman.yaml
> new file mode 100644
> index 0000000..4b1ecf6
> --- /dev/null
> +++ b/dtschema/schemas/firmware/binman.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Google LLC
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/binman.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binman firmware layout
> +
> +maintainers:
> +  - Simon Glass <sjg at 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:
> +  $nodename:
> +    const: binman

I think having a single /firmware/binman node for a single image *and*
having a "multiple-images" mode under it is quite bad because the
single-image mode is multiple-images with one image. Multiple images
should be the only mode of operation

Furthermore, each image should be able to function independently of
others, so I think to help enforce that maybe there shouldn't be any
formal binman node. Merely instances of images that may happen to be
under one parent node that may happen to be called /firmware/binman. The
images can be compatible = "data,image" or something and we could search
by that.

Or, heh, a single image node that happens to be named /firmware/binman
because we are embedding it into a u-boot.dtb in that image. But perhaps
we shouldn't restrain things like that. A bit contrived, but why should
we be unable to start from an image in SPI and make it load something
from microSD?

Going even further, let me suggest putting the image nodes as children
of the devices whose layout the image describes. Then I imagine we can
have something like a "data driver" (both for U-Boot and Linux) that
reads the entry data from the parent data/block device and makes it
available for other drivers or in sysfs.

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false

Including the binman version or a version number for the spec might be
useful later on.

> +
> +examples:
> +  - |
> +  - |
> +    firmware {
> +      binman {
> +        compatible = "u-boot,binman";

Consider including "image" or "namespace" in the compatible string to
disambiguate. Not sure of the conventions, "binman,*" could work just as
well, though I'm partial to "data,*" as I've suggested.

> +
> +        u-boot {
> +          size = <0xa0000>;
> +        };
> +
> +        atf-bl31 {
> +          offset = <0x100000>;
> +        };
> +      };
> +    };
> diff --git a/dtschema/schemas/firmware/binman/entry.yaml b/dtschema/schemas/firmware/binman/entry.yaml
> new file mode 100644
> index 0000000..16b84c5
> --- /dev/null
> +++ b/dtschema/schemas/firmware/binman/entry.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Google LLC
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/firmware/binman/entry.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binman entry
> +
> +maintainers:
> +  - Simon Glass <sjg at 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.

I really dislike the name being a fallback for the type, it's guiding
people to using types as the names (or even worse, to assuming names can
only be same as the type), leading to ugly combinations of "blob-ext",
"fdt-N", "fit", "section" and so on. Node names should be descriptive
even when you don't have the device-tree source in front of you, think
of "binman extract" and others. Ones like "u-boot" and "atf-bl31" are
fine only because they are very specific entry types.

> +
> +  Note: This definition is intended to be hierarchical, so that entries can
> +  appear in other entries. Schema for that is TBD.

It might be better to redesign things bottom-up -- first raw data, then
a few data types, then sections and images that hold these. Oh, and I
have a feeling section/image can be the same thing if we think hard
enough (e.g. explicit node labels/references for fdtmap).

> +
> +properties:
> +  $nodename:
> +    pattern: "^[-a-z]+(-[0-9]+)?$"
> +
> +  type:
> +    $ref: /schemas/types.yaml#/definitions/string

Consider compatible = "data,bytes" instead, making it required. Or maybe
"data,blob" but that's not what's there on the binman side. (e.g. rename
"blob" to "file", "blob-ext" could be "file" combined with a
globally-available "optional" property.)

> +
> +  offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Provides the offset of this entry from the start of its parent section.
> +
> +  size:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Provides the size of this entry in bytes.

Can't we use "reg" for these, and even entry@<addr> in the definitions?
I know the latter conflicts with the fdt at SEQ stuff, but that should be a
binman-input-only thing, and I dislike that anyway. Maybe could work if
we put image nodes under block devices...

Or maybe even better, I think we should make it like FIT: allow a "data"
property that has it inside the dtb, or a pair of "data-offset/position"
and "data-size" properties if it's outside. Inlining data inside the dtb
could help us do nice things in binman, like hashes/signatures as entry
types instead of special-casing them.

In fact it could be possible to turn binman images into a FIT 2.0 if we
do some more work on top of that, instead of nesting a FIT inside a
binman image.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware {
> +      binman {
> +        compatible = "u-boot,binman";
> +
> +        u-boot {
> +          size = <0xa0000>;
> +        };
> +
> +        second-area {
> +          type = "atf-bl31";
> +          offset = <0x100000>;
> +        };
> +      };
> +    };