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

Tom Rini
2012-01-13T14:41:37Z

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