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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 2 Jan 2018 10:51:25 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> Hi Boris,
> 
> 
> On Fri, Dec 22, 2017 at 9:51 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Fri, 22 Dec 2017 14:37:06 +0800
> > Peter Pan <peterpansjtu@gmail.com> wrote:
> >  
> >> Hi Boris and Frieder
> >>
> >> On Fri, Dec 22, 2017 at 8:49 AM, Peter Pan <peterpansjtu@gmail.com> wrote:  
> >> > Hi Frieder,
> >> >
> >> > On Thu, Dec 21, 2017 at 7:48 PM, Frieder Schrempf
> >> > <frieder.schrempf@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.

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