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

[Masahiro Yamada]

Hi Chin, Scott,


On Thu, 19 Jun 2014 20:39:20 -0500
Scott Wood <scottwood at freescale.com> wrote:

> I see that this driver exists in Linux...  Is this patch related to a
> particular Linux SHA1?

If so, should it be described in git-log or the header part of denali.c ?


> > +/* 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.


Looks like this part is like follows in the current Kernel:

                        dev_dbg(denali->dev,
                        "NAND Reset operation timed out on bank %d\n", i);

And it was as follows, prior to commit 7cfffac06ca0:

                        nand_dbg_print(NAND_DBG_WARN,
                        "NAND Reset operation timed out on bank %d\n", i);


I think we can use  dev_dbg(...) in U-Boot too.
( <linux/compat.h> should be included in that case.)

If there is no reason to change it,  let's stick to dev_dbg().



> 
> 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.

Agreed.
It would make is easier to sync this file with the Kernel for future updates.



> > +/* 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.

DMA setup sequence is completely different between 32bit bus IP core and 64bit bus IP core.
That's why this #ifdef conditional is necessary.

CONFIG_NAND_DENALI_64BIT should be defined for 64bit bus IP core.

I will post Panasonic SoCs support series right after this patch is applied,
enabling CONFIG_SYS_NAND_DENALI_64BIT.

I requested Chin to add this macro.
I can document it in a follow-up patch.
(Or if  he could do that for me in v9, it would be really appriciated.)

Linux Kernel does not support  64bit bus Denali core yet,
but I have a plan to post a patch to support it lator.



Best Regards
Masahiro Yamada