On Thu, Oct 17, 2013 at 3:14 AM, Gupta, Pekon <pekon@xxxxxx> wrote: >> From: Brian Norris [mailto:computersforpeace@xxxxxxxxx] >> > 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@xxxxxxxxxx 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 -- 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
