From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Mon, 30 Jan 2012 07:39:15 +0100 Subject: [U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it. In-Reply-To: References: <1326133550-6706-1-git-send-email-urwithsughosh@gmail.com> <20120113082618.GA1557@Hardy> <4F104DD9.2080308@denx.de> <20120113173821.GC4482@Hardy> <20120117064640.GA1547@Hardy> <20120119065332.GA21447@Hardy> <4F17EDBB.5000501@ti.com> <4F180485.7070004@ti.com> <4F192B48.5040905@ti.com> <4F195A62.7030207@ti.com> <4F1966EC.4000309@ti.com> Message-ID: <4F263B13.2090403@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Christian, Christian Riesch wrote: > Hi all, > > On Fri, Jan 27, 2012 at 7:33 PM, Tom Rini 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