On Thu, Oct 04, 2012 at 00:24:58, Ivan Djelic wrote:
> On Wed, Oct 03, 2012 at 03:29:48PM +0100, Philip, Avinash wrote:
> > Add support for BCH ECC scheme to gpmc driver and also enabling multi
> > sector read/write. This helps in doing single shot NAND page read and
> > write.
> >
> > ECC engine configurations
> > BCH 4 bit support
> > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
> > 2. read => ECC engine configured in wrap mode 1 and with ecc_size0 as
> > 13 and ecc_size1 as 1.
> >
> > BCH 8 bit support
> > 1. write => ECC engine configured in wrap mode 6 and with ecc_size0 as 32.
> > 2. read => ECC engine configured in wrap mode 1 and with ecc_size0 as
> > 26 and ecc_size1 as 2.
> >
> > Note: For BCH8 ECC bytes set to 14 to make compatible with RBL.
> >
>
> Hi Philip,
>
> I have a few comments/questions below,
>
> (...)
> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> > index 72428bd..c9bc3cf 100644
> > --- a/arch/arm/mach-omap2/gpmc.c
> > +++ b/arch/arm/mach-omap2/gpmc.c
> > @@ -24,6 +24,7 @@
> > #include <linux/io.h>
> > #include <linux/module.h>
> > #include <linux/interrupt.h>
> > +#include <linux/mtd/nand.h>
> >
> > #include <asm/mach-types.h>
> > #include <plat/gpmc.h>
> > @@ -83,6 +84,18 @@
> > #define ENABLE_PREFETCH (0x1 << 7)
> > #define DMA_MPU_MODE 2
> >
> > +/* GPMC ecc engine settings for read */
> > +#define BCH_WRAPMODE_1 1 /* BCH wrap mode 6 */
>
> Comment should say "mode 1".
Ok I will correct it.
>
> (...)
> > /**
> > + * gpmc_calculate_ecc_bch - Generate ecc bytes per block of 512 data bytes for entire page
> > + * @cs: chip select number
> > + * @dat: The pointer to data on which ECC is computed
> > + * @ecc: The ECC output buffer
> > + */
> > +int gpmc_calculate_ecc_bch(int cs, const u_char *dat, u_char *ecc)
> > +{
> > + int i, eccbchtsel;
> > + u32 nsectors, reg, bch_val1, bch_val2, bch_val3, bch_val4;
> > +
> > + if (gpmc_ecc_used != cs)
> > + return -EINVAL;
> > +
> > + /* read number of sectors for ecc to be calculated */
> > + nsectors = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 4) & 0x7) + 1;
> > + /*
> > + * find BCH scheme used
> > + * 0 -> BCH4
> > + * 1 -> BCH8
> > + */
> > + eccbchtsel = ((gpmc_read_reg(GPMC_ECC_CONFIG) >> 12) & 0x3);
> > +
> > + /* update ecc bytes for entire page */
> > + for (i = 0; i < nsectors; i++) {
> > +
> > + reg = GPMC_ECC_BCH_RESULT_0 + 16 * i;
> > +
> > + /* Read hw-computed remainder */
> > + bch_val1 = gpmc_read_reg(reg + 0);
> > + bch_val2 = gpmc_read_reg(reg + 4);
> > + if (eccbchtsel) {
> > + bch_val3 = gpmc_read_reg(reg + 8);
> > + bch_val4 = gpmc_read_reg(reg + 12);
> > + }
> > +
> > + if (eccbchtsel) {
> > + /* BCH8 ecc scheme */
> > + *ecc++ = (bch_val4 & 0xFF);
> > + *ecc++ = ((bch_val3 >> 24) & 0xFF);
> > + *ecc++ = ((bch_val3 >> 16) & 0xFF);
> > + *ecc++ = ((bch_val3 >> 8) & 0xFF);
> > + *ecc++ = (bch_val3 & 0xFF);
> > + *ecc++ = ((bch_val2 >> 24) & 0xFF);
> > + *ecc++ = ((bch_val2 >> 16) & 0xFF);
> > + *ecc++ = ((bch_val2 >> 8) & 0xFF);
> > + *ecc++ = (bch_val2 & 0xFF);
> > + *ecc++ = ((bch_val1 >> 24) & 0xFF);
> > + *ecc++ = ((bch_val1 >> 16) & 0xFF);
> > + *ecc++ = ((bch_val1 >> 8) & 0xFF);
> > + *ecc++ = (bch_val1 & 0xFF);
> > + /* 14th byte of ecc not used */
> > + *ecc++ = 0;
> > + } else {
> > + /* BCH4 ecc scheme */
> > + *ecc++ = ((bch_val2 >> 12) & 0xFF);
> > + *ecc++ = ((bch_val2 >> 4) & 0xFF);
> > + *ecc++ = (((bch_val2 & 0xF) << 4) |
> > + ((bch_val1 >> 28) & 0xF));
> > + *ecc++ = ((bch_val1 >> 20) & 0xFF);
> > + *ecc++ = ((bch_val1 >> 12) & 0xFF);
> > + *ecc++ = ((bch_val1 >> 4) & 0xFF);
> > + *ecc++ = ((bch_val1 & 0xF) << 4);
> > + }
> > + }
> > +
> > + gpmc_ecc_used = -EINVAL;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(gpmc_calculate_ecc_bch);
>
> Here you introduce a function very similar to gpmc_calculate_ecc_bch4 and
> gpmc_calculate_ecc_bch8, but without the added benefit (IMHO) of the constant
> polynomial that allows to get an ecc sequence of 0xFFs for a buffer filled with
> 0xFFs. Why ?
I don't exactly understand what we benefitted/achieve. In my observation,
this API does spare area also written with 0xFF if data area is 0xFFs.
So the area looks like erased page again.
> If using the ELM prevents you from reusing gpmc_calculate_ecc_bch[48], could you explain in which way ?
When using gpmc_calculate_ecc_bch[48], calculated ecc values modified.
The read sequence we following is
Read 512 byte -> read ECC bytes from spare area
Now the calculated ECC will be zero if no error is reported. In case of error, a syndrome
Polynomial is reported. In either case modifying will corrupt the data.
This is valid if we are writing a page with 0xFF also. But this time we were filling a valid
ecc in spare area not 0xFF as in gpmc_calculate_ecc_bch[48].
Additionally to make compatible with RBL ECC layout (14 byte), we were setting ecc[13] as zero.
Thanks
Avinash
>
> Best regards,
> --
> Ivan
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
