On 10/31/2011 08:23 AM, Marek Vasut wrote:
Signed-off-by: Marek Vasut marek.vasut@gmail.com Cc: Albert ARIBAUD albert.u.boot@aribaud.net
[...]
- for (page = 0; page <= total_pages; page++) {
ret = spl_onenand_read_page(0, page, addr, data.pagesize);if (ret)total_pages++;elseaddr += 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.
How much of this is board-specific?
+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++;elseaddr += 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?
Good idea.
+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.
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.
+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.
+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 (;;)
;+}
+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();
Why are you not specifying static on things that are not needed outside this file?
They are actually needed outside.
diff --git a/board/vpac270/vpac270.c b/board/vpac270/vpac270.c index 43bbdff..f146009 100644 --- a/board/vpac270/vpac270.c +++ b/board/vpac270/vpac270.c @@ -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;
}
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.
-Scott