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

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

On Thu, 14 Dec 2017 15:39:13 +0100
Frieder Schrempf <frieder.schrempf@exceet.de> wrote:

> Hello Peter, hello Boris
> 
> >>>>> 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.  
> 
> I will try to rebase on 4.15-rc1. Can you give me a quick explanation on 
> which patches are needed for SPI NAND + preparation, if I only want to 
> add the generic NAND and the SPI NAND layer?
> Previously I just rebased the whole branch, but I think you have some 
> other pending patches in there?
> Would this be the correct changeset: [1]
> Or are there other preparation patches needed?

Looks good. By preparation patches I meant

from:
"mtd: Do not allow MTD devices with inconsistent erase properties"
to
"mtd: nand: move raw NAND related code to the raw/ subdir"

so basically everything that touches existing code.

> 
> >>>>> 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.  
> 
> Yeah I meant Winbond of course. And I guess you meant "more confident". 

Yep.

> Or that was just irony? Nevermind ;)
> 
> >>>>> 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.  
> 
> The die selection is a separate SPI command and not integrated into the 
> program/read/erase sequence.
> So I can't customize an existing op, but have to issue a new "die 
> select" op.

Okay.

> 
> >>>
> >>> 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.  
> 
> But needing a die select op is quite common for any multi-die chips, 
> isn't it?

It is, it's just that some controllers are able to interleave
operations on multiple dies, so having  ->{select,unselect}_chip()
methods at the generic NAND level is not such a good idea, because that
means you'll have to serialize accesses, even if they could be done in
parallel.

> So shouldn't there be an spinand_die_select_op in the SPI NAND core that 
> is executed when necessary and skipped if there's only one die.

Sure, I was arguing against a ->select_chip() at the generic NAND
level. That's something you can add in the SPI NAND framework.

> 
> > Got your point. It makes sense.  
> >>  
> >>>>> * 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.  
> 
> Ok, I might try this and see where it leads me.
> 
> >>>>> * 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.  
> 
> Ok. If I have time I will think about how the ECC and OOB layout might 
> be implemented. But I have not much experience here, so if anyone of you 
> can propose something to get started, this would be great.

I can definitely help there, and Peter should be able to give some
insights as well.

> 
> >>>> 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 :-/).  
> 
> I hope, that as more people get interested in the topic, more people 
> will join. Thanks in advance for your further work on this.
> 
> >> 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  
> > 
> > To be honest, I also feel bad to keep pushing you on this...
> > I will try to communicate with them to see if they can help us to do some review
> > or valudation task.
> > You already make the path clear, I will take the rest. If anything
> > unclear, I will come
> > back to discuss with you.  
> 
> I don't have any experience in reviewing kernel code and only little in 
> submitting patches, but if I can be of any help let me know.
> 
> As I have a certain use case and the hardware, I will also be happy to 
> help testing.

Testing and reporting issues is already helpful.

Thanks,

Boris