[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Shinya Kuribayashi]

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)
 

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Wolfgang Denk]

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

-- 
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
"Ahead warp factor 1"                                  - Captain Kirk

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Andrew Dyer]

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_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?

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.

-- 
Hardware, n.:
 The parts of a computer system that can be kicked.

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Wolfgang Denk]

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

-- 
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
"In the long run, every program becomes rococo, and then rubble."
- Alan Perlis

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Shinya Kuribayashi]

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

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Shinya Kuribayashi]

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

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Shinya Kuribayashi]

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)

[U-Boot-Users] [RFC][MIPS] Remove mips_cache_lock

[Andrew Dyer]

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