mbox series

[v3,0/3] Stacked/parallel memories bindings

Message ID 20211206095921.33302-1-miquel.raynal@bootlin.com
Headers show
Series Stacked/parallel memories bindings | expand

Message

Miquel Raynal Dec. 6, 2021, 9:59 a.m. UTC
Hello Rob, Mark, Tudor & Pratyush,

Here is a third 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 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    | 21 +++++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

Comments

Rob Herring Dec. 6, 2021, 9:31 p.m. UTC | #1
On Mon, Dec 06, 2021 at 10:59:18AM +0100, Miquel Raynal wrote:
> Hello Rob, Mark, Tudor & Pratyush,
> 
> Here is a third versions for these bindings, which applies on top of
> Pratyush's work:
> https://lore.kernel.org/all/20211109181911.2251-1-p.yadav@ti.com/

Mark, can you either provide a stable branch with this or apply the 
series? Note that there's going to be some other patches needing 
spi-peripheral-props.yaml, so providing a branch might be better if you 
don't want to collect the patches.

Rob
Pratyush Yadav Dec. 7, 2021, 7:14 a.m. UTC | #2
On 06/12/21 10:59AM, 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    | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 5dd209206e88..13aa6a2374c9 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -82,6 +82,27 @@ properties:
>      description:
>        Delay, in microseconds, after a write transfer.
>  
> +  stacked-memories:
> +    type: boolean

I don't think a boolean is enough to completely describe the memory. 
Sure, you say the memories are stacked, but where do you specify when to 
switch the CS? They could be two 512 MiB memories, two 1 GiB memories, 
or one 512 MiB and one 256 MiB.

> +    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. XIP is
> +      usually not supported in this mode.
> +
> +  parallel-memories:
> +    type: boolean

With this I assume both memories have to be the same size?

> +    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. XIP is usually not
> +      supported in this mode.
> +
>  # The controller specific properties go here.
>  allOf:
>    - $ref: cdns,qspi-nor-peripheral-props.yaml#
> -- 
> 2.27.0
>
Tudor Ambarus Dec. 7, 2021, 7:35 a.m. UTC | #3
On 12/7/21 9:14 AM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 06/12/21 10:59AM, 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    | 21 +++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>> index 5dd209206e88..13aa6a2374c9 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>> @@ -82,6 +82,27 @@ properties:
>>      description:
>>        Delay, in microseconds, after a write transfer.
>>
>> +  stacked-memories:
>> +    type: boolean
> 
> I don't think a boolean is enough to completely describe the memory.
> Sure, you say the memories are stacked, but where do you specify when to
> switch the CS? They could be two 512 MiB memories, two 1 GiB memories,
> or one 512 MiB and one 256 MiB.

If the multi-die flash contains identical dies then the die boundary can be
determined with flash_size / number_of_cs. Are there any multi die flashes
with different types of dies?

> 
>> +    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. XIP is
>> +      usually not supported in this mode.
>> +
>> +  parallel-memories:
>> +    type: boolean
> 
> With this I assume both memories have to be the same size?

It looks like the assumption for both cases is that the dies are identical.

> 
>> +    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. XIP is usually not
>> +      supported in this mode.
>> +
>>  # The controller specific properties go here.
>>  allOf:
>>    - $ref: cdns,qspi-nor-peripheral-props.yaml#
>> --
>> 2.27.0
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.
>
Tudor Ambarus Dec. 7, 2021, 7:43 a.m. UTC | #4
On 12/7/21 9:35 AM, Tudor Ambarus wrote:
> On 12/7/21 9:14 AM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 06/12/21 10:59AM, 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    | 21 +++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> index 5dd209206e88..13aa6a2374c9 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>> @@ -82,6 +82,27 @@ properties:
>>>      description:
>>>        Delay, in microseconds, after a write transfer.
>>>
>>> +  stacked-memories:
>>> +    type: boolean
>>
>> I don't think a boolean is enough to completely describe the memory.
>> Sure, you say the memories are stacked, but where do you specify when to
>> switch the CS? They could be two 512 MiB memories, two 1 GiB memories,
>> or one 512 MiB and one 256 MiB.
> 
> If the multi-die flash contains identical dies then the die boundary can be
> determined with flash_size / number_of_cs. Are there any multi die flashes

but the problem is there, yes, there is still the case where there are stacked
devices with a single cs. We'll need to describe the size of the die in some
way.
Tudor Ambarus Dec. 7, 2021, 7:47 a.m. UTC | #5
On 12/7/21 9:43 AM, Tudor Ambarus wrote:
> On 12/7/21 9:35 AM, Tudor Ambarus wrote:
>> On 12/7/21 9:14 AM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 06/12/21 10:59AM, 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    | 21 +++++++++++++++++++
>>>>  1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>>> index 5dd209206e88..13aa6a2374c9 100644
>>>> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
>>>> @@ -82,6 +82,27 @@ properties:
>>>>      description:
>>>>        Delay, in microseconds, after a write transfer.
>>>>
>>>> +  stacked-memories:
>>>> +    type: boolean
>>>
>>> I don't think a boolean is enough to completely describe the memory.
>>> Sure, you say the memories are stacked, but where do you specify when to
>>> switch the CS? They could be two 512 MiB memories, two 1 GiB memories,
>>> or one 512 MiB and one 256 MiB.
>>
>> If the multi-die flash contains identical dies then the die boundary can be
>> determined with flash_size / number_of_cs. Are there any multi die flashes
> 
> but the problem is there, yes, there is still the case where there are stacked
> devices with a single cs. We'll need to describe the size of the die in some
> way.
> 

Even better, winbond stacks a NOR and a NAND:
https://www.winbond.com/hq/product/code-storage-flash-memory/spistack-flash/?__locale=en
Pratyush Yadav Dec. 7, 2021, 7:57 a.m. UTC | #6
On 07/12/21 07:35AM, Tudor.Ambarus@microchip.com wrote:
> On 12/7/21 9:14 AM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 06/12/21 10:59AM, 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    | 21 +++++++++++++++++++
> >>  1 file changed, 21 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> >> index 5dd209206e88..13aa6a2374c9 100644
> >> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> >> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> >> @@ -82,6 +82,27 @@ properties:
> >>      description:
> >>        Delay, in microseconds, after a write transfer.
> >>
> >> +  stacked-memories:
> >> +    type: boolean
> > 
> > I don't think a boolean is enough to completely describe the memory.
> > Sure, you say the memories are stacked, but where do you specify when to
> > switch the CS? They could be two 512 MiB memories, two 1 GiB memories,
> > or one 512 MiB and one 256 MiB.
> 
> If the multi-die flash contains identical dies then the die boundary can be
> determined with flash_size / number_of_cs. Are there any multi die flashes
> with different types of dies?

The way I see it, a multi-die flash is not much different from 2 
independent flashes attached to the same SPI bus. So if we are going to 
implement this feature, I want it to be generic enough to allow 
supporting this type of hardware setup as well.

