Re: [PATCH v4 2/3] dt-bindings: mtd: binman-partition: Add binman compatibles

[Simon Glass]

Hi Rob,

On Tue, 24 Oct 2023 at 09:16, Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Mon, Oct 09, 2023 at 04:04:14PM -0600, Simon Glass wrote:
> > Add two compatible for binman entries, as a starting point for the
> > schema.
> >
> > Note that, after discussion on v2, we decided to keep the existing
> > meaning of label so as not to require changes to existing userspace
> > software when moving to use binman nodes to specify the firmware
> > layout.
> >
> > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > ---
> >
> > Changes in v4:
> > - Correct selection of multiple compatible strings
> >
> > Changes in v3:
> > - Drop fixed-partitions from the example
> > - Use compatible instead of label
> >
> > Changes in v2:
> > - Use plain partition@xxx for the node name
> >
> >  .../mtd/partitions/binman-partition.yaml      | 49 +++++++++++++++++++
> >  1 file changed, 49 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..35a320359ec1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/binman-partition.yaml
> > @@ -0,0 +1,49 @@
> > +# 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@xxxxxxxxxxxx>
> > +
> > +select: false
>
> So this schema is never used. 'select: false' is only useful if
> something else if referencing the schema.

OK. Is there a user guide to this somewhere? I really don't understand
it very well.

>
> > +
> > +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:
> > +  compatible:
> > +    oneOf:
> > +      - const: binman,entry     # generic binman entry
>
> 'binman' is not a vendor. You could add it if you think that's useful.
> Probably not with only 1 case...

I think it is best to use this for generic things implemented by
binman, rather than some other project. For example, binman supports a
'fill' region. It also supports sections which are groups of
sub-entries. So we will likely start with half a dozen of these and it
will likely grow: binman,fill, binman,section, binman,files

If we don't use 'binman', what do you suggest?

>
> > +      - items:
> > +          - const: u-boot       # u-boot.bin from U-Boot project
> > +          - const: atf-bl31     # bl31.bin or bl31.elf from TF-A project
>
> Probably should use the new 'tfa' rather than old 'atf'. Is this the
> only binary for TFA? The naming seems inconsistent in that every image
> goes in (or can go in) a bl?? section. Why does TFA have it but u-boot
> doesn't? Perhaps BL?? is orthogonal to defining what is in each
> partition. Perhaps someone more familar with all this than I am can
> comment.

>From what I can tell TF-A can produce all sorts of binaries, of which
bl31 is one. U-Boot can also produce lots of binaries, but its naming
is different (u-boot, u-boot-spl, etc.). Bear in mind that U-Boot is
used on ARM, where this terminology is defined, and on x86 (for
example), where it is not.

>
> Once you actually test this, you'll find you are specifying:
>
> compatible = "u-boot", "atf-bl31";

I don't understand that, sorry. I'll send a v5 and see if the problem goes away.

>
>
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    partitions {
> > +        compatible = "binman";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        partition@100000 {
> > +            compatible = "u-boot";
> > +            reg = <0x100000 0xf00000>;
> > +        };
> > +
> > +        partition@200000 {
> > +            compatible = "atf-bl31";
> > +            reg = <0x200000 0x100000>;
> > +        };
> > +    };
> > --
> > 2.42.0.609.gbb76f46606-goog
> >

Regards,
Simon