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

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

[Stefan Roese]

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.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 cpu/ppc4xx/cache.S |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/cpu/ppc4xx/cache.S b/cpu/ppc4xx/cache.S
index 5124dec..ceb3ec0 100644
--- 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
-- 
1.5.5

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

[Wolfgang Denk]

In message <1208530299-9817-1-git-send-email-sr@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.

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?

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.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Q:  What's a light-year?
A:  One-third less calories than a regular year.

[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@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
=====================================================================

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

[Wolfgang Denk]

In message <200804210658.07170.sr@denx.de> you wrote:
>
> So what do you suggest instead? Removing these functions completely for 440? 

Please let's either proviude real, working implementations, or remove
the functions.

> 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.

I don't like to see these either, but it's better than lying in the
face of the user.

> 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.

Yes, let's do either (a) or (b). There is no other choice.

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

I am aware of this.

> > 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.

This still leaves the problem of the current "implementation" of  the
other  stubs. Please note that as is, we even have *random* behaviour
of the code, as the  functions  are  supposed  to  return  the  cache
status, but no return value gets loaded.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You're dead, Jim.
	-- McCoy, "The Tholian Web", stardate unknown

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

[Stefan Roese]

On Monday 21 April 2008, Wolfgang Denk wrote:
> > 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.
>
> I don't like to see these either, but it's better than lying in the
> face of the user.

Please note that it is not so easy on 440 to even define *what exactly* the 
functions/commands d/icache_en/disable mean. This is because 440 has MMU 
support and we can have different cache setups for all TLB entries. So to 
which TLB entries should these functions refer? Just those mapping SDRAM? 
And/or FLASH? And/or internal SRAM? ...

> > 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.
>
> Yes, let's do either (a) or (b). There is no other choice.

From my point of view, both "solutions" should not be done outside of a 
merge-window. I'll try to find some time to implement on of those options in 
the next weeks. But perhaps somebody else has more "free time" and sends 
patches to implement (and test) this stuff.

> > > 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.
>
> This still leaves the problem of the current "implementation" of  the
> other  stubs. Please note that as is, we even have *random* behaviour
> of the code, as the  functions  are  supposed  to  return  the  cache
> status, but no return value gets loaded.

I don't see such a problem with *random* behavior. d/icache_status return 0 on 
440.

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
=====================================================================

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

[Wolfgang Denk]

Dear Stefan,

in message <200804210729.53635.sr@denx.de> you wrote:
>
> > I don't like to see these either, but it's better than lying in the
> > face of the user.
> 
> Please note that it is not so easy on 440 to even define *what exactly* the
> functions/commands d/icache_en/disable mean. This is because 440 has MMU 
> support and we can have different cache setups for all TLB entries. So to 
> which TLB entries should these functions refer? Just those mapping SDRAM? 
> And/or FLASH? And/or internal SRAM? ...

You are the 440 expert, not me :-)

> > > 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 suppo> rt
> > > 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 ha> ve
> > > no time for such a thing currently.
> >
> > Yes, let's do either (a) or (b). There is no other choice.
> 
> From my point of view, both "solutions" should not be done outside of a 
> merge-window. I'll try to find some time to implement on of those options in 
> the next weeks. But perhaps somebody else has more "free time" and sends 
> patches to implement (and test) this stuff.

I agree that it is probably not  possible  to  fix  this  right  now,
especially  since the actual problem is older, it just became visible
now.

But I insist that this must be fixed  for  the  next  release  -  and
actually it seems to be a good thing to really enable the instruction
cache  -  this would allow to speed up booting on 440 systems quite a
bit.

> > This still leaves the problem of the current "implementation" of  the
> > other  stubs. Please note that as is, we even have *random* behaviour
> > of the code, as the  functions  are  supposed  to  return  the  cache
> > status, but no return value gets loaded.
> 
> I don't see such a problem with *random* behavior. d/icache_status return 0 on 
> 440.

Ah, you are  right.  I  thought  that  the  {i,d}cache_{en,dis}able()
functions  returned a status, but they are indeed void. Sorry for the
false alarm.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There's no sense in being precise  when  you  don't  even  know  what
you're talking about.                             -- John von Neumann

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

[Stefan Roese]

Hi Wolfgang,

On Monday 21 April 2008, Wolfgang Denk wrote:
> > Please note that it is not so easy on 440 to even define *what exactly*
> > the functions/commands d/icache_en/disable mean. This is because 440 has
> > MMU support and we can have different cache setups for all TLB entries.
> > So to which TLB entries should these functions refer? Just those mapping
> > SDRAM? And/or FLASH? And/or internal SRAM? ...
>
> You are the 440 expert, not me :-)

