[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Heiko Schocher]

Hello Christian,

Christian Riesch wrote:
> Hi all,
> 
> On Fri, Jan 27, 2012 at 7:33 PM, Tom Rini <tom.rini at 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