[U-Boot] [PATCH v2] ARM926ejs: Add routines to invalidate D-Cache

[Albert ARIBAUD]

Le 05/08/2011 08:38, Hong Xu a écrit :
> Hi Albert,
>
> On 08/05/2011 02:13 PM, Albert ARIBAUD wrote:
>> Hi Hong Xu,
>>
>> Le 05/08/2011 06:44, Hong Xu a écrit :
>>> After DMA operation, we need to maintain D-Cache coherency.
>>> We need to clean cache (write back the dirty lines) and then
>>> make the cache invalidate as well(hence CPU will fetch data
>>> written by DMA controller from RAM).
>>>
>>> Tested on AT91SAM9261EK with Peripheral DMA controller.
>>>
>>> Signed-off-by: Hong Xu<hong.xu at atmel.com>
>>> Tested-by: Elen Song<elen.song at atmel.com>
>>> CC: Albert Aribaud<albert.aribaud at free.fr>
>>> CC: Heiko Schocher<hs at denx.de>
>
> [...]
>
>>> +
>>> +#ifdef CONFIG_SYS_CACHELINE_SIZE
>>> + cache_line_len = CONFIG_SYS_CACHELINE_SIZE;
>>> +#else
>>> + cache_line_len = 32;
>>
>> Document magic number 32 -- for instance, indicate ARM architecture spec
>> paragraph reference in a comment above it (and possibly emit a
>> compile-time and/or run-time warning) -- or bail out with a compile
>> error if CONFIG_SYS_CACHELINE_SIZE is not defined.
>
> ARM926EJ-S Technical Reference Manual (r0p4/r0p5) only defines one cache
> line size, "32". From my understanding, the standard ARM926EJ-S
> implementation shall use 32 bytes cache line size. Of course this can be
> overwritten by CONFIG_SYS_CACHELINE_SIZE, but shall we force to define it?
>
> Frankly I have not found a proper place to define this(aka. not using 32
> here), for example as a Macro.

As I said, I am fine with a simple comment above that '32' that says 
where this value comes from.

>>> +#endif
>>> + /*
>>> + * If start and stop are not aligned to cache-line,
>>> + * the adjacent lines will be cleaned
>>> + */
>>> + if ((start& (cache_line_len - 1)) != 0)
>>> + asm("mcr p15, 0, %0, c7, c10, 1" : : "r" (start));
>>> + if ((stop& (cache_line_len - 1)) != 0)
>>> + asm("mcr p15, 0, %0, c7, c10, 1" : : "r" (stop));
>>> +
>>> + mva = start& ~(cache_line_len - 1);
>>> + while (mva< stop) {
>>> + asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(mva));
>>> + mva += cache_line_len;
>>> + }
>>
>> I, like Reinhard, prefer aligning start and stop and then looping
>> through a single invalidate mcr.
>>
>> But I also want to make sure logs tell us anything weird with caches,
>> and since unaligned start or stop invalidation could lead to trashing
>> third party data, I would like the code to emit a run-time warning if
>> that happens, like this:
>>
>> if ((start& (cache_line_len - 1)) != 0) {
>> printf("WARNING: aligning start %x on start of cache line\n",
>> start);
>> start &= ~(cache_line_len - 1);
>> }
>> if ((stop& (cache_line_len - 1)) != (cache_line_len -1) ) {
>> printf("WARNING: aligning stop %x on end of cache line\n",
>> stop);
>> start |= (cache_line_len - 1);
>> }
>
> I've tried to deal with the case that the (start, stop) is not aligned.
> If mis-align happens, the adjacent lines will be cleaned before
> invalidating. And from my view it's impossible for a driver to always
> pass aligned address to invalidate_dcache_range.

Why would it be impossible? If it is statically allocated it can use 
__attribute__(aligned)) and if it is dynamically aligned, it can be 
over-allocated to make sure it starts and ends at cache boundaries.

> To answer your question in another email
>
>  > How do you know the dirty data should be flushed rather than
> invalidated?
>
> "Dirty" means this cache is changed by CPU but not has not been written
> into memory or WB. If we invalidate it, data will lost. In most cases, I
> do not see a situation why the dirty data shall not be written into memory.

The problem is the cases that fall outside of 'most'. This kind of issue 
tends to have effects that, when unwanted, are extremely difficult to 
link to their cause and makes for long and painful debugging.

> BR,
> Eric

Amicalement,
-- 
Albert.