From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Mon, 26 Nov 2012 15:09:19 -0600
Subject: [U-Boot] [RFC/PATCH 3/4] omap_gpmc: add support for hw assisted
	BCH8
In-Reply-To: <1353683657-23496-4-git-send-email-andreas.devel@googlemail.com>
	(from andreas.devel@googlemail.com on Fri Nov 23 09:14:16 2012)
Message-ID: <1353964159.2383.13@tyr>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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