[U-Boot] [U-Boot, v8] nand/denali: Adding Denali NAND driver support

[Chin Liang See <clsee@altera.com>]

On Tue, 2014-08-19 at 16:28 +0800, Chin Liang See wrote:
> From: Scott Wood [mailto:scottwood at freescale.com] 
> Sent: Friday, June 20, 2014 9:39 AM
> To: Chin Liang See
> Cc: ZY - u-boot
> Subject: Re: [U-Boot,v8] nand/denali: Adding Denali NAND driver support
> 
> On Tue, Jun 10, 2014 at 12:42:19AM -0500, Chin Liang See wrote:
> > To add the Denali NAND driver support into U-Boot. It required
> > information such as register base address from configuration
> > header file  within include/configs folder.
> 
> This is hard to parse.  What exactly is required from include/configs and
> where is it documented?
> 
> I see that this driver exists in Linux...  Is this patch related to a
> particular Linux SHA1?
> 

Yup, this driver is leveraged from Linux. I will update the commit
message to mention about this.


> > Signed-off-by: Chin Liang See <clsee@altera.com>
> > Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> > Cc: David Woodhouse <David.Woodhouse@intel.com>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: Scott Wood <scottwood@freescale.com>
> > Cc: Masahiro Yamada <yamada.m@jp.panasonic.com>
> > Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> > Reviewed-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> > Tested-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> 
> Are these people on CC because they're involved in the U-Boot patch in
> some way, or is this just copy-and-paste from a Linux patch?

Actually they are part of reviewer for Linux Denali NAND driver. As they
didn't response on this patch, I will remove them.


