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

[Frieder Schrempf]

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?

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

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

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

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

Thanks and regards

Frieder