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

[Boris Brezillon <bbrezillon@xxxxxxxxxx>]

On Thu, 24 Jan 2019 09:52:28 +0100
Stefan Roese <sr@xxxxxxx> wrote:

> On 24.01.19 09:14, Boris Brezillon wrote:
> > On Thu, 24 Jan 2019 09:00:43 +0100
> > Stefan Roese <sr@xxxxxxx> wrote:
> >   
> >> On 24.01.19 08:50, Boris Brezillon wrote:  
> >>> On Thu, 24 Jan 2019 08:35:32 +0100
> >>> Stefan Roese <sr@xxxxxxx> wrote:
> >>>      
> >>>> On 23.01.19 13:57, Boris Brezillon wrote:  
> >>>>> On Wed, 23 Jan 2019 13:40:50 +0100
> >>>>> Boris Brezillon <bbrezillon@xxxxxxxxxx> wrote:
> >>>>>         
> >>>>>>> This definitely does look better. I assume that we are we on the
> >>>>>>> right track now?  
> >>>>>>
> >>>>>> Yep, and it confirms the ECC caps => 8bits/512bytes. Will send a proper
> >>>>>> commit for the fix I did and Cc you so you can add your
> >>>>>> Tested-by/Reviewed-by.  
> >>>>>
> >>>>> Oh, looks like a side-effect of migrating to the dirmap approach
> >>>>> (merged in nand/next [1]) is that this bug does not exist. Can you test
> >>>>> the nand/next branch and let me know if it still works?
> >>>>>
> >>>>> [1]http://git.infradead.org/linux-mtd.git/shortlog/refs/heads/nand/next  
> >>>>
> >>>> Unfortunately this does not seem to work. I was unable to boot my
> >>>> platform from this branch directly so I rebased all MTD/NAND related
> >>>> patches on top of the latest kernel.org tree for this.  
> >>>
> >>> You mean linux-next?  
> >>
> >> No. I can try linux-next as well if necessary.  
> > 
> > So which branch/tag is it based on?  
> 
> Linus's tree "master" (5.0.0-rc3) with some mostly platform
> patches applied on top.
> 
>  From your other mail:
> 
> > Can you find out which layer (spinand, spi-mem or the spi driver) is
> > returning this -EIO?  
> 
> Sure. With this small debug patch applied:
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 52f17fc42daa..80fa234ecbdd 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -298,9 +298,11 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
>          while (nbytes) {
>                  ret = spi_mem_dirmap_read(wdesc, column, nbytes,
>                                            spinand->databuf + column);
> +               printk("%s (%d): ret=%d nbytes=%d\n", __func__, __LINE__, ret, nbytes); // test-only
>                  if (!ret || ret > nbytes)
>                          ret = -EIO;
>   
> +               printk("%s (%d): ret=%d nbytes=%d\n", __func__, __LINE__, ret, nbytes); // test-only
>                  if (ret < 0)
>                          return ret;
>   
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 5217a5628be2..964ba3dc4e64 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -573,8 +573,10 @@ ssize_t spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
>          struct spi_controller *ctlr = desc->mem->spi->controller;
>          ssize_t ret;
>   
> +       printk("%s (%d)\n", __func__, __LINE__); // test-only
>          if (desc->info.op_tmpl.data.dir != SPI_MEM_DATA_IN)
>                  return -EINVAL;
> +       printk("%s (%d)\n", __func__, __LINE__); // test-only
>   
>          if (!len)
>                  return 0;
> 
> 
> I get this output:
> 
> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> incremental bite[   66.598843] spi_mem_dirmap_read (576)
> rrors test
> [   66.603779] spinand_write_to_cache_op (301): ret=-22 nbytes=2176
> [   66.610912] spinand_write_to_cache_op (305): ret=-5 nbytes=2176
> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
>          error 5 (Input/output error)
> Failed to write page 0 in block 0
> ERROR: 1 | root@mt7688:~# dmesg -c
> [   66.598843] spi_mem_dirmap_read (576)
> [   66.603779] spinand_write_to_cache_op (301): ret=-22 nbytes=2176
> [   66.610912] spinand_write_to_cache_op (305): ret=-5 nbytes=2176
> 
> 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,

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