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

Boris Brezillon
2019-01-24T12:18:27Z

On Thu, 24 Jan 2019 12:59:56 +0100
Stefan Roese <sr@denx.de> wrote:

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

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;

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