This has nothing to do with 440. It's more a general question. But OK, from my 
understanding, it makes most sense that the i/dcache U-Boot commands touch 
the cache attributes of all SDRAM related TLB's.

> > > > 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
> > > > suppo> rt 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 ha> ve no time for such a thing currently.
> > >
> > > Yes, let's do either (a) or (b). There is no other choice.
> >
> > From my point of view, both "solutions" should not be done outside of a
> > merge-window. I'll try to find some time to implement on of those options
> > in the next weeks. But perhaps somebody else has more "free time" and
> > sends patches to implement (and test) this stuff.
>
> I agree that it is probably not  possible  to  fix  this  right  now,
> especially  since the actual problem is older, it just became visible
> now.

I knew they existed and still thing this is no problem. But we seem to 
disagree here.

> But I insist that this must be fixed  for  the  next  release  -  and
> actually it seems to be a good thing to really enable the instruction
> cache  -  this would allow to speed up booting on 440 systems quite a
> bit.

You might remember that I mentioned that this currently existing cache support 
for 440 is not finished yet. There are still some issues, for example some 
drivers like USB won't work currently with d-cache enabled. This is the 
reason that I didn't enable this option for any of the 4xx boards that I 
maintain. Please note that I already spent some days of my "free time" to 
this cache support on 440. Matthias Fuchs also has contributed here.

And what does this mean that you "insist that this must be fixed for the next 
release"? I'm sorry, but I personally can't promise to "fix" this issue until 
the next merge window opens.

> > > This still leaves the problem of the current "implementation" of  the
> > > other  stubs. Please note that as is, we even have *random* behaviour
> > > of the code, as the  functions  are  supposed  to  return  the  cache
> > > status, but no return value gets loaded.
> >
> > I don't see such a problem with *random* behavior. d/icache_status return
> > 0 on 440.
>
> Ah, you are  right.  I  thought  that  the  {i,d}cache_{en,dis}able()
> functions  returned a status, but they are indeed void. Sorry for the
> false alarm.

OK.

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
=====================================================================

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

[Wolfgang Denk]

In message <200804211023.57611.sr@denx.de> you wrote:
> 
> > You are the 440 expert, not me :-)
> 
> This has nothing to do with 440. It's more a general question. But OK, from my 

Well, it affects only processors which need MMU support. Most doen't.

> understanding, it makes most sense that the i/dcache U-Boot commands touch 
> the cache attributes of all SDRAM related TLB's.

In general, the chackes should be enabled whenever and whereever
possible.

I think it should be pretty safe to enable the I cache for all SDRAM,
SRAM and flash areas; or,  put  differently,  for  all  memory  areas
except maybe any mapped PCI memory windows.

If possible, also D cahce should be enabled, but I cannot judge if
this works or not with the given driver code.

Also, it depends on where the initial stack and data is located  (you
probably cannot disable caches if you put initial data in cache).

> > I agree that it is probably not  possible  to  fix  this  right  now,
> > especially  since the actual problem is older, it just became visible
> > now.
> 
> I knew they existed and still thing this is no problem. But we seem to 
> disagree here.

The problem is that the code is misleading. I read some  source  file
and  see  "icache_enable()",  so  I  expect  that  the  I cache is on
afterwards. It is  completely  counterintuitive  if  it  isn't.  Such
things  have  caused  debugging  nightmares  before, and I don't have
enough hair left to tear the rest on such things.

> You might remember that I mentioned that this currently existing cache support 
> for 440 is not finished yet. There are still some issues, for example some 

Yes, I do remember that. But I assumed that the implementation is just
missing. Those fake functions are unacceptable.

> drivers like USB won't work currently with d-cache enabled. This is the 
> reason that I didn't enable this option for any of the 4xx boards that I 
> maintain. Please note that I already spent some days of my "free time" to 
> this cache support on 440. Matthias Fuchs also has contributed here.

I appreciate this.

> And what does this mean that you "insist that this must be fixed for the next 
> release"? I'm sorry, but I personally can't promise to "fix" this issue until 
> the next merge window opens.

Next release means the one that comes after the next merge window.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I object to intellect without discipline;  I object to power without
constructive purpose.
	-- Spock, "The Squire of Gothos", stardate 2124.5

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

[Stefan Roese]

On Monday 21 April 2008, Wolfgang Denk wrote:
> > This has nothing to do with 440. It's more a general question. But OK,
> > from my
>
> Well, it affects only processors which need MMU support. Most doen't.

I'm not so sure here anymore with all the newer PPC's and other platforms. But 
I have to admit that I'm no expert for those other platforms.

