Hello Christian, Christian Riesch wrote: > Hi all, > > On Fri, Jan 27, 2012 at 7:33 PM, Tom Rini <tom.r...@gmail.com> wrote: >> So, what do we want to do here? We really want to get this fix in so >> we can get the hawkboard SPL changes in, and the other platforms / >> fixups that are gated by that. >> >> If I can sum it up, in the relevant section of code we have incorrect >> comments and the init code is not following what the manual says the >> correct sequence is. However, given the (potentially wide) impact the >> changes would have, Albert had previously requested making the change >> opt-in (but I believe this request came before the "the manual says we >> must do ..."). If this is still the case? If so, can we get an >> updated patch? Thanks! > > A few thoughts: > > 1) Before commit ca4b55800ed74207c35271bf7335a092d4955416 the low > level initialization code that we are discussing here was only > executed if CONFIG_SKIP_LOWLEVEL_INIT is not defined. For ARM926EJS > the relevant section in start.S looked like this > > #ifndef CONFIG_SKIP_LOWLEVEL_INIT > flush caches > turn off dcache, do some other configurations of the CPU, enable icache > call the SoC specific lowlevel_init > #endif > > For DA850 SoCs we had no lowlevel_init routine and therefore > CONFIG_SKIP_LOWLEVEL_INIT defined in all board configurations. The > lowlevel initialization was done by TI's UBL boot loader. Now we have > boards that don't use UBL (e.g. enbw_cmc, calimain, da850evm when used > with the SPL), therefore we need some lowlevel init. Most important > configuration is enabling icache, otherwise u-boot startup gets really > slow. > > Since commit ca4b55800ed74207c35271bf7335a092d4955416 it is > > flush caches > turn off dcache, do some other configurations of the CPU, enable icache > #ifndef CONFIG_SKIP_LOWLEVEL_INIT > call the SoC specific lowlevel_init > #endif > > So the change that has a big impact is already done and in mainline.
Yep. > Perhaps we should revert that change and instead remove > CONFIG_SKIP_LOWLEVEL_INIT from the da850 board config files. But since > we don't need the lowlevel_init function for DA850 SoCs we must either > add a dummy function or add some additional defines that allow > removing the CONFIG_SKIP_LOWLEVEL_INIT define without calling > lowlevel_init. Any suggestions how such defines should be called or > how the logic behind them should be so it doesn't cause breakage of > existing boards? or we should introduce a "CONFIG_SKIP_CPU_CRIT_INIT" define, which if defined, prevent the jump to cpu_init_crit ... so we have the same behaviour as before commit "ca4b55800ed74207c35271bf7335a092d4955416" Boards which have problems with cpu_crit_init, should define this new define ... but it would be better to find out, what is really going on here ... > What do you think? Or is reverting to dangerous since we would change > the behavior of start.S twice if we do so? See comment above. I can send such a patch for this, if we decide to go this direction ... as I need the cpu_init_crit part for the enbw_cmc board, but do not need the branch to "lowlevel_init" ... > 2) The current version of Sughosh's patch does not change the logic > behind the LOWLEVEL_INIT defines but just fixes the code to agree with > ARM's manual. Instead of invalidating the cache it now is flushed. I > think we should therefore merge his patch (@Tom: Yes, the manual says > we must do.). The big change that Albert fears was already done > earlier in commit ca4b55800ed74207c35271bf7335a092d4955416. And while > we are doing this we also get the comments right. What do you mean with commit "ca4b55800ed74207c35271bf7335a092d4955416"? I could not find it in mainline ... > 3) As Sughosh pointed out, the current code changes the V bit > (location of exceptions). Sughosh's patch removes this code that does > this change. I'm not sure if this is correct or not, so maybe you, > Tom, could put your TI hat on again and clarify this? Yes, I am also not sure, what to do with this V Bit ... > 4) The current code turns on icache. This is great since my board > executes u-boot directly from flash until it relocates itself and thus > the startup time is greatly reduced by using icache. However, there is > this CONFIG_SYS_ICACHE_OFF define. Should the code be changed to > respect this define? Yep, that should be done. Default should be icache on, so we do not change exisiting behaviour. bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot