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

Simon Glass sjg at chromium.org
Fri Apr 13 20:25:20 CEST 2012


Hi Scott,

On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood <scottwood at freescale.com> wrote:
> On 04/13/2012 12:42 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Fri, Jan 20, 2012 at 2:55 PM, Scott Wood <scottwood at freescale.com> wrote:
>>> On 01/13/2012 05:10 PM, Simon Glass wrote:
>>>> +struct nand_info nand_ctrl;
>>>
>>> static (or better, dynamic).
>>
>> Done, what is dynamic?
>
> Allocate the structure dynamically in your init function, and write the
> code in such a way as to allow multiple instances.  Not mandatory if you
> really think you'll never have multiple instances, but would be nice and
> shouldn't complicate things much.

There can be only one in Tegra, at least with current devices.

>
>>>> +     struct nand_info *info;
>>>> +
>>>> +     info = (struct nand_info *) chip->priv;
>>>> +
>>>> +     dword_read = readl(&info->reg->resp);
>>>> +     dword_read = dword_read >> (8 * info->pio_byte_index);
>>>> +     info->pio_byte_index++;
>>>> +     return (uint8_t) dword_read;
>>>
>>> No space after casts.
>>
>> I think I have fixed that globally.
>>
>>>
>>> What happens when pio_byte_index > 3?  Please don't assume that upper
>>> bits will be ignored, even if that happens to be true on this chip.
>>
>> I added an assert() - there is no way to return an error here. I'm not
>> sure what you mean by upper bits - there is a uint8_t cast.
>
> By upper bits I mean in the shift count.  Don't assume that
> dword_read >> 32 is the same as dword_read >> 0.  It's undefined in C.

OK I see, I have handled that.

>
>>> How does info->reg->resp work?  You don't seem to be choosing the dword
>>> to read based on pio_byte_index, which suggests that the act of reading
>>> resp changes what will be read the next time.  But, you read it on each
>>> byte, which would advance resp four times too fast if it's simply
>>> returning a new dword each time.
>>>
>>> If the hardware is really auto-advancing resp only on every fourth
>>> access, that needs a comment.
>>
>> I was just looking at that. Have added a comment.
>>
>>>
>>>> +/* Hardware specific access to control-lines */
>>>> +static void nand_hwcontrol(struct mtd_info *mtd, int cmd,
>>>> +     unsigned int ctrl)
>>>> +{
>>>> +}
>>>
>>> Don't implement this at all if it doesn't make sense for this hardware.
>>
>> It isn't implemented (the function is empty), but if it is not defined
>> then the NAND layer will die when it calls a null function. What
>> should I do here?
>
> It should only be called if you don't replace nand_command[_lp] and
> nand_select_chip.  If it's because of nand_select_chip, I'd rather see a
> dummy implementation of that.

OK I see, will change it.

>
>>>> +     case NAND_CMD_READ0:
>>>> +             writel(NAND_CMD_READ0, &info->reg->cmd_reg1);
>>>> +             writel(NAND_CMD_READSTART, &info->reg->cmd_reg2);
>>>> +             writel((page_addr << 16) | (column & 0xFFFF),
>>>> +                     &info->reg->addr_reg1);
>>>> +             writel(page_addr >> 16, &info->reg->addr_reg2);
>>>> +             return;
>>>> +     case NAND_CMD_SEQIN:
>>>> +             writel(NAND_CMD_SEQIN, &info->reg->cmd_reg1);
>>>> +             writel(NAND_CMD_PAGEPROG, &info->reg->cmd_reg2);
>>>> +             writel((page_addr << 16) | (column & 0xFFFF),
>>>> +                     &info->reg->addr_reg1);
>>>> +             writel(page_addr >> 16,
>>>> +                     &info->reg->addr_reg2);
>>>> +             return;
>>>> +     case NAND_CMD_PAGEPROG:
>>>> +             return;
>>>> +     case NAND_CMD_ERASE1:
>>>> +             writel(NAND_CMD_ERASE1, &info->reg->cmd_reg1);
>>>> +             writel(NAND_CMD_ERASE2, &info->reg->cmd_reg2);
>>>> +             writel(page_addr, &info->reg->addr_reg1);
>>>> +             writel(CMD_GO | CMD_CLE | CMD_ALE |
>>>> +                     CMD_SEC_CMD | CMD_CE0 | CMD_ALE_BYTES3,
>>>> +                     &info->reg->command);
>>>> +             break;
>>>> +     case NAND_CMD_RNDOUT:
>>>> +             return;
>>>
>>> Don't silently ignore RNDOUT -- if you don't support it and it happens,
>>> complain loudly.
>>
>> OK, I added a printf(), that should be loud enough. Would prefer a
>> debug() though.
>
> I don't think it should be debug() -- this is indicating a problem (you
> could be losing data), not just potentially helpful information.

Yes, the API should be changed to return a value, then a debug might be useful.

