On 22.01.19 17:54, Boris Brezillon wrote:
On Tue, 22 Jan 2019 15:56:32 +0100
Stefan Roese <sr@xxxxxxx> wrote:
Add support for GigaDevice GD5F1GQ4UC SPI NAND chip.
Signed-off-by: Stefan Roese <sr@xxxxxxx>
Cc: Chuanhong Guo <gch981213@xxxxxxxxx>
Cc: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
Cc: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
