[RFC][PATCH] ARM: OMAP: Always enable the R/W access to all initiators in sram

[RFC][PATCH] ARM: OMAP: Always enable the R/W access to all initiators in sram

[Kyungmin Park]

Hi,

With recent runtime constatns patches, we can't boot the board if the sram is locked.
I'm not sure it's the proper patch enables R/W access to all omap242x.
But without this patch, it gives the following messages and abort on linefecth

sram: patching 0xd7000fec with 0xd8008544
sram: will not patch address 0xd7000fec (0xd7001000): no magic
srcd 0xc0029494, srcfn 0xc00293b4, offs 0xe0, sramfn 0xd7000f10
sram: patching 0xd7000ff0 with 0xf8009060
sram: will not patch address 0xd7000ff0 (0xd7001000): no magic
srcd 0xc0029498, srcfn 0xc00293b4, offs 0xe4, sramfn 0xd7000f10
sram: patching 0xd7000ff4 with 0xd8008050
sram: will not patch address 0xd7000ff4 (0xd7001000): no magic
srcd 0xc00294a0, srcfn 0xc00293b4, offs 0xec, sramfn 0xd7000f10
sram: patching 0xd7000ffc with 0xd8004010
sram: will not patch address 0xd7000ffc (0xd7001000): no magic
srcd 0xc002958c, srcfn 0xc00294a8, offs 0xe4, sramfn 0xd7000e14
sram: patching 0xd7000ef8 with 0xd8008544
sram: will not patch address 0xd7000ef8 (0xd7001000): no magic
srcd 0xc0029590, srcfn 0xc00294a8, offs 0xe8, sramfn 0xd7000e14
sram: patching 0xd7000efc with 0xf8009060
sram: will not patch address 0xd7000efc (0xd7001000): no magic
srcd 0xc0029594, srcfn 0xc00294a8, offs 0xec, sramfn 0xd7000e14
sram: patching 0xd7000f00 with 0xf80090a4
sram: will not patch address 0xd7000f00 (0xd7001000): no magic
...
Unhandled fault: external abort on linefetch (0x806) at 0x00000000

Also we get the 'type' from CONTROL_STATUS(0x2F0) instead previous (0x2F8).

Please let me know what is the right way.

Any comments are welcome.

Thank you,
Kyungmin Park
---
diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
index dd8dc04..f1e0af3 100644
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -97,15 +97,16 @@ static int is_sram_locked(void)
 		type = ctrl_read_reg(CONTROL_STATUS) & TYPE_MASK;
 #endif
 
-	if (type == GP_DEVICE) {
-		/* RAMFW: R/W access to all initiators for all qualifier sets */
-		if (cpu_is_omap242x()) {
-			__raw_writel(0xFF, VA_REQINFOPERM0); /* all q-vects */
-			__raw_writel(0xCFDE, VA_READPERM0);  /* all i-read */
-			__raw_writel(0xCFDE, VA_WRITEPERM0); /* all i-write */
-		}
+	/* RAMFW: R/W access to all initiators for all qualifier sets */
+	if (cpu_is_omap242x()) {
+		__raw_writel(0xFF, VA_REQINFOPERM0); /* all q-vects */
+		__raw_writel(0xCFDE, VA_READPERM0);  /* all i-read */
+		__raw_writel(0xCFDE, VA_WRITEPERM0); /* all i-write */
+	}
+
+	if (type == GP_DEVICE)
 		return 0;
-	} else
+	else
 		return 1; /* assume locked with no PPA or security driver */
 }
 
diff --git a/include/asm-arm/arch-omap/control.h b/include/asm-arm/arch-omap/control.h
index 5f42725..9f9075e 100644
--- a/include/asm-arm/arch-omap/control.h
+++ b/include/asm-arm/arch-omap/control.h
@@ -38,6 +38,7 @@
 #define CONTROL_DEVCONF0		(CONTROL_GENERAL + 0x04)
 #define CONTROL_DEVCONF1		(CONTROL_GENERAL + 0x68) /* > 242x */
 #define CONTROL_STATUS			(CONTROL_GENERAL + 0x80)
+#define CONTROL_STAT			(CONTROL_GENERAL + 0x88)
 
 
 /*

Re: [RFC][PATCH] ARM: OMAP: Always enable the R/W access to all initiators in sram

[Paul Walmsley]

Hello Kyungmin,

On Mon, 26 Nov 2007, Kyungmin Park wrote:

> With recent runtime constatns patches, we can't boot the board if the sram is locked.
> I'm not sure it's the proper patch enables R/W access to all omap242x.
> But without this patch, it gives the following messages and abort on linefecth

I am puzzled.  

You have definitely found a bug.  It seems that TI, for some unknown 
reason, moved some SCM registers, including CONTROL_STATUS, for 34xx, 
breaking forward compatibility.  And as you point out, the offset for 
CONTROL_STATUS in the recent patch is not right for 242x.  (It's the 
34xx offset.)

I would think this would prevent 242x from booting.  Indeed, you report 
that your setup doesn't boot.  But when I boot current git on N800 here, 
which is 2420-based, it boots fine.  Puzzling evidence.

Anyway, a test patch is below, for the CONTROL_STATUS register issue.  
(Not for git consumption - just for testing.)  Could you try it to see if 
it fixes the problems that you're seeing?  If not, perhaps something else 
is wrong.

Regards, 

- Paul


---
 arch/arm/mach-omap2/board-h4.c      |    2 +-
 arch/arm/mach-omap2/id.c            |   10 +++++++++-
 arch/arm/plat-omap/sram.c           |    2 +-
 include/asm-arm/arch-omap/control.h |    3 ++-
 4 files changed, 13 insertions(+), 4 deletions(-)

Index: linux-omap/arch/arm/mach-omap2/board-h4.c
===================================================================
--- linux-omap.orig/arch/arm/mach-omap2/board-h4.c	2007-11-26 03:13:50.000000000 -0700
+++ linux-omap/arch/arm/mach-omap2/board-h4.c	2007-11-26 03:28:30.000000000 -0700
@@ -272,7 +272,7 @@
 /* 2420 Sysboot setup (2430 is different) */
 static u32 get_sysboot_value(void)
 {
-	return (ctrl_read_reg(CONTROL_STATUS) & 0xFFF);
+	return (ctrl_read_reg(OMAP24XX_CONTROL_STATUS) & 0xFFF);
 }
 
 /* FIXME: This function should be moved to some other file, gpmc.c? */
Index: linux-omap/arch/arm/mach-omap2/id.c
===================================================================
--- linux-omap.orig/arch/arm/mach-omap2/id.c	2007-11-26 03:13:50.000000000 -0700
+++ linux-omap/arch/arm/mach-omap2/id.c	2007-11-26 03:35:27.000000000 -0700
@@ -162,7 +162,15 @@
 	/* Embedding the ES revision info in type field */
 	system_rev = omap_ids[j].type;
 
-	ctrl_status = ctrl_read_reg(CONTROL_STATUS);
+	if (cpu_is_omap24xx()) {
+		i = OMAP24XX_CONTROL_STATUS;
+	} else if (cpu_is_omap343x()) {
+		i = OMAP343X_CONTROL_STATUS;
+	} else {
+		printk(KERN_ERR "id: unknown CPU type\n");
+		BUG();
+	}
+	ctrl_status = ctrl_read_reg(i);
 	system_rev |= (ctrl_status & 0x3f);
 	system_rev |= (ctrl_status & 0x700);
 
Index: linux-omap/include/asm-arm/arch-omap/control.h
===================================================================
--- linux-omap.orig/include/asm-arm/arch-omap/control.h	2007-11-26 03:13:50.000000000 -0700
+++ linux-omap/include/asm-arm/arch-omap/control.h	2007-11-26 03:28:30.000000000 -0700
@@ -37,7 +37,8 @@
 
 #define CONTROL_DEVCONF0		(CONTROL_GENERAL + 0x04)
 #define CONTROL_DEVCONF1		(CONTROL_GENERAL + 0x68) /* > 242x */
