| Message ID | 20211210201039.729961-3-miquel.raynal@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Stacked/parallel memories bindings | expand |
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 > >
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.
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
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
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.
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
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
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
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#
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(+)