Re: [PATCH v5 0/6] mtd: rawnand: add NVIDIA Tegra NAND flash support

[Boris Brezillon]

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.

> 
> 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