I am not aware of any flashes with a different CS for each die (that 
isn't handled by the flash internally), let alone with different types 
of dies. IIRC from our IRC conversation, Miquel's use case was using 2 
smaller identical flashes connected to the same SPI bus with 1 CS each. 
Do I remember this right Miquel?

> 
> > 
> >> +    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. XIP is
> >> +      usually not supported in this mode.
> >> +
> >> +  parallel-memories:
> >> +    type: boolean
> > 
> > With this I assume both memories have to be the same size?
> 
> It looks like the assumption for both cases is that the dies are identical.

I would like to _not_ assume that for stacked-memories, unless 
implementing that becomes too complicated.

> 
> > 
> >> +    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. XIP is usually not
> >> +      supported in this mode.
> >> +
> >>  # The controller specific properties go here.
> >>  allOf:
> >>    - $ref: cdns,qspi-nor-peripheral-props.yaml#
> >> --
> >> 2.27.0
> >>
> > 
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.
> > 
>
Miquel Raynal Dec. 7, 2021, 8:37 a.m. UTC | #7
Hi Pratyush & Tudor,

p.yadav@ti.com wrote on Tue, 7 Dec 2021 13:27:23 +0530:

> On 07/12/21 07:35AM, Tudor.Ambarus@microchip.com wrote:
> > On 12/7/21 9:14 AM, Pratyush Yadav wrote:  
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > On 06/12/21 10:59AM, 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    | 21 +++++++++++++++++++
> > >>  1 file changed, 21 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > >> index 5dd209206e88..13aa6a2374c9 100644
> > >> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > >> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > >> @@ -82,6 +82,27 @@ properties:
> > >>      description:
> > >>        Delay, in microseconds, after a write transfer.
> > >>
> > >> +  stacked-memories:
> > >> +    type: boolean  
> > > 
> > > I don't think a boolean is enough to completely describe the memory.
> > > Sure, you say the memories are stacked, but where do you specify when to
> > > switch the CS? They could be two 512 MiB memories, two 1 GiB memories,
> > > or one 512 MiB and one 256 MiB.  
> > 
> > If the multi-die flash contains identical dies then the die boundary can be
> > determined with flash_size / number_of_cs. Are there any multi die flashes
> > with different types of dies?  
> 
> The way I see it, a multi-die flash is not much different from 2 
> independent flashes attached to the same SPI bus. So if we are going to 
> implement this feature, I want it to be generic enough to allow 
> supporting this type of hardware setup as well.
> 
> I am not aware of any flashes with a different CS for each die (that 
> isn't handled by the flash internally), let alone with different types 
> of dies. IIRC from our IRC conversation, Miquel's use case was using 2 
> smaller identical flashes connected to the same SPI bus with 1 CS each. 
> Do I remember this right Miquel?

I made the assumption that dies would be identical in order to use this
mode. However, if you think this is too risky I see two alternatives:
* Keep the bindings as I proposed and if we ever have the case, add
  another property, something like:
	stacked-memories;
	stacked-sizes = <x>, <y>;
* Merge these two properties into one:
	stacked-memories = <x>, <y>;

But TBH I prefer the former solution for these two reasons:
1/ You need to know the devices exact geometry when writing the
   bindings while this is something that is usually let to the core and
   the hardware designers.
2/ I am not sure this is really a valid use case. If we ever need to
   concatenate two devices, in particular if they are different, I
   would prefer reviving the mtd-concat series which, besides lacking a
   dynamic discovery feature, is almost ready to be used. Plus, adding
   too much complexity to the core logic (such as handling different
   die sizes) might impact negatively the overall performances even for
   simpler devices.

> > >> +    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. XIP is
> > >> +      usually not supported in this mode.
> > >> +
> > >> +  parallel-memories:
> > >> +    type: boolean  
> > > 
> > > With this I assume both memories have to be the same size?  
> > 
> > It looks like the assumption for both cases is that the dies are identical.  
> 
> I would like to _not_ assume that for stacked-memories, unless 
> implementing that becomes too complicated.
> 
> >   
> > >   
> > >> +    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. XIP is usually not
> > >> +      supported in this mode.
> > >> +
> > >>  # The controller specific properties go here.
> > >>  allOf:
> > >>    - $ref: cdns,qspi-nor-peripheral-props.yaml#
> > >> --
> > >> 2.27.0
> > >>  
> > > 
> > > --
> > > Regards,
> > > Pratyush Yadav
> > > Texas Instruments Inc.
> > >   
> >   
> 


Thanks,
Miquèl
Mark Brown Dec. 7, 2021, 2:31 p.m. UTC | #8
On Mon, Dec 06, 2021 at 03:31:32PM -0600, Rob Herring wrote:

> Mark, can you either provide a stable branch with this or apply the 
> series? Note that there's going to be some other patches needing 
> spi-peripheral-props.yaml, so providing a branch might be better if you 
> don't want to collect the patches.

I'll apply the series but there's still debate about it so it looks like
there will be further revisions.  Unfortunately the patch is buried in
the middle of history so it's hard to pull out a sensible stable branch,
Linus gets upset about topic branches which makes doing this sort of
thing retroactively painful.