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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Peter,

On Thu, 14 Dec 2017 14:15:57 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> Hi Boris and Frieder,
> 
> On Thu, Dec 14, 2017 at 5:27 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Frieder,
> >
> > On Tue, 12 Dec 2017 10:58:10 +0100
> > Frieder Schrempf <frieder.schrempf@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.

Actually, I'm trying to move away from this ->select_chip() approach in
the raw NAND framework, simply because the controller might be able to
parallelize operations (like 2 erase on 2 different dies, or one erase
on one die, and a program on the other), and having this ->select_chip()
done early in the chain prevents this kind of optimization.

Anyway, the controller should now have all the necessary information to
know which die an operation should be executed on, and if a specific
command has to be sent to the device to select a specific die, it can
be done in the NAND vendor specific code.

> 
> >  
> >>
> >> * 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.

Yep.

> 
> >
> > 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.

Then why are they not spending time reviewing the SPI NAND framework?
You know I'm not paid to work on SPI NAND, unlike those engineers
working for SoC/NAND vendors. While I agree being a maintainer implies
doing part of the work on our spare time, what I don't like is when
people that did not invest time in a specific feature ask for this
feature to be implemented/released. This is not your case, you invested
a lot of time in it, and that's the reason I feel bad when I can't spend
the necessary time to make SPI NAND code ready for inclusion.

So here are the next set of things to do if you want to move forward:
1/ Re-submit the preparation patches (those touching MTD core) and
   review them (or find someone to review them)
2/ Add the missing doc to the code (I was planning on doing that, but
   if you're in hurry you can take care of it), and add real commit
   messages.
3/ Fix the authorship on patches (some were mainly written by you, but
   during my rework authorship has been lost)
4/ Ask others to test and review the patches

Regards,

Boris