[U-Boot,4/5] devkit8000 nand_spl: Add SPL NAND support to omap_gpmc driver

Add support for NAND_SPL to omap gpmc driver. This means adding nand_read_buf16 to read from GPMC 32bit buffer (16 here means 16bit bus!) and adding omap_dev_ready as indicator if the GPMC is ready.  

Signed-off-by: Simon Schwarz <schwarz@corscience.de>
--

Dear Simon Schwarz,

Am 28.06.2011 16:14, schrieb simonschwarzcor@googlemail.com:
> Add support for NAND_SPL to omap gpmc driver. This means adding nand_read_buf16 to read from GPMC 32bit buffer (16 here means 16bit bus!) and adding omap_dev_ready as indicator if the GPMC is ready.  

You also set some special ECC handling in this patch, please honour this
in your commit message!

> 
> Signed-off-by: Simon Schwarz <schwarz@corscience.de>
> --
> 
> diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
> index 99b9cef..7dfb05d 100644
> --- a/drivers/mtd/nand/omap_gpmc.c
> +++ b/drivers/mtd/nand/omap_gpmc.c
> @@ -29,6 +29,9 @@
>  #include <linux/mtd/nand_ecc.h>
>  #include <nand.h>
>  
> +

no additional empty line

> +#define GPMC_WAIT0_PIN_ACTIVE (1 << 8)

this is only used once, why don't you use just (1<<8) where needed? Is
there some special meaning with 8 bit shift, can you use a register name
instead? Should it be configurable in any way for other omap3 devices?

> +
>  static uint8_t cs;
>  static struct nand_ecclayout hw_nand_oob = GPMC_NAND_HW_ECC_LAYOUT;
>  
> @@ -61,6 +64,37 @@ static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
>  		writeb(cmd, this->IO_ADDR_W);
>  }
>  
> +/* Check wait pin as dev ready indicator */
> +int omap_dev_ready(struct mtd_info *mtd)
> +{
> +	return gpmc_cfg->status & GPMC_WAIT0_PIN_ACTIVE;
> +}
> +
> +#ifdef CONFIG_PRELOADER

Isn't that SPL related? Why is it required for SPL and not for standard
U-Boot?

> +
> +/**
> + * nand_read_buf16 - [DEFAULT] read chip data into buffer
> + * @mtd:    MTD device structure
> + * @buf:    buffer to store date
> + * @len:    number of bytes to read
> + *
> + * Default read function for 16bit buswith
> + *
> + * This function is based on nand_read_buf16 from nand_base.c. This version
> + * reads 32bit not 16bit although the bus only has 16bit.
> + */
> +static void gpmc_read_buf16(struct mtd_info *mtd, uint8_t *buf, int len)
> +{
> +	int i;
> +	struct nand_chip *chip = mtd->priv;
> +	u32 *p = (u32 *) buf;
> +	len >>= 2;
> +
> +	for (i = 0; i < len; i++)
> +		p[i] = readl(chip->IO_ADDR_R);
> +}
> +#endif
> +
>  /*
>   * omap_hwecc_init - Initialize the Hardware ECC for NAND flash in
>   *                   GPMC controller
> @@ -278,7 +312,9 @@ void omap_nand_switch_ecc(int32_t hardware)
>  	/* Update NAND handling after ECC mode switch */
>  	nand_scan_tail(mtd);
>  
> +	#ifndef CONFIG_SPL
>  	nand->options &= ~NAND_OWN_BUFFERS;
> +	#endif
>  }
>  
>  /*
> @@ -337,8 +373,23 @@ int board_nand_init(struct nand_chip *nand)
>  		nand->options |= NAND_BUSWIDTH_16;
>  
>  	nand->chip_delay = 100;
> +	nand->dev_ready = omap_dev_ready;
>  	/* Default ECC mode */
> +#ifndef CONFIG_PRELOADER

should't this some CONFIG_USE_SOFT_ECC (or whatever config variable
define that behaviour)?

>  	nand->ecc.mode = NAND_ECC_SOFT;
> +#else
> +	nand->ecc.mode = NAND_ECC_HW;
> +	nand->ecc.layout = &hw_nand_oob;
> +	nand->ecc.size = 512;
> +	nand->ecc.bytes = 24;

Ouch, these two values are extremely HW spwcific and need to be
configurable then.

> +	nand->ecc.hwctl = omap_enable_hwecc;
> +	nand->ecc.correct = omap_correct_data;
> +	nand->ecc.calculate = omap_calculate_ecc;

Isn't that some kind of CONFIG_NAND_BUSWDITH (or whatever config
variable define that behaviour) related setting?

> +	nand->read_buf = gpmc_read_buf16;
> +	omap_hwecc_init(nand);
> +#endif
>  
>  	return 0;
>  }
> +
> +

please remove these two additional empty lines

regards

Andreas Bießmann

Dear Andreas,

> You also set some special ECC handling in this patch, please honour this
> in your commit message!
will do.

>> +
>> +/**
>> + * nand_read_buf16 - [DEFAULT] read chip data into buffer
>> + * @mtd:    MTD device structure
>> + * @buf:    buffer to store date
>> + * @len:    number of bytes to read
>> + *
>> + * Default read function for 16bit buswith
>> + *
>> + * This function is based on nand_read_buf16 from nand_base.c. This version
>> + * reads 32bit not 16bit although the bus only has 16bit.
>> + */
>> +static void gpmc_read_buf16(struct mtd_info *mtd, uint8_t *buf, int len)
>> +{
>> +     int i;
>> +     struct nand_chip *chip = mtd->priv;
>> +     u32 *p = (u32 *) buf;
>> +     len >>= 2;
>> +
>> +     for (i = 0; i < len; i++)
>> +             p[i] = readl(chip->IO_ADDR_R);
>> +}
>> +#endif
>> +
>>  /*
>>   * omap_hwecc_init - Initialize the Hardware ECC for NAND flash in
>>   *                   GPMC controller
>> @@ -278,7 +312,9 @@ void omap_nand_switch_ecc(int32_t hardware)
>>       /* Update NAND handling after ECC mode switch */
>>       nand_scan_tail(mtd);
>>
>> +     #ifndef CONFIG_SPL
>>       nand->options &= ~NAND_OWN_BUFFERS;
>> +     #endif
>>  }
>>
>>  /*
>> @@ -337,8 +373,23 @@ int board_nand_init(struct nand_chip *nand)
>>               nand->options |= NAND_BUSWIDTH_16;
>>
>>       nand->chip_delay = 100;
>> +     nand->dev_ready = omap_dev_ready;
>>       /* Default ECC mode */
>> +#ifndef CONFIG_PRELOADER
>
> should't this some CONFIG_USE_SOFT_ECC (or whatever config variable
> define that behaviour)?

Its the default ECC setting this is software for standard U-Boot. Just
for the Preloader it is HW. So i think using CONFIG_PRELOADER is ok
here.


>> +     nand->ecc.size = 512;
>> +     nand->ecc.bytes = 24;
>
> Ouch, these two values are extremely HW spwcific and need to be
> configurable then.
will change.

>
>> +     nand->ecc.hwctl = omap_enable_hwecc;
>> +     nand->ecc.correct = omap_correct_data;
>> +     nand->ecc.calculate = omap_calculate_ecc;
>
> Isn't that some kind of CONFIG_NAND_BUSWDITH (or whatever config
> variable define that behaviour) related setting?
Sorry don't understand that. These three functions are not related to
the bus width IMHO.

Thank you for the feedback!
Simon