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

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

[Hong Xu]

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@atmel.com>
Tested-by: Elen Song <elen.song@atmel.com>
CC: Albert Aribaud <albert.aribaud@free.fr>
CC: Heiko Schocher <hs@denx.de>
---
Changes since v1
~ Per Albert's suggestion, add invalidate_dcache_range originally defined
  in include/common.h

 arch/arm/lib/cache.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index 92b61a2..0436443 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -53,3 +53,49 @@ void	__flush_dcache_all(void)
 }
 void	flush_dcache_all(void)
 	__attribute__((weak, alias("__flush_dcache_all")));
+
+void __invalidate_dcache_range(unsigned long start, unsigned long stop)
+{
+	int cache_line_len;
+	unsigned long mva;
+
+#ifdef CONFIG_ARM926EJS
+
+#ifdef CONFIG_SYS_CACHELINE_SIZE
+	cache_line_len = CONFIG_SYS_CACHELINE_SIZE;
+#else
+	cache_line_len = 32;
+#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;
+	}
+
+	/* Drain the WB */
+	asm("mcr p15, 0, %0, c7, c10, 4" : : "r" (0));
+#endif
+
+	return;
+}
+void invalidate_dcache_range(unsigned long start, unsigned long stop)
+	__attribute__((weak, alias("__invalidate_dcache_range")));
+
+void __invalidate_dcache_all(void)
+{
+#ifdef CONFIG_ARM926EJS
+	asm("mcr p15, 0, %0, c7, c6, 0" : : "r" (0));
+#endif
+	return;
+}
+void  invalidate_dcache_all(void)
+	__attribute__((weak, alias("__invalidate_dcache_all")));
-- 
1.7.3.3

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

[Reinhard Meyer]

Dear Hong Xu,
> 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@atmel.com>
> Tested-by: Elen Song<elen.song@atmel.com>
> CC: Albert Aribaud<albert.aribaud@free.fr>
> CC: Heiko Schocher<hs@denx.de>
> ---
> Changes since v1
> ~ Per Albert's suggestion, add invalidate_dcache_range originally defined
>    in include/common.h
>
>   arch/arm/lib/cache.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> index 92b61a2..0436443 100644
> --- a/arch/arm/lib/cache.c
> +++ b/arch/arm/lib/cache.c
> @@ -53,3 +53,49 @@ void	__flush_dcache_all(void)
>   }
>   void	flush_dcache_all(void)
>   	__attribute__((weak, alias("__flush_dcache_all")));
> +
> +void __invalidate_dcache_range(unsigned long start, unsigned long stop)
> +{
> +	int cache_line_len;
> +	unsigned long mva;
> +
> +#ifdef CONFIG_ARM926EJS
> +
> +#ifdef CONFIG_SYS_CACHELINE_SIZE
> +	cache_line_len = CONFIG_SYS_CACHELINE_SIZE;
> +#else
> +	cache_line_len = 32;
> +#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));

Why so complicated?
How about:
	/* round down to the start of the cache line */
	start &= (cache_line_len - 1);
	/* round up to the end of the cache line */
note: if, what I assume, the range to be invalidated is
[start, stop) - that means stop is the first address not to be
invalidated, the next statement is not necessary
	stop |= (cache_line_len - 1);
	while (start < stop) {
		asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(start));
		start += cache_line_len;
	}
	/* Drain the WB */
	asm("mcr p15, 0, %0, c7, c10, 4" : : "r" (0));
> +#endif

Best Regards,
Reinhard

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

[Hong Xu]

H Reinhard,

On 08/05/2011 01:11 PM, Reinhard Meyer wrote:
> Dear Hong Xu,
>> 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@atmel.com>
>> Tested-by: Elen Song<elen.song@atmel.com>
>> CC: Albert Aribaud<albert.aribaud@free.fr>
>> CC: Heiko Schocher<hs@denx.de>
>> ---
>> Changes since v1
>> ~ Per Albert's suggestion, add invalidate_dcache_range originally defined
>> in include/common.h
>>
>> arch/arm/lib/cache.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 46 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
>> index 92b61a2..0436443 100644
>> --- a/arch/arm/lib/cache.c
>> +++ b/arch/arm/lib/cache.c
>> @@ -53,3 +53,49 @@ void __flush_dcache_all(void)
>> }
>> void flush_dcache_all(void)
>> __attribute__((weak, alias("__flush_dcache_all")));
>> +
>> +void __invalidate_dcache_range(unsigned long start, unsigned long stop)
>> +{
>> + int cache_line_len;
>> + unsigned long mva;
>> +
>> +#ifdef CONFIG_ARM926EJS
>> +
>> +#ifdef CONFIG_SYS_CACHELINE_SIZE
>> + cache_line_len = CONFIG_SYS_CACHELINE_SIZE;
>> +#else
>> + cache_line_len = 32;
>> +#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));
>
> Why so complicated?

In case of mis-aligned (start, stop), this is to clean the cache line, 
aka. write back the potential dirty lines before successive invalidating

BR,
Eric

> How about:
> /* round down to the start of the cache line */
> start &= (cache_line_len - 1);
> /* round up to the end of the cache line */
> note: if, what I assume, the range to be invalidated is
> [start, stop) - that means stop is the first address not to be
> invalidated, the next statement is not necessary
> stop |= (cache_line_len - 1);
> while (start < stop) {
> asm("mcr p15, 0, %0, c7, c6, 1" : : "r"(start));
> start += cache_line_len;
> }
> /* Drain the WB */
> asm("mcr p15, 0, %0, c7, c10, 4" : : "r" (0));
>> +#endif
>
> Best Regards,
> Reinhard

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

[Albert ARIBAUD]

Hi Hong Xu,

Le 05/08/2011 08:17, Hong Xu a ?crit :

>> Why so complicated?
>
> In case of mis-aligned (start, stop), this is to clean the cache line,
> aka. write back the potential dirty lines before successive invalidating

How do you know the dirty data should be flushed rather than invalidated?

Amicalement,
-- 
Albert.

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

[Albert ARIBAUD]

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@atmel.com>
> Tested-by: Elen Song<elen.song@atmel.com>
> CC: Albert Aribaud<albert.aribaud@free.fr>
> CC: Heiko Schocher<hs@denx.de>
> ---
> Changes since v1
> ~ Per Albert's suggestion, add invalidate_dcache_range originally defined
>    in include/common.h
>
>   arch/arm/lib/cache.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> index 92b61a2..0436443 100644
> --- a/arch/arm/lib/cache.c
> +++ b/arch/arm/lib/cache.c
> @@ -53,3 +53,49 @@ void	__flush_dcache_all(void)
>   }
>   void	flush_dcache_all(void)
>   	__attribute__((weak, alias("__flush_dcache_all")));
> +
> +void __invalidate_dcache_range(unsigned long start, unsigned long stop)
> +{
> +	int cache_line_len;
> +	unsigned long mva;
> +
> +#ifdef CONFIG_ARM926EJS
> +
> +#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.

> +#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);
   }

