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

[Benoît Thébaudeau]

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?

> 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