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

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

[Henrik Nordström]

Tom Rini wanted me to post this again. There is no change from previous
version.

I do agree with Wolfgang Denk that this really SHOULD NOT be arch
specific. The only reason why I made this ARMv7 specific is because
blindly calling invalidate_icache_all() from the go command will cause
loud complains on other arches where the icache is not
supported/enabled. The question is what is the correct solution

- Doing it arch specific like this (or other arch aproach?)

- Change invalidate_icache_all() to be silent if the icache is not
enabled/supported?

- Something else?

---

ARM v7 runs with icache enabled. For reliable results the go command
needs to flush the icache before jumping or it may risk running
cached instructions that differ from what currently is in memory.

---
 arch/arm/cpu/armv7/Makefile   |    1 +
 arch/arm/cpu/armv7/cmd_boot.c |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/cmd_boot.c

diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
index 4fdbee4..da1b5e8 100644
--- a/arch/arm/cpu/armv7/Makefile
+++ b/arch/arm/cpu/armv7/Makefile
@@ -31,6 +31,7 @@ COBJS	+= cache_v7.o
 
 COBJS	+= cpu.o
 COBJS	+= syslib.o
+COBJS	+= cmd_boot.o
 
 ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TEGRA20),)
 SOBJS	+= lowlevel_init.o
diff --git a/arch/arm/cpu/armv7/cmd_boot.c b/arch/arm/cpu/armv7/cmd_boot.c
new file mode 100644
index 0000000..6758a55
--- /dev/null
+++ b/arch/arm/cpu/armv7/cmd_boot.c
@@ -0,0 +1,37 @@
+/*
+ * (C) Copyright 2000-2003
+ * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/*
+ * Misc boot support
+ */
+#include <common.h>
+#include <command.h>
+
+#ifdef CONFIG_CMD_GO
+unsigned long do_go_exec(ulong (*entry)(int, char * const []), int argc,
+				 char * const argv[])
+{
+	invalidate_icache_all();
+	return entry(argc, argv);
+}
+#endif
-- 
1.7.7.6

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

[Albert ARIBAUD]

Hi Henrik,

On Tue, 14 May 2013 16:16:02 +0200, Henrik Nordstr?m
<henrik@henriknordstrom.net> wrote:

> Tom Rini wanted me to post this again. There is no change from previous
> version.
> 
> I do agree with Wolfgang Denk that this really SHOULD NOT be arch
> specific. The only reason why I made this ARMv7 specific is because
> blindly calling invalidate_icache_all() from the go command will cause
> loud complains on other arches where the icache is not
> supported/enabled. The question is what is the correct solution
> 
> - Doing it arch specific like this (or other arch aproach?)
> 
> - Change invalidate_icache_all() to be silent if the icache is not
> enabled/supported?
> 
> - Something else?

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?

Also:

> ARM v7 runs with icache enabled. For reliable results the go command
> needs to flush the icache before jumping or it may risk running
> cached instructions that differ from what currently is in memory.

IIUC, the issue is due to cache being enabled when doing the go, rather
than due to armv7 per se.

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.

Amicalement,
-- 
Albert.

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

[Henrik Nordström]

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.

> Also:
> 
> > ARM v7 runs with icache enabled. For reliable results the go command
> > needs to flush the icache before jumping or it may risk running
> > cached instructions that differ from what currently is in memory.
> 
> IIUC, the issue is due to cache being enabled when doing the go, rather
> than due to armv7 per se.

Not entirely sure what you mean.

> 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.

Regards
Henrik

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

[Albert ARIBAUD]

Hi Henrik,

On Wed, 15 May 2013 18:34:07 +0200, Henrik Nordstr?m
<henrik@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*?

> > Also:
> > 
> > > ARM v7 runs with icache enabled. For reliable results the go command
> > > needs to flush the icache before jumping or it may risk running
> > > cached instructions that differ from what currently is in memory.
> > 
> > IIUC, the issue is due to cache being enabled when doing the go, rather
> > than due to armv7 per se.
> 
> Not entirely sure what you mean.

I mean that the problem is having icache and DDR inconsistency at the
time of doing a go, whatever the CPU (or even architecture concerned).
This happens because the icache and DDR are inconsistent, not because
the CPU is an ARMv7. IOW, this could happen on an ARMv5 just as well,
or on any architecture that has separate i an dcache and loads code
in DDR.

