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

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

[Stefan Roese]

Add support for GigaDevice GD5F1GQ4UC SPI NAND chip.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Chuanhong Guo <gch981213@gmail.com>
Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
---
Frankly, I'm a bit unsure, if these new ooblayout_foo functions are 
needed for this device. I was unable to find a datasheet for the
already supported devices (GD5F1G/2G/4GQ4xA), so I couldn't compare
the OOB area values here with the ones for my SPI NAND chip.

I'm also not 100% sure, if I should use NAND_ECCREQ(8, 2048) or
NAND_ECCREQ(8, 512) for this device.

So comments welcome.

Thanks,
Stefan

 drivers/mtd/nand/spi/gigadevice.c | 39 +++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/mtd/nand/spi/gigadevice.c b/drivers/mtd/nand/spi/gigadevice.c
index e4141c20947a..a9d256fa2577 100644
--- a/drivers/mtd/nand/spi/gigadevice.c
+++ b/drivers/mtd/nand/spi/gigadevice.c
@@ -57,6 +57,31 @@ static int gd5fxgq4xa_ooblayout_free(struct mtd_info *mtd, int section,
 	return 0;
 }
 
+static int gd5f1gq4u_ooblayout_ecc(struct mtd_info *mtd, int section,
+				   struct mtd_oob_region *region)
+{
+	if (section)
+		return -ERANGE;
+
+	region->offset = 64;
+	region->length = 64;
+
+	return 0;
+}
+
+static int gd5f1gq4u_ooblayout_free(struct mtd_info *mtd, int section,
+				    struct mtd_oob_region *region)
+{
+	if (section)
+		return -ERANGE;
+
+	/* Reserve 2 bytes for the BBM. */
+	region->offset = 2;
+	region->length = 62;
+
+	return 0;
+}
+
 static int gd5fxgq4xa_ecc_get_status(struct spinand_device *spinand,
 					 u8 status)
 {
@@ -86,6 +111,11 @@ static const struct mtd_ooblayout_ops gd5fxgq4xa_ooblayout = {
 	.free = gd5fxgq4xa_ooblayout_free,
 };
 
+static const struct mtd_ooblayout_ops gd5f1gq4u_ooblayout = {
+	.ecc = gd5f1gq4u_ooblayout_ecc,
+	.free = gd5f1gq4u_ooblayout_free,
+};
+
 static const struct spinand_info gigadevice_spinand_table[] = {
 	SPINAND_INFO("GD5F1GQ4xA", 0xF1,
 		     NAND_MEMORG(1, 2048, 64, 64, 1024, 1, 1, 1),
@@ -114,6 +144,15 @@ static const struct spinand_info gigadevice_spinand_table[] = {
 		     0,
 		     SPINAND_ECCINFO(&gd5fxgq4xa_ooblayout,
 				     gd5fxgq4xa_ecc_get_status)),
+	SPINAND_INFO("GD5F1GQ4UC", 0xd1,
+		     NAND_MEMORG(1, 2048, 128, 64, 1024, 1, 1, 1),
+		     NAND_ECCREQ(8, 2048),
+		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
+					      &write_cache_variants,
+					      &update_cache_variants),
+		     0,
+		     SPINAND_ECCINFO(&gd5f1gq4u_ooblayout,
+				     gd5fxgq4xa_ecc_get_status)),
 };
 
 static int gigadevice_spinand_detect(struct spinand_device *spinand)
-- 
2.20.1


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

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

[Boris Brezillon]

On Tue, 22 Jan 2019 15:56:32 +0100
Stefan Roese <sr@denx.de> wrote:

> Add support for GigaDevice GD5F1GQ4UC SPI NAND chip.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Chuanhong Guo <gch981213@gmail.com>
> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>

You forgot to Cc me on this one ;-).

> ---
> Frankly, I'm a bit unsure, if these new ooblayout_foo functions are 
> needed for this device. I was unable to find a datasheet for the
> already supported devices (GD5F1G/2G/4GQ4xA), so I couldn't compare
> the OOB area values here with the ones for my SPI NAND chip.

Looks like it's using a different layout.

> 
> I'm also not 100% sure, if I should use NAND_ECCREQ(8, 2048) or
> NAND_ECCREQ(8, 512) for this device.

Given the size reserved to store ECC bytes I'd say 8bits/512bytes.
There's an easy way to validate that => nandbiterrs (in mtd-utils).

