From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Fri, 13 Apr 2012 13:09:42 -0500 Subject: [U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver In-Reply-To: References: <1326496256-5559-1-git-send-email-sjg@chromium.org> <1326496256-5559-6-git-send-email-sjg@chromium.org> <4F19F0E9.5080603@freescale.com> Message-ID: <4F886BE6.3020208@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 04/13/2012 12:42 PM, Simon Glass wrote: > Hi Scott, > > On Fri, Jan 20, 2012 at 2:55 PM, Scott Wood 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