From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.exceet.ch ([77.245.33.226]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1ePUjY-00058N-To for linux-mtd@lists.infradead.org; Thu, 14 Dec 2017 14:43:43 +0000 Subject: Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework From: Frieder Schrempf To: Peter Pan , Boris Brezillon CC: =?UTF-8?B?UGV0ZXIgUGFuIOa9mOagiyAocGV0ZXJwYW5kb25nKQ==?= , "linux-mtd@lists.infradead.org" References: <20170529225931.226da943@bbrezillon> <90bcb366-c494-2dee-3bb7-d16b77e347c6@exceet.de> <20171204150514.1f17671d@bbrezillon> <20171205135804.478902dc@bbrezillon> <20171205140313.40f3f17d@bbrezillon> <256b4ab6-c104-4580-87cd-527178bde460@exceet.de> <20171213222710.32d12514@bbrezillon> <20171214085036.436b4fcf@bbrezillon> <23d17416-e555-4b92-cc47-f644fbe676c6@exceet.de> Message-ID: <6e8ab15d-85a6-b84c-41c5-dfad882348db@exceet.de> Date: Thu, 14 Dec 2017 15:43:13 +0100 MIME-Version: 1.0 In-Reply-To: <23d17416-e555-4b92-cc47-f644fbe676c6@exceet.de> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Forgot the link: [1]: https://github.com/fschrempf/linux-0day/compare/374c97f3a6b8e83dd1b005a3aec64d7008259185...fschrempf:spi-nand-exceet On 14.12.2017 15:39, Frieder Schrempf 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? > >>>>>> 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