[U-Boot-Users] [PATCH] 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.
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
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_enabledcache_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
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_enabledcache_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@denx.de =====================================================================
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
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@denx.de =====================================================================
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
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@denx.de =====================================================================
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
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@denx.de =====================================================================
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
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);
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
participants (3)
-
Jean-Christophe PLAGNIOL-VILLARD -
Stefan Roese -
Wolfgang Denk