> 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
