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

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@gmail.com>
---
 .../mtd/partitions/dynamic-partitions.yaml    | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/partitions/dynamic-partitions.yaml

On Thu, Jan 20, 2022 at 07:49:26PM -0600, Rob Herring wrote:
> On Thu, Jan 20, 2022 at 09:26:14PM +0100, 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.
> 
> So you have some discoverable way to find all the partitions and the 
> nvmem cells are at an unknown (to the DT) location, but still need to be 
> described in DT?
>

Example: smem partition layout is saved in the bootloader and static. To
know the layout just boot the device and extract it. Aside from this the
naming convention is ""standard"" (example the standard nvmem location
for this is named 0:art)

What needs to be described in the DT is the cell offset and the
partition name (the location)

NVMEM doesn't support this and honestly I can't think of a simple and
direct way to solve this. Considering partition in this case are just
filled at runtime and they doesn't change (or at worst the partition
offset change but NEVER the name) it seems a good way to fix this by
describing a nvmem cells partition name and assign a of node to the
runtime filled partition.

> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  .../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
> 
> Some information in DT and some on the cmdline seems broken to me. Pick 
> one or the other.
> 

Converting a system from cmdline to fixedpartition is problematic
if the cmdline is dynamic. Example some system have dual partition and
are handled by editing the cmdline partition description. In this
cmdline tho the nvmem cell of our interest doesn't change and we can use
this new implemenation to add support for nvmem cells.

So really there are some case where nvmem won't work and it's not
possible to provide a correct configuration for nvmem to work correctly.

Is it that bad to have information in the DT about nvmem cells for a
partition named with a particular label that won't change.

> > +  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@gmail.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: dynamic-partitions
> 
> This is useless. This tells me nothing about the what's in the 
> 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>;
> > +        };
> > +      };
> > +    };
> > -- 
> > 2.33.1
> > 
> >

On 24.01.2022 23:12, Ansuel Smith wrote:
> 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@gmail.com>
>>> ---
>>>    .../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@gmail.com>
>>> +
>>> +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.

Legacy partitioning won't kick in if you have "partitions" with
"compatible" string. We're safe here. Just checked to be sure.


> 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'm confused. I think all dynamic partitioners already have a
"compatible" set.

Can you post an example of DT binging you described above, please?

On Tue, Jan 25, 2022 at 09:21:04PM +0100, Rafał Miłecki wrote:
> On 24.01.2022 23:12, Ansuel Smith wrote:
> > 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@gmail.com>
> > > > ---
> > > >    .../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@gmail.com>
> > > > +
> > > > +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.
> 
> Legacy partitioning won't kick in if you have "partitions" with
> "compatible" string. We're safe here. Just checked to be sure.
>

Oh ok then the dynamic partition compatible stuff is not needed.
To make sure I will change the "connect" function part and skip the
of_node assign if a compatible is not present. (The of_node assign
should be done only with the nvmem-cell compatible currently.)

> 
> > 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'm confused. I think all dynamic partitioners already have a
> "compatible" set.

Now that I think about it you are right. If a dynamic partition is
present in the system, a compatible must be present to use the correct
parser. And as I said up, all the nvmem cells should have the
correct compatible.

> 
> Can you post an example of DT binging you described above, please?

Was thinking something like this. But not needed.

partitions {
     compatible = "brcm,bcm947xx-cfe-partitions", "dynamic-partitions";

     partition-0 {
         compatible = "nvmem-cells";
         label = "boot";

         #address-cells = <1>;
         #size-cells = <1>;

         mac: macaddr@0 {
             reg = <0x100 0x6>;
         };
     }
};

So in short, a scheme like this should NOT be handled/should not have
of_node assigned. (and is actually very wrong)

partitions {
     compatible = "brcm,bcm947xx-cfe-partitions";

     partition-0 {
         label = "boot";

         #address-cells = <1>;
         #size-cells = <1>;

         mac: macaddr@0 {
             reg = <0x100 0x6>;
         };
     }
};