On 23.01.19 08:52, Boris Brezillon wrote:
On Wed, 23 Jan 2019 07:57:23 +0100
Stefan Roese <sr@xxxxxxx> wrote:
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 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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
