[U-Boot] [PATCH v5] nand/denali: Adding Denali NAND driver support

[Chin Liang See]

Hi Masahiro,

On Wed, 2014-03-12 at 20:25 +0900, Masahiro Yamada wrote:
> Hello Chin,
> 
> 
> Here is a little more comments about v5.
> 
> > +static void get_toshiba_nand_para(void)
> > +{
> > +	uint32_t tmp;
> > +
> > +	/* Workaround to fix a controller bug which reports a wrong */
> > +	/* spare area size for some kind of Toshiba NAND device */
> > +	if ((readl(denali.flash_reg + DEVICE_MAIN_AREA_SIZE) == 4096) &&
> > +	    (readl(denali.flash_reg + DEVICE_SPARE_AREA_SIZE)
> > +		== 64)){
> > +		writel(216, denali.flash_reg + DEVICE_SPARE_AREA_SIZE);
> > +		tmp = readl(denali.flash_reg + DEVICES_CONNECTED) *
> > +			readl(denali.flash_reg + DEVICE_SPARE_AREA_SIZE);
> > +		writel(tmp, denali.flash_reg + LOGICAL_PAGE_SPARE_SIZE);
> > +#if SUPPORT_15BITECC
> > +		writel(15, denali.flash_reg + ECC_CORRECTION);
> > +#elif SUPPORT_8BITECC
> > +		writel(8, denali.flash_reg + ECC_CORRECTION);
> > +#endif
> 
> I can't understand why ECC_CORRECTION must be set here.
> Can you?
> If not, could you delete it?
> 

Yup, this is not necessary as we setup this at init function.
Removed


> 
> 
> > +static void get_hynix_nand_para(uint8_t device_id)
> > +{
> > +	uint32_t main_size, spare_size;
> > +
> > +	switch (device_id) {
> > +	case 0xD5: /* Hynix H27UAG8T2A, H27UBG8U5A or H27UCG8VFA */
> > +	case 0xD7: /* Hynix H27UDG8VEM, H27UCG8UDM or H27UCG8V5A */
> > +		writel(128, denali.flash_reg + PAGES_PER_BLOCK);
> > +		writel(4096, denali.flash_reg + DEVICE_MAIN_AREA_SIZE);
> > +		writel(224, denali.flash_reg + DEVICE_SPARE_AREA_SIZE);
> > +		main_size = 4096 *
> > +			readl(denali.flash_reg + DEVICES_CONNECTED);
> > +		spare_size = 224 *
> > +			readl(denali.flash_reg + DEVICES_CONNECTED);
> > +		writel(main_size, denali.flash_reg + LOGICAL_PAGE_DATA_SIZE);
> > +		writel(spare_size, denali.flash_reg + LOGICAL_PAGE_SPARE_SIZE);
> > +		writel(0, denali.flash_reg + DEVICE_WIDTH);
> > +#if SUPPORT_15BITECC
> > +		writel(15, denali.flash_reg + ECC_CORRECTION);
> > +#elif SUPPORT_8BITECC
> > +		writel(8, denali.flash_reg + ECC_CORRECTION);
> > +#endif
> 
> Ditto.
> 

Removed


> 
> 
> > +void denali_nand_init(struct nand_chip *nand)
> > +{
> > +	denali.flash_reg = (void  __iomem *)CONFIG_SYS_NAND_REGS_BASE;
> > +	denali.flash_mem = (void  __iomem *)CONFIG_SYS_NAND_DATA_BASE;
> > +
> > +	nand->chip_delay  = 0;
> > +#ifdef CONFIG_SYS_NAND_USE_FLASH_BBT
> > +	/* check whether flash got BBT table (located at end of flash). As we
> > +	 * use NAND_BBT_NO_OOB, the BBT page will start with
> 
> 
> 
> 	nand->chip_delay  = 0;
> is not necessary.
> Please delete.
> 
> 
> if  chip_delay  is zero,  chip_delay is set to 20
> in nand_set_defaults() function.
> 
> Anyway it is never used in Denali NAND driver,
> because we are overriding cmdfunc with denali_cmdfunc().
> 

Good catch, this is not required.
Initially I would let user to use other delay value.
As we are using own cmdfunc, this is not relevant.

Thanks
Chin Liang

> 
> 
> 
> 
> Best Regards
> Masahiro Yamada
>