Hi Stephen, On Fri, Jan 20, 2012 at 9:31 AM, Stephen Warren <swar...@nvidia.com> wrote: > On 01/13/2012 04:10 PM, Simon Glass wrote: >> From: Jim Lin <ji...@nvidia.com> >> >> A device tree is used to configure the NAND, including memory >> timings and block/pages sizes. >> >> If this node is not present or is disabled, then NAND will not >> be initialized. >> >> Signed-off-by: Simon Glass <s...@chromium.org> > >> diff --git a/drivers/mtd/nand/tegra2_nand.c b/drivers/mtd/nand/tegra2_nand.c > >> +/** >> + * Wait for command completion >> + * >> + * @param reg nand_ctlr structure >> + * @return >> + * 1 - Command completed >> + * 0 - Timeout >> + */ >> +static int nand_waitfor_cmd_completion(struct nand_ctlr *reg) >> +{ >> + int i; >> + u32 reg_val; >> + >> + for (i = 0; i < NAND_CMD_TIMEOUT_MS * 1000; i++) { >> + if ((readl(®->command) & CMD_GO) || >> + !(readl(®->status) & >> + STATUS_RBSY0) || >> + !(readl(®->isr) & >> + ISR_IS_CMD_DONE)) { >> + udelay(1); >> + continue; >> + } >> + reg_val = readl(®->dma_mst_ctrl); >> + /* >> + * If DMA_MST_CTRL_EN_A_ENABLE or >> + * DMA_MST_CTRL_EN_B_ENABLE is set, >> + * that means DMA engine is running, then we >> + * have to wait until >> + * DMA_MST_CTRL_IS_DMA_DONE >> + * is cleared for DMA transfer completion. >> + */ >> + if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE | >> + DMA_MST_CTRL_EN_B_ENABLE)) { >> + if (reg_val & DMA_MST_CTRL_IS_DMA_DONE) >> + return 1; >> + } else >> + return 1; >> + udelay(1); > > > To be more consistent with the first if/continue block, wouldn't it be > better to recast that last if test and udelay as: > > if (reg_val & (DMA_MST_CTRL_EN_A_ENABLE | > DMA_MST_CTRL_EN_B_ENABLE)) { > if (!(reg_val & DMA_MST_CTRL_IS_DMA_DONE)) { > udelay(1); > continue; > } > } > > break;
Yes that is better - but I've split it up a bit as it is too ugly. Also I do want to actually make use of the udelay that is there, rather than breaking out of the loop by default. > > >> +/** >> + * [DEFAULT] Send command to NAND device >> + * >> + * @param mtd MTD device structure >> + * @param command the command to be sent >> + * @param column the column address for this command, -1 if none >> + * @param page_addr the page address for this command, -1 if none >> + */ >> +static void nand_command(struct mtd_info *mtd, unsigned int command, >> + int column, int page_addr) >> +{ >> + register struct nand_chip *chip = mtd->priv; >> + struct nand_info *info; >> + >> + info = (struct nand_info *) chip->priv; >> + >> + /* >> + * Write out the command to the device. >> + */ >> + if (mtd->writesize < 2048) { >> + /* >> + * Only command NAND_CMD_RESET or NAND_CMD_READID will come >> + * here before mtd->writesize is initialized, we don't have >> + * any action here because page size of NAND HY27UF084G2B >> + * is 2048 bytes and mtd->writesize will be 2048 after >> + * initialized. >> + */ > > What if the NAND flash doesn't have a page size of 2048 bytes? The > driver shouldn't make such assumptions about the flash chip that happens > to be connected. Should the if above be: > > if (mtd->writesize == 0) > > to be generic? Well I have just removed this, as Scott suggested. > > Should this if branch validate that the command being executed is a > legitimate command for the not-yet-fully-initialized case? Removed > >> +/** >> + * Set up NAND bus width and page size >> + * >> + * @param info nand_info structure >> + * @param *reg_val address of reg_val >> + * @return none - value is set in reg_val >> + */ >> +static void set_bus_width_page_size(struct fdt_nand *config, >> + u32 *reg_val) >> +{ >> + if (config->width == 8) >> + *reg_val = CFG_BUS_WIDTH_8BIT; >> + else > > Shouldn't that be else if (config->width == 16) > >> + *reg_val = CFG_BUS_WIDTH_16BIT; > > > ... and there be an else clause that returns an error? Done, have also called it from init as Scott suggests. > >> + >> + if (config->page_data_bytes == 256) >> + *reg_val |= CFG_PAGE_SIZE_256; >> + else if (config->page_data_bytes == 512) >> + *reg_val |= CFG_PAGE_SIZE_512; >> + else if (config->page_data_bytes == 1024) >> + *reg_val |= CFG_PAGE_SIZE_1024; >> + else if (config->page_data_bytes == 2048) >> + *reg_val |= CFG_PAGE_SIZE_2048; > > And similarly, and else clause that returns an error here? Done > > -- > nvpublic Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot