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

Stefan Roese
2019-01-23T08:24:03Z

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