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

From: Boris Brezillon <bbrezillon@kernel.org>
To: Stefan Roese <sr@denx.de>
Cc: Chuanhong Guo <gch981213@gmail.com>,
	linux-mtd@lists.infradead.org,
	Frieder Schrempf <frieder.schrempf@kontron.de>,
	Miquel Raynal <miquel.raynal@bootlin.com>
Subject: Re: [PATCH] mtd: spinand: Add support for GigaDevice GD5F1GQ4UC
Date: Tue, 22 Jan 2019 17:54:20 +0100	[thread overview]
Message-ID: <20190122175346.602894e4@bbrezillon> (raw)
In-Reply-To: <20190122145632.17547-1-sr@denx.de>

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/