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

[Boris Brezillon <bbrezillon@kernel.org>]

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/