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

Boris Brezillon
2019-01-23T07:52:18Z

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/