| Message ID | 20241026075347.580858-3-amit.kumar-mahapatra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for stacked and parallel memories | expand |
Hi Amit, On 26/10/2024 at 13:23:47 +0530, Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> wrote: > For implementing the proposed solution the current 'stacked-memories' & > 'parallel-memories' bindings need to be updated as follow. > > stacked-memories binding changes: > - Each flash will have its own flash node. This approach allows flashes of > different makes and sizes to be stacked together, as each flash will be > probed individually. > - Each of the flash node will have its own “reg” property that will contain > its physical CS. > - Remove the size information from the bindings as it can be retrived > drirectly from the flash. > - The stacked-memories DT bindings will contain the phandles of the flash > nodes connected in stacked mode. > > The new layer will update the mtd->size and other mtd_info parameters after > both the flashes are probed and will call mtd_device_register with the > combined information. > > spi@0 { > ... > flash@0 { > compatible = "jedec,spi-nor" > reg = <0x00>; > stacked-memories = <&flash@0 &flash@1>; > spi-max-frequency = <50000000>; > ... > 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" > reg = <0x01>; > stacked-memories = <&flash@0 &flash@1>; > spi-max-frequency = <50000000>; > ... > partitions { Same comment as before here. > compatible = "fixed-partitions"; > concat-partition = <&flash0_partition &flash1_partition>; > flash1_partition: partition@0 { > label = "part0_1"; > reg = <0x0 0x800000>; > } > } > } > > } > > parallel-memories binding changes: > - Remove the size information from the bindings and change the type to > boolen. > - Each flash connected in parallel mode should be identical and will have > one flash node for both the flash devices. > - The “reg” prop will contain the physical CS number for both the connected > flashes. > > The new layer will double the mtd-> size and register it with the mtd > layer. Not so sure about that, you'll need a new mtd device to capture the whole device. But this is implementation related, not relevant for binding. > > spi@1 { > ... > flash@3 { > compatible = "jedec,spi-nor" > reg = <0x00 0x01>; > paralle-memories ; Please fix the typos and the spacing (same above). > spi-max-frequency = <50000000>; > ... > partitions { > compatible = "fixed-partitions"; > flash0_partition: partition@0 { > label = "part0_0"; > reg = <0x0 0x800000>; > } > } > } > } > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > --- > .../bindings/spi/spi-controller.yaml | 23 +++++++++++++++++-- > .../bindings/spi/spi-peripheral-props.yaml | 9 +++----- > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml > index 093150c0cb87..2d300f98dd72 100644 > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > @@ -185,7 +185,26 @@ examples: > flash@2 { > compatible = "jedec,spi-nor"; > spi-max-frequency = <50000000>; > - reg = <2>, <3>; > - stacked-memories = /bits/ 64 <0x10000000 0x10000000>; > + reg = <2>; > + stacked-memories = <&flash0 &flash1>; > }; I'm sorry but this is not what you've talked about in this series. Either you have flash0 and flash1 and use the stacked-memories property in both of them (which is what you described) or you create a third virtual device which points to two other flashes. This example allows for an easier use of the partitions mechanism on top of a virtual mtd device but, heh, you're now describing a virtual mtd device, which is not a physical device as it "should" be. Thanks, Miquèl
Hello Miquel, > > flash@1 { > > compatible = "jedec,spi-nor" > > reg = <0x01>; > > stacked-memories = <&flash@0 &flash@1>; > > spi-max-frequency = <50000000>; > > ... > > partitions { > > Same comment as before here. Sorry again spi@0 { ... flash@0 { compatible = "jedec,spi-nor" reg = <0x00>; stacked-memories = <&flash@0 &flash@1>; spi-max-frequency = <50000000>; ... partitions { compatible = "fixed-partitions"; concat-partition = <&flash0_part0 &flash1_part0>; flash0_part0: partition@0 { label = "part0_0"; reg = <0x0 0x800000>; }; }; }; flash@1 { compatible = "jedec,spi-nor" reg = <0x01>; stacked-memories = <&flash@0 &flash@1>; spi-max-frequency = <50000000>; ... partitions { compatible = "fixed-partitions"; concat-partition = <&flash0_part0 &flash1_part0>; flash1_part0: partition@0 { label = "part0_1"; reg = <0x0 0x800000>; }; }; }; }; > > > compatible = "fixed-partitions"; > > concat-partition = <&flash0_partition &flash1_partition>; > > flash1_partition: partition@0 { > > label = "part0_1"; > > reg = <0x0 0x800000>; > > } > > } > > } > > > > } > > > > parallel-memories binding changes: > > - Remove the size information from the bindings and change the type to > > boolen. > > - Each flash connected in parallel mode should be identical and will have > > one flash node for both the flash devices. > > - The “reg” prop will contain the physical CS number for both the connected > > flashes. > > > > The new layer will double the mtd-> size and register it with the mtd > > layer. > > Not so sure about that, you'll need a new mtd device to capture the whole device. > But this is implementation related, not relevant for binding. > > > > > spi@1 { > > ... > > flash@3 { > > compatible = "jedec,spi-nor" > > reg = <0x00 0x01>; > > paralle-memories ; > > Please fix the typos and the spacing (same above). > > > spi-max-frequency = <50000000>; > > ... > > partitions { > > compatible = "fixed-partitions"; > > flash0_partition: partition@0 { > > label = "part0_0"; > > reg = <0x0 0x800000>; > > } > > } > > } > > } > > > > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > > --- > > .../bindings/spi/spi-controller.yaml | 23 +++++++++++++++++-- > > .../bindings/spi/spi-peripheral-props.yaml | 9 +++----- > > 2 files changed, 24 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml > > b/Documentation/devicetree/bindings/spi/spi-controller.yaml > > index 093150c0cb87..2d300f98dd72 100644 > > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > > @@ -185,7 +185,26 @@ examples: > > flash@2 { > > compatible = "jedec,spi-nor"; > > spi-max-frequency = <50000000>; > > - reg = <2>, <3>; > > - stacked-memories = /bits/ 64 <0x10000000 0x10000000>; > > + reg = <2>; > > + stacked-memories = <&flash0 &flash1>; > > }; > > I'm sorry but this is not what you've talked about in this series. > Either you have flash0 and flash1 and use the stacked-memories property in both of > them (which is what you described) or you create a third virtual device which points > to two other flashes. This example allows for an easier use of the partitions If I understand your point correctly, you're suggesting that we should avoid using stacked-memories and concat-partition properties together and instead choose one approach. Between the two, I believe concat-partition would be the better option. While looking into your mtdconcat patch [1], I noticed that it creates a virtual MTD device that points to partitions on two different flash nodes, which aligns perfectly with our requirements. However, there are two key concerns that, if addressed, could make this patch suitable for the stacked mode: 1/ The creation of a virtual device that does not have a physical existence. 2/ The creation of individual MTD devices that are concatenated to form the virtual MTD device, which may not be needed by the user. Regarding the first point, I currently cannot think of a better generic way to support the stacked feature than creating a virtual device. Please let me know you thoughts on this. For the second point, one possible solution is to hide the individual MTD devices (that form the concatenated virtual MTD device) from the user once the virtual device is created. Please let us know if you have any other suggestions to address this issue. [1] https://lore.kernel.org/linux-mtd/20191127105522.31445-5-miquel.raynal@bootlin.com/ Regards, Amit > mechanism on top of a virtual mtd device but, heh, you're now describing a virtual > mtd device, which is not a physical device as it "should" be. > > Thanks, > Miquèl
On 19/11/2024 at 17:02:45 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote: > Hello Miquel, > >> > flash@1 { >> > compatible = "jedec,spi-nor" >> > reg = <0x01>; >> > stacked-memories = <&flash@0 &flash@1>; >> > spi-max-frequency = <50000000>; >> > ... >> > partitions { >> >> Same comment as before here. > > Sorry again > > spi@0 { > ... > flash@0 { > compatible = "jedec,spi-nor" > reg = <0x00>; > stacked-memories = <&flash@0 &flash@1>; > spi-max-frequency = <50000000>; > ... > partitions { > compatible = "fixed-partitions"; > concat-partition = <&flash0_part0 &flash1_part0>; > > flash0_part0: partition@0 { > label = "part0_0"; > reg = <0x0 0x800000>; > }; > }; > }; > flash@1 { > compatible = "jedec,spi-nor" > reg = <0x01>; > stacked-memories = <&flash@0 &flash@1>; > spi-max-frequency = <50000000>; > ... > partitions { > compatible = "fixed-partitions"; > concat-partition = <&flash0_part0 &flash1_part0>; > > flash1_part0: partition@0 { > label = "part0_1"; > reg = <0x0 0x800000>; > }; > }; > }; > }; > >> >> > compatible = "fixed-partitions"; >> > concat-partition = <&flash0_partition &flash1_partition>; >> > flash1_partition: partition@0 { >> > label = "part0_1"; >> > reg = <0x0 0x800000>; >> > } >> > } >> > } >> > >> > } >> > >> > parallel-memories binding changes: >> > - Remove the size information from the bindings and change the type to >> > boolen. >> > - Each flash connected in parallel mode should be identical and will have >> > one flash node for both the flash devices. >> > - The “reg” prop will contain the physical CS number for both the connected >> > flashes. >> > >> > The new layer will double the mtd-> size and register it with the mtd >> > layer. >> >> Not so sure about that, you'll need a new mtd device to capture the whole device. >> But this is implementation related, not relevant for binding. >> >> > >> > spi@1 { >> > ... >> > flash@3 { >> > compatible = "jedec,spi-nor" >> > reg = <0x00 0x01>; >> > paralle-memories ; >> >> Please fix the typos and the spacing (same above). >> >> > spi-max-frequency = <50000000>; >> > ... >> > partitions { >> > compatible = "fixed-partitions"; >> > flash0_partition: partition@0 { >> > label = "part0_0"; >> > reg = <0x0 0x800000>; >> > } >> > } >> > } >> > } >> > >> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> >> > --- >> > .../bindings/spi/spi-controller.yaml | 23 +++++++++++++++++-- >> > .../bindings/spi/spi-peripheral-props.yaml | 9 +++----- >> > 2 files changed, 24 insertions(+), 8 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml >> > b/Documentation/devicetree/bindings/spi/spi-controller.yaml >> > index 093150c0cb87..2d300f98dd72 100644 >> > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml >> > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml >> > @@ -185,7 +185,26 @@ examples: >> > flash@2 { >> > compatible = "jedec,spi-nor"; >> > spi-max-frequency = <50000000>; >> > - reg = <2>, <3>; >> > - stacked-memories = /bits/ 64 <0x10000000 0x10000000>; >> > + reg = <2>; >> > + stacked-memories = <&flash0 &flash1>; >> > }; >> >> I'm sorry but this is not what you've talked about in this series. >> Either you have flash0 and flash1 and use the stacked-memories property in both of >> them (which is what you described) or you create a third virtual device which points >> to two other flashes. This example allows for an easier use of the partitions > > If I understand your point correctly, you're suggesting that we should > avoid using stacked-memories and concat-partition properties together and > instead choose one approach. Between the two, I believe concat-partition > would be the better option. That's not exactly it, look at the reg properties above, they do not match the flash devices. Your example above invalid but it is not clear whether this is another typo or voluntary. > While looking into your mtdconcat patch [1], I noticed that it creates a > virtual MTD device that points to partitions on two different flash nodes, > which aligns perfectly with our requirements. > > However, there are two key concerns that, if addressed, could make this > patch suitable for the stacked mode: > > 1/ The creation of a virtual device that does not have a physical > existence. We do already have: - the master mtd device (disabled by default for historical reasons, but can be enabled with a Kconfig option). - an mtd device per partition I don't see a problem in creating virtual mtd devices in the kernel. > 2/ The creation of individual MTD devices that are concatenated to form > the virtual MTD device, which may not be needed by the user. You can also get rid of them by default (or perhaps do the opposite and let a Kconfig option for that). > Regarding the first point, I currently cannot think of a better generic > way to support the stacked feature than creating a virtual device. > Please let me know you thoughts on this. > > For the second point, one possible solution is to hide the individual MTD > devices (that form the concatenated virtual MTD device) from the user once > the virtual device is created. Please let us know if you have any other > suggestions to address this issue. That is what is done with the master device by default. > [1] https://lore.kernel.org/linux-mtd/20191127105522.31445-5-miquel.raynal@bootlin.com/ Thanks, Miquèl
> > spi@0 { > > ... > > flash@0 { > > compatible = "jedec,spi-nor" > > reg = <0x00>; > > stacked-memories = <&flash@0 &flash@1>; > > spi-max-frequency = <50000000>; > > ... > > partitions { > > compatible = "fixed-partitions"; > > concat-partition = <&flash0_part0 &flash1_part0>; > > > > flash0_part0: partition@0 { > > label = "part0_0"; > > reg = <0x0 0x800000>; > > }; > > }; > > }; > > flash@1 { > > compatible = "jedec,spi-nor" > > reg = <0x01>; > > stacked-memories = <&flash@0 &flash@1>; > > spi-max-frequency = <50000000>; > > ... > > partitions { > > compatible = "fixed-partitions"; > > concat-partition = <&flash0_part0 &flash1_part0>; > > > > flash1_part0: partition@0 { > > label = "part0_1"; > > reg = <0x0 0x800000>; > > }; > > }; > > }; > > }; > > > >> > >> > compatible = "fixed-partitions"; > >> > concat-partition = <&flash0_partition &flash1_partition>; > >> > flash1_partition: partition@0 { > >> > label = "part0_1"; > >> > reg = <0x0 0x800000>; > >> > } > >> > } > >> > } > >> > > >> > } > >> > > >> > parallel-memories binding changes: > >> > - Remove the size information from the bindings and change the type to > >> > boolen. > >> > - Each flash connected in parallel mode should be identical and will have > >> > one flash node for both the flash devices. > >> > - The “reg” prop will contain the physical CS number for both the connected > >> > flashes. > >> > > >> > The new layer will double the mtd-> size and register it with the > >> > mtd layer. > >> > >> Not so sure about that, you'll need a new mtd device to capture the whole > device. > >> But this is implementation related, not relevant for binding. > >> > >> > > >> > spi@1 { > >> > ... > >> > flash@3 { > >> > compatible = "jedec,spi-nor" > >> > reg = <0x00 0x01>; > >> > paralle-memories ; > >> > >> Please fix the typos and the spacing (same above). > >> > >> > spi-max-frequency = <50000000>; > >> > ... > >> > partitions { > >> > compatible = "fixed-partitions"; > >> > flash0_partition: partition@0 { > >> > label = "part0_0"; > >> > reg = <0x0 0x800000>; > >> > } > >> > } > >> > } > >> > } > >> > > >> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> > >> > --- > >> > .../bindings/spi/spi-controller.yaml | 23 +++++++++++++++++-- > >> > .../bindings/spi/spi-peripheral-props.yaml | 9 +++----- > >> > 2 files changed, 24 insertions(+), 8 deletions(-) > >> > > >> > diff --git > >> > a/Documentation/devicetree/bindings/spi/spi-controller.yaml > >> > b/Documentation/devicetree/bindings/spi/spi-controller.yaml > >> > index 093150c0cb87..2d300f98dd72 100644 > >> > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > >> > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > >> > @@ -185,7 +185,26 @@ examples: > >> > flash@2 { > >> > compatible = "jedec,spi-nor"; > >> > spi-max-frequency = <50000000>; > >> > - reg = <2>, <3>; > >> > - stacked-memories = /bits/ 64 <0x10000000 0x10000000>; > >> > + reg = <2>; > >> > + stacked-memories = <&flash0 &flash1>; > >> > }; > >> > >> I'm sorry but this is not what you've talked about in this series. > >> Either you have flash0 and flash1 and use the stacked-memories > >> property in both of them (which is what you described) or you create > >> a third virtual device which points to two other flashes. This > >> example allows for an easier use of the partitions > > > > If I understand your point correctly, you're suggesting that we should > > avoid using stacked-memories and concat-partition properties together > > and instead choose one approach. Between the two, I believe > > concat-partition would be the better option. > > That's not exactly it, look at the reg properties above, they do not match the flash > devices. Your example above invalid but it is not clear whether this is another typo > or voluntary. Ah, I see. It's a typo ☹. However, based on the discussion below, it seems we might not need the 'stacked-memories' property anymore. > > > While looking into your mtdconcat patch [1], I noticed that it creates > > a virtual MTD device that points to partitions on two different flash > > nodes, which aligns perfectly with our requirements. > > > > However, there are two key concerns that, if addressed, could make > > this patch suitable for the stacked mode: > > > > 1/ The creation of a virtual device that does not have a physical > > existence. > > We do already have: > - the master mtd device (disabled by default for historical reasons, but > can be enabled with a Kconfig option). > - an mtd device per partition > > I don't see a problem in creating virtual mtd devices in the kernel. > > > 2/ The creation of individual MTD devices that are concatenated to > > form the virtual MTD device, which may not be needed by the user. > > You can also get rid of them by default (or perhaps do the opposite and let a Kconfig > option for that). Ok, great! I'll look into it and try to integrate it into your previous patch [1]. If everything works out, then IMHO this(patch[1] + the above changes) should suffice for adding stacked support. Regards, Amit > > > Regarding the first point, I currently cannot think of a better > > generic way to support the stacked feature than creating a virtual device. > > Please let me know you thoughts on this. > > > > For the second point, one possible solution is to hide the individual > > MTD devices (that form the concatenated virtual MTD device) from the > > user once the virtual device is created. Please let us know if you > > have any other suggestions to address this issue. > > That is what is done with the master device by default. > > > [1] > > https://lore.kernel.org/linux-mtd/20191127105522.31445-5-miquel.raynal > > @bootlin.com/ > > Thanks, > Miquèl
diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml index 093150c0cb87..2d300f98dd72 100644 --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml @@ -185,7 +185,26 @@ examples: flash@2 { compatible = "jedec,spi-nor"; spi-max-frequency = <50000000>; - reg = <2>, <3>; - stacked-memories = /bits/ 64 <0x10000000 0x10000000>; + reg = <2>; + stacked-memories = <&flash0 &flash1>; }; + }; + + - | + spi@90010000 { + #address-cells = <1>; + #size-cells = <0>; + compatible = "fsl,imx28-spi"; + reg = <0x90010000 0x2000>; + interrupts = <96>; + dmas = <&dma_apbh 0>; + dma-names = "rx-tx"; + + flash@0 { + compatible = "jedec,spi-nor"; + spi-max-frequency = <50000000>; + reg = <0>, <1>; + parallel-memories; + }; + }; diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml index 15938f81fdce..2a014160d701 100644 --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml @@ -96,7 +96,7 @@ properties: space with only a single additional wire, while still needing to repeat the commands when crossing a chip boundary. The size of each chip should be provided as members of the array. - $ref: /schemas/types.yaml#/definitions/uint64-array + $ref: /schemas/types.yaml#/definitions/phandle-array minItems: 2 maxItems: 4 @@ -107,11 +107,8 @@ properties: different memories (eg. even bits are stored in one memory, odd bits in the other). This basically doubles the address space and the throughput while greatly complexifying the wiring because as - many busses as devices must be wired. The size of each chip should - be provided as members of the array. - $ref: /schemas/types.yaml#/definitions/uint64-array - minItems: 2 - maxItems: 4 + many busses as devices must be wired. + $ref: /schemas/types.yaml#/definitions/flag st,spi-midi-ns: description: |
For implementing the proposed solution the current 'stacked-memories' & 'parallel-memories' bindings need to be updated as follow. stacked-memories binding changes: - Each flash will have its own flash node. This approach allows flashes of different makes and sizes to be stacked together, as each flash will be probed individually. - Each of the flash node will have its own “reg” property that will contain its physical CS. - Remove the size information from the bindings as it can be retrived drirectly from the flash. - The stacked-memories DT bindings will contain the phandles of the flash nodes connected in stacked mode. The new layer will update the mtd->size and other mtd_info parameters after both the flashes are probed and will call mtd_device_register with the combined information. spi@0 { ... flash@0 { compatible = "jedec,spi-nor" reg = <0x00>; stacked-memories = <&flash@0 &flash@1>; spi-max-frequency = <50000000>; ... 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" reg = <0x01>; stacked-memories = <&flash@0 &flash@1>; spi-max-frequency = <50000000>; ... partitions { compatible = "fixed-partitions"; concat-partition = <&flash0_partition &flash1_partition>; flash1_partition: partition@0 { label = "part0_1"; reg = <0x0 0x800000>; } } } } parallel-memories binding changes: - Remove the size information from the bindings and change the type to boolen. - Each flash connected in parallel mode should be identical and will have one flash node for both the flash devices. - The “reg” prop will contain the physical CS number for both the connected flashes. The new layer will double the mtd-> size and register it with the mtd layer. spi@1 { ... flash@3 { compatible = "jedec,spi-nor" reg = <0x00 0x01>; paralle-memories ; spi-max-frequency = <50000000>; ... partitions { compatible = "fixed-partitions"; flash0_partition: partition@0 { label = "part0_0"; reg = <0x0 0x800000>; } } } } Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> --- .../bindings/spi/spi-controller.yaml | 23 +++++++++++++++++-- .../bindings/spi/spi-peripheral-props.yaml | 9 +++----- 2 files changed, 24 insertions(+), 8 deletions(-)