Re: [U-Boot] [U-Boot, v8] nand/denali: Adding Denali NAND driver support
On Tue, 2014-08-19 at 16:28 +0800, Chin Liang See wrote:
From: Scott Wood [mailto:scottwood@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
Hi Chin,
On Tue, 19 Aug 2014 04:18:44 -0500 Chin Liang See clsee@altera.com wrote:
On Tue, 2014-08-19 at 16:28 +0800, Chin Liang See wrote:
From: Scott Wood [mailto:scottwood@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.
I know this driver is leveraged from Linux. Is it difficult to describe the particular SHA1 or tag you took it from?
+/* 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.
I pointed out that you could still use dev_err() and dev_warn as Linux and I don't see the necessity to diverge from Linux here.
Anyway your way work enough...
Best Regards Masahiro Yamada
participants (2)
-
Chin Liang See -
Masahiro Yamada