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

[Peter Pan]

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.

Thanks
Peter Pan

>
> Thanks,
>
> Boris
>
> [1]https://github.com/bbrezillon/linux-0day/commit/32eeea1c35bfdc2ac0fcea599f6a926c94b835bb
>