[U-Boot] [PATCH] 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.
Signed-off-by: Dave Liu daveliu@freescale.com --- lib_ppc/cache.c | 36 +++++++++++++++++------------------- 1 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/lib_ppc/cache.c b/lib_ppc/cache.c index 72c838e..dd4eb22 100644 --- a/lib_ppc/cache.c +++ b/lib_ppc/cache.c @@ -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) { + asm volatile("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 */ + asm volatile("sync" : : : "memory"); + + for (addr = start; addr <= end; addr += CONFIG_SYS_CACHELINE_SIZE) { + asm volatile("icbi 0,%0" : : "r" (addr) : "memory"); + WATCHDOG_RESET(); + } + asm volatile("sync" : : : "memory"); + /* flush prefetch queue */ + asm volatile("isync" : : : "memory"); #endif }
Dave Liu wrote:
- 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.
- 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
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.
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.
- 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).
I don't see what is the problem in my patch at here.
Dear "Liu Dave",
In message 79C363B768933F4FB918025FF7EB888856A046@zch01exm21.fsl.freescale.net you wrote:
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.
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.
- 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).
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
participants (4)
-
Dave Liu -
Liu Dave
-
Scott Wood -
Wolfgang Denk