[U-Boot] [PATCH] arm_cortexa8: support cache flush to other soc

[Dirk Behme <dirk.behme@googlemail.com>]

Kyungmin Park wrote:
> Hi,
> 
> As he goes to home, I reply it instead.

Nice weekend then :)

> On Fri, Sep 4, 2009 at 5:43 PM, Dirk Behme<dirk.behme@googlemail.com> wrote:
>> Dear Minkyu Kang,
>>
>> Minkyu Kang wrote:
>>> Current code is supported only omap3 soc.
>>> this patch will support s5pc1xx(s5pc100 and s5pc110) soc also.
>> Thanks for this patch!
>>
>> How is this patch related to
>>
>> http://lists.denx.de/pipermail/u-boot/2009-August/058492.html
>>
> 
> It's not good idea to move invalidate_cache to omap directory. we need it.

Well, yes and no ;)

Most probably you (== Samsung) can't use the invalidate_dcache version 
we move in above patch to omap directory, because the version we move 
above is OMAP3 specific (it has calls to OMAP3 ROM code). So no, it's 
a good idea to move OMAP3 specific code to omap directory.

But yes, you might need DCache flush (*). So the idea of above patch 
was to have your own (or generic) implementation, but let OMAP3 use 
the custom one where needed.

(*) Do you really need DCache flush? It always was Jean-Christophe's 
argument that U-Boot doesn't use any DCache at all.

>>> Signed-off-by: Minkyu Kang <mk7.kang@samsung.com>
>>> ---
>>>  cpu/arm_cortexa8/cpu.c |   24 +++++++++++-------------
>>>  1 files changed, 11 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/cpu/arm_cortexa8/cpu.c b/cpu/arm_cortexa8/cpu.c
>>> index 5a5981e..3d430b1 100644
>>> --- a/cpu/arm_cortexa8/cpu.c
>>> +++ b/cpu/arm_cortexa8/cpu.c
>>> @@ -35,9 +35,6 @@
>>>  #include <command.h>
>>>  #include <asm/system.h>
>>>  #include <asm/cache.h>
>>> -#ifndef CONFIG_L2_OFF
>>> -#include <asm/arch/sys_proto.h>
>>> -#endif
>>>
>>>  static void cache_flush(void);
>>>
>>> @@ -61,17 +58,18 @@ int cleanup_before_linux(void)
>>>       cache_flush();
>>>
>>>  #ifndef CONFIG_L2_OFF
>>> -     /* turn off L2 cache */
>>> -     l2_cache_disable();
>>> -     /* invalidate L2 cache also */
>>> -     v7_flush_dcache_all(get_device_type());
>>> -#endif
>>> -     i = 0;
>>> -     /* mem barrier to sync up things */
>>> -     asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i));
>>> +     if (get_device_type() != 0xC100) {
>> Hmm, what is this "0xC100" ?
> 
> Now we got two cpu, s5pc100 and s5pc110. In case of s5pc100 we don't
> need to turn off l2 cache. but s5pc110 needs it.
> So first check the device type, actually cpu type. then determine turn
> off l2 cache or not.

"0xC100" is the device type of s5pc100 then? So it should be

if (get_device_type() != S5PC100_DEVICE)

? I hear some people crying "please use macro" ;)

But I don't like this selection here. When we get additional similar 
SoCs, we will end with something like

if (get_device_type() != 0xC100) ||
   (get_device_type() != FOO) ||
   (get_device_type() != BAR))  ||
   ... {

modifying each time cpu/arm_cortexa8/cpu.c.

I would like more that we are able to compile the functionality based 
on the config file we use for compilation. E.g. provide emtpy 
l2_cache_disable(); function for SoCs that don't need it, but have 
functionality behind it where needed.

With above patch, this would then become something like

cpu/arm_cortexa8/s5pcxxx/dcache.S

-> Implements invalidate_dcache() (or implement a Cortex A8 generic 
one in cpu/arm_cortexa8/cache.S)

cpu/arm_cortexa8/s5pcxxx/cache_110.S

-> Implements l2_cache_enable()/disable()

cpu/arm_cortexa8/s5pcxxx/cache_100.S

-> Implements *empty* l2_cache_enable()/disable()

In cpu/arm_cortexa8/s5pcxxx/Makefile you then could have

SOBJS-y += dcache.o
SOBJS-$(CONFIG_S5PC100) += cache_100.o
SOBJS-$(CONFIG_S5PC110) += cache_110.o

What do you think about this?

>>> +             /* turn off L2 cache */
>>> +             l2_cache_disable();
>>> +             /* invalidate L2 cache also */
>>> +             v7_flush_dcache_all(get_device_type());
>>>
>>> -#ifndef CONFIG_L2_OFF
>>> -     l2_cache_enable();
>>> +             i = 0;
>>> +             /* mem barrier to sync up things */
>>> +             asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i));
>>> +
>>> +             l2_cache_enable();
>>> +     }
>>>  #endif
>> What's about the order of #ifndef CONFIG_L2_OFF?
>>
>> While we had before
>>
>>
>> #ifndef CONFIG_L2_OFF
>>        /* turn off L2 cache */
>>        l2_cache_disable();
>>        /* invalidate L2 cache also */
>>        v7_flush_dcache_all(get_device_type());
>> #endif
>>        i = 0;
>>        /* mem barrier to sync up things */
>>        asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i));
>>
>> #ifndef CONFIG_L2_OFF
>>        l2_cache_enable();
>> #endif
>>
>>
>> after this patch we would have
>>
>>
>> #ifndef CONFIG_L2_OFF
>>        if (get_device_type() != 0xC100) {
>>                /* turn off L2 cache */
>>                l2_cache_disable();
>>                /* invalidate L2 cache also */
>>                v7_flush_dcache_all(get_device_type());
>>                i = 0;
>>                /* mem barrier to sync up things */
>>                asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i));
>>
>>                l2_cache_enable();
>>        }
>> #endif
>>
>> Is this intended?
> 
> maybe not. now we only tested the smdkc100 but actual use is internal
> board for s5pc100 & s5pc110.
> He will be modify it.

Ok, thanks.

Best regards

Dirk