Hi Stephen,
On Fri, Jan 20, 2012 at 9:31 AM, Stephen Warren swarren@nvidia.com wrote:
On 01/13/2012 04:10 PM, Simon Glass wrote:
From: Jim Lin jilin@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 sjg@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