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

Boris Brezillon
2019-01-24T09:19:18Z

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

> On 24.01.19 09:14, Boris Brezillon wrote:
> > On Thu, 24 Jan 2019 09:00:43 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >
> >> On 24.01.19 08:50, Boris Brezillon wrote:
> >>> On Thu, 24 Jan 2019 08:35:32 +0100
> >>> Stefan Roese <sr@denx.de> wrote:
> >>>
> >>>> On 23.01.19 13:57, Boris Brezillon wrote:
> >>>>> On Wed, 23 Jan 2019 13:40:50 +0100
> >>>>> Boris Brezillon <bbrezillon@kernel.org> 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,