[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver
Scott Wood
scottwood at freescale.com
Fri Apr 13 20:09:42 CEST 2012
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.
>>> + 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.
>> 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.
>>> + 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.
>>> + 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.
>>> +/**
>>> + * 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.
Can this controller support 4096-byte pages (or 8192)?
>>> + 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?
>>> +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.
This code also needs better namespacing -- this isn't applicable for all
NAND controllers/bindings.
-Scott
More information about the U-Boot
mailing list