That will guarantee that unaligned cache invalidates will be detectable, 
and/or that will entice developers to align their starts and stops.

> +	/* Drain the WB */
> +	asm("mcr p15, 0, %0, c7, c10, 4" : : "r" (0));
> +#endif
> +
> +	return;
> +}
> +void invalidate_dcache_range(unsigned long start, unsigned long stop)
> +	__attribute__((weak, alias("__invalidate_dcache_range")));
> +
> +void __invalidate_dcache_all(void)
> +{
> +#ifdef CONFIG_ARM926EJS
> +	asm("mcr p15, 0, %0, c7, c6, 0" : : "r" (0));
> +#endif
> +	return;
> +}
> +void  invalidate_dcache_all(void)
> +	__attribute__((weak, alias("__invalidate_dcache_all")));

Amicalement,
-- 
Albert.

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

[Hong Xu]

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@atmel.com>
>> Tested-by: Elen Song<elen.song@atmel.com>
>> CC: Albert Aribaud<albert.aribaud@free.fr>
>> CC: Heiko Schocher<hs@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.

>
>> +#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.

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.

BR,
Eric
>
[...]
> Amicalement,

[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@atmel.com>
>>> Tested-by: Elen Song<elen.song@atmel.com>
>>> CC: Albert Aribaud<albert.aribaud@free.fr>
>>> CC: Heiko Schocher<hs@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.

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

[Hong Xu]

On 08/05/2011 02:46 PM, Albert ARIBAUD wrote:
> 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@atmel.com>
>>>> Tested-by: Elen Song<elen.song@atmel.com>
>>>> CC: Albert Aribaud<albert.aribaud@free.fr>
>>>> CC: Heiko Schocher<hs@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.
>

OK

>>>> +#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.

Technically it's possible, as you mentioned. But shall we add such 
restriction? Does this API initially have this alignment primitive?

>> 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.

Well, "most" here may be a little bit confusing. Assuming a 
write-through cache, how can you write data to memory and then regret? I 
think you have to undo explicitly.

How about keeping cleaning the cache when mis-aligned happens and print 
a WARNING?

BR,
Eric

>> BR,
>> Eric
>
> Amicalement,

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

[Aneesh V]

Hi Hong, Albert,

On Friday 05 August 2011 12:16 PM, Albert ARIBAUD wrote:
> Le 05/08/2011 08:38, Hong Xu a ?crit :
>> Hi Albert,
>>
>> 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.
>

IMHO, Hong's approach is correct. If the buffer that is invalidated is
not aligned to cache-line, one cache-line at the respective boundary
may have to be flushed to make sure the invalidation doesn't affect
somebody else's memory.

The solution is for drivers to ensure that any buffer that needs to be
invalidated is aligned to cache-line boundary at both ends. The above
approach puts this onus on the driver. I have documented the alignment
requirement in my recent patch series for fixing arm cache problems.

best regards,
Aneesh

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

[Albert ARIBAUD]

Hi Aneesh,

On 05/08/2011 09:10, Aneesh V wrote:
> Hi Hong, Albert,
>
> On Friday 05 August 2011 12:16 PM, Albert ARIBAUD wrote:
>> Le 05/08/2011 08:38, Hong Xu a ?crit :
>>> Hi Albert,
>>>
>>> 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.
>>
>
> IMHO, Hong's approach is correct. If the buffer that is invalidated is
> not aligned to cache-line, one cache-line at the respective boundary
> may have to be flushed to make sure the invalidation doesn't affect
> somebody else's memory.
>
> The solution is for drivers to ensure that any buffer that needs to be
> invalidated is aligned to cache-line boundary at both ends. The above
> approach puts this onus on the driver. I have documented the alignment
> requirement in my recent patch series for fixing arm cache problems.

AIUI, you describe what I requested, so I agree with it. :)

And if there is an alignment requirement, then there is no need any more 
to flush lines when unaligned start/stop is passed, because it would 
only have a use for call cases that do not conform to the requirement.

> best regards,
> Aneesh

Amicalement,
-- 
Albert.

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

[Aneesh V]

Hi Albert,

On Friday 05 August 2011 02:50 PM, Albert ARIBAUD wrote:
> Hi Aneesh,
>
> On 05/08/2011 09:10, Aneesh V wrote:
>> Hi Hong, Albert,
>>
>> On Friday 05 August 2011 12:16 PM, Albert ARIBAUD wrote:
>>> Le 05/08/2011 08:38, Hong Xu a ?crit :
>>>> Hi Albert,
>>>>
>>>> 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.
>>>
>>
>> IMHO, Hong's approach is correct. If the buffer that is invalidated is
>> not aligned to cache-line, one cache-line at the respective boundary
>> may have to be flushed to make sure the invalidation doesn't affect
>> somebody else's memory.
>>
>> The solution is for drivers to ensure that any buffer that needs to be
>> invalidated is aligned to cache-line boundary at both ends. The above
>> approach puts this onus on the driver. I have documented the alignment
>> requirement in my recent patch series for fixing arm cache problems.
>
> AIUI, you describe what I requested, so I agree with it. :)
>
> And if there is an alignment requirement, then there is no need any more
> to flush lines when unaligned start/stop is passed, because it would
> only have a use for call cases that do not conform to the requirement.

Yes, but when the alignment requirement is not met by a driver that
does invalidation:

1. Flushing will affect the driver itself.

2. Not flushing, may affect some other code for no fault of theirs.

(1) is clearly better in my opinion as it puts the penalty back on
the offending driver and not on some other code.

So, the flushing solution is a good way to make the drivers behave!

best regards,
Aneesh

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

[Hong Xu]

