[v11,07/10] mtd: spi-nor: Add stacked memories support in spi-nor

Commit Message

Mahapatra, Amit Kumar Nov. 25, 2023, 9:21 a.m. UTC

Each flash that is connected in stacked mode should have a separate
parameter structure. So, the flash parameter member(*params) of the spi_nor
structure is changed to an array (*params[2]). The array is used to store
the parameters of each flash connected in stacked configuration.

The current implementation assumes that a maximum of two flashes are
connected in stacked mode and both the flashes are of same make but can
differ in sizes. So, except the sizes all other flash parameters of both
the flashes are identical.

SPI-NOR is not aware of the chip_select values, for any incoming request
SPI-NOR will decide the flash index with the help of individual flash size
and the configuration type (single/stacked). SPI-NOR will pass on the flash
index information to the SPI core & SPI driver by setting the appropriate
bit in nor->spimem->spi->cs_index_mask. For example, if nth bit of
nor->spimem->spi->cs_index_mask is set then the driver would
assert/de-assert spi->chip_slect[n].

Signed-off-by: Amit Kumar Mahapatra <[e-mail]>
---
 drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-------
 drivers/mtd/spi-nor/core.h  |   4 +
 include/linux/mtd/spi-nor.h |  15 +-
 3 files changed, 240 insertions(+), 51 deletions(-)

Comments

Tudor Ambarus Dec. 6, 2023, 2:30 p.m. UTC | #1

Hi, Amit,

On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
> Each flash that is connected in stacked mode should have a separate
> parameter structure. So, the flash parameter member(*params) of the spi_nor
> structure is changed to an array (*params[2]). The array is used to store
> the parameters of each flash connected in stacked configuration.
> 
> The current implementation assumes that a maximum of two flashes are
> connected in stacked mode and both the flashes are of same make but can
> differ in sizes. So, except the sizes all other flash parameters of both
> the flashes are identical.

Do you plan to add support for different flashes in stacked mode? If
not, wouldn't it be simpler to have just an array of flash sizes instead
of duplicating the entire params struct?

> 
> SPI-NOR is not aware of the chip_select values, for any incoming request
> SPI-NOR will decide the flash index with the help of individual flash size
> and the configuration type (single/stacked). SPI-NOR will pass on the flash
> index information to the SPI core & SPI driver by setting the appropriate
> bit in nor->spimem->spi->cs_index_mask. For example, if nth bit of
> nor->spimem->spi->cs_index_mask is set then the driver would
> assert/de-assert spi->chip_slect[n].
> 
> Signed-off-by: Amit Kumar Mahapatra <[e-mail]>
> ---
>  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-------
>  drivers/mtd/spi-nor/core.h  |   4 +
>  include/linux/mtd/spi-nor.h |  15 +-
>  3 files changed, 240 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 93ae69b7ff83..e990be7c7eb6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c

cut

> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>  static int spi_nor_late_init_params(struct spi_nor *nor)
>  {
>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
> -	int ret;
> +	struct device_node *np = spi_nor_get_flash_node(nor);
> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> +	u32 idx = 0;
> +	int rc, ret;
>  
>  	if (nor->manufacturer && nor->manufacturer->fixups &&
>  	    nor->manufacturer->fixups->late_init) {
> @@ -2937,6 +3042,44 @@ static int spi_nor_late_init_params(struct spi_nor *nor)
>  	if (params->n_banks > 1)
>  		params->bank_size = div64_u64(params->size, params->n_banks);
>  
> +	nor->num_flash = 0;
> +
> +	/*
> +	 * The flashes that are connected in stacked mode should be of same make.
> +	 * Except the flash size all other properties are identical for all the
> +	 * flashes connected in stacked mode.
> +	 * The flashes that are connected in parallel mode should be identical.
> +	 */
> +	while (idx < SNOR_FLASH_CNT_MAX) {
> +		rc = of_property_read_u64_index(np, "stacked-memories", idx, &flash_size[idx]);

This is a little late in my opinion, as we don't have any sanity check
on the flashes that are stacked on top of the first. We shall at least
read and compare the ID for all.

Cheers,
ta

Tudor Ambarus Dec. 6, 2023, 2:43 p.m. UTC | #2

On 12/6/23 14:30, Tudor Ambarus wrote:
> Hi, Amit,
> 
> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
>> Each flash that is connected in stacked mode should have a separate
>> parameter structure. So, the flash parameter member(*params) of the spi_nor
>> structure is changed to an array (*params[2]). The array is used to store
>> the parameters of each flash connected in stacked configuration.
>>
>> The current implementation assumes that a maximum of two flashes are
>> connected in stacked mode and both the flashes are of same make but can
>> differ in sizes. So, except the sizes all other flash parameters of both
>> the flashes are identical.
> 
> Do you plan to add support for different flashes in stacked mode? If
> not, wouldn't it be simpler to have just an array of flash sizes instead
> of duplicating the entire params struct?
> 
>>
>> SPI-NOR is not aware of the chip_select values, for any incoming request
>> SPI-NOR will decide the flash index with the help of individual flash size
>> and the configuration type (single/stacked). SPI-NOR will pass on the flash
>> index information to the SPI core & SPI driver by setting the appropriate
>> bit in nor->spimem->spi->cs_index_mask. For example, if nth bit of
>> nor->spimem->spi->cs_index_mask is set then the driver would
>> assert/de-assert spi->chip_slect[n].
>>
>> Signed-off-by: Amit Kumar Mahapatra <[e-mail]>
>> ---
>>  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-------
>>  drivers/mtd/spi-nor/core.h  |   4 +
>>  include/linux/mtd/spi-nor.h |  15 +-
>>  3 files changed, 240 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 93ae69b7ff83..e990be7c7eb6 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
> 
> cut
> 
>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>>  static int spi_nor_late_init_params(struct spi_nor *nor)
>>  {
>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
>> -	int ret;
>> +	struct device_node *np = spi_nor_get_flash_node(nor);
>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
>> +	u32 idx = 0;
>> +	int rc, ret;
>>  
>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
>>  	    nor->manufacturer->fixups->late_init) {
>> @@ -2937,6 +3042,44 @@ static int spi_nor_late_init_params(struct spi_nor *nor)
>>  	if (params->n_banks > 1)
>>  		params->bank_size = div64_u64(params->size, params->n_banks);
>>  
>> +	nor->num_flash = 0;
>> +
>> +	/*
>> +	 * The flashes that are connected in stacked mode should be of same make.
>> +	 * Except the flash size all other properties are identical for all the
>> +	 * flashes connected in stacked mode.
>> +	 * The flashes that are connected in parallel mode should be identical.
>> +	 */
>> +	while (idx < SNOR_FLASH_CNT_MAX) {
>> +		rc = of_property_read_u64_index(np, "stacked-memories", idx, &flash_size[idx]);

also, it's not clear to me why you read this property multiple times.
Have you sent a device tree patch somewhere? It will help me understand
what you're trying to achieve.

> 
> This is a little late in my opinion, as we don't have any sanity check
> on the flashes that are stacked on top of the first. We shall at least
> read and compare the ID for all.
> 
> Cheers,
> ta

Tudor Ambarus Dec. 7, 2023, 5:24 p.m. UTC | #3

On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
> The current implementation assumes that a maximum of two flashes are
> connected in stacked mode and both the flashes are of same make but can
> differ in sizes. So, except the sizes all other flash parameters of both
> the flashes are identical.

Too much restrictions, isn't it? Have you thought about adding a layer
on top of SPI NOR managing the stacked/parallel flashes?

Mahapatra, Amit Kumar Dec. 8, 2023, 5:05 p.m. UTC | #4

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Wednesday, December 6, 2023 8:00 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> Hi, Amit,
> 
> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
> > Each flash that is connected in stacked mode should have a separate
> > parameter structure. So, the flash parameter member(*params) of the
> > spi_nor structure is changed to an array (*params[2]). The array is
> > used to store the parameters of each flash connected in stacked
> configuration.
> >
> > The current implementation assumes that a maximum of two flashes are
> > connected in stacked mode and both the flashes are of same make but
> > can differ in sizes. So, except the sizes all other flash parameters
> > of both the flashes are identical.
> 
> Do you plan to add support for different flashes in stacked mode? If not,

No, according to the current implementation, in stacked mode, both flashes 
must be of the same make.

> wouldn't it be simpler to have just an array of flash sizes instead of
> duplicating the entire params struct?

Yes, that is accurate. In alignment with our current stacked support use case we 
can have an array of flash sizes instead.
The primary purpose of having an array of params struct was to facilitate 
potential future extensions, allowing the addition of stacked support for 
different flashes

> 
> >
> > SPI-NOR is not aware of the chip_select values, for any incoming
> > request SPI-NOR will decide the flash index with the help of
> > individual flash size and the configuration type (single/stacked).
> > SPI-NOR will pass on the flash index information to the SPI core & SPI
> > driver by setting the appropriate bit in
> > nor->spimem->spi->cs_index_mask. For example, if nth bit of
> > nor->spimem->spi->cs_index_mask is set then the driver would
> > assert/de-assert spi->chip_slect[n].
> >
> > Signed-off-by: Amit Kumar Mahapatra <[e-mail]>
> > ---
> >  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-------
> >  drivers/mtd/spi-nor/core.h  |   4 +
> >  include/linux/mtd/spi-nor.h |  15 +-
> >  3 files changed, 240 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 93ae69b7ff83..e990be7c7eb6 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> 
> cut
> 
> > @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
> > spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
> > *nor)  {
> >  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
> 0);
> > -	int ret;
> > +	struct device_node *np = spi_nor_get_flash_node(nor);
> > +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> > +	u32 idx = 0;
> > +	int rc, ret;
> >
> >  	if (nor->manufacturer && nor->manufacturer->fixups &&
> >  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@
> > static int spi_nor_late_init_params(struct spi_nor *nor)
> >  	if (params->n_banks > 1)
> >  		params->bank_size = div64_u64(params->size, params-
> >n_banks);
> >
> > +	nor->num_flash = 0;
> > +
> > +	/*
> > +	 * The flashes that are connected in stacked mode should be of same
> make.
> > +	 * Except the flash size all other properties are identical for all the
> > +	 * flashes connected in stacked mode.
> > +	 * The flashes that are connected in parallel mode should be identical.
> > +	 */
> > +	while (idx < SNOR_FLASH_CNT_MAX) {
> > +		rc = of_property_read_u64_index(np, "stacked-memories",
> idx,
> > +&flash_size[idx]);
> 
> This is a little late in my opinion, as we don't have any sanity check on the
> flashes that are stacked on top of the first. We shall at least read and compare
> the ID for all.

Alright, I will incorporate a sanity check for reading and comparing the 
ID of the stacked flash. Subsequently, I believe this stacked logic 
should be relocated to spi_nor_get_flash_info() where we identify the 
first flash. Please share your thoughts on this. Additionally, do you 
anticipate that SPI-NOR should throw an error if the second or any 
subsequent flash within the stacked connection is different? Or would you 
prefer it to print a warning and operate in single mode (i.e., only the 
first flash)?

Regards,
Amit
> 
> Cheers,
> ta

Mahapatra, Amit Kumar Dec. 8, 2023, 5:06 p.m. UTC | #5

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Wednesday, December 6, 2023 8:14 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 12/6/23 14:30, Tudor Ambarus wrote:
> > Hi, Amit,
> >
> > On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
> >> Each flash that is connected in stacked mode should have a separate
> >> parameter structure. So, the flash parameter member(*params) of the
> >> spi_nor structure is changed to an array (*params[2]). The array is
> >> used to store the parameters of each flash connected in stacked
> configuration.
> >>
> >> The current implementation assumes that a maximum of two flashes are
> >> connected in stacked mode and both the flashes are of same make but
> >> can differ in sizes. So, except the sizes all other flash parameters
> >> of both the flashes are identical.
> >
> > Do you plan to add support for different flashes in stacked mode? If
> > not, wouldn't it be simpler to have just an array of flash sizes
> > instead of duplicating the entire params struct?
> >
> >>
> >> SPI-NOR is not aware of the chip_select values, for any incoming
> >> request SPI-NOR will decide the flash index with the help of
> >> individual flash size and the configuration type (single/stacked).
> >> SPI-NOR will pass on the flash index information to the SPI core &
> >> SPI driver by setting the appropriate bit in
> >> nor->spimem->spi->cs_index_mask. For example, if nth bit of
> >> nor->spimem->spi->cs_index_mask is set then the driver would
> >> assert/de-assert spi->chip_slect[n].
> >>
> >> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
> [e-mail]>
> >> ---
> >>  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-------
> >>  drivers/mtd/spi-nor/core.h  |   4 +
> >>  include/linux/mtd/spi-nor.h |  15 +-
> >>  3 files changed, 240 insertions(+), 51 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >> index 93ae69b7ff83..e990be7c7eb6 100644
> >> --- a/drivers/mtd/spi-nor/core.c
> >> +++ b/drivers/mtd/spi-nor/core.c
> >
> > cut
> >
> >> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
> >> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
> >> *nor)  {
> >>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
> 0);
> >> -	int ret;
> >> +	struct device_node *np = spi_nor_get_flash_node(nor);
> >> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> >> +	u32 idx = 0;
> >> +	int rc, ret;
> >>
> >>  	if (nor->manufacturer && nor->manufacturer->fixups &&
> >>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@
> >> static int spi_nor_late_init_params(struct spi_nor *nor)
> >>  	if (params->n_banks > 1)
> >>  		params->bank_size = div64_u64(params->size, params-
> >n_banks);
> >>
> >> +	nor->num_flash = 0;
> >> +
> >> +	/*
> >> +	 * The flashes that are connected in stacked mode should be of same
> make.
> >> +	 * Except the flash size all other properties are identical for all the
> >> +	 * flashes connected in stacked mode.
> >> +	 * The flashes that are connected in parallel mode should be identical.
> >> +	 */
> >> +	while (idx < SNOR_FLASH_CNT_MAX) {
> >> +		rc = of_property_read_u64_index(np, "stacked-memories",
> idx,
> >> +&flash_size[idx]);
> 
> also, it's not clear to me why you read this property multiple times.
> Have you sent a device tree patch somewhere? It will help me understand
> what you're trying to achieve.

Miquel submitted the device tree patch; here is the series.
https://lore.kernel.org/all/[e-mail]/

Regards,
Amit
> 
> >
> > This is a little late in my opinion, as we don't have any sanity check
> > on the flashes that are stacked on top of the first. We shall at least
> > read and compare the ID for all.
> >
> > Cheers,
> > ta

Tudor Ambarus Dec. 11, 2023, 3:33 a.m. UTC | #6

On 12/8/23 17:05, Mahapatra, Amit Kumar wrote:
> Hello Tudor,

Hi!

> 
>> -----Original Message-----
>> From: Tudor Ambarus <[e-mail]>
>> Sent: Wednesday, December 6, 2023 8:00 PM
>> To: Mahapatra, Amit Kumar <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Simek, Michal <[e-mail]>; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; git (AMD-
>> Xilinx) <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
>> in spi-nor
>>
>> Hi, Amit,
>>
>> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
>>> Each flash that is connected in stacked mode should have a separate
>>> parameter structure. So, the flash parameter member(*params) of the
>>> spi_nor structure is changed to an array (*params[2]). The array is
>>> used to store the parameters of each flash connected in stacked
>> configuration.
>>>
>>> The current implementation assumes that a maximum of two flashes are
>>> connected in stacked mode and both the flashes are of same make but
>>> can differ in sizes. So, except the sizes all other flash parameters
>>> of both the flashes are identical.
>>
>> Do you plan to add support for different flashes in stacked mode? If not,
> 
> No, according to the current implementation, in stacked mode, both flashes 
> must be of the same make.
> 
>> wouldn't it be simpler to have just an array of flash sizes instead of
>> duplicating the entire params struct?
> 
> Yes, that is accurate. In alignment with our current stacked support use case we 
> can have an array of flash sizes instead.
> The primary purpose of having an array of params struct was to facilitate 
> potential future extensions, allowing the addition of stacked support for 
> different flashes
> 

right. Don't do this change yet, let's decide on the overall
architecture first.

>>
>>>
>>> SPI-NOR is not aware of the chip_select values, for any incoming
>>> request SPI-NOR will decide the flash index with the help of
>>> individual flash size and the configuration type (single/stacked).
>>> SPI-NOR will pass on the flash index information to the SPI core & SPI
>>> driver by setting the appropriate bit in
>>> nor->spimem->spi->cs_index_mask. For example, if nth bit of
>>> nor->spimem->spi->cs_index_mask is set then the driver would
>>> assert/de-assert spi->chip_slect[n].
>>>
>>> Signed-off-by: Amit Kumar Mahapatra <[e-mail]>
>>> ---
>>>  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-------
>>>  drivers/mtd/spi-nor/core.h  |   4 +
>>>  include/linux/mtd/spi-nor.h |  15 +-
>>>  3 files changed, 240 insertions(+), 51 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 93ae69b7ff83..e990be7c7eb6 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>
>> cut
>>
>>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
>>> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
>>> *nor)  {
>>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
>> 0);
>>> -	int ret;
>>> +	struct device_node *np = spi_nor_get_flash_node(nor);
>>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
>>> +	u32 idx = 0;
>>> +	int rc, ret;
>>>
>>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
>>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@
>>> static int spi_nor_late_init_params(struct spi_nor *nor)
>>>  	if (params->n_banks > 1)
>>>  		params->bank_size = div64_u64(params->size, params-
>>> n_banks);
>>>
>>> +	nor->num_flash = 0;
>>> +
>>> +	/*
>>> +	 * The flashes that are connected in stacked mode should be of same
>> make.
>>> +	 * Except the flash size all other properties are identical for all the
>>> +	 * flashes connected in stacked mode.
>>> +	 * The flashes that are connected in parallel mode should be identical.
>>> +	 */
>>> +	while (idx < SNOR_FLASH_CNT_MAX) {
>>> +		rc = of_property_read_u64_index(np, "stacked-memories",
>> idx,
>>> +&flash_size[idx]);
>>
>> This is a little late in my opinion, as we don't have any sanity check on the
>> flashes that are stacked on top of the first. We shall at least read and compare
>> the ID for all.
> 
> Alright, I will incorporate a sanity check for reading and comparing the 
> ID of the stacked flash. Subsequently, I believe this stacked logic 
> should be relocated to spi_nor_get_flash_info() where we identify the 
> first flash. Please share your thoughts on this. Additionally, do you 

I'm wondering whether we can add a layer on top of the flash type to
handle the stacked/parallel modes. This way everything will become flash
type independent. Would it be possible to stack 2 SPI NANDs? How about a
SPI NOR and a SPI NAND?

Is the datasheet of the controller public?

> anticipate that SPI-NOR should throw an error if the second or any 
> subsequent flash within the stacked connection is different? Or would you 
> prefer it to print a warning and operate in single mode (i.e., only the 
> first flash)?

Both options are fine, but I haven't yet decided on the overall
architecture.

Cheers,
ta

Tudor Ambarus Dec. 11, 2023, 3:44 a.m. UTC | #7

On 12/8/23 17:06, Mahapatra, Amit Kumar wrote:
> Hello Tudor,

Hi!

> 
>> -----Original Message-----
>> From: Tudor Ambarus <[e-mail]>
>> Sent: Wednesday, December 6, 2023 8:14 PM
>> To: Mahapatra, Amit Kumar <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Simek, Michal <[e-mail]>; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; git (AMD-
>> Xilinx) <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
>> in spi-nor
>>
>>
>>
>> On 12/6/23 14:30, Tudor Ambarus wrote:
>>> Hi, Amit,
>>>
>>> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
>>>> Each flash that is connected in stacked mode should have a separate
>>>> parameter structure. So, the flash parameter member(*params) of the
>>>> spi_nor structure is changed to an array (*params[2]). The array is
>>>> used to store the parameters of each flash connected in stacked
>> configuration.
>>>>
>>>> The current implementation assumes that a maximum of two flashes are
>>>> connected in stacked mode and both the flashes are of same make but
>>>> can differ in sizes. So, except the sizes all other flash parameters
>>>> of both the flashes are identical.
>>>
>>> Do you plan to add support for different flashes in stacked mode? If
>>> not, wouldn't it be simpler to have just an array of flash sizes
>>> instead of duplicating the entire params struct?
>>>
>>>>
>>>> SPI-NOR is not aware of the chip_select values, for any incoming
>>>> request SPI-NOR will decide the flash index with the help of
>>>> individual flash size and the configuration type (single/stacked).
>>>> SPI-NOR will pass on the flash index information to the SPI core &
>>>> SPI driver by setting the appropriate bit in
>>>> nor->spimem->spi->cs_index_mask. For example, if nth bit of
>>>> nor->spimem->spi->cs_index_mask is set then the driver would
>>>> assert/de-assert spi->chip_slect[n].
>>>>
>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
>> [e-mail]>
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-------
>>>>  drivers/mtd/spi-nor/core.h  |   4 +
>>>>  include/linux/mtd/spi-nor.h |  15 +-
>>>>  3 files changed, 240 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index 93ae69b7ff83..e990be7c7eb6 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>
>>> cut
>>>
>>>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
>>>> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
>>>> *nor)  {
>>>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
>> 0);
>>>> -	int ret;
>>>> +	struct device_node *np = spi_nor_get_flash_node(nor);
>>>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
>>>> +	u32 idx = 0;
>>>> +	int rc, ret;
>>>>
>>>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@
>>>> static int spi_nor_late_init_params(struct spi_nor *nor)
>>>>  	if (params->n_banks > 1)
>>>>  		params->bank_size = div64_u64(params->size, params-
>>> n_banks);
>>>>
>>>> +	nor->num_flash = 0;
>>>> +
>>>> +	/*
>>>> +	 * The flashes that are connected in stacked mode should be of same
>> make.
>>>> +	 * Except the flash size all other properties are identical for all the
>>>> +	 * flashes connected in stacked mode.
>>>> +	 * The flashes that are connected in parallel mode should be identical.
>>>> +	 */
>>>> +	while (idx < SNOR_FLASH_CNT_MAX) {
>>>> +		rc = of_property_read_u64_index(np, "stacked-memories",
>> idx,
>>>> +&flash_size[idx]);
>>
>> also, it's not clear to me why you read this property multiple times.
>> Have you sent a device tree patch somewhere? It will help me understand
>> what you're trying to achieve.
> 
> Miquel submitted the device tree patch; here is the series.
> https://lore.kernel.org/all/[e-mail]/
> 

oh, yes, I remember seeing this on the ml, but I couldn't allocate time
to review it. Looking at:
https://lore.kernel.org/all/[e-mail]/

Flash size is not necessary for SPI NORs, as it can be discovered via
SFDP. And spi-max-frequency should have been specified for all flashes,
as I expect it can differ. At least so that the controller chooses the
minimum frequency from all the max (if it can't operate the stacks at
different frequencies).

Cheers,
ta

Mahapatra, Amit Kumar Dec. 11, 2023, 6:56 a.m. UTC | #8

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Monday, December 11, 2023 9:03 AM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 12/8/23 17:05, Mahapatra, Amit Kumar wrote:
> > Hello Tudor,
> 
> Hi!

Hello Tudor,

> 
> >
> >> -----Original Message-----
> >> From: Tudor Ambarus <[e-mail]>
> >> Sent: Wednesday, December 6, 2023 8:00 PM
> >> To: Mahapatra, Amit Kumar <[e-mail]>;
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; Simek, Michal <[e-mail]>;
> >> linux- [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; git (AMD-
> >> Xilinx) <[e-mail]>; [e-mail]
> >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
> >> support in spi-nor
> >>
> >> Hi, Amit,
> >>
> >> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
> >>> Each flash that is connected in stacked mode should have a separate
> >>> parameter structure. So, the flash parameter member(*params) of the
> >>> spi_nor structure is changed to an array (*params[2]). The array is
> >>> used to store the parameters of each flash connected in stacked
> >> configuration.
> >>>
> >>> The current implementation assumes that a maximum of two flashes are
> >>> connected in stacked mode and both the flashes are of same make but
> >>> can differ in sizes. So, except the sizes all other flash parameters
> >>> of both the flashes are identical.
> >>
> >> Do you plan to add support for different flashes in stacked mode? If
> >> not,
> >
> > No, according to the current implementation, in stacked mode, both
> > flashes must be of the same make.
> >
> >> wouldn't it be simpler to have just an array of flash sizes instead
> >> of duplicating the entire params struct?
> >
> > Yes, that is accurate. In alignment with our current stacked support
> > use case we can have an array of flash sizes instead.
> > The primary purpose of having an array of params struct was to
> > facilitate potential future extensions, allowing the addition of
> > stacked support for different flashes
> >
> 
> right. Don't do this change yet, let's decide on the overall architecture first.

Sure.
> 
> >>
> >>>
> >>> SPI-NOR is not aware of the chip_select values, for any incoming
> >>> request SPI-NOR will decide the flash index with the help of
> >>> individual flash size and the configuration type (single/stacked).
> >>> SPI-NOR will pass on the flash index information to the SPI core &
> >>> SPI driver by setting the appropriate bit in
> >>> nor->spimem->spi->cs_index_mask. For example, if nth bit of
> >>> nor->spimem->spi->cs_index_mask is set then the driver would
> >>> assert/de-assert spi->chip_slect[n].
> >>>
> >>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
> [e-mail]>
> >>> ---
> >>>  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-----
> --
> >>>  drivers/mtd/spi-nor/core.h  |   4 +
> >>>  include/linux/mtd/spi-nor.h |  15 +-
> >>>  3 files changed, 240 insertions(+), 51 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >>> index 93ae69b7ff83..e990be7c7eb6 100644
> >>> --- a/drivers/mtd/spi-nor/core.c
> >>> +++ b/drivers/mtd/spi-nor/core.c
> >>
> >> cut
> >>
> >>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
> >>> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
> >>> *nor)  {
> >>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
> >> 0);
> >>> -	int ret;
> >>> +	struct device_node *np = spi_nor_get_flash_node(nor);
> >>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> >>> +	u32 idx = 0;
> >>> +	int rc, ret;
> >>>
> >>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
> >>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@
> >>> static int spi_nor_late_init_params(struct spi_nor *nor)
> >>>  	if (params->n_banks > 1)
> >>>  		params->bank_size = div64_u64(params->size, params-
> n_banks);
> >>>
> >>> +	nor->num_flash = 0;
> >>> +
> >>> +	/*
> >>> +	 * The flashes that are connected in stacked mode should be of
> >>> +same
> >> make.
> >>> +	 * Except the flash size all other properties are identical for all the
> >>> +	 * flashes connected in stacked mode.
> >>> +	 * The flashes that are connected in parallel mode should be identical.
> >>> +	 */
> >>> +	while (idx < SNOR_FLASH_CNT_MAX) {
> >>> +		rc = of_property_read_u64_index(np, "stacked-memories",
> >> idx,
> >>> +&flash_size[idx]);
> >>
> >> This is a little late in my opinion, as we don't have any sanity
> >> check on the flashes that are stacked on top of the first. We shall
> >> at least read and compare the ID for all.
> >
> > Alright, I will incorporate a sanity check for reading and comparing
> > the ID of the stacked flash. Subsequently, I believe this stacked
> > logic should be relocated to spi_nor_get_flash_info() where we
> > identify the first flash. Please share your thoughts on this.
> > Additionally, do you
> 
> I'm wondering whether we can add a layer on top of the flash type to handle

When you mention "on top," are you referring to incorporating it into 
the MTD layer? Initially, Miquel had submitted this patch to address 
stacked/parallel handling in the MTD layer.
https://lore.kernel.org/linux-mtd/20200114191052.0a16d116@xps13/t/
However, the Device Tree bindings were initially not accepted. 
Following a series of discussions, the below bindings were 
eventually merged.
https://lore.kernel.org/all/[e-mail]/

> the stacked/parallel modes. This way everything will become flash type
> independent. Would it be possible to stack 2 SPI NANDs? How about a SPI
> NOR and a SPI NAND?
> 
> Is the datasheet of the controller public?

Yes, https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Quad-SPI-Controller

> 
> > anticipate that SPI-NOR should throw an error if the second or any
> > subsequent flash within the stacked connection is different? Or would
> > you prefer it to print a warning and operate in single mode (i.e.,
> > only the first flash)?
> 
> Both options are fine, but I haven't yet decided on the overall architecture.

Ok.

Regards,
Amit
> 
> Cheers,
> ta

Tudor Ambarus Dec. 11, 2023, 9:35 a.m. UTC | #9

On 12/11/23 06:56, Mahapatra, Amit Kumar wrote:
> 
> 
>> -----Original Message-----
>> From: Tudor Ambarus <[e-mail]>
>> Sent: Monday, December 11, 2023 9:03 AM
>> To: Mahapatra, Amit Kumar <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Simek, Michal <[e-mail]>; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; git (AMD-
>> Xilinx) <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
>> in spi-nor
>>
>>
>>
>> On 12/8/23 17:05, Mahapatra, Amit Kumar wrote:
>>> Hello Tudor,
>>
>> Hi!
> 
> Hello Tudor,
> 

Hi!

>>
>>>
>>>> -----Original Message-----
>>>> From: Tudor Ambarus <[e-mail]>
>>>> Sent: Wednesday, December 6, 2023 8:00 PM
>>>> To: Mahapatra, Amit Kumar <[e-mail]>;
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail]
>>>> Cc: [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Simek, Michal <[e-mail]>;
>>>> linux- [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; git (AMD-
>>>> Xilinx) <[e-mail]>; [e-mail]
>>>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
>>>> support in spi-nor
>>>>
>>>> Hi, Amit,
>>>>
>>>> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
>>>>> Each flash that is connected in stacked mode should have a separate
>>>>> parameter structure. So, the flash parameter member(*params) of the
>>>>> spi_nor structure is changed to an array (*params[2]). The array is
>>>>> used to store the parameters of each flash connected in stacked
>>>> configuration.
>>>>>
>>>>> The current implementation assumes that a maximum of two flashes are
>>>>> connected in stacked mode and both the flashes are of same make but
>>>>> can differ in sizes. So, except the sizes all other flash parameters
>>>>> of both the flashes are identical.
>>>>
>>>> Do you plan to add support for different flashes in stacked mode? If
>>>> not,
>>>
>>> No, according to the current implementation, in stacked mode, both
>>> flashes must be of the same make.
>>>
>>>> wouldn't it be simpler to have just an array of flash sizes instead
>>>> of duplicating the entire params struct?
>>>
>>> Yes, that is accurate. In alignment with our current stacked support
>>> use case we can have an array of flash sizes instead.
>>> The primary purpose of having an array of params struct was to
>>> facilitate potential future extensions, allowing the addition of
>>> stacked support for different flashes
>>>
>>
>> right. Don't do this change yet, let's decide on the overall architecture first.
> 
> Sure.
>>
>>>>
>>>>>
>>>>> SPI-NOR is not aware of the chip_select values, for any incoming
>>>>> request SPI-NOR will decide the flash index with the help of
>>>>> individual flash size and the configuration type (single/stacked).
>>>>> SPI-NOR will pass on the flash index information to the SPI core &
>>>>> SPI driver by setting the appropriate bit in
>>>>> nor->spimem->spi->cs_index_mask. For example, if nth bit of
>>>>> nor->spimem->spi->cs_index_mask is set then the driver would
>>>>> assert/de-assert spi->chip_slect[n].
>>>>>
>>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
>> [e-mail]>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/core.c  | 272 +++++++++++++++++++++++++++++-----
>> --
>>>>>  drivers/mtd/spi-nor/core.h  |   4 +
>>>>>  include/linux/mtd/spi-nor.h |  15 +-
>>>>>  3 files changed, 240 insertions(+), 51 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>>> index 93ae69b7ff83..e990be7c7eb6 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>
>>>> cut
>>>>
>>>>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
>>>>> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
>>>>> *nor)  {
>>>>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
>>>> 0);
>>>>> -	int ret;
>>>>> +	struct device_node *np = spi_nor_get_flash_node(nor);
>>>>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
>>>>> +	u32 idx = 0;
>>>>> +	int rc, ret;
>>>>>
>>>>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44 @@
>>>>> static int spi_nor_late_init_params(struct spi_nor *nor)
>>>>>  	if (params->n_banks > 1)
>>>>>  		params->bank_size = div64_u64(params->size, params-
>> n_banks);
>>>>>
>>>>> +	nor->num_flash = 0;
>>>>> +
>>>>> +	/*
>>>>> +	 * The flashes that are connected in stacked mode should be of
>>>>> +same
>>>> make.
>>>>> +	 * Except the flash size all other properties are identical for all the
>>>>> +	 * flashes connected in stacked mode.
>>>>> +	 * The flashes that are connected in parallel mode should be identical.
>>>>> +	 */
>>>>> +	while (idx < SNOR_FLASH_CNT_MAX) {
>>>>> +		rc = of_property_read_u64_index(np, "stacked-memories",
>>>> idx,
>>>>> +&flash_size[idx]);
>>>>
>>>> This is a little late in my opinion, as we don't have any sanity
>>>> check on the flashes that are stacked on top of the first. We shall
>>>> at least read and compare the ID for all.
>>>
>>> Alright, I will incorporate a sanity check for reading and comparing
>>> the ID of the stacked flash. Subsequently, I believe this stacked
>>> logic should be relocated to spi_nor_get_flash_info() where we
>>> identify the first flash. Please share your thoughts on this.
>>> Additionally, do you
>>
>> I'm wondering whether we can add a layer on top of the flash type to handle
> 
> When you mention "on top," are you referring to incorporating it into 
> the MTD layer? Initially, Miquel had submitted this patch to address 

I mean something above SPI MEM flashes, be it NOR, NANDs or whatever.
Instead of treating the stacked flashes as a monolithic device and treat
in SPI NOR some array of flashes, to have a layer above which probes the
SPI MEM flash driver for each stacked flash. In your case SPI NOR would
be probed twice, as you use 2 SPI NOR flashes.

> stacked/parallel handling in the MTD layer.
> https://lore.kernel.org/linux-mtd/20200114191052.0a16d116@xps13/t/
> However, the Device Tree bindings were initially not accepted. 

Okay, thanks for the pointer. I'll take a look.

> Following a series of discussions, the below bindings were 
> eventually merged.
> https://lore.kernel.org/all/[e-mail]/

I saw, thanks.

> 
>> the stacked/parallel modes. This way everything will become flash type
>> independent. Would it be possible to stack 2 SPI NANDs? How about a SPI
>> NOR and a SPI NAND?
>>
>> Is the datasheet of the controller public?
> 
> Yes, https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Quad-SPI-Controller
> 

Wonderful, I'll take a look. I see two clocks there. Does this mean that
the stacked flashes can be operated at different frequencies? Do you
know if we can combine a SPI NOR with a SPI NAND in stacked configuration?

I need to study this a bit. I'll try to involve Michael and Pratyush too.

Cheers,
ta

Mahapatra, Amit Kumar Dec. 11, 2023, 1:37 p.m. UTC | #10

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Monday, December 11, 2023 3:05 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 12/11/23 06:56, Mahapatra, Amit Kumar wrote:
> >
> >
> >> -----Original Message-----
> >> From: Tudor Ambarus <[e-mail]>
> >> Sent: Monday, December 11, 2023 9:03 AM
> >> To: Mahapatra, Amit Kumar <[e-mail]>;
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; Simek, Michal <[e-mail]>;
> >> linux- [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; git (AMD-
> >> Xilinx) <[e-mail]>; [e-mail]
> >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
> >> support in spi-nor
> >>
> >>
> >>
> >> On 12/8/23 17:05, Mahapatra, Amit Kumar wrote:
> >>> Hello Tudor,
> >>
> >> Hi!
> >
> > Hello Tudor,
> >
> 
> Hi!

Hello Tudor,
> 
> >>
> >>>
> >>>> -----Original Message-----
> >>>> From: Tudor Ambarus <[e-mail]>
> >>>> Sent: Wednesday, December 6, 2023 8:00 PM
> >>>> To: Mahapatra, Amit Kumar <[e-mail]>;
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail]
> >>>> Cc: [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; Simek, Michal <[e-mail]>;
> >>>> linux- [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; git
> >>>> (AMD-
> >>>> Xilinx) <[e-mail]>; [e-mail]
> >>>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
> >>>> support in spi-nor
> >>>>
> >>>> Hi, Amit,
> >>>>
> >>>> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
> >>>>> Each flash that is connected in stacked mode should have a
> >>>>> separate parameter structure. So, the flash parameter
> >>>>> member(*params) of the spi_nor structure is changed to an array
> >>>>> (*params[2]). The array is used to store the parameters of each
> >>>>> flash connected in stacked
> >>>> configuration.
> >>>>>
> >>>>> The current implementation assumes that a maximum of two flashes
> >>>>> are connected in stacked mode and both the flashes are of same
> >>>>> make but can differ in sizes. So, except the sizes all other flash
> >>>>> parameters of both the flashes are identical.
> >>>>
> >>>> Do you plan to add support for different flashes in stacked mode?
> >>>> If not,
> >>>
> >>> No, according to the current implementation, in stacked mode, both
> >>> flashes must be of the same make.
> >>>
> >>>> wouldn't it be simpler to have just an array of flash sizes instead
> >>>> of duplicating the entire params struct?
> >>>
> >>> Yes, that is accurate. In alignment with our current stacked support
> >>> use case we can have an array of flash sizes instead.
> >>> The primary purpose of having an array of params struct was to
> >>> facilitate potential future extensions, allowing the addition of
> >>> stacked support for different flashes
> >>>
> >>
> >> right. Don't do this change yet, let's decide on the overall architecture first.
> >
> > Sure.
> >>
> >>>>
> >>>>>
> >>>>> SPI-NOR is not aware of the chip_select values, for any incoming
> >>>>> request SPI-NOR will decide the flash index with the help of
> >>>>> individual flash size and the configuration type (single/stacked).
> >>>>> SPI-NOR will pass on the flash index information to the SPI core &
> >>>>> SPI driver by setting the appropriate bit in
> >>>>> nor->spimem->spi->cs_index_mask. For example, if nth bit of
> >>>>> nor->spimem->spi->cs_index_mask is set then the driver would
> >>>>> assert/de-assert spi->chip_slect[n].
> >>>>>
> >>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
> >> [e-mail]>
> >>>>> ---
> >>>>>  drivers/mtd/spi-nor/core.c  | 272
> >>>>> +++++++++++++++++++++++++++++-----
> >> --
> >>>>>  drivers/mtd/spi-nor/core.h  |   4 +
> >>>>>  include/linux/mtd/spi-nor.h |  15 +-
> >>>>>  3 files changed, 240 insertions(+), 51 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/mtd/spi-nor/core.c
> >>>>> b/drivers/mtd/spi-nor/core.c index 93ae69b7ff83..e990be7c7eb6
> >>>>> 100644
> >>>>> --- a/drivers/mtd/spi-nor/core.c
> >>>>> +++ b/drivers/mtd/spi-nor/core.c
> >>>>
> >>>> cut
> >>>>
> >>>>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
> >>>>> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
> >>>>> *nor)  {
> >>>>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
> >>>> 0);
> >>>>> -	int ret;
> >>>>> +	struct device_node *np = spi_nor_get_flash_node(nor);
> >>>>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> >>>>> +	u32 idx = 0;
> >>>>> +	int rc, ret;
> >>>>>
> >>>>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
> >>>>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44
> >>>>> @@ static int spi_nor_late_init_params(struct spi_nor *nor)
> >>>>>  	if (params->n_banks > 1)
> >>>>>  		params->bank_size = div64_u64(params->size, params-
> >> n_banks);
> >>>>>
> >>>>> +	nor->num_flash = 0;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * The flashes that are connected in stacked mode should be
> of
> >>>>> +same
> >>>> make.
> >>>>> +	 * Except the flash size all other properties are identical for all
> the
> >>>>> +	 * flashes connected in stacked mode.
> >>>>> +	 * The flashes that are connected in parallel mode should be
> identical.
> >>>>> +	 */
> >>>>> +	while (idx < SNOR_FLASH_CNT_MAX) {
> >>>>> +		rc = of_property_read_u64_index(np, "stacked-
> memories",
> >>>> idx,
> >>>>> +&flash_size[idx]);
> >>>>
> >>>> This is a little late in my opinion, as we don't have any sanity
> >>>> check on the flashes that are stacked on top of the first. We shall
> >>>> at least read and compare the ID for all.
> >>>
> >>> Alright, I will incorporate a sanity check for reading and comparing
> >>> the ID of the stacked flash. Subsequently, I believe this stacked
> >>> logic should be relocated to spi_nor_get_flash_info() where we
> >>> identify the first flash. Please share your thoughts on this.
> >>> Additionally, do you
> >>
> >> I'm wondering whether we can add a layer on top of the flash type to
> >> handle
> >
> > When you mention "on top," are you referring to incorporating it into
> > the MTD layer? Initially, Miquel had submitted this patch to address
> 
> I mean something above SPI MEM flashes, be it NOR, NANDs or whatever.
> Instead of treating the stacked flashes as a monolithic device and treat in SPI
> NOR some array of flashes, to have a layer above which probes the SPI MEM
> flash driver for each stacked flash. In your case SPI NOR would be probed
> twice, as you use 2 SPI NOR flashes.
> 
> > stacked/parallel handling in the MTD layer.
> > https://lore.kernel.org/linux-mtd/20200114191052.0a16d116@xps13/t/
> > However, the Device Tree bindings were initially not accepted.
> 
> Okay, thanks for the pointer. I'll take a look.
> 
> > Following a series of discussions, the below bindings were eventually
> > merged.
> > https://lore.kernel.org/all/20220126112608.955728-4-miquel.raynal@boot
> > lin.com/
> 
> I saw, thanks.
> 
> >
> >> the stacked/parallel modes. This way everything will become flash
> >> type independent. Would it be possible to stack 2 SPI NANDs? How
> >> about a SPI NOR and a SPI NAND?
> >>
> >> Is the datasheet of the controller public?
> >
> > Yes,
> > https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Quad-SPI-Control
> > ler
> >
> 
> Wonderful, I'll take a look. I see two clocks there. Does this mean that the
> stacked flashes can be operated at different frequencies? Do you know if we

In stacked configuration, both flashes utilize a common clock (single 
clock line). In a parallel setup, the flashes share the same clock but 
have distinct clock lines. Please refer the diagrams in the sections 
below for reference.
https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-Diagrams
> can combine a SPI NOR with a SPI NAND in stacked configuration?

No, Xilinx/AMD QSPI controllers doesn't support this configuration.

Regards,
Amit
> 
> I need to study this a bit. I'll try to involve Michael and Pratyush too.
> 
> Cheers,
> ta

Tudor Ambarus Dec. 12, 2023, 3:02 p.m. UTC | #11

On 12/11/23 13:37, Mahapatra, Amit Kumar wrote:

Hi!

cut

>>>>>>>  drivers/mtd/spi-nor/core.c  | 272
>>>>>>> +++++++++++++++++++++++++++++-----
>>>> --
>>>>>>>  drivers/mtd/spi-nor/core.h  |   4 +
>>>>>>>  include/linux/mtd/spi-nor.h |  15 +-
>>>>>>>  3 files changed, 240 insertions(+), 51 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/core.c
>>>>>>> b/drivers/mtd/spi-nor/core.c index 93ae69b7ff83..e990be7c7eb6
>>>>>>> 100644
>>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>>
>>>>>> cut
>>>>>>
>>>>>>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
>>>>>>> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
>>>>>>> *nor)  {
>>>>>>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
>>>>>> 0);
>>>>>>> -	int ret;
>>>>>>> +	struct device_node *np = spi_nor_get_flash_node(nor);
>>>>>>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
>>>>>>> +	u32 idx = 0;
>>>>>>> +	int rc, ret;
>>>>>>>
>>>>>>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
>>>>>>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44
>>>>>>> @@ static int spi_nor_late_init_params(struct spi_nor *nor)
>>>>>>>  	if (params->n_banks > 1)
>>>>>>>  		params->bank_size = div64_u64(params->size, params-
>>>> n_banks);
>>>>>>>
>>>>>>> +	nor->num_flash = 0;
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * The flashes that are connected in stacked mode should be
>> of
>>>>>>> +same
>>>>>> make.
>>>>>>> +	 * Except the flash size all other properties are identical for all
>> the
>>>>>>> +	 * flashes connected in stacked mode.
>>>>>>> +	 * The flashes that are connected in parallel mode should be
>> identical.
>>>>>>> +	 */
>>>>>>> +	while (idx < SNOR_FLASH_CNT_MAX) {
>>>>>>> +		rc = of_property_read_u64_index(np, "stacked-
>> memories",
>>>>>> idx,
>>>>>>> +&flash_size[idx]);
>>>>>>
>>>>>> This is a little late in my opinion, as we don't have any sanity
>>>>>> check on the flashes that are stacked on top of the first. We shall
>>>>>> at least read and compare the ID for all.
>>>>>
>>>>> Alright, I will incorporate a sanity check for reading and comparing
>>>>> the ID of the stacked flash. Subsequently, I believe this stacked
>>>>> logic should be relocated to spi_nor_get_flash_info() where we
>>>>> identify the first flash. Please share your thoughts on this.
>>>>> Additionally, do you
>>>>
>>>> I'm wondering whether we can add a layer on top of the flash type to
>>>> handle
>>>
>>> When you mention "on top," are you referring to incorporating it into
>>> the MTD layer? Initially, Miquel had submitted this patch to address
>>
>> I mean something above SPI MEM flashes, be it NOR, NANDs or whatever.
>> Instead of treating the stacked flashes as a monolithic device and treat in SPI
>> NOR some array of flashes, to have a layer above which probes the SPI MEM
>> flash driver for each stacked flash. In your case SPI NOR would be probed
>> twice, as you use 2 SPI NOR flashes.
>>
>>> stacked/parallel handling in the MTD layer.
>>> https://lore.kernel.org/linux-mtd/20200114191052.0a16d116@xps13/t/
>>> However, the Device Tree bindings were initially not accepted.
>>
>> Okay, thanks for the pointer. I'll take a look.

haven't yet read the thread from above, but I skimmed over the AMD
controller datasheet.

>>
>>> Following a series of discussions, the below bindings were eventually
>>> merged.
>>> https://lore.kernel.org/all/20220126112608.955728-4-miquel.raynal@boot
>>> lin.com/
>>
>> I saw, thanks.
>>
>>>
>>>> the stacked/parallel modes. This way everything will become flash
>>>> type independent. Would it be possible to stack 2 SPI NANDs? How
>>>> about a SPI NOR and a SPI NAND?
>>>>
>>>> Is the datasheet of the controller public?
>>>
>>> Yes,
>>> https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Quad-SPI-Control
>>> ler
>>>
>>
>> Wonderful, I'll take a look. I see two clocks there. Does this mean that the
>> stacked flashes can be operated at different frequencies? Do you know if we
> 
> In stacked configuration, both flashes utilize a common clock (single 
> clock line). In a parallel setup, the flashes share the same clock but 
> have distinct clock lines. Please refer the diagrams in the sections 
> below for reference.
> https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-Diagrams

Thanks! Can you share with us what flashes you used for testing in the
stacked and parallel configurations?

>> can combine a SPI NOR with a SPI NAND in stacked configuration?
> 
> No, Xilinx/AMD QSPI controllers doesn't support this configuration.
> 

2 SPI NANDs shall work with the AMD controller, right?

I skimmed over the QSPI controller datasheet and wondered why one would
get complicated with 2 Quad Flashes when we have Octal. But then I saw
that the same SoC can configure an Octal controller (the Octal and Quad
controllers seems mutual exclusive) and that the Octal one can operate 2
octal flashes in stacked mode :).

Cheers,
ta

Mahapatra, Amit Kumar Dec. 15, 2023, 7:55 a.m. UTC | #12

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Tuesday, December 12, 2023 8:33 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 12/11/23 13:37, Mahapatra, Amit Kumar wrote:
> 
> Hi!
> 
> cut
> 
> >>>>>>>  drivers/mtd/spi-nor/core.c  | 272
> >>>>>>> +++++++++++++++++++++++++++++-----
> >>>> --
> >>>>>>>  drivers/mtd/spi-nor/core.h  |   4 +
> >>>>>>>  include/linux/mtd/spi-nor.h |  15 +-
> >>>>>>>  3 files changed, 240 insertions(+), 51 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/mtd/spi-nor/core.c
> >>>>>>> b/drivers/mtd/spi-nor/core.c index 93ae69b7ff83..e990be7c7eb6
> >>>>>>> 100644
> >>>>>>> --- a/drivers/mtd/spi-nor/core.c
> >>>>>>> +++ b/drivers/mtd/spi-nor/core.c
> >>>>>>
> >>>>>> cut
> >>>>>>
> >>>>>>> @@ -2905,7 +3007,10 @@ static void
> >>>>>>> spi_nor_init_fixup_flags(struct spi_nor *nor)  static int
> >>>>>>> spi_nor_late_init_params(struct spi_nor
> >>>>>>> *nor)  {
> >>>>>>>  	struct spi_nor_flash_parameter *params =
> >>>>>>> spi_nor_get_params(nor,
> >>>>>> 0);
> >>>>>>> -	int ret;
> >>>>>>> +	struct device_node *np = spi_nor_get_flash_node(nor);
> >>>>>>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> >>>>>>> +	u32 idx = 0;
> >>>>>>> +	int rc, ret;
> >>>>>>>
> >>>>>>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
> >>>>>>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6
> >>>>>>> +3042,44 @@ static int spi_nor_late_init_params(struct spi_nor
> *nor)
> >>>>>>>  	if (params->n_banks > 1)
> >>>>>>>  		params->bank_size = div64_u64(params->size,
> params-
> >>>> n_banks);
> >>>>>>>
> >>>>>>> +	nor->num_flash = 0;
> >>>>>>> +
> >>>>>>> +	/*
> >>>>>>> +	 * The flashes that are connected in stacked mode should be
> >> of
> >>>>>>> +same
> >>>>>> make.
> >>>>>>> +	 * Except the flash size all other properties are identical
> >>>>>>> +for all
> >> the
> >>>>>>> +	 * flashes connected in stacked mode.
> >>>>>>> +	 * The flashes that are connected in parallel mode should be
> >> identical.
> >>>>>>> +	 */
> >>>>>>> +	while (idx < SNOR_FLASH_CNT_MAX) {
> >>>>>>> +		rc = of_property_read_u64_index(np, "stacked-
> >> memories",
> >>>>>> idx,
> >>>>>>> +&flash_size[idx]);
> >>>>>>
> >>>>>> This is a little late in my opinion, as we don't have any sanity
> >>>>>> check on the flashes that are stacked on top of the first. We
> >>>>>> shall at least read and compare the ID for all.
> >>>>>
> >>>>> Alright, I will incorporate a sanity check for reading and
> >>>>> comparing the ID of the stacked flash. Subsequently, I believe
> >>>>> this stacked logic should be relocated to spi_nor_get_flash_info()
> >>>>> where we identify the first flash. Please share your thoughts on this.
> >>>>> Additionally, do you
> >>>>
> >>>> I'm wondering whether we can add a layer on top of the flash type
> >>>> to handle
> >>>
> >>> When you mention "on top," are you referring to incorporating it
> >>> into the MTD layer? Initially, Miquel had submitted this patch to
> >>> address
> >>
> >> I mean something above SPI MEM flashes, be it NOR, NANDs or whatever.
> >> Instead of treating the stacked flashes as a monolithic device and
> >> treat in SPI NOR some array of flashes, to have a layer above which
> >> probes the SPI MEM flash driver for each stacked flash. In your case
> >> SPI NOR would be probed twice, as you use 2 SPI NOR flashes.
> >>
> >>> stacked/parallel handling in the MTD layer.
> >>> https://lore.kernel.org/linux-mtd/20200114191052.0a16d116@xps13/t/
> >>> However, the Device Tree bindings were initially not accepted.
> >>
> >> Okay, thanks for the pointer. I'll take a look.
> 
> haven't yet read the thread from above, but I skimmed over the AMD
> controller datasheet.
> 
> >>
> >>> Following a series of discussions, the below bindings were
> >>> eventually merged.
> >>> https://lore.kernel.org/all/20220126112608.955728-4-miquel.raynal@bo
> >>> ot
> >>> lin.com/
> >>
> >> I saw, thanks.
> >>
> >>>
> >>>> the stacked/parallel modes. This way everything will become flash
> >>>> type independent. Would it be possible to stack 2 SPI NANDs? How
> >>>> about a SPI NOR and a SPI NAND?
> >>>>
> >>>> Is the datasheet of the controller public?
> >>>
> >>> Yes,
> >>> https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Quad-SPI-Contr
> >>> ol
> >>> ler
> >>>
> >>
> >> Wonderful, I'll take a look. I see two clocks there. Does this mean
> >> that the stacked flashes can be operated at different frequencies? Do
> >> you know if we
> >
> > In stacked configuration, both flashes utilize a common clock (single
> > clock line). In a parallel setup, the flashes share the same clock but
> > have distinct clock lines. Please refer the diagrams in the sections
> > below for reference.
> > https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Inter
> > face-Diagrams
> 
> Thanks! Can you share with us what flashes you used for testing in the
> stacked and parallel configurations?

I used SPI-NOR QSPI flashes for testing stacked and parallel.

> 
> >> can combine a SPI NOR with a SPI NAND in stacked configuration?
> >
> > No, Xilinx/AMD QSPI controllers doesn't support this configuration.
> >
> 
> 2 SPI NANDs shall work with the AMD controller, right?

We never tested 2 SPI-NAND with AMD controller.
> 
> I skimmed over the QSPI controller datasheet and wondered why one would
> get complicated with 2 Quad Flashes when we have Octal. But then I saw that
> the same SoC can configure an Octal controller (the Octal and Quad
> controllers seems mutual exclusive) and that the Octal one can operate 2
> octal flashes in stacked mode :).

Thats correct .

Please let me know how you want me to proceed on this.

Regards,
Amit
> 
> Cheers,
> ta

Tudor Ambarus Dec. 15, 2023, 8:09 a.m. UTC | #13

On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote:
>> Thanks! Can you share with us what flashes you used for testing in the
>> stacked and parallel configurations?
> I used SPI-NOR QSPI flashes for testing stacked and parallel.

I got that, I wanted the flash name or device ID.

What I'm interested is if each flash is in its own package. Are they?

> 
>>>> can combine a SPI NOR with a SPI NAND in stacked configuration?
>>> No, Xilinx/AMD QSPI controllers doesn't support this configuration.
>>>
>> 2 SPI NANDs shall work with the AMD controller, right?
> We never tested 2 SPI-NAND with AMD controller.

I was asking because I think the stacked layer shall be SPI MEM generic,
and not particular to SPI NOR.

>> I skimmed over the QSPI controller datasheet and wondered why one would
>> get complicated with 2 Quad Flashes when we have Octal. But then I saw that
>> the same SoC can configure an Octal controller (the Octal and Quad
>> controllers seems mutual exclusive) and that the Octal one can operate 2
>> octal flashes in stacked mode 

Mahapatra, Amit Kumar Dec. 15, 2023, 10:02 a.m. UTC | #14

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Friday, December 15, 2023 1:40 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote:
> >> Thanks! Can you share with us what flashes you used for testing in
> >> the stacked and parallel configurations?
> > I used SPI-NOR QSPI flashes for testing stacked and parallel.
> 
> I got that, I wanted the flash name or device ID.

N25Q00A, MX66U2G45G, IS25LP01G & W25H02JV are some of the QSPI flashes on 
which we tested. Additionally, we conducted tests on over 30 different 
QSPI flashes from four distinct vendors (Miron, Winbond, Macronix, and ISSI).

> What I'm interested is if each flash is in its own package. Are they?

I'm sorry, but I don't quite understand what you mean by "if each flash in 
its own package."

> 
> >
> >>>> can combine a SPI NOR with a SPI NAND in stacked configuration?
> >>> No, Xilinx/AMD QSPI controllers doesn't support this configuration.
> >>>
> >> 2 SPI NANDs shall work with the AMD controller, right?
> > We never tested 2 SPI-NAND with AMD controller.
> 
> I was asking because I think the stacked layer shall be SPI MEM generic, and
> not particular to SPI NOR.

Yes, I agree.

> 
> >> I skimmed over the QSPI controller datasheet and wondered why one
> >> would get complicated with 2 Quad Flashes when we have Octal. But
> >> then I saw that the same SoC can configure an Octal controller (the
> >> Octal and Quad controllers seems mutual exclusive) and that the Octal
> >> one can operate 2 octal flashes in stacked mode 

Tudor Ambarus Dec. 15, 2023, 10:33 a.m. UTC | #15

On 12/15/23 10:02, Mahapatra, Amit Kumar wrote:
> Hello Tudor,

Hi,

> 
>> -----Original Message-----
>> From: Tudor Ambarus <[e-mail]>
>> Sent: Friday, December 15, 2023 1:40 PM
>> To: Mahapatra, Amit Kumar <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; Simek, Michal <[e-mail]>; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; git (AMD-
>> Xilinx) <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
>> in spi-nor
>>
>>
>>
>> On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote:
>>>> Thanks! Can you share with us what flashes you used for testing in
>>>> the stacked and parallel configurations?
>>> I used SPI-NOR QSPI flashes for testing stacked and parallel.
>>
>> I got that, I wanted the flash name or device ID.
> 
> N25Q00A, MX66U2G45G, IS25LP01G & W25H02JV are some of the QSPI flashes on 
> which we tested. Additionally, we conducted tests on over 30 different 
> QSPI flashes from four distinct vendors (Miron, Winbond, Macronix, and ISSI).
> 

Great.

>> What I'm interested is if each flash is in its own package. Are they?
> 
> I'm sorry, but I don't quite understand what you mean by "if each flash in 
> its own package."
> 

There are flashes that are stacked at the physical level. It's a single
flash with multiple dies, that are all under a single physical package.

As I understand, your stacked flash model is at logical level. You have
2 flashes each in its own package. 2 different entities. Is my
understanding correct?

Cheers,
ta

Mahapatra, Amit Kumar Dec. 15, 2023, 11:20 a.m. UTC | #16

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Friday, December 15, 2023 4:03 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Simek, Michal
> <[e-mail]>; [e-mail]; alsa-
> [e-mail]; [e-mail]; linux-
> [e-mail]; git (AMD-Xilinx) <[e-mail]>;
> [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 12/15/23 10:02, Mahapatra, Amit Kumar wrote:
> > Hello Tudor,
> 
> Hi,
> 
> >
> >> -----Original Message-----
> >> From: Tudor Ambarus <[e-mail]>
> >> Sent: Friday, December 15, 2023 1:40 PM
> >> To: Mahapatra, Amit Kumar <[e-mail]>;
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; Simek, Michal <[e-mail]>;
> >> linux- [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; git (AMD-
> >> Xilinx) <[e-mail]>; [e-mail]
> >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
> >> support in spi-nor
> >>
> >>
> >>
> >> On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote:
> >>>> Thanks! Can you share with us what flashes you used for testing in
> >>>> the stacked and parallel configurations?
> >>> I used SPI-NOR QSPI flashes for testing stacked and parallel.
> >>
> >> I got that, I wanted the flash name or device ID.
> >
> > N25Q00A, MX66U2G45G, IS25LP01G & W25H02JV are some of the QSPI
> flashes
> > on which we tested. Additionally, we conducted tests on over 30
> > different QSPI flashes from four distinct vendors (Miron, Winbond,
> Macronix, and ISSI).
> >
> 
> Great.
> 
> >> What I'm interested is if each flash is in its own package. Are they?
> >
> > I'm sorry, but I don't quite understand what you mean by "if each
> > flash in its own package."
> >
> 
> There are flashes that are stacked at the physical level. It's a single flash with
> multiple dies, that are all under a single physical package.

Got it. The W25H02JV QSPI flash I mentioned earlier is a device with 
with four dies that are stacked at the physical level.

> 
> As I understand, your stacked flash model is at logical level. You have
> 2 flashes each in its own package. 2 different entities. Is my understanding
> correct?

Yes, that’s correct.

I'd like to contribute to your earlier point regarding the placement of 
the stacked layer. As you correctly highlighted, it should be in the 
spi-mem generic layer. For instance, when a read/write operation extends 
across multiple flashes (whether SPI-NOR or SPI-NAND), the stacked layer 
must handle the flash crossover. This requires setting the appropriate CS 
index in mem->spi->cs_index_mask to select the correct slave device and 
updating the data buffer, address & data length in spi_mem_op struct 
variable. Does this align with your understanding?

Regards,
Amit
> 
> Cheers,
> ta

Tudor Ambarus Dec. 19, 2023, 8:26 a.m. UTC | #17

On 15.12.2023 13:20, Mahapatra, Amit Kumar wrote:
> Hello Tudor,
> 

Hi!

>> -----Original Message-----
>> From: Tudor Ambarus <[e-mail]>
>> Sent: Friday, December 15, 2023 4:03 PM
>> To: Mahapatra, Amit Kumar <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Simek, Michal
>> <[e-mail]>; [e-mail]; alsa-
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; git (AMD-Xilinx) <[e-mail]>;
>> [e-mail]
>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
>> in spi-nor
>>
>>
>>
>> On 12/15/23 10:02, Mahapatra, Amit Kumar wrote:
>>> Hello Tudor,
>>
>> Hi,
>>
>>>
>>>> -----Original Message-----
>>>> From: Tudor Ambarus <[e-mail]>
>>>> Sent: Friday, December 15, 2023 1:40 PM
>>>> To: Mahapatra, Amit Kumar <[e-mail]>;
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail]
>>>> Cc: [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; Simek, Michal <[e-mail]>;
>>>> linux- [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; git (AMD-
>>>> Xilinx) <[e-mail]>; [e-mail]
>>>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
>>>> support in spi-nor
>>>>
>>>>
>>>>
>>>> On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote:
>>>>>> Thanks! Can you share with us what flashes you used for testing in
>>>>>> the stacked and parallel configurations?
>>>>> I used SPI-NOR QSPI flashes for testing stacked and parallel.
>>>>
>>>> I got that, I wanted the flash name or device ID.
>>>
>>> N25Q00A, MX66U2G45G, IS25LP01G & W25H02JV are some of the QSPI
>> flashes
>>> on which we tested. Additionally, we conducted tests on over 30
>>> different QSPI flashes from four distinct vendors (Miron, Winbond,
>> Macronix, and ISSI).
>>>
>>
>> Great.
>>
>>>> What I'm interested is if each flash is in its own package. Are they?
>>>
>>> I'm sorry, but I don't quite understand what you mean by "if each
>>> flash in its own package."
>>>
>>
>> There are flashes that are stacked at the physical level. It's a single flash with
>> multiple dies, that are all under a single physical package.
> 
> Got it. The W25H02JV QSPI flash I mentioned earlier is a device with 
> with four dies that are stacked at the physical level.
> 
>>
>> As I understand, your stacked flash model is at logical level. You have
>> 2 flashes each in its own package. 2 different entities. Is my understanding
>> correct?
> 
> Yes, that’s correct.
> 
> I'd like to contribute to your earlier point regarding the placement of 
> the stacked layer. As you correctly highlighted, it should be in the 
> spi-mem generic layer. For instance, when a read/write operation extends 
> across multiple flashes (whether SPI-NOR or SPI-NAND), the stacked layer 
> must handle the flash crossover. This requires setting the appropriate CS 
> index in mem->spi->cs_index_mask to select the correct slave device and 
> updating the data buffer, address & data length in spi_mem_op struct 
> variable. Does this align with your understanding?
> 

This was the initial idea, yes, but we'll have to see how mtd concat
fits in. Maybe the abstraction can be made at the mtd level, which I
suspect mtd concat does. I have to read that driver, never opened it.

Something else to consider: I see that Micron has a twin quad mode:
https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500

The micron's "Separate Chip-Select and Clock Signals" resembles the
AMD's dual parallel 8-bit.
Micron's "Shared Chip-Select and Clock Signals" differs from the AMD's
stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD considers
both as DQ[3:0].

I had a short chat with Michael and he highlighted that instead of the
parallel mode, one would be better of with an octal device. I wonder
whether the quad parallel is worth the effort. I see AMD can select
either quad (single/stacked/parallel) or octal (single/stacked). Is the
parallel mode considered obsolete for new IPs?

Cheers,
ta

Mahapatra, Amit Kumar Dec. 21, 2023, 6:54 a.m. UTC | #18

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Tuesday, December 19, 2023 1:56 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 15.12.2023 13:20, Mahapatra, Amit Kumar wrote:
> > Hello Tudor,
> >
> 
> Hi!
> 
> >> -----Original Message-----
> >> From: Tudor Ambarus <[e-mail]>
> >> Sent: Friday, December 15, 2023 4:03 PM
> >> To: Mahapatra, Amit Kumar <[e-mail]>;
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail]
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; Simek, Michal <[e-mail]>;
> >> [e-mail]; alsa- [e-mail];
> >> [e-mail]; linux- [e-mail]; git
> >> (AMD-Xilinx) <[e-mail]>; [e-mail]
> >> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
> >> support in spi-nor
> >>
> >>
> >>
> >> On 12/15/23 10:02, Mahapatra, Amit Kumar wrote:
> >>> Hello Tudor,
> >>
> >> Hi,
> >>
> >>>
> >>>> -----Original Message-----
> >>>> From: Tudor Ambarus <[e-mail]>
> >>>> Sent: Friday, December 15, 2023 1:40 PM
> >>>> To: Mahapatra, Amit Kumar <[e-mail]>;
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail]
> >>>> Cc: [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]; Simek, Michal <[e-mail]>;
> >>>> linux- [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; git
> >>>> (AMD-
> >>>> Xilinx) <[e-mail]>; [e-mail]
> >>>> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories
> >>>> support in spi-nor
> >>>>
> >>>>
> >>>>
> >>>> On 15.12.2023 09:55, Mahapatra, Amit Kumar wrote:
> >>>>>> Thanks! Can you share with us what flashes you used for testing
> >>>>>> in the stacked and parallel configurations?
> >>>>> I used SPI-NOR QSPI flashes for testing stacked and parallel.
> >>>>
> >>>> I got that, I wanted the flash name or device ID.
> >>>
> >>> N25Q00A, MX66U2G45G, IS25LP01G & W25H02JV are some of the QSPI
> >> flashes
> >>> on which we tested. Additionally, we conducted tests on over 30
> >>> different QSPI flashes from four distinct vendors (Miron, Winbond,
> >> Macronix, and ISSI).
> >>>
> >>
> >> Great.
> >>
> >>>> What I'm interested is if each flash is in its own package. Are they?
> >>>
> >>> I'm sorry, but I don't quite understand what you mean by "if each
> >>> flash in its own package."
> >>>
> >>
> >> There are flashes that are stacked at the physical level. It's a
> >> single flash with multiple dies, that are all under a single physical package.
> >
> > Got it. The W25H02JV QSPI flash I mentioned earlier is a device with
> > with four dies that are stacked at the physical level.
> >
> >>
> >> As I understand, your stacked flash model is at logical level. You
> >> have
> >> 2 flashes each in its own package. 2 different entities. Is my
> >> understanding correct?
> >
> > Yes, that’s correct.
> >
> > I'd like to contribute to your earlier point regarding the placement
> > of the stacked layer. As you correctly highlighted, it should be in
> > the spi-mem generic layer. For instance, when a read/write operation
> > extends across multiple flashes (whether SPI-NOR or SPI-NAND), the
> > stacked layer must handle the flash crossover. This requires setting
> > the appropriate CS index in mem->spi->cs_index_mask to select the
> > correct slave device and updating the data buffer, address & data
> > length in spi_mem_op struct variable. Does this align with your
> understanding?
> >
> 
> This was the initial idea, yes, but we'll have to see how mtd concat fits in.
> Maybe the abstraction can be made at the mtd level, which I suspect mtd
> concat does. I have to read that driver, never opened it.

I haven't explored the mtd concat driver either.

> 
> Something else to consider: I see that Micron has a twin quad mode:
> https://media-www.micron.com/-
> /media/client/global/documents/products/data-sheet/nor-flash/serial-
> nor/mt25t/generation-
> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
> 
> The micron's "Separate Chip-Select and Clock Signals" resembles the AMD's
> dual parallel 8-bit.

Yes, I agree.

> Micron's "Shared Chip-Select and Clock Signals" differs from the AMD's
> stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD considers
> both as DQ[3:0].

Yes, correct.

> 
> I had a short chat with Michael and he highlighted that instead of the parallel
> mode, one would be better of with an octal device. I wonder whether the
> quad parallel is worth the effort. I see AMD can select either quad
> (single/stacked/parallel) or octal (single/stacked). Is the parallel mode
 
Indeed, customers have the flexibility to choose between quad or octal 
options. However, some opt for a cost-effective strategy by selecting 
only Quad SPI in their chipset and boosting throughput through the 
parallel use of two flashes. To gauge the popularity of this 
configuration, I will consult with our marketing team for further 
insights. Given that parallel is a controller feature, it can be 
integrated into the driver file. At present, we can emphasis on 
implementing support for stacked mode, either through a new interface 
like mtd/spi-nor/stacked.c or by utilizing the mtd concat driver.

> considered obsolete for new IPs?

No, the parallel mode feature is still present in AMD's new IPs.

Regards,
Amit
> 
> Cheers,
> ta

Tudor Ambarus Feb. 9, 2024, 11:06 a.m. UTC | #19

On 12/21/23 06:54, Mahapatra, Amit Kumar wrote:
>> Something else to consider: I see that Micron has a twin quad mode:
>> https://media-www.micron.com/-
>> /media/client/global/documents/products/data-sheet/nor-flash/serial-
>> nor/mt25t/generation-
>> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
>>
>> The micron's "Separate Chip-Select and Clock Signals" resembles the AMD's
>> dual parallel 8-bit.
> Yes, I agree.
> 
>> Micron's "Shared Chip-Select and Clock Signals" differs from the AMD's
>> stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD considers
>> both as DQ[3:0].
> Yes, correct.

Amit, please help me to assess this. I assume Micron and Microchip is
using the same concepts as AMD uses for the "Dual Parallel 8-bit IO
mode", but they call it "Twin Quad Mode".

I was wrong, the AMD datasheet [1] was misleading [2], it described the
IOs for both flashes as IO[3:0], but later on in the "Table QSPI
Interface Signals" the second flash is described with IO[7:4].

The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS# and
CLK# lines for each flash. As Micron does, isn't it?

Micron says [3] that:
"The device contains two quad I/O die, each able to operate
independently for a total of eight I/Os. The memory map applies to each
die. Each die has internal registers for status, configuration, and
device protection that can be set and read independently from one other.
Micron recommends that internal configuration settings for the two die
be set identically."

it also says that:
"When using quad commands in XIO-SPI or when using QIO-SPI,
DQ[3:0]/DQ[7:4] are I/O."

So I guess the upper layers just ask for a chunk of memory to be written
and the controller handles the cs# lines automatically. How is the AMD
controller working, do you have to drive the cs# lines manually, or you
just set the parallel mode and the controller takes care of everything?

I assume this is how mchp is handling things, they seem to just set a
bit the protocol into the QSPI_IFR.PROTTYP register field and that's all
[4]. They even seem to write the registers of both flashes at the same time.

In what regards the AMD's "dual stack interface", AMD is sharing the
clock and IO lines and uses dedicated CS# lines for the flashes, whereas
Micron shares the CS# and CLK# lines with different IO lines.

Amit, please study the architectures used by mchp, micron and amd and
let us know if they are the same or they differ, and if they differ what
are the differences.

I added Conor from mchp in cc, I see Nicolas is already there, and Bean
from micron.

Thanks,
ta

[1]
https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-Signals
[2]
https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv4ry78xM0_EF5kv4mA
[3]
https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
[4]
https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf

Tudor Ambarus Feb. 9, 2024, 4:13 p.m. UTC | #20

On 2/9/24 11:06, Tudor Ambarus wrote:
> 
> 
> On 12/21/23 06:54, Mahapatra, Amit Kumar wrote:
>>> Something else to consider: I see that Micron has a twin quad mode:
>>> https://media-www.micron.com/-
>>> /media/client/global/documents/products/data-sheet/nor-flash/serial-
>>> nor/mt25t/generation-
>>> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
>>>
>>> The micron's "Separate Chip-Select and Clock Signals" resembles the AMD's
>>> dual parallel 8-bit.
>> Yes, I agree.
>>
>>> Micron's "Shared Chip-Select and Clock Signals" differs from the AMD's
>>> stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD considers
>>> both as DQ[3:0].
>> Yes, correct.
> 
> Amit, please help me to assess this. I assume Micron and Microchip is
> using the same concepts as AMD uses for the "Dual Parallel 8-bit IO
> mode", but they call it "Twin Quad Mode".
> 
> I was wrong, the AMD datasheet [1] was misleading [2], it described the
> IOs for both flashes as IO[3:0], but later on in the "Table QSPI
> Interface Signals" the second flash is described with IO[7:4].
> 
> The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS# and
> CLK# lines for each flash. As Micron does, isn't it?
> 
> Micron says [3] that:
> "The device contains two quad I/O die, each able to operate
> independently for a total of eight I/Os. The memory map applies to each
> die. Each die has internal registers for status, configuration, and
> device protection that can be set and read independently from one other.
> Micron recommends that internal configuration settings for the two die
> be set identically."

Amit,

I forgot to say my first conclusion about the quote from above. Even if
the dies are in the same physical package, micron asks users to
configure each die as it is a self-standing entity, IOW to configure
each die as it is a flash on its own. To me it looks like 2 concatenated
flashes at first look. Thus identical to how AMD controller works.
Please clarify this.

> 
> it also says that:
> "When using quad commands in XIO-SPI or when using QIO-SPI,
> DQ[3:0]/DQ[7:4] are I/O."

and this would be a parallel concatenation of two flashes.

Then it would be good if you let us now the similarities and differences
between how amd and mchp controller work, I scrawled few ideas below.

thanks,
ta
> 
> So I guess the upper layers just ask for a chunk of memory to be written
> and the controller handles the cs# lines automatically. How is the AMD
> controller working, do you have to drive the cs# lines manually, or you
> just set the parallel mode and the controller takes care of everything?
> 
> I assume this is how mchp is handling things, they seem to just set a
> bit the protocol into the QSPI_IFR.PROTTYP register field and that's all
> [4]. They even seem to write the registers of both flashes at the same time.
> 
> In what regards the AMD's "dual stack interface", AMD is sharing the
> clock and IO lines and uses dedicated CS# lines for the flashes, whereas
> Micron shares the CS# and CLK# lines with different IO lines.
> 
> Amit, please study the architectures used by mchp, micron and amd and
> let us know if they are the same or they differ, and if they differ what
> are the differences.
> 
> I added Conor from mchp in cc, I see Nicolas is already there, and Bean
> from micron.
> 
> Thanks,
> ta
> 
> [1]
> https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-Signals
> [2]
> https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv4ry78xM0_EF5kv4mA
> [3]
> https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
> [4]
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf

Mahapatra, Amit Kumar March 13, 2024, 4:03 p.m. UTC | #21

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Friday, February 9, 2024 4:36 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]; Conor Dooley
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> On 12/21/23 06:54, Mahapatra, Amit Kumar wrote:
> >> Something else to consider: I see that Micron has a twin quad mode:
> >> https://media-www.micron.com/-
> >> /media/client/global/documents/products/data-sheet/nor-flash/serial-
> >> nor/mt25t/generation-
> >>
> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
> >>
> >> The micron's "Separate Chip-Select and Clock Signals" resembles the
> >> AMD's dual parallel 8-bit.
> > Yes, I agree.
> >
> >> Micron's "Shared Chip-Select and Clock Signals" differs from the
> >> AMD's stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD
> >> considers both as DQ[3:0].
> > Yes, correct.
> 
> Amit, please help me to assess this. I assume Micron and Microchip is using
> the same concepts as AMD uses for the "Dual Parallel 8-bit IO mode", but
> they call it "Twin Quad Mode".

That's accurate. It's the same concept.
> 
> I was wrong, the AMD datasheet [1] was misleading [2], it described the IOs
> for both flashes as IO[3:0], but later on in the "Table QSPI Interface Signals"
> the second flash is described with IO[7:4].

That’s correct.
> 
> The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS# and CLK#
> lines for each flash. As Micron does, isn't it?

Correct.
> 
> Micron says [3] that:
> "The device contains two quad I/O die, each able to operate independently
> for a total of eight I/Os. The memory map applies to each die. Each die has
> internal registers for status, configuration, and device protection that can be
> set and read independently from one other.
> Micron recommends that internal configuration settings for the two die be set
> identically."
> 
> it also says that:
> "When using quad commands in XIO-SPI or when using QIO-SPI,
> DQ[3:0]/DQ[7:4] are I/O."
> 
> So I guess the upper layers just ask for a chunk of memory to be written and
> the controller handles the cs# lines automatically. How is the AMD controller
> working, do you have to drive the cs# lines manually, or you just set the
> parallel mode and the controller takes care of everything?

https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Word-Format
In parallel mode, the driver sets both the CS_LOWER and CS_UPPER bits 
in the Cmd_FIFO_Data register(please refer the above link), and sets 
BUS_SEL to 2’b11 to utilize all 8 IO lines. The controller then manages 
the assertion and de-assertion of the CS# lines.
> 
> I assume this is how mchp is handling things, they seem to just set a bit the
> protocol into the QSPI_IFR.PROTTYP register field and that's all [4]. They even
> seem to write the registers of both flashes at the same time.

Yes, that's accurate, but the key distinction is that in Microchip, the 
QSPI controller has one CS# (referred to as QCS), whereas the AMD QSPI 
controller has 2 CS#(referred to as CS0 & CS1).

Regards,
Amit

> 
> In what regards the AMD's "dual stack interface", AMD is sharing the clock
> and IO lines and uses dedicated CS# lines for the flashes, whereas Micron
> shares the CS# and CLK# lines with different IO lines.
> 
> Amit, please study the architectures used by mchp, micron and amd and let
> us know if they are the same or they differ, and if they differ what are the
> differences.
> 
> I added Conor from mchp in cc, I see Nicolas is already there, and Bean from
> micron.
> 
> Thanks,
> ta
> 
> [1]
> https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-
> Signals
> [2]
> https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv
> 4ry78xM0_EF5kv4mA
> [3]
> https://media-www.micron.com/-
> /media/client/global/documents/products/data-sheet/nor-flash/serial-
> nor/mt25t/generation-
> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
> [4]
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/
> ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-
> DS60001765.pdf

Mahapatra, Amit Kumar March 13, 2024, 4:03 p.m. UTC | #22

> -----Original Message-----
> From: Tudor Ambarus <[e-mail]>
> Sent: Friday, February 9, 2024 9:44 PM
> To: Mahapatra, Amit Kumar <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Simek, Michal <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; git (AMD-
> Xilinx) <[e-mail]>; [e-mail]; Conor Dooley
> <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 2/9/24 11:06, Tudor Ambarus wrote:
> >
> >
> > On 12/21/23 06:54, Mahapatra, Amit Kumar wrote:
> >>> Something else to consider: I see that Micron has a twin quad mode:
> >>> https://media-www.micron.com/-
> >>> /media/client/global/documents/products/data-sheet/nor-flash/serial-
> >>> nor/mt25t/generation-
> >>>
> b/mt25t_qljs_l_512_xba_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
> >>>
> >>> The micron's "Separate Chip-Select and Clock Signals" resembles the
> >>> AMD's dual parallel 8-bit.
> >> Yes, I agree.
> >>
> >>> Micron's "Shared Chip-Select and Clock Signals" differs from the
> >>> AMD's stacked mode, as Micron uses DQ[3:0] and DQ[7:4], whereas AMD
> >>> considers both as DQ[3:0].
> >> Yes, correct.
> >
> > Amit, please help me to assess this. I assume Micron and Microchip is
> > using the same concepts as AMD uses for the "Dual Parallel 8-bit IO
> > mode", but they call it "Twin Quad Mode".
> >
> > I was wrong, the AMD datasheet [1] was misleading [2], it described
> > the IOs for both flashes as IO[3:0], but later on in the "Table QSPI
> > Interface Signals" the second flash is described with IO[7:4].
> >
> > The AMD's 8-bit Dual Flash Parallel Interface is using dedicated CS#
> > and CLK# lines for each flash. As Micron does, isn't it?
> >
> > Micron says [3] that:
> > "The device contains two quad I/O die, each able to operate
> > independently for a total of eight I/Os. The memory map applies to
> > each die. Each die has internal registers for status, configuration,
> > and device protection that can be set and read independently from one
> other.
> > Micron recommends that internal configuration settings for the two die
> > be set identically."
> 

Hello Tudor,

> Amit,
> 
> I forgot to say my first conclusion about the quote from above. Even if the
> dies are in the same physical package, micron asks users to configure each die
> as it is a self-standing entity, IOW to configure each die as it is a flash on its
> own. To me it looks like 2 concatenated flashes at first look. Thus identical to
> how AMD controller works.
> Please clarify this.

That’s correct, the Micron flash that you referred can communicate with 
the AMD QSPI controller in both parallel and stacked mode.
> 
> >
> > it also says that:
> > "When using quad commands in XIO-SPI or when using QIO-SPI,
> > DQ[3:0]/DQ[7:4] are I/O."
> 
> and this would be a parallel concatenation of two flashes.

That's correct.

Regards,
Amit
> 
> Then it would be good if you let us now the similarities and differences
> between how amd and mchp controller work, I scrawled few ideas below.
> 
> thanks,
> ta
> >
> > So I guess the upper layers just ask for a chunk of memory to be
> > written and the controller handles the cs# lines automatically. How is
> > the AMD controller working, do you have to drive the cs# lines
> > manually, or you just set the parallel mode and the controller takes care of
> everything?
> >
> > I assume this is how mchp is handling things, they seem to just set a
> > bit the protocol into the QSPI_IFR.PROTTYP register field and that's
> > all [4]. They even seem to write the registers of both flashes at the same
> time.
> >
> > In what regards the AMD's "dual stack interface", AMD is sharing the
> > clock and IO lines and uses dedicated CS# lines for the flashes,
> > whereas Micron shares the CS# and CLK# lines with different IO lines.
> >
> > Amit, please study the architectures used by mchp, micron and amd and
> > let us know if they are the same or they differ, and if they differ
> > what are the differences.
> >
> > I added Conor from mchp in cc, I see Nicolas is already there, and
> > Bean from micron.
> >
> > Thanks,
> > ta
> >
> > [1]
> > https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Inter
> > face-Signals
> > [2]
> >
> https://docs.xilinx.com/viewer/attachment/dwmjhDJGICdJqD4swyVzcQ/fD8nv
> > 4ry78xM0_EF5kv4mA
> > [3]
> > https://media-www.micron.com/-
> /media/client/global/documents/products/
> > data-sheet/nor-flash/serial-nor/mt25t/generation-b/mt25t_qljs_l_512_xb
> > a_0.pdf?rev=de70b770c5dc4da8b8ead06b57c03500
> > [4]
> >
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/
> Produ
> > ctDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf

Patch

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 93ae69b7ff83..e990be7c7eb6 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1821,13 +1821,18 @@  static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
 static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
-	u32 addr, len;
+	struct spi_nor_flash_parameter *params;
+	u32 addr, len, offset, cur_cs_num = 0;
 	uint32_t rem;
 	int ret;
+	u64 sz;
 
 	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
 			(long long)instr->len);
 
+	params = spi_nor_get_params(nor, 0);
+	sz = params->size;
+
 	if (spi_nor_has_uniform_erase(nor)) {
 		div_u64_rem(instr->len, mtd->erasesize, &rem);
 		if (rem)
@@ -1849,23 +1854,27 @@  static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 		if (ret)
 			goto erase_err;
 
-		ret = spi_nor_erase_chip(nor);
-		spi_nor_unlock_device(nor);
-		if (ret)
-			goto erase_err;
+		while (cur_cs_num < nor->num_flash) {
+			nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
+			ret = spi_nor_erase_chip(nor);
+			spi_nor_unlock_device(nor);
+			if (ret)
+				goto erase_err;
 
-		/*
-		 * Scale the timeout linearly with the size of the flash, with
-		 * a minimum calibrated to an old 2MB flash. We could try to
-		 * pull these from CFI/SFDP, but these values should be good
-		 * enough for now.
-		 */
-		timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
-			      CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
-			      (unsigned long)(mtd->size / SZ_2M));
-		ret = spi_nor_wait_till_ready_with_timeout(nor, timeout);
-		if (ret)
-			goto erase_err;
+			/*
+			 * Scale the timeout linearly with the size of the flash, with
+			 * a minimum calibrated to an old 2MB flash. We could try to
+			 * pull these from CFI/SFDP, but these values should be good
+			 * enough for now.
+			 */
+			timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
+				      CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
+				      (unsigned long)(params->size / SZ_2M));
+			ret = spi_nor_wait_till_ready_with_timeout(nor, timeout);
+			if (ret)
+				goto erase_err;
+			cur_cs_num++;
+		}
 
 	/* REVISIT in some cases we could speed up erasing large regions
 	 * by using SPINOR_OP_SE instead of SPINOR_OP_BE_4K.  We may have set up
@@ -1874,12 +1883,26 @@  static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 	/* "sector"-at-a-time erase */
 	} else if (spi_nor_has_uniform_erase(nor)) {
+		/* Determine the flash from which the operation need to start */
+		while ((cur_cs_num < nor->num_flash) && (addr > sz - 1)) {
+			cur_cs_num++;
+			params = spi_nor_get_params(nor, cur_cs_num);
+			sz += params->size;
+		}
+
 		while (len) {
 			ret = spi_nor_lock_device(nor);
 			if (ret)
 				goto erase_err;
 
-			ret = spi_nor_erase_sector(nor, addr);
+			nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
+			offset = addr;
+			if (nor->flags & SNOR_F_HAS_STACKED) {
+				params = spi_nor_get_params(nor, cur_cs_num);
+				offset -= (sz - params->size);
+			}
+
+			ret = spi_nor_erase_sector(nor, offset);
 			spi_nor_unlock_device(nor);
 			if (ret)
 				goto erase_err;
@@ -1890,13 +1913,45 @@  static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 			addr += mtd->erasesize;
 			len -= mtd->erasesize;
+
+			/*
+			 * Flash cross over condition in stacked mode.
+			 */
+			if ((nor->flags & SNOR_F_HAS_STACKED) && (addr > sz - 1)) {
+				cur_cs_num++;
+				params = spi_nor_get_params(nor, cur_cs_num);
+				sz += params->size;
+			}
 		}
 
 	/* erase multiple sectors */
 	} else {
-		ret = spi_nor_erase_multi_sectors(nor, addr, len);
-		if (ret)
-			goto erase_err;
+		u64 erase_len = 0;
+
+		/* Determine the flash from which the operation need to start */
+		while ((cur_cs_num < nor->num_flash) && (addr > sz - 1)) {
+			cur_cs_num++;
+			params = spi_nor_get_params(nor, cur_cs_num);
+			sz += params->size;
+		}
+		/* perform multi sector erase onec per Flash*/
+		while (len) {
+			erase_len = (len > (sz - addr)) ? (sz - addr) : len;
+			offset = addr;
+			nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
+			if (nor->flags & SNOR_F_HAS_STACKED) {
+				params = spi_nor_get_params(nor, cur_cs_num);
+				offset -= (sz - params->size);
+			}
+			ret = spi_nor_erase_multi_sectors(nor, offset, erase_len);
+			if (ret)
+				goto erase_err;
+			len -= erase_len;
+			addr += erase_len;
+			cur_cs_num++;
+			params = spi_nor_get_params(nor, cur_cs_num);
+			sz += params->size;
+		}
 	}
 
 	ret = spi_nor_write_disable(nor);
@@ -2087,9 +2142,11 @@  static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 			size_t *retlen, u_char *buf)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	struct spi_nor_flash_parameter *params;
+	ssize_t ret, read_len, len_lock =  len;
 	loff_t from_lock = from;
-	size_t len_lock = len;
-	ssize_t ret;
+	u32 cur_cs_num = 0;
+	u64 sz;
 
 	dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
 
@@ -2097,9 +2154,23 @@  static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (ret)
 		return ret;
 
+	params = spi_nor_get_params(nor, 0);
+	sz = params->size;
+
+	/* Determine the flash from which the operation need to start */
+	while ((cur_cs_num < nor->num_flash) && (from > sz - 1)) {
+		cur_cs_num++;
+		params = spi_nor_get_params(nor, cur_cs_num);
+		sz += params->size;
+	}
 	while (len) {
 		loff_t addr = from;
 
+		nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
+		read_len = (len > (sz - addr)) ? (sz - addr) : len;
+		params = spi_nor_get_params(nor, cur_cs_num);
+		addr -= (sz - params->size);
+
 		addr = spi_nor_convert_addr(nor, addr);
 
 		ret = spi_nor_read_data(nor, addr, len, buf);
@@ -2111,11 +2182,22 @@  static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 		if (ret < 0)
 			goto read_err;
 
-		WARN_ON(ret > len);
+		WARN_ON(ret > read_len);
 		*retlen += ret;
 		buf += ret;
 		from += ret;
 		len -= ret;
+
+		/*
+		 * Flash cross over condition in stacked mode.
+		 *
+		 */
+		if ((nor->flags & SNOR_F_HAS_STACKED) && (from > sz - 1)) {
+			cur_cs_num++;
+			params = spi_nor_get_params(nor, cur_cs_num);
+			sz += params->size;
+		}
+
 	}
 	ret = 0;
 
@@ -2136,8 +2218,9 @@  static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
 	struct spi_nor_flash_parameter *params;
 	size_t page_offset, page_remain, i;
+	u32 page_size, cur_cs_num = 0;
 	ssize_t ret;
-	u32 page_size;
+	u64 sz;
 
 	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
 
@@ -2147,6 +2230,14 @@  static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 	params = spi_nor_get_params(nor, 0);
 	page_size = params->page_size;
+	sz = params->size;
+
+	/* Determine the flash from which the operation need to start */
+	while ((cur_cs_num < nor->num_flash) && (to > sz - 1)) {
+		cur_cs_num++;
+		params = spi_nor_get_params(nor, cur_cs_num);
+		sz += params->size;
+	}
 
 	for (i = 0; i < len; ) {
 		ssize_t written;
@@ -2167,6 +2258,10 @@  static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 		/* the size of data remaining on the first page */
 		page_remain = min_t(size_t, page_size - page_offset, len - i);
 
+		nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
+		params = spi_nor_get_params(nor, cur_cs_num);
+		addr -= (sz - params->size);
+
 		addr = spi_nor_convert_addr(nor, addr);
 
 		ret = spi_nor_lock_device(nor);
@@ -2184,6 +2279,15 @@  static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 			goto write_err;
 		*retlen += written;
 		i += written;
+
+		/*
+		 * Flash cross over condition in stacked mode.
+		 */
+		if ((nor->flags & SNOR_F_HAS_STACKED) && ((to + i) > sz - 1)) {
+			cur_cs_num++;
+			params = spi_nor_get_params(nor, cur_cs_num);
+			sz += params->size;
+		}
 	}
 
 write_err:
@@ -2297,8 +2401,6 @@  int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
 static int spi_nor_spimem_check_op(struct spi_nor *nor,
 				   struct spi_mem_op *op)
 {
-	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
-
 	/*
 	 * First test with 4 address bytes. The opcode itself might
 	 * be a 3B addressing opcode but we don't care, because
@@ -2307,7 +2409,7 @@  static int spi_nor_spimem_check_op(struct spi_nor *nor,
 	 */
 	op->addr.nbytes = 4;
 	if (!spi_mem_supports_op(nor->spimem, op)) {
-		if (params->size > SZ_16M)
+		if (nor->mtd.size > SZ_16M)
 			return -EOPNOTSUPP;
 
 		/* If flash size <= 16MB, 3 address bytes are sufficient */
@@ -2905,7 +3007,10 @@  static void spi_nor_init_fixup_flags(struct spi_nor *nor)
 static int spi_nor_late_init_params(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
-	int ret;
+	struct device_node *np = spi_nor_get_flash_node(nor);
+	u64 flash_size[SNOR_FLASH_CNT_MAX];
+	u32 idx = 0;
+	int rc, ret;
 
 	if (nor->manufacturer && nor->manufacturer->fixups &&
 	    nor->manufacturer->fixups->late_init) {
@@ -2937,6 +3042,44 @@  static int spi_nor_late_init_params(struct spi_nor *nor)
 	if (params->n_banks > 1)
 		params->bank_size = div64_u64(params->size, params->n_banks);
 
+	nor->num_flash = 0;
+
+	/*
+	 * The flashes that are connected in stacked mode should be of same make.
+	 * Except the flash size all other properties are identical for all the
+	 * flashes connected in stacked mode.
+	 * The flashes that are connected in parallel mode should be identical.
+	 */
+	while (idx < SNOR_FLASH_CNT_MAX) {
+		rc = of_property_read_u64_index(np, "stacked-memories", idx, &flash_size[idx]);
+		if (rc)
+			break;
+		idx++;
+		if (!(nor->flags & SNOR_F_HAS_STACKED))
+			nor->flags |= SNOR_F_HAS_STACKED;
+
+		nor->num_flash++;
+	}
+
+	/*
+	 * By default one flash device should be connected
+	 * so, nor->num_flash is 1.
+	 */
+	if (!nor->num_flash)
+		nor->num_flash = 1;
+
+	if (nor->flags & SNOR_F_HAS_STACKED) {
+		for (idx = 1; idx < nor->num_flash; idx++) {
+			params = spi_nor_get_params(nor, idx);
+			params = devm_kzalloc(nor->dev, sizeof(*params), GFP_KERNEL);
+			if (params) {
+				memcpy(params, spi_nor_get_params(nor, 0), sizeof(*params));
+				params->size = flash_size[idx];
+				spi_nor_set_params(nor, idx, params);
+			}
+		}
+	}
+
 	return 0;
 }
 
@@ -3145,16 +3288,28 @@  static int spi_nor_set_octal_dtr(struct spi_nor *nor, bool enable)
  */
 static int spi_nor_quad_enable(struct spi_nor *nor)
 {
-	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
+	struct spi_nor_flash_parameter *params;
+	int err, idx;
 
-	if (!params->quad_enable)
-		return 0;
+	for (idx = 0; idx < nor->num_flash; idx++) {
+		params = spi_nor_get_params(nor, idx);
+		if (!params->quad_enable)
+			return 0;
 
-	if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
-	      spi_nor_get_protocol_width(nor->write_proto) == 4))
-		return 0;
+		if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
+		      spi_nor_get_protocol_width(nor->write_proto) == 4))
+			return 0;
+		/*
+		 * Set the appropriate CS index before
+		 * issuing the command.
+		 */
+		nor->spimem->spi->cs_index_mask = 0x01 << idx;
 
-	return params->quad_enable(nor);
+		err = params->quad_enable(nor);
+		if (err)
+			return err;
+	}
+	return err;
 }
 
 /**
@@ -3186,7 +3341,7 @@  int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 
 static int spi_nor_init(struct spi_nor *nor)
 {
-	int err;
+	int err, idx;
 
 	err = spi_nor_set_octal_dtr(nor, true);
 	if (err) {
@@ -3227,9 +3382,16 @@  static int spi_nor_init(struct spi_nor *nor)
 		 */
 		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
 			  "enabling reset hack; may not recover from unexpected reboots\n");
-		err = spi_nor_set_4byte_addr_mode(nor, true);
-		if (err)
-			return err;
+		for (idx = 0; idx < nor->num_flash; idx++) {
+			/*
+			 * Select the appropriate CS index before
+			 * issuing the command.
+			 */
+			nor->spimem->spi->cs_index_mask = 0x01 << idx;
+			err = spi_nor_set_4byte_addr_mode(nor, true);
+			if (err)
+				return err;
+		}
 	}
 
 	return 0;
@@ -3344,18 +3506,28 @@  static void spi_nor_put_device(struct mtd_info *mtd)
 static void spi_nor_restore(struct spi_nor *nor)
 {
 	int ret;
+	int idx;
 
 	/* restore the addressing mode */
 	if (nor->addr_nbytes == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
 	    nor->flags & SNOR_F_BROKEN_RESET) {
-		ret = spi_nor_set_4byte_addr_mode(nor, false);
-		if (ret)
+		for (idx = 0; idx < nor->num_flash; idx++) {
 			/*
-			 * Do not stop the execution in the hope that the flash
-			 * will default to the 3-byte address mode after the
-			 * software reset.
+			 * Select the appropriate CS index before
+			 * issuing the command.
 			 */
-			dev_err(nor->dev, "Failed to exit 4-byte address mode, err = %d\n", ret);
+			nor->spimem->spi->cs_index_mask = 1 << idx;
+			ret = spi_nor_set_4byte_addr_mode(nor, false);
+			if (ret)
+				/*
+				 * Do not stop the execution in the hope that the flash
+				 * will default to the 3-byte address mode after the
+				 * software reset.
+				 */
+				dev_err(nor->dev,
+					"Failed to exit 4-byte address mode, err = %d\n",
+					ret);
+		}
 	}
 
 	if (nor->flags & SNOR_F_SOFT_RESET)
@@ -3422,6 +3594,8 @@  static void spi_nor_set_mtd_info(struct spi_nor *nor)
 	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
 	struct mtd_info *mtd = &nor->mtd;
 	struct device *dev = nor->dev;
+	u64 total_sz = 0;
+	int idx;
 
 	spi_nor_set_mtd_locking_ops(nor);
 	spi_nor_set_mtd_otp_ops(nor);
@@ -3440,7 +3614,11 @@  static void spi_nor_set_mtd_info(struct spi_nor *nor)
 		mtd->_erase = spi_nor_erase;
 	mtd->writesize = params->writesize;
 	mtd->writebufsize = params->page_size;
-	mtd->size = params->size;
+	for (idx = 0; idx < nor->num_flash; idx++) {
+		params = spi_nor_get_params(nor, idx);
+		total_sz += params->size;
+	}
+	mtd->size = total_sz;
 	mtd->_read = spi_nor_read;
 	/* Might be already set by some SST flashes. */
 	if (!mtd->_write)
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 93cd2fc3606d..b2997eca7551 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -18,6 +18,9 @@ 
 #define SPI_NOR_DEFAULT_N_BANKS 1
 #define SPI_NOR_DEFAULT_SECTOR_SIZE SZ_64K
 
+/* In single configuration enable CS0 */
+#define SPI_NOR_ENABLE_CS0     BIT(0)
+
 /* Standard SPI NOR flash operations. */
 #define SPI_NOR_READID_OP(naddr, ndummy, buf, len)			\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0),			\
@@ -140,6 +143,7 @@  enum spi_nor_option_flags {
 	SNOR_F_RWW		= BIT(14),
 	SNOR_F_ECC		= BIT(15),
 	SNOR_F_NO_WP		= BIT(16),
+	SNOR_F_HAS_STACKED      = BIT(17),
 };
 
 struct spi_nor_read_command {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 59909e7d6f53..9d72b0bbab94 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -127,6 +127,12 @@ 
 #define SR2_LB3			BIT(5)	/* Security Register Lock Bit 3 */
 #define SR2_QUAD_EN_BIT7	BIT(7)
 
+/*
+ * Maximum number of flashes that can be connected
+ * in stacked/parallel configuration
+ */
+#define	SNOR_FLASH_CNT_MAX	4
+
 /* Supported SPI protocols */
 #define SNOR_PROTO_INST_MASK	GENMASK(23, 16)
 #define SNOR_PROTO_INST_SHIFT	16
@@ -378,6 +384,7 @@  struct spi_nor_flash_parameter;
  *                      hooks, or dynamically when parsing the SFDP tables.
  * @dirmap:		pointers to struct spi_mem_dirmap_desc for reads/writes.
  * @priv:		pointer to the private data
+ * @num_flash		number of flashes connected in parallel or stacked mode
  */
 struct spi_nor {
 	struct mtd_info		mtd;
@@ -412,13 +419,13 @@  struct spi_nor {
 
 	const struct spi_nor_controller_ops *controller_ops;
 
-	struct spi_nor_flash_parameter *params;
+	struct spi_nor_flash_parameter *params[SNOR_FLASH_CNT_MAX];
 
 	struct {
 		struct spi_mem_dirmap_desc *rdesc;
 		struct spi_mem_dirmap_desc *wdesc;
 	} dirmap;
-
+	u32			num_flash;
 	void *priv;
 };
 
@@ -435,13 +442,13 @@  static inline struct device_node *spi_nor_get_flash_node(struct spi_nor *nor)
 
 static inline struct spi_nor_flash_parameter *spi_nor_get_params(const struct spi_nor *nor, u8 idx)
 {
-	return nor->params;
+	return nor->params[idx];
 }
 
 static inline void spi_nor_set_params(struct spi_nor *nor, u8 idx,
 				      struct spi_nor_flash_parameter *params)
 {
-	nor->params = params;
+	nor->params[idx] = params;
 }
 /**
  * spi_nor_scan() - scan the SPI NOR