[U-Boot] omap_gpmc: Do not default to HAM1 when SW ECC is selected

[Adam Lee]

Had a conversation with Ash @ Gumstix and he pointed out relying on
CONFIG_NAND_OMAP_ECCSCHEME could be dangerous as it could be anything other
than the two SW ECC schemes available for OMAP3.

Also it looks like making a selection between OMAP_ECC_BCH8_CODE_HW and
OMAP_ECC_BCH8_CODE_HW_DETECTION_SW may not make sense as the latter
clearly requires CONFIG_BCH, which enables "software" library for BCH.

On Wed, Feb 18, 2015 at 9:33 AM, Adam Lee <adam.yh.lee at gmail.com> wrote:

> Hi Andreas, for OMAP3 and AM35xx boards, it would have been ok omitting
> the
> CONFIG_BCH check and simply use CONFIG_NAND_OMAP_ECCSCHEME.
> Those boards use the ecc scheme config already. However I just wasn't 100%
> sure if I could rely on this config for all TI OMAP/AM based boards. I
> know OMAP3
>  and AM35xx board configs already have CONFIG_NAND_OMAP_ECCSCHEME.
> Should I check for other OMAP and AM series? The original ecc detection
>  function
> seems to be written with an assumption that config is nonexistent - hence
> defaulting
> to HAM1.
>
> That said, I agree that the whole omap_nand_switch_ecc() could be improved.
> However, to me, the confusion arised from the fact that OMAP3 can do BCH8
> hw ECC
> calculation but needs software to do the correction. Hence my patch
> changes the
> software part of the omap_nand_switch_ecc(), and not the hardware part.
>
> Just to clarify, what you are saying is that I should leave the software
> part as it was (defaulting
> to HAM1), and in the hardware part I should check for ELM support and
> choose a BCH8
> scheme accordingly, regardless of what's defined by
> CONFIG_NAND_OMAP_ECCSCHEME?
>
> In other words, I will run 'nandecc hw' to enable BCH8?
>
> Let me know,
>
> Thanks,
>
> Adam
>
>
>
> On Wed, Feb 18, 2015 at 6:14 AM, Andreas Bießmann <
> andreas.devel at googlemail.com> wrote:
>
>> Hi Adam,
>>
>> On 02/18/2015 03:58 AM, Adam YH Lee wrote:
>> > The ECC scheme selection algorithm in OMAP GPMC appears to be left
>> untested when
>> > BCH8 handling code was added. Running 'nandecc sw' defaults to HAM1
>> even if
>> > the board is using another scheme (ex.
>> OMAP_ECC_BCH8_CODE_HW_DETECTION_SW on
>> > OMAP3). This results in unrecoverable ECC errors when reading data.
>> This commit
>> > fixes the behavior by checking for CONFIG_BCH and using the scheme
>> defined by
>> > CONFIG_NAND_OMAP_ECCSCHEME in the board configuration file.
>> >
>> > This has been tested on Gumstix Overo (OMAP3).
>> >
>> > Signed-off-by: Adam YH Lee <adam.yh.lee at gmail.com>
>> > ---
>> >  drivers/mtd/nand/omap_gpmc.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
>> > index fc64f48..5daf932 100644
>> > --- a/drivers/mtd/nand/omap_gpmc.c
>> > +++ b/drivers/mtd/nand/omap_gpmc.c
>> > @@ -901,8 +901,13 @@ int __maybe_unused omap_nand_switch_ecc(uint32_t
>> hardware, uint32_t eccstrength)
>> >                       return -EINVAL;
>> >               }
>> >       } else {
>> > +             #ifdef CONFIG_BCH
>> > +             err = omap_select_ecc_scheme(nand,
>> CONFIG_NAND_OMAP_ECCSCHEME,
>> > +                                     mtd->writesize, mtd->oobsize);
>> > +             #else
>> >               err = omap_select_ecc_scheme(nand, OMAP_ECC_HAM1_CODE_SW,
>> >                                       mtd->writesize, mtd->oobsize);
>>
>> Couldn't we just use the CONFIG_NAND_OMAP_ECCSCHEME instead of
>> OMAP_ECC_HAM1_CODE_SW here and omit the CONFIG_BCH define?
>>
>> On the other hand I think the whole function omap_nand_switch_ecc() is
>> wrong. AFAIR it should only be used on omap3 aka am35xx/dm37xx devices
>> witch the nandecc command.
>> These SoC do not have a ELM. Therefore I decided to say BCH8 on those
>> devices is always 'HW ECC' when introduced BCH8 on omap3 first. Nowadays
>> it is the so called BCH8_CODE_HW_DETECTION_SW. Coming from this mindset
>> the right solution is to use some detection if the ELM is supported or
>> not and switch in the HW part of omap_nand_switch_ecc() between
>> OMAP_ECC_BCH8_CODE_HW and OMAP_ECC_BCH8_CODE_HW_DETECTION_SW.
>>
>> Best regards
>>
>> Andreas Bießmann
>>
>
>