| Message ID | 20180617204605.4648-1-stefan@agner.ch |
|---|---|
| Headers | show |
| Series | mtd: rawnand: add NVIDIA Tegra NAND flash support | expand |
On Sun, 17 Jun 2018 22:45:59 +0200 Stefan Agner <stefan@agner.ch> wrote: > Changes definitly calm down, most noteably probably the changes > around checking whether a page is empty if the stack reports ECC > errors.. I verified the code using raw nandwrites with OOB to > simulate an empty page which has some bits flipped in the OOB area, > everthing seems to work as I would expect it. > > For now I do not check extra OOB bytes since those are at variable > locations depending on algorithm. Hm, if you expose them as free OOB bytes, you should also check them, otherwise you might end up with corrupted data without noticing it. Note that, depending on whether those free OOB bytes are ECC-protected or not, you should change the way you do the check: - non-protected OOB bytes: all bytes should be 0xff (no bitflips allowed) - data+free OOB bytes protected by the same ECC bytes: you should pass the free OOB bytes buffer to nand_check_erased_ecc_chunk() along with the data and ECC buffers - free OOB bytes have their own ECC bytes: call nand_check_erased_ecc_chunk() separately and pass it the ECC + free OOB buffers. > > -- > Stefan > > Changes since v1: > - Split controller and NAND chip structure > - Add BCH support > - Allow to select algorithm and strength using device tree > - Improve HW ECC error reporting and use DEC_STATUS_BUF only > - Use SPDX license identifier > - Use per algorithm mtd_ooblayout_ops > - Use setup_data_interface callback for NAND timing configuration > > Changes since v2: > - Set clock rate using assigned-clocks > - Use BIT() macro > - Fix and improve timing calculation > - Improve ECC error handling > - Store OOB layout for tag area in Tegra chip structure > - Update/fix bindings > - Use more specific variable names (replace "value") > - Introduce nand-is-boot-medium > - Choose sensible ECC strenght automatically > - Use wait_for_completion_timeout > - Print register dump on completion timeout > - Unify tegra_nand_(read|write)_page in tegra_nand_page_xfer > > Changes since v3: > - Implement tegra_nand_(read|write)_raw using DMA > - Implement tegra_nand_(read|write)_oob using DMA > - Name registers according to Tegra 2 Technical Reference Manual (v02p) > - Use wait_for_completion_io_timeout to account for IO > - Get chip select id from device tree reg property > - Clear interrupts and reinit wait queues in case command/DMA times out > - Set default MTD name after nand_set_flash_node > - Move MODULE_DEVICE_TABLE after declaration of tegra_nand_of_match > - Make (rs|bch)_strength static > > Changes since v4: > - Pass OOB area to nand_check_erased_ecc_chunk > - Pass algorithm specific bits_per_step to tegra_nand_get_strength > - Store ECC layout in chip structure > - Fix pointer assignment (use NULL) > - Removed obsolete header delay.h > - Fixed newlines > - Use non-_io variant of wait_for_completion_timeout > > Lucas Stach (1): > ARM: dts: tegra: add Tegra20 NAND flash controller node > > Stefan Agner (5): > mtd: rawnand: add Reed-Solomon error correction algorithm > mtd: rawnand: add an option to specify NAND chip as a boot device > mtd: rawnand: tegra: add devicetree binding > mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver > ARM: dts: tegra: enable NAND flash on Colibri T20 > > .../devicetree/bindings/mtd/nand.txt | 6 +- > .../bindings/mtd/nvidia-tegra20-nand.txt | 64 + > MAINTAINERS | 7 + > arch/arm/boot/dts/tegra20-colibri-512.dtsi | 16 + > arch/arm/boot/dts/tegra20.dtsi | 15 + > drivers/mtd/nand/raw/Kconfig | 6 + > drivers/mtd/nand/raw/Makefile | 1 + > drivers/mtd/nand/raw/nand_base.c | 4 + > drivers/mtd/nand/raw/tegra_nand.c | 1268 +++++++++++++++++ > include/linux/mtd/rawnand.h | 7 + > 10 files changed, 1393 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/mtd/nvidia-tegra20-nand.txt > create mode 100644 drivers/mtd/nand/raw/tegra_nand.c > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18.06.2018 11:58, Boris Brezillon wrote: > On Sun, 17 Jun 2018 22:45:59 +0200 > Stefan Agner <stefan@agner.ch> wrote: > >> Changes definitly calm down, most noteably probably the changes >> around checking whether a page is empty if the stack reports ECC >> errors.. I verified the code using raw nandwrites with OOB to >> simulate an empty page which has some bits flipped in the OOB area, >> everthing seems to work as I would expect it. >> >> For now I do not check extra OOB bytes since those are at variable >> locations depending on algorithm. > > Hm, if you expose them as free OOB bytes, you should also check them, > otherwise you might end up with corrupted data without noticing it. Note > that, depending on whether those free OOB bytes are ECC-protected or > not, you should change the way you do the check: > > - non-protected OOB bytes: all bytes should be 0xff (no bitflips > allowed) > - data+free OOB bytes protected by the same ECC bytes: you should pass > the free OOB bytes buffer to nand_check_erased_ecc_chunk() along with > the data and ECC buffers > - free OOB bytes have their own ECC bytes: call > nand_check_erased_ecc_chunk() separately and pass it the ECC + free > OOB buffers. This graphic taken from the public Tegra 2 Technical Reference Manual is quite useful: https://imgur.com/a/0Hqzbkc Tegra basically has all of the above, which makes the whole business really tricky... I am not sure if we really could do variant 1, non-protected OOB, but since we have the option of protected OOB, we probably anyway would do that. RS/Hamming implements variant 3. BCH implements variant 2. OOB is protected with the last data buffer. So this would require a algorithm depending implementation, which is probably not a big deal. But there is one more issue with BCH: Only if extra data are actually transferred, tag space is actually allocated. If no tag bytes are transferred, parity follows immediately skip bytes. As far as I know the MTD stacks OOB layout assumes that is always the same layout, no matter whether we write extra OOB data or not. For the Tegra NAND controller this would mean that we have to always transfer tag bytes and therefor penalize the use case we are most interested in (which is no extra OOB bytes, since UBI does not make use of it)... Furthermore I realized that testing is not easily possible since nandwrite with --oob seems not to make use of "oob_required" in the main page write but issues a separate OOB write command. I did not found a way to issue a write from user space which sets oob_required... Due to all this I rather prefer to not implement extra OOB support at this point. How do I do this properly? Set mtd_ooblayout_ops.free to NULL? -- Stefan > >> >> -- >> Stefan >> >> Changes since v1: >> - Split controller and NAND chip structure >> - Add BCH support >> - Allow to select algorithm and strength using device tree >> - Improve HW ECC error reporting and use DEC_STATUS_BUF only >> - Use SPDX license identifier >> - Use per algorithm mtd_ooblayout_ops >> - Use setup_data_interface callback for NAND timing configuration >> >> Changes since v2: >> - Set clock rate using assigned-clocks >> - Use BIT() macro >> - Fix and improve timing calculation >> - Improve ECC error handling >> - Store OOB layout for tag area in Tegra chip structure >> - Update/fix bindings >> - Use more specific variable names (replace "value") >> - Introduce nand-is-boot-medium >> - Choose sensible ECC strenght automatically >> - Use wait_for_completion_timeout >> - Print register dump on completion timeout >> - Unify tegra_nand_(read|write)_page in tegra_nand_page_xfer >> >> Changes since v3: >> - Implement tegra_nand_(read|write)_raw using DMA >> - Implement tegra_nand_(read|write)_oob using DMA >> - Name registers according to Tegra 2 Technical Reference Manual (v02p) >> - Use wait_for_completion_io_timeout to account for IO >> - Get chip select id from device tree reg property >> - Clear interrupts and reinit wait queues in case command/DMA times out >> - Set default MTD name after nand_set_flash_node >> - Move MODULE_DEVICE_TABLE after declaration of tegra_nand_of_match >> - Make (rs|bch)_strength static >> >> Changes since v4: >> - Pass OOB area to nand_check_erased_ecc_chunk >> - Pass algorithm specific bits_per_step to tegra_nand_get_strength >> - Store ECC layout in chip structure >> - Fix pointer assignment (use NULL) >> - Removed obsolete header delay.h >> - Fixed newlines >> - Use non-_io variant of wait_for_completion_timeout >> >> Lucas Stach (1): >> ARM: dts: tegra: add Tegra20 NAND flash controller node >> >> Stefan Agner (5): >> mtd: rawnand: add Reed-Solomon error correction algorithm >> mtd: rawnand: add an option to specify NAND chip as a boot device >> mtd: rawnand: tegra: add devicetree binding >> mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver >> ARM: dts: tegra: enable NAND flash on Colibri T20 >> >> .../devicetree/bindings/mtd/nand.txt | 6 +- >> .../bindings/mtd/nvidia-tegra20-nand.txt | 64 + >> MAINTAINERS | 7 + >> arch/arm/boot/dts/tegra20-colibri-512.dtsi | 16 + >> arch/arm/boot/dts/tegra20.dtsi | 15 + >> drivers/mtd/nand/raw/Kconfig | 6 + >> drivers/mtd/nand/raw/Makefile | 1 + >> drivers/mtd/nand/raw/nand_base.c | 4 + >> drivers/mtd/nand/raw/tegra_nand.c | 1268 +++++++++++++++++ >> include/linux/mtd/rawnand.h | 7 + >> 10 files changed, 1393 insertions(+), 1 deletion(-) >> create mode 100644 Documentation/devicetree/bindings/mtd/nvidia-tegra20-nand.txt >> create mode 100644 drivers/mtd/nand/raw/tegra_nand.c >> -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stefan, On Mon, 18 Jun 2018 12:51:52 +0200 Stefan Agner <stefan@agner.ch> wrote: > On 18.06.2018 11:58, Boris Brezillon wrote: > > On Sun, 17 Jun 2018 22:45:59 +0200 > > Stefan Agner <stefan@agner.ch> wrote: > > > >> Changes definitly calm down, most noteably probably the changes > >> around checking whether a page is empty if the stack reports ECC > >> errors.. I verified the code using raw nandwrites with OOB to > >> simulate an empty page which has some bits flipped in the OOB area, > >> everthing seems to work as I would expect it. > >> > >> For now I do not check extra OOB bytes since those are at variable > >> locations depending on algorithm. > > > > Hm, if you expose them as free OOB bytes, you should also check them, > > otherwise you might end up with corrupted data without noticing it. Note > > that, depending on whether those free OOB bytes are ECC-protected or > > not, you should change the way you do the check: > > > > - non-protected OOB bytes: all bytes should be 0xff (no bitflips > > allowed) > > - data+free OOB bytes protected by the same ECC bytes: you should pass > > the free OOB bytes buffer to nand_check_erased_ecc_chunk() along with > > the data and ECC buffers > > - free OOB bytes have their own ECC bytes: call > > nand_check_erased_ecc_chunk() separately and pass it the ECC + free > > OOB buffers. > > This graphic taken from the public Tegra 2 Technical Reference Manual is > quite useful: > https://imgur.com/a/0Hqzbkc Thanks for sharing this doc. > > Tegra basically has all of the above, which makes the whole business > really tricky... I'm not sure. Are "Skip bytes" protected by "main data parity bytes"? AFAICT, you have "Tag bytes" that fall in case #3 and "Remaining spare bytes" that fall in case #1. If "Skip bytes" are protected by the "main data parity bytes", then it falls in case #2, otherwise it probably goes in case #1. > > I am not sure if we really could do variant 1, non-protected OOB, but > since we have the option of protected OOB, we probably anyway would do > that. That's up to you, but in this case, you should not declare those bytes as free (didn't check what is currently done in the driver). > > RS/Hamming implements variant 3. It seems to be a mix of #1 and #3, but I'm not sure (see above). > > BCH implements variant 2. I'd say it's a mix of #1 (skip + remaining bytes) and #2(tag bytes). > OOB is protected with the last data buffer. That would be weird, but maybe you're right. HW ECC engine usually split the OOB area in X portions, X being the number of ECC steps needed to cover a NAND page, and then have ECC bytes cover a sub-portion of data+OOB. For example, for a NAND page of 2k with 64 bytes of OOB, and assuming the ECC step is 512bytes, you usually have something like: [512(data)+8(protected-oob)+8(ecc)] x 4 > > So this would require a algorithm depending implementation, which is > probably not a big deal. True. > > But there is one more issue with BCH: Only if extra data are actually > transferred, tag space is actually allocated. If no tag bytes are > transferred, parity follows immediately skip bytes. As far as I know the > MTD stacks OOB layout assumes that is always the same layout, no matter > whether we write extra OOB data or not. For the Tegra NAND controller > this would mean that we have to always transfer tag bytes and therefor > penalize the use case we are most interested in (which is no extra OOB > bytes, since UBI does not make use of it)... Hm, given the amount of tag bytes I don't think you'll have a huge penalty, so I'd recommend always sending those bytes. Alternatively, you could decide that you never want to have those tag bytes and expose none of them. > > Furthermore I realized that testing is not easily possible since > nandwrite with --oob seems not to make use of "oob_required" in the main > page write but issues a separate OOB write command. I did not found a > way to issue a write from user space which sets oob_required... Maybe it's time to patch those tools. The ioctl exists, so it's just a matter of using it in nandwrite/mtd-utils. > > Due to all this I rather prefer to not implement extra OOB support at > this point. I'm fine with that, but that means no JFFS2 support, as I think JFFS2 wants to place some of its metadata in the OOB area. Also, I fear it will be a mess to add support for that kind of things without breaking existing setup afterwards, so, by taking this decision you're pretty much saying that this controller will never expose free OOB bytes. That's not a problem from my PoV, but I want you to be aware of that. > > How do I do this properly? Set mtd_ooblayout_ops.free to NULL? Just implement a dummy function that returns -ERANGE. Regards, Boris -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Boris, On 18.06.2018 13:59, Boris Brezillon wrote: > Hi Stefan, > > On Mon, 18 Jun 2018 12:51:52 +0200 > Stefan Agner <stefan@agner.ch> wrote: > >> On 18.06.2018 11:58, Boris Brezillon wrote: >> > On Sun, 17 Jun 2018 22:45:59 +0200 >> > Stefan Agner <stefan@agner.ch> wrote: >> > >> >> Changes definitly calm down, most noteably probably the changes >> >> around checking whether a page is empty if the stack reports ECC >> >> errors.. I verified the code using raw nandwrites with OOB to >> >> simulate an empty page which has some bits flipped in the OOB area, >> >> everthing seems to work as I would expect it. >> >> >> >> For now I do not check extra OOB bytes since those are at variable >> >> locations depending on algorithm. >> > >> > Hm, if you expose them as free OOB bytes, you should also check them, >> > otherwise you might end up with corrupted data without noticing it. Note >> > that, depending on whether those free OOB bytes are ECC-protected or >> > not, you should change the way you do the check: >> > >> > - non-protected OOB bytes: all bytes should be 0xff (no bitflips >> > allowed) >> > - data+free OOB bytes protected by the same ECC bytes: you should pass >> > the free OOB bytes buffer to nand_check_erased_ecc_chunk() along with >> > the data and ECC buffers >> > - free OOB bytes have their own ECC bytes: call >> > nand_check_erased_ecc_chunk() separately and pass it the ECC + free >> > OOB buffers. >> >> This graphic taken from the public Tegra 2 Technical Reference Manual is >> quite useful: >> https://imgur.com/a/0Hqzbkc > > Thanks for sharing this doc. > >> >> Tegra basically has all of the above, which makes the whole business >> really tricky... > > I'm not sure. Are "Skip bytes" protected by "main data parity bytes"? > > AFAICT, you have "Tag bytes" that fall in case #3 and "Remaining spare > bytes" that fall in case #1. If "Skip bytes" are protected by the "main > data parity bytes", then it falls in case #2, otherwise it probably > goes in case #1. > Skip bytes are not protected. I think they are mainly meant to skip the bad block marker. Only 4, 8, 12 or 16 bytes are supported. >> >> I am not sure if we really could do variant 1, non-protected OOB, but >> since we have the option of protected OOB, we probably anyway would do >> that. > > That's up to you, but in this case, you should not declare those bytes > as free (didn't check what is currently done in the driver). > >> >> RS/Hamming implements variant 3. > > It seems to be a mix of #1 and #3, but I'm not sure (see above). > >> >> BCH implements variant 2. > > I'd say it's a mix of #1 (skip + remaining bytes) and #2(tag bytes). > >> OOB is protected with the last data buffer. > > That would be weird, but maybe you're right. HW ECC engine usually > split the OOB area in X portions, X being the number of ECC steps needed > to cover a NAND page, and then have ECC bytes cover a sub-portion of > data+OOB. > > For example, for a NAND page of 2k with 64 bytes of OOB, and assuming > the ECC step is 512bytes, you usually have something like: > > [512(data)+8(protected-oob)+8(ecc)] x 4 > The TRM explicitly states so: "BCH mode Error correction - Error correction with involving spare only transfers is not supported. ECC calculation of last sub-page includes tag data in spare area. - Error correction with Main only transfers is supported. - Maximum possible length of Tag data size is 252 bytes." >> >> So this would require a algorithm depending implementation, which is >> probably not a big deal. > > True. > >> >> But there is one more issue with BCH: Only if extra data are actually >> transferred, tag space is actually allocated. If no tag bytes are >> transferred, parity follows immediately skip bytes. As far as I know the >> MTD stacks OOB layout assumes that is always the same layout, no matter >> whether we write extra OOB data or not. For the Tegra NAND controller >> this would mean that we have to always transfer tag bytes and therefor >> penalize the use case we are most interested in (which is no extra OOB >> bytes, since UBI does not make use of it)... > > Hm, given the amount of tag bytes I don't think you'll have a huge > penalty, so I'd recommend always sending those bytes. Alternatively, > you could decide that you never want to have those tag bytes and expose > none of them. > >> >> Furthermore I realized that testing is not easily possible since >> nandwrite with --oob seems not to make use of "oob_required" in the main >> page write but issues a separate OOB write command. I did not found a >> way to issue a write from user space which sets oob_required... > > Maybe it's time to patch those tools. The ioctl exists, so it's just a > matter of using it in nandwrite/mtd-utils. > >> >> Due to all this I rather prefer to not implement extra OOB support at >> this point. > > I'm fine with that, but that means no JFFS2 support, as I think JFFS2 > wants to place some of its metadata in the OOB area. Also, I fear it > will be a mess to add support for that kind of things without breaking > existing setup afterwards, so, by taking this decision you're pretty > much saying that this controller will never expose free OOB bytes. > That's not a problem from my PoV, but I want you to be aware of that. > We already operate without extra OOB byte support in our downstream BSP. I'd rather have a easy upgrade path today... Another issue I just realized: The boot ROM only supports BCH without tag bytes... So at least the boot loader has to be written without tag bytes. >> >> How do I do this properly? Set mtd_ooblayout_ops.free to NULL? > > Just implement a dummy function that returns -ERANGE. > Ok, I will go with this then. Also, thanks for all your valuable feedback, really appreciated! -- Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html