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

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

On Fri, 15 Dec 2017 10:35:16 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> Hi Boris
> 
> On Thu, Dec 14, 2017 at 3:50 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > 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)  
> 
> I already finished the rebase of your preparation patches on l2-mtd
> nand/next brach (v4.15-rc1). Should I send them as v3 or reset to v1?
> I think we don't need to wait for SPI NAND code. These preparation
> patches should be review ASAP. I'd like to review it after the sending,
> since they are all your effort. What's your opinion?

I just sent them on the ML. Feel free to review.