[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Stephen Warren <swarren@nvidia.com>]

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(&reg->command) & CMD_GO) ||
> +                       !(readl(&reg->status) &
> +                               STATUS_RBSY0) ||
> +                       !(readl(&reg->isr) &
> +                               ISR_IS_CMD_DONE)) {
> +                       udelay(1);
> +                       continue;
> +               }
> +               reg_val = readl(&reg->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;


> +/**
> + * [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?

Should this if branch validate that the command being executed is a
legitimate command for the not-yet-fully-initialized case?

> +/**
> + * 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?

> +
> +       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?

-- 
nvpublic