> On 10/31/2011 08:23 AM, Marek Vasut wrote: > > Signed-off-by: Marek Vasut <marek.va...@gmail.com> > > Cc: Albert ARIBAUD <albert.u.b...@aribaud.net> > > --- [...]
> > > + 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. > > 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++; > > + else > > + 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? 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot