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

[Peter Pan]

Hi Boris and Frieder

On Thu, Dec 14, 2017 at 11:38 PM, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> On Thu, 14 Dec 2017 15:39:13 +0100
> Frieder Schrempf <frieder.schrempf at 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 think "mtd: Add sanity checks in mtd_write/read_oob()" is also
included since I cannot
find it in nand/next branch of l2-mtd

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

Frieder.

Actually the v5 patchset of SPI NAND framework from me[1] includes on-die ECC.
You can have a reference (maybe patch 2 and patch 4 is enough).
Of course v5 is quite different with the latest code, but I think you
can get some idea from it.

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

I would like to thank you for your help in advance, Frieder.


Thanks,
Peter Pan

[1] http://patchwork.ozlabs.org/project/linux-mtd/list/?series=&submitter=65489&state=9&q=v5&archive=&delegate=

>
> Thanks,
>
> Boris