[U-Boot] [PATCH 00/15] sunxi_nand_spl: cleanups, auto config and bad block handling
Hi All,
As promised I've been working on cleaning up the sunxi nand spl driver, as well as adding bad block handling in the same way as the BROM does, as the SPL + u-boot must be placed in a BROM compatible nand partition.
This series is a bit of a patch bomb, but as you will see they are all nice and small (which is why there are quite a few).
Regards,
Hans
CONFIG_SPL_NAND_SUPPORT gets used via IS_ENABLED so it must be defined to 1, rather then just being defined.
While at remove 2 other unused NAND related defines from sunxi-common.h.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- include/configs/sunxi-common.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 1abf73c..3735afb 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -138,15 +138,10 @@ #define CONFIG_SERIAL_TAG
#if defined(CONFIG_SPL_NAND_SUNXI) -#define CONFIG_SPL_NAND_DRIVERS -#define CONFIG_SPL_NAND_SUPPORT - -#define CONFIG_SYS_NAND_SPL_KERNEL_OFFS 0x280000 +#define CONFIG_SPL_NAND_SUPPORT 1 #define CONFIG_SYS_NAND_U_BOOT_OFFS 0x008000 - #endif
- /* mmc config */ #if !defined(CONFIG_UART0_PORT_F) #define CONFIG_MMC
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
CONFIG_SPL_NAND_SUPPORT gets used via IS_ENABLED so it must be defined to 1, rather then just being defined.
While at remove 2 other unused NAND related defines from sunxi -common.h.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
On Sat, Aug 15, 2015 at 10:02:34PM +0200, Hans de Goede wrote:
CONFIG_SPL_NAND_SUPPORT gets used via IS_ENABLED so it must be defined to 1, rather then just being defined.
While at remove 2 other unused NAND related defines from sunxi-common.h.
Signed-off-by: Hans de Goede hdegoede@redhat.com
include/configs/sunxi-common.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 1abf73c..3735afb 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -138,15 +138,10 @@ #define CONFIG_SERIAL_TAG
#if defined(CONFIG_SPL_NAND_SUNXI) -#define CONFIG_SPL_NAND_DRIVERS -#define CONFIG_SPL_NAND_SUPPORT
-#define CONFIG_SYS_NAND_SPL_KERNEL_OFFS 0x280000 +#define CONFIG_SPL_NAND_SUPPORT 1
Every other platform just use an empty define for this one, and the only user of IS_ENABLED together with this option is the sunxi board. Why not simply replace the IS_ENABLED by an ifdef to maintain consistency?
AFAIK, IS_ENABLED doesn't bring anythnig to the table here.
Maxime
Hi,
On 08/19/2015 03:45 PM, Maxime Ripard wrote:
On Sat, Aug 15, 2015 at 10:02:34PM +0200, Hans de Goede wrote:
CONFIG_SPL_NAND_SUPPORT gets used via IS_ENABLED so it must be defined to 1, rather then just being defined.
While at remove 2 other unused NAND related defines from sunxi-common.h.
Signed-off-by: Hans de Goede hdegoede@redhat.com
include/configs/sunxi-common.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 1abf73c..3735afb 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -138,15 +138,10 @@ #define CONFIG_SERIAL_TAG
#if defined(CONFIG_SPL_NAND_SUNXI) -#define CONFIG_SPL_NAND_DRIVERS -#define CONFIG_SPL_NAND_SUPPORT
-#define CONFIG_SYS_NAND_SPL_KERNEL_OFFS 0x280000 +#define CONFIG_SPL_NAND_SUPPORT 1
Every other platform just use an empty define for this one, and the only user of IS_ENABLED together with this option is the sunxi board. Why not simply replace the IS_ENABLED by an ifdef to maintain consistency?
Because u-boot is moving to Kconfig which will define it to 1, and ...
AFAIK, IS_ENABLED doesn't bring anythnig to the table here.
IS_ENABLED leads too cleaner / easier to read code, so it does bring something to the table.
REgards,
Hans
On Thu, Aug 20, 2015 at 08:33:01AM +0200, Hans de Goede wrote:
Hi,
On 08/19/2015 03:45 PM, Maxime Ripard wrote:
On Sat, Aug 15, 2015 at 10:02:34PM +0200, Hans de Goede wrote:
CONFIG_SPL_NAND_SUPPORT gets used via IS_ENABLED so it must be defined to 1, rather then just being defined.
While at remove 2 other unused NAND related defines from sunxi-common.h.
Signed-off-by: Hans de Goede hdegoede@redhat.com
include/configs/sunxi-common.h | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h index 1abf73c..3735afb 100644 --- a/include/configs/sunxi-common.h +++ b/include/configs/sunxi-common.h @@ -138,15 +138,10 @@ #define CONFIG_SERIAL_TAG
#if defined(CONFIG_SPL_NAND_SUNXI) -#define CONFIG_SPL_NAND_DRIVERS -#define CONFIG_SPL_NAND_SUPPORT
-#define CONFIG_SYS_NAND_SPL_KERNEL_OFFS 0x280000 +#define CONFIG_SPL_NAND_SUPPORT 1
Every other platform just use an empty define for this one, and the only user of IS_ENABLED together with this option is the sunxi board. Why not simply replace the IS_ENABLED by an ifdef to maintain consistency?
Because u-boot is moving to Kconfig which will define it to 1, and ...
AFAIK, IS_ENABLED doesn't bring anythnig to the table here.
IS_ENABLED leads too cleaner / easier to read code, so it does bring something to the table.
Ok, if the plan is to move to Kconfig eventually, I guess it makes sense.
Maxime
nand_spl_load_image() always gets called with either CONFIG_SYS_TEXT_BASE or spl_image.load_addr as destination, both of which are properly aligened, and have plenty of space for "overshooting" up to CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE bytes, as we read in CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE bytes chunks.
This saves CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE (typically 1k) in SPL size, which is a lot on the total 24k we have.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index ac5f56d..46654e4 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -85,6 +85,7 @@
#define SUNXI_DMA_DDMA_CFG_REG_LOADING (1 << 31) #define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25) +#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16) #define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9) #define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5) #define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0) @@ -94,10 +95,6 @@
/* minimal "boot0" style NAND support for Allwinner A20 */
-/* temporary buffer in internal ram */ -unsigned char temp_buf[CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE] - __aligned(0x10) __section(".text#"); - /* random seed used by linux */ const uint16_t random_seed[128] = { 0x2b75, 0x0bd0, 0x5ca3, 0x62d1, 0x1c93, 0x07e9, 0x2162, 0x3a72, @@ -167,8 +164,8 @@ void nand_init(void) } }
-static void nand_read_page(unsigned int real_addr, int syndrome, - uint32_t *ecc_errors) +static void nand_read_page(unsigned int real_addr, dma_addr_t dst, + int syndrome, uint32_t *ecc_errors) { uint32_t val; int ecc_off = 0; @@ -226,9 +223,6 @@ static void nand_read_page(unsigned int real_addr, int syndrome, return; }
- /* clear temp_buf */ - memset(temp_buf, 0, CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE); - /* set CMD */ writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET, SUNXI_NFC_BASE + NFC_CMD); @@ -278,8 +272,7 @@ static void nand_read_page(unsigned int real_addr, int syndrome, writel(SUNXI_NFC_BASE + NFC_IO_DATA, SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0); /* read to RAM */ - writel((uint32_t)temp_buf, - SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0); + writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0); writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC | SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0); @@ -287,6 +280,7 @@ static void nand_read_page(unsigned int real_addr, int syndrome, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0); /* 1kB */ writel(SUNXI_DMA_DDMA_CFG_REG_LOADING | SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 + | SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM | SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 | SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO | SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC, @@ -324,27 +318,14 @@ static void nand_read_page(unsigned int real_addr, int syndrome, int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) { void *current_dest; - uint32_t count; - uint32_t current_count; uint32_t ecc_errors = 0;
- memset(dest, 0x0, size); /* clean destination memory */ for (current_dest = dest; current_dest < (dest + size); current_dest += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) { - nand_read_page(offs, offs - < CONFIG_NAND_SUNXI_SPL_SYNDROME_PARTITIONS_END, - &ecc_errors); - count = current_dest - dest; - - if (size - count > CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) - current_count = CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE; - else - current_count = size - count; - - memcpy(current_dest, - temp_buf, - current_count); + nand_read_page(offs, (dma_addr_t)current_dest, + offs < CONFIG_NAND_SUNXI_SPL_SYNDROME_PARTITIONS_END, + &ecc_errors); offs += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE; } return ecc_errors ? -1 : 0;
On Sun, Aug 16, 2015 at 4:02 AM, Hans de Goede hdegoede@redhat.com wrote:
nand_spl_load_image() always gets called with either CONFIG_SYS_TEXT_BASE or spl_image.load_addr as destination, both of which are properly aligened, and have plenty of space for "overshooting" up to CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE bytes, as we read in CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE bytes chunks.
This saves CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE (typically 1k) in SPL size, which is a lot on the total 24k we have.
Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/mtd/nand/sunxi_nand_spl.c | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index ac5f56d..46654e4 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -85,6 +85,7 @@
#define SUNXI_DMA_DDMA_CFG_REG_LOADING (1 << 31) #define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25) +#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
Might want to mention this change (correct DMA type?) in the commit log.
#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9) #define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5) #define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0) @@ -94,10 +95,6 @@
/* minimal "boot0" style NAND support for Allwinner A20 */
-/* temporary buffer in internal ram */ -unsigned char temp_buf[CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE]
__aligned(0x10) __section(".text#");/* random seed used by linux */ const uint16_t random_seed[128] = { 0x2b75, 0x0bd0, 0x5ca3, 0x62d1, 0x1c93, 0x07e9, 0x2162, 0x3a72, @@ -167,8 +164,8 @@ void nand_init(void) } }
-static void nand_read_page(unsigned int real_addr, int syndrome,
uint32_t *ecc_errors)+static void nand_read_page(unsigned int real_addr, dma_addr_t dst,
int syndrome, uint32_t *ecc_errors){ uint32_t val; int ecc_off = 0; @@ -226,9 +223,6 @@ static void nand_read_page(unsigned int real_addr, int syndrome, return; }
/* clear temp_buf */memset(temp_buf, 0, CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE);/* set CMD */ writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET, SUNXI_NFC_BASE + NFC_CMD);@@ -278,8 +272,7 @@ static void nand_read_page(unsigned int real_addr, int syndrome, writel(SUNXI_NFC_BASE + NFC_IO_DATA, SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0); /* read to RAM */
writel((uint32_t)temp_buf,SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0);
writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0); writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC | SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0);@@ -287,6 +280,7 @@ static void nand_read_page(unsigned int real_addr, int syndrome, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0); /* 1kB */ writel(SUNXI_DMA_DDMA_CFG_REG_LOADING | SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32
| SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM | SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 | SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO | SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,@@ -324,27 +318,14 @@ static void nand_read_page(unsigned int real_addr, int syndrome, int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) { void *current_dest;
uint32_t count;uint32_t current_count; uint32_t ecc_errors = 0;memset(dest, 0x0, size); /* clean destination memory */ for (current_dest = dest; current_dest < (dest + size); current_dest += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) {nand_read_page(offs, offs< CONFIG_NAND_SUNXI_SPL_SYNDROME_PARTITIONS_END,&ecc_errors);count = current_dest - dest;if (size - count > CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE)current_count = CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE;elsecurrent_count = size - count;memcpy(current_dest,temp_buf,current_count);
nand_read_page(offs, (dma_addr_t)current_dest,offs < CONFIG_NAND_SUNXI_SPL_SYNDROME_PARTITIONS_END,&ecc_errors); offs += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE; } return ecc_errors ? -1 : 0;-- 2.4.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Mon, 2015-08-17 at 15:22 +0800, Chen-Yu Tsai wrote:
+#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
Might want to mention this change (correct DMA type?) in the commit log.
Yes, would be good.
With that: Acked-by: Ian Campbell ijc@hellion.org.uk
There is no need to reset the nand chip for every ecc-block read.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 46654e4..56c0be0 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -162,6 +162,16 @@ void nand_init(void) NFC_CTL_RESET, MAX_RETRIES)) { printf("Couldn't initialize nand\n"); } + + /* reset NAND */ + writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET, + SUNXI_NFC_BASE + NFC_CMD); + + if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_CMD_INT_FLAG, + MAX_RETRIES)) { + printf("Error timeout waiting for nand reset\n"); + return; + } }
static void nand_read_page(unsigned int real_addr, dma_addr_t dst, @@ -223,16 +233,6 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, return; }
- /* set CMD */ - writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET, - SUNXI_NFC_BASE + NFC_CMD); - - if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_CMD_INT_FLAG, - MAX_RETRIES)) { - printf("Error while initilizing command interrupt\n"); - return; - } - page = real_addr / CONFIG_NAND_SUNXI_SPL_PAGE_SIZE; column = real_addr % CONFIG_NAND_SUNXI_SPL_PAGE_SIZE;
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
There is no need to reset the nand chip for every ecc-block read.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
In syndrome mode we set the NFC_SEQ bit in the command register, so the spare-area register is not used. Also the value currently being written is actual wrong, the ecc sits at "column + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE.
So the current code only serves to confuse the user -> remove it.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 56c0be0..f6f4928 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -256,10 +256,7 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, val = readl(SUNXI_NFC_BASE + NFC_CTL); writel(val | NFC_CTL_RAM_METHOD, SUNXI_NFC_BASE + NFC_CTL);
- if (syndrome) { - writel(CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, - SUNXI_NFC_BASE + NFC_SPARE_AREA); - } else { + if (!syndrome) { oob_offset = CONFIG_NAND_SUNXI_SPL_PAGE_SIZE + (column / CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) * ecc_off;
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
In syndrome mode we set the NFC_SEQ bit in the command register, so the spare-area register is not used. Also the value currently being written is actual wrong, the ecc sits at "column + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE.
So the current code only serves to confuse the user -> remove it.
Signed-off-by: Hans de Goede hdegoede@redhat.com
There's a bunch of other uses of the syndrome parameter in this function. Does syndrome=true work even without this particular bit of code?
I suppose I'm asking, should the paramter and the other uses be removed? Or should an ASSERT(!syndrome) be added, or am I worrying about nothing and everything is just fine as it is after this patch?
I suspect the latter, so if that is indeed the case: Acked-by: Ian Campbell < ijc@hellion.org.uk >
Hi,
On 17-08-15 10:19, Ian Campbell wrote:
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
In syndrome mode we set the NFC_SEQ bit in the command register, so the spare-area register is not used. Also the value currently being written is actual wrong, the ecc sits at "column + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE.
So the current code only serves to confuse the user -> remove it.
Signed-off-by: Hans de Goede hdegoede@redhat.com
There's a bunch of other uses of the syndrome parameter in this function. Does syndrome=true work even without this particular bit of code?
Yes, since in syndrome we are passing in the NFC_SEQ bit in the command register, which tells the controller that the ecc data is directly behind the actual data, the spare-area address register is not used.
I stumbled over this because the code we had was writing the wrong address for the ecc data to the spare-area address register, leading me to wonder why syndrome mode was working at all.
I suppose I'm asking, should the paramter and the other uses be removed? Or should an ASSERT(!syndrome) be added, or am I worrying about nothing and everything is just fine as it is after this patch?
Everything is just fine after this patch, actually it was fine before this patch except that it did an unnecessary write, with the wrong ecc data address which I found confusing, hence this patch to remove that write.
I suspect the latter, so if that is indeed the case: Acked-by: Ian Campbell < ijc@hellion.org.uk >
Thanks for all the reviews.
Regards,
Hans
On Mon, 2015-08-17 at 10:59 +0200, Hans de Goede wrote:
Hi,
On 17-08-15 10:19, Ian Campbell wrote:
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
In syndrome mode we set the NFC_SEQ bit in the command register, so the spare-area register is not used. Also the value currently being written is actual wrong, the ecc sits at "column + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE" not just CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE.
So the current code only serves to confuse the user -> remove it.
Signed-off-by: Hans de Goede hdegoede@redhat.com
There's a bunch of other uses of the syndrome parameter in this function. Does syndrome=true work even without this particular bit of code?
Yes, since in syndrome we are passing in the NFC_SEQ bit in the command register, which tells the controller that the ecc data is directly behind the actual data, the spare-area address register is not used.
I stumbled over this because the code we had was writing the wrong address for the ecc data to the spare-area address register, leading me to wonder why syndrome mode was working at all.
Right, that was the line of thinking which took me here too.
I suppose I'm asking, should the paramter and the other uses be removed? Or should an ASSERT(!syndrome) be added, or am I worrying about nothing and everything is just fine as it is after this patch?
Everything is just fine after this patch, actually it was fine before this patch except that it did an unnecessary write, with the wrong ecc data address which I found confusing, hence this patch to remove that write.
Super, thanks for the explanation. The Ack stands then.
I suspect the latter, so if that is indeed the case: Acked-by: Ian Campbell < ijc@hellion.org.uk >
Thanks for all the reviews.
Regards,
Hans
Use SYS_NAND_SELF_INIT and only setup the pinmux and clocks when we are actually using the nand.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- board/sunxi/board.c | 12 +++++++----- drivers/mtd/nand/Kconfig | 1 + drivers/mtd/nand/sunxi_nand_spl.c | 2 ++ 3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 680523a..b76bb83 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -31,6 +31,7 @@ #include <asm/arch/usb_phy.h> #include <asm/gpio.h> #include <asm/io.h> +#include <nand.h> #include <net.h>
#if defined CONFIG_VIDEO_LCD_PANEL_I2C && !(defined CONFIG_SPL_BUILD) @@ -127,6 +128,12 @@ static void nand_clock_setup(void) setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0)); setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1); } + +void board_nand_init(void) +{ + nand_pinmux_setup(); + nand_clock_setup(); +} #endif
#ifdef CONFIG_GENERIC_MMC @@ -453,11 +460,6 @@ void sunxi_board_init(void) power_failed |= axp221_set_eldo(3, CONFIG_AXP221_ELDO3_VOLT); #endif
-#ifdef CONFIG_SPL_NAND_SUNXI - nand_pinmux_setup(); - nand_clock_setup(); -#endif - printf("DRAM:"); ramsize = sunxi_dram_init(); printf(" %lu MiB\n", ramsize >> 20); diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index 41ebfea..18039f7 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -88,6 +88,7 @@ config SPL_NAND_DENALI config SPL_NAND_SUNXI bool "Support for NAND on Allwinner A20 in SPL" depends on MACH_SUN7I + select SYS_NAND_SELF_INIT ---help--- Enable support for NAND. This option allows SPL to read from sunxi NAND using DMA transfers. diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index f6f4928..9efe904 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -153,6 +153,8 @@ void nand_init(void) { uint32_t val;
+ board_nand_init(); + val = readl(SUNXI_NFC_BASE + NFC_CTL); /* enable and reset CTL */ writel(val | NFC_CTL_EN | NFC_CTL_RESET,
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Use SYS_NAND_SELF_INIT and only setup the pinmux and clocks when we are actually using the nand.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
We use DMA for nand data transfers in the SPL, so make sure the DMA controller is enabled.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- board/sunxi/board.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index b76bb83..1ebd0a4 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -125,7 +125,13 @@ static void nand_clock_setup(void) { struct sunxi_ccm_reg *const ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE; + setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0)); +#ifdef CONFIG_MACH_SUN9I + setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA)); +#else + setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA)); +#endif setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1); }
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
We use DMA for nand data transfers in the SPL, so make sure the DMA controller is enabled.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
Turn off the nand and dma clocks when we're done with the nand, this puts the nand and dma controllers back into a clean state for when the kernel boots.
Without this the kernel will not boot properly when it is build with dma-controller support.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 9efe904..147d476 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -5,9 +5,10 @@ * SPDX-License-Identifier: GPL-2.0+ */
+#include <asm/arch/clock.h> +#include <asm/io.h> #include <common.h> #include <config.h> -#include <asm/io.h> #include <nand.h>
/* registers */ @@ -330,4 +331,16 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) return ecc_errors ? -1 : 0; }
-void nand_deselect(void) {} +void nand_deselect(void) +{ + struct sunxi_ccm_reg *const ccm = + (struct sunxi_ccm_reg *)SUNXI_CCM_BASE; + + clrbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0)); +#ifdef CONFIG_MACH_SUN9I + clrbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA)); +#else + clrbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA)); +#endif + clrbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1); +}
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Turn off the nand and dma clocks when we're done with the nand, this puts the nand and dma controllers back into a clean state for when the kernel boots.
Without this the kernel will not boot properly when it is build with
built
dma-controller support.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
On Sat, Aug 15, 2015 at 10:02:40PM +0200, Hans de Goede wrote:
Turn off the nand and dma clocks when we're done with the nand, this puts the nand and dma controllers back into a clean state for when the kernel boots.
Without this the kernel will not boot properly when it is build with dma-controller support.
The kernel shouldn't behave like that. What issue are you seeing?
Maxime
Hi,
On 08/19/2015 03:48 PM, Maxime Ripard wrote:
On Sat, Aug 15, 2015 at 10:02:40PM +0200, Hans de Goede wrote:
Turn off the nand and dma clocks when we're done with the nand, this puts the nand and dma controllers back into a clean state for when the kernel boots.
Without this the kernel will not boot properly when it is build with dma-controller support.
The kernel shouldn't behave like that. What issue are you seeing?
The kernel does not output anything, not even on the serial console, so it seems to crash quite early on. I guess that the dma controller has an irq asserted and that that fires as soon as the dma driver unmasks it.
I did not investigate this further since u-boot really should put the hardware in a clean state before booting the os, so we want this change even if the kernel were fixed to deal with the unclean state.
Regards,
Hans
On Thu, Aug 20, 2015 at 08:35:20AM +0200, Hans de Goede wrote:
On 08/19/2015 03:48 PM, Maxime Ripard wrote:
On Sat, Aug 15, 2015 at 10:02:40PM +0200, Hans de Goede wrote:
Turn off the nand and dma clocks when we're done with the nand, this puts the nand and dma controllers back into a clean state for when the kernel boots.
Without this the kernel will not boot properly when it is build with dma-controller support.
The kernel shouldn't behave like that. What issue are you seeing?
The kernel does not output anything, not even on the serial console, so it seems to crash quite early on. I guess that the dma controller has an irq asserted and that that fires as soon as the dma driver unmasks it.
Hmmm, fishy.
I did not investigate this further since u-boot really should put the hardware in a clean state before booting the os, so we want this change even if the kernel were fixed to deal with the unclean state.
I guess we should aim at "fixing" both. The kernel should not really care about the state the hardware has been left in, and U-boot should not really leave the dma controller running either.
Maxime
We are using dma, so we should flush the cache before starting the dma, and invalidate it once the dma is done.
Things are working without this by mostly luck, but lets not rely on that.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 147d476..663c03e 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -266,6 +266,10 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, writel(oob_offset, SUNXI_NFC_BASE + NFC_SPARE_AREA); }
+ flush_dcache_range(dst, + ALIGN(dst + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, + ARCH_DMA_MINALIGN)); + /* SUNXI_DMA */ writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */ /* read from REG_IO_DATA */ @@ -311,6 +315,10 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, return; }
+ invalidate_dcache_range(dst, + ALIGN(dst + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, + ARCH_DMA_MINALIGN)); + if (readl(SUNXI_NFC_BASE + NFC_ECC_ST)) (*ecc_errors)++; }
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
We are using dma, so we should flush the cache before starting the dma, and invalidate it once the dma is done.
Things are working without this by mostly luck, but lets not rely on that.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
Sync the code for figuring out the ecc_mode and ecc_offset with the kernel code. Keeping this in sync seems like a good idea in general, and it fixes / adds support for ecc strengths of 56, 60 and 64 bits.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 58 ++++++++------------------------------- 1 file changed, 12 insertions(+), 46 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 663c03e..61eb393 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -181,60 +181,26 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, int syndrome, uint32_t *ecc_errors) { uint32_t val; - int ecc_off = 0; + int i, ecc_off = 0; uint16_t ecc_mode = 0; uint16_t rand_seed; uint32_t page; uint16_t column; uint32_t oob_offset; + static const u8 strengths[] = { 16, 24, 28, 32, 40, 48, 56, 60, 64 };
- switch (CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH) { - case 16: - ecc_mode = 0; - ecc_off = 0x20; - break; - case 24: - ecc_mode = 1; - ecc_off = 0x2e; - break; - case 28: - ecc_mode = 2; - ecc_off = 0x32; - break; - case 32: - ecc_mode = 3; - ecc_off = 0x3c; - break; - case 40: - ecc_mode = 4; - ecc_off = 0x4a; - break; - case 48: - ecc_mode = 4; - ecc_off = 0x52; - break; - case 56: - ecc_mode = 4; - ecc_off = 0x60; - break; - case 60: - ecc_mode = 4; - ecc_off = 0x0; - break; - case 64: - ecc_mode = 4; - ecc_off = 0x0; - break; - default: - ecc_mode = 0; - ecc_off = 0; + for (i = 0; i < ARRAY_SIZE(strengths); i++) { + if (CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH == strengths[i]) { + ecc_mode = i; + break; + } }
- if (ecc_off == 0) { - printf("Unsupported ECC strength (%d)!\n", - CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH); - return; - } + /* HW ECC always request ECC bytes for 1024 bytes blocks */ + ecc_off = DIV_ROUND_UP(CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH * fls(8 * 1024), 8); + /* HW ECC always work with even numbers of ECC bytes */ + ecc_off += (ecc_off & 1); + ecc_off += 4; /* prepad */
page = real_addr / CONFIG_NAND_SUNXI_SPL_PAGE_SIZE; column = real_addr % CONFIG_NAND_SUNXI_SPL_PAGE_SIZE;
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Sync the code for figuring out the ecc_mode and ecc_offset with the kernel code. Keeping this in sync seems like a good idea in general,
I agree. When doing this IME it is useful to include the last version sync'd in, either as a code comment or in the commit message, so next time the person doing the sync has a clue where in the upstream history to start looking.
and it fixes / adds support for ecc strengths of 56, 60 and 64 bits.
Signed-off-by: Hans de Goede hdegoede@redhat.com
With a reference to the specific Linux version: Acked-by: Ian Campbell < ijc@hellion.org.uk >
We currently only use the spl nand code to get u-boot itself, which _always_ is located on a syndrome partition. Once we figure out how we are going to store the u-boot env on nand, we may need non syndrome support, but even then there will likely be better ways to determine whether to use syndrome or not (e.g. look at the loadaddr passed in).
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/Kconfig | 8 -------- drivers/mtd/nand/sunxi_nand_spl.c | 3 +-- 2 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index 18039f7..8a3fae5 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -95,14 +95,6 @@ config SPL_NAND_SUNXI Depending on the NAND chip, values like ECC strength and page sizes have to be configured.
-config NAND_SUNXI_SPL_SYNDROME_PARTITIONS_END - hex "Size of syndrome partitions in sunxi NAND" - default 0x400000 - depends on SPL_NAND_SUNXI - ---help--- - End address for boot partitions on NAND. Those partitions have a - different random seed that has to match the sunxi BROM setting. - config NAND_SUNXI_SPL_ECC_STRENGTH int "ECC Strength for sunxi NAND" default 40 diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 61eb393..9c304f4 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -297,8 +297,7 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) for (current_dest = dest; current_dest < (dest + size); current_dest += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) { - nand_read_page(offs, (dma_addr_t)current_dest, - offs < CONFIG_NAND_SUNXI_SPL_SYNDROME_PARTITIONS_END, + nand_read_page(offs, (dma_addr_t)current_dest, 1, &ecc_errors); offs += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE; }
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
We currently only use the spl nand code to get u-boot itself, which _always_ is located on a syndrome partition. Once we figure out how we are going to store the u-boot env on nand, we may need non syndrome support, but even then there will likely be better ways to determine whether to use syndrome or not (e.g. look at the loadaddr passed in).
Signed-off-by: Hans de Goede hdegoede@redhat.com
I suppose I should back reference my question as to whether we should remove this option (even though it seems to have ended up the other way round to what I was anticipating).
Anyway, I'm fine with this anyway: Acked-by: Ian Campbell < ijc@hellion.org.uk >
Hi,
On 17-08-15 10:27, Ian Campbell wrote:
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
We currently only use the spl nand code to get u-boot itself, which _always_ is located on a syndrome partition. Once we figure out how we are going to store the u-boot env on nand, we may need non syndrome support, but even then there will likely be better ways to determine whether to use syndrome or not (e.g. look at the loadaddr passed in).
Signed-off-by: Hans de Goede hdegoede@redhat.com
I suppose I should back reference my question as to whether we should remove this option (even though it seems to have ended up the other way round to what I was anticipating).
Actually I've been discussing nand partitioning with Boris Brezillon and it looks like that we need may need to use a non syndrome partition for u-boot on some nands, because the ecc options supported by the BROM are too weak for this nand, so we will only use a BROM compatible ecc setting for the spl and use a separate nand partition for u-boot. We're still figuring this out, but I think I'll drop this patch for now, as we may need this after all.
Regards,
Hans
Other then having a few less chip-select lines the nand controller on sun4i, sun5i and sun7i is identical.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- board/sunxi/board.c | 12 +++++++++--- drivers/mtd/nand/Kconfig | 4 ++-- 2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1ebd0a4..d411e96 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -112,13 +112,19 @@ int dram_init(void) static void nand_pinmux_setup(void) { unsigned int pin; - for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(6); pin++) - sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_NAND);
- for (pin = SUNXI_GPC(8); pin <= SUNXI_GPC(22); pin++) + for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(19); pin++) sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_NAND);
+#if defined CONFIG_MACH_SUN4I || defined CONFIG_MACH_SUN7I + for (pin = SUNXI_GPC(20); pin <= SUNXI_GPC(22); pin++) + sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_NAND); +#endif + /* sun4i / sun7i do have a PC23, but it is not used for nand, + * only sun7i has a PC24 */ +#ifdef CONFIG_MACH_SUN7I sunxi_gpio_set_cfgpin(SUNXI_GPC(24), SUNXI_GPC_NAND); +#endif }
static void nand_clock_setup(void) diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index 8a3fae5..e3cbc31 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -86,8 +86,8 @@ config SPL_NAND_DENALI for use on SPL.
config SPL_NAND_SUNXI - bool "Support for NAND on Allwinner A20 in SPL" - depends on MACH_SUN7I + bool "Support for NAND on Allwinner SoCs in SPL" + depends on MACH_SUN4I || MACH_SUN5I || MACH_SUN7I select SYS_NAND_SELF_INIT ---help--- Enable support for NAND. This option allows SPL to read from
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Other then having a few less chip-select lines the nand controller on sun4i, sun5i and sun7i is identical.
Signed-off-by: Hans de Goede hdegoede@redhat.com
board/sunxi/board.c | 12 +++++++++--- drivers/mtd/nand/Kconfig | 4 ++-- 2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1ebd0a4..d411e96 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -112,13 +112,19 @@ int dram_init(void) static void nand_pinmux_setup(void) { unsigned int pin;
for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(6); pin++)
sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_NAND);for (pin = SUNXI_GPC(8); pin <= SUNXI_GPC(22); pin++)
- for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(19); pin++)
This looks to have added GPC(7) to all platforms, was that intentional?
Ian.
Hi,
On 17-08-15 10:29, Ian Campbell wrote:
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Other then having a few less chip-select lines the nand controller on sun4i, sun5i and sun7i is identical.
Signed-off-by: Hans de Goede hdegoede@redhat.com
board/sunxi/board.c | 12 +++++++++--- drivers/mtd/nand/Kconfig | 4 ++-- 2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1ebd0a4..d411e96 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -112,13 +112,19 @@ int dram_init(void) static void nand_pinmux_setup(void) { unsigned int pin;
for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(6); pin++)
sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_NAND);for (pin = SUNXI_GPC(8); pin <= SUNXI_GPC(22); pin++)
- for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(19); pin++)
This looks to have added GPC(7) to all platforms, was that intentional?
Yes, as GPC7 is a nand pin on all platforms, no idea why the original code was skipping it. I should probably mention this in the commit msg though.
Regards,
Hans
On Mon, 2015-08-17 at 10:31 +0200, Hans de Goede wrote:
Hi,
On 17-08-15 10:29, Ian Campbell wrote:
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Other then having a few less chip-select lines the nand controller on sun4i, sun5i and sun7i is identical.
Signed-off-by: Hans de Goede hdegoede@redhat.com
board/sunxi/board.c | 12 +++++++++--- drivers/mtd/nand/Kconfig | 4 ++-- 2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1ebd0a4..d411e96 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -112,13 +112,19 @@ int dram_init(void) static void nand_pinmux_setup(void) { unsigned int pin;
for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(6); pin++)
sunxi_gpio_set_cfgpin(pin, SUNXI_GPC_NAND);for (pin = SUNXI_GPC(8); pin <= SUNXI_GPC(22); pin++)
- for (pin = SUNXI_GPC(0); pin <= SUNXI_GPC(19); pin++)
This looks to have added GPC(7) to all platforms, was that intentional?
Yes, as GPC7 is a nand pin on all platforms, no idea why the original code was skipping it. I should probably mention this in the commit msg though.
Please, then: Acked-by: Ian Campbell ijc@hellion.org.uk
Properly config page-size in the nand ctl register, it seems that things work fine without doing this, but still lets play it safe and properly set the page-size.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 9c304f4..34bb1a3 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -42,6 +42,8 @@ #define NFC_CTL_EN (1 << 0) #define NFC_CTL_RESET (1 << 1) #define NFC_CTL_RAM_METHOD (1 << 14) +#define NFC_CTL_PAGE_SIZE_MASK (0xf << 8) +#define NFC_CTL_PAGE_SIZE(a) ((fls(a) - 11) << 8)
#define NFC_ECC_EN (1 << 0) @@ -294,6 +296,9 @@ int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) void *current_dest; uint32_t ecc_errors = 0;
+ clrsetbits_le32(SUNXI_NFC_BASE + NFC_CTL, NFC_CTL_PAGE_SIZE_MASK, + NFC_CTL_PAGE_SIZE(CONFIG_NAND_SUNXI_SPL_PAGE_SIZE)); + for (current_dest = dest; current_dest < (dest + size); current_dest += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) {
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Properly config page-size in the nand ctl register, it seems that things work fine without doing this, but still lets play it safe and properly set the page-size.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
Parametrize the lowlevel nand_read_page function, instead of directly using the CONFIG_foo settings for page-size, etc. there and add a few wrappers / helper functions for calling it.
This is a preparation patch for adding auto-detecting of the nand parameters like the BROM does.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 82 +++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 37 deletions(-)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 34bb1a3..4cfcdb3 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -179,8 +179,8 @@ void nand_init(void) } }
-static void nand_read_page(unsigned int real_addr, dma_addr_t dst, - int syndrome, uint32_t *ecc_errors) +static int nand_read_page(int page_size, int ecc_strength, int ecc_page_size, + int addr_cycles, uint32_t real_addr, dma_addr_t dst, int syndrome) { uint32_t val; int i, ecc_off = 0; @@ -188,28 +188,26 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, uint16_t rand_seed; uint32_t page; uint16_t column; - uint32_t oob_offset; static const u8 strengths[] = { 16, 24, 28, 32, 40, 48, 56, 60, 64 };
for (i = 0; i < ARRAY_SIZE(strengths); i++) { - if (CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH == strengths[i]) { + if (ecc_strength == strengths[i]) { ecc_mode = i; break; } }
/* HW ECC always request ECC bytes for 1024 bytes blocks */ - ecc_off = DIV_ROUND_UP(CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH * fls(8 * 1024), 8); + ecc_off = DIV_ROUND_UP(ecc_strength * fls(8 * 1024), 8); /* HW ECC always work with even numbers of ECC bytes */ ecc_off += (ecc_off & 1); ecc_off += 4; /* prepad */
- page = real_addr / CONFIG_NAND_SUNXI_SPL_PAGE_SIZE; - column = real_addr % CONFIG_NAND_SUNXI_SPL_PAGE_SIZE; + page = real_addr / page_size; + column = real_addr % page_size;
if (syndrome) - column += (column / CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) - * ecc_off; + column += (column / ecc_page_size) * ecc_off;
/* clear ecc status */ writel(0, SUNXI_NFC_BASE + NFC_ECC_ST); @@ -227,16 +225,11 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, val = readl(SUNXI_NFC_BASE + NFC_CTL); writel(val | NFC_CTL_RAM_METHOD, SUNXI_NFC_BASE + NFC_CTL);
- if (!syndrome) { - oob_offset = CONFIG_NAND_SUNXI_SPL_PAGE_SIZE - + (column / CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) - * ecc_off; - writel(oob_offset, SUNXI_NFC_BASE + NFC_SPARE_AREA); - } + if (!syndrome) + writel(page_size + (column / ecc_page_size) * ecc_off, + SUNXI_NFC_BASE + NFC_SPARE_AREA);
- flush_dcache_range(dst, - ALIGN(dst + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, - ARCH_DMA_MINALIGN)); + flush_dcache_range(dst, ALIGN(dst + ecc_page_size, ARCH_DMA_MINALIGN));
/* SUNXI_DMA */ writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */ @@ -248,7 +241,7 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC | SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0); - writel(CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, + writel(ecc_page_size, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0); /* 1kB */ writel(SUNXI_DMA_DDMA_CFG_REG_LOADING | SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 @@ -267,46 +260,61 @@ static void nand_read_page(unsigned int real_addr, dma_addr_t dst, SUNXI_NFC_BASE + NFC_ADDR_LOW); writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH); writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_DATA_TRANS | - NFC_PAGE_CMD | NFC_WAIT_FLAG | (4 << NFC_ADDR_NUM_OFFSET) | + NFC_PAGE_CMD | NFC_WAIT_FLAG | + ((addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR | NFC_DATA_SWAP_METHOD | (syndrome ? NFC_SEQ : 0), SUNXI_NFC_BASE + NFC_CMD);
if (!check_value(SUNXI_NFC_BASE + NFC_ST, (1 << 2), MAX_RETRIES)) { printf("Error while initializing dma interrupt\n"); - return; + return -1; }
if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0, SUNXI_DMA_DDMA_CFG_REG_LOADING, MAX_RETRIES)) { printf("Error while waiting for dma transfer to finish\n"); - return; + return -1; }
invalidate_dcache_range(dst, - ALIGN(dst + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, - ARCH_DMA_MINALIGN)); + ALIGN(dst + ecc_page_size, ARCH_DMA_MINALIGN));
if (readl(SUNXI_NFC_BASE + NFC_ECC_ST)) - (*ecc_errors)++; + return -1; + + return 0; }
-int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) +static int nand_read_ecc(int page_size, int ecc_strength, int ecc_page_size, + int addr_cycles, uint32_t offs, uint32_t size, void *dest, int syndrome) { - void *current_dest; - uint32_t ecc_errors = 0; + void *end = dest + size;
clrsetbits_le32(SUNXI_NFC_BASE + NFC_CTL, NFC_CTL_PAGE_SIZE_MASK, - NFC_CTL_PAGE_SIZE(CONFIG_NAND_SUNXI_SPL_PAGE_SIZE)); - - for (current_dest = dest; - current_dest < (dest + size); - current_dest += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE) { - nand_read_page(offs, (dma_addr_t)current_dest, 1, - &ecc_errors); - offs += CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE; + NFC_CTL_PAGE_SIZE(page_size)); + + for ( ;dest < end; dest += ecc_page_size, offs += ecc_page_size) { + if (nand_read_page(page_size, ecc_strength, ecc_page_size, + addr_cycles, offs, (dma_addr_t)dest, 1)) + return -1; } - return ecc_errors ? -1 : 0; + + return 0; +} + +static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest, + int syndrome) +{ + return nand_read_ecc(CONFIG_NAND_SUNXI_SPL_PAGE_SIZE, + CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH, + CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, + 5, offs, size, dest, syndrome); +} + +int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) +{ + return nand_read_buffer(offs, size, dest, 1); }
void nand_deselect(void)
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Parametrize the lowlevel nand_read_page function, instead of directly using the CONFIG_foo settings for page-size, etc. there and add a few wrappers / helper functions for calling it.
This is a preparation patch for adding auto-detecting of the nand parameters like the BROM does.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
Auto detect the nand configuration parameters, like the BROM does.
This allows us to get rid of various Kconfig settings, and is necessary to support generic boards like the mk802 which have seen many production runs with different nands.
The full blown u-boot/kernel nand driver uses the nand id to determine this info, for the SPL we do as the BROM does and simply try a few standard configs.
Note the table only contains configs which are known to actually be used, rather then all the configs the BROM tries. This means that it may need to be updated in the future as we add support for nand on more boards.
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/Kconfig | 23 --------------------- drivers/mtd/nand/sunxi_nand_spl.c | 42 +++++++++++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index e3cbc31..a07cbeb 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -95,29 +95,6 @@ config SPL_NAND_SUNXI Depending on the NAND chip, values like ECC strength and page sizes have to be configured.
-config NAND_SUNXI_SPL_ECC_STRENGTH - int "ECC Strength for sunxi NAND" - default 40 - depends on SPL_NAND_SUNXI - ---help--- - ECC strength used by the sunxi NAND SPL driver. This is specific to the - chosen NAND chip and has to match the value used by the sunxi BROM. - -config NAND_SUNXI_SPL_ECC_PAGE_SIZE - hex "ECC page size for sunxi NAND" - default 0x400 - depends on SPL_NAND_SUNXI - ---help--- - ECC page size used by the sunxi NAND SPL driver for syndrome partitions. - This setting has to match the value used by the sunxi BROM. - -config NAND_SUNXI_SPL_PAGE_SIZE - hex "Page size for sunxi NAND" - default 0x2000 - depends on SPL_NAND_SUNXI - ---help--- - Page size of the NAND flash used by the sunxi NAND SPL driver. This is - specific to the chosen NAND chip. endif
endmenu diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 4cfcdb3..5687cd8 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -306,10 +306,44 @@ static int nand_read_ecc(int page_size, int ecc_strength, int ecc_page_size, static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest, int syndrome) { - return nand_read_ecc(CONFIG_NAND_SUNXI_SPL_PAGE_SIZE, - CONFIG_NAND_SUNXI_SPL_ECC_STRENGTH, - CONFIG_NAND_SUNXI_SPL_ECC_PAGE_SIZE, - 5, offs, size, dest, syndrome); + const struct { + int page_size; + int ecc_strength; + int ecc_page_size; + int addr_cycles; + } nand_configs[] = { + { 8192, 40, 1024, 5 }, + { 8192, 24, 1024, 5 }, + }; + static int nand_config = -1; + int i; + + if (nand_config == -1) { + for (i = 0; i < ARRAY_SIZE(nand_configs); i++) { + debug("nand: trying page %d ecc %d / %d addr %d: ", + nand_configs[i].page_size, + nand_configs[i].ecc_strength, + nand_configs[i].ecc_page_size, + nand_configs[i].addr_cycles); + if (nand_read_ecc(nand_configs[i].page_size, + nand_configs[i].ecc_strength, + nand_configs[i].ecc_page_size, + nand_configs[i].addr_cycles, + offs, size, dest, syndrome) == 0) { + debug("success\n"); + nand_config = i; + return 0; + } + debug("failed\n"); + } + return -1; + } + + return nand_read_ecc(nand_configs[nand_config].page_size, + nand_configs[nand_config].ecc_strength, + nand_configs[nand_config].ecc_page_size, + nand_configs[nand_config].addr_cycles, + offs, size, dest, syndrome); }
int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest)
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
Auto detect the nand configuration parameters, like the BROM does.
This allows us to get rid of various Kconfig settings, and is necessary to support generic boards like the mk802 which have seen many production runs with different nands.
The full blown u-boot/kernel nand driver uses the nand id to determine this info, for the SPL we do as the BROM does and simply try a few standard configs.
Note the table only contains configs which are known to actually be used, rather then all the configs the BROM tries. This means that it may need to be updated in the future as we add support for nand on more boards.
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
(assuming table vs. probing by nand id is OK by Scott)
The BROM does not care / use bad page markings, instead it deals with any bad pages in the first erase-block by simply trying to load "boot0" from the first next erase-block.
This commit implements the same strategy for the sunxi spl nand code, allowing it to boot from the backup boot partition when the main boot partition is bad (tested by erasing the main boot partition).
Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/mtd/nand/sunxi_nand_spl.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 5687cd8..a75e674 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -348,6 +348,23 @@ static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest,
int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) { + const uint32_t boot_offsets[] = { + 0 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS, + 1 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS, + 2 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS, + 4 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS, + }; + int i; + + if (offs == CONFIG_SYS_NAND_U_BOOT_OFFS) { + for (i = 0; i < ARRAY_SIZE(boot_offsets); i++) { + if (nand_read_buffer(boot_offsets[i], size, + dest, 1) == 0) + return 0; + } + return -1; + } + return nand_read_buffer(offs, size, dest, 1); }
On Sat, 2015-08-15 at 22:02 +0200, Hans de Goede wrote:
The BROM does not care / use bad page markings, instead it deals with any bad pages in the first erase-block by simply trying to load "boot0" from the first next erase-block.
"first/next"? Maybe, or maybe s/first //?
This commit implements the same strategy for the sunxi spl nand code, allowing it to boot from the backup boot partition when the main boot partition is bad (tested by erasing the main boot partition).
Signed-off-by: Hans de Goede hdegoede@redhat.com
Acked-by: Ian Campbell ijc@hellion.org.uk
drivers/mtd/nand/sunxi_nand_spl.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c index 5687cd8..a75e674 100644 --- a/drivers/mtd/nand/sunxi_nand_spl.c +++ b/drivers/mtd/nand/sunxi_nand_spl.c @@ -348,6 +348,23 @@ static int nand_read_buffer(uint32_t offs, unsigned int size, void *dest,
int nand_spl_load_image(uint32_t offs, unsigned int size, void *dest) {
- const uint32_t boot_offsets[] = {
0 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,1 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,2 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,4 * 1024 * 1024 + CONFIG_SYS_NAND_U_BOOT_OFFS,- };
- int i;
- if (offs == CONFIG_SYS_NAND_U_BOOT_OFFS) {
for (i = 0; i < ARRAY_SIZE(boot_offsets); i++) {if (nand_read_buffer(boot_offsets[i], size,dest, 1) == 0)return 0;}return -1;- }
- return nand_read_buffer(offs, size, dest, 1);
}
participants (4)
-
Chen-Yu Tsai -
Hans de Goede -
Ian Campbell -
Maxime Ripard