Hi Chin, Scott,
On Thu, 19 Jun 2014 20:39:20 -0500 Scott Wood scottwood@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