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

Wolfgang Denk
2008-12-14T11:48:30Z

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