Hi Aneesh,

On 08/05/2011 03:10 PM, Aneesh V wrote:
> Hi Hong, Albert,
>
> On Friday 05 August 2011 12:16 PM, Albert ARIBAUD wrote:
>> Le 05/08/2011 08:38, Hong Xu a ?crit :
>>> Hi Albert,
>>>
>>> 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.
>>
>
> IMHO, Hong's approach is correct. If the buffer that is invalidated is
> not aligned to cache-line, one cache-line at the respective boundary
> may have to be flushed to make sure the invalidation doesn't affect
> somebody else's memory.
>
> The solution is for drivers to ensure that any buffer that needs to be
> invalidated is aligned to cache-line boundary at both ends. The above
> approach puts this onus on the driver. I have documented the alignment
> requirement in my recent patch series for fixing arm cache problems.

I have not noticed the patch series. ;-)
If we put the alignment burden to the driver, I'm afraid many drivers 
which make use of something like a DMA controller have to modify the 
code heavily. This sounds not good. :)

BR,
Eric

> best regards,
> Aneesh

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

[Aneesh V]

Hi Eric,

On Friday 05 August 2011 04:03 PM, Hong Xu wrote:
> Hi Aneesh,
[snip ..]
>>
>> IMHO, Hong's approach is correct. If the buffer that is invalidated is
>> not aligned to cache-line, one cache-line at the respective boundary
>> may have to be flushed to make sure the invalidation doesn't affect
>> somebody else's memory.
>>
>> The solution is for drivers to ensure that any buffer that needs to be
>> invalidated is aligned to cache-line boundary at both ends. The above
>> approach puts this onus on the driver. I have documented the alignment
>> requirement in my recent patch series for fixing arm cache problems.
>
> I have not noticed the patch series. ;-)
> If we put the alignment burden to the driver, I'm afraid many drivers
> which make use of something like a DMA controller have to modify the
> code heavily. This sounds not good. :)

We have a fundamental problem when it comes to invalidating an
un-aligned buffer. Either you flush the boundary lines and corrupt your
buffer at boundaries OR you invalidate without flushing and corrupt
memory around your buffer. Both are not good! The only real solution is
to have aligned buffers, if you want to have D-cache enabled and do DMA
at the same time.

br,
Aneesh

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

[Albert ARIBAUD]

Hi Aneesh,

On 05/08/2011 12:47, Aneesh V wrote:
> Hi Eric,
>
> On Friday 05 August 2011 04:03 PM, Hong Xu wrote:
>> Hi Aneesh,
> [snip ..]
>>>
>>> IMHO, Hong's approach is correct. If the buffer that is invalidated is
>>> not aligned to cache-line, one cache-line at the respective boundary
>>> may have to be flushed to make sure the invalidation doesn't affect
>>> somebody else's memory.
>>>
>>> The solution is for drivers to ensure that any buffer that needs to be
>>> invalidated is aligned to cache-line boundary at both ends. The above
>>> approach puts this onus on the driver. I have documented the alignment
>>> requirement in my recent patch series for fixing arm cache problems.
>>
>> I have not noticed the patch series. ;-)
>> If we put the alignment burden to the driver, I'm afraid many drivers
>> which make use of something like a DMA controller have to modify the
>> code heavily. This sounds not good. :)
>
> We have a fundamental problem when it comes to invalidating an
> un-aligned buffer. Either you flush the boundary lines and corrupt your
> buffer at boundaries OR you invalidate without flushing and corrupt
> memory around your buffer. Both are not good! The only real solution is
> to have aligned buffers, if you want to have D-cache enabled and do DMA
> at the same time.

Plus, there should not be *heavy* modifications; DMA engines tend to use 
essentially two types of memory-resident objects: data buffers and 
buffer descriptors. There's only a small handful of places in the driver 
code to look at to find where these objects are allocated and how.

So I stand by my opinion: since the cache invalidation routine should 
only be called with cache-aligned objects, there is no requirement to 
flush the first (resp. last) cache line in case of unaligned start 
(resp.stop), and I don't want cache operations performed when they are 
not required.

> br,
> Aneesh

Amicalement,
-- 
Albert.

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

[Reinhard Meyer]

Dear Albert, Aneesh, Eric,
> > We have a fundamental problem when it comes to invalidating an
> > un-aligned buffer. Either you flush the boundary lines and corrupt your
> > buffer at boundaries OR you invalidate without flushing and corrupt
> > memory around your buffer. Both are not good! The only real solution is
> > to have aligned buffers, if you want to have D-cache enabled and do DMA
> > at the same time.
> 
> Plus, there should not be *heavy* modifications; DMA engines tend to use 
> essentially two types of memory-resident objects: data buffers and 
> buffer descriptors. There's only a small handful of places in the driver 
> code to look at to find where these objects are allocated and how.
> 
> So I stand by my opinion: since the cache invalidation routine should 
> only be called with cache-aligned objects, there is no requirement to 
> flush the first (resp. last) cache line in case of unaligned start 
> (resp.stop), and I don't want cache operations performed when they are 
> not required.

After considering all issues, any driver that does flush OR invalidate a
cache line that it does not fully "own" is prone to cause problems.

At flushing: some DMA might just have put data into the partial line.
At invalidating: some Software might have put data, but the writeback
had not occured.

So both flush AND invalidate functions should check for this event and
emit a proper warning on the console.

My 2.7 cents...
Reinhard

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

[Albert ARIBAUD]

Hi Reinhard,

On 05/08/2011 13:23, Reinhard Meyer wrote:
> Dear Albert, Aneesh, Eric,
>>> We have a fundamental problem when it comes to invalidating an
>>> un-aligned buffer. Either you flush the boundary lines and corrupt your
>>> buffer at boundaries OR you invalidate without flushing and corrupt
>>> memory around your buffer. Both are not good! The only real solution is
>>> to have aligned buffers, if you want to have D-cache enabled and do DMA
>>> at the same time.
>>
>> Plus, there should not be *heavy* modifications; DMA engines tend to use
>> essentially two types of memory-resident objects: data buffers and
>> buffer descriptors. There's only a small handful of places in the driver
>> code to look at to find where these objects are allocated and how.
>>
>> So I stand by my opinion: since the cache invalidation routine should
>> only be called with cache-aligned objects, there is no requirement to
>> flush the first (resp. last) cache line in case of unaligned start
>> (resp.stop), and I don't want cache operations performed when they are
>> not required.
>
> After considering all issues, any driver that does flush OR invalidate a
> cache line that it does not fully "own" is prone to cause problems.
>
> At flushing: some DMA might just have put data into the partial line.
> At invalidating: some Software might have put data, but the writeback
> had not occured.
>
> So both flush AND invalidate functions should check for this event and
> emit a proper warning on the console.