> > understanding, it makes most sense that the i/dcache U-Boot commands
> > touch the cache attributes of all SDRAM related TLB's.
>
> In general, the chackes should be enabled whenever and whereever
> possible.
>
> I think it should be pretty safe to enable the I cache for all SDRAM,
> SRAM and flash areas; or,  put  differently,  for  all  memory  areas
> except maybe any mapped PCI memory windows.
>
> If possible, also D cahce should be enabled, but I cannot judge if
> this works or not with the given driver code.
>
> Also, it depends on where the initial stack and data is located  (you
> probably cannot disable caches if you put initial data in cache).

Another problemtic issue could be the POST area for caches etc. I know that 
self modifying code is used here in some places. This could be more 
problematic with caches enabled.

<snip>

> > And what does this mean that you "insist that this must be fixed for the
> > next release"? I'm sorry, but I personally can't promise to "fix" this
> > issue until the next merge window opens.
>
> Next release means the one that comes after the next merge window.

I did understand this part of the sentence. I'm just not sure what should 
happen with the current code if it doesn't get changed. Again, I personally 
can't promise to "fix" this issue until the next merge window opens.

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
=====================================================================

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

[Wolfgang Denk]

In message <200804211145.28004.sr@denx.de> you wrote:
>
> > Well, it affects only processors which need MMU support. Most doen't.
> 
> I'm not so sure here anymore with all the newer PPC's and other platforms. But 
> I have to admit that I'm no expert for those other platforms.

It's only processors which run in 32 bit  mode  but  are  capable  of
addressing > 32 bit physical address space.

> Another problemtic issue could be the POST area for caches etc. I know that 
> self modifying code is used here in some places. This could be more 
> problematic with caches enabled.

The POST code is supposed to take care of that.

> > Next release means the one that comes after the next merge window.
> 
> I did understand this part of the sentence. I'm just not sure what should 
> happen with the current code if it doesn't get changed. Again, I personally 
> can't promise to "fix" this issue until the next merge window opens.

If you fix it "until the next merge window opens", it will come in
time for the next release, so we all are happy.

For now (i. e. for the upcoming 1.3.3 release), let's use this  quick
and really dirty hack to silence the 440 build errors.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There you go man, Keep as cool as you can. It riles them  to  believe
that you perceive the web they weave. Keep on being free!

[U-Boot-Users] [PATCH] Fix missing dcache_enable symbol and declare cache function as weak

[Jean-Christophe PLAGNIOL-VILLARD]

Recent commit a4986459 adds reference to dcache_enable set dcache and ecache
function as __weak.

For cache status function it will return 0 if the function is not implemented

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

diff --git a/include/common.h b/include/common.h
index f2adebf..ca68802 100644
--- a/include/common.h
+++ b/include/common.h
@@ -386,12 +386,12 @@ void	wr_ic_adr     (uint);
 uint	rd_dc_cst     (void);
 void	wr_dc_cst     (uint);
 void	wr_dc_adr     (uint);
-int	icache_status (void);
-void	icache_enable (void);
-void	icache_disable(void);
-int	dcache_status (void);
-void	dcache_enable (void);
-void	dcache_disable(void);
+int	icache_status (void) __attribute__((weak));
+void	icache_enable (void) __attribute__((weak));
+void	icache_disable(void) __attribute__((weak));
+int	dcache_status (void) __attribute__((weak));
+void	dcache_enable (void) __attribute__((weak));
+void	dcache_disable(void) __attribute__((weak));
 void	relocate_code (ulong, gd_t *, ulong) __attribute__ ((noreturn));
 ulong	get_endaddr   (void);
 void	trap_init     (ulong);
-- 
1.5.4.5

[U-Boot-Users] [PATCH] Fix missing dcache_enable symbol and declare cache function as weak

[Wolfgang Denk]

In message <1208777868-11252-1-git-send-email-plagnioj@jcrosoft.com> you wrote:
> Recent commit a4986459 adds reference to dcache_enable set dcache and ecache
> function as __weak.
> 
> For cache status function it will return 0 if the function is not implemented
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

I hereby reject this.

As discussed before (both here on the ML and on IRC), this is
completely the wrong move to take.

My concern is not silencing  compile  errors,  but  making  the  code
actually easy to read and to understand. And for this it is extremely
important that the code actually does what it claims to do. If I read
"icache_enable()"  I  want  to  be  able  to rely on the icache being
enabled, and not having to check zillions of other source  or  header
files  if there is just a dummy function that does nothing or a macro
definition that lies into my face either.

"But let your statement be, 'Yes, yes ' or 'No, no'; anything beyond
these is of evil." << Matthew 5:37 >>

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A conservative is a man with two perfectly good legs  who  has  never
learned to walk.                              - Franklin D. Roosevelt