[U-Boot] [PATCH] am335x: NAND: add BCH16 and 4k page size support

[Scott Wood]

On 01/28/2013 07:35:40 AM, Jordy van Wolferen wrote:
> This is tested with a custom AM3359 (rev 2.0) board.
> NAND chip: MT29F16G08ABABAWP
> 
> This code allows me to boot from ROM code.
> The ROM code forces BCH16 on NAND chips with a 4k page size.
> 
> BCH16 is not enabled by default.
> 
> 
> ---

Missing Signed-off-by (please read the "Sign your work" section of  
Documentation/SubmittingPatches in Linux and be sure that you meet the  
conditions of the Developer's Certificate of Origin before adding your  
sign off).

Could you explain the patch in a bit more detail?  You say it is "not  
enabled by default" -- what would be required to enable it?

>  arch/arm/include/asm/arch-am33xx/cpu.h       |   8 +-
>  arch/arm/include/asm/arch-am33xx/omap_gpmc.h |  43 ++++++++
>  drivers/mtd/nand/omap_gpmc.c                 | 150  
> +++++++++++++++++----------
>  include/linux/mtd/mtd-abi.h                  |   2 +-
>  4 files changed, 148 insertions(+), 55 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-am33xx/cpu.h  
> b/arch/arm/include/asm/arch-am33xx/cpu.h
> index 16e8a80..0a1f1ff 100644
> --- a/arch/arm/include/asm/arch-am33xx/cpu.h
> +++ b/arch/arm/include/asm/arch-am33xx/cpu.h
> @@ -78,6 +78,10 @@ struct bch_res_0_3 {
>  	u32 bch_result_x[4];
>  };
> 
> +struct bch_res_4_6 {
> +	u32 bch_result_x[3];
> +};
> +
>  struct gpmc {
>  	u8 res1[0x10];
>  	u32 sysconfig;		/* 0x10 */
> @@ -107,7 +111,9 @@ struct gpmc {
>  	u8 res7[12];		/* 0x224 */
>  	u32 testmomde_ctrl;	/* 0x230 */
>  	u8 res8[12];		/* 0x234 */
> -	struct bch_res_0_3 bch_result_0_3[2];	/* 0x240 */
> +	struct bch_res_0_3 bch_result_0_3;	/* 0x240 */
> +	u32 dummy[44];		/* not used */
> +	struct bch_res_4_6 bch_result_4_6;	/* 300 */
>  };
> 
>  /* Used for board specific gpmc initialization */
> diff --git a/arch/arm/include/asm/arch-am33xx/omap_gpmc.h  
> b/arch/arm/include/asm/arch-am33xx/omap_gpmc.h
> index 572f9d0..534fa6e 100644
> --- a/arch/arm/include/asm/arch-am33xx/omap_gpmc.h
> +++ b/arch/arm/include/asm/arch-am33xx/omap_gpmc.h
> @@ -117,4 +117,47 @@
>  		{.offset = 106,\
>  		 .length = 8 } } \
>  }
> +
> +#define GPMC_NAND_4K_HW_BCH8_ECC_LAYOUT {\
> +	.eccbytes = 112,\
> +	.eccpos = {2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,\
> +				16, 17, 18, 19, 20, 21, 22, 23, 24, 25,  
> 26, 27,\
> +				28, 29, 30, 31, 32, 33, 34, 35, 36, 37,  
> 38, 39,\
> +				40, 41, 42, 43, 44, 45, 46, 47, 48, 49,  
> 50, 51,\
> +				52, 53, 54, 55, 56, 57, 58, 59, 60, 61,  
> 62, 63,\
> +				64, 65, 66, 67, 68, 69, 70, 71, 72, 73,  
> 74, 75,\
> +				76, 77, 78, 79, 80, 81, 82, 83, 84, 85,  
> 86, 87,\
> +				88, 89, 90, 91, 92, 93, 94, 95, 96, 97,  
> 98, 99,\
> +				100, 101, 102, 103, 104, 105, 106, 107,  
> 108, 109,\
> +				110, 111, 112, 113},\
> +	.oobfree = {\
> +		{.offset = 114,\
> +		 .length = 110 } } \
> +}
> +
> +#define GPMC_NAND_4K_HW_BCH16_ECC_LAYOUT {\
> +	.eccbytes = 208,\
> +	.eccpos = { 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,\
> +				16, 17, 18, 19, 20, 21, 22, 23, 24, 25,  
> 26, 27,\
> +				28, 29, 30, 31, 32, 33, 34, 35, 36, 37,  
> 38, 39,\
> +				40, 41, 42, 43, 44, 45, 46, 47, 48, 49,  
> 50, 51,\
> +				52, 53, 54, 55, 56, 57, 58, 59, 60, 61,  
> 62, 63,\
> +				64, 65, 66, 67, 68, 69, 70, 71, 72, 73,  
> 74, 75,\
> +				76, 77, 78, 79, 80, 81, 82, 83, 84, 85,  
> 86, 87,\
> +				88, 89, 90, 91, 92, 93, 94, 95, 96, 97,  
> 98, 99,\
> +				100, 101, 102, 103, 104, 105, 106, 107,  
> 108, 109,\
> +				110, 111, 112, 113, 114, 115, 116, 117,  
> 118, 119,\
> +				120, 121, 122, 123, 124, 125, 126, 127,  
> 128, 129,\
> +				130, 131, 132, 133, 134, 135, 136, 137,  
> 138, 139,\
> +				140, 141, 142, 143, 144, 145, 146, 147,  
> 148, 149,\
> +				150, 151, 152, 153, 154, 155, 156, 157,  
> 158, 159,\
> +				160, 161, 162, 163, 164, 165, 166, 167,  
> 168, 169,\
> +				170, 171, 172, 173, 174, 175, 176, 177,  
> 178, 179,\
> +				180, 181, 182, 183, 184, 185, 186, 187,  
> 188, 189,\
> +				190, 191, 192, 193, 194, 195, 196, 197,  
> 198, 199,\
> +				200, 201, 202, 203, 204, 205, 206, 207,  
> 208, 209},\

You have too many tabs here -- be sure your editor is set for  
8-character tabs.

> +	.oobfree = {\
> +		{.offset = 210,\
> +		 .length = 14 } } \
> +}
>  #endif /* __ASM_ARCH_OMAP_GPMC_H */
> diff --git a/drivers/mtd/nand/omap_gpmc.c  
> b/drivers/mtd/nand/omap_gpmc.c
> index cee394e..3c42a54 100644
> --- a/drivers/mtd/nand/omap_gpmc.c
> +++ b/drivers/mtd/nand/omap_gpmc.c
> @@ -76,8 +76,8 @@ int omap_spl_dev_ready(struct mtd_info *mtd)
> 
>  /*
>   * omap_hwecc_init - Initialize the Hardware ECC for NAND flash in
> - *                   GPMC controller
> - * @mtd:        MTD device structure
> + *					 GPMC controller
> + * @mtd:		MTD device structure
>   *
>   */
>  static void __maybe_unused omap_hwecc_init(struct nand_chip *chip)

No unrelated whitespace changes, please.

> @@ -258,7 +258,7 @@ struct nand_bch_priv {
>  #define ECC_BCH8_NIBBLES	26
>  #define ECC_BCH16_NIBBLES	52
> 
> -static struct nand_ecclayout hw_bch8_nand_oob =  
> GPMC_NAND_HW_BCH8_ECC_LAYOUT;
> +static struct nand_ecclayout nand_ecclayout =  
> GPMC_NAND_HW_BCH8_ECC_LAYOUT;

Why the name change?  It's still the bch8 layout.

If your intent is for this to be the configuration knob, U-Boot is not  
configured by making edits to random source files.  It needs to be a  
proper config symbol (and documented).

>  static struct nand_bch_priv bch_priv = {
>  	.mode = NAND_ECC_HW_BCH,
> @@ -280,21 +280,21 @@ static void omap_read_bch8_result(struct  
> mtd_info *mtd, uint8_t big_endian,
>  	int8_t i = 0, j;
> 
>  	if (big_endian) {
> -		ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[3];
> +		ptr = &gpmc_cfg->bch_result_0_3.bch_result_x[3];
>  		ecc_code[i++] = readl(ptr) & 0xFF;
>  		ptr--;
>  		for (j = 0; j < 3; j++) {
>  			ecc_code[i++] = (readl(ptr) >> 24) & 0xFF;
>  			ecc_code[i++] = (readl(ptr) >> 16) & 0xFF;
> -			ecc_code[i++] = (readl(ptr) >>  8) & 0xFF;
> +			ecc_code[i++] = (readl(ptr) >>	8) & 0xFF;
>  			ecc_code[i++] = readl(ptr) & 0xFF;
>  			ptr--;
>  		}
>  	} else {
> -		ptr = &gpmc_cfg->bch_result_0_3[0].bch_result_x[0];
> +		ptr = &gpmc_cfg->bch_result_0_3.bch_result_x[0];
>  		for (j = 0; j < 3; j++) {
>  			ecc_code[i++] = readl(ptr) & 0xFF;
> -			ecc_code[i++] = (readl(ptr) >>  8) & 0xFF;
> +			ecc_code[i++] = (readl(ptr) >>	8) & 0xFF;
>  			ecc_code[i++] = (readl(ptr) >> 16) & 0xFF;
>  			ecc_code[i++] = (readl(ptr) >> 24) & 0xFF;
>  			ptr++;
> @@ -304,6 +304,53 @@ static void omap_read_bch8_result(struct  
> mtd_info *mtd, uint8_t big_endian,
>  	}
>  }
> 
> +static void omap_read_bch16_result(struct mtd_info *mtd, uint8_t  
> big_endian,
> +				uint8_t *ecc_code)
> +{
> +	uint32_t *ptr;
> +	int8_t i = 0, j;
> +	uint32_t data;
> +
> +	if(big_endian) {
> +		ptr = &gpmc_cfg->bch_result_4_6.bch_result_x[2];
> +
> +		for (j = 0; j < 7; j++) {
> +			if(j == 3) {
> +				ptr =  
> &gpmc_cfg->bch_result_0_3.bch_result_x[3];
> +			}

Please put a space after "if".  Don't use braces around a single line.

> +
> +			data = readl(ptr);
> +			ptr--;
> +
> +			if(i > 0) {
> +				ecc_code[i++] = (data >> 24) & 0xFF;
> +				ecc_code[i++] = (data >> 16) & 0xFF;
> +			}
> +			ecc_code[i++] = (data >> 8) & 0xFF;
> +			ecc_code[i++] = data & 0xFF;
> +		}
> +		ecc_code[i++] = 0;
> +		ecc_code[i++] = 0;
> +	}
> +	else {

} else {

> +		ptr = &gpmc_cfg->bch_result_0_3.bch_result_x[0];
> +
> +		for (j = 0; j < 7; j++) {
> +			if(j == 4) {
> +				ptr =  
> &gpmc_cfg->bch_result_4_6.bch_result_x[0];
> +			}
> +
> +			data = readl(ptr);
> +			ptr++;
> +
> +			ecc_code[i++] = data & 0xFF;
> +			ecc_code[i++] = (data >> 8) & 0xFF;
> +			ecc_code[i++] = (data >> 16) & 0xFF;
> +			ecc_code[i++] = (data >> 24) & 0xFF;
> +		}
> +	}
> +}
> +
>  /*
>   * omap_ecc_disable - Disable H/W ECC calculation
>   *
> @@ -330,7 +377,7 @@ static void omap_rotate_ecc_bch(struct mtd_info  
> *mtd, uint8_t *calc_ecc,
>  	struct nand_chip *chip = mtd->priv;
>  	struct nand_bch_priv *bch = chip->priv;
>  	uint8_t n_bytes = 0;
> -	int8_t i, j;
> +	int8_t i;
> 
>  	switch (bch->type) {
>  	case ECC_BCH4:
> @@ -338,7 +385,12 @@ static void omap_rotate_ecc_bch(struct mtd_info  
> *mtd, uint8_t *calc_ecc,
>  		break;
> 
>  	case ECC_BCH16:
> -		n_bytes = 28;
> +		n_bytes = 26;
> +
> +		/* Last 2 register of ELM need to be zero */

s/register/registers/

> @@ -347,16 +399,17 @@ static void omap_rotate_ecc_bch(struct mtd_info  
> *mtd, uint8_t *calc_ecc,
>  		break;
>  	}
> 
> -	for (i = 0, j = (n_bytes-1); i < n_bytes; i++, j--)
> -		syndrome[i] =  calc_ecc[j];
> +	for (i = 0; i < n_bytes; i++) {
> +		syndrome[i] = calc_ecc[(n_bytes-1)-i];
> +	}

Please put spaces around binary operators such as '-' (and again, no  
braces around single lines).

>  void omap_nand_switch_ecc(int32_t hardware)
>  {
> +#ifndef CONFIG_AM33XX
>  	struct nand_chip *nand;
>  	struct mtd_info *mtd;
> 
>  	if (nand_curr_device < 0 ||
> -	    nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
> -	    !nand_info[nand_curr_device].name) {
> +		nand_curr_device >= CONFIG_SYS_MAX_NAND_DEVICE ||
> +		!nand_info[nand_curr_device].name) {
>  		printf("Error: Can't switch ecc, no devices  
> available\n");
>  		return;
>  	}

Please never have the continuation line of a conditional lined up with  
the body of the construct.

> @@ -646,19 +702,6 @@ void omap_nand_switch_ecc(int32_t hardware)
>  		nand->ecc.calculate = omap_calculate_ecc;
>  		omap_hwecc_init(nand);
>  		printf("HW ECC selected\n");
> -#ifdef CONFIG_AM33XX
> -	} else if (hardware == 2) {
> -		nand->ecc.mode = NAND_ECC_HW;
> -		nand->ecc.layout = &hw_bch8_nand_oob;
> -		nand->ecc.size = 512;
> -		nand->ecc.bytes = 14;
> -		nand->ecc.read_page = omap_read_page_bch;
> -		nand->ecc.hwctl = omap_enable_ecc_bch;
> -		nand->ecc.correct = omap_correct_data_bch;
> -		nand->ecc.calculate = omap_calculate_ecc_bch;
> -		omap_hwecc_init_bch(nand, NAND_ECC_READ);
> -		printf("HW BCH8 selected\n");
> -#endif
>  	} else {

Explain.

> diff --git a/include/linux/mtd/mtd-abi.h b/include/linux/mtd/mtd-abi.h
> index 8bdd231..6979a2a 100644
> --- a/include/linux/mtd/mtd-abi.h
> +++ b/include/linux/mtd/mtd-abi.h
> @@ -125,7 +125,7 @@ struct nand_oobfree {
>   */
>  struct nand_ecclayout {
>  	uint32_t eccbytes;
> -	uint32_t eccpos[128];
> +	uint32_t eccpos[208];
>  	uint32_t oobavail;
>  	struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES];
>  };

Changes to generic code should ideally be separate patches.

-Scott