[U-Boot] [PATCH 0/2] Use DMA in SPL
This implements the DMA copy in the SPL.
The DMA usage is activated for devkit8000.
Based on DMA driver patch: http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/109744/focus=109747
Simon Schwarz (2): nand_spl_simple: Add omap3 DMA usage to SPL devkit8000: Activate DMA support in SPL
drivers/mtd/nand/nand_spl_simple.c | 185 ++++++++++++++++++++++++++++++++++-- include/configs/devkit8000.h | 2 + 2 files changed, 178 insertions(+), 9 deletions(-)
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(-)
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)
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] */
NAND_CTRL_ALE); /* A[28:25] */
NAND_CTRL_ALE); /* A[28:25] */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] */
NAND_CTRL_ALE); /* A[28:25] */
NAND_CTRL_ALE); /* A[28:25] */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
This activates the DMA support in the SPL.
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 --- include/configs/devkit8000.h | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/include/configs/devkit8000.h b/include/configs/devkit8000.h index 5d1014b..4799c6d 100644 --- a/include/configs/devkit8000.h +++ b/include/configs/devkit8000.h @@ -366,4 +366,6 @@
#define CONFIG_SYS_NAND_SPL_KERNEL_OFFS 0x280000 #define CONFIG_SYS_SPL_ARGS_ADDR (PHYS_SDRAM_1 + 0x100) +#define CONFIG_OMAP3_DMA +#define CONFIG_SPL_DMA_SUPPORT #endif /* __CONFIG_H */
participants (3)
-
Scott Wood -
Simon Schwarz -
Wolfgang Denk