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

[Albert ARIBAUD]

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.

> > Please provide a V2 patch, I promise it'll get processed faster than
> > this V1.
> 
> OK ;) , I'll do that after your answer to the question above.
> 
> Best regards,
> Benoît

Amicalement,
-- 
Albert.