> 
> So comments welcome.
> 
> Thanks,
> Stefan
> 
>  drivers/mtd/nand/spi/gigadevice.c | 39 +++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/gigadevice.c b/drivers/mtd/nand/spi/gigadevice.c
> index e4141c20947a..a9d256fa2577 100644
> --- a/drivers/mtd/nand/spi/gigadevice.c
> +++ b/drivers/mtd/nand/spi/gigadevice.c
> @@ -57,6 +57,31 @@ static int gd5fxgq4xa_ooblayout_free(struct mtd_info *mtd, int section,
>  	return 0;
>  }
>  
> +static int gd5f1gq4u_ooblayout_ecc(struct mtd_info *mtd, int section,
> +				   struct mtd_oob_region *region)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	region->offset = 64;
> +	region->length = 64;
> +
> +	return 0;
> +}
> +
> +static int gd5f1gq4u_ooblayout_free(struct mtd_info *mtd, int section,
> +				    struct mtd_oob_region *region)
> +{
> +	if (section)
> +		return -ERANGE;
> +
> +	/* Reserve 2 bytes for the BBM. */
> +	region->offset = 2;

According to the datasheet, the BBM is only one byte large.

> +	region->length = 62;
> +
> +	return 0;
> +}
> +
>  static int gd5fxgq4xa_ecc_get_status(struct spinand_device *spinand,
>  					 u8 status)
>  {
> @@ -86,6 +111,11 @@ static const struct mtd_ooblayout_ops gd5fxgq4xa_ooblayout = {
>  	.free = gd5fxgq4xa_ooblayout_free,
>  };
>  
> +static const struct mtd_ooblayout_ops gd5f1gq4u_ooblayout = {
> +	.ecc = gd5f1gq4u_ooblayout_ecc,
> +	.free = gd5f1gq4u_ooblayout_free,
> +};
> +
>  static const struct spinand_info gigadevice_spinand_table[] = {
>  	SPINAND_INFO("GD5F1GQ4xA", 0xF1,
>  		     NAND_MEMORG(1, 2048, 64, 64, 1024, 1, 1, 1),
> @@ -114,6 +144,15 @@ static const struct spinand_info gigadevice_spinand_table[] = {
>  		     0,
>  		     SPINAND_ECCINFO(&gd5fxgq4xa_ooblayout,
>  				     gd5fxgq4xa_ecc_get_status)),
> +	SPINAND_INFO("GD5F1GQ4UC", 0xd1,
> +		     NAND_MEMORG(1, 2048, 128, 64, 1024, 1, 1, 1),
> +		     NAND_ECCREQ(8, 2048),
> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> +					      &write_cache_variants,
> +					      &update_cache_variants),
> +		     0,
> +		     SPINAND_ECCINFO(&gd5f1gq4u_ooblayout,
> +				     gd5fxgq4xa_ecc_get_status)),

The gd5fxgq4xa_ecc_get_status() func does not work for this chip.
Something like that should do the trick:

#define GD5F1GQ4U_ECC_STATUS_MASK	GENMASK(6, 4)

static int gd5f1gq4u_ecc_get_status(struct spinand_device *spinand,
				    u8 status)
{
	unsigned int nbitflips;

	nbitflips = (status & GD5F1GQ4U_ECC_STATUS_MASK) >> 4;
	if (!nbitflips)
		return 0;

	nbitflips += 2;
	if (nbitflips > 8)
		return -EBADMSG;

	return nbitflips
}

>  };
>  
>  static int gigadevice_spinand_detect(struct spinand_device *spinand)


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

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

[Boris Brezillon]

On Tue, 22 Jan 2019 15:56:32 +0100
Stefan Roese <sr@denx.de> wrote:

> Add support for GigaDevice GD5F1GQ4UC SPI NAND chip.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Chuanhong Guo <gch981213@gmail.com>
> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> Frankly, I'm a bit unsure, if these new ooblayout_foo functions are 
> needed for this device. I was unable to find a datasheet for the
> already supported devices (GD5F1G/2G/4GQ4xA), so I couldn't compare
> the OOB area values here with the ones for my SPI NAND chip.
> 
> I'm also not 100% sure, if I should use NAND_ECCREQ(8, 2048) or
> NAND_ECCREQ(8, 512) for this device.
> 
> So comments welcome.
> 
> Thanks,
> Stefan
> 
>  drivers/mtd/nand/spi/gigadevice.c | 39 +++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/gigadevice.c b/drivers/mtd/nand/spi/gigadevice.c
> index e4141c20947a..a9d256fa2577 100644
> --- a/drivers/mtd/nand/spi/gigadevice.c
> +++ b/drivers/mtd/nand/spi/gigadevice.c
> @@ -57,6 +57,31 @@ static int gd5fxgq4xa_ooblayout_free(struct mtd_info *mtd, int section,
>  	return 0;
>  }
>  
> +static int gd5f1gq4u_ooblayout_ecc(struct mtd_info *mtd, int section,

We should probably prefix all funcs/objs with gd5fxgq4xc: the datasheet
I have is named GD5FxGQ4xC which means it should apply to other
variants of the GD5F1GQ4UC part.

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

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

[Stefan Roese]

On 22.01.19 17:54, Boris Brezillon wrote:
> On Tue, 22 Jan 2019 15:56:32 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> Add support for GigaDevice GD5F1GQ4UC SPI NAND chip.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Chuanhong Guo <gch981213@gmail.com>
>> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> You forgot to Cc me on this one ;-).

Ah, sorry about that.
  
>> ---
>> Frankly, I'm a bit unsure, if these new ooblayout_foo functions are
>> needed for this device. I was unable to find a datasheet for the
>> already supported devices (GD5F1G/2G/4GQ4xA), so I couldn't compare
>> the OOB area values here with the ones for my SPI NAND chip.
> 
> Looks like it's using a different layout.

Thanks. Can you please point me to a datasheet for the
GD5F1G/2G/4GQ4xA?
  
>>
>> I'm also not 100% sure, if I should use NAND_ECCREQ(8, 2048) or
>> NAND_ECCREQ(8, 512) for this device.
> 
> Given the size reserved to store ECC bytes I'd say 8bits/512bytes.
> There's an easy way to validate that => nandbiterrs (in mtd-utils).

How so. Here some output of this test tool:

# ./nandbiterrs /dev/mtd5 -k -o
overwrite biterrors test
Read reported 7 corrected bit errors
Read reported 8 corrected bit errors
Failed to recover 1 bitflips
Bit error histogram (669 operations total):
Page reads with   0 corrected bit errors: 80
Page reads with   1 corrected bit errors: 0
Page reads with   2 corrected bit errors: 0
Page reads with   3 corrected bit errors: 0
Page reads with   4 corrected bit errors: 0
Page reads with   5 corrected bit errors: 0
Page reads with   6 corrected bit errors: 0
Page reads with   7 corrected bit errors: 500

How does this tell me, if it's 8bits/512bytes or some other
value?
  
>>
>> So comments welcome.
>>
>> Thanks,
>> Stefan
>>
>>   drivers/mtd/nand/spi/gigadevice.c | 39 +++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/gigadevice.c b/drivers/mtd/nand/spi/gigadevice.c
>> index e4141c20947a..a9d256fa2577 100644
>> --- a/drivers/mtd/nand/spi/gigadevice.c
>> +++ b/drivers/mtd/nand/spi/gigadevice.c
>> @@ -57,6 +57,31 @@ static int gd5fxgq4xa_ooblayout_free(struct mtd_info *mtd, int section,
>>   	return 0;
>>   }
>>   
>> +static int gd5f1gq4u_ooblayout_ecc(struct mtd_info *mtd, int section,
>> +				   struct mtd_oob_region *region)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	region->offset = 64;
>> +	region->length = 64;
>> +
>> +	return 0;
>> +}
>> +
>> +static int gd5f1gq4u_ooblayout_free(struct mtd_info *mtd, int section,
>> +				    struct mtd_oob_region *region)
>> +{
>> +	if (section)
>> +		return -ERANGE;
>> +
>> +	/* Reserve 2 bytes for the BBM. */
>> +	region->offset = 2;
> 
> According to the datasheet, the BBM is only one byte large.

Yes, will change in v2.

> 
>> +	region->length = 62;
>> +
>> +	return 0;
>> +}
>> +
>>   static int gd5fxgq4xa_ecc_get_status(struct spinand_device *spinand,
>>   					 u8 status)
>>   {
>> @@ -86,6 +111,11 @@ static const struct mtd_ooblayout_ops gd5fxgq4xa_ooblayout = {
>>   	.free = gd5fxgq4xa_ooblayout_free,
>>   };
>>   
>> +static const struct mtd_ooblayout_ops gd5f1gq4u_ooblayout = {
>> +	.ecc = gd5f1gq4u_ooblayout_ecc,
>> +	.free = gd5f1gq4u_ooblayout_free,
>> +};
>> +
>>   static const struct spinand_info gigadevice_spinand_table[] = {
>>   	SPINAND_INFO("GD5F1GQ4xA", 0xF1,
>>   		     NAND_MEMORG(1, 2048, 64, 64, 1024, 1, 1, 1),
>> @@ -114,6 +144,15 @@ static const struct spinand_info gigadevice_spinand_table[] = {
>>   		     0,
>>   		     SPINAND_ECCINFO(&gd5fxgq4xa_ooblayout,
>>   				     gd5fxgq4xa_ecc_get_status)),
>> +	SPINAND_INFO("GD5F1GQ4UC", 0xd1,
>> +		     NAND_MEMORG(1, 2048, 128, 64, 1024, 1, 1, 1),
>> +		     NAND_ECCREQ(8, 2048),
>> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> +					      &write_cache_variants,
>> +					      &update_cache_variants),
>> +		     0,
>> +		     SPINAND_ECCINFO(&gd5f1gq4u_ooblayout,
>> +				     gd5fxgq4xa_ecc_get_status)),
> 
> The gd5fxgq4xa_ecc_get_status() func does not work for this chip.

Why not? Using the ECCS0/1 bits are also defined as bits 4/5 in the
status byte (addr 0xc0). The ECCSE0/1 bits would provide a more
fine grained info though.

> Something like that should do the trick:
> 
> #define GD5F1GQ4U_ECC_STATUS_MASK	GENMASK(6, 4)

Bit 6 is reserved in my datasheet version. Which version are you
referring to?
  
> static int gd5f1gq4u_ecc_get_status(struct spinand_device *spinand,
> 				    u8 status)
> {
> 	unsigned int nbitflips;
> 
> 	nbitflips = (status & GD5F1GQ4U_ECC_STATUS_MASK) >> 4;
> 	if (!nbitflips)
> 		return 0;
> 
> 	nbitflips += 2;
> 	if (nbitflips > 8)
> 		return -EBADMSG;
> 
> 	return nbitflips
> }

Hmmm this does not match my datasheet AFAICT (please see above).

Many thanks for the review,
Stefan

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

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

[Boris Brezillon]

On Wed, 23 Jan 2019 07:57:23 +0100
Stefan Roese <sr@denx.de> wrote:

> On 22.01.19 17:54, Boris Brezillon wrote:
> > On Tue, 22 Jan 2019 15:56:32 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> Add support for GigaDevice GD5F1GQ4UC SPI NAND chip.
> >>
> >> Signed-off-by: Stefan Roese <sr@denx.de>
> >> Cc: Chuanhong Guo <gch981213@gmail.com>
> >> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
> >> Cc: Miquel Raynal <miquel.raynal@bootlin.com>  
> > 
> > You forgot to Cc me on this one ;-).  
> 
> Ah, sorry about that.
>   
> >> ---
> >> Frankly, I'm a bit unsure, if these new ooblayout_foo functions are
> >> needed for this device. I was unable to find a datasheet for the
> >> already supported devices (GD5F1G/2G/4GQ4xA), so I couldn't compare
> >> the OOB area values here with the ones for my SPI NAND chip.  
> > 
> > Looks like it's using a different layout.  
> 
> Thanks. Can you please point me to a datasheet for the
> GD5F1G/2G/4GQ4xA?

I don't have it, I just looked at the code.

>   
> >>
> >> I'm also not 100% sure, if I should use NAND_ECCREQ(8, 2048) or
> >> NAND_ECCREQ(8, 512) for this device.  
> > 
> > Given the size reserved to store ECC bytes I'd say 8bits/512bytes.
> > There's an easy way to validate that => nandbiterrs (in mtd-utils).  
> 
> How so. Here some output of this test tool:
> 
> # ./nandbiterrs /dev/mtd5 -k -o

use the -i instead of -o.

> overwrite biterrors test
> Read reported 7 corrected bit errors
> Read reported 8 corrected bit errors
> Failed to recover 1 bitflips
> Bit error histogram (669 operations total):
> Page reads with   0 corrected bit errors: 80
> Page reads with   1 corrected bit errors: 0
> Page reads with   2 corrected bit errors: 0
> Page reads with   3 corrected bit errors: 0
> Page reads with   4 corrected bit errors: 0
> Page reads with   5 corrected bit errors: 0
> Page reads with   6 corrected bit errors: 0
> Page reads with   7 corrected bit errors: 500
> 
> How does this tell me, if it's 8bits/512bytes or some other
> value?

This one doesn't, incremental mode (-i) should.

>   
> >>
> >> So comments welcome.
> >>
> >> Thanks,
> >> Stefan
> >>
> >>   drivers/mtd/nand/spi/gigadevice.c | 39 +++++++++++++++++++++++++++++++
> >>   1 file changed, 39 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/spi/gigadevice.c b/drivers/mtd/nand/spi/gigadevice.c
> >> index e4141c20947a..a9d256fa2577 100644
> >> --- a/drivers/mtd/nand/spi/gigadevice.c
> >> +++ b/drivers/mtd/nand/spi/gigadevice.c
> >> @@ -57,6 +57,31 @@ static int gd5fxgq4xa_ooblayout_free(struct mtd_info *mtd, int section,
> >>   	return 0;
> >>   }
> >>   
> >> +static int gd5f1gq4u_ooblayout_ecc(struct mtd_info *mtd, int section,
> >> +				   struct mtd_oob_region *region)
> >> +{
> >> +	if (section)
> >> +		return -ERANGE;
> >> +
> >> +	region->offset = 64;
> >> +	region->length = 64;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int gd5f1gq4u_ooblayout_free(struct mtd_info *mtd, int section,
> >> +				    struct mtd_oob_region *region)
> >> +{
> >> +	if (section)
> >> +		return -ERANGE;
> >> +
> >> +	/* Reserve 2 bytes for the BBM. */
> >> +	region->offset = 2;  
> > 
> > According to the datasheet, the BBM is only one byte large.  
> 
> Yes, will change in v2.
> 
> >   
> >> +	region->length = 62;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   static int gd5fxgq4xa_ecc_get_status(struct spinand_device *spinand,
> >>   					 u8 status)
> >>   {
> >> @@ -86,6 +111,11 @@ static const struct mtd_ooblayout_ops gd5fxgq4xa_ooblayout = {
> >>   	.free = gd5fxgq4xa_ooblayout_free,
> >>   };
> >>   
> >> +static const struct mtd_ooblayout_ops gd5f1gq4u_ooblayout = {
> >> +	.ecc = gd5f1gq4u_ooblayout_ecc,
> >> +	.free = gd5f1gq4u_ooblayout_free,
> >> +};
> >> +
> >>   static const struct spinand_info gigadevice_spinand_table[] = {
> >>   	SPINAND_INFO("GD5F1GQ4xA", 0xF1,
> >>   		     NAND_MEMORG(1, 2048, 64, 64, 1024, 1, 1, 1),
> >> @@ -114,6 +144,15 @@ static const struct spinand_info gigadevice_spinand_table[] = {
> >>   		     0,
> >>   		     SPINAND_ECCINFO(&gd5fxgq4xa_ooblayout,
> >>   				     gd5fxgq4xa_ecc_get_status)),
> >> +	SPINAND_INFO("GD5F1GQ4UC", 0xd1,
> >> +		     NAND_MEMORG(1, 2048, 128, 64, 1024, 1, 1, 1),
> >> +		     NAND_ECCREQ(8, 2048),
> >> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> >> +					      &write_cache_variants,
> >> +					      &update_cache_variants),
> >> +		     0,
> >> +		     SPINAND_ECCINFO(&gd5f1gq4u_ooblayout,
> >> +				     gd5fxgq4xa_ecc_get_status)),  
> > 
> > The gd5fxgq4xa_ecc_get_status() func does not work for this chip.  
> 
> Why not? Using the ECCS0/1 bits are also defined as bits 4/5 in the
> status byte (addr 0xc0). The ECCSE0/1 bits would provide a more
> fine grained info though.
> 
> > Something like that should do the trick:
> > 
> > #define GD5F1GQ4U_ECC_STATUS_MASK	GENMASK(6, 4)  
> 
> Bit 6 is reserved in my datasheet version. Which version are you
> referring to?

http://cn.gigadevice.com/product/detail/30/469.html?locale=en_US

and the datasheet I downloaded says GD5FxGQ4xC, which seems to match
the GD5F1GQ4UC part you mention in your commit message.

>   
> > static int gd5f1gq4u_ecc_get_status(struct spinand_device *spinand,
> > 				    u8 status)
> > {
> > 	unsigned int nbitflips;
> > 
> > 	nbitflips = (status & GD5F1GQ4U_ECC_STATUS_MASK) >> 4;
> > 	if (!nbitflips)
> > 		return 0;
> > 
> > 	nbitflips += 2;
> > 	if (nbitflips > 8)
> > 		return -EBADMSG;
> > 
> > 	return nbitflips
> > }  
> 
> Hmmm this does not match my datasheet AFAICT (please see above).
> 
> Many thanks for the review,
> Stefan
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/


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

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

[Stefan Roese]

On 23.01.19 08:52, Boris Brezillon wrote:
> On Wed, 23 Jan 2019 07:57:23 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> On 22.01.19 17:54, Boris Brezillon wrote:
>>> On Tue, 22 Jan 2019 15:56:32 +0100
>>> Stefan Roese <sr@denx.de> wrote:
>>>    
>>>> Add support for GigaDevice GD5F1GQ4UC SPI NAND chip.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Chuanhong Guo <gch981213@gmail.com>
>>>> Cc: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
>>>
>>> You forgot to Cc me on this one ;-).
>>
>> Ah, sorry about that.
>>    
>>>> ---
>>>> Frankly, I'm a bit unsure, if these new ooblayout_foo functions are
>>>> needed for this device. I was unable to find a datasheet for the
>>>> already supported devices (GD5F1G/2G/4GQ4xA), so I couldn't compare
>>>> the OOB area values here with the ones for my SPI NAND chip.
>>>
>>> Looks like it's using a different layout.
>>
>> Thanks. Can you please point me to a datasheet for the
>> GD5F1G/2G/4GQ4xA?
> 
> I don't have it, I just looked at the code.
> 
>>    
>>>>
>>>> I'm also not 100% sure, if I should use NAND_ECCREQ(8, 2048) or
>>>> NAND_ECCREQ(8, 512) for this device.
>>>
>>> Given the size reserved to store ECC bytes I'd say 8bits/512bytes.
>>> There's an easy way to validate that => nandbiterrs (in mtd-utils).
>>
>> How so. Here some output of this test tool:
>>
>> # ./nandbiterrs /dev/mtd5 -k -o
> 
> use the -i instead of -o.
> 
>> overwrite biterrors test
>> Read reported 7 corrected bit errors
>> Read reported 8 corrected bit errors
>> Failed to recover 1 bitflips
>> Bit error histogram (669 operations total):
>> Page reads with   0 corrected bit errors: 80
>> Page reads with   1 corrected bit errors: 0
>> Page reads with   2 corrected bit errors: 0
>> Page reads with   3 corrected bit errors: 0
>> Page reads with   4 corrected bit errors: 0
>> Page reads with   5 corrected bit errors: 0
>> Page reads with   6 corrected bit errors: 0
>> Page reads with   7 corrected bit errors: 500
>>
>> How does this tell me, if it's 8bits/512bytes or some other
>> value?
> 
> This one doesn't, incremental mode (-i) should.

Here you go:

# ./nandbiterrs /dev/mtd5 -k -i
incremental biterrors test
Failed to recover 1 bitflips
Read error after 0 bit errors per page

I'm still unsure how this helps here. Is there anything else I should
test?
  
>>    
>>>>
>>>> So comments welcome.
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>    drivers/mtd/nand/spi/gigadevice.c | 39 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/spi/gigadevice.c b/drivers/mtd/nand/spi/gigadevice.c
>>>> index e4141c20947a..a9d256fa2577 100644
>>>> --- a/drivers/mtd/nand/spi/gigadevice.c
>>>> +++ b/drivers/mtd/nand/spi/gigadevice.c
>>>> @@ -57,6 +57,31 @@ static int gd5fxgq4xa_ooblayout_free(struct mtd_info *mtd, int section,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static int gd5f1gq4u_ooblayout_ecc(struct mtd_info *mtd, int section,
>>>> +				   struct mtd_oob_region *region)
>>>> +{
>>>> +	if (section)
>>>> +		return -ERANGE;
>>>> +
>>>> +	region->offset = 64;
>>>> +	region->length = 64;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int gd5f1gq4u_ooblayout_free(struct mtd_info *mtd, int section,
>>>> +				    struct mtd_oob_region *region)
>>>> +{
>>>> +	if (section)
>>>> +		return -ERANGE;
>>>> +
>>>> +	/* Reserve 2 bytes for the BBM. */
>>>> +	region->offset = 2;
>>>
>>> According to the datasheet, the BBM is only one byte large.
>>
>> Yes, will change in v2.
>>
>>>    
>>>> +	region->length = 62;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    static int gd5fxgq4xa_ecc_get_status(struct spinand_device *spinand,
>>>>    					 u8 status)
>>>>    {
>>>> @@ -86,6 +111,11 @@ static const struct mtd_ooblayout_ops gd5fxgq4xa_ooblayout = {
>>>>    	.free = gd5fxgq4xa_ooblayout_free,
>>>>    };
>>>>    
>>>> +static const struct mtd_ooblayout_ops gd5f1gq4u_ooblayout = {
>>>> +	.ecc = gd5f1gq4u_ooblayout_ecc,
>>>> +	.free = gd5f1gq4u_ooblayout_free,
>>>> +};
>>>> +
>>>>    static const struct spinand_info gigadevice_spinand_table[] = {
>>>>    	SPINAND_INFO("GD5F1GQ4xA", 0xF1,
>>>>    		     NAND_MEMORG(1, 2048, 64, 64, 1024, 1, 1, 1),
>>>> @@ -114,6 +144,15 @@ static const struct spinand_info gigadevice_spinand_table[] = {
>>>>    		     0,
>>>>    		     SPINAND_ECCINFO(&gd5fxgq4xa_ooblayout,
>>>>    				     gd5fxgq4xa_ecc_get_status)),
>>>> +	SPINAND_INFO("GD5F1GQ4UC", 0xd1,
>>>> +		     NAND_MEMORG(1, 2048, 128, 64, 1024, 1, 1, 1),
>>>> +		     NAND_ECCREQ(8, 2048),
>>>> +		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>>>> +					      &write_cache_variants,
>>>> +					      &update_cache_variants),
>>>> +		     0,
>>>> +		     SPINAND_ECCINFO(&gd5f1gq4u_ooblayout,
>>>> +				     gd5fxgq4xa_ecc_get_status)),
>>>
>>> The gd5fxgq4xa_ecc_get_status() func does not work for this chip.
>>
>> Why not? Using the ECCS0/1 bits are also defined as bits 4/5 in the
>> status byte (addr 0xc0). The ECCSE0/1 bits would provide a more
>> fine grained info though.
>>
>>> Something like that should do the trick:
>>>
>>> #define GD5F1GQ4U_ECC_STATUS_MASK	GENMASK(6, 4)
>>
>> Bit 6 is reserved in my datasheet version. Which version are you
>> referring to?
> 
> http://cn.gigadevice.com/product/detail/30/469.html?locale=en_US
> 
> and the datasheet I downloaded says GD5FxGQ4xC, which seems to match
> the GD5F1GQ4UC part you mention in your commit message.

Yes, but it does not show the device ID 0xd1 which I'm reading to
detect the chip. Here the datasheet that I'm using right now:

www.gigadevice.com/datasheet/gd5f1gq4xexxg/

Looking again, its "GD5F1GQ4UExxG" on the GigaDevice webpage and
in the datasheet, 0xd1 references "GD5F1GQ4U". So I should change
the chip name to "GD5F1GQ4UExxG" instead.

Sorry for the confusion here.

I'll try to enhance the ecc_get_status() function to also read the
status register at addr 0xf0 for the ECCSE0/1 bits to determine a
more fine grained ECC information for this chip.

Thanks,
Stefan

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

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

[Boris Brezillon]

On Wed, 23 Jan 2019 09:23:47 +0100
Stefan Roese <sr@denx.de> wrote:

> > This one doesn't, incremental mode (-i) should.  
> 
> Here you go:
> 
> # ./nandbiterrs /dev/mtd5 -k -i
> incremental biterrors test
> Failed to recover 1 bitflips
> Read error after 0 bit errors per page
> 
> I'm still unsure how this helps here.

It helps, it tells us the ECC doesn't work properly (fails to recover
one bitflip), or maybe it's the raw accessors that don't don't work.

> Is there anything else I should test?

Add traces to the get_ecc_status() func and print the status value.
> 
> www.gigadevice.com/datasheet/gd5f1gq4xexxg/
> 
> Looking again, its "GD5F1GQ4UExxG" on the GigaDevice webpage and
> in the datasheet, 0xd1 references "GD5F1GQ4U". So I should change
> the chip name to "GD5F1GQ4UExxG" instead.

Yes, please do that.

> 
> Sorry for the confusion here.

No problem.

> 
> I'll try to enhance the ecc_get_status() function to also read the
> status register at addr 0xf0 for the ECCSE0/1 bits to determine a
> more fine grained ECC information for this chip.

Let's first figure out why nandbiterrs fails to correct 1 bitflip.

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

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

[Stefan Roese]

On 23.01.19 09:55, Boris Brezillon wrote:
> On Wed, 23 Jan 2019 09:23:47 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>>> This one doesn't, incremental mode (-i) should.
>>
>> Here you go:
>>
>> # ./nandbiterrs /dev/mtd5 -k -i
>> incremental biterrors test
>> Failed to recover 1 bitflips
>> Read error after 0 bit errors per page
>>
>> I'm still unsure how this helps here.
> 
> It helps, it tells us the ECC doesn't work properly (fails to recover
> one bitflip), or maybe it's the raw accessors that don't don't work.
> 
>> Is there anything else I should test?
> 
> Add traces to the get_ecc_status() func and print the status value.

# ./nandbiterrs /dev/mtd5 -k -i
[   22.098436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
[   22.117184] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00

<snip many identical lines>

[   23.085412] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
incremental biterrors test
[   23.102973] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
Failed to recover 1 bitflips
Read error after 0 bit errors per page

Strange, this does not seem to match what the datasheet tells us. Any
further ideas what I should test?

Thanks,
Stefan

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

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

[Boris Brezillon]

On Wed, 23 Jan 2019 10:06:59 +0100
Stefan Roese <sr@denx.de> wrote:

> On 23.01.19 09:55, Boris Brezillon wrote:
> > On Wed, 23 Jan 2019 09:23:47 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >>> This one doesn't, incremental mode (-i) should.  
> >>
> >> Here you go:
> >>
> >> # ./nandbiterrs /dev/mtd5 -k -i
> >> incremental biterrors test
> >> Failed to recover 1 bitflips
> >> Read error after 0 bit errors per page
> >>
> >> I'm still unsure how this helps here.  
> > 
> > It helps, it tells us the ECC doesn't work properly (fails to recover
> > one bitflip), or maybe it's the raw accessors that don't don't work.
> >   
> >> Is there anything else I should test?  
> > 
> > Add traces to the get_ecc_status() func and print the status value.  
> 
> # ./nandbiterrs /dev/mtd5 -k -i
> [   22.098436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> [   22.117184] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> 
> <snip many identical lines>
> 
> [   23.085412] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> incremental biterrors test
> [   23.102973] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
> Failed to recover 1 bitflips

Hm, looks like the ECC reports error as soon as you start writing to
the NAND. Maybe we have a problem in the write path...

> Read error after 0 bit errors per page
> 
> Strange, this does not seem to match what the datasheet tells us. Any
> further ideas what I should test?

Erase a block (save data before if you need to), write random data with
the ECC enabled and dump it back (once in raw mode, once with ECC
enabled):

# flash_erase /dev/mtdX 0 1
# nandwrite --input-size=<pagesize> /dev/mtdX /dev/urandom
# nanddump -f /tmp/dump-ecc -l <pagesize> -o /dev/mtdX
# nanddump -f /tmp/dump-raw -l <pagesize> -o -n /dev/mtdX

Send me both dumps (plus the console output), and we'll see how it
looks.

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

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

[Stefan Roese]

[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]

On 23.01.19 10:35, Boris Brezillon wrote:
> On Wed, 23 Jan 2019 10:06:59 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> On 23.01.19 09:55, Boris Brezillon wrote:
>>> On Wed, 23 Jan 2019 09:23:47 +0100
>>> Stefan Roese <sr@denx.de> wrote:
>>>    
>>>>> This one doesn't, incremental mode (-i) should.
>>>>
>>>> Here you go:
>>>>
>>>> # ./nandbiterrs /dev/mtd5 -k -i
>>>> incremental biterrors test
>>>> Failed to recover 1 bitflips
>>>> Read error after 0 bit errors per page
>>>>
>>>> I'm still unsure how this helps here.
>>>
>>> It helps, it tells us the ECC doesn't work properly (fails to recover
>>> one bitflip), or maybe it's the raw accessors that don't don't work.
>>>    
>>>> Is there anything else I should test?
>>>
>>> Add traces to the get_ecc_status() func and print the status value.
>>
>> # ./nandbiterrs /dev/mtd5 -k -i
>> [   22.098436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>> [   22.117184] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>>
>> <snip many identical lines>
>>
>> [   23.085412] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>> incremental biterrors test
>> [   23.102973] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
>> Failed to recover 1 bitflips
> 
> Hm, looks like the ECC reports error as soon as you start writing to
> the NAND. Maybe we have a problem in the write path...
> 
>> Read error after 0 bit errors per page
>>
>> Strange, this does not seem to match what the datasheet tells us. Any
>> further ideas what I should test?
> 
> Erase a block (save data before if you need to), write random data with
> the ECC enabled and dump it back (once in raw mode, once with ECC
> enabled):
> 
> # flash_erase /dev/mtdX 0 1
> # nandwrite --input-size=<pagesize> /dev/mtdX /dev/urandom
> # nanddump -f /tmp/dump-ecc -l <pagesize> -o /dev/mtdX
> # nanddump -f /tmp/dump-raw -l <pagesize> -o -n /dev/mtdX
> 
> Send me both dumps (plus the console output), and we'll see how it
> looks.

Here you go:

root@mt7688:~# flash_erase /dev/mtd5 0 1
Erasing 128 Kibyte @ 0 -- 100 % complete
root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
Writing data to block 0 at offset 0x0
root@mt7688:~# nanddump -f /tmp/dump-ecc -l 2048 -o /dev/mtd5
ECC failed: 0
ECC corrected:[  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
  0
Number of ba[  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
d blocks: 2
Number of bbt blocks: 0
Block size 131072, page size 2048, OOB size 128
Dumping data starting at 0x00000000 and ending at 0x00000800...
root@mt7688:~# dmesg -c
[  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
[  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
root@mt7688:~# nanddump -f /tmp/dump-raw -l 2048 -o -n /dev/mtd5
Block size 131072, page size 2048, OOB size 128
Dumping data starting at 0x00000000 and ending at 0x00000800...
root@mt7688:~# dmesg -c
root@mt7688:~#

The attached files are identical. Thanks for looking into this.

Thanks,
Stefan

[-- Attachment #2: dump-raw --]
[-- Type: application/octet-stream, Size: 2176 bytes --]

[-- Attachment #3: dump-ecc --]
[-- Type: application/octet-stream, Size: 2176 bytes --]

[-- Attachment #4: Type: text/plain, Size: 144 bytes --]

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

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

[Boris Brezillon]

On Wed, 23 Jan 2019 11:04:36 +0100
Stefan Roese <sr@denx.de> wrote:

> On 23.01.19 10:35, Boris Brezillon wrote:
> > On Wed, 23 Jan 2019 10:06:59 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> On 23.01.19 09:55, Boris Brezillon wrote:  
> >>> On Wed, 23 Jan 2019 09:23:47 +0100
> >>> Stefan Roese <sr@denx.de> wrote:
> >>>      
> >>>>> This one doesn't, incremental mode (-i) should.  
> >>>>
> >>>> Here you go:
> >>>>
> >>>> # ./nandbiterrs /dev/mtd5 -k -i
> >>>> incremental biterrors test
> >>>> Failed to recover 1 bitflips
> >>>> Read error after 0 bit errors per page
> >>>>
> >>>> I'm still unsure how this helps here.  
> >>>
> >>> It helps, it tells us the ECC doesn't work properly (fails to recover
> >>> one bitflip), or maybe it's the raw accessors that don't don't work.
> >>>      
> >>>> Is there anything else I should test?  
> >>>
> >>> Add traces to the get_ecc_status() func and print the status value.  
> >>
> >> # ./nandbiterrs /dev/mtd5 -k -i
> >> [   22.098436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >> [   22.117184] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >>
> >> <snip many identical lines>
> >>
> >> [   23.085412] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >> incremental biterrors test
> >> [   23.102973] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
> >> Failed to recover 1 bitflips  
> > 
> > Hm, looks like the ECC reports error as soon as you start writing to
> > the NAND. Maybe we have a problem in the write path...
> >   
> >> Read error after 0 bit errors per page
> >>
> >> Strange, this does not seem to match what the datasheet tells us. Any
> >> further ideas what I should test?  
> > 
> > Erase a block (save data before if you need to), write random data with
> > the ECC enabled and dump it back (once in raw mode, once with ECC
> > enabled):
> > 
> > # flash_erase /dev/mtdX 0 1
> > # nandwrite --input-size=<pagesize> /dev/mtdX /dev/urandom
> > # nanddump -f /tmp/dump-ecc -l <pagesize> -o /dev/mtdX
> > # nanddump -f /tmp/dump-raw -l <pagesize> -o -n /dev/mtdX
> > 
> > Send me both dumps (plus the console output), and we'll see how it
> > looks.  
> 
> Here you go:
> 
> root@mt7688:~# flash_erase /dev/mtd5 0 1
> Erasing 128 Kibyte @ 0 -- 100 % complete
> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
> Writing data to block 0 at offset 0x0
> root@mt7688:~# nanddump -f /tmp/dump-ecc -l 2048 -o /dev/mtd5
> ECC failed: 0
> ECC corrected:[  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>   0
> Number of ba[  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> d blocks: 2
> Number of bbt blocks: 0
> Block size 131072, page size 2048, OOB size 128
> Dumping data starting at 0x00000000 and ending at 0x00000800...
> root@mt7688:~# dmesg -c
> [  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> [  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> root@mt7688:~# nanddump -f /tmp/dump-raw -l 2048 -o -n /dev/mtd5
> Block size 131072, page size 2048, OOB size 128
> Dumping data starting at 0x00000000 and ending at 0x00000800...
> root@mt7688:~# dmesg -c
> root@mt7688:~#
> 
> The attached files are identical. Thanks for looking into this.

First weird thing, the first portion of OOB (bytes 0x800 to 0x83F) are
set to 0x0, and I'd expect to have 0xff in there. BTW, can you try
nandbiterrs again without the '-k'?

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

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

[Boris Brezillon]

On Wed, 23 Jan 2019 12:25:14 +0100
Boris Brezillon <bbrezillon@kernel.org> wrote:

> On Wed, 23 Jan 2019 11:04:36 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
> > On 23.01.19 10:35, Boris Brezillon wrote:  
> > > On Wed, 23 Jan 2019 10:06:59 +0100
> > > Stefan Roese <sr@denx.de> wrote:
> > >     
> > >> On 23.01.19 09:55, Boris Brezillon wrote:    
> > >>> On Wed, 23 Jan 2019 09:23:47 +0100
> > >>> Stefan Roese <sr@denx.de> wrote:
> > >>>        
> > >>>>> This one doesn't, incremental mode (-i) should.    
> > >>>>
> > >>>> Here you go:
> > >>>>
> > >>>> # ./nandbiterrs /dev/mtd5 -k -i
> > >>>> incremental biterrors test
> > >>>> Failed to recover 1 bitflips
> > >>>> Read error after 0 bit errors per page
> > >>>>
> > >>>> I'm still unsure how this helps here.    
> > >>>
> > >>> It helps, it tells us the ECC doesn't work properly (fails to recover
> > >>> one bitflip), or maybe it's the raw accessors that don't don't work.
> > >>>        
> > >>>> Is there anything else I should test?    
> > >>>
> > >>> Add traces to the get_ecc_status() func and print the status value.    
> > >>
> > >> # ./nandbiterrs /dev/mtd5 -k -i
> > >> [   22.098436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> > >> [   22.117184] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> > >>
> > >> <snip many identical lines>
> > >>
> > >> [   23.085412] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> > >> incremental biterrors test
> > >> [   23.102973] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
> > >> Failed to recover 1 bitflips    
> > > 
> > > Hm, looks like the ECC reports error as soon as you start writing to
> > > the NAND. Maybe we have a problem in the write path...
> > >     
> > >> Read error after 0 bit errors per page
> > >>
> > >> Strange, this does not seem to match what the datasheet tells us. Any
> > >> further ideas what I should test?    
> > > 
> > > Erase a block (save data before if you need to), write random data with
> > > the ECC enabled and dump it back (once in raw mode, once with ECC
> > > enabled):
> > > 
> > > # flash_erase /dev/mtdX 0 1
> > > # nandwrite --input-size=<pagesize> /dev/mtdX /dev/urandom
> > > # nanddump -f /tmp/dump-ecc -l <pagesize> -o /dev/mtdX
> > > # nanddump -f /tmp/dump-raw -l <pagesize> -o -n /dev/mtdX
> > > 
> > > Send me both dumps (plus the console output), and we'll see how it
> > > looks.    
> > 
> > Here you go:
> > 
> > root@mt7688:~# flash_erase /dev/mtd5 0 1
> > Erasing 128 Kibyte @ 0 -- 100 % complete
> > root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
> > Writing data to block 0 at offset 0x0
> > root@mt7688:~# nanddump -f /tmp/dump-ecc -l 2048 -o /dev/mtd5
> > ECC failed: 0
> > ECC corrected:[  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >   0
> > Number of ba[  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> > d blocks: 2
> > Number of bbt blocks: 0
> > Block size 131072, page size 2048, OOB size 128
> > Dumping data starting at 0x00000000 and ending at 0x00000800...
> > root@mt7688:~# dmesg -c
> > [  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> > [  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> > root@mt7688:~# nanddump -f /tmp/dump-raw -l 2048 -o -n /dev/mtd5
> > Block size 131072, page size 2048, OOB size 128
> > Dumping data starting at 0x00000000 and ending at 0x00000800...
> > root@mt7688:~# dmesg -c
> > root@mt7688:~#
> > 
> > The attached files are identical. Thanks for looking into this.  
> 
> First weird thing, the first portion of OOB (bytes 0x800 to 0x83F) are
> set to 0x0, and I'd expect to have 0xff in there. BTW, can you try
> nandbiterrs again without the '-k'?

BTW, which version of the mtd-utils are you using?

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

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

[Stefan Roese]

On 23.01.19 12:25, Boris Brezillon wrote:
> On Wed, 23 Jan 2019 11:04:36 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> On 23.01.19 10:35, Boris Brezillon wrote:
>>> On Wed, 23 Jan 2019 10:06:59 +0100
>>> Stefan Roese <sr@denx.de> wrote:
>>>    
>>>> On 23.01.19 09:55, Boris Brezillon wrote:
>>>>> On Wed, 23 Jan 2019 09:23:47 +0100
>>>>> Stefan Roese <sr@denx.de> wrote:
>>>>>       
>>>>>>> This one doesn't, incremental mode (-i) should.
>>>>>>
>>>>>> Here you go:
>>>>>>
>>>>>> # ./nandbiterrs /dev/mtd5 -k -i
>>>>>> incremental biterrors test
>>>>>> Failed to recover 1 bitflips
>>>>>> Read error after 0 bit errors per page
>>>>>>
>>>>>> I'm still unsure how this helps here.
>>>>>
>>>>> It helps, it tells us the ECC doesn't work properly (fails to recover
>>>>> one bitflip), or maybe it's the raw accessors that don't don't work.
>>>>>       
>>>>>> Is there anything else I should test?
>>>>>
>>>>> Add traces to the get_ecc_status() func and print the status value.
>>>>
>>>> # ./nandbiterrs /dev/mtd5 -k -i
>>>> [   22.098436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>>>> [   22.117184] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>>>>
>>>> <snip many identical lines>
>>>>
>>>> [   23.085412] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>>>> incremental biterrors test
>>>> [   23.102973] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
>>>> Failed to recover 1 bitflips
>>>
>>> Hm, looks like the ECC reports error as soon as you start writing to
>>> the NAND. Maybe we have a problem in the write path...
>>>    
>>>> Read error after 0 bit errors per page
>>>>
>>>> Strange, this does not seem to match what the datasheet tells us. Any
>>>> further ideas what I should test?
>>>
>>> Erase a block (save data before if you need to), write random data with
>>> the ECC enabled and dump it back (once in raw mode, once with ECC
>>> enabled):
>>>
>>> # flash_erase /dev/mtdX 0 1
>>> # nandwrite --input-size=<pagesize> /dev/mtdX /dev/urandom
>>> # nanddump -f /tmp/dump-ecc -l <pagesize> -o /dev/mtdX
>>> # nanddump -f /tmp/dump-raw -l <pagesize> -o -n /dev/mtdX
>>>
>>> Send me both dumps (plus the console output), and we'll see how it
>>> looks.
>>
>> Here you go:
>>
>> root@mt7688:~# flash_erase /dev/mtd5 0 1
>> Erasing 128 Kibyte @ 0 -- 100 % complete
>> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
>> Writing data to block 0 at offset 0x0
>> root@mt7688:~# nanddump -f /tmp/dump-ecc -l 2048 -o /dev/mtd5
>> ECC failed: 0
>> ECC corrected:[  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>>    0
>> Number of ba[  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>> d blocks: 2
>> Number of bbt blocks: 0
>> Block size 131072, page size 2048, OOB size 128
>> Dumping data starting at 0x00000000 and ending at 0x00000800...
>> root@mt7688:~# dmesg -c
>> [  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>> [  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
>> root@mt7688:~# nanddump -f /tmp/dump-raw -l 2048 -o -n /dev/mtd5
>> Block size 131072, page size 2048, OOB size 128
>> Dumping data starting at 0x00000000 and ending at 0x00000800...
>> root@mt7688:~# dmesg -c
>> root@mt7688:~#
>>
>> The attached files are identical. Thanks for looking into this.
> 
> First weird thing, the first portion of OOB (bytes 0x800 to 0x83F) are
> set to 0x0, and I'd expect to have 0xff in there. BTW, can you try
> nandbiterrs again without the '-k'?

Same result:

root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
incremental biterrors test
[ 5748.988596] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
Failed to recover 1 bitflips
Read error after 0 bit errors per page

And from your other mail:

> BTW, which version of the mtd-utils are you using?

I'm currently using the one provided with my Yocto build:

root@mt7688:~# mtdinfo --version
mtdinfo (mtd-utils) 2.0.1

I hope that is recent enough.

Thanks,
Stefan

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

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

[Stefan Roese]

On 23.01.19 12:37, Stefan Roese wrote:

<snip>

>> BTW, which version of the mtd-utils are you using?
> 
> I'm currently using the one provided with my Yocto build:
> 
> root@mt7688:~# mtdinfo --version
> mtdinfo (mtd-utils) 2.0.1
> 
> I hope that is recent enough.

I just updated to 2.0.2, git commit ID 23b99432e59b4 as TOT does
not compile for me. Some result though.

Thanks,
Stefan

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

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

[Boris Brezillon]

On Wed, 23 Jan 2019 12:37:57 +0100
Stefan Roese <sr@denx.de> wrote:

> On 23.01.19 12:25, Boris Brezillon wrote:
> > On Wed, 23 Jan 2019 11:04:36 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> On 23.01.19 10:35, Boris Brezillon wrote:  
> >>> On Wed, 23 Jan 2019 10:06:59 +0100
> >>> Stefan Roese <sr@denx.de> wrote:
> >>>      
> >>>> On 23.01.19 09:55, Boris Brezillon wrote:  
> >>>>> On Wed, 23 Jan 2019 09:23:47 +0100
> >>>>> Stefan Roese <sr@denx.de> wrote:
> >>>>>         
> >>>>>>> This one doesn't, incremental mode (-i) should.  
> >>>>>>
> >>>>>> Here you go:
> >>>>>>
> >>>>>> # ./nandbiterrs /dev/mtd5 -k -i
> >>>>>> incremental biterrors test
> >>>>>> Failed to recover 1 bitflips
> >>>>>> Read error after 0 bit errors per page
> >>>>>>
> >>>>>> I'm still unsure how this helps here.  
> >>>>>
> >>>>> It helps, it tells us the ECC doesn't work properly (fails to recover
> >>>>> one bitflip), or maybe it's the raw accessors that don't don't work.
> >>>>>         
> >>>>>> Is there anything else I should test?  
> >>>>>
> >>>>> Add traces to the get_ecc_status() func and print the status value.  
> >>>>
> >>>> # ./nandbiterrs /dev/mtd5 -k -i
> >>>> [   22.098436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >>>> [   22.117184] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >>>>
> >>>> <snip many identical lines>
> >>>>
> >>>> [   23.085412] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >>>> incremental biterrors test
> >>>> [   23.102973] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
> >>>> Failed to recover 1 bitflips  
> >>>
> >>> Hm, looks like the ECC reports error as soon as you start writing to
> >>> the NAND. Maybe we have a problem in the write path...
> >>>      
> >>>> Read error after 0 bit errors per page
> >>>>
> >>>> Strange, this does not seem to match what the datasheet tells us. Any
> >>>> further ideas what I should test?  
> >>>
> >>> Erase a block (save data before if you need to), write random data with
> >>> the ECC enabled and dump it back (once in raw mode, once with ECC
> >>> enabled):
> >>>
> >>> # flash_erase /dev/mtdX 0 1
> >>> # nandwrite --input-size=<pagesize> /dev/mtdX /dev/urandom
> >>> # nanddump -f /tmp/dump-ecc -l <pagesize> -o /dev/mtdX
> >>> # nanddump -f /tmp/dump-raw -l <pagesize> -o -n /dev/mtdX
> >>>
> >>> Send me both dumps (plus the console output), and we'll see how it
> >>> looks.  
> >>
> >> Here you go:
> >>
> >> root@mt7688:~# flash_erase /dev/mtd5 0 1
> >> Erasing 128 Kibyte @ 0 -- 100 % complete
> >> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
> >> Writing data to block 0 at offset 0x0
> >> root@mt7688:~# nanddump -f /tmp/dump-ecc -l 2048 -o /dev/mtd5
> >> ECC failed: 0
> >> ECC corrected:[  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >>    0
> >> Number of ba[  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >> d blocks: 2
> >> Number of bbt blocks: 0
> >> Block size 131072, page size 2048, OOB size 128
> >> Dumping data starting at 0x00000000 and ending at 0x00000800...
> >> root@mt7688:~# dmesg -c
> >> [  100.171120] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >> [  100.178436] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> >> root@mt7688:~# nanddump -f /tmp/dump-raw -l 2048 -o -n /dev/mtd5
> >> Block size 131072, page size 2048, OOB size 128
> >> Dumping data starting at 0x00000000 and ending at 0x00000800...
> >> root@mt7688:~# dmesg -c
> >> root@mt7688:~#
> >>
> >> The attached files are identical. Thanks for looking into this.  
> > 
> > First weird thing, the first portion of OOB (bytes 0x800 to 0x83F) are
> > set to 0x0, and I'd expect to have 0xff in there. BTW, can you try
> > nandbiterrs again without the '-k'?  
> 
> Same result:
> 
> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> incremental biterrors test
> [ 5748.988596] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
> Failed to recover 1 bitflips
> Read error after 0 bit errors per page

Okay. There's something interesting in section "10.1 Page Program" of
the datasheet:

"
Note:
1. The contents of Cache Register doesn’t reset when Program Load (02h)
command, Program Random Load (84h)
command and RESET (FFh) command.
2. When Program Execute (10h) command was issued just after Program Load
(02h) command, SPI-NAND controller
outputs 0xFF data to the NAND for the address that data was not loaded
by Program Load (02h) command.
3. When Program Execute (10h) command was issued just after Program Load
Random Data (84h) command,
SPI-NAND controller outputs contents of Cache Register to the NAND.
"

Until now, I assumed that a "Program Load" would reset the page cache
content to 0xff (as is done on the NANDs I had tested on), but it seems
some vendors decided to implement it differently (keep the cache in its
previous state and send 0xff at execute time if the previous command
was a Program Load and some bytes were left uninitialized in the cache).

This forces us to fill the whole cache if we want the logic to work on
all NANDs otherwise we might corrupt things in the OOB area. It might
also explain while nandbiterrs does not work properly. Can you try to
apply the following diff and run nandbiterrs -i again?

> 
> And from your other mail:
> 
> > BTW, which version of the mtd-utils are you using?  
> 
> I'm currently using the one provided with my Yocto build:
> 
> root@mt7688:~# mtdinfo --version
> mtdinfo (mtd-utils) 2.0.1
> 
> I hope that is recent enough.

Should be good.

--->8---
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 479c2f2cf17f..10c92cc48428 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -313,15 +313,9 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
               nanddev_page_size(nand) +
               nanddev_per_page_oobsize(nand));
 
-       if (req->datalen) {
+       if (req->datalen)
                memcpy(spinand->databuf + req->dataoffs, req->databuf.out,
                       req->datalen);
-               adjreq.dataoffs = 0;
-               adjreq.datalen = nanddev_page_size(nand);
-               adjreq.databuf.out = spinand->databuf;
-               nbytes = adjreq.datalen;
-               buf = spinand->databuf;
-       }
 
        if (req->ooblen) {
                if (req->mode == MTD_OPS_AUTO_OOB)
@@ -332,16 +326,23 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
                else
                        memcpy(spinand->oobbuf + req->ooboffs, req->oobbuf.out,
                               req->ooblen);
-
-               adjreq.ooblen = nanddev_per_page_oobsize(nand);
-               adjreq.ooboffs = 0;
-               nbytes += nanddev_per_page_oobsize(nand);
-               if (!buf) {
-                       buf = spinand->oobbuf;
-                       column = nanddev_page_size(nand);
-               }
        }
 
+       /*
+        * Looks like PROGRAM LOAD (AKA write cache) does not necessarily reset
+        * the cache content to 0xFF (depends on vendor implementation), so we
+        * must fill the page cache entirely even if we only want to program
+        * the data portion of the page, otherwise we might corrupt the BBM or
+        * user data previously programmed in OOB area.
+        */
+       adjreq.dataoffs = 0;
+       adjreq.datalen = nanddev_page_size(nand);
+       adjreq.databuf.out = spinand->databuf;
+       adjreq.ooblen = nanddev_per_page_oobsize(nand);
+       adjreq.ooboffs = 0;
+       nbytes = nanddev_page_size(nand) + nanddev_per_page_oobsize(nand);
+       buf = spinand->databuf;
+
        spinand_cache_op_adjust_colum(spinand, &adjreq, &column);
 
        op = *spinand->op_templates.write_cache;

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

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

[Stefan Roese]

On 23.01.19 13:22, Boris Brezillon wrote:

<snip>

> Okay. There's something interesting in section "10.1 Page Program" of
> the datasheet:
> 
> "
> Note:
> 1. The contents of Cache Register doesn’t reset when Program Load (02h)
> command, Program Random Load (84h)
> command and RESET (FFh) command.
> 2. When Program Execute (10h) command was issued just after Program Load
> (02h) command, SPI-NAND controller
> outputs 0xFF data to the NAND for the address that data was not loaded
> by Program Load (02h) command.
> 3. When Program Execute (10h) command was issued just after Program Load
> Random Data (84h) command,
> SPI-NAND controller outputs contents of Cache Register to the NAND.
> "
> 
> Until now, I assumed that a "Program Load" would reset the page cache
> content to 0xff (as is done on the NANDs I had tested on), but it seems
> some vendors decided to implement it differently (keep the cache in its
> previous state and send 0xff at execute time if the previous command
> was a Program Load and some bytes were left uninitialized in the cache).
> 
> This forces us to fill the whole cache if we want the logic to work on
> all NANDs otherwise we might corrupt things in the OOB area. It might
> also explain while nandbiterrs does not work properly. Can you try to
> apply the following diff and run nandbiterrs -i again?

Definitely something different this time:

root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
incremental biterrors test
[   24.467652] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
Successfully corrected 0 bit errors per subpage
Inserted biterr[   24.478946] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
or @ 1/7
[   24.486718] gd5f1gq4u_ecc_get_status (138): status2=0x00
Read reported 4 corrected bit errors
Successfully corrected 1 b[   24.497760] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
it errors per su[   24.505708] gd5f1gq4u_ecc_get_status (138): status2=0x00
bpage
Inserted biterror @ 3/7
Read reported 4 corrected bit er[   24.517400] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
rors
Successful[   24.524623] gd5f1gq4u_ecc_get_status (138): status2=0x00
ly corrected 2 bit errors per subpage
Inserted biterror @ 5/7
[   24.536350] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
Read reported 4 [   24.543391] gd5f1gq4u_ecc_get_status (138): status2=0x00
corrected bit errors
Successfully corrected 3 bit errors per su[   24.555245] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x10
bpage
Inserted [   24.562436] gd5f1gq4u_ecc_get_status (138): status2=0x10
biterror @ 7/7
Read reported 4 corrected bit errors
Successful[   24.574014] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x20
ly corrected 4 b[   24.581360] gd5f1gq4u_ecc_get_status (138): status2=0x20
it errors per subpage
Inserted biterror @ 8/7
Read reported 5 [   24.593042] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x30
corrected bit er[   24.600284] gd5f1gq4u_ecc_get_status (138): status2=0x30
rors
Successfully corrected 5 bit errors per subpage
Inserted [   24.612127] gd5f1gq4u_ecc_get_status (124): status=0x30 status2=0x00
biterror @ 10/7
Read reported 6 corrected bit errors
Successfu[   24.623870] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
lly 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


I removed the debug output from the chip ecc_get_status function for
a clearer output. Here the same without the debug output:

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

This definitely does look better. I assume that we are we on the
right track now?

Thanks,
Stefan

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

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

[Boris Brezillon]

On Wed, 23 Jan 2019 13:34:06 +0100
Stefan Roese <sr@denx.de> wrote:

> On 23.01.19 13:22, Boris Brezillon wrote:
> 
> <snip>
> 
> > Okay. There's something interesting in section "10.1 Page Program" of
> > the datasheet:
> > 
> > "
> > Note:
> > 1. The contents of Cache Register doesn’t reset when Program Load (02h)
> > command, Program Random Load (84h)
> > command and RESET (FFh) command.
> > 2. When Program Execute (10h) command was issued just after Program Load
> > (02h) command, SPI-NAND controller
> > outputs 0xFF data to the NAND for the address that data was not loaded
> > by Program Load (02h) command.
> > 3. When Program Execute (10h) command was issued just after Program Load
> > Random Data (84h) command,
> > SPI-NAND controller outputs contents of Cache Register to the NAND.
> > "
> > 
> > Until now, I assumed that a "Program Load" would reset the page cache
> > content to 0xff (as is done on the NANDs I had tested on), but it seems
> > some vendors decided to implement it differently (keep the cache in its
> > previous state and send 0xff at execute time if the previous command
> > was a Program Load and some bytes were left uninitialized in the cache).
> > 
> > This forces us to fill the whole cache if we want the logic to work on
> > all NANDs otherwise we might corrupt things in the OOB area. It might
> > also explain while nandbiterrs does not work properly. Can you try to
> > apply the following diff and run nandbiterrs -i again?  
> 
> Definitely something different this time:
> 
> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> incremental biterrors test
> [   24.467652] gd5f1gq4u_ecc_get_status (124): status=0x00 status2=0x00
> Successfully corrected 0 bit errors per subpage
> Inserted biterr[   24.478946] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
> or @ 1/7
> [   24.486718] gd5f1gq4u_ecc_get_status (138): status2=0x00
> Read reported 4 corrected bit errors
> Successfully corrected 1 b[   24.497760] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
> it errors per su[   24.505708] gd5f1gq4u_ecc_get_status (138): status2=0x00
> bpage
> Inserted biterror @ 3/7
> Read reported 4 corrected bit er[   24.517400] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
> rors
> Successful[   24.524623] gd5f1gq4u_ecc_get_status (138): status2=0x00
> ly corrected 2 bit errors per subpage
> Inserted biterror @ 5/7
> [   24.536350] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x00
> Read reported 4 [   24.543391] gd5f1gq4u_ecc_get_status (138): status2=0x00
> corrected bit errors
> Successfully corrected 3 bit errors per su[   24.555245] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x10
> bpage
> Inserted [   24.562436] gd5f1gq4u_ecc_get_status (138): status2=0x10
> biterror @ 7/7
> Read reported 4 corrected bit errors
> Successful[   24.574014] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x20
> ly corrected 4 b[   24.581360] gd5f1gq4u_ecc_get_status (138): status2=0x20
> it errors per subpage
> Inserted biterror @ 8/7
> Read reported 5 [   24.593042] gd5f1gq4u_ecc_get_status (124): status=0x10 status2=0x30
> corrected bit er[   24.600284] gd5f1gq4u_ecc_get_status (138): status2=0x30
> rors
> Successfully corrected 5 bit errors per subpage
> Inserted [   24.612127] gd5f1gq4u_ecc_get_status (124): status=0x30 status2=0x00
> biterror @ 10/7
> Read reported 6 corrected bit errors
> Successfu[   24.623870] gd5f1gq4u_ecc_get_status (124): status=0x20 status2=0x00
> lly 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
> 
> 
> I removed the debug output from the chip ecc_get_status function for
> a clearer output. Here the same without the debug output:
> 
> 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
> 
> 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.

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

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

[Boris Brezillon]

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

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

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

[Stefan Roese]

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?

Sure, will gladly do - most likely tomorrow though.

Thanks,
Stefan

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

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

[Stefan Roese]

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. Here a log
with this version (new error this time):

root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
incremental biterrors test
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

Here a log with nandwrite errors:

root@mt7688:~# flash_erase /dev/mtd5 0 1
Erasing 128 Kibyte @ 0 -- 100 % complete
root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
Writing data to block 0 at offset 0x0
libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
         error 5 (Input/output error)
Erasing failed write from 00000000 to 0x01ffff
Writing data to block 1 at offset 0x20000
libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 1, offset 0)
         error 5 (Input/output error)
Erasing failed write from 0x020000 to 0x03ffff
Writing data to block 2 at offset 0x40000
libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 2, offset 0)
         error 5 (Input/output error)
Erasing failed write from 0x040000 to 0x05ffff
...

Any ideas on this?

Thanks,
Stefan

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

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

[Boris Brezillon]

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?

> Here a log
> with this version (new error this time):
> 
> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> incremental biterrors test
> 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
> 
> Here a log with nandwrite errors:
> 
> root@mt7688:~# flash_erase /dev/mtd5 0 1
> Erasing 128 Kibyte @ 0 -- 100 % complete
> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
> Writing data to block 0 at offset 0x0
> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
>          error 5 (Input/output error)
> Erasing failed write from 00000000 to 0x01ffff

Weird, I wasn't expecting ERASE to fail (nothing changed in the erase
path).

> Writing data to block 1 at offset 0x20000
> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 1, offset 0)
>          error 5 (Input/output error)
> Erasing failed write from 0x020000 to 0x03ffff
> Writing data to block 2 at offset 0x40000
> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 2, offset 0)
>          error 5 (Input/output error)
> Erasing failed write from 0x040000 to 0x05ffff
> ...
> 
> Any ideas on this?

Try to revert b47b307ac23d ("mtd: spinand: Use the spi-mem dirmap API").

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

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

[Boris Brezillon]

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. Here a log
> with this version (new error this time):
> 
> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> incremental biterrors test
> 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
> 
> Here a log with nandwrite errors:
> 
> root@mt7688:~# flash_erase /dev/mtd5 0 1
> Erasing 128 Kibyte @ 0 -- 100 % complete
> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
> Writing data to block 0 at offset 0x0
> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
>          error 5 (Input/output error)
> Erasing failed write from 00000000 to 0x01ffff
> Writing data to block 1 at offset 0x20000
> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 1, offset 0)
>          error 5 (Input/output error)
> Erasing failed write from 0x020000 to 0x03ffff
> Writing data to block 2 at offset 0x40000
> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 2, offset 0)
>          error 5 (Input/output error)
> Erasing failed write from 0x040000 to 0x05ffff
> ...
> 
> Any ideas on this?

BTW, what's the SPI controller connected to this chip?

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

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

[Stefan Roese]

[-- Attachment #1: Type: text/plain, Size: 2458 bytes --]

On 24.01.19 08:52, 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. Here a log
>> with this version (new error this time):
>>
>> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
>> incremental biterrors test
>> 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
>>
>> Here a log with nandwrite errors:
>>
>> root@mt7688:~# flash_erase /dev/mtd5 0 1
>> Erasing 128 Kibyte @ 0 -- 100 % complete
>> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
>> Writing data to block 0 at offset 0x0
>> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
>>           error 5 (Input/output error)
>> Erasing failed write from 00000000 to 0x01ffff
>> Writing data to block 1 at offset 0x20000
>> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 1, offset 0)
>>           error 5 (Input/output error)
>> Erasing failed write from 0x020000 to 0x03ffff
>> Writing data to block 2 at offset 0x40000
>> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 2, offset 0)
>>           error 5 (Input/output error)
>> Erasing failed write from 0x040000 to 0x05ffff
>> ...
>>
>> Any ideas on this?
> 
> BTW, what's the SPI controller connected to this chip?

The platform is MediaTek MT7688 MIPS and the SPI controller driver is
not mainlined yet. I've attached the currently used version. Please
be warned that its not mainline quality yet. This is still on our list.

Thanks,
Stefan

[-- Attachment #2: spi-mt7621.c --]
[-- Type: text/x-csrc, Size: 10678 bytes --]

/*
 * spi-mt7621.c -- MediaTek MT7621 SPI controller driver
 *
 * Copyright (C) 2011 Sergiy <piratfm@gmail.com>
 * Copyright (C) 2011-2013 Gabor Juhos <juhosg@openwrt.org>
 * Copyright (C) 2014-2015 Felix Fietkau <nbd@nbd.name>
 *
 * Some parts are based on spi-orion.c:
 *   Author: Shadi Ammouri <shadi@marvell.com>
 *   Copyright (C) 2007-2008 Marvell Ltd.
 *
 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 2 as
 * published by the Free Software Foundation.
 */

#include <linux/init.h>
#include <linux/module.h>
#include <linux/clk.h>
#include <linux/err.h>
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/reset.h>
#include <linux/spi/spi.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/swab.h>

#include <ralink_regs.h>

#define SPI_BPW_MASK(bits) BIT((bits) - 1)

#define DRIVER_NAME			"spi-mt7621"
/* in usec */
#define RALINK_SPI_WAIT_MAX_LOOP	2000

/* SPISTAT register bit field */
#define SPISTAT_BUSY			BIT(0)

#define MT7621_SPI_TRANS	0x00
#define SPITRANS_BUSY		BIT(16)

#define MT7621_SPI_OPCODE	0x04
#define MT7621_SPI_DATA0	0x08
#define MT7621_SPI_DATA4	0x18
#define SPI_CTL_TX_RX_CNT_MASK	0xff
#define SPI_CTL_START		BIT(8)

#define MT7621_SPI_POLAR	0x38
#define MT7621_SPI_MASTER	0x28
#define MT7621_SPI_MOREBUF	0x2c
#define MT7621_SPI_SPACE	0x3c

#define MT7621_CPHA		BIT(5)
#define MT7621_CPOL		BIT(4)
#define MT7621_LSB_FIRST	BIT(3)

#define RT2880_SPI_MODE_BITS	(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST | SPI_CS_HIGH)

struct mt7621_spi;

struct mt7621_spi {
	struct spi_master	*master;
	void __iomem		*base;
	unsigned int		sys_freq;
	unsigned int		speed;
	struct clk		*clk;

	struct mt7621_spi_ops	*ops;
};

static inline struct mt7621_spi *spidev_to_mt7621_spi(struct spi_device *spi)
{
	return spi_master_get_devdata(spi->master);
}

static inline u32 mt7621_spi_read(struct mt7621_spi *rs, u32 reg)
{
	return ioread32(rs->base + reg);
}

static inline void mt7621_spi_write(struct mt7621_spi *rs, u32 reg, u32 val)
{
	iowrite32(val, rs->base + reg);
}

static void mt7621_spi_reset(struct mt7621_spi *rs, int duplex)
{
	u32 master = mt7621_spi_read(rs, MT7621_SPI_MASTER);

	master |= 7 << 29;
	master |= 1 << 2;
	master &= ~(1 << 10);

	mt7621_spi_write(rs, MT7621_SPI_MASTER, master);
}

static void mt7621_spi_set_cs(struct spi_device *spi, int enable)
{
	struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
	int cs = spi->chip_select;
	u32 polar = 0;

	mt7621_spi_reset(rs, cs);

	if (enable)
		polar = BIT(cs);
	mt7621_spi_write(rs, MT7621_SPI_POLAR, polar);
}

static int mt7621_spi_prepare(struct spi_device *spi, unsigned int speed)
{
	struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
	u32 rate;
	u32 reg;

	dev_dbg(&spi->dev, "speed:%u\n", speed);

	rate = DIV_ROUND_UP(rs->sys_freq, speed);
	dev_dbg(&spi->dev, "rate-1:%u\n", rate);

	if (rate > 4097)
		return -EINVAL;

	if (rate < 2)
		rate = 2;

	reg = mt7621_spi_read(rs, MT7621_SPI_MASTER);
	reg &= ~(0xfff << 16);
	reg |= (rate - 2) << 16;
	rs->speed = speed;

	reg &= ~MT7621_LSB_FIRST;
	if (spi->mode & SPI_LSB_FIRST)
		reg |= MT7621_LSB_FIRST;

	reg &= ~(MT7621_CPHA | MT7621_CPOL);
	switch(spi->mode & (SPI_CPOL | SPI_CPHA)) {
		case SPI_MODE_0:
			break;
		case SPI_MODE_1:
			reg |= MT7621_CPHA;
			break;
		case SPI_MODE_2:
			reg |= MT7621_CPOL;
			break;
		case SPI_MODE_3:
			reg |= MT7621_CPOL | MT7621_CPHA;
			break;
	}
	mt7621_spi_write(rs, MT7621_SPI_MASTER, reg);

	return 0;
}

static inline int mt7621_spi_wait_till_ready(struct spi_device *spi)
{
	struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);
	int i;

	for (i = 0; i < RALINK_SPI_WAIT_MAX_LOOP; i++) {
		u32 status;

		status = mt7621_spi_read(rs, MT7621_SPI_TRANS);
		if ((status & SPITRANS_BUSY) == 0) {
			return 0;
		}
		cpu_relax();
		udelay(1);
	}

	return -ETIMEDOUT;
}

static int mt7621_spi_mb_transfer_half_duplex(struct spi_master *master,
						struct spi_message *m)
{
	struct mt7621_spi *rs = spi_master_get_devdata(master);
	struct spi_device *spi = m->spi;
	unsigned int speed = spi->max_speed_hz;
	struct spi_transfer *t = NULL;
	int status = 0;
	int i = 0, len = 0;
	u8 is_write = 0;
	u32 data[9] = { 0 };
	u32 val = 0;
	u32 transfer_len = 0;
	int cs_active = 0;

	mt7621_spi_wait_till_ready(spi);
	dev_dbg(&spi->dev, "seven spidev test ->cs:\n");

	list_for_each_entry(t, &m->transfers, transfer_list) {
		const u8 *txbuf = t->tx_buf;
		u8 *rxbuf = t->rx_buf;

		if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {
			dev_err(&spi->dev,
				 "message rejected: invalid transfer data buffers\n");
			status = -EIO;
			goto msg_done;
		}

		if (rxbuf)
			is_write = 0;
		else if(txbuf)
			is_write = 1;

		if (mt7621_spi_prepare(spi, speed)) {
			status = -EIO;
			goto msg_done;
		}

		transfer_len = t->len/4;
		if (!cs_active) {
			mt7621_spi_set_cs(spi, 1);
			cs_active = 1;
		}

		if(transfer_len) {   /* for word transfer */
			u32 u32TxNum = 0;

			while ( transfer_len > 0 ) {
				u32TxNum = transfer_len % 8;
				if ( !u32TxNum )
					u32TxNum = 8;

				for ( i=0; i<u32TxNum*4; i++) {
					if ( is_write ) { /* for write transfer */
						data[i / 4] |= *txbuf++ << (8 * (i & 3));
					}
					//else  /* for read transfer */
				}
#if 0
				for(i=0; i<u32TxNum*4; i += 4)
					printk("0x%x, ", data[i/4]);

				printk("\n");
#endif
				data[0] = swab32(data[0]);
				val = 0;
				if(is_write) {
					for(i=0; i<u32TxNum*4; i += 4)
						mt7621_spi_write(rs, MT7621_SPI_OPCODE + i, data[i / 4]);

					val = (min_t(int, u32TxNum*4, 4) * 8) << 24; /* must be set 32 */
					val |= ((u32TxNum*4) - 4) * 8;               /* mosi_cnt */
				} else
					val |= ((u32TxNum*4) * 8) << 12;             /* miso_cnt */

				mt7621_spi_write(rs, MT7621_SPI_MOREBUF, val);
				val = mt7621_spi_read(rs, MT7621_SPI_TRANS);
				val |= SPI_CTL_START;
				mt7621_spi_write(rs, MT7621_SPI_TRANS, val);

				mt7621_spi_wait_till_ready(spi);

				if(!is_write) {
					for (i = 0; i < u32TxNum*4; i += 4)
						data[i / 4] = mt7621_spi_read(rs, MT7621_SPI_DATA0 + i);

					for (i = 0; i < u32TxNum*4; i++)
						*rxbuf++ = data[i / 4] >> (8 * (i & 3));
				}

				len += u32TxNum*4;
				transfer_len -= u32TxNum;
				memset(data, 0, sizeof(data));
			}
		}

		transfer_len = t->len % 4;
		if(transfer_len) { /* for bytes transfer 0-3bytes*/
			for ( i=0; i<transfer_len; i++ ) {
				if(is_write)
					data[i / 4] |= *txbuf++ << (8 * (i & 3));
			}

			data[0] = swab32(data[0]);
			data[0] >>= (4 - transfer_len) * 8;

			val = 0;
			if(is_write) {
				for(i=0; i<transfer_len; i += 4)
					mt7621_spi_write(rs, MT7621_SPI_OPCODE + i, data[i / 4]);

				val = (min_t(int, transfer_len, 4) * 8) << 24;  /* must be 32 */
			} else {
				 val |= (transfer_len* 8) << 12;         /* miso_cnt */
			}
			mt7621_spi_write(rs, MT7621_SPI_MOREBUF, val);
			val = mt7621_spi_read(rs, MT7621_SPI_TRANS);
			val |= SPI_CTL_START;
			mt7621_spi_write(rs, MT7621_SPI_TRANS, val);

			mt7621_spi_wait_till_ready(spi);

			if(!is_write) {
				for (i = 0; i < transfer_len; i += 4)
					data[i / 4] = mt7621_spi_read(rs, MT7621_SPI_DATA0 + i);

				for (i = 0; i < transfer_len; i++)
					*rxbuf++ = data[i / 4] >> (8 * (i & 3));
			}
			len += transfer_len;
			memset(data, 0, sizeof(data));
		}

		m->actual_length = len;     //+ rx_len;
		if (t->cs_change) {
			 mt7621_spi_set_cs(spi, 0);
			 cs_active = 0;
		}
	}

msg_done:
	if (cs_active)
		mt7621_spi_set_cs(spi, 0);

	m->status = status;
	spi_finalize_current_message(master);

	return 0;
}

static int mt7621_spi_transfer_one_message(struct spi_master *master,
						struct spi_message *m)
{
	return mt7621_spi_mb_transfer_half_duplex(master, m);
}

static int mt7621_spi_setup(struct spi_device *spi)
{
	struct mt7621_spi *rs = spidev_to_mt7621_spi(spi);

	if ((spi->max_speed_hz == 0) ||
		(spi->max_speed_hz > (rs->sys_freq / 2)))
		spi->max_speed_hz = (rs->sys_freq / 2);

	if (spi->max_speed_hz < (rs->sys_freq / 4097)) {
		dev_err(&spi->dev, "setup: requested speed is too low %d Hz\n",
			spi->max_speed_hz);
		return -EINVAL;
	}

	return 0;
}

static const struct of_device_id mt7621_spi_match[] = {
	{ .compatible = "ralink,mt7621-spi" },
	{},
};
MODULE_DEVICE_TABLE(of, mt7621_spi_match);

static size_t mt7621_max_transfer_size(struct spi_device *spi)
{
	return 32;
}

static int mt7621_spi_probe(struct platform_device *pdev)
{
	const struct of_device_id *match;
	struct spi_master *master;
	struct mt7621_spi *rs;
	void __iomem *base;
	struct resource *r;
	int status = 0;
	struct clk *clk;
	struct mt7621_spi_ops *ops;
	int ret;

	match = of_match_device(mt7621_spi_match, &pdev->dev);
	if (!match)
		return -EINVAL;
	ops = (struct mt7621_spi_ops *)match->data;

	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	base = devm_ioremap_resource(&pdev->dev, r);
	if (IS_ERR(base))
		return PTR_ERR(base);

	clk = devm_clk_get(&pdev->dev, NULL);
	if (IS_ERR(clk)) {
		dev_err(&pdev->dev, "unable to get SYS clock, err=%d\n",
			status);
		return PTR_ERR(clk);
	}

	status = clk_prepare_enable(clk);
	if (status)
		return status;

	master = spi_alloc_master(&pdev->dev, sizeof(*rs));
	if (master == NULL) {
		dev_info(&pdev->dev, "master allocation failed\n");
		return -ENOMEM;
	}

	master->mode_bits = RT2880_SPI_MODE_BITS;

	master->setup = mt7621_spi_setup;
	master->transfer_one_message = mt7621_spi_transfer_one_message;
	master->bits_per_word_mask = SPI_BPW_MASK(8);
	master->dev.of_node = pdev->dev.of_node;
	master->num_chipselect = 2;
	master->max_transfer_size = mt7621_max_transfer_size;

	dev_set_drvdata(&pdev->dev, master);

	rs = spi_master_get_devdata(master);
	rs->base = base;
	rs->clk = clk;
	rs->master = master;
	rs->sys_freq = clk_get_rate(rs->clk);
	rs->ops = ops;
	dev_info(&pdev->dev, "sys_freq: %u\n", rs->sys_freq);

	ret = device_reset(&pdev->dev);
	if (ret) {
		dev_err(&pdev->dev, "device_reset failed (%d)!\n", ret);
		return ret;
	}

	mt7621_spi_reset(rs, 0);

	return spi_register_master(master);
}

static int mt7621_spi_remove(struct platform_device *pdev)
{
	struct spi_master *master;
	struct mt7621_spi *rs;

	master = dev_get_drvdata(&pdev->dev);
	rs = spi_master_get_devdata(master);

	clk_disable(rs->clk);
	spi_unregister_master(master);

	return 0;
}

MODULE_ALIAS("platform:" DRIVER_NAME);

static struct platform_driver mt7621_spi_driver = {
	.driver = {
		.name = DRIVER_NAME,
		.owner = THIS_MODULE,
		.of_match_table = mt7621_spi_match,
	},
	.probe = mt7621_spi_probe,
	.remove = mt7621_spi_remove,
};

module_platform_driver(mt7621_spi_driver);

MODULE_DESCRIPTION("MT7621 SPI driver");
MODULE_AUTHOR("Felix Fietkau <nbd@nbd.name>");
MODULE_LICENSE("GPL");

[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

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

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

[Stefan Roese]

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.
  
>> Here a log
>> with this version (new error this time):
>>
>> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
>> incremental biterrors test
>> 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
>>
>> Here a log with nandwrite errors:
>>
>> root@mt7688:~# flash_erase /dev/mtd5 0 1
>> Erasing 128 Kibyte @ 0 -- 100 % complete
>> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
>> Writing data to block 0 at offset 0x0
>> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
>>           error 5 (Input/output error)
>> Erasing failed write from 00000000 to 0x01ffff
> 
> Weird, I wasn't expecting ERASE to fail (nothing changed in the erase
> path).
> 
>> Writing data to block 1 at offset 0x20000
>> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 1, offset 0)
>>           error 5 (Input/output error)
>> Erasing failed write from 0x020000 to 0x03ffff
>> Writing data to block 2 at offset 0x40000
>> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 2, offset 0)
>>           error 5 (Input/output error)
>> Erasing failed write from 0x040000 to 0x05ffff
>> ...
>>
>> Any ideas on this?
> 
> Try to revert b47b307ac23d ("mtd: spinand: Use the spi-mem dirmap API").

Then I get this error again:

root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
incremental biterrors test
Failed to recover 1 bitflips
Read error after 0 bit errors per page

And with your patch from yesterday applied:

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

Thanks,
Stefan

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

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

[Boris Brezillon]

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?

>   
> >> Here a log
> >> with this version (new error this time):
> >>
> >> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> >> incremental biterrors test
> >> 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
> >>
> >> Here a log with nandwrite errors:
> >>
> >> root@mt7688:~# flash_erase /dev/mtd5 0 1
> >> Erasing 128 Kibyte @ 0 -- 100 % complete
> >> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
> >> Writing data to block 0 at offset 0x0
> >> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
> >>           error 5 (Input/output error)
> >> Erasing failed write from 00000000 to 0x01ffff  
> > 
> > Weird, I wasn't expecting ERASE to fail (nothing changed in the erase
> > path).
> >   
> >> Writing data to block 1 at offset 0x20000
> >> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 1, offset 0)
> >>           error 5 (Input/output error)
> >> Erasing failed write from 0x020000 to 0x03ffff
> >> Writing data to block 2 at offset 0x40000
> >> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 2, offset 0)
> >>           error 5 (Input/output error)
> >> Erasing failed write from 0x040000 to 0x05ffff
> >> ...
> >>
> >> Any ideas on this?  
> > 
> > Try to revert b47b307ac23d ("mtd: spinand: Use the spi-mem dirmap API").  
> 
> Then I get this error again:
> 
> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> incremental biterrors test
> Failed to recover 1 bitflips
> Read error after 0 bit errors per page
> 
> And with your patch from yesterday applied:
> 
> 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

Okay, that means we have a problem with the dirmap patch :-/.

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

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

[Boris Brezillon]

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.
>   
> >> Here a log
> >> with this version (new error this time):
> >>
> >> root@mt7688:~# ./nandbiterrs /dev/mtd5 -i
> >> incremental biterrors test
> >> 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
> >>
> >> Here a log with nandwrite errors:
> >>
> >> root@mt7688:~# flash_erase /dev/mtd5 0 1
> >> Erasing 128 Kibyte @ 0 -- 100 % complete
> >> root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
> >> Writing data to block 0 at offset 0x0
> >> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 0, offset 0)
> >>           error 5 (Input/output error)
> >> Erasing failed write from 00000000 to 0x01ffff  
> > 
> > Weird, I wasn't expecting ERASE to fail (nothing changed in the erase
> > path).
> >   
> >> Writing data to block 1 at offset 0x20000
> >> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 1, offset 0)
> >>           error 5 (Input/output error)
> >> Erasing failed write from 0x020000 to 0x03ffff
> >> Writing data to block 2 at offset 0x40000
> >> libmtd: error!: cannot write 2048 bytes to mtd5 (eraseblock 2, offset 0)
> >>           error 5 (Input/output error)

Can you find out which layer (spinand, spi-mem or the spi driver) is
returning this -EIO?

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

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

[Stefan Roese]

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.

Thanks,
Stefan

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

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

[Boris Brezillon]

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,

					^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/

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

[Boris Brezillon]

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,

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

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

[Stefan Roese]

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
root@mt7688:~# flash_erase /dev/mtd5 0 1
Erasing 128 Kibyte @ 0 -- 100 % complete
root@mt7688:~# nandwrite --input-size=2048 /dev/mtd5 /dev/urandom
Writing data to block 0 at offset 0x0
root@mt7688:~# ./nandbiterrs /dev/mtd5 -o
overwrite biterrors test
ECC failure, invalid data despite read success
Bit error histogram (0 operations total):

Thanks,
Stefan

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

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

[Boris Brezillon]

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/

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

[Stefan Roese]

On 24.01.19 12:14, Boris Brezillon wrote:
> 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,
> 

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:

[   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?

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;
         }
  
         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);
                 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;

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

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

[Boris Brezillon]

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

> On 24.01.19 12:14, Boris Brezillon wrote:
> > 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,
> >   
> 
> 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/

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

[Stefan Roese]

On 24.01.19 13:18, Boris Brezillon wrote:

<snip>

>> 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;

Sure.
  
>>           }
>>    
>>           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;
> +
> +

Double empty line.

>                  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,
> 
> 

This one works as well. Feel free to add my:

Reviewed-by: Stefan Roese <sr@denx.de>
Tested-by: Stefan Roese <sr@denx.de>

tags if appropriate.

Another question though regarding older kernel versions (e.g. v4.19 LTS).
Could you perhaps push your fix [1] from yesterday as well?

Many thanks again for all your help here,
Stefan

[1] http://lists.infradead.org/pipermail/linux-mtd/2019-January/086931.html

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

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

[Boris Brezillon]

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

> This one works as well. Feel free to add my:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> Tested-by: Stefan Roese <sr@denx.de>
> 
> tags if appropriate.

I will.

> 
> Another question though regarding older kernel versions (e.g. v4.19 LTS).
> Could you perhaps push your fix [1] from yesterday as well?

Yep, will send a separate patch with Fixes+stable tags and queue it to
the mtd/fixes branch after you have reviewed/tested it.

> 
> Many thanks again for all your help here,

And thanks for testing/debugging my mess ;-).

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

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

[Stefan Roese]

On 24.01.19 13:41, Boris Brezillon wrote:
> On Thu, 24 Jan 2019 13:28:36 +0100
> Stefan Roese <sr@denx.de> wrote:
> 
>> This one works as well. Feel free to add my:
>>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>> Tested-by: Stefan Roese <sr@denx.de>
>>
>> tags if appropriate.
> 
> I will.
> 
>>
>> Another question though regarding older kernel versions (e.g. v4.19 LTS).
>> Could you perhaps push your fix [1] from yesterday as well?
> 
> Yep, will send a separate patch with Fixes+stable tags and queue it to
> the mtd/fixes branch after you have reviewed/tested it.

Thanks. Could you perhaps also check if the U-Boot implementation also
needs a fix/change in this area?

Thanks,
Stefan

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

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

[Boris Brezillon]

On Thu, 24 Jan 2019 14:59:36 +0100
Stefan Roese <sr@denx.de> wrote:

> On 24.01.19 13:41, Boris Brezillon wrote:
> > On Thu, 24 Jan 2019 13:28:36 +0100
> > Stefan Roese <sr@denx.de> wrote:
> >   
> >> This one works as well. Feel free to add my:
> >>
> >> Reviewed-by: Stefan Roese <sr@denx.de>
> >> Tested-by: Stefan Roese <sr@denx.de>
> >>
> >> tags if appropriate.  
> > 
> > I will.
> >   
> >>
> >> Another question though regarding older kernel versions (e.g. v4.19 LTS).
> >> Could you perhaps push your fix [1] from yesterday as well?  
> > 
> > Yep, will send a separate patch with Fixes+stable tags and queue it to
> > the mtd/fixes branch after you have reviewed/tested it.  
> 
> Thanks. Could you perhaps also check if the U-Boot implementation also
> needs a fix/change in this area?

Yes it does.

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