* [PATCH v4 0/3] Stacked/parallel memories bindings @ 2021-12-10 20:10 Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 1/3] dt-bindings: mtd: spi-nor: Allow two CS per device Miquel Raynal ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Miquel Raynal @ 2021-12-10 20:10 UTC (permalink / raw) To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd Cc: Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi, Miquel Raynal Hello Rob, Mark, Tudor & Pratyush, Here is a fourth versions for these bindings, which applies on top of Pratyush's work: https://lore.kernel.org/all/20211109181911.2251-1-p.yadav@ti.com/ Cheers, Miquèl Changes in v4: * Changed the type of properties to uint64-arrays in order to be able to describe the size of each element in the array. * Updated the example accordingly. Changes in v3: * Rebased on top of Pratyush's recent changes. * Dropped the commit allowing to provide two reg entries on the node name. * Dropped the commit referencing spi-controller.yaml from jedec,spi-nor.yaml, now replaced by spi-peripheral-props.yaml and already done in Pratyush's series. * Added Rob's Ack. * Enhanced a commit message. * Moved the new properties to the new SPI peripheral binding file. Changes in v2: * Dropped the dtc changes for now. * Moved the properties in the device's nodes, not the controller's. * Dropped the useless #address-cells change. * Added a missing "minItems". * Moved the new properties in the spi-controller.yaml file. * Added an example using two stacked memories in the spi-controller.yaml file. * Renamed the properties to drop the Xilinx prefix. * Added a patch to fix the spi-nor jedec yaml file. Miquel Raynal (3): dt-bindings: mtd: spi-nor: Allow two CS per device spi: dt-bindings: Describe stacked/parallel memories modes spi: dt-bindings: Add an example with two stacked flashes .../bindings/mtd/jedec,spi-nor.yaml | 3 +- .../bindings/spi/spi-controller.yaml | 7 +++++ .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 1/3] dt-bindings: mtd: spi-nor: Allow two CS per device 2021-12-10 20:10 [PATCH v4 0/3] Stacked/parallel memories bindings Miquel Raynal @ 2021-12-10 20:10 ` Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes Miquel Raynal 2 siblings, 0 replies; 14+ messages in thread From: Miquel Raynal @ 2021-12-10 20:10 UTC (permalink / raw) To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd Cc: Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi, Miquel Raynal, Rob Herring The Xilinx QSPI controller has two advanced modes which allow the controller to behave differently and consider two flashes as one single storage. One of these two modes is quite complex to support from a binding point of view and is the dual parallel memories. In this mode, each byte of data is stored in both devices: the even bits in one, the odd bits in the other. The split is automatically handled by the QSPI controller and is transparent for the user. The other mode is simpler to support, it is called dual stacked memories. The controller shares the same SPI bus but each of the devices contain half of the data. Once in this mode, the controller does not follow CS requests but instead internally wires the two CS levels with the value of the most significant address bit. Supporting these two modes will involve core changes which include the possibility of providing two CS for a single SPI device Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Acked-by: Rob Herring <robh@kernel.org> --- Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml index 39421f7233e4..4abfb4cfc157 100644 --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml @@ -47,7 +47,8 @@ properties: identified by the JEDEC READ ID opcode (0x9F). reg: - maxItems: 1 + minItems: 1 + maxItems: 2 spi-max-frequency: true spi-rx-bus-width: true -- 2.27.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-10 20:10 [PATCH v4 0/3] Stacked/parallel memories bindings Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 1/3] dt-bindings: mtd: spi-nor: Allow two CS per device Miquel Raynal @ 2021-12-10 20:10 ` Miquel Raynal 2021-12-14 17:32 ` Rob Herring 2021-12-14 19:44 ` Pratyush Yadav 2021-12-10 20:10 ` [PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes Miquel Raynal 2 siblings, 2 replies; 14+ messages in thread From: Miquel Raynal @ 2021-12-10 20:10 UTC (permalink / raw) To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd Cc: Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi, Miquel Raynal Describe two new memories modes: - A stacked mode when the bus is common but the address space extended with an additinals wires. - A parallel mode with parallel busses accessing parallel flashes where the data is spread. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml index 5dd209206e88..4194fee8f556 100644 --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml @@ -82,6 +82,35 @@ properties: description: Delay, in microseconds, after a write transfer. + stacked-memories: + $ref: /schemas/types.yaml#/definitions/uint64-matrix + description: Several SPI memories can be wired in stacked mode. + This basically means that either a device features several chip + selects, or that different devices must be seen as a single + bigger chip. This basically doubles (or more) the total address + 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. + minItems: 2 + maxItems: 2 + items: + maxItems: 1 + + parallel-memories: + $ref: /schemas/types.yaml#/definitions/uint64-matrix + description: Several SPI memories can be wired in parallel mode. + The devices are physically on a different buses but will always + act synchronously as each data word is spread across the + 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. + minItems: 2 + maxItems: 2 + items: + maxItems: 1 + # The controller specific properties go here. allOf: - $ref: cdns,qspi-nor-peripheral-props.yaml# -- 2.27.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-10 20:10 ` [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes Miquel Raynal @ 2021-12-14 17:32 ` Rob Herring 2021-12-16 15:02 ` Miquel Raynal 2021-12-14 19:44 ` Pratyush Yadav 1 sibling, 1 reply; 14+ messages in thread From: Rob Herring @ 2021-12-14 17:32 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, devicetree, Mark Brown, linux-spi On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote: > Describe two new memories modes: > - A stacked mode when the bus is common but the address space extended > with an additinals wires. > - A parallel mode with parallel busses accessing parallel flashes where > the data is spread. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > index 5dd209206e88..4194fee8f556 100644 > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > @@ -82,6 +82,35 @@ properties: > description: > Delay, in microseconds, after a write transfer. > > + stacked-memories: > + $ref: /schemas/types.yaml#/definitions/uint64-matrix matrix or... > + description: Several SPI memories can be wired in stacked mode. > + This basically means that either a device features several chip > + selects, or that different devices must be seen as a single > + bigger chip. This basically doubles (or more) the total address > + 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. array? Sounds like an array from the description as there is only 1 element, the size. > + minItems: 2 > + maxItems: 2 > + items: > + maxItems: 1 This says you can only have 2 64-bit entries. Probably not what you want. This looks like a case for a maxItems 'should be enough for now' type of value. > + > + parallel-memories: > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > + description: Several SPI memories can be wired in parallel mode. > + The devices are physically on a different buses but will always > + act synchronously as each data word is spread across the > + 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. > + minItems: 2 > + maxItems: 2 > + items: > + maxItems: 1 > + > # The controller specific properties go here. > allOf: > - $ref: cdns,qspi-nor-peripheral-props.yaml# > -- > 2.27.0 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-14 17:32 ` Rob Herring @ 2021-12-16 15:02 ` Miquel Raynal 2022-01-10 8:31 ` Miquel Raynal 2022-01-21 15:54 ` Rob Herring 0 siblings, 2 replies; 14+ messages in thread From: Miquel Raynal @ 2021-12-16 15:02 UTC (permalink / raw) To: Rob Herring Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, devicetree, Mark Brown, linux-spi Hi Rob, robh@kernel.org wrote on Tue, 14 Dec 2021 11:32:56 -0600: > On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote: > > Describe two new memories modes: > > - A stacked mode when the bus is common but the address space extended > > with an additinals wires. > > - A parallel mode with parallel busses accessing parallel flashes where > > the data is spread. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > index 5dd209206e88..4194fee8f556 100644 > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > @@ -82,6 +82,35 @@ properties: > > description: > > Delay, in microseconds, after a write transfer. > > > > + stacked-memories: > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > matrix or... > > > + description: Several SPI memories can be wired in stacked mode. > > + This basically means that either a device features several chip > > + selects, or that different devices must be seen as a single > > + bigger chip. This basically doubles (or more) the total address > > + 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. > > array? > > Sounds like an array from the description as there is only 1 element, > the size. Well, what I expected to have was something like: dt: <property> = <uint64>, <uint64>; It seemed like the only possible way (that the tooling would validate) was to use: bindings: $ref: /schemas/types.yaml#/definitions/uint64-matrix So I assumed I was defining a matrix of AxB elements, where A is the number of devices I want to "stack" and B is the number of values needed to describe its size, so 1. I realized that the following example, which I was expecting to work, was failing: bindings: $ref: /schemas/types.yaml#/definitions/uint64-array dt: <property> = <uint64>, <uint64>; Indeed, as you propose, this actually works but describes two values (tied somehow) into a single element, which is not exactly what I wanted: bindings: $ref: /schemas/types.yaml#/definitions/uint64-array dt: <property> = <uint64 uint64>; But more disturbing, all the following constructions worked, when using 32-bits values instead: bindings: $ref: /schemas/types.yaml#/definitions/uint32-array dt: <property> = <uint32 uint32>; bindings: $ref: /schemas/types.yaml#/definitions/uint32-array dt: <property> = <uint32>, <uint32>; bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix dt: <property> = <uint32 uint32>; bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix dt: <property> = <uint32>, <uint32>; I am fine waiting a bit if you think there is a need for some tooling update on your side. Otherwise, do you really think that this solution is the one we should really use? bindings: $ref: /schemas/types.yaml#/definitions/uint64-array dt: <property> = <uint64 uint64>; Because from my point of view it does not match what we usually do for other "types" of elements, such as: dt: <property> = <phandle1 index1>, <phandle2 index2>; or dt: <property> = <small-val1>, <small-val2>; > > > + minItems: 2 > > + maxItems: 2 > > + items: > > + maxItems: 1 > > This says you can only have 2 64-bit entries. Probably not what you > want. This looks like a case for a maxItems 'should be enough for now' > type of value. Yes, that is what I wanted to describe. In my recent contributions you always preferred to bound things as much as possible, even though later it might become necessary to loosen the constraint. Right now I see the use of these properties for 2 devices, but in theory there is no limit. Of course if we switch to the array representation I suppose I should stick to: + minItems: 2 + maxItems: 2 > > > + > > + parallel-memories: > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > + description: Several SPI memories can be wired in parallel mode. > > + The devices are physically on a different buses but will always > > + act synchronously as each data word is spread across the > > + 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. > > + minItems: 2 > > + maxItems: 2 > > + items: > > + maxItems: 1 > > + > > # The controller specific properties go here. > > allOf: > > - $ref: cdns,qspi-nor-peripheral-props.yaml# > > -- > > 2.27.0 > > > > Thanks, Miquèl ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-16 15:02 ` Miquel Raynal @ 2022-01-10 8:31 ` Miquel Raynal 2022-01-21 15:54 ` Rob Herring 1 sibling, 0 replies; 14+ messages in thread From: Miquel Raynal @ 2022-01-10 8:31 UTC (permalink / raw) To: Rob Herring Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, devicetree, Mark Brown, linux-spi Hi Rob, miquel.raynal@bootlin.com wrote on Thu, 16 Dec 2021 16:02:26 +0100: > Hi Rob, > > robh@kernel.org wrote on Tue, 14 Dec 2021 11:32:56 -0600: > > > On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote: > > > Describe two new memories modes: > > > - A stacked mode when the bus is common but the address space extended > > > with an additinals wires. > > > - A parallel mode with parallel busses accessing parallel flashes where > > > the data is spread. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > > > 1 file changed, 29 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > index 5dd209206e88..4194fee8f556 100644 > > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > @@ -82,6 +82,35 @@ properties: > > > description: > > > Delay, in microseconds, after a write transfer. > > > > > > + stacked-memories: > > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > > > matrix or... > > > > > + description: Several SPI memories can be wired in stacked mode. > > > + This basically means that either a device features several chip > > > + selects, or that different devices must be seen as a single > > > + bigger chip. This basically doubles (or more) the total address > > > + 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. > > > > array? > > > > Sounds like an array from the description as there is only 1 element, > > the size. > > Well, what I expected to have was something like: > > dt: <property> = <uint64>, <uint64>; > > It seemed like the only possible way (that the tooling would validate) > was to use: > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-matrix > > So I assumed I was defining a matrix of AxB elements, where A is the > number of devices I want to "stack" and B is the number of values > needed to describe its size, so 1. > > I realized that the following example, which I was expecting to work, > was failing: > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > dt: <property> = <uint64>, <uint64>; > > Indeed, as you propose, this actually works but describes two values > (tied somehow) into a single element, which is not exactly what I > wanted: > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > dt: <property> = <uint64 uint64>; > > But more disturbing, all the following constructions worked, when using > 32-bits values instead: > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-array > dt: <property> = <uint32 uint32>; > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-array > dt: <property> = <uint32>, <uint32>; > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix > dt: <property> = <uint32 uint32>; > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix > dt: <property> = <uint32>, <uint32>; > > I am fine waiting a bit if you think there is a need for some tooling > update on your side. Otherwise, do you really think that this solution > is the one we should really use? > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > dt: <property> = <uint64 uint64>; > > Because from my point of view it does not match what we usually do for > other "types" of elements, such as: > > dt: <property> = <phandle1 index1>, <phandle2 index2>; > > or > > dt: <property> = <small-val1>, <small-val2>; Sorry for bothering you, is this something you still have in mind? It seems that the tooling is the culprit here and I would highly appreciate your help on that point. Thanks, Miquèl > > > > > > + minItems: 2 > > > + maxItems: 2 > > > + items: > > > + maxItems: 1 > > > > This says you can only have 2 64-bit entries. Probably not what you > > want. This looks like a case for a maxItems 'should be enough for now' > > type of value. > > Yes, that is what I wanted to describe. > > In my recent contributions you always preferred to bound things as much > as possible, even though later it might become necessary to loosen the > constraint. Right now I see the use of these properties for 2 devices, > but in theory there is no limit. > > Of course if we switch to the array representation I suppose I should > stick to: > > + minItems: 2 > + maxItems: 2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-16 15:02 ` Miquel Raynal 2022-01-10 8:31 ` Miquel Raynal @ 2022-01-21 15:54 ` Rob Herring 2022-01-26 11:18 ` Miquel Raynal 1 sibling, 1 reply; 14+ messages in thread From: Rob Herring @ 2022-01-21 15:54 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, devicetree, Mark Brown, linux-spi On Thu, Dec 16, 2021 at 9:02 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Rob, > > robh@kernel.org wrote on Tue, 14 Dec 2021 11:32:56 -0600: > > > On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote: > > > Describe two new memories modes: > > > - A stacked mode when the bus is common but the address space extended > > > with an additinals wires. > > > - A parallel mode with parallel busses accessing parallel flashes where > > > the data is spread. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > > > 1 file changed, 29 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > index 5dd209206e88..4194fee8f556 100644 > > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > @@ -82,6 +82,35 @@ properties: > > > description: > > > Delay, in microseconds, after a write transfer. > > > > > > + stacked-memories: > > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > > > matrix or... > > > > > + description: Several SPI memories can be wired in stacked mode. > > > + This basically means that either a device features several chip > > > + selects, or that different devices must be seen as a single > > > + bigger chip. This basically doubles (or more) the total address > > > + 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. > > > > array? > > > > Sounds like an array from the description as there is only 1 element, > > the size. > > Well, what I expected to have was something like: > > dt: <property> = <uint64>, <uint64>; The downside to this is you really need: property = /bits/ 64 <uint64>, /bits/ 64 <uint64>; And I just fixed some examples that had: property = /bits/ 64 <uint64>, <uint64>; The property length there is 12 bytes. That aspect of the syntax is really not obvious. I only realized that somewhat recently. You can even do: property = /bits/ 64 <uint64>, "a string", /bits/ 8 <uint8>; While dts syntax allows this, our sanity does not. > It seemed like the only possible way (that the tooling would validate) > was to use: > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-matrix > > So I assumed I was defining a matrix of AxB elements, where A is the > number of devices I want to "stack" and B is the number of values > needed to describe its size, so 1. Yeah, that's well reasoned and I agree. The other array case is you have N values where each value represents different data rather than instances of the same data. The challenge is for the schema fixups to recognize which is which without saying the schema must look like exactly X or Y as there will be exceptions. > I realized that the following example, which I was expecting to work, > was failing: > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > dt: <property> = <uint64>, <uint64>; > > Indeed, as you propose, this actually works but describes two values > (tied somehow) into a single element, which is not exactly what I > wanted: > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > dt: <property> = <uint64 uint64>; > > But more disturbing, all the following constructions worked, when using > 32-bits values instead: > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-array > dt: <property> = <uint32 uint32>; > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-array > dt: <property> = <uint32>, <uint32>; > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix > dt: <property> = <uint32 uint32>; > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix > dt: <property> = <uint32>, <uint32>; That works because there's some really ugly code to transform the schema into both forms. > I am fine waiting a bit if you think there is a need for some tooling > update on your side. Otherwise, do you really think that this solution > is the one we should really use? > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > dt: <property> = <uint64 uint64>; Because of the /bits/ issue, yes. More importantly, the bracketing in dts files is not going to matter soon (from a validation perspective). I'm working on moving validation from using the yaml encoded DT (which depends on and preserves brackets) to using dtbs. This will use the schemas to decode the property values into the right format/type. > Because from my point of view it does not match what we usually do for > other "types" of elements, such as: > > dt: <property> = <phandle1 index1>, <phandle2 index2>; > > or > > dt: <property> = <small-val1>, <small-val2>; > > > > > > + minItems: 2 > > > + maxItems: 2 > > > + items: > > > + maxItems: 1 > > > > This says you can only have 2 64-bit entries. Probably not what you > > want. This looks like a case for a maxItems 'should be enough for now' > > type of value. > > Yes, that is what I wanted to describe. > > In my recent contributions you always preferred to bound things as much > as possible, even though later it might become necessary to loosen the > constraint. Right now I see the use of these properties for 2 devices, > but in theory there is no limit. Ok, as long as we're not loosening it frequently. Rob ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2022-01-21 15:54 ` Rob Herring @ 2022-01-26 11:18 ` Miquel Raynal 0 siblings, 0 replies; 14+ messages in thread From: Miquel Raynal @ 2022-01-26 11:18 UTC (permalink / raw) To: Rob Herring Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, devicetree, Mark Brown, linux-spi Hi Rob, > > It seemed like the only possible way (that the tooling would validate) > > was to use: > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-matrix > > > > So I assumed I was defining a matrix of AxB elements, where A is the > > number of devices I want to "stack" and B is the number of values > > needed to describe its size, so 1. > > Yeah, that's well reasoned and I agree. The other array case is you > have N values where each value represents different data rather than > instances of the same data. The challenge is for the schema fixups to > recognize which is which without saying the schema must look like > exactly X or Y as there will be exceptions. Ok, now I see the problem on the tooling side and why you chose not to use this syntax. > > I realized that the following example, which I was expecting to work, > > was failing: > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > > dt: <property> = <uint64>, <uint64>; > > > > Indeed, as you propose, this actually works but describes two values > > (tied somehow) into a single element, which is not exactly what I > > wanted: > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > > dt: <property> = <uint64 uint64>; > > > > But more disturbing, all the following constructions worked, when using > > 32-bits values instead: > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-array > > dt: <property> = <uint32 uint32>; > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-array > > dt: <property> = <uint32>, <uint32>; > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix > > dt: <property> = <uint32 uint32>; > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint32-matrix > > dt: <property> = <uint32>, <uint32>; > > That works because there's some really ugly code to transform the > schema into both forms. Good to know, this kind of puzzled me when I tried all the configurations :) > > I am fine waiting a bit if you think there is a need for some tooling > > update on your side. Otherwise, do you really think that this solution > > is the one we should really use? > > > > bindings: $ref: /schemas/types.yaml#/definitions/uint64-array > > dt: <property> = <uint64 uint64>; > > Because of the /bits/ issue, yes. > > More importantly, the bracketing in dts files is not going to matter > soon (from a validation perspective). I'm working on moving validation > from using the yaml encoded DT (which depends on and preserves > brackets) to using dtbs. This will use the schemas to decode the > property values into the right format/type. Ok. Well, thanks for the feedback, with the latest dt-schema the tooling now validates the binding so I am going to send it as a v6 to collect your Ack. Thanks, Miquèl ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-10 20:10 ` [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes Miquel Raynal 2021-12-14 17:32 ` Rob Herring @ 2021-12-14 19:44 ` Pratyush Yadav 2021-12-16 16:25 ` Miquel Raynal 1 sibling, 1 reply; 14+ messages in thread From: Pratyush Yadav @ 2021-12-14 19:44 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi Hi Miquel, On 10/12/21 09:10PM, Miquel Raynal wrote: > Describe two new memories modes: > - A stacked mode when the bus is common but the address space extended > with an additinals wires. > - A parallel mode with parallel busses accessing parallel flashes where > the data is spread. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > index 5dd209206e88..4194fee8f556 100644 > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > @@ -82,6 +82,35 @@ properties: > description: > Delay, in microseconds, after a write transfer. > > + stacked-memories: > + $ref: /schemas/types.yaml#/definitions/uint64-matrix Why matrix? Can't you use array here? Sorry, I am not much familiar with JSON schema. > + description: Several SPI memories can be wired in stacked mode. > + This basically means that either a device features several chip > + selects, or that different devices must be seen as a single > + bigger chip. This basically doubles (or more) the total address > + 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. > + minItems: 2 > + maxItems: 2 > + items: > + maxItems: 1 Thanks. This looks better to me. But before we go ahead, I think there has been some confusion around what exactly your patches intend to support. Let's clear them up first. What type of setup do you want to support? 1. One single flash but with multiple dies, with each die sitting on a different CS. 2. Two (or more) identical but independent flash memories to be treated as one. 3. Two (or more) different and independent flash memories to be treated as one. In our earlier exchanges you said you want to support 2. And when I wanted you to account for 3 as well you said we should use mtdconcat for that. So my question is, why can't we use mtdconcat for 2 as well, since it is just a special case of 3? And if we are using mtdconcat, then why do we need this at all? Shouldn't you then choose the chip at MTD layer and use the respective SPI device to get the CS value, which would make this property useless? I can see this making sense for case 1. For that case you said you don't have an existing datasheet or device to propose. And if there is no real device doing it I see little point in figuring out a binding for it. -- Regards, Pratyush Yadav Texas Instruments Inc. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-14 19:44 ` Pratyush Yadav @ 2021-12-16 16:25 ` Miquel Raynal 2021-12-17 12:39 ` Pratyush Yadav 0 siblings, 1 reply; 14+ messages in thread From: Miquel Raynal @ 2021-12-16 16:25 UTC (permalink / raw) To: Pratyush Yadav Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi Hello Pratyush, p.yadav@ti.com wrote on Wed, 15 Dec 2021 01:14:33 +0530: > Hi Miquel, > > On 10/12/21 09:10PM, Miquel Raynal wrote: > > Describe two new memories modes: > > - A stacked mode when the bus is common but the address space extended > > with an additinals wires. > > - A parallel mode with parallel busses accessing parallel flashes where > > the data is spread. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > > 1 file changed, 29 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > index 5dd209206e88..4194fee8f556 100644 > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > @@ -82,6 +82,35 @@ properties: > > description: > > Delay, in microseconds, after a write transfer. > > > > + stacked-memories: > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > Why matrix? Can't you use array here? Sorry, I am not much familiar with > JSON schema. Yes, Rob also opened the discussion, I've answered there on this point, but I agree, this should be define as an array, but the current tooling refused to accept what I wanted from a dt-writing point of view. More on that on the dedicated thread. > > + description: Several SPI memories can be wired in stacked mode. > > + This basically means that either a device features several chip > > + selects, or that different devices must be seen as a single > > + bigger chip. This basically doubles (or more) the total address > > + 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. > > + minItems: 2 > > + maxItems: 2 > > + items: > > + maxItems: 1 > > Thanks. This looks better to me. > > But before we go ahead, I think there has been some confusion around > what exactly your patches intend to support. Let's clear them up first. > What type of setup do you want to support? Before I try to clarify your questions below, the setup that I have is: Xilinx ZynqMP QSPI controller + 2 flashes. What I want to describe is the specific handling of what the Xilinx QSPI controller is able to do. This controller has two modes when wired to more than one flash: - stacked - parallel I have not entered the documentation nor the code in details yet, but I believe that handling two flashes in the stacked configuration, besides a couple of possible optimizations that are possible by the hardware, is close to what any controller would do. Maybe there is one difference though, when in the "stacked" mode, this controller treats the two flashes as a single one, so that is why, if we want to support this "advanced" mode, we *need* a way to know that this mode should be used because the controller expects a wider range of addresses. About the parallel configuration, there is absolutely no doubt that we have no other choice than describing how the data is spread across two devices. We don't really care about the manner, but the controller needs to know how to assert the two CS internally so this definitely something that needs to be described. Now the question you might ask is, why do we define these properties as flash properties then? And this is a real good question, I think both actually work as long as we consider that we can only have either a spi-memory or any other type of device on a single bus, but not both at the same time. In v1 I proposed a controller property. Mark proposed to switch for a device property which I did in v2 onward. > 1. One single flash but with multiple dies, with each die sitting on a > different CS. Yes. > 2. Two (or more) identical but independent flash memories to be > treated as one. Yes. > 3. Two (or more) different and independent flash memories to be > treated as one. I don't know. My first proposal excluded these, but I believe we can handle them with the change your proposed (the device size as part of the property). > In our earlier exchanges you said you want to support 2. And when I > wanted you to account for 3 as well you said we should use mtdconcat for > that. Not that we should, but that we could because from a core perspective it's way more challenging than supporting identical devices. But the conversation drifted about the software backend that we should use rather than on the actual description, because mtdconcat is not a feature which benefits from any kind of description yet, so even if we use mtdconcat as backed, we will need some kind of description here as well. So, as I told previously, I am fine considering these setups in my proposal, that's why I updated my proposal to include the devices size, even though that is way out of scope compared to my initial target. But here we are talking about driver code, which has nothing to do with the bindings. If we focus on the bindings, I believe the solution with the sizes covers all cases. > So my question is, why can't we use mtdconcat for 2 as well, since > it is just a special case of 3? And if we are using mtdconcat, then why > do we need this at all? Shouldn't you then choose the chip at MTD layer > and use the respective SPI device to get the CS value, which would make > this property useless? Reason 1: As depicted above, while the stacked mode might more or less fit the mtdconcat idea, it is absolutely not the case for the parallel mode. Reason 2: mtdconcat is a software backend. Here we are discussing bindings. No matter what backed we finally pick, there will be the need for a proper description and so far there has been no binding for mtdconcat (even though I tried to push in favor of it a while ago without success). So no matter what software solution we we adopt, we *will* need an upstream binding description at some point. But mtdconcat really means "there are two devices we want to consider as a single". While here the point is: we have two devices that get abstracted by the controller, and we still must describe that. > I can see this making sense for case 1. For that case you said you don't > have an existing datasheet or device to propose. And if there is no real > device doing it I see little point in figuring out a binding for it. Yes, because somewhat we focused the debate over the devices, while I was initially talking about a controller abstraction. There is (at least) one controller doing such abstractions, the Xilinx ZynqMP generic QSPI controller, the spec is public (chapter 24): https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf I hope all this clarifies the situation! Cheers, Miquèl ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-16 16:25 ` Miquel Raynal @ 2021-12-17 12:39 ` Pratyush Yadav 2021-12-17 12:58 ` Miquel Raynal 0 siblings, 1 reply; 14+ messages in thread From: Pratyush Yadav @ 2021-12-17 12:39 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi On 16/12/21 05:25PM, Miquel Raynal wrote: > Hello Pratyush, > > p.yadav@ti.com wrote on Wed, 15 Dec 2021 01:14:33 +0530: > > > Hi Miquel, > > > > On 10/12/21 09:10PM, Miquel Raynal wrote: > > > Describe two new memories modes: > > > - A stacked mode when the bus is common but the address space extended > > > with an additinals wires. > > > - A parallel mode with parallel busses accessing parallel flashes where > > > the data is spread. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > > > 1 file changed, 29 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > index 5dd209206e88..4194fee8f556 100644 > > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > @@ -82,6 +82,35 @@ properties: > > > description: > > > Delay, in microseconds, after a write transfer. > > > > > > + stacked-memories: > > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > > > Why matrix? Can't you use array here? Sorry, I am not much familiar with > > JSON schema. > > Yes, Rob also opened the discussion, I've answered there on this point, > but I agree, this should be define as an array, but the current tooling > refused to accept what I wanted from a dt-writing point of view. More > on that on the dedicated thread. > > > > + description: Several SPI memories can be wired in stacked mode. > > > + This basically means that either a device features several chip > > > + selects, or that different devices must be seen as a single > > > + bigger chip. This basically doubles (or more) the total address > > > + 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. > > > + minItems: 2 > > > + maxItems: 2 > > > + items: > > > + maxItems: 1 > > > > Thanks. This looks better to me. > > > > But before we go ahead, I think there has been some confusion around > > what exactly your patches intend to support. Let's clear them up first. > > What type of setup do you want to support? > > Before I try to clarify your questions below, the setup that I have is: > > Xilinx ZynqMP QSPI controller + 2 flashes. > > What I want to describe is the specific handling of what the Xilinx > QSPI controller is able to do. This controller has two modes when wired > to more than one flash: > - stacked > - parallel > > I have not entered the documentation nor the code in details yet, but I > believe that handling two flashes in the stacked configuration, besides > a couple of possible optimizations that are possible by the hardware, > is close to what any controller would do. Maybe there is one difference > though, when in the "stacked" mode, this controller treats the two > flashes as a single one, so that is why, if we want to support this > "advanced" mode, we *need* a way to know that this mode should be used > because the controller expects a wider range of addresses. Ok. > > About the parallel configuration, there is absolutely no doubt that we > have no other choice than describing how the data is spread across two > devices. We don't really care about the manner, but the controller > needs to know how to assert the two CS internally so this definitely > something that needs to be described. Yes. And with device sizes as part of the property we should be able to do this. > > Now the question you might ask is, why do we define these properties as > flash properties then? And this is a real good question, I think both > actually work as long as we consider that we can only have either a > spi-memory or any other type of device on a single bus, but not both at > the same time. In v1 I proposed a controller property. Mark proposed to > switch for a device property which I did in v2 onward. I also think making it a device property makes more sense. You are describing how the flashes are laid out, which is independent of the controller. With the same SoC, I could have one board with a single flash and another with a dual-die flash. > > > 1. One single flash but with multiple dies, with each die sitting on a > > different CS. > > Yes. > > > 2. Two (or more) identical but independent flash memories to be > > treated as one. > > Yes. > > > 3. Two (or more) different and independent flash memories to be > > treated as one. > > I don't know. My first proposal excluded these, but I believe we can > handle them with the change your proposed (the device size as part of > the property). > > > In our earlier exchanges you said you want to support 2. And when I > > wanted you to account for 3 as well you said we should use mtdconcat for > > that. > > Not that we should, but that we could because from a core perspective > it's way more challenging than supporting identical devices. But the > conversation drifted about the software backend that we should use > rather than on the actual description, because mtdconcat is not a > feature which benefits from any kind of description yet, so even if we > use mtdconcat as backed, we will need some kind of description here as > well. So, as I told previously, I am fine considering these setups > in my proposal, that's why I updated my proposal to include the > devices size, even though that is way out of scope compared to my > initial target. Right, and I appreciate that :-) > > But here we are talking about driver code, which has nothing to do with > the bindings. If we focus on the bindings, I believe the solution with > the sizes covers all cases. I think it is important to discuss _how_ we would implement this in the driver because this could help identify flaws or shortcomings in the bindings. In this case, I agree with you that the solution with sizes should cover all 3 cases and we now have lot more flexibility in how we design the driver. > > > So my question is, why can't we use mtdconcat for 2 as well, since > > it is just a special case of 3? And if we are using mtdconcat, then why > > do we need this at all? Shouldn't you then choose the chip at MTD layer > > and use the respective SPI device to get the CS value, which would make > > this property useless? > > Reason 1: > As depicted above, while the stacked mode might more or less > fit the mtdconcat idea, it is absolutely not the case for the parallel > mode. Right. My question was mostly about the stacked mode. > > Reason 2: > mtdconcat is a software backend. Here we are discussing bindings. > No matter what backed we finally pick, there will be the need for a > proper description and so far there has been no binding for mtdconcat > (even though I tried to push in favor of it a while ago without > success). So no matter what software solution we we adopt, we > *will* need an upstream binding description at some point. But > mtdconcat really means "there are two devices we want to consider as a > single". While here the point is: we have two devices that get > abstracted by the controller, and we still must describe that. > > > I can see this making sense for case 1. For that case you said you don't > > have an existing datasheet or device to propose. And if there is no real > > device doing it I see little point in figuring out a binding for it. > > Yes, because somewhat we focused the debate over the devices, while I > was initially talking about a controller abstraction. There is (at > least) one controller doing such abstractions, the Xilinx ZynqMP > generic QSPI controller, the spec is public (chapter 24): > https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf I think this _is_ a device problem first. The controller part should be easy to do once we solve the device problem. Once we clearly describe how the devices are laid out it should be simple for the controller side to figure out what to do. > > I hope all this clarifies the situation! Thanks. I am quite happy with this patch. I'll drop my R-by once you figure out what type to use with these properties in v5. -- Regards, Pratyush Yadav Texas Instruments Inc. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes 2021-12-17 12:39 ` Pratyush Yadav @ 2021-12-17 12:58 ` Miquel Raynal 0 siblings, 0 replies; 14+ messages in thread From: Miquel Raynal @ 2021-12-17 12:58 UTC (permalink / raw) To: Pratyush Yadav Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi Hello Pratyush, p.yadav@ti.com wrote on Fri, 17 Dec 2021 18:09:10 +0530: > On 16/12/21 05:25PM, Miquel Raynal wrote: > > Hello Pratyush, > > > > p.yadav@ti.com wrote on Wed, 15 Dec 2021 01:14:33 +0530: > > > > > Hi Miquel, > > > > > > On 10/12/21 09:10PM, Miquel Raynal wrote: > > > > Describe two new memories modes: > > > > - A stacked mode when the bus is common but the address space extended > > > > with an additinals wires. > > > > - A parallel mode with parallel busses accessing parallel flashes where > > > > the data is spread. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > .../bindings/spi/spi-peripheral-props.yaml | 29 +++++++++++++++++++ > > > > 1 file changed, 29 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > > index 5dd209206e88..4194fee8f556 100644 > > > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml > > > > @@ -82,6 +82,35 @@ properties: > > > > description: > > > > Delay, in microseconds, after a write transfer. > > > > > > > > + stacked-memories: > > > > + $ref: /schemas/types.yaml#/definitions/uint64-matrix > > > > > > Why matrix? Can't you use array here? Sorry, I am not much familiar with > > > JSON schema. > > > > Yes, Rob also opened the discussion, I've answered there on this point, > > but I agree, this should be define as an array, but the current tooling > > refused to accept what I wanted from a dt-writing point of view. More > > on that on the dedicated thread. > > > > > > + description: Several SPI memories can be wired in stacked mode. > > > > + This basically means that either a device features several chip > > > > + selects, or that different devices must be seen as a single > > > > + bigger chip. This basically doubles (or more) the total address > > > > + 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. > > > > + minItems: 2 > > > > + maxItems: 2 > > > > + items: > > > > + maxItems: 1 > > > > > > Thanks. This looks better to me. > > > > > > But before we go ahead, I think there has been some confusion around > > > what exactly your patches intend to support. Let's clear them up first. > > > What type of setup do you want to support? > > > > Before I try to clarify your questions below, the setup that I have is: > > > > Xilinx ZynqMP QSPI controller + 2 flashes. > > > > What I want to describe is the specific handling of what the Xilinx > > QSPI controller is able to do. This controller has two modes when wired > > to more than one flash: > > - stacked > > - parallel > > > > I have not entered the documentation nor the code in details yet, but I > > believe that handling two flashes in the stacked configuration, besides > > a couple of possible optimizations that are possible by the hardware, > > is close to what any controller would do. Maybe there is one difference > > though, when in the "stacked" mode, this controller treats the two > > flashes as a single one, so that is why, if we want to support this > > "advanced" mode, we *need* a way to know that this mode should be used > > because the controller expects a wider range of addresses. > > Ok. > > > > > About the parallel configuration, there is absolutely no doubt that we > > have no other choice than describing how the data is spread across two > > devices. We don't really care about the manner, but the controller > > needs to know how to assert the two CS internally so this definitely > > something that needs to be described. > > Yes. And with device sizes as part of the property we should be able to > do this. > > > > > Now the question you might ask is, why do we define these properties as > > flash properties then? And this is a real good question, I think both > > actually work as long as we consider that we can only have either a > > spi-memory or any other type of device on a single bus, but not both at > > the same time. In v1 I proposed a controller property. Mark proposed to > > switch for a device property which I did in v2 onward. > > I also think making it a device property makes more sense. You are > describing how the flashes are laid out, which is independent of the > controller. With the same SoC, I could have one board with a single > flash and another with a dual-die flash. > > > > > > 1. One single flash but with multiple dies, with each die sitting on a > > > different CS. > > > > Yes. > > > > > 2. Two (or more) identical but independent flash memories to be > > > treated as one. > > > > Yes. > > > > > 3. Two (or more) different and independent flash memories to be > > > treated as one. > > > > I don't know. My first proposal excluded these, but I believe we can > > handle them with the change your proposed (the device size as part of > > the property). > > > > > In our earlier exchanges you said you want to support 2. And when I > > > wanted you to account for 3 as well you said we should use mtdconcat for > > > that. > > > > Not that we should, but that we could because from a core perspective > > it's way more challenging than supporting identical devices. But the > > conversation drifted about the software backend that we should use > > rather than on the actual description, because mtdconcat is not a > > feature which benefits from any kind of description yet, so even if we > > use mtdconcat as backed, we will need some kind of description here as > > well. So, as I told previously, I am fine considering these setups > > in my proposal, that's why I updated my proposal to include the > > devices size, even though that is way out of scope compared to my > > initial target. > > Right, and I appreciate that :-) > > > > > But here we are talking about driver code, which has nothing to do with > > the bindings. If we focus on the bindings, I believe the solution with > > the sizes covers all cases. > > I think it is important to discuss _how_ we would implement this in the > driver because this could help identify flaws or shortcomings in the > bindings. In this case, I agree with you that the solution with sizes > should cover all 3 cases and we now have lot more flexibility in how we > design the driver. > > > > > > So my question is, why can't we use mtdconcat for 2 as well, since > > > it is just a special case of 3? And if we are using mtdconcat, then why > > > do we need this at all? Shouldn't you then choose the chip at MTD layer > > > and use the respective SPI device to get the CS value, which would make > > > this property useless? > > > > Reason 1: > > As depicted above, while the stacked mode might more or less > > fit the mtdconcat idea, it is absolutely not the case for the parallel > > mode. > > Right. My question was mostly about the stacked mode. > > > > > Reason 2: > > mtdconcat is a software backend. Here we are discussing bindings. > > No matter what backed we finally pick, there will be the need for a > > proper description and so far there has been no binding for mtdconcat > > (even though I tried to push in favor of it a while ago without > > success). So no matter what software solution we we adopt, we > > *will* need an upstream binding description at some point. But > > mtdconcat really means "there are two devices we want to consider as a > > single". While here the point is: we have two devices that get > > abstracted by the controller, and we still must describe that. > > > > > I can see this making sense for case 1. For that case you said you don't > > > have an existing datasheet or device to propose. And if there is no real > > > device doing it I see little point in figuring out a binding for it. > > > > Yes, because somewhat we focused the debate over the devices, while I > > was initially talking about a controller abstraction. There is (at > > least) one controller doing such abstractions, the Xilinx ZynqMP > > generic QSPI controller, the spec is public (chapter 24): > > https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf > > I think this _is_ a device problem first. The controller part should be > easy to do once we solve the device problem. Once we clearly describe > how the devices are laid out it should be simple for the controller side > to figure out what to do. > > > > > I hope all this clarifies the situation! > > Thanks. I am quite happy with this patch. I'll drop my R-by once you > figure out what type to use with these properties in v5. Great! Thank you very much for this discussion, it forced me to clarify things in my head as well :-) Thanks, Miquèl ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes 2021-12-10 20:10 [PATCH v4 0/3] Stacked/parallel memories bindings Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 1/3] dt-bindings: mtd: spi-nor: Allow two CS per device Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes Miquel Raynal @ 2021-12-10 20:10 ` Miquel Raynal 2021-12-14 17:37 ` Rob Herring 2 siblings, 1 reply; 14+ messages in thread From: Miquel Raynal @ 2021-12-10 20:10 UTC (permalink / raw) To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd Cc: Michal Simek, Thomas Petazzoni, Rob Herring, devicetree, Mark Brown, linux-spi, Miquel Raynal, Rob Herring Provide an example of how to describe two flashes in eg. stacked mode. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Reviewed-by: Rob Herring <robh@kernel.org> --- Documentation/devicetree/bindings/spi/spi-controller.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml index 36b72518f565..048d2bbc74a5 100644 --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml @@ -139,4 +139,11 @@ examples: spi-max-frequency = <100000>; reg = <1>; }; + + flash@2 { + compatible = "jedec,spi-nor"; + spi-max-frequency = <50000000>; + reg = <2>, <3>; + stacked-memories = /bits/ 64 <0x10000000>, /bits/ 64 <0x10000000>; + }; }; -- 2.27.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes 2021-12-10 20:10 ` [PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes Miquel Raynal @ 2021-12-14 17:37 ` Rob Herring 0 siblings, 0 replies; 14+ messages in thread From: Rob Herring @ 2021-12-14 17:37 UTC (permalink / raw) To: Miquel Raynal Cc: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus, Pratyush Yadav, Michael Walle, linux-mtd, Michal Simek, Thomas Petazzoni, devicetree, Mark Brown, linux-spi On Fri, Dec 10, 2021 at 09:10:39PM +0100, Miquel Raynal wrote: > Provide an example of how to describe two flashes in eg. stacked mode. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Reviewed-by: Rob Herring <robh@kernel.org> > --- > Documentation/devicetree/bindings/spi/spi-controller.yaml | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml > index 36b72518f565..048d2bbc74a5 100644 > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml > @@ -139,4 +139,11 @@ examples: > spi-max-frequency = <100000>; > reg = <1>; > }; > + > + flash@2 { > + compatible = "jedec,spi-nor"; > + spi-max-frequency = <50000000>; > + reg = <2>, <3>; > + stacked-memories = /bits/ 64 <0x10000000>, /bits/ 64 <0x10000000>; stacked-memories = /bits/ 64 <0x10000000 0x10000000>; While the source syntax allows for multiple (and mixed) /bits/ annotations, that's not something we support really. If you have problems with the schema tools on this, let me know. The handling of this is not the greatest. Rob ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-01-26 11:19 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-10 20:10 [PATCH v4 0/3] Stacked/parallel memories bindings Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 1/3] dt-bindings: mtd: spi-nor: Allow two CS per device Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes Miquel Raynal 2021-12-14 17:32 ` Rob Herring 2021-12-16 15:02 ` Miquel Raynal 2022-01-10 8:31 ` Miquel Raynal 2022-01-21 15:54 ` Rob Herring 2022-01-26 11:18 ` Miquel Raynal 2021-12-14 19:44 ` Pratyush Yadav 2021-12-16 16:25 ` Miquel Raynal 2021-12-17 12:39 ` Pratyush Yadav 2021-12-17 12:58 ` Miquel Raynal 2021-12-10 20:10 ` [PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes Miquel Raynal 2021-12-14 17:37 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).