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

Stefan Roese
2019-01-23T06:57:38Z

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