> 
> > +/* Reset the flash controller */
> > +static uint32_t denali_nand_reset(struct denali_nand_info *denali)
> > +{
> > +	uint32_t i;
> > +
> > +	for (i = 0; i < denali->max_banks; i++)
> > +		writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
> > +		       denali->flash_reg + INTR_STATUS(i));
> > +
> > +	for (i = 0; i < denali->max_banks; i++) {
> > +		writel(1 << i, denali->flash_reg + DEVICE_RESET);
> > +		while (!(readl(denali->flash_reg + INTR_STATUS(i)) &
> > +			(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
> > +			if (readl(denali->flash_reg + INTR_STATUS(i)) &
> > +				INTR_STATUS__TIME_OUT)
> > +				debug(KERN_DEBUG "NAND Reset operation "
> > +					"timed out on bank %d\n", i);
> > +	}
> 
> WARNING: quoted string split across lines
> #283: FILE: drivers/mtd/nand/denali.c:203:
> +                               debug(KERN_DEBUG "NAND Reset operation "
> +                                       "timed out on bank %d\n", i);
> 
> (likewise elsewhere)
> 
> This instance is not even from Linux -- and where does KERN_DEBUG come
> from?  The Linux driver has never used it.

Its defined as empty. I will remove them.


> 
> > +#if ONFI_BLOOM_TIME
> > +	if ((en_hi * CLK_X) < (treh[mode] + 2))
> > +		en_hi++;
> > +#endif
> 
> When would this #if not be true?

I suspect this code is added due to some issue the original developer
hit during high speed data transfer. Nevertheless, its default in Linux
too. With that, I will remove the macro.


> 
> > +	if ((en_lo + en_hi) * CLK_X < trc[mode])
> > +		en_lo += DIV_ROUND_UP((trc[mode] - (en_lo + en_hi) * CLK_X),
> > +				      CLK_X);
> > +
> > +	if ((en_lo + en_hi) < CLK_MULTI)
> > +		en_lo += CLK_MULTI - en_lo - en_hi;
> > +
> > +	while (dv_window < 8) {
> > +		data_invalid_rhoh = en_lo * CLK_X + trhoh[mode];
> > +
> > +		data_invalid_rloh = (en_lo + en_hi) * CLK_X + trloh[mode];
> > +
> > +		data_invalid =
> > +		    data_invalid_rhoh <
> > +		    data_invalid_rloh ? data_invalid_rhoh : data_invalid_rloh;
> > +
> > +		dv_window = data_invalid - trea[mode];
> > +
> > +		if (dv_window < 8)
> > +			en_lo++;
> > +	}
> > +
> > +	acc_clks = DIV_ROUND_UP(trea[mode], CLK_X);
> > +
> > +	while (((acc_clks * CLK_X) - trea[mode]) < 3)
> > +		acc_clks++;
> > +
> > +	if ((data_invalid - acc_clks * CLK_X) < 2)
> > +		debug(KERN_WARNING "%s, Line %d: Warning!\n",
> > +		      __FILE__, __LINE__);
> > +
> > +	addr_2_data = DIV_ROUND_UP(tadl[mode], CLK_X);
> > +	re_2_we = DIV_ROUND_UP(trhw[mode], CLK_X);
> > +	re_2_re = DIV_ROUND_UP(trhz[mode], CLK_X);
> > +	we_2_re = DIV_ROUND_UP(twhr[mode], CLK_X);
> > +	cs_cnt = DIV_ROUND_UP((tcs[mode] - trp[mode]), CLK_X);
> > +	if (!tclsrising)
> > +		cs_cnt = DIV_ROUND_UP(tcs[mode], CLK_X);
> > +	if (cs_cnt == 0)
> > +		cs_cnt = 1;
> > +
> > +	if (tcea[mode]) {
> > +		while (((cs_cnt * CLK_X) + trea[mode]) < tcea[mode])
> > +			cs_cnt++;
> > +	}
> > +
> > +#if MODE5_WORKAROUND
> > +	if (mode == 5)
> > +		acc_clks = 5;
> > +#endif
> 
> Likewise, this is dead code.
> 
> If these are meant to be configurable, they need to be named as config
> symbols, documented, and located in a config file rather than denali.h --
> and ideally there should be at least one board that sets the option and
> at least one that does not set it, so that both possibilities get build
> coverage.
> 
> I see that the Linux driver has the same thing, so perhaps we can excuse
> it if it's not actually meant to be altered by a user, but in that case
> the driver ought to actually correspond to a specific SHA1 of the Linux
> driver.

It seems these debug code had upstreamed into Linux git. I will remove
them. I also ensured all these similar macro are removed too.


> 
> > +/* validation function to verify that the controlling software is making
> > + * a valid request
> > + */
> > +static inline bool is_flash_bank_valid(int flash_bank)
> > +{
> > +	return (flash_bank >= 0 && flash_bank < 4);
> > +}
> 
> ERROR: return is not a function, parentheses are not required
> #589: FILE: drivers/mtd/nand/denali.c:509:
> +       return (flash_bank >= 0 && flash_bank < 4);

Removed

> 
> > +/* setups the HW to perform the data DMA */
> > +static void denali_setup_dma(struct denali_nand_info *denali, int op)
> > +{
> > +	uint32_t mode;
> > +	const int page_count = 1;
> > +	uint32_t addr = (uint32_t)denali->buf.dma_buf;
> > +
> > +	flush_dcache_range(addr, addr + sizeof(denali->buf.dma_buf));
> > +
> > +#ifdef CONFIG_NAND_DENALI_64BIT
> 
> Needs to be documented -- and depending on what it means,
> should possibly be CONFIG_SYS_NAND_DENALI_64BIT.  The Linux driver seems
> to only have the "else" variant.

Yup, I added SYS to ensure standard macro definition. 64 bit variant is
the one used by Panasonic. I will add more explanation about 32 bit and
64 bit variant in comments.


> 
> > +static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
> > +			const uint8_t *buf, bool raw_xfer, int oob_required)
> > +{
> > +	struct denali_nand_info *denali = mtd_to_denali(mtd);
> > +
> > +	uint32_t irq_status = 0;
> > +	uint32_t irq_mask = INTR_STATUS__DMA_CMD_COMP;
> > +
> > +	denali->status = 0;
> > +
> > +	/* copy buffer into DMA buffer */
> > +	memcpy((void *)denali->buf.dma_buf, buf, mtd->writesize);
> 
> Why do you have this cast?

Removed 

> 
> > +	/* need extra memcpoy for raw transfer */
> 
> memcpoy?

Fixed


> 
> > +	/* check whether any ECC error */
> > +	if (irq_status & INTR_STATUS__ECC_UNCOR_ERR) {
> > +		/* is the ECC cause by erase page, check using read_page_raw */
> > +		debug("  Uncorrected ECC detected\n");
> > +		denali_read_page_raw(mtd, chip, buf, oob_required, denali->page);
> 
> WARNING: line over 80 characters
> #1044: FILE: drivers/mtd/nand/denali.c:964:
> +               denali_read_page_raw(mtd, chip, buf, oob_required, denali->page);
> 

Fixed

Thanks
Chin Liang

> 
> -Scott