Skip to contentSkip to conversation

ARM: OMAP: Always enable the R/W access to all initiators in sram

RFC PATCH In review Kyungmin Park 6 participants 7 messages Nov 26, 2007 – Nov 27, 2007

Subsystems (3)
Arm (32-bit) Arm sub-architectures OMAP2+ support
lore
Share & tools
lore

Kyungmin Park

author
#0 lore raw

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

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

arch/arm/plat-omap/sram.c +9 -8
index dd8dc04..f1e0af3 100644
@@ -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 */
 }
include/asm-arm/arch-omap/control.h +1 -0
index 5f42725..9f9075e 100644
@@ -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)
 
 
 /*
#1 lore raw

Hello Kyungmin,

Quoted 3 lines · Kyungmin Park

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(-)
arch/arm/mach-omap2/board-h4.c +1 -1
Index: linux-omap/arch/arm/mach-omap2/board-h4.c
===================================================================
@@ -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? */
arch/arm/mach-omap2/id.c +9 -1
Index: linux-omap/arch/arm/mach-omap2/id.c
===================================================================
@@ -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);
include/asm-arm/arch-omap/control.h +2 -1
Index: linux-omap/include/asm-arm/arch-omap/control.h
===================================================================
@@ -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)
 
 
 /*
arch/arm/plat-omap/sram.c +1 -1
Index: linux-omap/arch/arm/plat-omap/sram.c
===================================================================
@@ -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) {
#2 lore raw
Quoted 8 lines · Kyungmin Park
Quoted 2 lines

With recent runtime constatns patches, we can't boot the board if the

sram is locked.

Quoted 2 lines

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

Quoted 7 lines

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.

Quoted 3 lines

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.

maintainer
#3 lore raw

Hi,

Quoted 10 lines · Woodruff, Richard
Quoted 10 lines · Kyungmin Park
Quoted 2 lines

With recent runtime constatns patches, we can't boot the board if the

sram is locked.

Quoted 2 lines

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.

Quoted 7 lines
Quoted 5 lines

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.

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 7 lines

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

#4 lore raw
In reply to Paul Walmsley

Hi,

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

arch/arm/plat-omap/sram.c +2 -2
index dd8dc04..71d953f 100644
@@ -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

Quoted 103 lines · Paul Walmsley

Hello Kyungmin,

Quoted 3 lines · Kyungmin Park

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

arch/arm/mach-omap2/board-h4.c 2007-11-26 03:28:30.000000000 -0700 renamed from arch/arm/mach-omap2/board-h4.c 2007-11-26 03:13:50.000000000 -0700 +1 -1
@@ -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
===================================================================

arch/arm/mach-omap2/id.c 2007-11-26 03:35:27.000000000 -0700 renamed from arch/arm/mach-omap2/id.c 2007-11-26 03:13:50.000000000 -0700 +9 -1
@@ -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
===================================================================

include/asm-arm/arch-omap/control.h 2007-11-26 03:28:30.000000000 -0700 renamed from include/asm-arm/arch-omap/control.h 2007-11-26 03:13:50.000000000 -0700 +2 -1
@@ -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
===================================================================

arch/arm/plat-omap/sram.c 2007-11-26 03:28:30.000000000 -0700 renamed from arch/arm/plat-omap/sram.c 2007-11-26 03:13:50.000000000 -0700 +1 -1
@@ -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
[email protected]
http://linux.omap.com/mailman/listinfo/linux-omap-open-source

#5 lore raw

Hello Kyungmin,

Quoted 1 line · Kyungmin Park

There's no configuration ARCH_OMAP242X. :)

Yikes! Nice catch :-)

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

Quoted 1 line

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?

Quoted 1 line

After your patch commits, I will send the patch.

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

- Paul

#6 lore raw

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

In reply to Kyungmin Park
Quoted 8 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Kyungmin Park
Sent: Monday, November 26, 2007 7:38 AM
To: [email protected]

00

Quoted 6 lines

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

Quoted 57 lines

Any comments are welcome.

Thank you,
Kyungmin Park
---

(quoted diff) +0 -0

b/arch/arm/plat-omap/sram.c index dd8dc04..f1e0af3 100644

arch/arm/plat-omap/sram.c +0 -0
@@ -97,15 +97,16 @@ static int is_sram_locked(void)
 		type = ctrl_read_reg(CONTROL_STATUS) &

TYPE_MASK; #endif

arch/arm/plat-omap/sram.c +0 -2
-	if (type == GP_DEVICE) {
-		/* RAMFW: R/W access to all initiators for all

qualifier sets */

arch/arm/plat-omap/sram.c +0 -2
-		if (cpu_is_omap242x()) {
-			__raw_writel(0xFF, VA_REQINFOPERM0); /*

all q-vects */

arch/arm/plat-omap/sram.c +0 -1
-			__raw_writel(0xCFDE, VA_READPERM0);  /*

all i-read */

arch/arm/plat-omap/sram.c +0 -1
-			__raw_writel(0xCFDE, VA_WRITEPERM0); /*

all i-write */

arch/arm/plat-omap/sram.c +8 -1
-		}
+	/* 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;
arch/arm/plat-omap/sram.c +1 -1
-	} else
+	else

return 1; /* assume locked with no PPA or
security driver */ }

include/asm-arm/arch-omap/control.h +0 -0
index 5f42725..9f9075e 100644
@@ -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
[email protected]
http://linux.omap.com/mailman/listinfo/linux-omap-open-source