lib_ppc: rework the flush_cache
PATCH 1/1 lib_ppc: rework the flush_cache
- It is possible to miss flush/invalidate the last cache line, we fix it at here.
- add the volatile and memory clobber.
the bugs is pointed by Scott Wood.
--- lib_ppc/cache.c | 36 +++++++++++++++++------------------- 1 files changed, 17 insertions(+), 19 deletions(-)
lib_ppc/cache.c +17 -19
@@ -25,29 +25,27 @@#include <asm/cache.h> #include <watchdog.h> -void flush_cache (ulong start_addr,ulong size) +void flush_cache (ulong start_addr,ulong size) {#ifndef CONFIG_5xx -ulong addr, end_addr = start_addr + size; +ulong addr, start, end; -if (CONFIG_SYS_CACHELINE_SIZE ) { - addr = start_addr & (CONFIG_SYS_CACHELINE_SIZE -1 ); -for (addr = start_addr; - addr < end_addr; - addr +=CONFIG_SYS_CACHELINE_SIZE ) { - asm ("dcbst 0,%0" : :"r" (addr)); -WATCHDOG_RESET (); - } - asm ("sync" );/* Wait for all dcbst to complete on bus */ + start = start_addr & ~(CONFIG_SYS_CACHELINE_SIZE -1 ); + end = (start_addr + size) & ~(CONFIG_SYS_CACHELINE_SIZE -1 ); -for (addr = start_addr; - addr < end_addr; - addr +=CONFIG_SYS_CACHELINE_SIZE ) { - asm ("icbi 0,%0" : :"r" (addr)); -WATCHDOG_RESET (); - } +for (addr = start; addr <= end; addr +=CONFIG_SYS_CACHELINE_SIZE ) { + asmvolatile ("dcbst 0,%0" : :"r" (addr) :"memory" ); +WATCHDOG_RESET (); } - asm ("sync" );/* Always flush prefetch queue in any case */ - asm ("isync" ); +/* wait for all dcbst to complete on bus */ + asmvolatile ("sync" : : :"memory" ); + +for (addr = start; addr <= end; addr +=CONFIG_SYS_CACHELINE_SIZE ) { + asmvolatile ("icbi 0,%0" : :"r" (addr) :"memory" ); +WATCHDOG_RESET (); + } + asmvolatile ("sync" : : :"memory" ); +/* flush prefetch queue */ + asmvolatile ("isync" : : :"memory" );#endif }
Signature · 1 line
1.5.4
PATCH 1/1 lib_ppc: rework the flush_cache
Quoted 2 lines · Dave Liu
- It is possible to miss flush/invalidate the last cache line, we fix it at here.
That comment was on the version you posted in the NAND patch; the lib_ppc version actually looks worse -- it tried to round down to avoid the issue, but it was missing a ~. Thus, it flushed everything from address 0 to the end.
lib_ppc/cache.c line 34
Quoted 2 lines
lib_ppc/cache.c +2
-0+ start = start_addr & ~(CONFIG_SYS_CACHELINE_SIZE -1 ); + end = (start_addr + size) & ~(CONFIG_SYS_CACHELINE_SIZE -1 );
end = start_addr + size - 1;
The rounding is unnecessary for end, and without the - 1, if start_addr
+ size is on a cacheline boundary, you'll flush one cache line too many (which might not be mapped, or might cause end to wrap around to zero if flushing at the end of the address space).
-Scott
PATCH 1/1 lib_ppc: rework the flush_cache
Quoted 6 lines
That comment was on the version you posted in the NAND patch; the
lib_ppcversion actually looks worse -- it tried to round down to avoid
the issue, but it was missing a ~. Thus, it flushed everything from address 0 to the end.
the lib_ppc version basically is as below.
void flush_cache (ulong start_addr, ulong size)
{
ulong addr, end_addr = start_addr + size;
addr = start_addr & (CONFIG_SYS_CACHELINE_SIZE - 1);
for (addr = start_addr; addr < end_addr; addr +=
CONFIG_SYS_CACHELINE_SIZE) {
asm ("dcbst 0,%0": :"r" (addr));
}
}so, you are not completely right, the flush is from start_addr. I believe my commit log also is proper for lib_ppc version.
Quoted 5 lines
lib_ppc/cache.c line 34
Quoted 2 lines
lib_ppc/cache.c +2
-0+ start = start_addr & ~(CONFIG_SYS_CACHELINE_SIZE -1 ); + end = (start_addr + size) & ~(CONFIG_SYS_CACHELINE_SIZE -1 );end =
start_addr+ size - 1;
Quoted 7 lines
The rounding is unnecessary for end, and without the - 1, if
start_addr
+ size is on a cacheline boundary, you'll flush one cache line too many
(which might not be mapped, or might cause end to wrap around to zero if
flushing at the end of the address space).
I don't see what is the problem in my patch at here.
PATCH 1/1 lib_ppc: rework the flush_cache
Dear "Liu Dave",
Quoted 34 lines · In message <79C363B768933F4FB918025FF7EB888856A046@zch01exm21.fsl.freescale.net…
Quoted 6 lines
That comment was on the version you posted in the NAND patch; the
lib_ppcversion actually looks worse -- it tried to round down to avoid
the issue, but it was missing a ~. Thus, it flushed everything from address 0 to the end.the
lib_ppcversion basically is as below.
voidflush_cache(ulongstart_addr, ulong size){ ulong addr, end_addr = start_addr + size; addr = start_addr & (CONFIG_SYS_CACHELINE_SIZE - 1); for (addr = start_addr; addr < end_addr; addr += CONFIG_SYS_CACHELINE_SIZE) { asm ("dcbst 0,%0": :"r" (addr)); } }so, you are not completely right, the flush is from
start_addr. I believe my commit log also is proper forlib_ppcversion.Quoted 5 lines
lib_ppc/cache.c line 34
Quoted 2 lines
lib_ppc/cache.c +2
-0+ start = start_addr & ~(CONFIG_SYS_CACHELINE_SIZE -1 ); + end = (start_addr + size) & ~(CONFIG_SYS_CACHELINE_SIZE -1 );end =
start_addr+ size - 1;Quoted 7 lines
The rounding is unnecessary for end, and without the - 1, if
start_addr
+ size is on a cacheline boundary, you'll flush one cache line too many
(which might not be mapped, or might cause end to wrap around to zero if
flushing at the end of the address space).
Quoted 1 line
I don't see what is the problem in my patch at here.
Scott explained that instead of
end = (start_addr + size) ...
you should use
end = (start_addr + size - 1) ...
(wether or not the rounding makes sense is not really clear to me.
Best regards,
Wolfgang Denk
Signature · 5 lines
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 The nice thing about standards is that there are so many to choose from. - Andrew S. Tanenbaum