Voipac PXA270 OneNAND SPL

The start.S on PXA was very obscure. This reworks it back to be close to arm1136 start.S and others.

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---
 arch/arm/cpu/pxa/cpu.c   |   16 ++
 arch/arm/cpu/pxa/start.S |  382 ++++++++++++++++-----------------------------
 2 files changed, 152 insertions(+), 246 deletions(-)

Am 31.10.2011 14:23, schrieb Marek Vasut:

Quoted 11 lines

The start.S on PXA was very obscure. This reworks it back to be close to arm1136 start.S and others.

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---

 arch/arm/cpu/pxa/cpu.c   |   16 ++
 arch/arm/cpu/pxa/start.S |  382

++++++++++++++++----------------------------- 2 files changed, 152 insertions(+), 246 deletions(-)

Hi Marek,

if I understand your code right, you change the initial memory from cache to SRAM,
but this is not available on the pxa255. Do you plan to drop the pxa255 support?

Do you plan to update the pxa boards config?

I assume that I can drop my patch to fix the relocation on pxa.

Regards,
Stefan

Quoted 12 lines

Am 31.10.2011 14:23, schrieb Marek Vasut:

Quoted 11 lines

The start.S on PXA was very obscure. This reworks it back to be close to arm1136 start.S and others.

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---

 arch/arm/cpu/pxa/cpu.c   |   16 ++
 arch/arm/cpu/pxa/start.S |  382

++++++++++++++++----------------------------- 2 files changed, 152 insertions(+), 246 deletions(-)

Quoted 6 lines

Hi Marek,

if I understand your code right, you change the initial memory from cache to SRAM,
but this is not available on the pxa255. Do you plan to drop the pxa255 support?

No, you're right. I'll probably have to add a workaround for this issue for pxa25x and 26x. Damn.

Quoted 3 lines

Do you plan to update the pxa boards config?

I assume that I can drop my patch to fix the relocation on pxa.

Do you have any pxa250 board and are you willing to help with this patch?

drivers/mtd/onenand/onenand_spl.c new file +18 -0

