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

[Boris Brezillon]

On Thu, 4 Jan 2018 10:01:57 +0800
Peter Pan <peterpansjtu at gmail.com> wrote:

> Boris,
> 
> On Thu, Jan 4, 2018 at 12:46 AM, Boris Brezillon
> <boris.brezillon at free-electrons.com> wrote:
> > On Tue, 2 Jan 2018 10:51:25 +0800
> > Peter Pan <peterpansjtu at gmail.com> wrote:
> >  
> >> Hi Boris,
> >>
> >>
> >> On Fri, Dec 22, 2017 at 9:51 PM, Boris Brezillon
> >> <boris.brezillon at free-electrons.com> wrote:  
> >> > On Fri, 22 Dec 2017 14:37:06 +0800
> >> > Peter Pan <peterpansjtu at gmail.com> wrote:
> >> >  
> >> >> Hi Boris and Frieder
> >> >>
> >> >> On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu at gmail.com> wrote:  
> >> >> > Hi Frieder,
> >> >> >
> >> >> > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf
> >> >> > <frieder.schrempf at exceet.de> 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.  
> 
> Noted.
> 
> >  
> >>
> >> 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).  
> 
> OK.
> 
> >  
> >>
> >> With the two thing fixed, we can pass UBIFS mount and mtd test.  
> >
> > Do you still have the problem you reported earlier (OOB test)?  
> 
> I forgot to reply that mail :(. Actually there is no problem because
> mtd_write_oob() already has mtd_check_oob_ops() to check it.
> That failure in OOB test occurs because mtd->oobavail is zero.

I reconsidered what I said earlier, and I think it's safer to set
->oobavail to 0 are for now, otherwise we might end up with setups that
start using some of it and we'll break things as soon as we'll add
support for ECC.