[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock
I don't know the background why this lock added. So if I miss something, please let me know. Or I'll push this before merge window close().
========>
I don't see any reason why we have to lock cache line here. Maybe some targets require this locking, but that's completely target specific re- quirement and we should not have such things in global start codes.
Signed-off-by: Shinya Kuribayashi skuribay@ruby.dti.ne.jp ---
cpu/mips/cache.S | 26 -------------------------- cpu/mips/start.S | 5 ----- 2 files changed, 0 insertions(+), 31 deletions(-)
diff --git a/cpu/mips/cache.S b/cpu/mips/cache.S index 443240e..9d793bf 100644 --- a/cpu/mips/cache.S +++ b/cpu/mips/cache.S @@ -237,29 +237,3 @@ dcache_disable: j ra
.end dcache_disable - -/******************************************************************************* -* -* mips_cache_lock - lock RAM area pointed to by a0 in cache. -* -* RETURNS: N/A -* -*/ -#if defined(CONFIG_PURPLE) -# define CACHE_LOCK_SIZE (CFG_DCACHE_SIZE/2) -#else -# define CACHE_LOCK_SIZE (CFG_DCACHE_SIZE) -#endif - .globl mips_cache_lock - .ent mips_cache_lock -mips_cache_lock: - li a1, K0BASE - CACHE_LOCK_SIZE - addu a0, a1 - li a2, CACHE_LOCK_SIZE - li a3, CFG_CACHELINE_SIZE - move a1, a2 - icacheop(a0,a1,a2,a3,0x1d) - - j ra - - .end mips_cache_lock diff --git a/cpu/mips/start.S b/cpu/mips/start.S index c92b162..fd44da0 100644 --- a/cpu/mips/start.S +++ b/cpu/mips/start.S @@ -267,11 +267,6 @@ reset:
/* Set up temporary stack. */ - li a0, CFG_INIT_SP_OFFSET - la t9, mips_cache_lock - jalr t9 - nop - li t0, CFG_SDRAM_BASE + CFG_INIT_SP_OFFSET la sp, 0(t0)
In message 47DE835F.6040500@ruby.dti.ne.jp you wrote:
I don't know the background why this lock added. So if I miss something, please let me know. Or I'll push this before merge window close().
...
/* Set up temporary stack. */
- li a0, CFG_INIT_SP_OFFSET
- la t9, mips_cache_lock
- jalr t9
- nop
Isn't the lock necessary to use the cache as memory for stack and initial data?
Best regards,
Wolfgang Denk
On Mon, Mar 17, 2008 at 4:04 PM, Wolfgang Denk wd@denx.de wrote:
In message 47DE835F.6040500@ruby.dti.ne.jp you wrote:
I don't know the background why this lock added. So if I miss something, please let me know. Or I'll push this before merge window close().
...
/* Set up temporary stack. */
li a0, CFG_INIT_SP_OFFSETla t9, mips_cache_lockjalr t9nopIsn't the lock necessary to use the cache as memory for stack and initial data?
This code has some problems:
Cache locking is not consistent across MIPS implementations. Some implementations do not support locking at all. The style of locking varies - some support per line locking, others per way, etc, etc. Some parts use bits in status registers instead of the cache ops.
IIRC, some MIPS cache implementations require valid zeroed RAM to init cache parity correctly.
The cache never gets unlocked, so the code relies on whatever gets loaded after u-boot to reinitialize the cache and clear the locks.
In message c166aa9f0803171655q7820858aoaa48b56ad57525c2@mail.gmail.com you wrote:
Isn't the lock necessary to use the cache as memory for stack and initial data?
This code has some problems:
Cache locking is not consistent across MIPS implementations. Some implementations do not support locking at all. The style of locking varies - some support per line locking, others per way, etc, etc. Some parts use bits in status registers instead of the cache ops.
IIRC, some MIPS cache implementations require valid zeroed RAM to init cache parity correctly.
The cache never gets unlocked, so the code relies on whatever gets loaded after u-boot to reinitialize the cache and clear the locks.
This is most probably true. When this code was implemented first, it was most probably with a single CPU in mind.
But simply deleting it is definitely not a good idea, as it would most probably break existing board support.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message c166aa9f0803171655q7820858aoaa48b56ad57525c2@mail.gmail.com you wrote:
Isn't the lock necessary to use the cache as memory for stack and initial data?
Thanks, now I understand the original concept of this locking.
But from the technical POV; looking at start.S, mips_cache_lock() is processed after lowlevel_init. SDRAM is already available at this point, therefore we can use true memory for stack. No lock is needed I think.
# The reason why we have this order is that, cache initialization needs memory itself (this is used by Fill operation for I-cache, and load instruction for D-cache). This conflicts the original mips_cache_lock purpse, doesn't it?
IIRC, some MIPS cache implementations require valid zeroed RAM to init cache parity correctly.
Ah, summarized in 2 lines.
The cache never gets unlocked, so the code relies on whatever gets loaded after u-boot to reinitialize the cache and clear the locks.
And I have to say this is important. IMHO most U-Boot MIPS users must disable this locking due to above reasons. I've disabled for long time since 1.1.X (our boards initial porting), too.
But simply deleting it is definitely not a good idea, as it would most probably break existing board support.
I still think this removal will not break any existing targets, but yes agreed. I'll try to introduce a #ifdef alternative.
Thanks for your comments,
Shinya
Shinya Kuribayashi wrote:
But simply deleting it is definitely not a good idea, as it would most probably break existing board support.
I still think this removal will not break any existing targets, but yes agreed. I'll try to introduce a #ifdef alternative.
Patch attached. Any comments are still appreciated. Thanks.
================> [MIPS] mips_cache_lock: Introduce CFG_INIT_RAM_LOCK
From: Shinya Kuribayashi skuribay@ruby.dti.ne.jp
We don't have to lock cache lines for stack use. Meanwhile, once U-Boot locks cache, but never gets them unlocked. So the code relies on whatever gets loaded after U-Boot to re-initialize the cache and clear the locks. For these reasons, I proposed the removal of mips_cache_lock() from the global start-up routines.
This patch makes mips_cache_lock() user-selectable so that we don't break any existing board support. Let's see how things go for a while.
Signed-off-by: Shinya Kuribayashi skuribay@ruby.dti.ne.jp ---
cpu/mips/cache.S | 2 ++ cpu/mips/start.S | 2 ++ include/configs/dbau1x00.h | 1 + include/configs/gth2.h | 1 + include/configs/incaip.h | 1 + include/configs/pb1x00.h | 1 + include/configs/purple.h | 2 ++ include/configs/qemu-mips.h | 1 + include/configs/tb0229.h | 1 + 9 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/cpu/mips/cache.S b/cpu/mips/cache.S index 443240e..ea921a0 100644 --- a/cpu/mips/cache.S +++ b/cpu/mips/cache.S @@ -238,6 +238,7 @@ dcache_disable:
.end dcache_disable
+#ifdef CFG_INIT_RAM_LOCK /******************************************************************************* * * mips_cache_lock - lock RAM area pointed to by a0 in cache. @@ -263,3 +264,4 @@ mips_cache_lock: j ra
.end mips_cache_lock +#endif /* CFG_INIT_RAM_LOCK */ \ No newline at end of file diff --git a/cpu/mips/start.S b/cpu/mips/start.S index c92b162..901e3ef 100644 --- a/cpu/mips/start.S +++ b/cpu/mips/start.S @@ -267,10 +267,12 @@ reset:
/* Set up temporary stack. */ +#ifdef CFG_INIT_RAM_LOCK li a0, CFG_INIT_SP_OFFSET la t9, mips_cache_lock jalr t9 nop +#endif
li t0, CFG_SDRAM_BASE + CFG_INIT_SP_OFFSET la sp, 0(t0) diff --git a/include/configs/dbau1x00.h b/include/configs/dbau1x00.h index b2f606f..5dfe7b3 100644 --- a/include/configs/dbau1x00.h +++ b/include/configs/dbau1x00.h @@ -187,6 +187,7 @@ #define CFG_MONITOR_BASE TEXT_BASE #define CFG_MONITOR_LEN (192 << 10)
+#define CFG_INIT_RAM_LOCK 1 #define CFG_INIT_SP_OFFSET 0x400000
/* We boot from this flash, selected with dip switch */ diff --git a/include/configs/gth2.h b/include/configs/gth2.h index c2a50c1..a50e212 100644 --- a/include/configs/gth2.h +++ b/include/configs/gth2.h @@ -141,6 +141,7 @@ #define CFG_MONITOR_BASE TEXT_BASE #define CFG_MONITOR_LEN (192 << 10)
+#define CFG_INIT_RAM_LOCK 1 #define CFG_INIT_SP_OFFSET 0x400000
/* We boot from this flash, selected with dip switch */ diff --git a/include/configs/incaip.h b/include/configs/incaip.h index 5ca00b3..2528b35 100644 --- a/include/configs/incaip.h +++ b/include/configs/incaip.h @@ -140,6 +140,7 @@ #define CFG_MONITOR_BASE TEXT_BASE #define CFG_MONITOR_LEN (192 << 10)
+#define CFG_INIT_RAM_LOCK 1 #define CFG_INIT_SP_OFFSET 0x400000
#define CFG_FLASH_BASE PHYS_FLASH_1 diff --git a/include/configs/pb1x00.h b/include/configs/pb1x00.h index 810e0f0..e3a38f8 100644 --- a/include/configs/pb1x00.h +++ b/include/configs/pb1x00.h @@ -105,6 +105,7 @@ #define CFG_MONITOR_BASE TEXT_BASE #define CFG_MONITOR_LEN (192 << 10)
+#define CFG_INIT_RAM_LOCK 1 #define CFG_INIT_SP_OFFSET 0x4000000
/* We boot from this flash, selected with dip switch */ diff --git a/include/configs/purple.h b/include/configs/purple.h index 1be4e05..f936f59 100644 --- a/include/configs/purple.h +++ b/include/configs/purple.h @@ -101,6 +101,8 @@
#define CFG_SDRAM_BASE 0x80000000
+#define CFG_INIT_RAM_LOCK 1 + #define CFG_INIT_SP_OFFSET 0x400000
#define CFG_MALLOC_LEN 128*1024 diff --git a/include/configs/qemu-mips.h b/include/configs/qemu-mips.h index e164019..5371e32 100644 --- a/include/configs/qemu-mips.h +++ b/include/configs/qemu-mips.h @@ -142,6 +142,7 @@ #define CFG_MONITOR_BASE TEXT_BASE #define CFG_MONITOR_LEN (192 << 10)
+#define CFG_INIT_RAM_LOCK 1 #define CFG_INIT_SP_OFFSET 0x400000
/* We boot from this flash, selected with dip switch */ diff --git a/include/configs/tb0229.h b/include/configs/tb0229.h index dadf5d3..61495a7 100644 --- a/include/configs/tb0229.h +++ b/include/configs/tb0229.h @@ -143,6 +143,7 @@ #define CFG_MONITOR_BASE TEXT_BASE #define CFG_MONITOR_LEN (192 << 10)
+#define CFG_INIT_RAM_LOCK 1 #define CFG_INIT_SP_OFFSET 0x400000
#define CFG_FLASH_BASE PHYS_FLASH_1
Shinya Kuribayashi wrote:
Shinya Kuribayashi wrote:
But simply deleting it is definitely not a good idea, as it would most probably break existing board support.
I still think this removal will not break any existing targets, but yes agreed. I'll try to introduce a #ifdef alternative.
Patch attached. Any comments are still appreciated. Thanks.
================> [MIPS] mips_cache_lock: Introduce CFG_INIT_RAM_LOCK
From: Shinya Kuribayashi skuribay@ruby.dti.ne.jp
We don't have to lock cache lines for stack use. Meanwhile, once U-Boot locks cache, but never gets them unlocked. So the code relies on whatever gets loaded after U-Boot to re-initialize the cache and clear the locks. For these reasons, I proposed the removal of mips_cache_lock() from the global start-up routines.
This patch makes mips_cache_lock() user-selectable so that we don't break any existing board support. Let's see how things go for a while.
Signed-off-by: Shinya Kuribayashi skuribay@ruby.dti.ne.jp
cpu/mips/cache.S | 2 ++ cpu/mips/start.S | 2 ++ include/configs/dbau1x00.h | 1 + include/configs/gth2.h | 1 + include/configs/incaip.h | 1 + include/configs/pb1x00.h | 1 + include/configs/purple.h | 2 ++ include/configs/qemu-mips.h | 1 + include/configs/tb0229.h | 1 + 9 files changed, 12 insertions(+), 0 deletions(-)
After careful thought, I changed my mind. I'd like to change the default behavior. The one who makes no regression does none of the work B-)
I hope the patche below is acceptable for the next release.
================>
[MIPS] Request for the 'mips_cache_lock()' removal
The initial intension of having mips_cache_lock() was to use the cache as memory for temporary stack use so that a C environment can be set up as early as possible.
But now mips_cache_lock() follow lowlevel_init(). We've already have the real memory initilaized at this point, therefore we could/should use it. No reason to lock at all.
Other problems:
Cache locking is not consistent across MIPS implementaions. Some imple- mentations don't support locking at all. The style of locking varies - some support per line locking, others per way, etc. Some parts use bits in status registers instead of cache ops. Current mips_cache_lock() is not necessarily general-purpose.
And this is worthy of special mention; once U-Boot/MIPS locks the lines, they are never get unlocked, so the code relies on whatever gets loaded after U-Boot to re-initialize the cache and clear the locks. We're sup- posed to have CFG_INIT_RAM_LOCK and unlock_ram_in_cache() implemented, but leave the situation as it is for a long time.
For these reasons, I proposed the removal of mips_cache_lock() from the global start-up code.
This patch adds CFG_INIT_RAM_LOCK_MIPS to make existing users aware that *things have changed*. If he wants the same behavior as before, he needs to have CFG_INIT_RAM_LOCK_MIPS in his config file.
If we don't have any regression report through several releases, then we'll remove codes entirely.
Signed-off-by: Shinya Kuribayashi skuribay@ruby.dti.ne.jp ---
cpu/mips/cache.S | 2 ++ cpu/mips/start.S | 2 ++ 2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/cpu/mips/cache.S b/cpu/mips/cache.S index 443240e..67ee19f 100644 --- a/cpu/mips/cache.S +++ b/cpu/mips/cache.S @@ -238,6 +238,7 @@ dcache_disable:
.end dcache_disable
+#ifdef CFG_INIT_RAM_LOCK_MIPS /******************************************************************************* * * mips_cache_lock - lock RAM area pointed to by a0 in cache. @@ -263,3 +264,4 @@ mips_cache_lock: j ra
.end mips_cache_lock +#endif /* CFG_INIT_RAM_LOCK_MIPS */ diff --git a/cpu/mips/start.S b/cpu/mips/start.S index c92b162..930f9b3 100644 --- a/cpu/mips/start.S +++ b/cpu/mips/start.S @@ -267,10 +267,12 @@ reset:
/* Set up temporary stack. */ +#ifdef CFG_INIT_RAM_LOCK_MIPS li a0, CFG_INIT_SP_OFFSET la t9, mips_cache_lock jalr t9 nop +#endif
li t0, CFG_SDRAM_BASE + CFG_INIT_SP_OFFSET la sp, 0(t0)
On Fri, Mar 21, 2008 at 8:45 AM, Shinya Kuribayashi skuribay@ruby.dti.ne.jp wrote:
[MIPS] Request for the 'mips_cache_lock()' removal
The initial intension of having mips_cache_lock() was to use the cache as memory for temporary stack use so that a C environment can be set up as early as possible.
But now mips_cache_lock() follow lowlevel_init(). We've already have the real memory initilaized at this point, therefore we could/should use it. No reason to lock at all.
Other problems:
Cache locking is not consistent across MIPS implementaions. Some imple- mentations don't support locking at all. The style of locking varies - some support per line locking, others per way, etc. Some parts use bits in status registers instead of cache ops. Current mips_cache_lock() is not necessarily general-purpose.
And this is worthy of special mention; once U-Boot/MIPS locks the lines, they are never get unlocked, so the code relies on whatever gets loaded after U-Boot to re-initialize the cache and clear the locks. We're sup- posed to have CFG_INIT_RAM_LOCK and unlock_ram_in_cache() implemented, but leave the situation as it is for a long time.
For these reasons, I proposed the removal of mips_cache_lock() from the global start-up code.
This patch adds CFG_INIT_RAM_LOCK_MIPS to make existing users aware that *things have changed*. If he wants the same behavior as before, he needs to have CFG_INIT_RAM_LOCK_MIPS in his config file.
If we don't have any regression report through several releases, then we'll remove codes entirely.
Signed-off-by: Shinya Kuribayashi skuribay@ruby.dti.ne.jp
Acked-by: Andrew Dyer amdyer@gmail.com
cpu/mips/cache.S | 2 ++ cpu/mips/start.S | 2 ++ 2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/cpu/mips/cache.S b/cpu/mips/cache.S index 443240e..67ee19f 100644 --- a/cpu/mips/cache.S +++ b/cpu/mips/cache.S @@ -238,6 +238,7 @@ dcache_disable:
.end dcache_disable+#ifdef CFG_INIT_RAM_LOCK_MIPS /*******************************************************************************
- mips_cache_lock - lock RAM area pointed to by a0 in cache.
@@ -263,3 +264,4 @@ mips_cache_lock: j ra
.end mips_cache_lock+#endif /* CFG_INIT_RAM_LOCK_MIPS */ diff --git a/cpu/mips/start.S b/cpu/mips/start.S index c92b162..930f9b3 100644 --- a/cpu/mips/start.S +++ b/cpu/mips/start.S @@ -267,10 +267,12 @@ reset:
/* Set up temporary stack. */+#ifdef CFG_INIT_RAM_LOCK_MIPS li a0, CFG_INIT_SP_OFFSET la t9, mips_cache_lock jalr t9 nop +#endif
li t0, CFG_SDRAM_BASE + CFG_INIT_SP_OFFSET la sp, 0(t0)
participants (3)
-
Andrew Dyer -
Shinya Kuribayashi -
Wolfgang Denk