+void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{
+	uint32_t tmp;
+	uint32_t dev_id, density;
+
+	/* Default geometry -- 2048b page, 128k erase block. */
+	data->pagesize = 2048;
+	data->erasesize = 0x20000;
+
+	tmp = onenand_readw(ONENAND_REG_TECHNOLOGY);
+	if (tmp)
+		goto dev_4k;
+
+	dev_id = onenand_readw(ONENAND_REG_DEVICE_ID);
+	density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
+	density &= ONENAND_DEVICE_DENSITY_MASK;
+
+	if (density < ONENAND_DEVICE_DENSITY_4Gb)

drivers/mtd/onenand/onenand_spl.c new file +4 -0

+int spl_onenand_read_block(uint32_t block, uint8_t *buf, uint32_t *read)
+{
+	struct spl_onenand_data data;
+	uint32_t page;

include/onenand_uboot.h +7 -0

index 92279d5..66828ce 100644
@@ -16,6 +16,8 @@
 
 #include <linux/types.h>
 
+#ifndef	CONFIG_SPL_BUILD
+
 /* Forward declarations */
 struct mtd_info;
 struct mtd_oob_ops;

@@ -52,4 +54,20 @@ extern int flexonenand_set_boundary(struct mtd_info *mtd, int die,
 extern void s3c64xx_onenand_init(struct mtd_info *);
 extern void s3c64xx_set_width_regs(struct onenand_chip *);
 
+#else
+
+#define ONENAND_PAGES_PER_BLOCK		64
+
+struct spl_onenand_data {

drivers/mtd/onenand/onenand_spl.c new file +18 -0

+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{
+	uint32_t tmp;
+	uint32_t dev_id, density;
+
+	/* Default geometry -- 2048b page, 128k erase block. */
+	data->pagesize = 2048;
+	data->erasesize = 0x20000;
+
+	tmp = onenand_readw(ONENAND_REG_TECHNOLOGY);
+	if (tmp)
+		goto dev_4k;
+
+	dev_id = onenand_readw(ONENAND_REG_DEVICE_ID);
+	density = dev_id >> ONENAND_DEVICE_DENSITY_SHIFT;
+	density &= ONENAND_DEVICE_DENSITY_MASK;
+
+	if (density < ONENAND_DEVICE_DENSITY_4Gb)

+int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len)

drivers/mtd/onenand/onenand_spl.c new file +8 -0

+	struct spl_onenand_data data;
+	uint32_t total_pages;
+	uint32_t block;
+	uint32_t page, rpage;
+	int ret, err = 0;
+
+	spl_onenand_get_geometry(&data);
+

Quoted 2 lines

(quoted diff) +2 -0

+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	if (data.pagesize == 2048) {
+		total_pages = len / 2048;
+		page = offset / 2048;
+		total_pages += !!(len & 2047);

drivers/mtd/onenand/onenand_spl.c line 137

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	} else if (data.pagesize == 4096) {
+		total_pages = len / 4096;
+		page = offset / 4096;
+		total_pages += !!(len & 4095);
+	}

What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here...

drivers/mtd/onenand/onenand_spl.c line 148

Quoted 11 lines

drivers/mtd/onenand/onenand_spl.c new file +10 -0

+	for (; page <= total_pages; page++) {
+		block = page / ONENAND_PAGES_PER_BLOCK;
+		rpage = page & (ONENAND_PAGES_PER_BLOCK - 1);
+		ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
+		if (ret) {
+			total_pages++;
+			err |= 1;
+		} else
+			addr += data.pagesize / 4;
+	}

As discussed, please retain the existing block-skipping semantics. And if you do skip a block, that's not an error to be propagated upward (as opposed to something like an uncorrectable ECC error).

If one side of an if statement requires braces, both sides should have them.

include/onenand_uboot.h line 20

Quoted 10 lines

include/onenand_uboot.h +2 -0

index 92279d5..fcb50ff 100644
@@ -16,6 +16,8 @@
 
 #include <linux/types.h>
 
+#ifndef	CONFIG_SPL_BUILD
+

Please use a space rather than a tab after #define, #ifndef, etc.

Quoted 5 lines

include/onenand_uboot.h +0 -0

 /* Forward declarations */
 struct mtd_info;
 struct mtd_oob_ops;

include/onenand_uboot.h extern int flexonenand_set_boundary(struct mtd_info · line 61

Quoted 10 lines

include/onenand_uboot.h +5 -0

@@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info
 *mtd, int die,
 
 extern void s3c64xx_onenand_init(struct mtd_info *);
 extern void s3c64xx_set_width_regs(struct onenand_chip *);
 
+#else
+
+int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t len);
+
+#endif

Why does this need to be ifdeffed at all? We normally don't ifdef header declarations.

-Scott

Quoted 2 lines

(quoted diff) +2 -0

+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	if (data.pagesize == 2048) {
+		total_pages = len / 2048;
+		page = offset / 2048;
+		total_pages += !!(len & 2047);

drivers/mtd/onenand/onenand_spl.c line 137

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	} else if (data.pagesize == 4096) {
+		total_pages = len / 4096;
+		page = offset / 4096;
+		total_pages += !!(len & 4095);
+	}

What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here...

drivers/mtd/onenand/onenand_spl.c

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +4 -0

+	for (; page <= total_pages; page++) {
+		block = page / ONENAND_PAGES_PER_BLOCK;
+		rpage = page & (ONENAND_PAGES_PER_BLOCK - 1);
+		ret = spl_onenand_read_page(block, rpage, addr,

drivers/mtd/onenand/onenand_spl.c line 148

Quoted 6 lines

drivers/mtd/onenand/onenand_spl.c new file +6 -0

+		if (ret) {
+			total_pages++;
+			err |= 1;
+		} else
+			addr += data.pagesize / 4;
+	}

As discussed, please retain the existing block-skipping semantics. And if you do skip a block, that's not an error to be propagated upward (as opposed to something like an uncorrectable ECC error).

If one side of an if statement requires braces, both sides should have them.

include/onenand_uboot.h line 20

Quoted 10 lines

include/onenand_uboot.h +2 -0

index 92279d5..fcb50ff 100644
@@ -16,6 +16,8 @@
 
 #include <linux/types.h>
 
+#ifndef	CONFIG_SPL_BUILD
+

Please use a space rather than a tab after #define, #ifndef, etc.

Quoted 7 lines

include/onenand_uboot.h +0 -0

 /* Forward declarations */
 struct mtd_info;
 struct mtd_oob_ops;

@@ -52,4 +54,10 @@ extern int flexonenand_set_boundary(struct mtd_info
 *mtd, int die,
 

include/onenand_uboot.h extern int flexonenand_set_boundary(struct mtd_info *mtd, int die,

Quoted 6 lines

include/onenand_uboot.h +3 -0

 extern void s3c64xx_onenand_init(struct mtd_info *);
 extern void s3c64xx_set_width_regs(struct onenand_chip *);
 
+#else
+
+int spl_onenand_load_image(uint32_t dst, uint32_t offset, uint32_t

include/onenand_uboot.h

Quoted 2 lines

include/onenand_uboot.h +2 -0

+
+#endif

Why does this need to be ifdeffed at all? We normally don't ifdef header declarations.

-Scott

Hi all,

Marek, did you see the onenand_ipl/onenand_read.c at u-boot? It's already implemented and used for OneNAND boot.

How do you think migrate this file into generic SPL framework?

Thank you,
Kyungmin Park

Quoted 3 lines · Marek Vasut

Quoted 2 lines

(quoted diff) +2 -0

+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{

[...]

Quoted 5 lines

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	if (data.pagesize == 2048) {
+		total_pages = len / 2048;
+		page = offset / 2048;
+		total_pages += !!(len & 2047);

Quoted 8 lines

drivers/mtd/onenand/onenand_spl.c line 137

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	} else if (data.pagesize == 4096) {
+		total_pages = len / 4096;
+		page = offset / 4096;
+		total_pages += !!(len & 4095);
+	}

What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here...

It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

Quoted 20 lines

Quoted 3 lines · Marek Vasut

Quoted 2 lines

(quoted diff) +2 -0

+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{

[...]

Quoted 5 lines

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	if (data.pagesize == 2048) {
+		total_pages = len / 2048;
+		page = offset / 2048;
+		total_pages += !!(len & 2047);

Quoted 8 lines

drivers/mtd/onenand/onenand_spl.c line 137

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	} else if (data.pagesize == 4096) {
+		total_pages = len / 4096;
+		page = offset / 4096;
+		total_pages += !!(len & 4095);
+	}

What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here...

Quoted 2 lines

It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

It shouldn't do that if the divisor is a constant power of 2. The compiler will turn it into a shift, just like with the other divides in the above code fragment.

You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it in each branch of the if statement instead of that awkward and slightly more expensive !!(len & 4095) construct.

Quoted 28 lines · Marek Vasut

Quoted 20 lines

Quoted 3 lines · Marek Vasut

Quoted 2 lines

(quoted diff) +2 -0

+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{

[...]

Quoted 5 lines

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	if (data.pagesize == 2048) {
+		total_pages = len / 2048;
+		page = offset / 2048;
+		total_pages += !!(len & 2047);

Quoted 8 lines

drivers/mtd/onenand/onenand_spl.c line 137

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	} else if (data.pagesize == 4096) {
+		total_pages = len / 4096;
+		page = offset / 4096;
+		total_pages += !!(len & 4095);
+	}

What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here...

Quoted 2 lines

It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

It shouldn't do that if the divisor is a constant power of 2. The compiler will turn it into a shift, just like with the other divides in the above code fragment.

Quoted 3 lines

You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it in each branch of the if statement instead of that awkward and slightly more expensive !!(len & 4095) construct.

Expensive in what way?

Either way, I don't think this matters that much.

Quoted 28 lines

Quoted 28 lines · Marek Vasut

Quoted 20 lines

Quoted 3 lines · Marek Vasut

Quoted 2 lines

(quoted diff) +2 -0

+static void spl_onenand_get_geometry(struct spl_onenand_data *data)
+{

[...]

Quoted 5 lines

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	if (data.pagesize == 2048) {
+		total_pages = len / 2048;
+		page = offset / 2048;
+		total_pages += !!(len & 2047);

Quoted 8 lines

drivers/mtd/onenand/onenand_spl.c line 137

Quoted 5 lines

drivers/mtd/onenand/onenand_spl.c new file +5 -0

+	} else if (data.pagesize == 4096) {
+		total_pages = len / 4096;
+		page = offset / 4096;
+		total_pages += !!(len & 4095);
+	}

What's wrong with DIV_ROUND_UP? It should produce smaller code than what you've done here...

Quoted 2 lines

It pulls in aeabi_*div* functions, which won't fit into block 0 of Onenand.

It shouldn't do that if the divisor is a constant power of 2. The compiler will turn it into a shift, just like with the other divides in the above code fragment.

Quoted 5 lines

Quoted 3 lines

You can't use DIV_ROUND_UP directly on data.pagesize, but you can use it in each branch of the if statement instead of that awkward and slightly more expensive !!(len & 4095) construct.

Expensive in what way?

Compare the resulting asm code. You're replacing this:

	a = (b + 4095) >> 12;

with this:

	a = b >> 12;
	if (b & 4095)
		a++;

Quoted 1 line

Either way, I don't think this matters that much.

This is code that has to fit in 1K -- why waste instructions by writing code in a way that is *less* readable?

-Scott

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---
 board/vpac270/Makefile    |    6 ++
 board/vpac270/onenand.c   |  136 +++++++++++++++++++++++++++++++++++++++++++++
 board/vpac270/vpac270.c   |    2 +
 include/configs/vpac270.h |   25 +++++++--
 4 files changed, 164 insertions(+), 5 deletions(-)
 create mode 100644 board/vpac270/onenand.c

board/vpac270/Makefile +0 -0

index b5c60fd..f25822f 100644

board/vpac270/Makefile +0 -0

 SRCS	:= $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS))

board/vpac270/onenand.c new file +37 -0

new file mode 100644
index 0000000..50de2ab
@@ -0,0 +1,136 @@
+/*
+ * Voipac PXA270 OneNAND SPL
+ *
+ * Copyright (C) 2011 Marek Vasut <[email protected]>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <config.h>
+#include <asm/io.h>
+#include <onenand_uboot.h>
+
+extern void pxa_dram_init(void);
+
+inline void spl_copy_self(void)
+{
+	extern uint32_t _end;
+	struct spl_onenand_data data;
+	uint32_t page;
+	uint32_t total_bytes = (uint32_t)&_end - CONFIG_SPL_TEXT_BASE;

board/vpac270/onenand.c new file +19 -0

+inline void spl_copy_uboot(void)
+{
+	uint8_t *addr = (uint8_t *)CONFIG_SYS_TEXT_BASE;
+	struct spl_onenand_data data;
+	uint32_t total_pages;
+	uint32_t block;
+	uint32_t page, rpage;
+	int ret;
+
+	spl_onenand_get_geometry(&data);
+
+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	total_pages = CONFIG_SPL_ONENAND_LOAD_SIZE >> 11;
+	page = CONFIG_SPL_ONENAND_LOAD_ADDR >> 11;
+	if (data.pagesize == 4096) {
+		total_pages >>= 1;
+		page >>= 1;
+	}
+

board/vpac270/onenand.c new file +10 -0

+inline int printf(const char *fmt, ...)
+{
+	return 0;
+}
+
+inline void __coloured_LED_init(void) {}
+inline void __red_LED_on(void) {}
+void coloured_LED_init(void)
+	__attribute__((weak, alias("__coloured_LED_init")));
+void red_LED_on(void)

board/vpac270/vpac270.c +0 -0

index 43bbdff..f146009 100644

Quoted 3 lines

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---

board/vpac270/onenand.c line 56

Quoted 9 lines

board/vpac270/onenand.c new file +8 -0

+	for (page = 0; page <= total_pages; page++) {
+		ret = spl_onenand_read_page(0, page, addr, data.pagesize);
+		if (ret)
+			total_pages++;
+		else
+			addr += data.pagesize;
+	}
+}

You want to skip to the next block if spl_onenand_read_page() fails (which can occur after you've already read some of the block).

How much of this is board-specific?

Quoted 26 lines

board/vpac270/onenand.c new file +26 -0

+inline void spl_copy_uboot(void)
+{
+	uint8_t *addr = (uint8_t *)CONFIG_SYS_TEXT_BASE;
+	struct spl_onenand_data data;
+	uint32_t total_pages;
+	uint32_t block;
+	uint32_t page, rpage;
+	int ret;
+
+	spl_onenand_get_geometry(&data);
+
+	/* The page can be either 2k or 4k, avoid using DIV_ROUND_UP. */
+	total_pages = CONFIG_SPL_ONENAND_LOAD_SIZE >> 11;
+	page = CONFIG_SPL_ONENAND_LOAD_ADDR >> 11;
+	if (data.pagesize == 4096) {
+		total_pages >>= 1;
+		page >>= 1;
+	}
+
+	for (; page <= total_pages; page++) {
+		block = page >> 6;
+		rpage = page & 0xff;
+		ret = spl_onenand_read_page(block, rpage, addr, data.pagesize);
+		if (ret)
+			total_pages++;
+		else

board/vpac270/onenand.c line 56

Quoted 3 lines

board/vpac270/onenand.c new file +3 -0

+			addr += data.pagesize;
+	}
+}

What is so different about this compared to spl_copy_self, that warrants such duplication? Can't you just pass in offset, length, and destination as parameters? Or just have the OneNAND SPL driver export nand_spl_load_image(), as any other NAND SPL driver would?

board/vpac270/onenand.c line 99

Quoted 13 lines

board/vpac270/onenand.c new file +12 -0

+inline void board_init_f(unsigned long unused)
+{
+	uint32_t tmp;
+
+	asm volatile("mov %0, pc" : "=r"(tmp));
+	tmp >>= 24;
+
+	/* The code runs from OneNAND RAM, copy SPL to SRAM and execute it. */
+	if (tmp == 0) {
+		spl_copy_self();
+		asm volatile("mov pc, %0" : : "r"(CONFIG_SPL_TEXT_BASE));
+	}

Is it not possible to use a simple memcpy for spl_copy_self()? If the CPU can run the code, you'd think it could read it.

board/vpac270/onenand.c line 115

Quoted 6 lines

board/vpac270/onenand.c new file +5 -0

+inline void board_init_r(gd_t *id, ulong dest_addr)
+{
+	for (;;)
+		;
+}

This doesn't seem like a useful board_init_r(). If you don't need it, maybe make sure it's not called, and save yourself some bytes in the SPL. Likewise for the other stub functions, where practical.

Quoted 15 lines

board/vpac270/onenand.c new file +15 -0

+inline int printf(const char *fmt, ...)
+{
+	return 0;
+}
+
+inline void __coloured_LED_init(void) {}
+inline void __red_LED_on(void) {}
+void coloured_LED_init(void)
+	__attribute__((weak, alias("__coloured_LED_init")));
+void red_LED_on(void)
+	__attribute__((weak, alias("__red_LED_on")));
+void hang(void) __attribute__ ((noreturn));
+void hang(void)
+{
+	for (;;)

board/vpac270/onenand.c line 136

Quoted 5 lines

board/vpac270/onenand.c new file +5 -0

+		;
+}
+
+inline void icache_disable(void) {}
+inline void dcache_disable(void) {}

Why are you specifying inline on just about everything, even functions that are not used in this file?

Why are you not specifying static on things that are not needed outside this file?

Quoted 17 lines

board/vpac270/vpac270.c +2 -0

index 43bbdff..f146009 100644
@@ -56,7 +56,9 @@ struct serial_device *default_serial_console(void)
 
 extern void pxa_dram_init(void);
 int dram_init(void)
 {
 
+#ifndef	CONFIG_ONENAND
 
 	pxa_dram_init();
 
+#endif

board/vpac270/vpac270.c struct serial_device *default_serial_console(void) · line 64

Quoted 4 lines

board/vpac270/vpac270.c +0 -0

 	gd->ram_size = PHYS_SDRAM_1_SIZE;
 	return 0;
 
 }

Should this really be about whether OneNAND support is present, or should it be based on whether you're using the OneNAND SPL?

-Scott

Quoted 4 lines · Marek Vasut

Quoted 3 lines

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---

[...]

Quoted 12 lines

board/vpac270/onenand.c line 56

Quoted 9 lines

board/vpac270/onenand.c new file +8 -0

+	for (page = 0; page <= total_pages; page++) {
+		ret = spl_onenand_read_page(0, page, addr, data.pagesize);
+		if (ret)
+			total_pages++;
+		else
+			addr += data.pagesize;
+	}
+}

You want to skip to the next block if spl_onenand_read_page() fails (which can occur after you've already read some of the block).

I want to skip to next page, not next block.

Quoted 2 lines

Is it not possible to use a simple memcpy for spl_copy_self()? If the CPU can run the code, you'd think it could read it.

Not exactly. The OneNAND only exposes first 1kb of the contents (aka 1 half of the page 0 in my case). That's why I link all of the relevant code there and the rest of the SPL is aligned beyond that. Then I copy the whole SPL to SRAM and execute it again. Then I init DRAM, copy U-Boot there and run it. Simple, isn't it.

Quoted 5 lines

board/vpac270/onenand.c line 136

Quoted 2 lines

board/vpac270/onenand.c new file +2 -0

+inline void icache_disable(void) {}
+inline void dcache_disable(void) {}

Why are you specifying inline on just about everything, even functions that are not used in this file?

They are, by dram_init();

Quoted 2 lines

Why are you not specifying static on things that are not needed outside this file?

They are actually needed outside.

Quoted 19 lines

Quoted 19 lines

board/vpac270/vpac270.c +2 -0

index 43bbdff..f146009 100644
@@ -56,7 +56,9 @@ struct serial_device *default_serial_console(void)
 
 extern void pxa_dram_init(void);
 int dram_init(void)
 {
 
+#ifndef	CONFIG_ONENAND
 
 	pxa_dram_init();
 
+#endif
 
 	gd->ram_size = PHYS_SDRAM_1_SIZE;
 	return 0;

Quoted 4 lines

Quoted 1 line

}

Should this really be about whether OneNAND support is present, or should it be based on whether you're using the OneNAND SPL?

Basically, on this board this is the same thing.

Quoted 19 lines

Quoted 4 lines · Marek Vasut

Quoted 3 lines

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---

[...]

Quoted 11 lines

board/vpac270/onenand.c line 56

Quoted 8 lines

board/vpac270/onenand.c new file +8 -0

+	for (page = 0; page <= total_pages; page++) {
+		ret = spl_onenand_read_page(0, page, addr, data.pagesize);
+		if (ret)
+			total_pages++;
+		else
+			addr += data.pagesize;
+	}
+}

You want to skip to the next block if spl_onenand_read_page() fails (which can occur after you've already read some of the block).

Quoted 1 line

I want to skip to next page, not next block.

That's not how we normally do things, and is not what the current OneNAND IPL does.

Bad block markers apply to the entire block -- unless this is a difference I'm not aware of between NAND and OneNAND.

Quoted 9 lines

Quoted 3 lines

Is it not possible to use a simple memcpy for spl_copy_self()? If the CPU can run the code, you'd think it could read it.

Not exactly. The OneNAND only exposes first 1kb of the contents (aka 1 half of the page 0 in my case). That's why I link all of the relevant code there and the rest of the SPL is aligned beyond that. Then I copy the whole SPL to SRAM and execute it again. Then I init DRAM, copy U-Boot there and run it. Simple, isn't it.

Where do you ensure that the stuff used so far is within the 1K? What parts are not within the 1K?

I don't see a linker script.

Quoted 8 lines

Quoted 6 lines

board/vpac270/onenand.c line 136

Quoted 3 lines

board/vpac270/onenand.c new file +2 -0

+inline void icache_disable(void) {}
+inline void dcache_disable(void) {}

Why are you specifying inline on just about everything, even functions that are not used in this file?

They are, by dram_init();

There's no point marking something inline if it's not used later on in the same file -- functions aren't inlined across file boundaries. You've got inline functions at the very end of the file.

For that matter, there's not much point marking anything inline that isn't a static inline in a header file (where the compiler must not generate a non-inline version) -- the compiler has heuristics for inlining things, and excessive inlining tends to make things bigger rather than smaller.

Quoted 4 lines

Quoted 2 lines

Why are you not specifying static on things that are not needed outside this file?

They are actually needed outside.

All of them, including spl_copy_uboot and spl_copy_self?

Quoted 24 lines

Quoted 23 lines

Quoted 10 lines

board/vpac270/vpac270.c +0 -0

index 43bbdff..f146009 100644
@@ -56,7 +56,9 @@ struct serial_device *default_serial_console(void)
 
 extern void pxa_dram_init(void);
 int dram_init(void)
 {
 

board/vpac270/vpac270.c struct serial_device *default_serial_console(void) · line 64

Quoted 10 lines

board/vpac270/vpac270.c +2 -0

+#ifndef	CONFIG_ONENAND
 
 	pxa_dram_init();
 
+#endif
 
 	gd->ram_size = PHYS_SDRAM_1_SIZE;
 	return 0;
 
 }

Should this really be about whether OneNAND support is present, or should it be based on whether you're using the OneNAND SPL?

Quoted 1 line

Basically, on this board this is the same thing.

If you can turn off onenand at all, that suggests there's another boot source. Is it not possible to access onenand when using that other boot source?

In any case, best to use the symbol that most closely matches the reason you're skipping it, which is something SPL-related.

-Scott

Quoted 22 lines · Marek Vasut

Quoted 20 lines

Quoted 4 lines · Marek Vasut

Quoted 3 lines

Signed-off-by Marek Vasut <[email protected]>

Cc Albert ARIBAUD <[email protected]>

---

[...]

Quoted 11 lines

board/vpac270/onenand.c line 56

Quoted 8 lines

board/vpac270/onenand.c new file +8 -0

+	for (page = 0; page <= total_pages; page++) {
+		ret = spl_onenand_read_page(0, page, addr, data.pagesize);
+		if (ret)
+			total_pages++;
+		else
+			addr += data.pagesize;
+	}
+}

You want to skip to the next block if spl_onenand_read_page() fails (which can occur after you've already read some of the block).

I want to skip to next page, not next block.

Quoted 5 lines

That's not how we normally do things, and is not what the current OneNAND IPL does.

Bad block markers apply to the entire block -- unless this is a difference I'm not aware of between NAND and OneNAND.

Well then it will fail reading the whole block and continue onwards ... it's a bit slower like this.

+inline void board_init_f(unsigned long unused)

board/vpac270/u-boot-spl.lds new file +10 -0

+	/* Start of the rest of the SPL */
+	. = CONFIG_SPL_TEXT_BASE + 0x800;
+
+	.text.1	:
+	{
+		*(.text*)
+	}
+
+	. = ALIGN(4);
+	.rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }

board/vpac270/onenand.c line 52

Quoted 3 lines

board/vpac270/onenand.c new file +3 -0

+	onenand_spl_load_image(CONFIG_SPL_ONENAND_LOAD_ADDR,
+				CONFIG_SPL_ONENAND_LOAD_SIZE,
+				(void *)CONFIG_SYS_TEXT_BASE);

If we make it "nand_spl_load_image", and make the #defines conform, we can have the same code call the function for nand and onenand. I don't see any reason why onenand is a completely different subsystem in general, rather than just another NAND driver. The NAND subsystem's driver interface is lower level than it should be, but that affects other NAND controllers as well (such as fsl_elbc).

Switching to the generic nand_boot() in
http://patchwork.ozlabs.org/patch/123219/ would get you the ability to load the environment during the SPL.

-Scott

Quoted 12 lines · Marek Vasut

board/vpac270/onenand.c line 52

Quoted 3 lines

board/vpac270/onenand.c new file +3 -0

+	onenand_spl_load_image(CONFIG_SPL_ONENAND_LOAD_ADDR,
+				CONFIG_SPL_ONENAND_LOAD_SIZE,
+				(void *)CONFIG_SYS_TEXT_BASE);

If we make it "nand_spl_load_image", and make the #defines conform, we can have the same code call the function for nand and onenand. I don't see any reason why onenand is a completely different subsystem in general, rather than just another NAND driver. The NAND subsystem's driver interface is lower level than it should be, but that affects other NAND controllers as well (such as fsl_elbc).

Quoted 5 lines

Switching to the generic nand_boot() in
http://patchwork.ozlabs.org/patch/123219/ would get you the ability to load the environment during the SPL.

-Scott

I don't think I understand. Why do you want to mix onenand and nand ?

Also, will your approach still allow me to squeeze the important code into the first 1kb for the initial copying of SPL?

Quoted 16 lines

Quoted 16 lines · Marek Vasut

board/vpac270/onenand.c line 52

Quoted 3 lines

board/vpac270/onenand.c new file +3 -0

+	onenand_spl_load_image(CONFIG_SPL_ONENAND_LOAD_ADDR,
+				CONFIG_SPL_ONENAND_LOAD_SIZE,
+				(void *)CONFIG_SYS_TEXT_BASE);

If we make it "nand_spl_load_image", and make the #defines conform, we can have the same code call the function for nand and onenand. I don't see any reason why onenand is a completely different subsystem in general, rather than just another NAND driver. The NAND subsystem's driver interface is lower level than it should be, but that affects other NAND controllers as well (such as fsl_elbc).

Switching to the generic nand_boot() in
http://patchwork.ozlabs.org/patch/123219/ would get you the ability to load the environment during the SPL.

Quoted 3 lines

Quoted 1 line

-Scott

I don't think I understand. Why do you want to mix onenand and nand ?

Why do we want to separate them? What is the fundamental difference between OneNAND, and a high-level NAND controller such as fsl_elbc?

Maybe there would be some differences on init if we can't produce "normal" ID data, but that doesn't justify duplicating the whole subsystem.

Why should the code that just wants to use an API to move data around need to care which it is? Why should there be behavioral differences that aren't rooted in the actual hardware? Another approach might be to use MTD as the common interface, but factor out common code into libraries that drivers can use, and avoid the main nand_base.c code even for things like fsl_elbc.

This is not a new complaint -- I've asked for this before but nobody's put the time into sorting out the mess (and I have neither time nor hardware nor documentation). The SPL load_image function is a simple enough interface to start with, though. :-)

In fact, it should probably just be spl_load_image() with whatever boot source has been configured into this SPL build.

Quoted 3 lines

Also, will your approach still allow me to squeeze the important code into the first 1kb for the initial copying of SPL?

If you can't fit the common load sequence in, then of course don't use it, but there's no need for the function name to be different.

-Scott

Quoted 3 lines · Marek Vasut

Why do we want to separate them? What is the fundamental difference between OneNAND, and a high-level NAND controller such as fsl_elbc?

Honestly, I'm not the author of the subsystem, but please check the documentation. The way we retrieve data from onenand is different to NAND.

Quoted 2 lines

Maybe there would be some differences on init if we can't produce "normal" ID data, but that doesn't justify duplicating the whole subsystem.

Where do you see such duplication? cmd_onenand ?

Quoted 6 lines

Why should the code that just wants to use an API to move data around need to care which it is? Why should there be behavioral differences that aren't rooted in the actual hardware? Another approach might be to use MTD as the common interface, but factor out common code into libraries that drivers can use, and avoid the main nand_base.c code even for things like fsl_elbc.

I think you're mistaken here. OneNAND != NAND.

Quoted 7 lines

Quoted 3 lines · Marek Vasut

Why do we want to separate them? What is the fundamental difference between OneNAND, and a high-level NAND controller such as fsl_elbc?

Honestly, I'm not the author of the subsystem, but please check the documentation. The way we retrieve data from onenand is different to NAND.

What documentation? How is it different? There are substantial differences in how we "retrieve data" between drivers that use the NAND subsystem. Surely you've seen that in the mxs_nand driver. :-)

Quoted 5 lines

Quoted 3 lines

Maybe there would be some differences on init if we can't produce "normal" ID data, but that doesn't justify duplicating the whole subsystem.

Where do you see such duplication? cmd_onenand ?

cmd_onenand and env_onenand are the most irritating, since they're at a layer that really shouldn't care about the differences -- we should probably have a plain "mtd" command instead, for most of the functionality.

There's also onenand_bbt.c -- what are the hardware-based differences in how the bad block table is managed?

nand_base.c/onenand_base.c are less clear. Obviously much of what is in onenand_base.c would be in the controller driver if it used the NAND subsystem. But it looks like some of it is duplication.

Quoted 8 lines

Quoted 6 lines

Why should the code that just wants to use an API to move data around need to care which it is? Why should there be behavioral differences that aren't rooted in the actual hardware? Another approach might be to use MTD as the common interface, but factor out common code into libraries that drivers can use, and avoid the main nand_base.c code even for things like fsl_elbc.

I think you're mistaken here. OneNAND != NAND.

Well, last I tried I couldn't find any public documentation, so all I have to go on is the code, some marketing-type info, and asking questions of people that appear to know more about OneNAND than I do. :-)

From what I can see, it looks like NAND with an integrated controller that exposes an unusual command set, but still for the most part provides the same operations. Several of our existing NAND-subsystem drivers have to fake the command set for the generic layer as well. Initialization/identification might be a problem area that current drivers don't have to deal with, though. Actually integrating OneNAND with NAND would likely involve an already-needed restructuring of the subsystem.

If the answer really is that it makes sense to consider OneNAND to be a totally different thing from NAND, then it's outside my jurisdiction as NAND custodian -- which is fine with me. Frankly, even if it does make sense to merge them, I'd rather not be custodian of the OneNAND stuff unless someone is actually willing to do the merge. I don't have access to hardware or documentation, and it's an entirely separate codebase. People just started sending me the patches a few years back.

Quoted 5 lines

Quoted 4 lines

This is not a new complaint -- I've asked for this before but nobody's put the time into sorting out the mess (and I have neither time nor hardware nor documentation). The SPL load_image function is a simple enough interface to start with, though. :-)

Quoted 7 lines

Well, it seems what you are proposing is way beyond the scope of this patchset.

Quoted 2 lines

In fact, it should probably just be spl_load_image() with whatever boot source has been configured into this SPL build.

What if you have two boot sources?

Traditionally SPL has been small and purpose-built to do exactly one thing -- so we decide at compile-time things that we might otherwise decide at runtime.

If there's a requirement for multiple boot sources decided at runtime, then we'll obviously need a runtime mechanism. -- but it seems a bit hackish to why does it matter whether it's two different types of device, or two of the same type of device (possibly with different controller types)? If the answer is that, for example, NAND versus USB versus MMC is a likely use case, but two different NANDs is not likely, is NAND versus OneNAND any more likely?

Maybe spl_load_image should be a function pointer that board code sets, with each implementation being distinctly named (in which case nand_spl_load_image would become nand_spl_simple_load_image, unless we move it to nand_spl_load.c and make nand_read_page a function pointer).
If needed to save a few bytes, we could use #defines to eliminate the
function pointers in a single-target SPL build.

For now, until we decide to do something SPL-wide, call it what you want.

Quoted 2 lines

For now, until we decide to do something SPL-wide, call it what you want.

Well basically from what I see, you'd like me to do the NAND/OneNAND merge. As I already said, that's way out of the scope of this patchset so I'm not doing that.

Either way, are you OK with current state of the patch?

Quoted 6 lines

Quoted 2 lines

For now, until we decide to do something SPL-wide, call it what you want.

Well basically from what I see, you'd like me to do the NAND/OneNAND merge. As I already said, that's way out of the scope of this patchset so I'm not doing that.

Oh, I didn't mean to imply that was within the scope of this patch. I was just describing the background behind why I'm frustrated with the OneNAND situation, and this looked like an extension of the "onenand is a totally different thing" model, in an independent context.

Quoted 2 lines

Either way, are you OK with current state of the patch?

I won't block it.

-Scott

Quoted 6 lines

Quoted 2 lines

For now, until we decide to do something SPL-wide, call it what you want.

Well basically from what I see, you'd like me to do the NAND/OneNAND merge. As I already said, that's way out of the scope of this patchset so I'm not doing that.

Oh, I didn't mean to imply that was within the scope of this patch. I was just describing the background behind why I'm frustrated with the OneNAND situation, and this looked like an extension of the "onenand is a totally different thing" model, in an independent context.

Quoted 1 line

Either way, are you OK with current state of the patch?

I won't block it.

-Scott