Re: [RFC PATCH 1/2] dt-bindings: mtd: partitions: Document new dynamic-partitions node

[Ansuel Smith]

On Mon, Jan 24, 2022 at 11:02:24PM +0100, Rafał Miłecki wrote:
> On 20.01.2022 21:26, Ansuel Smith wrote:
> > Document new dynamic-partitions node used to provide an of node for
> > partition registred at runtime by parsers. This is required for nvmem
> > system to declare and detect nvmem-cells.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > ---
> >   .../mtd/partitions/dynamic-partitions.yaml    | 59 +++++++++++++++++++
> >   1 file changed, 59 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/mtd/partitions/dynamic-partitions.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/partitions/dynamic-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/dynamic-partitions.yaml
> > new file mode 100644
> > index 000000000000..7528e49f2d7e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/dynamic-partitions.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mtd/partitions/dynamic-partitions.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Dynamic partitions
> > +
> > +description: |
> > +  This binding can be used on platforms which have partitions registered at
> > +  runtime by parsers or partition table present on the flash. Example are
> > +  partitions declared from smem parser or cmdlinepart. This will create an
> > +  of node for these dynamic partition where systems like Nvmem can get a
> > +  reference to register nvmem-cells.
> > +
> > +  The partition table should be a node named "dynamic-partitions".
> > +  Partitions are then defined as subnodes. Only the label is required
> > +  as any other data will be taken from the parser.
> > +
> > +maintainers:
> > +  - Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    const: dynamic-partitions
> > +
> > +patternProperties:
> > +  "@[0-9a-f]+$":
> > +    $ref: "partition.yaml#"
> > +
> > +additionalProperties: true
> > +
> > +examples:
> > +  - |
> > +    partitions {
> > +        compatible = "qcom,smem";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +    };
> > +
> > +    dynamic-partitions {
> > +      compatible = "dynamic-partitions";
> > +
> > +      art: art {
> > +        label = "0:art";
> > +        read-only;
> > +        compatible = "nvmem-cells";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +
> > +        macaddr_art_0: macaddr@0 {
> > +          reg = <0x0 0x6>;
> > +        };
> > +
> > +        macaddr_art_6: macaddr@6 {
> > +          reg = <0x6 0x6>;
> > +        };
> > +      };
> > +    };
> 
> First of all: I fully support such a feature. I need it for Broadom
> platforms that use "brcm,bcm947xx-cfe-partitions" dynamic partitions.
> In my case bootloader partition is created dynamically (it doesn't have
> const offset and size). It contains NVMEM data however that needs to be
> described in DT.
> 
> This binding however looks loose and confusing to me.
>

I agree.

> First of all did you really mean to use "qcom,smem"? My first guess is
> you meant "qcom,smem-part".
> 

Yes sorry, I was referring to the smem parser qcom,smem-part 

> Secondly can't we have partitions defined just as subnodes of the
> partitions { ... }; node?
> 

I would love to use it. My only concern is that due to the fact
that we have to support legacy partition declaring, wonder if this could
create some problem. I'm referring to declaring fixed partition without
using any compatible/standard binding name.

I remember we improved that with the introduction of the nvmem binding
by making the fixed-partition compatible mandatory. But I would like to
have extra check. Wonder if to be on the safe part we can consider
appending to the "dynamic parser" a compatible like "dynamic-partitions"
and use your way to declare them (aka keeping the dynamic-partition and
removing the extra parallel partitions list)

Feel free to tell me it's just a stupid and unnecessary idea. I just
have fear to introduce regression in the partition parsing logic.

> 
> I think sth like below would make more sense:
> 
> partitions {
>     compatible = "qcom,smem-part";
> 
>     art {
>         label = "0:art";
>         read-only;
>         compatible = "nvmem-cells";
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         macaddr_art_0: macaddr@0 {
>             reg = <0x0 0x6>;
>         };
> 
>         macaddr_art_6: macaddr@6 {
>             reg = <0x6 0x6>;
>         };
>     };
> };
> 
> 
> Then I could also reuse that for something like:
> 
> partitions {
>     compatible = "brcm,bcm947xx-cfe-partitions";
> 
>     partition-0 {
>         compatible = "nvmem-cells";
>         label = "boot";
> 
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         mac: macaddr@0 {
>             reg = <0x100 0x6>;
>         };
>     }
> };

-- 
	Ansuel