[PATCH v6 00/15] A SPI NAND framework under generic NAND framework

[Peter Pan]

Hi Boris and Frieder,

On Thu, Dec 14, 2017 at 5:27 AM, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> Hi Frieder,
>
> On Tue, 12 Dec 2017 10:58:10 +0100
> Frieder Schrempf <frieder.schrempf at exceet.de> wrote:
>
>> Hi Boris,
>>
>> I spent some time looking at and working with your latest SPI NAND code.
>> In my previous mail I said, that we have some working code for our 3.14
>> kernel based on the "mt29f_spinand" staging driver, but that was wrong
>> as I got things mixed up in my head.
>> Actually our codebase was an early version of Peter's code, so the
>> effort to port it to the latest framework code was not that big.
>>
>> I forked your repo and you can find my working tree at [1].
>
> Great, I'll try to have a look.
>
>>
>> What I did so far:
>>
>> * Rebase your patches on latest Linux 4.14.5
>
> Cool, rebasing on 4.15-rc1 would be even better, but I can do that if
> you don't have the time.
>
>>
>> * Add a driver for the Winbond W25M02GV SPI NAND chip, that we have on
>> some of our boards.
>
> Okay, that's actually a good thing to have tested with another chip,
> This way we can make sure the framework is generic enough.
>
>>
>> * Add a driver for the Freescale QSPI controller, derived from the
>> existing QSPI-NOR driver at drivers/mtd/spi-nor/fsl-quadspi.c.
>
> That's an interesting case as well, the generic NAND controller is
> probably the easiest implementation, and that's a good thing to have
> another controller.
>
>>
>> * Add a setup and setup_late op to the controller layer (see [3]). I
>> don't know if that makes sense, but at least the setup_late is needed
>> for the FSL QSPI driver, as it needs to know the size of the flash to
>> setup the memory mapping for the QSPI read operations.
>
> Hm, not sure what ->setup_late() is for, but I'll have a look.
>
>>
>> * A bit of testing and fixing two small bugs, which look like copy and
>> paste mistakes. See [2].
>
> Thanks, I'll squash the fixes in the original commit.
>
>>
>> * Running nandtest successfully on our hardware (i.MX6UL -> FSL-QSPI ->
>> W25M02GV)
>>
>> I hope this is of use while moving on.
>
> Definitely. I'd also like to have a review on the framework code if
> possible, but that can be done when posted on the ML.
>
>> I guess you would like to have the basic framework with Micron support
>> and generic SPI tested and stabilized first, before adding more code,
>> but to be able to test with our hardware I need Micron and FSL QSPI.
>
> I guess you mean Winbond not Micron. That's okay if those patches are
> posted after the initial series. All I need is reviews and tests from
> different parties, so that I'm less confident in merging the code.
>
>>
>> Some questions/flaws that occurred to me:
>>
>> * The W25M02GV chip has two dies of 128M each. My current driver only
>> makes use of the first die. The chip expects a die-select command to
>> switch between the two dies.
>> What would be the place to implement this?
>> Can I just add the command and issue it in
>> winbond_spinand_adjust_cache_op if luns_per_target > 1?
>
> It really depends when the die selection happens. I guess it happens in
> 2 places: when preparing a program/read operation and when doing the
> transfer to the in-chip cache. Anyway, the die information is already
> passed in the nand_pos object, so all you'll have to do is create a new
> hook to customize the SPI command when needed.

I don't know whether we can do the the die switch in the generic NAND core
(drivers/mtd/nand/core.c). I'm not sure if chip->select_chip() in nand_base.c
does the same thing or not. If so, we can do the die switch before
read/write/erase
operation. And let spi nand core to implement a hook to support it.

>
>>
>> * The FSL QSPI controller has a lookup table that needs to be filled
>> with command sequences at the time of setup. Therefore the number of
>> dummy bytes for each command is fixed and in my current implementation
>> op->dummy_bytes is ignored.
>> That's not a problem if all chips need the same number of dummy bytes
>> for each command, but I guess that's not the case, as there is a
>> _spinand_get_dummy function.
>
> We should definitely expose that in a generic way, and on a
> per-operation basis, so that, after the detection step, the NAND
> controller can query this information.
>
>>
>> * What is your plan for ECC and BBT? At least enabling the onchip ECC
>> will be necessary to be able to use the flash properly (e.g. with UBI),
>> or am I wrong?
>
> Well, if all SPI NAND chips are supporting ECC the same way and
> on-die ECC support is mandatory for SPI NANDs, then supporting that
> directly in the core is probably the best option. If, on the other
> hand, you have various implementations, and some SPI controllers have
> their own ECC engine that you can use with SPI NANDs, then it's
> probably a better idea to abstract ECC engine in the generic NAND layer.

As far as I know, all the SPI NAND supports on-die ECC (at least all
the SPI NAND
I heard). But different chips may have different ECC layout in OOB
area. But I think
this cannot be a problem, we can handle this in vendor's file.

>
> For BBTs, I'd like to have a clean version of the nand_bbt logic that
> uses all of the helpers exposed by the generic NAND layer. I'd also
> like to simplify the code if possible.
>
>>
>> * Do you have any special test cases? What do you usually do to test the
>> code?
>
> Well, make sure all the mtd tests are passing (drivers/mtd/tests), and
> then, the next step is to test it with UBI+UBIFS. But honestly, I'm not
> so worried, this is new code, and we've isolated from the raw NAND
> layer, so if it's buggy or instable at the beginning it's not a big
> deal, it will be noticed and fixed for the next few releases.
>
>>
>> Thanks for your patience and best regards,
>
> No problem. Thanks for your work, and I'll try to be more active on
> this topic (I promised that several times, and failed it :-/).

Boris,

I think we need to move forward, MediaTek's engineer has already asked for
the status of the spi nand framework upstreaming, since they want to upstream
their SPI NAND controller driver. And Hisilicon also has a controller driver.
Since more and more platforms start to support SPI NAND, we'd better merge a
simple but well designed framework first. I will run your modified
version on Zedboard
with generic SPI controller this or next week.

Thanks,
Peter Pan

>
> Regards,
>
> Boris