>
>>>> +     case NAND_CMD_ERASE2:
>>>> +             return;
>>>> +     case NAND_CMD_STATUS:
>>>> +             writel(NAND_CMD_STATUS, &info->reg->cmd_reg1);
>>>> +             writel(CMD_GO | CMD_CLE | CMD_PIO | CMD_RX
>>>> +                     | (CMD_TRANS_SIZE_BYTES1 <<
>>>> +                             CMD_TRANS_SIZE_SHIFT)
>>>> +                     | CMD_CE0,
>>>> +                     &info->reg->command);
>>>> +             info->pio_byte_index = 0;
>>>> +             break;
>>>> +     case NAND_CMD_RESET:
>>>> +             writel(NAND_CMD_RESET, &info->reg->cmd_reg1);
>>>> +             writel(CMD_GO | CMD_CLE | CMD_CE0,
>>>> +                     &info->reg->command);
>>>> +             break;
>>>> +     default:
>>>> +             return;
>>>
>>> Likewise, complain if you get an unrecognized command.
>>
>> Done - I wonder why we can't just return an error?
>
> Because the interface wasn't designed to allow that, unfortunately.  If
> you want to change that, start in Linux.

http://www.youtube.com/watch?v=K8E_zMLCRNg

>
>>>> +/**
>>>> + * 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
>>>> +             *reg_val = CFG_BUS_WIDTH_16BIT;
>>>> +
>>>> +     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;
>>>
>>> Really, page sizes of 256 and 1024 bytes?
>>>
>>> From elsewhere in the driver you only support 2048 byte pages, so why
>>> not just check for that and error (not silently continue) if it's
>>> anything else?
>>
>> I don't think it is that bad - I believe the driver should all the
>> options, although I have not tested it or gone into it carefully. Have
>> added error checking.
>
> I've never heard of a NAND chip with 1024-byte pages, and the only
> reference to 256-byte pages I've seen is in the "museum IDs" section of
> the ID table.

I will remove them then.

>
> Can this controller support 4096-byte pages (or 8192)?

Have added 4096, but I don't think it supports 8192 (shown as
'reserved' in datasheet).

>
>>>> +     if (!is_writing) {
>>>> +             invalidate_dcache_range((unsigned long) buf,
>>>> +                     ((unsigned long) buf) +
>>>> +                     (1 << chip->page_shift));
>>>> +     } else {
>>>> +             flush_dcache_range((unsigned long) buf,
>>>> +                     ((unsigned long) buf) +
>>>> +                     (1 << chip->page_shift));
>>>> +     }
>>>
>>> Please factor out all this cache stuff into a dma prepare function that
>>> takes start, length, and is_writing.  Is it even really needed to
>>> invalidate rather than flush in the read case?  It doesn't look like
>>> these buffers are even going to be cacheline-aligned...
>>
>> Done
>>
>> I am not keen on adding cache alignment into every driver - IMO this
>> should happen at the level above as with MMC, USB, etc. A fairly
>> trivial fix to nand_base caches most of the problems. I will include a
>> patch in my next series. Anyway for now, Tegra has cache off because
>> of all these warnings.
>
> Why would nand_base be in a position to know that you have special
> alignment needs?  Most NAND controllers don't do DMA, and many systems
> don't require cacheline alignment for DMA.  Linux hasn't needed this,
> why does U-Boot?

There is now an ARCH_DMA_MINALIGN define in U-Boot, and a
ALLOC_CACHE_ALIGN_BUFFER macro just for this purpose. There has been
some effort in making things like ext2, mmc and USB work properly with
DMA alignment. I don't see any need to make it hard to the driver - if
we can avoid bounce buffers we should.

>
>>>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>>>> +{
>>>> +     int err;
>>>> +
>>>> +     config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>>> +     config->enabled = fdtdec_get_is_enabled(blob, node);
>>>> +     config->width = fdtdec_get_int(blob, node, "width", 8);
>>>> +     err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>>>> +     if (err)
>>>> +             return err;
>>>> +     err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>>>> +                     config->timing, FDT_NAND_TIMING_COUNT);
>>>> +     if (err < 0)
>>>> +             return err;
>>>> +
>>>> +     /* Now look up the controller and decode that */
>>>> +     node = fdt_next_node(blob, node, NULL);
>>>> +     if (node < 0)
>>>> +             return node;
>>>> +
>>>> +     config->page_data_bytes = fdtdec_get_int(blob, node,
>>>> +                                             "page-data-bytes", -1);
>>>> +     config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>>>> +                                             "tag-ecc-bytes", -1);
>>>> +     config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>>>> +     config->data_ecc_bytes = fdtdec_get_int(blob, node,
>>>> +                                             "data-ecc-bytes", -1);
>>>> +     config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>>>> +                                             "skipped-spare-bytes", -1);
>>>> +     config->page_spare_bytes = fdtdec_get_int(blob, node,
>>>> +                                             "page-spare-bytes", -1);
>>>
>>> Has this binding been accepted into Linux's documentation or another
>>> canonical source?
>>
>> No
>
> It would be good to get that settled first.  I assume you'll want to use
> this binding with Linux eventually.

I am not sure - from what I hear Linux does ID lookup instead - but
really this binding is only useful without ONFI support I suspect.

>
> This code also needs better namespacing -- this isn't applicable for all
> NAND controllers/bindings.

Do you mean the "nvidia," prefix?

I will send another version of the series with comments received so
far addressed.

>
> -Scott
>

Regards,
Simon


More information about the U-Boot mailing list