RE: [RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

["Mahapatra, Amit Kumar" <amit.kumar-mahapatra@xxxxxxx>]

Hello Miquel,
 
> > This approach was suggested by Rob [1] during a discussion on Miquel's
> > initial approach [2] to extend the MTD-CONCAT driver to support
> > stacked memories.
> > Define each flash node separately with its respective partitions, and
> > add a 'concat-parts' binding to link the partitions of the two flash
> > nodes that need to be concatenated.
> >
> > flash@0 {
> >         compatible = "jedec,spi-nor"
> >         ...
> >                 partitions {
> 
> Wrong indentation here and below which makes the example hard to read.

Sorry about that. I am redefining both the flash nodes here with proper 
indentation.

flash@0 {
	compatible = "jedec,spi-nor"
	...
	partitions {
		compatible = "fixed-partitions";
		concat-partition = <&flash0_partition &flash1_partition>;
		
		flash0_partition: partition@0 {
			label = "part0_0";
			reg = <0x0 0x800000>;
		};
	};
};

flash@1 {
	compatible = "jedec,spi-nor"
	...
	partitions {
		compatible = "fixed-partitions";
		concat-partition = <&flash0_partition &flash1_partition>;
                        
		flash1_partition: partition@0 {
			label = "part0_1";
			reg = <0x0 0x800000>;
		};
	};
};

> 
> >                 compatible = "fixed-partitions";
> >                         concat-partition = <&flash0_partition &flash1_partition>;
> >                         flash0_partition: partition@0 {
> >                                 label = "part0_0";
> >                                 reg = <0x0 0x800000>;
> >                         }
> >                 }
> > }
> > flash@1 {
> >         compatible = "jedec,spi-nor"
> >         ...
> >                 partitions {
> >                 compatible = "fixed-partitions";
> >                         concat-partition = <&flash0_partition &flash1_partition>;
> >                         flash1_partition: partition@0 {
> >                                 label = "part0_1";
> >                                 reg = <0x0 0x800000>;
> >                         }
> >                 }
> > }
> 
> This approach has a limitation I didn't think about before: you cannot use anything
> else than fixed partitions as partition parser.

Yes, that's correct—it won't function when partitions are defined via the 
command line. In my opinion, we should start by adding support for fixed 
partitions, add comments in code stating the same. If needed, we can later 
extend the support to dynamic partitions as well.

Regards,
Amit

> 
> > Based on the bindings the MTD-CONCAT driver need to be updated to
> > create virtual mtd-concat devices.
> >
> > [1] https://lore.kernel.org/all/20191118221341.GA30937@bogus/
> > [2]
> > https://lore.kernel.org/all/20191113171505.26128-4-miquel.raynal@bootl
> > in.com/
> >
> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@xxxxxxx>
> > ---
> >  .../mtd/partitions/fixed-partitions.yaml       | 18 ++++++++++++++++++
> >  .../bindings/mtd/partitions/partitions.yaml    |  6 ++++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.ya
> > ml
> > b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.ya
> > ml index 058253d6d889..df4ccb3dfeba 100644
> > ---
> > a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.ya
> > ml
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partition
> > +++ s.yaml
> > @@ -183,3 +183,21 @@ examples:
> >              read-only;
> >          };
> >      };
> > +
> > +  - |
> > +    partitions {
> > +        compatible = "fixed-partitions";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> 
> This is not strictly related but I believe we will soon have issues with these, as we
> will soon cross the 4GiB boundary.
> 
> > +        concat-parts = <&part0 &part1>;
> > +
> > +        part0: partition@0 {
> > +            label = "flash0-part0";
> > +            reg = <0x0000000 0x100000>;
> > +        };
> > +
> > +        part1: partition@100000 {
> > +            label = "flash1-part0";
> > +            reg = <0x0100000 0x200000>;
> > +        };
> > +    };
> > diff --git
> > a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > index 1dda2c80747b..86bbd83c3f6d 100644
> > --- a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > @@ -32,6 +32,12 @@ properties:
> >    '#size-cells':
> >      enum: [1, 2]
> >
> > +  concat-parts:
> > +    description: List of MTD partitions phandles that should be concatenated.
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    minItems: 2
> > +    maxItems: 4
> > +
> >  patternProperties:
> >    "^partition(-.+|@[0-9a-f]+)$":
> >      $ref: partition.yaml
> 
> Fine by me otherwise.
> 
> Thanks,
> Miquèl