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,

					^write(

>                                            spinand->databuf + column);


> +               printk("%s (%d): ret=%d nbytes=%d\n", __func__, __LINE__, ret, nbytes); // test-only
>                  if (!ret || ret > nbytes)

Looks like this comparison between signed and unsigned int is broken,
because you start with -EINVAL...

>                          ret = -EIO;
>   
> +               printk("%s (%d): ret=%d nbytes=%d\n", __func__, __LINE__, ret, nbytes); // test-only

and end up with -EIO here.

>                  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.
> 
> Thanks,
> Stefan


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