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

[Tom Rini]

On Fri, Jan 13, 2012 at 1:26 AM, Sughosh Ganu <urwithsughosh at gmail.com> wrote:
> 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 at 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.

I think while we're in here and noticing these things we should fix
the comments at least.

-- 
Tom