[U-Boot] [PATCH] ARM926: Add mb to the cache invalidate/flush

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi Marek,

First, a (long) preamble with some general considerations:

A. This patch does not fix an actual issues; it is a prospective patch,
modifying code which so far has not malfunctioned, or at least has not
been reported to malfunction.

B. My comments on the patch below are based on the general consideration
that the effect of a memory clobber is to contrain the reordering of
statements around the clobbering. For the sake of simplicity -- and
serenity :) -- my comments are also made under the assumption that the
clobber prevents any access (volatile or not, write or read) from
crossing it.

C. Another general comment is that adding clobber to instructions other
than barriers is IMO not a good thing and isb() should be used instead,
for two reasons:

1) it mixes an implicit secondary purpose into a statement written for
another, explicit purpose; this can drown the implicit purpose into
oblivion, when it should actually be emphasized, which is the goal and
effect of isb();

2) it mixes the ends and the means. The end of your patch is to
put instruction barriers between statements so that their relative
order is preserved during optimization; adding "memory" to the clobber
list of an asm instruction that happens to be one of the statements is
a means, but so is isb() with the added benefit that using isb() allows
architectures to use whatever means (memory clobber, specialized
instruction, other) are best for the arch.

D. My comments on the patch below are based on the current source code.

One could argue that this may change if the function becomes inline.
While this is true, I do not consider this right now because 1) the
function is a strong replacement of a weak symbol, which AFAIU is not
compatible with inlining, and 2) this patch is against the current
tree, not a potential future tree. If/when a patch arrives to make the
function inline, we'll consider the implications and how to solve them
*then*.

Ther may be another impact on this function, namely LTO; but --again,
AFAIU -- LTO does not rewrite the inside of functions; it only
optimizes between functions. And again, we'll deal with LTO if/when
patches for LTO get submitted.

... and, there may be future changes not imagined here that will break
things. We'll deal with them then.

On Wed, 10 Oct 2012 00:44:29 +0200, Marek Vasut <marex@denx.de> wrote:

> Add memory barrier to cache invalidate and flush calls.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> CC: Albert Aribaud <albert.u.boot@aribaud.net>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Otavio Salvador <otavio@ossystems.com.br>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
>  arch/arm/cpu/arm926ejs/cache.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> index 2740ad7..1c67608 100644
> --- a/arch/arm/cpu/arm926ejs/cache.c
> +++ b/arch/arm/cpu/arm926ejs/cache.c
> @@ -30,7 +30,7 @@
>  
>  void invalidate_dcache_all(void)
>  {
> -	asm volatile("mcr p15, 0, %0, c7, c6, 0\n" : : "r"(0));
> +	asm volatile("mcr p15, 0, %0, c7, c6, 0\n" : : "r"(0) : "memory");
>  }

This one is useless since there are no accesses in the function to be
reordered.

>  void flush_dcache_all(void)
> @@ -67,7 +67,8 @@ void invalidate_dcache_range(unsigned long start, unsigned long stop)
>  		return;
>  
>  	while (start < stop) {
> -		asm volatile("mcr p15, 0, %0, c7, c6, 1\n" : : "r"(start));
> +		asm volatile("mcr p15, 0, %0, c7, c6, 1\n"
> +				: : "r"(start) : "memory");
>  		start += CONFIG_SYS_CACHELINE_SIZE;
>  	}
>  }

This one is useless too, as the only access it could constrain is the
one affecting start, which is also affected by the would-be-clobbered
statement (and the enclosing while's condition, thus already preventing
the compiler from reordering.

> @@ -78,11 +79,12 @@ void flush_dcache_range(unsigned long start, unsigned long stop)
>  		return;
>  
>  	while (start < stop) {
> -		asm volatile("mcr p15, 0, %0, c7, c14, 1\n" : : "r"(start));
> +		asm volatile("mcr p15, 0, %0, c7, c14, 1\n"
> +				: : "r"(start) : "memory");
>  		start += CONFIG_SYS_CACHELINE_SIZE;
>  	}
  
Here again, the only access the clobber could constrain is the one
affecting start, which is also affected by the would-be-clobbered
statement (and the enclosing while's condition, thus already preventing
the compiler from reordering.

> -	asm volatile("mcr p15, 0, %0, c7, c10, 4\n" : : "r"(0));
> +	asm volatile("mcr p15, 0, %0, c7, c10, 4\n" : : "r"(0) : "memory");
>  }

Now this asm statement might potentially move around as it does not have
input or output dependencies that the compiler could possibly use to
assess ordering constraints. I would thus suggest replacing the memory
clobber with an 'isb();' placed on the line before the asm volatile,
for the reasons indicated in part C of my (long) preamble above.

>  void flush_cache(unsigned long start, unsigned long size)

Amicalement,
-- 
Albert.