[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu <urwithsughosh@gmail.com>]

hi Christian,

On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
> Hi Sughosh,
> I had a look at the patch and I tried to understand what's going on
> here (I must confess that I didn't know anything about this cache
> stuff).

  Ok, thanks for taking time off to understand this issue.

> 
> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> > The current implementation invalidates the cache instead of flushing
> > it. This causes problems on platforms where the spl/u-boot is already
> > loaded to the RAM, with caches enabled by a first stage bootloader.
> >
> > The V bit of the cp15's control register c1 is set to the value of
> > VINITHI on reset. Do not clear this bit by default, as there are SOC's
> > with no valid memory region at 0x0.
> [...]
> > diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
> > index 6a09c02..6e261c2 100644
> > --- a/arch/arm/cpu/arm926ejs/start.S
> > +++ b/arch/arm/cpu/arm926ejs/start.S
> > @@ -355,17 +355,20 @@ _dynsym_start_ofs:
> > ?*/
> > ?cpu_init_crit:
> > ? ? ? ?/*
> > - ? ? ? ?* flush v4 I/D caches
> > + ? ? ? ?* flush D cache before disabling it
> > ? ? ? ? */
> > ? ? ? ?mov ? ? r0, #0
> > - ? ? ? mcr ? ? p15, 0, r0, c7, c7, 0 ? /* flush v3/v4 cache */
> > +flush_dcache:
> > + ? ? ? mrc ? ? p15, 0, r15, c7, c10, 3
> > + ? ? ? bne ? ? flush_dcache
> > +
> 
> Ok, instead of invalidating the D-cache you are cleaning it. From the
> ARM926EJ-S Technical Reference Manual [1]: "To guarantee that memory
> coherency is maintained, the DCache must be cleaned of dirty data
> before it is disabled." So since we are disabling D-Cache a few lines
> later (bic r0, r0, #0x00000087), this must be the right way to do it.

  Right, so the problem here is that instead of flushing the cache,
  the code currently invalidates it. This would not be a problem on
  platforms where cache is not turned ON(which would be the majority
  of cases), but on my board, it seems to be turned ON by the rbl,
  due to which it does not boot.

  If you check the manual, the loop that i have introduced is one for
  "Testing and Cleaning", which means that in case the lines are not
  dirty, it won't be flushed. So this should not break any platforms
  where the cache isn't enabled.

> 
> > ? ? ? ?mcr ? ? p15, 0, r0, c8, c7, 0 ? /* flush v4 TLB */
> >
> > ? ? ? ?/*
> > ? ? ? ? * disable MMU stuff and caches
> > ? ? ? ? */
> > ? ? ? ?mrc ? ? p15, 0, r0, c1, c0, 0
> > - ? ? ? bic ? ? r0, r0, #0x00002300 ? ? /* clear bits 13, 9:8 (--V- --RS) */
> > + ? ? ? bic ? ? r0, r0, #0x00000300 ? ? /* clear bits 9:8 ( --RS) */
> 
> Ok, I read your comment above.
> 
> > ? ? ? ?bic ? ? r0, r0, #0x00000087 ? ? /* clear bits 7, 2:0 (B--- -CAM) */
> > ? ? ? ?orr ? ? r0, r0, #0x00000002 ? ? /* set bit 2 (A) Align */
> > ? ? ? ?orr ? ? r0, r0, #0x00001000 ? ? /* set bit 12 (I) I-Cache */
> 
> Although this is not changed in your patch, the last line makes me
> wonder. The comment says "disable MMU stuff and cached", but actually
> the last line sets bit 12 (I), which means that I-Cache gets enabled
> according to [1].

  Yeah, this seems to be copied code, with discrepancies in the code
  and comments. You would see that the line i have removed has a
  comment for flushing the cache, but instead it is invalidating the
  cache. I have just fixed the comments for the lines that i made
  changes to.

-sughosh