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

[Simon Glass <sjg@chromium.org>]

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(&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;

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