[U-Boot] [RFC/PATCH 3/4] omap_gpmc: add support for hw assisted BCH8

Scott Wood
2012-11-26T21:09:19Z

On 11/23/2012 09:14:16 AM, Andreas Bie?mann wrote:
> diff --git a/arch/arm/cpu/armv7/omap3/board.c
> b/arch/arm/cpu/armv7/omap3/board.c
> index f3cd81a..22286c0 100644
> --- a/arch/arm/cpu/armv7/omap3/board.c
> +++ b/arch/arm/cpu/armv7/omap3/board.c
> @@ -330,8 +330,10 @@ static int do_switch_ecc(cmd_tbl_t * cmdtp, int
> flag, int argc, char * const arg
> {
> if (argc != 2)
> goto usage;
> - if (strncmp(argv[1], "hw", 2) == 0)
> + if (strncmp(argv[1], "hw1", 3) == 0)
> omap_nand_switch_ecc(1);
> + else if (strncmp(argv[1], "hw2", 3) == 0)
> + omap_nand_switch_ecc(2);
> else if (strncmp(argv[1], "sw", 2) == 0)
> omap_nand_switch_ecc(0);
> else

Will this break any existing scripts? Maybe "hw" should be left alone
and "hw2" renamed to something more descriptive (e.g. "bch8")?

> @@ -347,7 +349,9 @@ usage:
> U_BOOT_CMD(
> nandecc, 2, 1, do_switch_ecc,
> "switch OMAP3 NAND ECC calculation algorithm",
> - "[hw/sw] - Switch between NAND hardware (hw) or software (sw)
> ecc algorithm"
> + "[hw1/hw2/sw] - Switch between NAND hardware (hw1 - 1bit
> hamming),\n"
> + "\tNAND hardware assisted BCH8 (hw2 - 8bit BCH)\n"
> + "\tor software (sw) ecc algorithm"
> );
>
> #endif /* CONFIG_NAND_OMAP_GPMC & !CONFIG_SPL_BUILD */
> diff --git a/drivers/mtd/nand/omap_gpmc.c
> b/drivers/mtd/nand/omap_gpmc.c
> index f1469d1..a5ab046 100644
> --- a/drivers/mtd/nand/omap_gpmc.c
> +++ b/drivers/mtd/nand/omap_gpmc.c
> @@ -27,6 +27,7 @@
> #include <asm/arch/mem.h>
> #include <asm/arch/omap_gpmc.h>
> #include <linux/mtd/nand_ecc.h>
> +#include <linux/bch.h>
> #include <linux/compiler.h>
> #include <nand.h>
>
> @@ -234,12 +235,194 @@ static void __maybe_unused
> omap_enable_hwecc(struct mtd_info *mtd, int32_t mode)
> }
> }
>
> +/*
> + * basic BCH8 support
> + */
> +#ifdef CONFIG_NAND_OMAP_BCH8
> +static struct nand_ecclayout hw_bch8_nand_oob =
> GPMC_NAND_HW_BCH8_ECC_LAYOUT;

Is this layout going to be used anywhere other than here? If not, why
hide the layout in a #define?

> +
> +/*
> + * omap_init_hwecc_bch - Initialize the BCH Hardware ECC for NAND
> flash in
> + * GPMC controller
> + * @mtd: MTD device structure
> + * @mode: Read/Write mode
> + */
> +static void omap_init_hwecc_bch(struct nand_chip *chip, int32_t mode)
> +{
> + uint32_t val, dev_width = (chip->options & NAND_BUSWIDTH_16) >>
> 1;
> +
> + /* Clear the ecc result registers, select ecc reg as 1 */
> + writel(ECCCLEAR | ECCRESULTREG1, &gpmc_cfg->ecc_control);
> +
> + /*
> + * When using BCH, sector size is hardcoded to 512 bytes.
> + * Here we are using wrapping mode 6 both for reading and
> writing, with:
> + * size0 = 0 (no additional protected byte in spare area)
> + * size1 = 32 (skip 32 nibbles = 16 bytes per sector in spare
> area)
> + */
> + writel((32 << 22) | (0 << 12), &gpmc_cfg->ecc_size_config);
> +
> + /* BCH configuration */
> + val = ((1 << 16) | /* enable BCH */
> + (1 << 12) | /* set fix to BCH8 */
> + (0x06 << 8) | /* wrap mode = 6 */
> + (dev_width << 7) | /* bus width */
> + (0 << 4) | /* number of sectors
> (we use 1 sector)*/
> + (cs << 1) | /* ECC CS */
> + (0x1)); /* enable ECC */

Most of those magic numbers should be referred to symbolicly.

> +static void omap_free_bch(struct mtd_info *mtd)
> +{
> + struct nand_chip *chip = mtd->priv;
> + if (chip->priv) {
> + free_bch((struct bch_control *)chip->priv);
> + chip->priv = NULL;
> + }

Do you really need this cast?

> +#ifdef CONFIG_NAND_OMAP_BCH8
> + nand->priv = init_bch(13, 8, 0x201b /* hw polynominal */);
> + if (!nand->priv) {
> + puts("Could not init_bch()\n");
> + }
> +#endif
> +
> +#if defined CONFIG_NAND_OMAP_BCH8

Merge these two ifdef sections.

Shouldn't you take some action on the failure case other than printing
a message and continuing to try to use bch8?

-Scott