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@xxxxxxxx> wrote: > >> On 18.06.2018 11:58, Boris Brezillon wrote: >> > On Sun, 17 Jun 2018 22:45:59 +0200 >> > Stefan Agner <stefan@xxxxxxxx> 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 devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
