| Message ID | 20241026075347.580858-2-amit.kumar-mahapatra@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for stacked and parallel memories | expand |
On 26/10/2024 09:53, Amit Kumar Mahapatra wrote: > 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. I understand this was not sent to proper addresses for review because it is a RFC. It's fine then. If this was not the intention and this should be reviewed (and tested, although I assume you tested these patches first), then please read the standard form bellow: <form letter> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to CC. It might happen, that command when run on an older kernel, gives you outdated entries. Therefore please be sure you base your patches on recent Linux kernel. Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your workflow. Tools might also fail if you work on some ancient tree (don't, instead use mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and everything should be fine, although remember about `b4 prep --auto-to-cc` if you added new patches to the patchset. You missed at least devicetree list (maybe more), so this won't be tested by automated tooling. Performing review on untested code might be a waste of time. Please kindly resend and include all necessary To/Cc entries. </form letter> Best regards, Krzysztof
Hello Krzysztof > > 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. > > I understand this was not sent to proper addresses for review because it is a RFC. Yes, that’s correct. Regards, Amit > It's fine then. > > If this was not the intention and this should be reviewed (and tested, although I > assume you tested these patches first), then please read the standard form bellow: > > <form letter> > Please use scripts/get_maintainers.pl to get a list of necessary people and lists to > CC. It might happen, that command when run on an older kernel, gives you outdated > entries. Therefore please be sure you base your patches on recent Linux kernel. > > Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your > workflow. Tools might also fail if you work on some ancient tree (don't, instead use > mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and > everything should be fine, although remember about `b4 prep --auto-to-cc` if you > added new patches to the patchset. > > You missed at least devicetree list (maybe more), so this won't be tested by > automated tooling. Performing review on untested code might be a waste of time. > > Please kindly resend and include all necessary To/Cc entries. > </form letter> > > Best regards, > Krzysztof
On 26/10/2024 at 13:23:46 +0530, Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> wrote: > 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. > 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. > 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@bootlin.com/ > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > --- > .../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.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml > index 058253d6d889..df4ccb3dfeba 100644 > --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml > +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.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
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@amd.com> > > --- > > .../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
On 19/11/2024 at 17:02:33 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote: > 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. New thought. What if it was a pure fixed-partition capability? That's actually what we want: defining fixed partitions through device boundaries. It automatically removes the need for further dynamic partition extensions. Thanks, Miquèl
> > 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. > > New thought. What if it was a pure fixed-partition capability? That's actually what we Yes, I agree—it’s better to present it as a purely fixed-partition capability. Regards, Amit > want: defining fixed partitions through device boundaries. It automatically removes > the need for further dynamic partition extensions. > > Thanks, > Miquèl
diff --git a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml index 058253d6d889..df4ccb3dfeba 100644 --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml @@ -183,3 +183,21 @@ examples: read-only; }; }; + + - | + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + 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
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 { 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>; } } } 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@bootlin.com/ Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> --- .../mtd/partitions/fixed-partitions.yaml | 18 ++++++++++++++++++ .../bindings/mtd/partitions/partitions.yaml | 6 ++++++ 2 files changed, 24 insertions(+)