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.