On Tue, 2014-08-19 at 16:28 +0800, Chin Liang See wrote: > From: Scott Wood [mailto:scottw...@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 <cl...@altera.com> > > Cc: Artem Bityutskiy <artem.bityuts...@linux.intel.com> > > Cc: David Woodhouse <david.woodho...@intel.com> > > Cc: Brian Norris <computersforpe...@gmail.com> > > Cc: Scott Wood <scottw...@freescale.com> > > Cc: Masahiro Yamada <yamad...@jp.panasonic.com> > > Signed-off-by: Masahiro Yamada <yamad...@jp.panasonic.com> > > Reviewed-by: Masahiro Yamada <yamad...@jp.panasonic.com> > > Tested-by: Masahiro Yamada <yamad...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot