From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.free-electrons.com ([62.4.15.54]) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1eWmBa-0003if-JB for linux-mtd@lists.infradead.org; Wed, 03 Jan 2018 16:46:41 +0000 Date: Wed, 3 Jan 2018 17:46:14 +0100 From: Boris Brezillon To: Peter Pan Cc: Frieder Schrempf , "Peter Pan =?UTF-8?B?5r2Y5qCL?= (peterpandong)" , "linux-mtd@lists.infradead.org" Subject: Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework Message-ID: <20180103174614.6d760b3d@bbrezillon> In-Reply-To: References: <20170529225931.226da943@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> <20171214163848.41f6d277@bbrezillon> <20171222145147.3697fb02@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Tue, 2 Jan 2018 10:51:25 +0800 Peter Pan wrote: > Hi Boris, > > > On Fri, Dec 22, 2017 at 9:51 PM, Boris Brezillon > wrote: > > On Fri, 22 Dec 2017 14:37:06 +0800 > > Peter Pan wrote: > > > >> Hi Boris and Frieder > >> > >> On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan wrote: > >> > Hi Frieder, > >> > > >> > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf > >> > wrote: > >> >> Hello Boris, > >> >> > >> >>>>>> 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. > >> >> > >> >> > >> >> I added an op to send the die selection command and call it, where I think > >> >> it is needed: [1] > >> > > >> > I think we should add die_select_op in vendor's file and call a hook in core.c > >> > since the die select command implementation is different by vendors. > >> > > >> > Thanks > >> > Peter Pan > >> > > >> > > >> >> > >> >> Accessing both dies on the Winbond SPI NAND works fine like this. > >> >> > >> >> While running tests I came across some problems: > >> >> > >> >> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, the > >> >> calculation of position and offset seems to be wrong. > >> >> My fix is here: [2] > >> >> > >> >> * On calculating the row address for read/program/erase via > >> >> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong. > >> >> > >> >> * Also, I'm not sure if the LUN should be taken into account when > >> >> calculating the row address. At least when you select the LUN by issuing a > >> >> separate command, the row address sent to the chip should only contain the > >> >> page address. > >> >> But I'm not sure if that's the case in general, or if some code is needed to > >> >> differentiate. > >> >> > >> >> See my changes of nanddev_pos_to_row here: [3] > >> >> > >> >> * I run into a mutex deadlock, when spinand_write_page fails (e.g. because > >> >> of a bad block) as the lock is not released in such cases. See my fix here: > >> >> [4] > >> >> > >> >> With these fixes applied and as far as I can tell everything seems to work > >> >> fine. I'll do some tests with UBI next and look into the ECC topic. > >> > >> UBI attach failed since missing mtd->writebufsize assignment in nanddev_init() > >> After fixing it and with Frieder's fixing, UBIFS can be mounted successfully on > >> Zedboard with generic spi controller and Micron SPI NAND 2Gb device. > >> > >> Also, the code now can pass mtd read and page test. 1 error found in oob test > >> since we don't have "past end of partition" check in part_write_oob() which I > >> mentioned in earlier mail. Since we don't support ECC right now, I didn't try > >> nandbiterror test. > >> > > > > Peter, Frider, I just pushed a new version to my nand/spi-nand branch > > [1] fixing the authorship, adding/fixing/removing comments where needed. > > > > Can you please have a look at those changes (everything I changed > > should appear as !fixup commits) and let me know if I did something > > wrong. > > You forgot to initialize mtd->writebufsize and mtd->oobavail. > For mtd->writebufsize I think we can do it in nanddev_init() like below: > > --- a/drivers/mtd/nand/core.c > +++ b/drivers/mtd/nand/core.c > @@ -196,7 +196,9 @@ int nanddev_init(struct nand_device *nand, const > struct nand_ops *ops, > mtd->flags = MTD_CAP_NANDFLASH; > mtd->erasesize = memorg->pagesize * memorg->pages_per_eraseblock; > mtd->writesize = memorg->pagesize; > + mtd->writebufsize = mtd->writesize; > mtd->oobsize = memorg->oobsize; > mtd->size = nanddev_size(nand); > mtd->owner = owner; I fixed it already, see [1]. Note that I haven't squashed fixup commits yet. > > For mtd->oobavail, I think we need to call some hooks in spinand_init(), > and implement on-die ECC layout in vendor's file and HW ECC layout in > controller's file. > > What's your opinion? Hm, I'm not so sure. spinand_init() should be called just before registering the MTD device, so by that time everything should have been setup correctly by the upper layer. This being said, I'd like to keep a clear separation between the MTD and NAND layers, and this means preventing any NAND sub-layer from directly manipulating the MTD device and its fields. Really, I don't know yet how I want to abstract ECC stuff, so let's keep it simple for now and expose the whole OOB area (I'll fix the core to do that if you're okay). > > With the two thing fixed, we can pass UBIFS mount and mtd test. Do you still have the problem you reported earlier (OOB test)? Thanks, Boris [1]https://github.com/bbrezillon/linux-0day/commit/32eeea1c35bfdc2ac0fcea599f6a926c94b835bb