ppc4xx: Add dcache_enable() for 440
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
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.
--- cpu/ppc4xx/cache.S | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
cpu/ppc4xx/cache.S +2 -0
@@ -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
Signature · 1 line
1.5.5
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Quoted 7 lines · In message <[email protected]> you wrote:
dcache_enable()was missing for 440 and the patch017e9b7925f74878d0e9475388cca9bda5ef9482["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.
Quoted 4 lines
cpu/ppc4xx/cache.S +0
-0@@ -166,9 +166,11 @@ _GLOBAL(invalidate_dcache) #ifdef CONFIG_440
cpu/ppc4xx/cache.S _GLOBAL(invalidate_dcache) · line 176
Quoted 10 lines
cpu/ppc4xx/cache.S +2
-0.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 ofdo_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
Signature · 5 lines
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.
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Quoted 17 lines · Wolfgang Denk
Quoted 7 lines · In message <[email protected]> you wrote:
dcache_enable()was missing for 440 and the patch017e9b7925f74878d0e9475388cca9bda5ef9482["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.
Quoted 5 lines
cpu/ppc4xx/cache.S +0
-0@@ -166,9 +166,11 @@ _GLOBAL(invalidate_dcache) #ifdef CONFIG_440
Quoted 14 lines
cpu/ppc4xx/cache.S _GLOBAL(invalidate_dcache) · line 176
Quoted 9 lines
cpu/ppc4xx/cache.S +2
-0.globl dcache_disable + .globl dcache_enable .globl icache_disable .globl icache_enable dcache_disable: +dcache_enable: icache_disable: icache_enable: blrI 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.
Quoted 7 lines
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.
Quoted 3 lines
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
=====================================================================
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Quoted 2 lines · In message <[email protected]> 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.
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 2 lines
BTW: All the already existing 440 dummy cache entries were not introduced by myself.
I am aware of this.
Quoted 5 lines
Quoted 3 lines
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
Signature · 5 lines
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
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Quoted 6 lines · Wolfgang Denk
Quoted 3 lines
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? ...
Quoted 8 lines
Quoted 6 lines
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.
Quoted 10 lines
Quoted 5 lines
Quoted 3 lines
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
=====================================================================
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Dear Stefan,
Quoted 9 lines · in message <[email protected]> you wrote:
Quoted 3 lines
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 :-)
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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.
Quoted 6 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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
Signature · 5 lines
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
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Hi Wolfgang,
Quoted 7 lines · Wolfgang Denk
Quoted 5 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 11 lines
Quoted 7 lines
Quoted 4 lines
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
=====================================================================
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Quoted 4 lines · In message <[email protected]> you wrote:
Quoted 2 lines
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.
Quoted 2 lines
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).
Quoted 6 lines
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
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
Signature · 6 lines
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
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Quoted 4 lines · Wolfgang Denk
Quoted 2 lines
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.
Quoted 10 lines
Quoted 2 lines
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.
Quoted 5 lines
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>
Quoted 5 lines
Quoted 3 lines
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
=====================================================================
PATCH 1/1 ppc4xx: Add dcache_enable() for 440
Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Quoted 1 line
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
Signature · 5 lines
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!
PATCH 1/1 Fix missing dcache_enable symbol and declare cache function as weak
In reply to Stefan RoeseRecent 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
include/common.h +6 -6
@@ -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 );
Signature · 1 line
1.5.4.5
PATCH 1/1 Fix missing dcache_enable symbol and declare cache function as weak
Quoted 6 lines · In message <[email protected]> you wrote:
Recent commit
a4986459adds reference todcache_enableset 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 <[email protected]>
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
Signature · 5 lines
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