Fully agreed.

> My 2.7 cents...
> Reinhard

Amicalement,
-- 
Albert.

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

[Aneesh V]

Hi Albert,

On Friday 05 August 2011 04:33 PM, Albert ARIBAUD wrote:
> Hi Aneesh,
>
> On 05/08/2011 12:47, Aneesh V wrote:
>> Hi Eric,
>>
>> On Friday 05 August 2011 04:03 PM, Hong Xu wrote:
>>> Hi Aneesh,
>> [snip ..]
>>>>
>>>> IMHO, Hong's approach is correct. If the buffer that is invalidated is
>>>> not aligned to cache-line, one cache-line at the respective boundary
>>>> may have to be flushed to make sure the invalidation doesn't affect
>>>> somebody else's memory.
>>>>
>>>> The solution is for drivers to ensure that any buffer that needs to be
>>>> invalidated is aligned to cache-line boundary at both ends. The above
>>>> approach puts this onus on the driver. I have documented the alignment
>>>> requirement in my recent patch series for fixing arm cache problems.
>>>
>>> I have not noticed the patch series. ;-)
>>> If we put the alignment burden to the driver, I'm afraid many drivers
>>> which make use of something like a DMA controller have to modify the
>>> code heavily. This sounds not good. :)
>>
>> We have a fundamental problem when it comes to invalidating an
>> un-aligned buffer. Either you flush the boundary lines and corrupt your
>> buffer at boundaries OR you invalidate without flushing and corrupt
>> memory around your buffer. Both are not good! The only real solution is
>> to have aligned buffers, if you want to have D-cache enabled and do DMA
>> at the same time.
>
> Plus, there should not be *heavy* modifications; DMA engines tend to use
> essentially two types of memory-resident objects: data buffers and
> buffer descriptors. There's only a small handful of places in the driver
> code to look at to find where these objects are allocated and how.
>
> So I stand by my opinion: since the cache invalidation routine should
> only be called with cache-aligned objects, there is no requirement to
> flush the first (resp. last) cache line in case of unaligned start
> (resp.stop), and I don't want cache operations performed when they are
> not required.

IMHO, flushing is better, because the person who commits the
mistake of invalidating the un-aligned buffer is the one who is
affected and is likely to fix the issue soon. If we didn't flush, the
resulting corruption will cause totally random errors that will be hard
to debug. Doing an extra flush operation for a maximum of 2 lines
doesn't cost us anything. This is the approach followed by the kernel
too.

br,
Aneesh

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

[Albert ARIBAUD]

(BTW: responders to this thread please stop using my @free.fr address. I 
just noticed the big pile of U-Boot related messages that went to an 
account which I do not use for U-Boot any more)

On 05/08/2011 13:51, Aneesh V wrote:
> Hi Albert,
>
> On Friday 05 August 2011 04:33 PM, Albert ARIBAUD wrote:
>> Hi Aneesh,
>>
>> On 05/08/2011 12:47, Aneesh V wrote:
>>> Hi Eric,
>>>
>>> On Friday 05 August 2011 04:03 PM, Hong Xu wrote:
>>>> Hi Aneesh,
>>> [snip ..]
>>>>>
>>>>> IMHO, Hong's approach is correct. If the buffer that is invalidated is
>>>>> not aligned to cache-line, one cache-line at the respective boundary
>>>>> may have to be flushed to make sure the invalidation doesn't affect
>>>>> somebody else's memory.
>>>>>
>>>>> The solution is for drivers to ensure that any buffer that needs to be
>>>>> invalidated is aligned to cache-line boundary at both ends. The above
>>>>> approach puts this onus on the driver. I have documented the alignment
>>>>> requirement in my recent patch series for fixing arm cache problems.
>>>>
>>>> I have not noticed the patch series. ;-)
>>>> If we put the alignment burden to the driver, I'm afraid many drivers
>>>> which make use of something like a DMA controller have to modify the
>>>> code heavily. This sounds not good. :)
>>>
>>> We have a fundamental problem when it comes to invalidating an
>>> un-aligned buffer. Either you flush the boundary lines and corrupt your
>>> buffer at boundaries OR you invalidate without flushing and corrupt
>>> memory around your buffer. Both are not good! The only real solution is
>>> to have aligned buffers, if you want to have D-cache enabled and do DMA
>>> at the same time.
>>
>> Plus, there should not be *heavy* modifications; DMA engines tend to use
>> essentially two types of memory-resident objects: data buffers and
>> buffer descriptors. There's only a small handful of places in the driver
>> code to look at to find where these objects are allocated and how.
>>
>> So I stand by my opinion: since the cache invalidation routine should
>> only be called with cache-aligned objects, there is no requirement to
>> flush the first (resp. last) cache line in case of unaligned start
>> (resp.stop), and I don't want cache operations performed when they are
>> not required.
>
> IMHO, flushing is better, because the person who commits the
> mistake of invalidating the un-aligned buffer is the one who is
> affected and is likely to fix the issue soon. If we didn't flush, the
> resulting corruption will cause totally random errors that will be hard
> to debug. Doing an extra flush operation for a maximum of 2 lines
> doesn't cost us anything. This is the approach followed by the kernel
> too.

As pointed out by Reinhard, flushing while invalidating is only almost 
good, and not required at all if alignment requirements are followed.

Especially, I don't buy the argument that "the person who commits the 
mistake of invalidating the un-aligned buffer is the one who is affected 
and is likely to fix the issue soon". The issue might not appear right 
after the call to flush is added; it might appear quite later, after 
several reorganizations of the ordering of data in RAM, and affect some 
completely unrelated person doing something completely unrelated.

OTOH, aligning buffers on cache boundaries removes the need to flush 
within invalidates, and will ensure no other data is at any risk.

Between an implementation that "should cause no issue" and an 
implementation that "cannot cause issues", I definitely favor the 
latter: so that's a no on my side to any flushing while invalidating a 
range.

