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

Tudor Ambarus
2023-12-12T15:02:36Z

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