[U-Boot] [PATCH] arm1136: cosmetic: Remove double test on CONFIG_SYS_DCACHE_OFF

[Albert ARIBAUD]

Hi Benoît,

On Fri, 5 Oct 2012 19:36:34 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau at advansee.com> wrote:

> Hi Albert,
> 
> On Friday, October 5, 2012 7:05:19 PM, Albert ARIBAUD wrote:
> > Hi Benoît,
> > 
> > On Thu, 4 Oct 2012 18:57:19 +0200 (CEST), Benoît Thébaudeau
> > <benoit.thebaudeau at advansee.com> wrote:
> > 
> > > Hi Albert,
> > > 
> > > On Thursday, October 4, 2012 3:39:41 PM, Albert ARIBAUD wrote:
> > > > Hi Benoît,
> > > > 
> > > > On Tue, 14 Aug 2012 15:17:09 +0200 (CEST), Benoît Thébaudeau
> > > > <benoit.thebaudeau at advansee.com> wrote:
> > > > 
> > > > > Remove a redundant '#ifndef CONFIG_SYS_DCACHE_OFF' nested in
> > > > > the
> > > > > same #ifndef.
> > > > > 
> > > > > Signed-off-by: Benoît Thébaudeau
> > > > > <benoit.thebaudeau at advansee.com>
> > > > > Cc: Albert Aribaud <albert.u.boot at aribaud.net>
> > > > > ---
> > > > >  .../arch/arm/cpu/arm1136/cpu.c                     |    2 --
> > > > >  1 file changed, 2 deletions(-)
> > > > > 
> > > > > diff --git u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/cpu.c
> > > > > u-boot-4d3c95f/arch/arm/cpu/arm1136/cpu.c
> > > > > index b98e3d9..1136c1d 100644
> > > > > --- u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/cpu.c
> > > > > +++ u-boot-4d3c95f/arch/arm/cpu/arm1136/cpu.c
> > > > > @@ -146,9 +146,7 @@ void enable_caches(void)
> > > > >  #ifndef CONFIG_SYS_ICACHE_OFF
> > > > >  	icache_enable();
> > > > >  #endif
> > > > > -#ifndef CONFIG_SYS_DCACHE_OFF
> > > > >  	dcache_enable();
> > > > > -#endif
> > > > >  }
> > > > >  
> > > > >  #else /* #ifndef CONFIG_SYS_DCACHE_OFF */
> > > > 
> > > > I'll NAK this one because:
> > > > 
> > > > 1) obviously the big #ifndef CONFIG_SYS_DCACHE_OFF / #else
> > > > /#endif is
> > > > there to provide either working D$ functions or empty ones;
> > > > 
> > > > 2) enable_caches() exists only in the "then" branch, not at all
> > > > in
> > > > the
> > > > "else" branch, which makes it a surprising exception;
> > > > 
> > > > 3) enable_caches() is the only function in the if/then/else
> > > > acting on
> > > > I$
> > > > as well as D$;
> > > > 
> > > > ... so I suspect it did not actually belong in the big
> > > > if/then/else
> > > > in
> > > > the first place and should not be modified but moved after the
> > > > #endif.
> > > 
> > > I agree, simply because with the current code, enable_caches() does
> > > not enable
> > > icache if CONFIG_SYS_ICACHE_OFF is not defined but
> > > CONFIG_SYS_DCACHE_OFF is.
> > > 
> > > But is it enough to move it? We could indeed move it after the
> > > #endif, but also
> > > change it to:
> > > 
> > > ---
> > > #if !defined(CONFIG_SYS_ICACHE_OFF) ||
> > > !defined(CONFIG_SYS_DCACHE_OFF)
> > > void enable_caches(void)
> > > {
> > > #ifndef CONFIG_SYS_ICACHE_OFF
> > > 	icache_enable();
> > > #endif
> > > #ifndef CONFIG_SYS_DCACHE_OFF
> > > 	dcache_enable();
> > > #endif
> > > }
> > > #endif
> > > ---
> > > 
> > > In that way, the default __enable_caches() from cache.c (outputting
> > > "WARNING: Caches not enabled\n") would be linked if both
> > > CONFIG_SYS_ICACHE_OFF
> > > and CONFIG_SYS_DCACHE_OFF are defined.
> > > 
> > > Do you agree?
> > 
> > When CONFIG_SYS_DCACHE_OFF, dcache functions are not compiled out,
> > they
> > are compiled in but empty. IOW, even with caches off, the API remains
> > callable albeit useless. This is done so that client code does not
> > have to test CONFIG_SYS_DCACHE_OFF before deciding to call the API or
> > not.
> > 
> > Therefore, for consistency, enable_caches() should be defined and
> > empty
> > even when both CONFIG_SYS_DCACHE_OFF and CONFIG_SYS_ICACHE_OFF are
> > defined, which is not the case in the example above due to the
> > enclosing #if/#endif.
> 
> In the example, enable_caches() is still defined if both CONFIG_SYS_DCACHE_OFF
> and CONFIG_SYS_ICACHE_OFF are defined. See arch/arm/lib/cache.c:
> ---
> /*
>  * Default implementation of enable_caches()
>  * Real implementation should be in platform code
>  */
> void __enable_caches(void)
> {
> 	puts("WARNING: Caches not enabled\n");
> }
> void enable_caches(void)
> 	__attribute__((weak, alias("__enable_caches")));
> ---
> 
> But you're right, it's not empty in this case. Is it that you want to remove
> this message in this case?
> 
> This default implementation is used in the same way for several other ARM
> targets.

OK, I see your point: enable_cache() needs not be defined here if
caches are #define'd out, because there is a weak default definition
in arch/arm/lib/cache.c that just prints a warning about caches not
being enabled.

But then, the same holds for flush_dcache_all and flush_cache, which
are not compiled out from as enable_cache is -- and if they were, then
the arch/arm/lib/cache.c versions would kick in, and those are not
empty, especially __flush_cache which contains a conditional call to...
arm1136_cache_flush()

That is certainly not consistent... And on second thought, not defining
a cache-related function at cpu level if caches are compiled out is
more sensible than defining it empty.

All this could undergo a greater change to move cpu-specifics out of
the ARM library so that all lib functions are empty or generic, and
define cpu-specific functions only if caches are compiled in... But
I won't ask for something that's far beyond the goal of your patch.

Conclusion: V2 is good as it is. I'll apply it on /next.

> Best regards,
> Benoît

Amicalement,
-- 
Albert.