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

[Peter Pan <peterpansjtu@gmail.com>]

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;

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?

With the two thing fixed, we can pass UBIFS mount and mtd test.

Thanks
Peter Pan

>
> We still need to make commit messages a bit more verbose.
>
> This afternoon I'll have a look at the ->select_die() feature.
>
> Regards,
>
> Boris
>
> [1]https://github.com/bbrezillon/linux-0day/commits/nand/spi-nand