From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 1 Nov 2011 23:44:42 +0100 Subject: [U-Boot] [PATCH 4/4] PXA: Adapt Voipac PXA270 to OneNAND SPL In-Reply-To: <4EB073DB.1020903@freescale.com> References: <1320067393-18822-1-git-send-email-marek.vasut@gmail.com> <201111012312.20339.marek.vasut@gmail.com> <4EB073DB.1020903@freescale.com> Message-ID: <201111012344.42992.marek.vasut@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de > On 11/01/2011 05:12 PM, Marek Vasut wrote: > >> On 10/31/2011 08:23 AM, Marek Vasut wrote: > >>> Signed-off-by: Marek Vasut > >>> Cc: Albert ARIBAUD > >>> --- > > > > [...] > > > >>> + 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. > > 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. > > >> 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. Is in V2, missing. > > >>> +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. > > >> 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? > > >>> 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. > > 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? No, they are mutually exclusive. > > In any case, best to use the symbol that most closely matches the reason > you're skipping it, which is something SPL-related. > > -Scott