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

From: Boris Brezillon <bbrezillon@kernel.org>
To: Stefan Roese <sr@denx.de>
Cc: linux-mtd@lists.infradead.org,
	Chuanhong Guo <gch981213@gmail.com>,
	Frieder Schrempf <frieder.schrempf@kontron.de>,
	Miquel Raynal <miquel.raynal@bootlin.com>
Subject: Re: [PATCH] mtd: spinand: Add support for GigaDevice GD5F1GQ4UC
Date: Thu, 24 Jan 2019 12:14:35 +0100	[thread overview]
Message-ID: <20190124121435.4ed6069d@bbrezillon> (raw)
In-Reply-To: <29f49243-6b52-5c98-29a6-81f9a32697b9@denx.de>

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,


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