> br,
> Aneesh

Amicalement,
-- 
Albert.

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

[Aneesh V]

Hi Albert,

On Friday 05 August 2011 06:47 PM, Albert ARIBAUD wrote:
> (BTW: responders to this thread please stop using my @free.fr address. I
> just noticed the big pile of U-Boot related messages that went to an
> account which I do not use for U-Boot any more)
>
> On 05/08/2011 13:51, Aneesh V wrote:
>> Hi Albert,
>>
>> On Friday 05 August 2011 04:33 PM, Albert ARIBAUD wrote:
>>> Hi Aneesh,
>>>
>>> On 05/08/2011 12:47, Aneesh V wrote:
>>>> Hi Eric,
>>>>
>>>> On Friday 05 August 2011 04:03 PM, Hong Xu wrote:
>>>>> Hi Aneesh,
>>>> [snip ..]
>>>>>>
>>>>>> IMHO, Hong's approach is correct. If the buffer that is
>>>>>> invalidated is
>>>>>> not aligned to cache-line, one cache-line at the respective boundary
>>>>>> may have to be flushed to make sure the invalidation doesn't affect
>>>>>> somebody else's memory.
>>>>>>
>>>>>> The solution is for drivers to ensure that any buffer that needs
>>>>>> to be
>>>>>> invalidated is aligned to cache-line boundary at both ends. The above
>>>>>> approach puts this onus on the driver. I have documented the
>>>>>> alignment
>>>>>> requirement in my recent patch series for fixing arm cache problems.
>>>>>
>>>>> I have not noticed the patch series. ;-)
>>>>> If we put the alignment burden to the driver, I'm afraid many drivers
>>>>> which make use of something like a DMA controller have to modify the
>>>>> code heavily. This sounds not good. :)
>>>>
>>>> We have a fundamental problem when it comes to invalidating an
>>>> un-aligned buffer. Either you flush the boundary lines and corrupt your
>>>> buffer at boundaries OR you invalidate without flushing and corrupt
>>>> memory around your buffer. Both are not good! The only real solution is
>>>> to have aligned buffers, if you want to have D-cache enabled and do DMA
>>>> at the same time.
>>>
>>> Plus, there should not be *heavy* modifications; DMA engines tend to use
>>> essentially two types of memory-resident objects: data buffers and
>>> buffer descriptors. There's only a small handful of places in the driver
>>> code to look at to find where these objects are allocated and how.
>>>
>>> So I stand by my opinion: since the cache invalidation routine should
>>> only be called with cache-aligned objects, there is no requirement to
>>> flush the first (resp. last) cache line in case of unaligned start
>>> (resp.stop), and I don't want cache operations performed when they are
>>> not required.
>>
>> IMHO, flushing is better, because the person who commits the
>> mistake of invalidating the un-aligned buffer is the one who is
>> affected and is likely to fix the issue soon. If we didn't flush, the
>> resulting corruption will cause totally random errors that will be hard
>> to debug. Doing an extra flush operation for a maximum of 2 lines
>> doesn't cost us anything. This is the approach followed by the kernel
>> too.
>
> As pointed out by Reinhard, flushing while invalidating is only almost
> good, and not required at all if alignment requirements are followed.

I don't dispute that having buffers aligned is the ideal scenario. The
question is about error-handling the situation when this requirement is
not met.

>
> Especially, I don't buy the argument that "the person who commits the
> mistake of invalidating the un-aligned buffer is the one who is affected
> and is likely to fix the issue soon". The issue might not appear right
> after the call to flush is added; it might appear quite later, after
> several reorganizations of the ordering of data in RAM, and affect some
> completely unrelated person doing something completely unrelated.

I don't get how a flush can affect an un-related person unless the
surrounding buffer also happens to be a DMA receive buffer. Please note
that flush is something that can happen to any dirty line as part of
the cache line replacement done by hardware when new lines are brought
into the cache, it's not such a dangerous thing for normal memory
locations.

>
> OTOH, aligning buffers on cache boundaries removes the need to flush
> within invalidates, and will ensure no other data is at any risk.

If the buffers are aligned, the flush operation will not get executed.
So, what is the risk?

>
> Between an implementation that "should cause no issue" and an
> implementation that "cannot cause issues", I definitely favor the
> latter: so that's a no on my side to any flushing while invalidating a
> range.

Let's look at the different cases:

Let A be an un-aligned DMA receive buffer. Let B be a buffer lying
next to A belonging to another program.

Case I - A is aligned
1. Invalidate with flush - no issue
2. Invalidate without flush - no issue

Case II - A is un-aligned, B is DMA receive buffer:
1. Invalidate with flush - corrupts B if DMA is on-going.
2. Invalidate without flush - no issue.

Case III - A is un-aligned, B is normal memory.
1. Invalidate with flush - A corrupted, no issue for B.
2. Invalidate without flush - no issue for A, B is corrupted.

Case I doesn't cause any issue either way. Case II would be rather
rare. If it happens, it's more likely that B belongs to the same driver
owning A. Case III is the more common error case and you can see that
Invalidate with Flush is better because it doesn't affect B.

best regards,
Aneesh

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

[Albert ARIBAUD]

Hi Aneesh,

(cutting quotation for readability)

Le 05/08/2011 16:59, Aneesh V a ?crit :
> Hi Albert,

> I don't dispute that having buffers aligned is the ideal scenario. The
> question is about error-handling the situation when this requirement is
> not met.

I understand what you're trying to achieve in this respect, that is, 
make the code as correct as it can be under unspecified conditions. I 
believe we are differing in how we construe 'as correct as it can be': 
you want to make the implementation of the called function as correct as 
it can be' at the expense of introducing a non-intuitive behavior (flush 
while invalidating), while I prefer the overall system to be as correct 
as it can be by 'doing exactly what it says on the tin', i.e. 
invalidating only.

I believe  that the added intelligence of trying to perform as 
resiliently as possible is perfectly understandable, desirable and 
achievable in an operating system, but that in a bootloader, which has 
tighter constraints of size and speed notably, adding intelligence just 
to supplement lack of conformance in the code should be avoided.

