| Message ID | 1318759804-18688-2-git-send-email-simonschwarzcor@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Simon Schwarz, In message <1318759804-18688-2-git-send-email-simonschwarzcor@gmail.com> you wrote: > This adds DMA copy for the nand spl implementation. If CONFIG_SPL_DMA_SUPPORT > is defined the DMA is used. > > Based on DMA driver patch: > http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/109744/focus=109747 > > Signed-off-by: Simon Schwarz <simonschwarzcor@gmail.com> > Cc: scottwood@freescale.com > Cc: s-paulraj@ti.com > --- > drivers/mtd/nand/nand_spl_simple.c | 185 ++++++++++++++++++++++++++++++++++-- > 1 files changed, 176 insertions(+), 9 deletions(-) ... > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { > + res += omap3_dma_conf_transfer(0, nand_chip.IO_ADDR_R, > + (uint32_t *)p, CONFIG_SYS_NAND_ECCSIZE/4); IIUC, drivers/mtd/nand/nand_spl_simple.c is a global, architecture independent file. However, you are adding OMAP3 specific code here. If we did the same for all other potentially supported architectures and SoCs, we'd soon have a serious mess. Please move architecture / SoC specific code out of such global files. Best regards, Wolfgang Denk
On 10/16/2011 05:10 AM, Simon Schwarz wrote: > This adds DMA copy for the nand spl implementation. If CONFIG_SPL_DMA_SUPPORT > is defined the DMA is used. > > Based on DMA driver patch: > http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/109744/focus=109747 As Wolfgang pointed out, this doesn't belong here. Create your own alternate SPL driver if your hardware doesn't work with the simple one (similar to the not-yet-migrated nand_spl/nand_boot_fsl_elbc.c, nand_spl/nand_boot_fsl_ifc.c, etc). > @@ -46,11 +59,11 @@ static int nand_command(int block, int page, uint32_t offs, > this->cmd_ctrl(&mtd, offs, NAND_CTRL_ALE | NAND_CTRL_CHANGE); > this->cmd_ctrl(&mtd, page_addr & 0xff, NAND_CTRL_ALE); /* A[16:9] */ > this->cmd_ctrl(&mtd, (page_addr >> 8) & 0xff, > - NAND_CTRL_ALE); /* A[24:17] */ > + NAND_CTRL_ALE); /* A[24:17] */ > #ifdef CONFIG_SYS_NAND_4_ADDR_CYCLE > /* One more address cycle for devices > 32MiB */ > this->cmd_ctrl(&mtd, (page_addr >> 16) & 0x0f, > - NAND_CTRL_ALE); /* A[28:25] */ > + NAND_CTRL_ALE); /* A[28:25] */ > #endif Please refrain from making random unrelated whitespace changes in a patch that also makes functional changes, particularly when they are extensive enough to make it hard to spot the functional changes. In this particular case, I think the whitespace was fine the way it was; the continuation lines were nicely aligned. -Scott
Dear Scott, On 10/25/2011 08:24 PM, Scott Wood wrote: > On 10/16/2011 05:10 AM, Simon Schwarz wrote: >> This adds DMA copy for the nand spl implementation. If CONFIG_SPL_DMA_SUPPORT >> is defined the DMA is used. >> >> Based on DMA driver patch: >> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/109744/focus=109747 > > As Wolfgang pointed out, this doesn't belong here. Create your own > alternate SPL driver if your hardware doesn't work with the simple one > (similar to the not-yet-migrated nand_spl/nand_boot_fsl_elbc.c, > nand_spl/nand_boot_fsl_ifc.c, etc). > Hm. The naming of the functions was a fault. Will rename the calls in nand_spl_simple to remove omap parts. So omap3_dma_wait_for_transfer will become dma_wait_for_transfer etc. So a board which intents to use DMA in SPL can implement these functions. Would this be ok? A whole new driver is IMHO not the right thing as there is too much duplicated code then. >> @@ -46,11 +59,11 @@ static int nand_command(int block, int page, uint32_t offs, >> this->cmd_ctrl(&mtd, offs, NAND_CTRL_ALE | NAND_CTRL_CHANGE); >> this->cmd_ctrl(&mtd, page_addr& 0xff, NAND_CTRL_ALE); /* A[16:9] */ >> this->cmd_ctrl(&mtd, (page_addr>> 8)& 0xff, >> - NAND_CTRL_ALE); /* A[24:17] */ >> + NAND_CTRL_ALE); /* A[24:17] */ >> #ifdef CONFIG_SYS_NAND_4_ADDR_CYCLE >> /* One more address cycle for devices> 32MiB */ >> this->cmd_ctrl(&mtd, (page_addr>> 16)& 0x0f, >> - NAND_CTRL_ALE); /* A[28:25] */ >> + NAND_CTRL_ALE); /* A[28:25] */ >> #endif > > Please refrain from making random unrelated whitespace changes in a > patch that also makes functional changes, particularly when they are > extensive enough to make it hard to spot the functional changes. > > In this particular case, I think the whitespace was fine the way it was; > the continuation lines were nicely aligned. If I remember right I changed these because of checkpatch errors. Regards Simon
On 10/31/2011 03:56 AM, Simon Schwarz wrote: > Dear Scott, > > On 10/25/2011 08:24 PM, Scott Wood wrote: >> On 10/16/2011 05:10 AM, Simon Schwarz wrote: >>> This adds DMA copy for the nand spl implementation. If CONFIG_SPL_DMA_SUPPORT >>> is defined the DMA is used. >>> >>> Based on DMA driver patch: >>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/109744/focus=109747 >> >> As Wolfgang pointed out, this doesn't belong here. Create your own >> alternate SPL driver if your hardware doesn't work with the simple one >> (similar to the not-yet-migrated nand_spl/nand_boot_fsl_elbc.c, >> nand_spl/nand_boot_fsl_ifc.c, etc). >> > > Hm. The naming of the functions was a fault. Will rename the calls in > nand_spl_simple to remove omap parts. So > omap3_dma_wait_for_transfer > will become > dma_wait_for_transfer > etc. > > So a board which intents to use DMA in SPL can implement these > functions. Would this be ok? What would the semantics of a generic dma_wait_for_transfer() be? I just don't see how this is generic at all, whatever the name. > A whole new driver is IMHO not the right thing as there is too much > duplicated code then. So factor the common bits out into a separate file. >>> @@ -46,11 +59,11 @@ static int nand_command(int block, int page, uint32_t offs, >>> this->cmd_ctrl(&mtd, offs, NAND_CTRL_ALE | NAND_CTRL_CHANGE); >>> this->cmd_ctrl(&mtd, page_addr& 0xff, NAND_CTRL_ALE); /* A[16:9] */ >>> this->cmd_ctrl(&mtd, (page_addr>> 8)& 0xff, >>> - NAND_CTRL_ALE); /* A[24:17] */ >>> + NAND_CTRL_ALE); /* A[24:17] */ >>> #ifdef CONFIG_SYS_NAND_4_ADDR_CYCLE >>> /* One more address cycle for devices> 32MiB */ >>> this->cmd_ctrl(&mtd, (page_addr>> 16)& 0x0f, >>> - NAND_CTRL_ALE); /* A[28:25] */ >>> + NAND_CTRL_ALE); /* A[28:25] */ >>> #endif >> >> Please refrain from making random unrelated whitespace changes in a >> patch that also makes functional changes, particularly when they are >> extensive enough to make it hard to spot the functional changes. >> >> In this particular case, I think the whitespace was fine the way it was; >> the continuation lines were nicely aligned. > > > If I remember right I changed these because of checkpatch errors. I believe checkpatch only complains when you have 8 or more spaces in a row, which isn't the case here. I don't think there's any prohibition on lining things up with single-column granularity. Further, checkpatch should not be complaining about lines that you don't touch. Where reformatting is warranted, it should be a separate patch. -Scott
Hi Scott On 10/31/2011 10:22 PM, Scott Wood wrote: > What would the semantics of a generic dma_wait_for_transfer() be? > > I just don't see how this is generic at all, whatever the name. > Hm. It would be a check if the given DMA channel is active - and if it is busy waiting for it. So, what would then be a generic interface for DMA? I see that this is a verrry basic solution - but where do you see the actual problems implementing this interface for other DMA controllers? Or do you think that the interface is to simple? >> A whole new driver is IMHO not the right thing as there is too much >> duplicated code then. > > So factor the common bits out into a separate file. > I haven't given up on the general solution yet. If I don't see another way I will do that. Regards Simon
On 11/02/2011 04:57 AM, Simon Schwarz wrote: > Hi Scott > > On 10/31/2011 10:22 PM, Scott Wood wrote: > >> What would the semantics of a generic dma_wait_for_transfer() be? >> >> I just don't see how this is generic at all, whatever the name. >> > > Hm. It would be a check if the given DMA channel is active - and if it > is busy waiting for it. > > So, what would then be a generic interface for DMA? I see that this is a > verrry basic solution - but where do you see the actual problems > implementing this interface for other DMA controllers? Or do you think > that the interface is to simple? I'd stick with something closer to the read_buf() interface -- something like read_buf_async() and wait_for_async(). Let the controller driver deal with the details of how DMA is done. Parameter is the mtd pointer, not a channel number. Certainly all the DMA setup stuff in nand_spl_load_image() needs to go elsewhere. >>> A whole new driver is IMHO not the right thing as there is too much >>> duplicated code then. I think it can be done with less duplication than in this patch -- should only need some ifdefs in the current code. There's no need to support both modes of operation in one SPL image, right? -Scott
diff --git a/drivers/mtd/nand/nand_spl_simple.c b/drivers/mtd/nand/nand_spl_simple.c index 71491d4..b45322b 100644 --- a/drivers/mtd/nand/nand_spl_simple.c +++ b/drivers/mtd/nand/nand_spl_simple.c @@ -2,6 +2,9 @@ * (C) Copyright 2006-2008 * Stefan Roese, DENX Software Engineering, sr@denx.de. * + * Copyright (C) 2011 + * Corscience GmbH & Co. KG - Simon Schwarz <schwarz@corscience.de> + * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as * published by the Free Software Foundation; either version 2 of @@ -21,10 +24,21 @@ #include <common.h> #include <nand.h> #include <asm/io.h> +#include <asm/arch/dma.h> +#include <asm/arch/cpu.h> static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS; static nand_info_t mtd; static struct nand_chip nand_chip; +static struct nand_chip *this; + +struct ecc_wait_entry { + int valid; + uint8_t *p; + u_char *ecc_code; + u_char *ecc_calc; +}; +static struct ecc_wait_entry ecc_wait; #if (CONFIG_SYS_NAND_PAGE_SIZE <= 512) /* @@ -33,7 +47,6 @@ static struct nand_chip nand_chip; static int nand_command(int block, int page, uint32_t offs, u8 cmd) { - struct nand_chip *this = mtd.priv; int page_addr = page + block * CONFIG_SYS_NAND_PAGE_COUNT; while (!this->dev_ready(&mtd)) @@ -46,11 +59,11 @@ static int nand_command(int block, int page, uint32_t offs, this->cmd_ctrl(&mtd, offs, NAND_CTRL_ALE | NAND_CTRL_CHANGE); this->cmd_ctrl(&mtd, page_addr & 0xff, NAND_CTRL_ALE); /* A[16:9] */ this->cmd_ctrl(&mtd, (page_addr >> 8) & 0xff, - NAND_CTRL_ALE); /* A[24:17] */ + NAND_CTRL_ALE); /* A[24:17] */ #ifdef CONFIG_SYS_NAND_4_ADDR_CYCLE /* One more address cycle for devices > 32MiB */ this->cmd_ctrl(&mtd, (page_addr >> 16) & 0x0f, - NAND_CTRL_ALE); /* A[28:25] */ + NAND_CTRL_ALE); /* A[28:25] */ #endif /* Latch in address */ this->cmd_ctrl(&mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE); @@ -93,20 +106,20 @@ static int nand_command(int block, int page, uint32_t offs, /* Set ALE and clear CLE to start address cycle */ /* Column address */ hwctrl(&mtd, offs & 0xff, - NAND_CTRL_ALE | NAND_CTRL_CHANGE); /* A[7:0] */ + NAND_CTRL_ALE | NAND_CTRL_CHANGE); /* A[7:0] */ hwctrl(&mtd, (offs >> 8) & 0xff, NAND_CTRL_ALE); /* A[11:9] */ /* Row address */ hwctrl(&mtd, (page_addr & 0xff), NAND_CTRL_ALE); /* A[19:12] */ hwctrl(&mtd, ((page_addr >> 8) & 0xff), - NAND_CTRL_ALE); /* A[27:20] */ + NAND_CTRL_ALE); /* A[27:20] */ #ifdef CONFIG_SYS_NAND_5_ADDR_CYCLE /* One more address cycle for devices > 128MiB */ hwctrl(&mtd, (page_addr >> 16) & 0x0f, - NAND_CTRL_ALE); /* A[31:28] */ + NAND_CTRL_ALE); /* A[31:28] */ #endif /* Latch in address */ hwctrl(&mtd, NAND_CMD_READSTART, - NAND_CTRL_CLE | NAND_CTRL_CHANGE); + NAND_CTRL_CLE | NAND_CTRL_CHANGE); hwctrl(&mtd, NAND_CMD_NONE, NAND_NCE | NAND_CTRL_CHANGE); /* @@ -121,7 +134,6 @@ static int nand_command(int block, int page, uint32_t offs, static int nand_is_bad_block(int block) { - struct nand_chip *this = mtd.priv; nand_command(block, 0, CONFIG_SYS_NAND_BAD_BLOCK_POS, NAND_CMD_READOOB); @@ -187,7 +199,7 @@ static int nand_read_page(int block, int page, void *dst) return 0; } -int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst) +int nand_spl_load_image_nondma(uint32_t offs, unsigned int size, void *dst) { unsigned int block, lastblock; unsigned int page; @@ -221,16 +233,171 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst) return 0; } +#ifdef CONFIG_SPL_DMA_SUPPORT +void correct_ecc_dma(void) +{ + int eccsize = CONFIG_SYS_NAND_ECCSIZE; + int eccbytes = CONFIG_SYS_NAND_ECCBYTES; + int eccsteps = CONFIG_SYS_NAND_ECCSTEPS; + int i; + uint8_t *p = ecc_wait.p; + + for (i = 0 ; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { + /* No chance to do something with the possible error message + * from correct_data(). We just hope that all possible errors + * are corrected by this routine. + */ + this->ecc.correct(&mtd, ecc_wait.p, &ecc_wait.ecc_code[i], + &ecc_wait.ecc_calc[i]); + } + ecc_wait.valid = 0; +} + +static int nand_read_page_dma(int block, int page, void *dst) +{ + u_char *ecc_calc; + u_char *ecc_code; + u_char *oob_data; + int i; + int res; + int eccsize = CONFIG_SYS_NAND_ECCSIZE; + int eccbytes = CONFIG_SYS_NAND_ECCBYTES; + int eccsteps = CONFIG_SYS_NAND_ECCSTEPS; + uint8_t *p = dst; + + nand_command(block, page, 0, NAND_CMD_READ0); + + /* No malloc available for now, just use some temporary locations + * in SDRAM + */ + ecc_calc = (u_char *)(CONFIG_SYS_SDRAM_BASE + 0x10000); + ecc_code = ecc_calc + 0x100; + oob_data = ecc_calc + 0x200; + res = 0; + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { + res += omap3_dma_conf_transfer(0, nand_chip.IO_ADDR_R, + (uint32_t *)p, CONFIG_SYS_NAND_ECCSIZE/4); + this->ecc.hwctl(&mtd, NAND_ECC_READ); + res += omap3_dma_start_transfer(0); + /* correct ecc from former transfer */ + if (ecc_wait.valid != 0) + correct_ecc_dma(); + res += omap3_dma_wait_for_transfer(0); + this->ecc.calculate(&mtd, p, &ecc_calc[i]); + if (res) { + printf("spl: DMA error while data read\n"); + return -1; + } + } + + /*this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE);*/ + res = omap3_dma_conf_transfer(0, nand_chip.IO_ADDR_R, + (uint32_t *)oob_data, CONFIG_SYS_NAND_OOBSIZE/4); + res += omap3_dma_start_transfer(0); + res += omap3_dma_wait_for_transfer(0); + if (res) { + printf("spl: DMA error while oob read\n"); + return -1; + } + + /* Pick the ECC bytes out of the oob data */ + for (i = 0; i < CONFIG_SYS_NAND_ECCTOTAL; i++) + ecc_code[i] = oob_data[nand_ecc_pos[i]]; + + /* add to ecc_wait - just ok until the nex ecc.calc!*/ + ecc_wait.valid = 1; + ecc_wait.p = dst; + ecc_wait.ecc_code = ecc_code; + ecc_wait.ecc_calc = ecc_calc; + return 0; +} + +int nand_spl_load_image_dma(uint32_t offs, unsigned int size, void *dst) +{ + unsigned int block, lastblock; + unsigned int page; + + /* + * offs has to be aligned to a page address! + */ + block = offs / CONFIG_SYS_NAND_BLOCK_SIZE; + lastblock = (offs + size - 1) / CONFIG_SYS_NAND_BLOCK_SIZE; + page = (offs % CONFIG_SYS_NAND_BLOCK_SIZE) / CONFIG_SYS_NAND_PAGE_SIZE; + ecc_wait.valid = 0; + while (block <= lastblock) { + if (!nand_is_bad_block(block)) { + /* + * Skip bad blocks + */ + while (page < CONFIG_SYS_NAND_PAGE_COUNT) { + nand_read_page_dma(block, page, dst); + dst += CONFIG_SYS_NAND_PAGE_SIZE; + page++; + } + if (ecc_wait.valid != 0) + correct_ecc_dma(); + + page = 0; + } else { + lastblock++; + } + + block++; + } + return 0; +} + +/* Read from NAND with DMA if appropriate + * offs: Offset in NAND to read from + * size: site to read + * dst: destination pointer (multiple of page sizes -> + * CONFIG_SYS_NAND_PAGE_SIZE*roundup(size/CONFIG_SYS_NAND_PAGE_SIZE)) + * + * RETURN non-null means error + */ +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst) +{ + struct dma4_chan cfg; + debug("using DMA load...\n"); + omap3_dma_init(); + + /* config the channel */ + omap3_dma_get_conf_chan(0, &cfg); + cfg.csdp = CSDP_DATA_TYPE_32BIT | CSDP_SRC_BURST_EN_64BYTES | + CSDP_DST_BURST_EN_64BYTES | CSDP_DST_ENDIAN_LOCK_ADAPT | + CSDP_DST_ENDIAN_LITTLE | CSDP_SRC_ENDIAN_LOCK_ADAPT | + CSDP_SRC_ENDIAN_LITTLE; + cfg.cfn = 1; + cfg.ccr = CCR_READ_PRIORITY_HIGH | CCR_SRC_AMODE_CONSTANT | + CCR_DST_AMODE_POST_INC; + cfg.cicr = (1 << 5) | (1 << 11) | (1 << 10) | (1 << 8); + omap3_dma_conf_chan(0, &cfg); + + + nand_spl_load_image_dma(offs, size, dst); + return 0; +} + +#else /* use cpu copy */ +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dst) + __attribute__ ((alias("nand_spl_load_image_nondma"))); + +#endif /* CONFIG_SPL_DMA_SUPPORT */ + + + /* nand_init() - initialize data to make nand usable by SPL */ void nand_init(void) { /* * Init board specific nand support */ + debug(">>nand_init()\n"); mtd.priv = &nand_chip; nand_chip.IO_ADDR_R = nand_chip.IO_ADDR_W = (void __iomem *)CONFIG_SYS_NAND_BASE; nand_chip.options = 0; + this = mtd.priv; board_nand_init(&nand_chip); if (nand_chip.select_chip)
This adds DMA copy for the nand spl implementation. If CONFIG_SPL_DMA_SUPPORT is defined the DMA is used. Based on DMA driver patch: http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/109744/focus=109747 Signed-off-by: Simon Schwarz <simonschwarzcor@gmail.com> Cc: scottwood@freescale.com Cc: s-paulraj@ti.com --- drivers/mtd/nand/nand_spl_simple.c | 185 ++++++++++++++++++++++++++++++++++-- 1 files changed, 176 insertions(+), 9 deletions(-)