Re: [RESEND PATCH 2/5] mtd: rawnand: add NVIDIA Tegra NAND Flash controller driver

[Benjamin Lindqvist]

Hi Stefan,

It seems to me that a probe similar to what the BootROM does shouldn't
be awfully complicated to implement - just cycle through the switch
cases in case of an ECC error. But I guess that's more of an idea for
further improvements rather than a comment to the patch set under
review.

However, I think that allowing for an override of the oobsize
inference would be a good idea before merging, no? This could just be
a trivial #ifdef (at least temporarily). If you agree but don't feel
like doing it yourself, I'd be happy to pitch in. Let me know.

Best regards,
Benjamin

2018-05-24 13:00 GMT+02:00 Stefan Agner <stefan@xxxxxxxx>:
> On 24.05.2018 09:45, Benjamin Lindqvist wrote:
>> Hi Stefan (and all),
>>
>> First off, I apoloigize in advance if I'm deviating from common
>> kernel mailing list courtesy -- this is my first time responding.
>> I just have a comment on the NAND driver that I'd like to bring
>> to the public.
>>
>
> Welcome!
>
>>> +       switch (mtd->oobsize) {
>>> ...
>>> +       case 224:
>>> +               mtd_set_ooblayout(mtd, &tegra_nand_oob_224_ops);
>>> +               chip->ecc.strength = 8;
>>> +               chip->ecc.bytes = 18;
>>> +               value |= CFG_ECC_SEL | CFG_TVAL_8;
>>> +               break; +       case 224:
>>
>> I am not sure how you arrived at this oobsize-based inference. I
>> have not seen any explicit relation between oob size and ECC
>> algorithm used in the reference manual. Indeed, the U-Boot I was
>> working on (a fork of the Toradex 2015.04 U-Boot) always has
>> oobsize == 224 but used either BCH[t=16] or RS[t=4]. In fact, we
>> tried choosing RS[t=8] in U-Boot but we failed to make the
>> BootROM decode this at all. So we had to use RS[t=4]. But
>> changing the algorithm did not automatically change the oobsize,
>> at least it didn't for us. So maybe you should consider if this
>> is really the way to go about deciding which algorithm is used.
>
> The oobsize based inference to set the HW ECC mode comes from the
> patchset I picked up. Typically, the size of the OOB area is such that
> it allows "good enough" error correction required for that chip. So
> using it as indicator for the ECC algorithm is not entirely off...
>
> But yeah I agree we have better means, and I already started working on
> a better mechanism. Also, I worked on BCH support, and it looks pretty
> good already.
>
> If we want to auto select mode we can use the ECC requirements from
> ONFI/JEDEC/parameter page. Or we could use device tree only.
>
> Thanks for bringing up the Boot ROM issue. In fact I investigated the
> supported modes the recent days too. First off, as you mentioned, the
> boot ROM seems to probe different modes until it succeeds. By trying
> through all RS and BCH modes, it seems that it only probes some modes:
> - RS t=4
> - BCH t=8
> - BCH t=16
>
> I guess those are preferred modes in practise. Not sure if we should
> make sure the auto selection such that it only chooses from this list...
>
>>
>> Note that we're in fact using this patch set in Linux today, but
>> we had to remove the oobsize inference part. Currently we're
>> simply hard coding it to CFG_TVAL_4, but maybe it would be
>> cleaner to add ECC algo as a board config instead, e.g. in the
>> .dts file or whatever. This seems to be done by other vendors
>> already, see for example excerpt of
>> Documentation/devicetree/bindings/mtd/gpmc-nand.txt below:
>>
>>  - ti,nand-ecc-opt: A string setting the ECC layout to use. One of:
>> "sw" 1-bit Hamming ecc code via software
>> "hw" <deprecated> use "ham1" instead
>> "hw-romcode" <deprecated> use "ham1" instead
>> "ham1" 1-bit Hamming ecc code
>> "bch4" 4-bit BCH ecc code
>> "bch8" 8-bit BCH ecc code
>> "bch16" 16-bit BCH ECC code
>> Refer below "How to select correct ECC scheme for your device ?"
>>
>> It seems as if this method would be equally applicable to Tegra NAND.
>
> Yeah, ideally we can reuse "nand-ecc-algo". Although, Reed-Solomon is
> not yet in the list.  So using this property would require to extend
> this standard property.
>
> --
> Stefan