>> Especially, I don't buy the argument that "the person who commits the
>> mistake of invalidating the un-aligned buffer is the one who is affected
>> and is likely to fix the issue soon". The issue might not appear right
>> after the call to flush is added; it might appear quite later, after
>> several reorganizations of the ordering of data in RAM, and affect some
>> completely unrelated person doing something completely unrelated.
>
> I don't get how a flush can affect an un-related person unless the
> surrounding buffer also happens to be a DMA receive buffer.

There you have it: it can. :)

More seriously: both leaving out the flush and doing it have issues when 
unaligned buffers are involved. Of these to imperfect solutions, I 
prefer the one that performs more efficiently in nominal cases.

> Please note
> that flush is something that can happen to any dirty line as part of
> the cache line replacement done by hardware when new lines are brought
> into the cache, it's not such a dangerous thing for normal memory
> locations.

For normal memory indeed, it does not matter. But as soon as you start 
using -- needing -- flushes and invalidates, that means we're not 
dealing with normal memory any more, but with memory shared with other 
devices, DMAs or CPUs -- but that is true for doing flushes at the ends 
of an invalidate region... and for not doing them. Both solutions are 
simply imperfect due to the simple fact that the cache line just might 
contain parts of *two* different DMA buffers.

> If the buffers are aligned, the flush operation will not get executed.
> So, what is the risk?

If the operation is not performed in the nominal case, then let's just 
not do it -- things have already gone wrong anyway.

>> Between an implementation that "should cause no issue" and an
>> implementation that "cannot cause issues", I definitely favor the
>> latter: so that's a no on my side to any flushing while invalidating a
>> range.
>
> Let's look at the different cases:
>
> Let A be an un-aligned DMA receive buffer. Let B be a buffer lying
> next to A belonging to another program.
>
> Case I - A is aligned
> 1. Invalidate with flush - no issue
> 2. Invalidate without flush - no issue
>
> Case II - A is un-aligned, B is DMA receive buffer:
> 1. Invalidate with flush - corrupts B if DMA is on-going.
> 2. Invalidate without flush - no issue.
>
> Case III - A is un-aligned, B is normal memory.
> 1. Invalidate with flush - A corrupted, no issue for B.
> 2. Invalidate without flush - no issue for A, B is corrupted.
>
> Case I doesn't cause any issue either way. Case II would be rather
> rare. If it happens, it's more likely that B belongs to the same driver
> owning A. Case III is the more common error case and you can see that
> Invalidate with Flush is better because it doesn't affect B.

I do realize situations which will result in issues are rare. But I do 
realize that adding the extra logic of flushing is overkill compared to 
what the driver designer should have done, i.e. aligning some buffers, 
and that it will not fix all cases.

Also, I don't believe (any more...) in words like 'rare', 'likely' or 
'probably'. I've gone through some pains myself about 'rare' conditions 
which actually were not that rare and cost me days, if not weeks on... 
rare... occasions.

So let's not try to solve out-of-spec conditions with hypotheses about 
'probable' situations it would or would not handle -- we're out of 
specs, we don't handle things any more. Let's just do 'what it says on 
the tin', and since the function says it invalidates, let's just 
invalidate. The code will be simpler, cleaner, more understandable for 
nominal case, and will noisily signal out-of-spec cases and possibly 
misbehave then, but any code would, so that's ok.

And lets fix alignement issues where that belongs, i.e. in the driver 
buffers alignment. :)

> best regards,
> Aneesh

Amicalement,
-- 
Albert.

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

[Aneesh V]

Hi Albert,

On Sunday 07 August 2011 12:25 PM, Albert ARIBAUD wrote:
> Hi Aneesh,
>
> (cutting quotation for readability)
>
> Le 05/08/2011 16:59, Aneesh V a ?crit :
>> Hi Albert,
>
>> I don't dispute that having buffers aligned is the ideal scenario. The
>> question is about error-handling the situation when this requirement is
>> not met.
>
> I understand what you're trying to achieve in this respect, that is,
> make the code as correct as it can be under unspecified conditions. I
> believe we are differing in how we construe 'as correct as it can be':
> you want to make the implementation of the called function as correct as
> it can be' at the expense of introducing a non-intuitive behavior (flush
> while invalidating), while I prefer the overall system to be as correct
> as it can be by 'doing exactly what it says on the tin', i.e.
> invalidating only.

I understand your point of view now. I shall update my cache fix series
to invalidate only the aligned part of the buffer and to print a big
warning when the buffer is not aligned.

best regards,
Aneesh

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

[Albert ARIBAUD]

Le 08/08/2011 10:24, Aneesh V a ?crit :
> Hi Albert,
>
> On Sunday 07 August 2011 12:25 PM, Albert ARIBAUD wrote:
>> Hi Aneesh,
>>
>> (cutting quotation for readability)
>>
>> Le 05/08/2011 16:59, Aneesh V a ?crit :
>>> Hi Albert,
>>
>>> I don't dispute that having buffers aligned is the ideal scenario. The
>>> question is about error-handling the situation when this requirement is
>>> not met.
>>
>> I understand what you're trying to achieve in this respect, that is,
>> make the code as correct as it can be under unspecified conditions. I
>> believe we are differing in how we construe 'as correct as it can be':
>> you want to make the implementation of the called function as correct as
>> it can be' at the expense of introducing a non-intuitive behavior (flush
>> while invalidating), while I prefer the overall system to be as correct
>> as it can be by 'doing exactly what it says on the tin', i.e.
>> invalidating only.
>
> I understand your point of view now. I shall update my cache fix series
> to invalidate only the aligned part of the buffer and to print a big
> warning when the buffer is not aligned.

Thanks Aneesh.

