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".
(...)
> /**
> + * 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 ?
If using the ELM prevents you from reusing gpmc_calculate_ecc_bch[48], could
you explain in which way ?
Best regards,
--
Ivan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
