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