mtd layer: support of hybrid flash(W25M161AW) having both NOR and NAND flash

[Frieder Schrempf]

Hi Boris,

On 08.01.2018 13:31, Boris Brezillon wrote:
> Hi Frieder,
> 
> On Mon, 8 Jan 2018 12:02:19 +0100
> Frieder Schrempf <frieder.schrempf at exceet.de> wrote:
> 
>>>>>>> with the "windbond,w25q16fw" driver modeled as a simple
>>>>>>> "spi-multiplexer" that registers its own virtual spi-bus. Then when
>>>>>>> spi-nor or spi-nand tries to communicate with their appropriate die,
>>>>>>> it sends the Software Die Select command if needed and then passes on
>>>>>>> the message to its parent bus.
>>>>>>>
>>>>>>> That way there should be no changes needed for spi-nor / spi-nand
>>>>>>> themselves. (The devil is probably in the details ;-)
>>>>>>
>>>>>> Yep, I thought about this approach, and it's indeed quite elegant, but
>>>>>> we're missing the lock I was mentioning in my previous reply. We need
>>>>>> to prevent die selection not only for the time we're sending a single
>>>>>> SPI message, but for the whole operation (which can be formed of
>>>>>> several SPI messages). Or maybe I'm wrong, and operations can actually
>>>>>> be interleaved, but I wouldn't bet on that ;-).
>>
>> With operations, that consist of several SPI messages, do you mean
>> something like NAND page program?
> 
> Yep.
> 
>>
>> Because I'm quite sure something like this should be possible (and all
>> of these commands consist only of one spi message, don't they?):
>> 1. Select second (NAND) die
>> 2. Send data to the NAND page buffer (PROGRAM)
>> 3. Select first (NOR) die
>> 4. Program data to a NOR block
>> 5. Select second (NAND) die
>> 6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)
> 
> Yes, and that's the problem, if you don't have a lock, the sequence you
> describe above could be re-ordered like this:
> 
> 1. Select second (NAND) die
> 2. Select first (NOR) die
> 3. Send data to the NAND page buffer (PROGRAM)
> 4. Program data to a NOR block
> ...

Ok thanks. Now I got the problem.

> 
>>
>>>>>
>>>>> Ah, I missed that. I thought about it, and then tried to hand wave it
>>>>> away with the "if they behave like normal chips" ;-)
>>>>>
>>>>> The mdio-bus supports nested locking, so you can do something like this:
>>>>>
>>>>> mutex_lock_nested(bus->mdio_lock, MDIO_BUS_NESTED);
>>>>> bus->write();
>>>>> bus->read();
>>>>> mutex_unlock(bus->mdio_lock);
>>>>>
>>>>> without worrying someone else using the bus in between. [1] for an example
>>>>> user.
>>>>>
>>>>> So going a similar approach with flagging the appropriate chips in
>>>>> spi-nor/spi-nand as needing nested locking and then doing it for the
>>>>> appropriate commands should solve that issue.
>>>>>
>>>>>      
>>>>
>>>> Device tree update:-
>>>>
>>>> &qspi {
>>>>      ...
>>>>          qflash0: dual-flash at 0 {
>>>>                  compatible = "winbond,w25q16fw", "hybrid"; <-- new compatibility value
>>>
>>> "hybrid" is not needed, you know that the flash is hybrid with the
>>> "winbond,w25q16fw" string.
>>>    
>>>>                  reg = <0>;
>>>>                  spi-max-frequency = <20000000>;
>>>>                  #address-cells = <1>;
>>>>                  #size-cells = <0>;
>>>>
>>>>                  nor at 0 {
>>>>                                  compatible = "jedec,spi-nor";
>>>>                                  reg = <0>;
>>>>                                  #address-cells = <1>;
>>>>                                  #size-cells = <1>;
>>>>
>>>>                                  partitions {
>>>>                                                  ...
>>>>                                  };
>>>>                  };
>>>>
>>>>                  nand at 1 {
>>>>                                  compatible = "jedec,spi-nand"; /* or
>>>> whatever the correct nand-compatible would be */
>>>>                                  reg = <1>;
>>>>                                  #address-cells = <1>;
>>>>                                  #size-cells = <1>;
>>>>
>>>>                                  partitions {
>>>>                                                  ...
>>>>                                  };
>>>>                  };
>>>
>>> Not sure exposing the dies in the DT is such a good idea. You should
>>> have a specific handling for "winbond,w25q16fw" which registers one
>>> NAND and one NOR.
>>>    
>>>>
>>>>      };
>>>> };
>>>
>>>
>>>    
>>>>
>>>>
>>>> There will be only one file i.e. fsl_qspi.c handing NOR and NAND. QSPI controller will have SPI NOR and a SPI NAND controller embedded.
>>>> Question: What should be the location of this file?  driver/mtd/spi-nor definitely is not right place??
>>>
>>> It stay in driver/mtd/spi-nor until we create a spi-flash layer
>>> (driver/mtd/spi-flash).
>>
>> Can it really stay there even if it would have NAND support implemented
>> and be used by the SPI NAND framework?
> 
> Yes, as long as this is a temporary situation.

Ok.

> 
>>
>> How does the longterm plan for implementing SPI NAND, FSL QSPI NAND/NOR
>> and spi-flash layer look like?
>>
>> I would propose something like this, but I'm not sure if this is
>> appropriate:
>>
>> 1. Adding SPI NAND framework (with Micron and generic SPI)
> 
> Yep, that's the first step, and I think we should move on with what we
> have and work on improving the situation (to share more code between
> SPI NOR and SPI NAND) in parallel.

Ok.

> 
>> 2. Adding FSL QSPI as a SPI NAND controller in mtd/nand/spi/controllers
>> 3. Merging FSL QSPI NAND driver from mtd/nand/spi/controllers with NOR
>> driver at mtd/spi-nor/fsl_quadspi.c
> 
> I'd really prefer to have a single driver supporting both NOR and NAND
> devices. What's the problem with adding SPI NAND support to the
> mtd/spi-nor/fsl_quadspi.c?

There's not really a problem. I will see if can adapt my FSL QSPI NAND 
code to fit into the NOR driver.

> 
>> 4. Creating spi-flash layer and move FSL QSPI NOR/NAND driver to
>> mtd/spi-flash
> 
> Yep, that's the long term goal.
> 
>>
>> The support for hybrid NAND/NOR chips would not be available until the
>> spi-flash layer is done, right?
> 
> Exactly.
> 
> Regards,
> 
> Boris
> 

Thanks and regards,

Frieder