[PATCH v9 7/9] mtd: nand: omap: use drivers/mtd/nand/nand_bch.c wrapper for BCH ECC instead of lib/bch.c

[Brian Norris]

On Thu, Oct 17, 2013 at 3:14 AM, Gupta, Pekon <pekon at ti.com> wrote:
>> From: Brian Norris [mailto:computersforpeace at gmail.com]
>> > On Tue, Oct 15, 2013 at 11:19:55AM +0530, Pekon Gupta wrote:
> [snip]
>
>> > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> > index d885298..5836039 100644
>> > --- a/drivers/mtd/nand/Kconfig
>> > +++ b/drivers/mtd/nand/Kconfig
>> > @@ -96,35 +96,13 @@ config MTD_NAND_OMAP2
>> >
>> >  config MTD_NAND_OMAP_BCH
>> >     depends on MTD_NAND && MTD_NAND_OMAP2 && ARCH_OMAP3
>> > -   tristate "Enable support for hardware BCH error correction"
>> > +   tristate "Support hardware based BCH error correction"
>> >     default n
>> >     select BCH
>> > -   select BCH_CONST_PARAMS
>>
>> Do you know what will happen now if someone configures
>> BCH_CONST_PARAMS?
>> Would this cause problems?
>>
> As per comments in lib/bch.c
> ---------------------------
> * Option CONFIG_BCH_CONST_PARAMS can be used to force fixed values of
>  * parameters m and t; thus allowing extra compiler optimizations and providing
>  * better (up to 2x) encoding performance. Using this option makes sense when
>  * (m,t) are fixed and known in advance, e.g. when using BCH error correction
>  * on a particular NAND flash device.
> ---------------------------
> 'BCH_CONST_PARAMS' is required for optimization when BCH algorithm
> is fixed. But in omap-nand case selection of type of BCH algorithm
> (BCH4 or BCH8) comes from DT binding "ti,nand-ecc-opts".
>
> If enabled (CONFIG_BCH_CONST_PARAMS) will optimize lib/bch.c
> for BCH8 algorithm by default, so
> CASE: if BCH8 is selected by DT, then no issues
> CASE: if BCH4 is selected  then nand_bch_init() fails with following error
> + if (!info->nand.ecc.priv) {
>                 pr_err("nand: error: unable to use s/w BCH library\n");
>                 err = -EINVAL;
>                 goto out_release_mem_region;
> }

Ah, so lib/bch.c handles this gracefully enough:

#if defined(CONFIG_BCH_CONST_PARAMS)
        if ((m != (CONFIG_BCH_CONST_M)) || (t != (CONFIG_BCH_CONST_T))) {
                printk(KERN_ERR "bch encoder/decoder was configured to support "
                       "parameters m=%d, t=%d only!\n",
                       CONFIG_BCH_CONST_M, CONFIG_BCH_CONST_T);
                goto fail;
        }
#endif

>> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> [snip]
>> > -           info->bch = init_bch(nand_chip->ecc.bytes,
>> > -                                   nand_chip->ecc.strength,
>> > -                                   OMAP_ECC_BCH8_POLYNOMIAL);
>> > -           if (!info->bch) {
>> > +           info->nand.ecc.priv     = nand_bch_init(mtd,
>> > +                                           info->nand.ecc.size,
>> > +                                           info->nand.ecc.bytes,
>> > +                                           &info->nand.ecc.layout);
>>
>> Are you sure nand_bch_init() is a proper drop-in replacement for the
>> implementation you had based on init_bch()? It looks to me like they at
>> least use a differnt polynomial value (0x201b vs. 0). Is this a problem
>> for backwards compatibility?
>>
> It's not the polynomial value = 0. Rather 0x201b is selected in both cases
> Refer below code.
> ---------------
> When init_bch(m,t, 0) is called from nand_bch_init() then,
> lib/bch.c @@ init_bch(int m, int t, unsigned int prim_poly)
> (a)     /* default primitive polynomials */
>         static const unsigned int prim_poly_tab[] = {
>                 0x25, 0x43, 0x83, 0x11d, 0x211, 0x409, 0x805, 0x1053, 0x201b,
>                 0x402b, 0x8003,
>         };
> (b)     /* select a primitive polynomial for generating GF(2^m) */
>         if (prim_poly == 0)
>                 prim_poly = prim_poly_tab[m-min_m];
> (c) And, const int min_m = 5;
>
> So, for BCH8 m=13, min_m=5; So
> prim_poly = prim_poly_tab[13-5] = prim_poly_tab[8] = 0x201b
> ---------------
>
> Hence, there is no change in polynomial, it's just that instead of
> hard-coding the value, polynomial selection depends on 'm' and 't'.

I missed that. Thanks for pointing it out.

>> [...]
>>
>> A related question: do we have any current users of this driver that can
>> provide testing results for this series? Or is this work just tested
>> with new hardware?
>>
> Got a tested-by: jp.francois at cynove.com for BCH4
> But that was in May,2013 :-)
> http://lists.infradead.org/pipermail/linux-mtd/2013-May/047065.html

OK, well that's good to know. I suppose the operation of the driver
probably hasn't changed a lot in the meantime, just the DT bindings
and a few other smaller details. But we still can't call it
"Tested-by:" in its current form.

Brian