| Message ID | 20210213095724.3411058-1-daniel@0x0f.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | [v2] mtd: spinand: add support for Foresee FS35ND01G-S1Y2 | expand |
Hi Daniel, Daniel Palmer <daniel@0x0f.com> wrote on Sat, 13 Feb 2021 18:57:24 +0900: > Add support for the Foresee FS35ND01G-S1Y2 manufactured by Longsys. > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > > Link: https://datasheet.lcsc.com/szlcsc/2008121142_FORESEE-FS35ND01G-S1Y2QWFI000_C719495.pdf > --- Can you please add a changelog here when you send a new version of a patch? > drivers/mtd/nand/spi/Makefile | 2 +- > drivers/mtd/nand/spi/core.c | 1 + > drivers/mtd/nand/spi/longsys.c | 86 ++++++++++++++++++++++++++++++++++ > include/linux/mtd/spinand.h | 1 + > 4 files changed, 89 insertions(+), 1 deletion(-) > create mode 100644 drivers/mtd/nand/spi/longsys.c > > diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile > index 9662b9c1d5a9..1d6819022e43 100644 > --- a/drivers/mtd/nand/spi/Makefile > +++ b/drivers/mtd/nand/spi/Makefile > @@ -1,3 +1,3 @@ > # SPDX-License-Identifier: GPL-2.0 > -spinand-objs := core.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o > +spinand-objs := core.o gigadevice.o longsys.o macronix.o micron.o paragon.o toshiba.o winbond.o > obj-$(CONFIG_MTD_SPI_NAND) += spinand.o > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index 61d932c1b718..8c36f0f6b1c9 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -864,6 +864,7 @@ static const struct nand_ops spinand_ops = { > > static const struct spinand_manufacturer *spinand_manufacturers[] = { > &gigadevice_spinand_manufacturer, > + &longsys_spinand_manufacturer, > ¯onix_spinand_manufacturer, > µn_spinand_manufacturer, > ¶gon_spinand_manufacturer, > diff --git a/drivers/mtd/nand/spi/longsys.c b/drivers/mtd/nand/spi/longsys.c > new file mode 100644 > index 000000000000..418bd5a1f20d > --- /dev/null > +++ b/drivers/mtd/nand/spi/longsys.c > @@ -0,0 +1,86 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2020 Daniel Palmer <daniel@thingy.jp> > + * > + */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/mtd/spinand.h> > + > +#define SPINAND_MFR_LONGSYS 0xCD > + > +static SPINAND_OP_VARIANTS(read_cache_variants, > + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), > + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); > + > +static SPINAND_OP_VARIANTS(write_cache_variants, > + SPINAND_PROG_LOAD(true, 0, NULL, 0)); > + > +static SPINAND_OP_VARIANTS(update_cache_variants, > + SPINAND_PROG_LOAD(false, 0, NULL, 0)); > + > +static int fs35nd01g_s1y2_ooblayout_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + if (section > 3) > + return -ERANGE; > + > + /* ECC is not user accessible */ > + region->offset = 0; > + region->length = 0; > + > + return 0; > +} > + > +static int fs35nd01g_s1y2_ooblayout_free(struct mtd_info *mtd, int section, > + struct mtd_oob_region *region) > +{ > + if (section > 3) > + return -ERANGE; > + > + /* > + * No ECC data is stored in the accessible OOB so the full 16 bytes > + * of each spare region is available to the user. Apparently also > + * covered by the internal ECC. How is this even possible? ECC must be stored somewhere, maybe it is not possible to retrieve it but I guess you cannot use the 32 bytes of OOB for user data. Can you please verify that? > + */ > + if (section) { > + region->offset = 16 * section; > + region->length = 16; > + } else { > + /* First byte in spare0 area is used for bad block marker */ > + region->offset = 1; > + region->length = 15; > + } > + > + return 0; > +} > + > +static const struct mtd_ooblayout_ops fs35nd01g_s1y2_ooblayout = { > + .ecc = fs35nd01g_s1y2_ooblayout_ecc, > + .free = fs35nd01g_s1y2_ooblayout_free, > +}; > + > +static const struct spinand_info longsys_spinand_table[] = { > + SPINAND_INFO("FS35ND01G-S1Y2", > + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xEA, 0x11), > + NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), > + NAND_ECCREQ(4, 512), > + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, > + &write_cache_variants, > + &update_cache_variants), > + SPINAND_HAS_QE_BIT, > + SPINAND_ECCINFO(&fs35nd01g_s1y2_ooblayout, > + NULL)), > +}; > + > +static const struct spinand_manufacturer_ops longsys_spinand_manuf_ops = { > +}; > + > +const struct spinand_manufacturer longsys_spinand_manufacturer = { > + .id = SPINAND_MFR_LONGSYS, > + .name = "Longsys", > + .chips = longsys_spinand_table, > + .nchips = ARRAY_SIZE(longsys_spinand_table), > + .ops = &longsys_spinand_manuf_ops, > +}; > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index 6bb92f26833e..8651e63a2f8a 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -239,6 +239,7 @@ struct spinand_manufacturer { > > /* SPI NAND manufacturers */ > extern const struct spinand_manufacturer gigadevice_spinand_manufacturer; > +extern const struct spinand_manufacturer longsys_spinand_manufacturer; > extern const struct spinand_manufacturer macronix_spinand_manufacturer; > extern const struct spinand_manufacturer micron_spinand_manufacturer; > extern const struct spinand_manufacturer paragon_spinand_manufacturer; Thanks, Miquèl
Hi Miquel, On Mon, 15 Feb 2021 at 19:24, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Can you please add a changelog here when you send a new version of a > patch? Sorry, I was going to add a cover letter but elsewhere got told that one isn't needed for a single patch.. Basically I changed FS35ND01G to FS35ND01G-S1Y2 as that's the proper part number for the chip I have and there seem to be a few variations of this. Aside from that I fixed up the hex numbers to be uppercase and added the oob layout callbacks. > > +static int fs35nd01g_s1y2_ooblayout_free(struct mtd_info *mtd, int section, > > + struct mtd_oob_region *region) > > +{ > > + if (section > 3) > > + return -ERANGE; > > + > > + /* > > + * No ECC data is stored in the accessible OOB so the full 16 bytes > > + * of each spare region is available to the user. Apparently also > > + * covered by the internal ECC. > > How is this even possible? ECC must be stored somewhere, maybe it is > not possible to retrieve it but I guess you cannot use the 32 bytes of > OOB for user data. Can you please verify that? This worried me too as I could not find the OOB layout anywhere. They simply list there being 4 512 byte main areas and then 4 16 byte spare areas. The only other note is that the first byte of spare0 is used for the bad block marker. I contacted Longsys but they didn't get back to me. So what I did here was I started googling strings within the datasheet to find other chips that are probably the same IP inside and I found the FM25G01. It's datasheet shares a lot of the same text and the flash layout diagrams etc are the same. It has the same table for the flash layout. 4 512 byte areas and 4 16 byte spare areas. It has the same note for the bad block marker and then one additional note: "2. Spare area 800H to 83FH is all available for user. ECC parity codes are programmed in additional space and not user accessible." It would seem that the pages are actually bigger than 2K + 64 or there is some other place they keep the ECC. Or both datasheets are lying. Somewhere else in the datasheets it says that writes to the ECC area will be ignored but that doesn't make a lot of sense if the ECC area isn't user accessible in the first place. I didn't think about it at the time but I can take a dump of the OOB area of my FS35ND01G-S1Y2 to confirm it's all 0xff except for any factory marked bad blocks. Thanks, Daniel
Hi Daniel, Daniel Palmer <daniel@0x0f.com> wrote on Mon, 15 Feb 2021 19:53:13 +0900: > Hi Miquel, > > On Mon, 15 Feb 2021 at 19:24, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Can you please add a changelog here when you send a new version of a > > patch? > > Sorry, I was going to add a cover letter but elsewhere got told that > one isn't needed for a single patch.. A cover letter is useful when there are many patches, or when there is some context that is important to remember. But a changelog should always be added when you change something between two versions. And the changelog can be located below the three dashes ("---") without being part of the final commit message, it does not need to be in a separate cover letter. > Basically I changed FS35ND01G to FS35ND01G-S1Y2 as that's the proper > part number for the chip I have and there seem to be a few variations > of this. > Aside from that I fixed up the hex numbers to be uppercase and added > the oob layout callbacks. > > > > +static int fs35nd01g_s1y2_ooblayout_free(struct mtd_info *mtd, int section, > > > + struct mtd_oob_region *region) > > > +{ > > > + if (section > 3) > > > + return -ERANGE; > > > + > > > + /* > > > + * No ECC data is stored in the accessible OOB so the full 16 bytes > > > + * of each spare region is available to the user. Apparently also > > > + * covered by the internal ECC. > > > > How is this even possible? ECC must be stored somewhere, maybe it is > > not possible to retrieve it but I guess you cannot use the 32 bytes of > > OOB for user data. Can you please verify that? > > This worried me too as I could not find the OOB layout anywhere. > They simply list there being 4 512 byte main areas and then 4 16 byte > spare areas. The only other note is that the first byte of spare0 is > used for the bad block marker. > > I contacted Longsys but they didn't get back to me. > So what I did here was I started googling strings within the datasheet > to find other chips that are probably the same IP inside and I found > the FM25G01. > It's datasheet shares a lot of the same text and the flash layout > diagrams etc are the same. > It has the same table for the flash layout. 4 512 byte areas and 4 16 > byte spare areas. It has the same note for the bad block marker and > then one additional note: > > "2. Spare area 800H to 83FH is all available for user. > ECC parity codes are programmed in > additional space and not user accessible." > > It would seem that the pages are actually bigger than 2K + 64 or there > is some other place they keep the ECC. > Or both datasheets are lying. Somewhere else in the datasheets it says > that writes to the ECC area will be ignored but that doesn't make a > lot of sense if the ECC area isn't user accessible in the first place. > > I didn't think about it at the time but I can take a dump of the OOB > area of my FS35ND01G-S1Y2 to confirm it's all 0xff except for any > factory marked bad blocks. I see. Can you please try the following: nandwrite -o /dev/mtdx /dev/zero nanddump -ol1 /dev/mtdx If the entire area is effectively free to be used, you should see 0's everywhere. Otherwise you should have ff's somewhere. Thanks, Miquèl
Hi Miquel, On Mon, 15 Feb 2021 at 20:16, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > But a changelog should always be added when you change something > between two versions. And the changelog can be located below the three > dashes ("---") without being part of the final commit message, it does > not need to be in a separate cover letter. Oh. I didn't even realise that feature existed. :) > I see. Can you please try the following: > > nandwrite -o /dev/mtdx /dev/zero > nanddump -ol1 /dev/mtdx > > If the entire area is effectively free to be used, you should see 0's > everywhere. Otherwise you should have ff's somewhere. Ok, I will confirm this and get back to you. Thanks, Daniel
Hi Miquel, Sorry for the resend. Gmail randomly switched to HTML email so the original version seems to have bounced. On Mon, 15 Feb 2021 at 20:16, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > "2. Spare area 800H to 83FH is all available for user. > > ECC parity codes are programmed in > > additional space and not user accessible." > > > > It would seem that the pages are actually bigger than 2K + 64 or there > > is some other place they keep the ECC. > > Or both datasheets are lying. Somewhere else in the datasheets it says > > that writes to the ECC area will be ignored but that doesn't make a > > lot of sense if the ECC area isn't user accessible in the first place. > > > > I didn't think about it at the time but I can take a dump of the OOB > > area of my FS35ND01G-S1Y2 to confirm it's all 0xff except for any > > factory marked bad blocks. > > I see. Can you please try the following: > > nandwrite -o /dev/mtdx /dev/zero > nanddump -ol1 /dev/mtdx > If the entire area is effectively free to be used, you should see 0's > everywhere. Otherwise you should have ff's somewhere. Sorry I didn't follow up sooner on this. I needed to order another of this flash chip to test with as I couldn't destroy the data on the one I have. Anyhow: Erased the page with flash erase (I'm forcing it to erase bad blocks here as I mess up the marker, I have a hack to allow erasing bad blocks..) Everything is 0xFF for that page. # flash_erase -N /dev/mtd1 0 1 Erasing 128 Kibyte @ 0 -- 100 % complete # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 Block size 131072, page size 2048, OOB size 64 Dumping data starting at 0x00000000 and ending at 0x00000800... 0x00000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| .... 0x000007f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| Write zeros into the page and OOB. Get all zeros back including the OOB. # nandwrite -o /dev/mtd1 /dev/zero Writing data to block 0 at offset 0x0 Bad block at 0, 1 block(s) will be skipped Writing data to block 1 at offset 0x20000 # nandwrite -N -o /dev/mtd1 /dev/zero Writing data to block 0 at offset 0x0 # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 Block size 131072, page size 2048, OOB size 64 Dumping data starting at 0x00000000 and ending at 0x00000800... 0x00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| ... 0x000007f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| Erase the page again and writing random junk into it. Seeing random junk everywhere including the OOB. # flash_erase -N /dev/mtd1 0 1 Erasing 128 Kibyte @ 0 -- 100 % complete # nandwrite -N -o /dev/mtd1 /dev/urandom Writing data to [ 230.506260] random: nandwrite: uninitialized urandom read (2048 bytes read) block 0 at offse[ 230.514705] random: nandwrite: uninitialized urandom read (64 bytes read) t 0x0 # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 Block size 131072, page size 2048, OOB size 64 Dumping data starting at 0x00000000 and ending at 0x00000800... 0x00000000: 5e 24 bd 5f d9 c6 ce c5 b1 85 52 4d 27 94 c9 98 |^$._......RM'...| ... 0x000007f0: fa 9f 7f 7d ce 99 33 88 d6 9f 99 7d 84 e7 0c 4d |...}..3....}...M| OOB Data: b1 81 07 6a 8d 47 8b ed 89 88 ac 62 e8 ae 48 54 |...j.G.....b..HT| OOB Data: 7d b2 ea 73 f3 29 ba 65 e6 45 cb 8b 1a c6 5b dc |}..s.).e.E....[.| OOB Data: b2 2e 77 56 e0 e1 04 59 86 31 7a e5 bd 43 f9 48 |..wV...Y.1z..C.H| OOB Data: 52 05 b2 f1 65 64 59 22 79 50 ec 89 55 6b 6e 23 |R...edY"yP..Ukn#| I think this shows that the datasheet is right in that the complete 64 bytes of "spare area" is usable. I have no idea where it puts the ECC though. :) Thanks, Daniel
Hi Daniel, Daniel Palmer <daniel@0x0f.com> wrote on Mon, 22 Mar 2021 21:44:40 +0900: > Hi Miquel, > > Sorry for the resend. Gmail randomly switched to HTML email so the > original version seems to have bounced. > > On Mon, 15 Feb 2021 at 20:16, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > "2. Spare area 800H to 83FH is all available for user. > > > ECC parity codes are programmed in > > > additional space and not user accessible." > > > > > > It would seem that the pages are actually bigger than 2K + 64 or there > > > is some other place they keep the ECC. > > > Or both datasheets are lying. Somewhere else in the datasheets it says > > > that writes to the ECC area will be ignored but that doesn't make a > > > lot of sense if the ECC area isn't user accessible in the first place. > > > > > > I didn't think about it at the time but I can take a dump of the OOB > > > area of my FS35ND01G-S1Y2 to confirm it's all 0xff except for any > > > factory marked bad blocks. > > > > I see. Can you please try the following: > > > > nandwrite -o /dev/mtdx /dev/zero > > nanddump -ol1 /dev/mtdx > > If the entire area is effectively free to be used, you should see 0's > > everywhere. Otherwise you should have ff's somewhere. > > Sorry I didn't follow up sooner on this. I needed to order another of > this flash chip to test with as I couldn't destroy the data on the one > I have. > > Anyhow: > > Erased the page with flash erase (I'm forcing it to erase bad blocks > here as I mess up the marker, I have a hack to allow erasing bad > blocks..) > Everything is 0xFF for that page. > > # flash_erase -N /dev/mtd1 0 1 > Erasing 128 Kibyte @ 0 -- 100 % complete > # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 > Block size 131072, page size 2048, OOB size 64 > Dumping data starting at 0x00000000 and ending at 0x00000800... > 0x00000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| > .... > 0x000007f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| > OOB Data: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................| > > Write zeros into the page and OOB. > Get all zeros back including the OOB. > > # nandwrite -o /dev/mtd1 /dev/zero > Writing data to block 0 at offset 0x0 > Bad block at 0, 1 block(s) will be skipped > Writing data to block 1 at offset 0x20000 > # nandwrite -N -o /dev/mtd1 /dev/zero > Writing data to block 0 at offset 0x0 > # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 > Block size 131072, page size 2048, OOB size 64 > Dumping data starting at 0x00000000 and ending at 0x00000800... > 0x00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| > ... > 0x000007f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................| > OOB Data: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > |................| > > Erase the page again and writing random junk into it. > Seeing random junk everywhere including the OOB. > > # flash_erase -N /dev/mtd1 0 1 > Erasing 128 Kibyte @ 0 -- 100 % complete > # nandwrite -N -o /dev/mtd1 /dev/urandom > Writing data to [ 230.506260] random: nandwrite: uninitialized > urandom read (2048 bytes read) > block 0 at offse[ 230.514705] random: nandwrite: uninitialized > urandom read (64 bytes read) > t 0x0 > # nanddump --bb=dumpbad -n -l2048 -o -c -s 0x0 /dev/mtd1 > Block size 131072, page size 2048, OOB size 64 > Dumping data starting at 0x00000000 and ending at 0x00000800... > 0x00000000: 5e 24 bd 5f d9 c6 ce c5 b1 85 52 4d 27 94 c9 98 |^$._......RM'...| > ... > 0x000007f0: fa 9f 7f 7d ce 99 33 88 d6 9f 99 7d 84 e7 0c 4d |...}..3....}...M| > OOB Data: b1 81 07 6a 8d 47 8b ed 89 88 ac 62 e8 ae 48 54 |...j.G.....b..HT| > OOB Data: 7d b2 ea 73 f3 29 ba 65 e6 45 cb 8b 1a c6 5b dc |}..s.).e.E....[.| > OOB Data: b2 2e 77 56 e0 e1 04 59 86 31 7a e5 bd 43 f9 48 |..wV...Y.1z..C.H| > OOB Data: 52 05 b2 f1 65 64 59 22 79 50 ec 89 55 6b 6e 23 |R...edY"yP..Ukn#| > > I think this shows that the datasheet is right in that the complete 64 > bytes of "spare area" is usable. > I have no idea where it puts the ECC though. :) Argh, I don't like when hardware tries to be smart. Ok then let's declare no ECC bytes in the OOB layout, I guess it's the best thing to do... Thanks for checking btw! I don't recall the state of the patch which triggered this discussion, so I guess it's a good time to respin. Cheers, Miquèl
Hi Miquel, On Tue, 23 Mar 2021 at 03:32, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > I think this shows that the datasheet is right in that the complete 64 > > bytes of "spare area" is usable. > > I have no idea where it puts the ECC though. :) > > Argh, I don't like when hardware tries to be smart. I'm sort of worried that there just isn't any ECC :D > Ok then let's declare no ECC bytes in the OOB layout, I guess it's the > best thing to do... Ok. I think I might add a comment in the code that this is a best guess. > I don't recall the state of the patch which triggered this discussion, > so I guess it's a good time to respin. I'll resend it. I've added support for the 2Gb version in the meantime so I need to resend it either way. Thanks, Daniel
Hi Daniel, Daniel Palmer <daniel@0x0f.com> wrote on Tue, 23 Mar 2021 18:33:54 +0900: > Hi Miquel, > > On Tue, 23 Mar 2021 at 03:32, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > I think this shows that the datasheet is right in that the complete 64 > > > bytes of "spare area" is usable. > > > I have no idea where it puts the ECC though. :) > > > > Argh, I don't like when hardware tries to be smart. > > I'm sort of worried that there just isn't any ECC :D You can run nandbiterrs -i /dev/mtdX You'll see if there is ECC correction or not (and its level). Thanks, Miquèl
Hi Miquel, On Tue, 23 Mar 2021 at 19:32, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > You can run nandbiterrs -i /dev/mtdX > > You'll see if there is ECC correction or not (and its level). These are results I get for both of the nandbiterrs tests. # nandbiterrs -i /dev/mtd1 incremental biterrors test Successfully corrected 0 bit errors per subpage Inserted biterror @ 0/5 Read reported 4 corrected bit errors ECC failure, invalid data despite read success # nandbiterrs -o /dev/mtd1 overwrite biterrors test Read reported 4 corrected bit errors Failed to recover 1 bitflips Bit error histogram (883 operations total): Page reads with 0 corrected bit errors: 845 Page reads with 1 corrected bit errors: 0 Page reads with 2 corrected bit errors: 0 Page reads with 3 corrected bit errors: 0 I guess this means ECC is there and it's working. Thanks, Daniel
Hi Daniel, Daniel Palmer <daniel@0x0f.com> wrote on Tue, 23 Mar 2021 20:47:10 +0900: > Hi Miquel, > > On Tue, 23 Mar 2021 at 19:32, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > You can run nandbiterrs -i /dev/mtdX > > > > You'll see if there is ECC correction or not (and its level). > > These are results I get for both of the nandbiterrs tests. > > # nandbiterrs -i /dev/mtd1 > incremental biterrors test > Successfully corrected 0 bit errors per subpage > Inserted biterror @ 0/5 > Read reported 4 corrected bit errors > ECC failure, invalid data despite read success This is not a valid behavior. There is something wrong with the way ECC status is read/retrieved. The read should indeed report 4 corrected bit errors, but then the data should be valid. Here it means that the introduced error appears corrected but in fact is not. We need to understand what status are available and write the appropriate vendor code. Thanks, Miquèl
Hi Miquel, On Tue, 23 Mar 2021 at 23:06, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > # nandbiterrs -i /dev/mtd1 > > incremental biterrors test > > Successfully corrected 0 bit errors per subpage > > Inserted biterror @ 0/5 > > Read reported 4 corrected bit errors > > ECC failure, invalid data despite read success > > This is not a valid behavior. There is something wrong with the way ECC > status is read/retrieved. The read should indeed report 4 corrected bit > errors, but then the data should be valid. Here it means that the > introduced error appears corrected but in fact is not. > > We need to understand what status are available and write the > appropriate vendor code. Ok. Glad we checked then :). Before sending again I will recheck how the ECC status is read and fix that up. Thanks, Daniel
Hi Miquel, Sorry for the constant pestering on this.. On Tue, 23 Mar 2021 at 23:06, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > # nandbiterrs -i /dev/mtd1 > > incremental biterrors test > > Successfully corrected 0 bit errors per subpage > > Inserted biterror @ 0/5 > > Read reported 4 corrected bit errors > > ECC failure, invalid data despite read success > > This is not a valid behavior. There is something wrong with the way ECC > status is read/retrieved. The read should indeed report 4 corrected bit > errors, but then the data should be valid. Here it means that the > introduced error appears corrected but in fact is not. > We need to understand what status are available and write the > appropriate vendor code. I looked at the datasheet again and the encoding for ecc status seems a bit funky. The chip seems to have only two bits for ECC status with this encoding: 0 - Read success with 0-3 flipped bits. 1 - Read success with 4 flipped bits. 2 - Uncorrectable. 3 - Reserved. This looks almost the same as the Winbond chips with 0 and 1 being successes but with no totally error free status. Anyhow, if 4 bits were corrected is returned for 1 then nandbiterrs reports "ECC failure, invalid data despite read success". If I return 3 bit errors for 0 and -EBADMSG for anything else nandbiterrs doesn't complain anymore but is this right?: # nandbiterrs -i /dev/mtd1 incremental biterrors test Read reported 3 corrected bit errors Successfully corrected 0 bit errors per subpage biterror @ 0/5 Failed to recover 1 bitflips Read error after 1 bit errors per page When running nandbiterrs the status is always 0 before injecting the error and then sometimes 1 and sometimes 2 after the error is injected. Thanks, Daniel
Hi Daniel, Daniel Palmer <daniel@0x0f.com> wrote on Fri, 26 Mar 2021 23:09:28 +0900: > Hi Miquel, > > Sorry for the constant pestering on this.. > > On Tue, 23 Mar 2021 at 23:06, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > # nandbiterrs -i /dev/mtd1 > > > incremental biterrors test > > > Successfully corrected 0 bit errors per subpage > > > Inserted biterror @ 0/5 > > > Read reported 4 corrected bit errors > > > ECC failure, invalid data despite read success > > > > This is not a valid behavior. There is something wrong with the way ECC > > status is read/retrieved. The read should indeed report 4 corrected bit > > errors, but then the data should be valid. Here it means that the > > introduced error appears corrected but in fact is not. > > We need to understand what status are available and write the > > appropriate vendor code. > > I looked at the datasheet again and the encoding for ecc status seems > a bit funky. > The chip seems to have only two bits for ECC status with this encoding: > 0 - Read success with 0-3 flipped bits. > 1 - Read success with 4 flipped bits. > 2 - Uncorrectable. > 3 - Reserved. > Very nice information. > This looks almost the same as the Winbond chips with 0 and 1 being successes > but with no totally error free status. > > Anyhow, if 4 bits were corrected is returned for 1 then nandbiterrs > reports "ECC failure, invalid data despite read success". > If I return 3 bit errors for 0 and -EBADMSG for anything else > nandbiterrs doesn't complain anymore but is this right?: You may look at micron_8_ecc_get_status() helper to guide you. But IMHO, if there are 0-3 bf, you should probably assume there were 3 bf and return 3, if there were 4, return 4, if it's uncorrectable return -EBADMSG otherwise -EINVAL. We should verify that this does not mess with UBI wear leveling though. Please check that returning 3-bit errors no matter the actual number of flipped bits does not lead UBI to move the data away (I think it's fine but we need to be sure otherwise the implementation proposal is not valid). Thanks, Miquèl
Hi Miquel, On Wed, 7 Apr 2021 at 17:02, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > You may look at micron_8_ecc_get_status() helper to guide you. But > IMHO, if there are 0-3 bf, you should probably assume there were 3 bf > and return 3, if there were 4, return 4, if it's uncorrectable return > -EBADMSG otherwise -EINVAL. Understood. > We should verify that this does not mess with UBI wear leveling > though. Please check that returning 3-bit errors no matter the > actual number of flipped bits does not lead UBI to move the data away > (I think it's fine but we need to be sure otherwise the implementation > proposal is not valid). Ok. I'm not sure how to check that yet but I'll look into it. Thanks for all of the help on this. Cheers, Daniel
Hi Daniel, Daniel Palmer <daniel@0x0f.com> wrote on Wed, 7 Apr 2021 21:01:01 +0900: > Hi Miquel, > > On Wed, 7 Apr 2021 at 17:02, Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > You may look at micron_8_ecc_get_status() helper to guide you. But > > IMHO, if there are 0-3 bf, you should probably assume there were 3 bf > > and return 3, if there were 4, return 4, if it's uncorrectable return > > -EBADMSG otherwise -EINVAL. > > Understood. > > > We should verify that this does not mess with UBI wear leveling > > though. Please check that returning 3-bit errors no matter the > > actual number of flipped bits does not lead UBI to move the data away > > (I think it's fine but we need to be sure otherwise the implementation > > proposal is not valid). > > Ok. I'm not sure how to check that yet but I'll look into it. > You can probably check the threshold in sysfs (/sys/class/mtd/mtdX/*threshold*). Thanks, Miquèl
diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile index 9662b9c1d5a9..1d6819022e43 100644 --- a/drivers/mtd/nand/spi/Makefile +++ b/drivers/mtd/nand/spi/Makefile @@ -1,3 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 -spinand-objs := core.o gigadevice.o macronix.o micron.o paragon.o toshiba.o winbond.o +spinand-objs := core.o gigadevice.o longsys.o macronix.o micron.o paragon.o toshiba.o winbond.o obj-$(CONFIG_MTD_SPI_NAND) += spinand.o diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 61d932c1b718..8c36f0f6b1c9 100644 --- a/drivers/mtd/nand/spi/core.c +++ b/drivers/mtd/nand/spi/core.c @@ -864,6 +864,7 @@ static const struct nand_ops spinand_ops = { static const struct spinand_manufacturer *spinand_manufacturers[] = { &gigadevice_spinand_manufacturer, + &longsys_spinand_manufacturer, ¯onix_spinand_manufacturer, µn_spinand_manufacturer, ¶gon_spinand_manufacturer, diff --git a/drivers/mtd/nand/spi/longsys.c b/drivers/mtd/nand/spi/longsys.c new file mode 100644 index 000000000000..418bd5a1f20d --- /dev/null +++ b/drivers/mtd/nand/spi/longsys.c @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2020 Daniel Palmer <daniel@thingy.jp> + * + */ + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/mtd/spinand.h> + +#define SPINAND_MFR_LONGSYS 0xCD + +static SPINAND_OP_VARIANTS(read_cache_variants, + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0), + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0)); + +static SPINAND_OP_VARIANTS(write_cache_variants, + SPINAND_PROG_LOAD(true, 0, NULL, 0)); + +static SPINAND_OP_VARIANTS(update_cache_variants, + SPINAND_PROG_LOAD(false, 0, NULL, 0)); + +static int fs35nd01g_s1y2_ooblayout_ecc(struct mtd_info *mtd, int section, + struct mtd_oob_region *region) +{ + if (section > 3) + return -ERANGE; + + /* ECC is not user accessible */ + region->offset = 0; + region->length = 0; + + return 0; +} + +static int fs35nd01g_s1y2_ooblayout_free(struct mtd_info *mtd, int section, + struct mtd_oob_region *region) +{ + if (section > 3) + return -ERANGE; + + /* + * No ECC data is stored in the accessible OOB so the full 16 bytes + * of each spare region is available to the user. Apparently also + * covered by the internal ECC. + */ + if (section) { + region->offset = 16 * section; + region->length = 16; + } else { + /* First byte in spare0 area is used for bad block marker */ + region->offset = 1; + region->length = 15; + } + + return 0; +} + +static const struct mtd_ooblayout_ops fs35nd01g_s1y2_ooblayout = { + .ecc = fs35nd01g_s1y2_ooblayout_ecc, + .free = fs35nd01g_s1y2_ooblayout_free, +}; + +static const struct spinand_info longsys_spinand_table[] = { + SPINAND_INFO("FS35ND01G-S1Y2", + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xEA, 0x11), + NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1), + NAND_ECCREQ(4, 512), + SPINAND_INFO_OP_VARIANTS(&read_cache_variants, + &write_cache_variants, + &update_cache_variants), + SPINAND_HAS_QE_BIT, + SPINAND_ECCINFO(&fs35nd01g_s1y2_ooblayout, + NULL)), +}; + +static const struct spinand_manufacturer_ops longsys_spinand_manuf_ops = { +}; + +const struct spinand_manufacturer longsys_spinand_manufacturer = { + .id = SPINAND_MFR_LONGSYS, + .name = "Longsys", + .chips = longsys_spinand_table, + .nchips = ARRAY_SIZE(longsys_spinand_table), + .ops = &longsys_spinand_manuf_ops, +}; diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h index 6bb92f26833e..8651e63a2f8a 100644 --- a/include/linux/mtd/spinand.h +++ b/include/linux/mtd/spinand.h @@ -239,6 +239,7 @@ struct spinand_manufacturer { /* SPI NAND manufacturers */ extern const struct spinand_manufacturer gigadevice_spinand_manufacturer; +extern const struct spinand_manufacturer longsys_spinand_manufacturer; extern const struct spinand_manufacturer macronix_spinand_manufacturer; extern const struct spinand_manufacturer micron_spinand_manufacturer; extern const struct spinand_manufacturer paragon_spinand_manufacturer;
Add support for the Foresee FS35ND01G-S1Y2 manufactured by Longsys. Signed-off-by: Daniel Palmer <daniel@0x0f.com> Link: https://datasheet.lcsc.com/szlcsc/2008121142_FORESEE-FS35ND01G-S1Y2QWFI000_C719495.pdf --- drivers/mtd/nand/spi/Makefile | 2 +- drivers/mtd/nand/spi/core.c | 1 + drivers/mtd/nand/spi/longsys.c | 86 ++++++++++++++++++++++++++++++++++ include/linux/mtd/spinand.h | 1 + 4 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 drivers/mtd/nand/spi/longsys.c