[U-Boot] [PATCH] lib_ppc: rework the flush_cache

[U-Boot] [PATCH] lib_ppc: rework the flush_cache

[Dave Liu]

- 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
 }
-- 
1.5.4

[U-Boot] [PATCH] lib_ppc: rework the flush_cache

[Scott Wood]

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

[U-Boot] [PATCH] lib_ppc: rework the flush_cache

[Liu Dave]

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

[U-Boot] [PATCH] lib_ppc: rework the flush_cache

[Scott Wood]

Liu Dave wrote:
> 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.

Ah, right, the first addr assignment is just dead code.

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

start_addr = 0
size = 0x1000

start will be 0
end will be 0x1000

The loop will flush the cache line at 0x1000, because it uses <= rather 
than <.  That is outside the area that was requested to be flushed. 
Maybe it's not mapped.  Or, maybe start + size = 0 and nothing gets flushed.

-Scott

[U-Boot] [PATCH] lib_ppc: rework the flush_cache

[Liu Dave]

> start_addr = 0
> size = 0x1000
> 
> start will be 0
> end will be 0x1000
> 
> The loop will flush the cache line at 0x1000, because it uses 
> <= rather than <.  That is outside the area that was requested to be
flushed. 
> Maybe it's not mapped.  Or, maybe start + size = 0 and > nothing gets
flushed.

thanks, I got it.
it was off-one error.

[U-Boot] [PATCH] lib_ppc: rework the flush_cache

[Wolfgang Denk]

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

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