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

Stefan Roese
2019-01-24T12:00:10Z

On 24.01.19 12:14, Boris Brezillon wrote:
> On Thu, 24 Jan 2019 11:57:33 +0100
> Stefan Roese <sr@denx.de> wrote:
>
>> On 24.01.19 10:19, Boris Brezillon wrote:
>>
>> <snip>
>>
>>>> So spi_mem_dirmap_read() returns -EINVAL to spinand_write_to_cache_op()
>>>> which then returns -EIO.
>>>
>>> Can you try with the following diff applied?
>>> --->8---
>>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>>> index 52f17fc42daa..67c568f0c47f 100644
>>> --- a/drivers/mtd/nand/spi/core.c
>>> +++ b/drivers/mtd/nand/spi/core.c
>>> @@ -238,7 +238,7 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>>>
>>> while (nbytes) {
>>> ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf);
>>> - if (!ret || ret > nbytes)
>>> + if (!ret || (ret > 0 && ret > nbytes))
>>> ret = -EIO;
>>>
>>> if (ret < 0)
>>> @@ -296,9 +296,9 @@ 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 = spi_mem_dirmap_write(wdesc, column, nbytes,
>>> + spinand->databuf + column);
>>> + if (!ret || (ret > 0 && ret > nbytes))
>>> ret = -EIO;
>>>
>>> if (ret < 0)
>>> @@ -761,21 +761,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,
>>>
>>
>> Sure. Here some logs:
>>
>> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
>> incremental biterrors test
>> ECC failure, invalid data despite read success
>
> Okay, so now we're back to a problem in the read path. Looks like the
> buf pointer passed to spi_mem_dirmap_read() is only valid for the first
> iteration of the "while (nbytes)" loop.
>
> Can you try with this diff (and sorry for asking you to debug/test that,
> but I no longer have access to a device with a SPI NAND)?
>
> --->8---
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 52f17fc42daa..90da21b52bb5 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -237,13 +237,14 @@ 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);
> - if (!ret || ret > nbytes)
> - ret = -EIO;
> -
> + ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf + column);
> if (ret < 0)
> return ret;
>
> + if (!ret || ret > nbytes)
> + return -EIO;
> +
> +
> nbytes -= ret;
> column += ret;
> }
> @@ -296,14 +297,14 @@ 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,
> + spinand->databuf + column);
> if (ret < 0)
> return ret;
>
> + if (!ret || ret > nbytes)
> + return -EIO;
> +
> nbytes -= ret;
> column += ret;
> }
> @@ -761,21 +762,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,
>

Not good at all, I'm afraid:

root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
[ 161.408159] nand: attempt to erase a bad/reserved block @0
libmtd: error!: MEMERASE64 ioctl failed for eraseblock 0 (mtd5)
error 5 (Input/output error)
Cannot erase block 0

root@mt7688:~# cat /sys/class/mtd/mtd5/bad_blocks
992

Without this last patch, I only have 2 bad blocks:

root@mt7688:~# cat /sys/class/mtd/mtd5/bad_blocks
2

As it seems, adding "column" to the buffer address is not a good
idea. Here a short log with some debugging:

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

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

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