[U-Boot-Users] [PATCH] ppc4xx: Add dcache_enable() for 440

[Stefan Roese]

On Monday 21 April 2008, Wolfgang Denk wrote:
> In message <1208530299-9817-1-git-send-email-sr at denx.de> you wrote:
> > dcache_enable() was missing for 440 and the patch
> > 017e9b7925f74878d0e9475388cca9bda5ef9482 ["allow ports to override
> > bootelf "] behavior uses this function.
> >
> > Note: Currently the cache handling functions like
> > d/icache_disable/enable() are NOP's on 440. This may be changed in the
> > future.
>
> Sorry, but I don't think this is a good idea. Actually I tend to
> reject this patch.
>
> > --- a/cpu/ppc4xx/cache.S
> > +++ b/cpu/ppc4xx/cache.S
> > @@ -166,9 +166,11 @@ _GLOBAL(invalidate_dcache)
> >  #ifdef CONFIG_440
> >
> >         .globl  dcache_disable
> > +       .globl  dcache_enable
> >         .globl  icache_disable
> >         .globl  icache_enable
> >  dcache_disable:
> > +dcache_enable:
> >  icache_disable:
> >  icache_enable:
> >  	blr
>
> I was not are that we have such code in U-Boot; I think it  is  plain
> unacceptable.  Either  we  implement these functions correctly, i. e.
> such that they perform the actions their name suggests, or we do  not
> define these functions at all.

So what do you suggest instead? Removing these functions completely for 440? 
This would result in bigger changes to common code currently using those 
functions (especially dcache_disable). Probably by using more #ifdefs there 
which I would really like not to see.

> But providing a function that promises to enable  or  disable  the  I
> resp.  D  cache, and which then actually does nothing, is *extremely*
> misleading. I see all kind of regressions and lost  time  because  of
> people debug completely different parts of the code just because they
> don't know that these functions are dummies.
>
> Please: let's not do this. May I please ask to clean this up?

OK, I removed this patch from my custodian repository. But I assume that you 
are you asking for additional changes too. Are you asking me to remove (a) 
all dummy cache entries or (b) to support *real* cache support functions for 
440? (a) would lead as explained above to bigger code changes in the common 
code and (b) is extremely difficult and I just have no time for such a thing 
currently.

BTW: All the already existing 440 dummy cache entries were not introduced by 
myself.

> Yes,  I  am  aware  that  the   current   (new)   implementation   of
> do_bootelf_exec()  needs  to  be fixed for this, too - and maybe some
> other places as well. But this is important enough to me.

Understood. We should propably revert this patch then.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================