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

Boris Brezillon
2019-01-22T16:54:36Z

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)