Hi Scott,
On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood scottwood@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@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