From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Fri, 13 Apr 2012 13:34:09 -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> <4F886BE6.3020208@freescale.com> Message-ID: <4F8871A1.4080908@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 01:25 PM, Simon Glass wrote: > Hi Scott, > > On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood wrote: >> On 04/13/2012 12:42 PM, Simon Glass wrote: >>> 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. I understand the desire to avoid bounce buffers, but I also don't want to introduce unnecessary differences between Linux and U-Boot in nand_base.c. How does your Linux driver handle this? >>>>> +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. I'm not sure what difference you're seeing between U-Boot and Linux. Both U-Boot and Linux uses the ID table in the absence of ONFI. >> This code also needs better namespacing -- this isn't applicable for all >> NAND controllers/bindings. > > Do you mean the "nvidia," prefix? No, I mean the fact that this is a global function called "fdt_decode_nand", taking a "struct fdt_nand" argument, that is specific to one controller type. -Scott