Another point I raised with Hong Xu's patch: for range-limited 
operations, in case of a misalignment, why not try to *reduce* to 
aligned addresses rather than *expand* it? Moving start up to the next 
cache line boundary, and moving stop down, would still cause an 
imperfect situation (can't help it anyway) but it would not affect third 
party data any more, only the data which the cache range operation was 
supposed to affect.

What do you (and others) think?

> best regards,
> Aneesh

Amicalement,
-- 
Albert.

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

[Aneesh V]

Hi Albert,

On Monday 08 August 2011 03:09 PM, Albert ARIBAUD wrote:
> Le 08/08/2011 10:24, Aneesh V a ?crit :
>> Hi Albert,
>>
>> On Sunday 07 August 2011 12:25 PM, Albert ARIBAUD wrote:
>>> Hi Aneesh,
>>>
>>> (cutting quotation for readability)
>>>
>>> Le 05/08/2011 16:59, Aneesh V a ?crit :
>>>> Hi Albert,
>>>
>>>> I don't dispute that having buffers aligned is the ideal scenario. The
>>>> question is about error-handling the situation when this requirement is
>>>> not met.
>>>
>>> I understand what you're trying to achieve in this respect, that is,
>>> make the code as correct as it can be under unspecified conditions. I
>>> believe we are differing in how we construe 'as correct as it can be':
>>> you want to make the implementation of the called function as correct as
>>> it can be' at the expense of introducing a non-intuitive behavior (flush
>>> while invalidating), while I prefer the overall system to be as correct
>>> as it can be by 'doing exactly what it says on the tin', i.e.
>>> invalidating only.
>>
>> I understand your point of view now. I shall update my cache fix series
>> to invalidate only the aligned part of the buffer and to print a big
>> warning when the buffer is not aligned.
>
> Thanks Aneesh.
>
> Another point I raised with Hong Xu's patch: for range-limited
> operations, in case of a misalignment, why not try to *reduce* to
> aligned addresses rather than *expand* it? Moving start up to the next
> cache line boundary, and moving stop down, would still cause an
> imperfect situation (can't help it anyway) but it would not affect third
> party data any more, only the data which the cache range operation was
> supposed to affect.
>
> What do you (and others) think?

I agree. Indeed this is what I meant when I wrote this above:
"invalidate *only the aligned part* of the buffer"

best regards,
Aneesh

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

[Reinhard Meyer]

Dear Albert, Aneesh, Hong,

There seem to be functions of type

xxx(start, end) and xxx(start, size).

Can't it be somehow decided to use only one variant
in all cases (flush, invalidate)?

On a personal taste, I'd prefer (start, size) :)

Best Regards,
Reinhard

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

[Aneesh V]

Hi Reinhard,

On Monday 08 August 2011 03:29 PM, Reinhard Meyer wrote:
> Dear Albert, Aneesh, Hong,
>
> There seem to be functions of type
>
> xxx(start, end) and xxx(start, size).
>
> Can't it be somehow decided to use only one variant
> in all cases (flush, invalidate)?

The u-boot standard seems to be xxx(start, end) where the operation
will be done on the range [start, end). This is what I figured out by
looking at the prototypes and existing implementations when I did the
armv7 work and I have stuck to this standard.

Hong also seems to be following the same standard.

If there is no objection, I shall add this definition to the README I
am adding.

best regards,
Aneesh

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

[Reinhard Meyer]

Hi Aneesh,
> On Monday 08 August 2011 03:29 PM, Reinhard Meyer wrote:
>> Dear Albert, Aneesh, Hong,
>>
>> There seem to be functions of type
>>
>> xxx(start, end) and xxx(start, size).
>>
>> Can't it be somehow decided to use only one variant
>> in all cases (flush, invalidate)?
> 
> The u-boot standard seems to be xxx(start, end) where the operation
> will be done on the range [start, end). This is what I figured out by
> looking at the prototypes and existing implementations when I did the
> armv7 work and I have stuck to this standard.
> 
> Hong also seems to be following the same standard.
> 
> If there is no objection, I shall add this definition to the README I
> am adding.

Maybe its arch specific, I just saw this in another thread, thats why I asked:


+++ b/arch/mips/cpu/mips32/cpu.c
@@ -52,6 +52,11 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 void flush_cache(ulong start_addr, ulong size)


Best Regards,
Reinhard

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

[Aneesh V]

Hi Reinhard,

On Monday 08 August 2011 03:55 PM, Reinhard Meyer wrote:
> Hi Aneesh,
>> On Monday 08 August 2011 03:29 PM, Reinhard Meyer wrote:
>>> Dear Albert, Aneesh, Hong,
>>>
>>> There seem to be functions of type
>>>
>>> xxx(start, end) and xxx(start, size).
>>>
>>> Can't it be somehow decided to use only one variant
>>> in all cases (flush, invalidate)?
>>
>> The u-boot standard seems to be xxx(start, end) where the operation
>> will be done on the range [start, end). This is what I figured out by
>> looking at the prototypes and existing implementations when I did the
>> armv7 work and I have stuck to this standard.
>>
>> Hong also seems to be following the same standard.
>>
>> If there is no objection, I shall add this definition to the README I
>> am adding.
>
> Maybe its arch specific, I just saw this in another thread, thats why I asked:
>
>
> +++ b/arch/mips/cpu/mips32/cpu.c
> @@ -52,6 +52,11 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
>   void flush_cache(ulong start_addr, ulong size)

I think the confusion about flush_cache() is because in
include/common.h the prototype for flush_cache() doesn't have names for
the paramaeters. It's like this:

void	flush_cache   (unsigned long, unsigned long);

However, the invalidate and flush range functions are clearly defined:

void	flush_dcache_range(unsigned long start, unsigned long stop);
void	invalidate_dcache_range(unsigned long start, unsigned long stop);

I don't know what to do about flush_cache() now that it seems to have
conflicting implementations.

best regards,
Aneesh

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

[Albert ARIBAUD]

Hi Aneesh,

On 08/08/2011 12:27, Aneesh V wrote:
> Hi Reinhard,
>
> On Monday 08 August 2011 03:55 PM, Reinhard Meyer wrote:
>> Hi Aneesh,
>>> On Monday 08 August 2011 03:29 PM, Reinhard Meyer wrote:
>>>> Dear Albert, Aneesh, Hong,
>>>>
>>>> There seem to be functions of type
>>>>
>>>> xxx(start, end) and xxx(start, size).
>>>>
>>>> Can't it be somehow decided to use only one variant
>>>> in all cases (flush, invalidate)?
>>>
>>> The u-boot standard seems to be xxx(start, end) where the operation
>>> will be done on the range [start, end). This is what I figured out by
>>> looking at the prototypes and existing implementations when I did the
>>> armv7 work and I have stuck to this standard.
>>>
>>> Hong also seems to be following the same standard.
>>>
>>> If there is no objection, I shall add this definition to the README I
>>> am adding.
>>
>> Maybe its arch specific, I just saw this in another thread, thats why
>> I asked:
>>
>>
>> +++ b/arch/mips/cpu/mips32/cpu.c
>> @@ -52,6 +52,11 @@ int do_reset(cmd_tbl_t *cmdtp, int flag, int argc,
>> char * const argv[])
>>
>> void flush_cache(ulong start_addr, ulong size)
>
> I think the confusion about flush_cache() is because in
> include/common.h the prototype for flush_cache() doesn't have names for
> the paramaeters. It's like this:
>
> void flush_cache (unsigned long, unsigned long);
>
> However, the invalidate and flush range functions are clearly defined:
>
> void flush_dcache_range(unsigned long start, unsigned long stop);
> void invalidate_dcache_range(unsigned long start, unsigned long stop);
>
> I don't know what to do about flush_cache() now that it seems to have
> conflicting implementations.

My opinion is that global cache functions should not have arguments at 
all and should act on the full cache. Note that drivers should only use 
range versions, not global versions; the only places I see global cache 
actions occurring are initialization when invalidating the whole cache 
just before enabling it, and control transfer to an OS where the whole 
cache should be flushed, invalidated and then disabled before jumping to 
the OS.

So here, if any arch has cache functions that are non-global but don't 
have "range" in their names, that should be changed.

Also, any function with only parameter types have parameter names added 
(cosmetic patch welcome for anything of this kind in ARM).

> best regards,
> Aneesh

Amicalement,
-- 
Albert.

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

[J. William Campbell]

On 8/5/2011 4:51 AM, Aneesh V wrote:
> Hi Albert,
>
> On Friday 05 August 2011 04:33 PM, Albert ARIBAUD wrote:
>> Hi Aneesh,
>>
>> On 05/08/2011 12:47, Aneesh V wrote:
>>> Hi Eric,
>>>
>>> On Friday 05 August 2011 04:03 PM, Hong Xu wrote:
>>>> Hi Aneesh,
>>> [snip ..]
>>>>> IMHO, Hong's approach is correct. If the buffer that is invalidated is
>>>>> not aligned to cache-line, one cache-line at the respective boundary
>>>>> may have to be flushed to make sure the invalidation doesn't affect
>>>>> somebody else's memory.
>>>>>
>>>>> The solution is for drivers to ensure that any buffer that needs to be
>>>>> invalidated is aligned to cache-line boundary at both ends. The above
>>>>> approach puts this onus on the driver. I have documented the alignment
>>>>> requirement in my recent patch series for fixing arm cache problems.
>>>> I have not noticed the patch series. ;-)
>>>> If we put the alignment burden to the driver, I'm afraid many drivers
>>>> which make use of something like a DMA controller have to modify the
>>>> code heavily. This sounds not good. :)
>>> We have a fundamental problem when it comes to invalidating an
>>> un-aligned buffer. Either you flush the boundary lines and corrupt your
>>> buffer at boundaries OR you invalidate without flushing and corrupt
>>> memory around your buffer. Both are not good! The only real solution is
>>> to have aligned buffers, if you want to have D-cache enabled and do DMA
>>> at the same time.
>> Plus, there should not be *heavy* modifications; DMA engines tend to use
>> essentially two types of memory-resident objects: data buffers and
>> buffer descriptors. There's only a small handful of places in the driver
>> code to look at to find where these objects are allocated and how.
>>
>> So I stand by my opinion: since the cache invalidation routine should
>> only be called with cache-aligned objects, there is no requirement to
>> flush the first (resp. last) cache line in case of unaligned start
>> (resp.stop), and I don't want cache operations performed when they are
>> not required.
> IMHO, flushing is better, because the person who commits the
> mistake of invalidating the un-aligned buffer is the one who is
> affected and is likely to fix the issue soon. If we didn't flush, the
> resulting corruption will cause totally random errors that will be hard
> to debug. Doing an extra flush operation for a maximum of 2 lines
> doesn't cost us anything. This is the approach followed by the kernel
> too.

