[U-Boot] ARM v7: Flush icache when executing a program with go

[Albert ARIBAUD]

Hi Tom,

On Wed, 15 May 2013 12:51:21 -0400, Tom Rini <trini at ti.com> wrote:

> On Wed, May 15, 2013 at 06:44:10PM +0200, Albert ARIBAUD wrote:
> > Hi Henrik,
> > 
> > On Wed, 15 May 2013 18:34:07 +0200, Henrik Nordstr??m
> > <henrik at henriknordstrom.net> wrote:
> > 
> > > ons 2013-05-15 klockan 17:11 +0200 skrev Albert ARIBAUD:
> > > 
> > > > What is the rationale behind putting it in arch/ rather than in common/
> > > > by adding this to the existing common/cmd_boot.c file under ARMv7
> > > > conditionals?
> > > 
> > > Only because of what I said earlier: blindly calling
> > > invalidate_icache_all() from the go command will cause loud complains on
> > > other arches where the icache is not supported/enabled.
> > 
> > I probably didn't make myself clear: why not put it in comm/cmd_boot.c
> > *whthin a pair of #if ... ARMV7... /#endif conditionals*?
> 
> There is no such #if test we can do for ARMv7 I believe.  There was,
> long ago, a CONFIG_ARMV7 but that's been removed for ages.

Further below I suggest another approach, but for the record, testing
for ARMv7 might be performed with

	#if defined(__ARM_ARCH_7A__) || defined(__ARM_ARCH_7R__)

These macros are generated by ARM gcc with -march=armv7-{a,r}
respectively. Now, generating ARMv7 code does not mean we're building
/for/ ARMv7, but it may be close enough. And if that's not good enough,
then one could always add a define in arch/arm/cpu/armv7/config.mk.

> [snip]
> > > > So, should we not have this icache flush conditioned at compile time on
> > > > cache being compiled in, and at run time on cache being enabled? This
> > > > way, we would automatically cater for the same issue appearing in other
> > > > ARM CPUs, and even more, in other architectures.
> > > 
> > > Except.. see common/cmd_cache.c top of file and you'll see the yelling I
> > > am talking about.
> > 
> > I'd rather you explained it, as you certainly read this file with the
> > knowledge of the issue, and thus immediately focus on part of it that
> > are salient to your eyes, while I will read the same file without such
> > knowledge and might miss the point.
> 
> To summarize why I rejected the last patch in the end, most arches do
> not define an invalidate_icache_all so they will see for every "go":
> No arch specific invalidate_icache_all available!
> And many other arches simply won't compile now as they don't link
> common/cmd_cache.o and do not have an invalidate_icache_all anywhere.

I understand all this, but what I am interested in is the root issue.

IIUC, the problem is that some code is loaded in DDR, and the CPU is
about to jump to it, but its instruction cache is enabled so maybe some
instructions after 'go' will be (wrongly) fetched from I-cache instead
of being read from DDR (and fed into I-cache).

Nothing in this is ARMv7 specific; it could happen in an arm926ejs just
as well. It could happen on any CPU with distinct, non-consistent I-
and D- caches an enabled I-cache.

I do also understand that the problem only appears for ARMv7 because it
is has enabled icache; I also understand that the *solution* only builds
for ARM.

So my suggestion is to implement the icache_flush in common/bmmt_cmd.c
as follows:

...
/* just about to 'go' */
#if CONFIG_ARM
#if CONFIG_ICACHE
if (icache_status())
	invalidate_icache_all();
#endif /* CONFIG_ICACHE */
#endif /* CONFIG_ARM */
/* now go */

When other arches with the same type of problem want to be in on this,
they can be added in the outer #if. When all arches are ok for this
(possible with default, no-op, versions of icache_status and/or
invalidate_icache_all) then the outer #if/#endif will just go away.

The only problem I see for now is that, while ARM has a default
icache_status(), apparently the targets that need the invalidate above
do not have their own version of icache_status(); they'll need to
provide one for this code to work.

Amicalement,
-- 
Albert.