> > 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.

> Regards
> Henrik

Amicalement,
-- 
Albert.

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

[Tom Rini]

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@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.

[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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130515/4a9b2582/attachment.pgp>

[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@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@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.

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

[Henrik Nordström]

ons 2013-05-15 klockan 19:39 +0200 skrev Albert ARIBAUD:



> 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).

Yes.

> 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.

Correct on all accounts.

> 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 */

This style is a nightmare for adding more arches needing this, but
solves the problem today.

But there is no CONFIG_ICACHE. In ARM there is CONFIG_SYS_ICACHE_OFF but
it's not quite the same.

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

[Wolfgang Denk]

Dear Henrik Nordstr?m,

In message <1368669278.27007.43.camel@localhost> you wrote:
> 
> > So my suggestion is to implement the icache_flush in common/bmmt_cmd.c
> > as follows:
...
> From what I can tell there is no need to theck icache_status(). It's
> always safe to call invalidate_icache_all().
> 
> So it's back to use the compiler define for ARMv7.

I don't think this would be an acceptable approach.  There are several
serious issues with the suggested patch.


First, I think your solution in not complete.   Invalidating the
instruction cache is only one part of what needs to be done.  You
should also make sure to flush the data cache.

Second, flushing _all_ caches may be a time consuming operation on
some systems, and it is not necessary.  It should be sufficient to
flush the address range where the loaded code resides.

Third, Albert is perfectly right:  there is no reason to make this
architecture specific.  We should NOT add such code here and there on
a per-arch base; this results only in code fragmentation, duplication
and a maintenance nightmare.

Finally, a very similar topic has been discussed here not so long ago;
please see the thread "command/cache: Add flush command" at [1], [2],
and [3]; see especially the attempt of a summary at [4].

[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/156449
[2] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158038
[3] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158101

[4] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158101/focus=158559


In the summary I tried to explain what I think we should do to strive
for more common code; unfortunately did not follow this suggestion but
decided to keep his code out-of-tree.  But I still think this is what
should be done, also in your case.

We should not add architecture-specific code like you suggested.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"A dirty mind is a joy forever."                       - Randy Kunkee

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

[Tom Rini]

On Thu, May 16, 2013 at 09:14:06AM +0200, Wolfgang Denk wrote:
> Dear Henrik Nordstr??m,
> 
> In message <1368669278.27007.43.camel@localhost> you wrote:
> > 
> > > So my suggestion is to implement the icache_flush in common/bmmt_cmd.c
> > > as follows:
> ...
> > From what I can tell there is no need to theck icache_status(). It's
> > always safe to call invalidate_icache_all().
> > 
> > So it's back to use the compiler define for ARMv7.
> 
> I don't think this would be an acceptable approach.  There are several
> serious issues with the suggested patch.
> 
> 
> First, I think your solution in not complete.   Invalidating the
> instruction cache is only one part of what needs to be done.  You
> should also make sure to flush the data cache.
> 
> Second, flushing _all_ caches may be a time consuming operation on
> some systems, and it is not necessary.  It should be sufficient to
> flush the address range where the loaded code resides.
> 
> Third, Albert is perfectly right:  there is no reason to make this
> architecture specific.  We should NOT add such code here and there on
> a per-arch base; this results only in code fragmentation, duplication
> and a maintenance nightmare.
> 
> Finally, a very similar topic has been discussed here not so long ago;
> please see the thread "command/cache: Add flush command" at [1], [2],
> and [3]; see especially the attempt of a summary at [4].

That this topic keeps coming up is one of the reasons I asked Henrik to
post this patch when I was looking over the Allwinner support queue.  I
thought this was a rather clever fixup.

> [1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/156449
> [2] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158038
> [3] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158101
> 
> [4] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158101/focus=158559
> 
> 
> In the summary I tried to explain what I think we should do to strive
> for more common code; unfortunately did not follow this suggestion but
> decided to keep his code out-of-tree.  But I still think this is what
> should be done, also in your case.
> 
> We should not add architecture-specific code like you suggested.

The problem is that with go we can't know 'size' for go because it's
just a binary blob, so we need, roughly, flush_cache(gd->ram_top -
gd->reloc_off, gd->ram_size) yes?  That I believe is one of the points
that wasn't solved in the last go-round here.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130516/7a987476/attachment.pgp>

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

[Henrik Nordström]

tor 2013-05-16 klockan 09:37 -0400 skrev Tom Rini:

> That this topic keeps coming up is one of the reasons I asked Henrik to
> post this patch when I was looking over the Allwinner support queue.  I
> thought this was a rather clever fixup.

For what it's worth a similar issue is also relevant to bootm, but it's
a bit different there trying to disable the icache entirely.

I do not like having these things arch specific. I implemented it as an
ARMv7 hook only because there is no general cross-platform u-boot
function for clearing the icache. I'd much rather have a generic
function call for invalidating the cache and leave it up to the arches
to get it implemented where needed. 

icache_invalidate_all() is close to that call, except that it's only
reasonably possible to use it on ARMv7 currently. Trying to use it
anywhere else will give confusing (but correct) and unneeded errors on
the console, if it at all builds (requires CONFIG_CACHE).

Cluttering the places that need the flush with ARCH specific details
such as defines is generally not very maintainable. I'd rather have
those in the appropriate header.

> The problem is that with go we can't know 'size' for go because it's
> just a binary blob, so we need, roughly, flush_cache(gd->ram_top -
> gd->reloc_off, gd->ram_size) yes?  That I believe is one of the points
> that wasn't solved in the last go-round here.

Yes.

For this approach the icache flushes need to go into the places where
data are loaded into memory.  But that would be even worse performance
killer.

Or maybe just punt it. If you are on an arch with incoherent caches then
make sure to make use of the cache command to flush caches and maybe
even disable caches before using go.

Regards
Henrik

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

[Wolfgang Denk]

Dear Henrik,

In message <1368718669.25965.14.camel@localhost> you wrote:
> 
> I do not like having these things arch specific. I implemented it as an
> ARMv7 hook only because there is no general cross-platform u-boot
> function for clearing the icache. I'd much rather have a generic
> function call for invalidating the cache and leave it up to the arches
> to get it implemented where needed. 

There is a common, architecture-independent C API that implements
cache flushing/invalidation; please re-read the summary at [1]

[1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/158101/focus=158559

> Or maybe just punt it. If you are on an arch with incoherent caches then
> make sure to make use of the cache command to flush caches and maybe
> even disable caches before using go.

The cache commands currently do not support subcommands for flushing
/ invalidating the caches.  See again [1].

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Roses are red
Violets are blue
Some poems rhyme

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

[Henrik Nordström]

fre 2013-05-17 klockan 00:13 +0200 skrev Wolfgang Denk:

> There is a common, architecture-independent C API that implements
> cache flushing/invalidation; please re-read the summary at [1]

Sorry I missed that discussion. Had a bit too much mail for a while.

> > Or maybe just punt it. If you are on an arch with incoherent caches then
> > make sure to make use of the cache command to flush caches and maybe
> > even disable caches before using go.
> 
> The cache commands currently do not support subcommands for flushing
> / invalidating the caches.  See again [1].

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

[Kuo-Jung Su]

2013/5/17 Henrik Nordstr?m <henrik@henriknordstrom.net>:
> fre 2013-05-17 klockan 00:13 +0200 skrev Wolfgang Denk:
>
>> There is a common, architecture-independent C API that implements
>> cache flushing/invalidation; please re-read the summary at [1]
>
> Sorry I missed that discussion. Had a bit too much mail for a while.
>
>> > Or maybe just punt it. If you are on an arch with incoherent caches then
>> > make sure to make use of the cache command to flush caches and maybe
>> > even disable caches before using go.
>>
>> The cache commands currently do not support subcommands for flushing
>> / invalidating the caches.  See again [1].
>
> From what I see the cache commands do have a sub commands for flushing.
>
> "icache flush" calls icache_invalidate_all().
>
> "dcache flush" calls flush_dcache_all().
>
> but imho the user shouldn't really need to care for these and is why I
> hooked into the go command.
>
> I don't see much of a performance problem with a automatic full cache
> flush when using the go command on platforms with incoherent caches. In
> most use cases it's not very different from the bootm command which also
> needs to do the same (and using a similar hooking mechanism as what I
> used for go, but not saying that is a good thing)
>
> Platforms with coherent caches likely do not need to care and can keep
> the cache as-is.
>
> And I do not think there needs to be commands for flushing specific
> regions other than for testing. Region based flushing is better done by
> code which knows what is needed. It's hard for users to get this right
> as most times it works anyway.
>
> Regards
> Henrik
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot


How about making the weak aliased arch_preboot_os() global, and then
get it invoked
in both bootm & go?
It looks much pretty to me, and we don't even worry about the i-cache issues.


--
Best wishes,
Kuo-Jung Su

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

[Wolfgang Denk]

Dear Kuo-Jung Su,

In message <CAK65tU4sGY638wWY88Nv_pQj4Edu1L1c7c3Qarunh0Duz7_tjA@mail.gmail.com> you wrote:
>
> How about making the weak aliased arch_preboot_os() global, and then
> get it invoked
> in both bootm & go?
> It looks much pretty to me, and we don't even worry about the i-cache issues.

That would not really help as it would be architecure specific, so it
does not really solve the problem buty instead create new ones like
incompatible code & behaviour.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Include the success of others in your dreams for your own success.

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

[Wolfgang Denk]

Dear Henrik Nordstr?m,

In message <1368792981.765.21.camel@localhost> you wrote:
> 
> > There is a common, architecture-independent C API that implements
> > cache flushing/invalidation; please re-read the summary at [1]
> 
> Sorry I missed that discussion. Had a bit too much mail for a while.

No problem; that's why I pointed you to the sumnmary (and the rest of
the thread).

> > The cache commands currently do not support subcommands for flushing
> > / invalidating the caches.  See again [1].
> 
> From what I see the cache commands do have a sub commands for flushing.
> 
> "icache flush" calls icache_invalidate_all().
> 
> "dcache flush" calls flush_dcache_all().

Indeed. I was not aware of this :-(

> but imho the user shouldn't really need to care for these and is why I
> hooked into the go command.

In this case you should use the common C API.

> I don't see much of a performance problem with a automatic full cache
> flush when using the go command on platforms with incoherent caches. In
> most use cases it's not very different from the bootm command which also
> needs to do the same (and using a similar hooking mechanism as what I
> used for go, but not saying that is a good thing)

No.  bootm  knows exactly the location and size of the image, so it is
sufficient to flush / invalidate a part of the memory, usually a
pretty small one compared to the total RAM size.

> And I do not think there needs to be commands for flushing specific
> regions other than for testing. Region based flushing is better done by
> code which knows what is needed. It's hard for users to get this right
> as most times it works anyway.

The code should know which ranges need flushing, not the user.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Now here's something you're really going to like!"
- Rocket J. Squirrel

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

[Henrik Nordström]

tis 2013-05-21 klockan 14:26 +0200 skrev Wolfgang Denk:

> > but imho the user shouldn't really need to care for these and is why I
> > hooked into the go command.
> 
> In this case you should use the common C API.

Unfortunately the go command do now know what range(s) it needs to
flush.

> No.  bootm  knows exactly the location and size of the image, so it is
> sufficient to flush / invalidate a part of the memory, usually a
> pretty small one compared to the total RAM size.

See arch/arm/cpu/armv7/cpu.c cleaup_before_linux() for what I am talking
about regarding bootm.

> > And I do not think there needs to be commands for flushing specific
> > regions other than for testing. Region based flushing is better done by
> > code which knows what is needed. It's hard for users to get this right
> > as most times it works anyway.
> 
> The code should know which ranges need flushing, not the user.

Agreed. But I don't see how this can be done for go.

Should we instrument all the load commands to remember which ranges
something have been loaded into since last flush?

Regards
Henrik

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

[Kees Jongenburger]

Hello,
On Thu, May 16, 2013 at 5:37 PM, Henrik Nordstr?m
<henrik@henriknordstrom.net> wrote:
> Or maybe just punt it. If you are on an arch with incoherent caches then
> make sure to make use of the cache command to flush caches and maybe
> even disable caches before using go.

This is indeed the behaviour one would expect from a bootloader. To my
understanding also enabling d-cache on ARM has no effect as long as
the MMU is not turned on so I totally miss the point.

Greetings

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

[Albert ARIBAUD]

Hi Kees,

On Tue, 21 May 2013 14:38:01 +0200, Kees Jongenburger
<kees.jongenburger@gmail.com> wrote:

> To my
> understanding also enabling d-cache on ARM has no effect as long as
> the MMU is not turned on so I totally miss the point.

Enabling dcache gives DDR access performance benefits regardless of
enabling MMU.

> Greetings

Amicalement,
-- 
Albert.