Re: [PATCH] mtd: spinand: Add support for GigaDevice GD5F1GQ4UC

Stefan Roese
2019-01-24T12:28:51Z

On 24.01.19 13:18, Boris Brezillon wrote:

<snip>

>> As it seems, adding "column" to the buffer address is not a good
>> idea. Here a short log with some debugging:
>
> Oh, you're right, buf points to the OOB portion in case of OOB-only
> accesses.
>
>>
>> [ 27.721104] spinand_read_from_cache_op (241): ret=32 nbytes=128 column=2048
>>
>> This small patch below of your last one, seems to fix this issue
>> for me:
>>
>> root@mt7688:~# cat /sys/class/mtd/mtd5/bad_blocks
>> 2
>> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
>> incremental biterrors test
>> Successfully corrected 0 bit errors per subpage
>> Inserted biterror @ 1/7
>> Read reported 4 corrected bit errors
>> Successfully corrected 1 bit errors per subpage
>> Inserted biterror @ 3/7
>> Read reported 4 corrected bit errors
>> Successfully corrected 2 bit errors per subpage
>> Inserted biterror @ 5/7
>> Read reported 4 corrected bit errors
>> Successfully corrected 3 bit errors per subpage
>> Inserted biterror @ 7/7
>> Read reported 4 corrected bit errors
>> Successfully corrected 4 bit errors per subpage
>> Inserted biterror @ 8/7
>> Read reported 5 corrected bit errors
>> Successfully corrected 5 bit errors per subpage
>> Inserted biterror @ 10/7
>> Read reported 6 corrected bit errors
>> Successfully corrected 6 bit errors per subpage
>> Inserted biterror @ 12/7
>> Read reported 7 corrected bit errors
>> Successfully corrected 7 bit errors per subpage
>> Inserted biterror @ 14/7
>> Read reported 8 corrected bit errors
>> Successfully corrected 8 bit errors per subpage
>> Inserted biterror @ 17/7
>> Failed to recover 1 bitflips
>> Read error after 9 bit errors per page
>>
>> What do you think? Could you fold it into your patch if its
>> acceptable?
>
> Actually, I asked Miquel to drop/revert the initial patch, so I'll
> squash whatever makes it work for you in the original commit and send a
> new version.
>
>>
>> Thanks,
>> Stefan
>>
>> --->8---
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 659a0e716ee0..a5ccd4adc30f 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -218,6 +218,7 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>> unsigned int nbytes = 0;
>> void *buf = NULL;
>> u16 column = 0;
>> + int offs = 0;
>> ssize_t ret;
>>
>> if (req->datalen) {
>> @@ -237,7 +238,7 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>> rdesc = spinand->dirmaps[req->pos.plane].rdesc;
>>
>> while (nbytes) {
>> - ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf + column);
>> + ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf + offs);
>> if (ret < 0)
>> return ret;
>>
>> @@ -246,6 +247,7 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>>
>> nbytes -= ret;
>> column += ret;
>> + offs += ret;
>
> How about incrementing buf directly?
>
> buf += ret;

Sure.

>> }
>>
>> if (req->datalen)
>> @@ -273,6 +275,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
>> struct mtd_info *mtd = nanddev_to_mtd(nand);
>> struct spi_mem_dirmap_desc *wdesc;
>> unsigned int nbytes, column = 0;
>> + int offs = 0;
>> ssize_t ret;
>>
>> nbytes = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
>> @@ -297,7 +300,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
>>
>> while (nbytes) {
>> ret = spi_mem_dirmap_write(wdesc, column, nbytes,
>> - spinand->databuf + column);
>> + spinand->databuf + offs);
>
> In the write case 'spinand->databuf + column' works because we always
> write the page entirely, but making the write path consistent with
> the read one is a good thing, so let's add a buf pointer.
>
>> if (ret < 0)
>> return ret;
>>
>> @@ -306,6 +309,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
>>
>> nbytes -= ret;
>> column += ret;
>> + offs += ret;
>> }
>>
>> return 0;
>
> How about the following diff (hopefully the last one :-))?
>
> --->8---
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 52f17fc42daa..98e4ad74df7f 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -238,14 +238,16 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>
> while (nbytes) {
> while (nbytes) {
> ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf);
> - if (!ret || ret > nbytes)
> - ret = -EIO;
> -
> if (ret < 0)
> return ret;
>
> + if (!ret || ret > nbytes)
> + return -EIO;
> +
> +

Double empty line.

> nbytes -= ret;
> column += ret;
> + buf += ret;
> }
>
> if (req->datalen)
> @@ -273,6 +275,7 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
> struct mtd_info *mtd = nanddev_to_mtd(nand);
> struct spi_mem_dirmap_desc *wdesc;
> unsigned int nbytes, column = 0;
> + void *buf = spinand->databuf;
> ssize_t ret;
>
> nbytes = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
> @@ -296,16 +299,16 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
> wdesc = spinand->dirmaps[req->pos.plane].wdesc;
>
> while (nbytes) {
> - ret = spi_mem_dirmap_read(wdesc, column, nbytes,
> - spinand->databuf + column);
> - if (!ret || ret > nbytes)
> - ret = -EIO;
> -
> + ret = spi_mem_dirmap_write(wdesc, column, nbytes, buf);
> if (ret < 0)
> return ret;
>
> + if (!ret || ret > nbytes)
> + return -EIO;
> +
> nbytes -= ret;
> column += ret;
> + buf += ret;
> }
>
> return 0;
> @@ -761,21 +764,6 @@ static void spinand_destroy_dirmaps(struct spinand_device *spinand)
> spinand_destroy_dirmap(spinand, i);
> }
>
> -const struct spi_mem_op *
> -spinand_find_supported_op(struct spinand_device *spinand,
> - const struct spi_mem_op *ops,
> - unsigned int nops)
> -{
> - unsigned int i;
> -
> - for (i = 0; i < nops; i++) {
> - if (spi_mem_supports_op(spinand->spimem, &ops[i]))
> - return &ops[i];
> - }
> -
> - return NULL;
> -}
> -
> static const struct nand_ops spinand_ops = {
> .erase = spinand_erase,
> .markbad = spinand_markbad,
>
>

This one works as well. Feel free to add my:

Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>

tags if appropriate.

Another question though regarding older kernel versions (e.g. v4.19 LTS).
Could you perhaps push your fix [1] from yesterday as well?

Many thanks again for all your help here,
Stefan

[1] http://lists.infradead.org/pipermail/linux-mtd/2019-January/086931.html