From mboxrd@z Thu Jan 1 00:00:00 1970 From: Masahiro Yamada Date: Tue, 24 Jun 2014 19:24:25 +0900 Subject: [U-Boot] [U-Boot, v8] nand/denali: Adding Denali NAND driver support In-Reply-To: <20140620013920.GA6586@home.buserror.net> References: <1402378939-2320-1-git-send-email-clsee@altera.com> <20140620013920.GA6586@home.buserror.net> Message-ID: <20140624192425.9367.AA925319@jp.panasonic.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Chin, Scott, On Thu, 19 Jun 2014 20:39:20 -0500 Scott Wood 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. ( 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