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

[Boris Brezillon <bbrezillon@xxxxxxxxxx>]

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

> On 24.01.19 12:14, Boris Brezillon wrote:
> > On Thu, 24 Jan 2019 11:57:33 +0100
> > Stefan Roese <sr@xxxxxxx> 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,



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/