Hi All,
         I am interested in this last statement in particular, that 
Linux allows non-cache aligned buffers for DMA. In a previous discussion 
series, we demonstrated why it was IMPOSSIBLE for a non-cache aligned 
DMA buffer to work in conjunction with a "write back" type cache. To be 
clear, by write-back, we mean a cache system that has a single dirty bit 
per cache line, and that if there are stores to any addresses in that 
line, the ENTIRE LINE will be written back into memory, not just the 
changed data. I also seem to recall that the ARM V7 caches were defined 
as write back, but I am not an ARM person so I don't know for sure what 
kind of cache we are talking about here. If it is write-back, there is 
only one possible solution that always works. Write-through will work 
with un-aligned buffers if the correct flushes and invalidates are 
present. In that case, buffer alignment is not so important. However, if 
the same driver is to be used in both cases, it must use cache-aligned 
buffers only.

Best Regards,
Bill Campbell


> br,
> Aneesh
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
>

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

[Albert ARIBAUD]

Hi Bill,

Le 06/08/2011 01:04, J. William Campbell a ?crit :

> Hi All,
>           I am interested in this last statement in particular, that
> Linux allows non-cache aligned buffers for DMA. In a previous discussion
> series, we demonstrated why it was IMPOSSIBLE for a non-cache aligned
> DMA buffer to work in conjunction with a "write back" type cache. To be
> clear, by write-back, we mean a cache system that has a single dirty bit
> per cache line, and that if there are stores to any addresses in that
> line, the ENTIRE LINE will be written back into memory, not just the
> changed data. I also seem to recall that the ARM V7 caches were defined
> as write back, but I am not an ARM person so I don't know for sure what
> kind of cache we are talking about here. If it is write-back, there is
> only one possible solution that always works. Write-through will work
> with un-aligned buffers if the correct flushes and invalidates are
> present. In that case, buffer alignment is not so important. However, if
> the same driver is to be used in both cases, it must use cache-aligned
> buffers only.

The type of caches in ARM vary greatly, can be configurable at runtime, 
with the ability to have settings per regions for some implementations, 
so we could use either write-through or write-back, even in a single run.

I don't like however the idea of solving the issue discussed here by 
adding constraints on the type of caching, because it has other effects 
than helping work in an out-of-spec case: it lowers DMA buffer write 
throughput, it adds complexity in cache configuration on architectures 
which can handle cache regions, and on those which cannot, it causes a 
general RAM write performance hit -- all this for an out-of-spec case 
which is not really complicated to turn into an in-spec case.

> Best Regards,
> Bill Campbell

Amicalement,
-- 
Albert.