-#define CONTROL_STATUS			(CONTROL_GENERAL + 0x80)
+#define OMAP343X_CONTROL_STATUS		(CONTROL_GENERAL + 0x80)
+#define OMAP24XX_CONTROL_STATUS		(CONTROL_GENERAL + 0x88)
 
 
 /*
Index: linux-omap/arch/arm/plat-omap/sram.c
===================================================================
--- linux-omap.orig/arch/arm/plat-omap/sram.c	2007-11-26 03:13:50.000000000 -0700
+++ linux-omap/arch/arm/plat-omap/sram.c	2007-11-26 03:28:30.000000000 -0700
@@ -94,7 +94,7 @@
 
 #if defined(CONFIG_ARCH_OMAP242X)
 	if (cpu_is_omap242x())
-		type = ctrl_read_reg(CONTROL_STATUS) & TYPE_MASK;
+		type = ctrl_read_reg(OMAP24XX_CONTROL_STATUS) & TYPE_MASK;
 #endif
 
 	if (type == GP_DEVICE) {

RE: [RFC][PATCH] ARM: OMAP: Always enable the R/W access to all initiators in sram

[Woodruff, Richard]

> On Mon, 26 Nov 2007, Kyungmin Park wrote:
> 
> > With recent runtime constatns patches, we can't boot the board if the
> sram is locked.
> > I'm not sure it's the proper patch enables R/W access to all omap242x.
> > But without this patch, it gives the following messages and abort on
> linefecth
> 
> I am puzzled.
> 
> You have definitely found a bug.  It seems that TI, for some unknown
> reason, moved some SCM registers, including CONTROL_STATUS, for 34xx,
> breaking forward compatibility.  And as you point out, the offset for
> CONTROL_STATUS in the recent patch is not right for 242x.  (It's the
> 34xx offset.)

24xx is not 34xx.  Among others pieces around PRCM, Security, package-configuration will be different.  Trying to hide omap3 under omap2 just makes some of these things harder to rediscover as the git tree evolves.  

To make OFF mode work for targeted use cases, and match with trust zone security some register groups have been split or moved.

> I would think this would prevent 242x from booting.  Indeed, you report
> that your setup doesn't boot.  But when I boot current git on N800 here,
> which is 2420-based, it boots fine.  Puzzling evidence.

** Probably your boot loader does it and his doesn't.  And/Or there are a couple other bugs around Type (emu,hs,gp,tst) and ES version.

There are real behavioral differences between GP and EMU devices and in some cases also differences between ES versions of the chip as to what mask ROM code does for you.  Firewall settings are one major part.  Boot testing on one GP device of one ES version is really not enough.

I think this kind of thing also was evident in the UART clock setting.  The u-boot we use is only turning on what is necessary where it seems the nokia one is configuring a lot more.  

* The Nokia boot loader should do as little as possible in clock and security regards else there is a danger the kernel will need a lot of rework when you try and use these features more optimally.

Regards,
Richard W.

Re: [RFC][PATCH] ARM: OMAP: Always enable the R/W access to all initiators in sram

[Tony Lindgren]

Hi,

* Woodruff, Richard <r-woodruff2@ti.com> [071126 09:06]:
> > On Mon, 26 Nov 2007, Kyungmin Park wrote:
> > 
> > > With recent runtime constatns patches, we can't boot the board if the
> > sram is locked.
> > > I'm not sure it's the proper patch enables R/W access to all omap242x.
> > > But without this patch, it gives the following messages and abort on
> > linefecth
> > 
> > I am puzzled.
> > 
> > You have definitely found a bug.  It seems that TI, for some unknown
> > reason, moved some SCM registers, including CONTROL_STATUS, for 34xx,
> > breaking forward compatibility.  And as you point out, the offset for
> > CONTROL_STATUS in the recent patch is not right for 242x.  (It's the
> > 34xx offset.)
> 
> 24xx is not 34xx.  Among others pieces around PRCM, Security, package-configuration will be different.  Trying to hide omap3 under omap2 just makes some of these things harder to rediscover as the git tree evolves.  

Although it's more work, making things work in a general way is the best
solution in the long run.

> To make OFF mode work for targeted use cases, and match with trust zone security some register groups have been split or moved.
> 
> > I would think this would prevent 242x from booting.  Indeed, you report
> > that your setup doesn't boot.  But when I boot current git on N800 here,
> > which is 2420-based, it boots fine.  Puzzling evidence.
> 
> ** Probably your boot loader does it and his doesn't.  And/Or there are a couple other bugs around Type (emu,hs,gp,tst) and ES version.
> 
> There are real behavioral differences between GP and EMU devices and in some cases also differences between ES versions of the chip as to what mask ROM code does for you.  Firewall settings are one major part.  Boot testing on one GP device of one ES version is really not enough.
> 
> I think this kind of thing also was evident in the UART clock setting.  The u-boot we use is only turning on what is necessary where it seems the nokia one is configuring a lot more.  
> 
> * The Nokia boot loader should do as little as possible in clock and security regards else there is a danger the kernel will need a lot of rework when you try and use these features more optimally.

I agree with that. We must (re-)initialize everything in the kernel,
otherwise we'll end up with mysterious bug reports like this on things
working on some boards and failing on others.

Regards,

Tony

Re: [RFC][PATCH] ARM: OMAP: Always enable the R/W access to all initiators in sram

[Kyungmin Park]

Hi,

Yes, we should read the OMAP24XX_CONTROL_STATUS instead of
CONTROL_STATUS. and also we must fix following line.

diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
index dd8dc04..71d953f 100644
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -92,9 +92,9 @@ static int is_sram_locked(void)
 {
        int type = 0;

-#if defined(CONFIG_ARCH_OMAP242X)
+#if defined(CONFIG_ARCH_OMAP24XX)
        if (cpu_is_omap242x())
-               type = ctrl_read_reg(CONTROL_STATUS) & TYPE_MASK;
+               type = ctrl_read_reg(OMAP24XX_CONTROL_STATUS) & TYPE_MASK;
 #endif

        if (type == GP_DEVICE) {

There's no configuration ARCH_OMAP242X. :)
I'm not sure is it thre right to check cpu_is_omap242x()? or cpu_is_omap24xx()?

After your patch commits, I will send the patch.

Thank you,
Kyungmin Park


On 11/26/07, Paul Walmsley <paul@booyaka.com> wrote:
> Hello Kyungmin,
>
> On Mon, 26 Nov 2007, Kyungmin Park wrote:
>
> > With recent runtime constatns patches, we can't boot the board if the sram is locked.
> > I'm not sure it's the proper patch enables R/W access to all omap242x.
> > But without this patch, it gives the following messages and abort on linefecth
>
> I am puzzled.
>
> You have definitely found a bug.  It seems that TI, for some unknown
> reason, moved some SCM registers, including CONTROL_STATUS, for 34xx,
> breaking forward compatibility.  And as you point out, the offset for
> CONTROL_STATUS in the recent patch is not right for 242x.  (It's the
> 34xx offset.)
>
> I would think this would prevent 242x from booting.  Indeed, you report
> that your setup doesn't boot.  But when I boot current git on N800 here,
> which is 2420-based, it boots fine.  Puzzling evidence.
>
> Anyway, a test patch is below, for the CONTROL_STATUS register issue.
> (Not for git consumption - just for testing.)  Could you try it to see if
> it fixes the problems that you're seeing?  If not, perhaps something else
> is wrong.
>
> Regards,
>
> - Paul
>
>
> ---
>  arch/arm/mach-omap2/board-h4.c      |    2 +-
>  arch/arm/mach-omap2/id.c            |   10 +++++++++-
>  arch/arm/plat-omap/sram.c           |    2 +-
>  include/asm-arm/arch-omap/control.h |    3 ++-
>  4 files changed, 13 insertions(+), 4 deletions(-)
>
> Index: linux-omap/arch/arm/mach-omap2/board-h4.c
> ===================================================================
> --- linux-omap.orig/arch/arm/mach-omap2/board-h4.c      2007-11-26 03:13:50.000000000 -0700
> +++ linux-omap/arch/arm/mach-omap2/board-h4.c   2007-11-26 03:28:30.000000000 -0700
> @@ -272,7 +272,7 @@
>  /* 2420 Sysboot setup (2430 is different) */
>  static u32 get_sysboot_value(void)
>  {
> -       return (ctrl_read_reg(CONTROL_STATUS) & 0xFFF);
> +       return (ctrl_read_reg(OMAP24XX_CONTROL_STATUS) & 0xFFF);
>  }
>
>  /* FIXME: This function should be moved to some other file, gpmc.c? */
> Index: linux-omap/arch/arm/mach-omap2/id.c
> ===================================================================
> --- linux-omap.orig/arch/arm/mach-omap2/id.c    2007-11-26 03:13:50.000000000 -0700
> +++ linux-omap/arch/arm/mach-omap2/id.c 2007-11-26 03:35:27.000000000 -0700
> @@ -162,7 +162,15 @@
>        /* Embedding the ES revision info in type field */
>        system_rev = omap_ids[j].type;
>
> -       ctrl_status = ctrl_read_reg(CONTROL_STATUS);
> +       if (cpu_is_omap24xx()) {
> +               i = OMAP24XX_CONTROL_STATUS;
> +       } else if (cpu_is_omap343x()) {
> +               i = OMAP343X_CONTROL_STATUS;
> +       } else {
> +               printk(KERN_ERR "id: unknown CPU type\n");
> +               BUG();
> +       }
> +       ctrl_status = ctrl_read_reg(i);
>        system_rev |= (ctrl_status & 0x3f);
>        system_rev |= (ctrl_status & 0x700);
>
> Index: linux-omap/include/asm-arm/arch-omap/control.h
> ===================================================================
> --- linux-omap.orig/include/asm-arm/arch-omap/control.h 2007-11-26 03:13:50.000000000 -0700
> +++ linux-omap/include/asm-arm/arch-omap/control.h      2007-11-26 03:28:30.000000000 -0700
> @@ -37,7 +37,8 @@
>
>  #define CONTROL_DEVCONF0               (CONTROL_GENERAL + 0x04)
>  #define CONTROL_DEVCONF1               (CONTROL_GENERAL + 0x68) /* > 242x */
> -#define CONTROL_STATUS                 (CONTROL_GENERAL + 0x80)
> +#define OMAP343X_CONTROL_STATUS                (CONTROL_GENERAL + 0x80)
> +#define OMAP24XX_CONTROL_STATUS                (CONTROL_GENERAL + 0x88)
>
>
>  /*
> Index: linux-omap/arch/arm/plat-omap/sram.c
> ===================================================================
> --- linux-omap.orig/arch/arm/plat-omap/sram.c   2007-11-26 03:13:50.000000000 -0700
> +++ linux-omap/arch/arm/plat-omap/sram.c        2007-11-26 03:28:30.000000000 -0700
> @@ -94,7 +94,7 @@
>
>  #if defined(CONFIG_ARCH_OMAP242X)
>        if (cpu_is_omap242x())
> -               type = ctrl_read_reg(CONTROL_STATUS) & TYPE_MASK;
> +               type = ctrl_read_reg(OMAP24XX_CONTROL_STATUS) & TYPE_MASK;
>  #endif
>
>        if (type == GP_DEVICE) {
> _______________________________________________
> Linux-omap-open-source mailing list
> Linux-omap-open-source@linux.omap.com
> http://linux.omap.com/mailman/listinfo/linux-omap-open-source
>

Re: [RFC][PATCH] ARM: OMAP: Always enable the R/W access to all initiators in sram

[Paul Walmsley]

Hello Kyungmin,

On Tue, 27 Nov 2007, Kyungmin Park wrote:

> There's no configuration ARCH_OMAP242X. :)

Yikes!  Nice catch :-)  

Time to start checking each patch for nonexistent config symbols before I 
send them.

> I'm not sure is it thre right to check cpu_is_omap242x()? or cpu_is_omap24xx()?

Probably Richard would be the right person to ask.  What do you think, 
Richard?

> After your patch commits, I will send the patch.

Okay, I will send in a patch for CONTROL_STATUS later today.


- Paul

RE: [RFC][PATCH] ARM: OMAP: Always enable the R/W access to allinitiators in sram

[Girish]

 

>-----Original Message-----
>From: linux-omap-open-source-bounces@linux.omap.com 
>[mailto:linux-omap-open-source-bounces@linux.omap.com] On 
>Behalf Of Kyungmin Park
>Sent: Monday, November 26, 2007 7:38 AM
>To: linux-omap-open-source@linux.omap.com
00
>
>Also we get the 'type' from CONTROL_STATUS(0x2F0) instead 
>previous (0x2F8).
>
>Please let me know what is the right way.
>
It seems like the CONTROL_STATUS which was defined earlier in omap[2/3]4xx.h has been moved somewhere else.

Well, just a quick look it seems that CONTROL_STATUS definition has been moved to asm/arch/control.h, here CONTROL_STATUS has been
defined only for 3430 and doesn't handle for 24xx, so 3430 CONTROL_STATUS value is getting included for 24xx. This is breaking code
on 24xx guess. 

Regards,
girish
>Any comments are welcome.
>
>Thank you,
>Kyungmin Park
>---
>diff --git a/arch/arm/plat-omap/sram.c 
>b/arch/arm/plat-omap/sram.c index dd8dc04..f1e0af3 100644
>--- a/arch/arm/plat-omap/sram.c
>+++ b/arch/arm/plat-omap/sram.c
>@@ -97,15 +97,16 @@ static int is_sram_locked(void)
> 		type = ctrl_read_reg(CONTROL_STATUS) & 
>TYPE_MASK;  #endif
> 
>-	if (type == GP_DEVICE) {
>-		/* RAMFW: R/W access to all initiators for all 
>qualifier sets */
>-		if (cpu_is_omap242x()) {
>-			__raw_writel(0xFF, VA_REQINFOPERM0); /* 
>all q-vects */
>-			__raw_writel(0xCFDE, VA_READPERM0);  /* 
>all i-read */
>-			__raw_writel(0xCFDE, VA_WRITEPERM0); /* 
>all i-write */
>-		}
>+	/* RAMFW: R/W access to all initiators for all qualifier sets */
>+	if (cpu_is_omap242x()) {
>+		__raw_writel(0xFF, VA_REQINFOPERM0); /* all q-vects */
>+		__raw_writel(0xCFDE, VA_READPERM0);  /* all i-read */
>+		__raw_writel(0xCFDE, VA_WRITEPERM0); /* all i-write */
>+	}
>+
>+	if (type == GP_DEVICE)
> 		return 0;
>-	} else
>+	else
> 		return 1; /* assume locked with no PPA or 
>security driver */  }
> 
>diff --git a/include/asm-arm/arch-omap/control.h 
>b/include/asm-arm/arch-omap/control.h
>index 5f42725..9f9075e 100644
>--- a/include/asm-arm/arch-omap/control.h
>+++ b/include/asm-arm/arch-omap/control.h
>@@ -38,6 +38,7 @@
> #define CONTROL_DEVCONF0		(CONTROL_GENERAL + 0x04)
> #define CONTROL_DEVCONF1		(CONTROL_GENERAL + 
>0x68) /* > 242x */
> #define CONTROL_STATUS			(CONTROL_GENERAL + 0x80)
>+#define CONTROL_STAT			(CONTROL_GENERAL + 0x88)
> 
> 
> /*
>_______________________________________________
>Linux-omap-open-source mailing list
>Linux-omap-open-source@linux.omap.com
>http://linux.omap.com/mailman/listinfo/linux-omap-open-source
>