Skip to contentSkip to conversation

mx35: Fix boot ROM hang in internal boot mode

PATCH Superseded Hans J. Koch 3 participants 8 messages Aug 11, 2010 – Aug 13, 2010

Subsystems (3)
Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Hans J. Koch

author
#0 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

If a watchdog reset occurs after booting in internal boot mode, the i.MX35 won't boot anymore. The boot ROM code seems to assume that some clocks are turned on (they are after a power-on reset). This patch turns on the necessary clocks.

Signed-off-by Hans J. Koch <[email protected]>
---
 arch/arm/mach-mx3/clock-imx35.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
arch/arm/mach-mx3/clock-imx35.c +16 -4
index d3af0fd..e619bdc 100644
@@ -485,10 +485,10 @@ static struct clk_lookup lookups[] = {
 
 int __init mx35_clocks_init()
 {
-	unsigned int ll = 0;
+	unsigned int cgr2 = 3 << 26, cgr3 = 0;
 
 #if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_DEBUG_ICEDCC)
-	ll = (3 << 16);
+	cgr2 |= (3 << 16);
 #endif
 
 	clkdev_add_table(lookups, ARRAY_SIZE(lookups));

@@ -499,8 +499,20 @@ int __init mx35_clocks_init()
 	__raw_writel((3 << 18), CCM_BASE + CCM_CGR0);
 	__raw_writel((3 << 2) | (3 << 4) | (3 << 6) | (3 << 8) | (3 << 16),
 			CCM_BASE + CCM_CGR1);
-	__raw_writel((3 << 26) | ll, CCM_BASE + CCM_CGR2);
-	__raw_writel(0, CCM_BASE + CCM_CGR3);
+
+	/*
+	 * Check if we came up in internal boot mode. If yes, we need some
+	 * extra clocks turned on, otherwise the MX35 boot ROM code will
+	 * hang after a watchdog reset.
+	 */
+	if (!(__raw_readl(CCM_BASE + CCM_RCSR) & (3 << 10))) {
+		/* Additionally turn on UART0, SCC, and IIM clocks */
+		cgr2 |= (3 << 16) | (3 << 4);
+		cgr3 |= (3 << 2);
+	}
+
+	__raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+	__raw_writel(cgr3, CCM_BASE + CCM_CGR3);
 
 	mxc_timer_init(&gpt_clk,
 			MX35_IO_ADDRESS(MX35_GPT1_BASE_ADDR), MX35_INT_GPT);
Signature · 1 line
1.7.1
#1 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

Hello Hans,

Quoted 15 lines · Hans J. Koch

If a watchdog reset occurs after booting in internal boot mode, the i.MX35 won't boot anymore. The boot ROM code seems to assume that some clocks are turned on (they are after a power-on reset). This patch turns on the necessary clocks.

Signed-off-by Hans J. Koch <[email protected]>
---
 arch/arm/mach-mx3/clock-imx35.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
arch/arm/mach-mx3/clock-imx35.c +0 -0
index d3af0fd..e619bdc 100644

arch/arm/mach-mx3/clock-imx35.c static struct clk_lookup lookups[] = { · line 491

Quoted 9 lines
arch/arm/mach-mx3/clock-imx35.c +2 -2
@@ -485,10 +485,10 @@ static struct clk_lookup lookups[] = {
 
 int __init mx35_clocks_init()
 {
-	unsigned int ll = 0;
+	unsigned int cgr2 = 3 << 26, cgr3 = 0;
 
 #if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_DEBUG_ICEDCC)
-	ll = (3 << 16);
+	cgr2 |= (3 << 16);

Please remove the parentheses here ...

Quoted 8 lines
arch/arm/mach-mx3/clock-imx35.c +0 -1
 #endif
 
 	clkdev_add_table(lookups, ARRAY_SIZE(lookups));

@@ -499,8 +499,20 @@ int __init mx35_clocks_init()
 	__raw_writel((3 << 18), CCM_BASE + CCM_CGR0);
 	__raw_writel((3 << 2) | (3 << 4) | (3 << 6) | (3 << 8) | (3 << 16),
 			CCM_BASE + CCM_CGR1);
-	__raw_writel((3 << 26) | ll, CCM_BASE + CCM_CGR2);

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 510

Quoted 10 lines
arch/arm/mach-mx3/clock-imx35.c +9 -1
-	__raw_writel(0, CCM_BASE + CCM_CGR3);
+
+	/*
+	 * Check if we came up in internal boot mode. If yes, we need some
+	 * extra clocks turned on, otherwise the MX35 boot ROM code will
+	 * hang after a watchdog reset.
+	 */
+	if (!(__raw_readl(CCM_BASE + CCM_RCSR) & (3 << 10))) {
+		/* Additionally turn on UART0, SCC, and IIM clocks */
+		cgr2 |= (3 << 16) | (3 << 4);

... and here ...

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 511

Quoted 1 line
arch/arm/mach-mx3/clock-imx35.c +1 -0
+		cgr3 |= (3 << 2);

... and here.

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 515

Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +4 -0
+	}
+
+	__raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+	__raw_writel(cgr3, CCM_BASE + CCM_CGR3);

Note that my question concerning the UART clock was less about UART1 vs UART0 but more if the ROM really needs a UART clock. And I prefer using the name used in the manual, so UART1 please.

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
author
#2 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

Quoted 24 lines · Uwe Kleine-K?nig

Hello Hans,

Quoted 21 lines · Hans J. Koch

If a watchdog reset occurs after booting in internal boot mode, the i.MX35 won't boot anymore. The boot ROM code seems to assume that some clocks are turned on (they are after a power-on reset). This patch turns on the necessary clocks.

Signed-off-by Hans J. Koch <[email protected]>
---
 arch/arm/mach-mx3/clock-imx35.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
arch/arm/mach-mx3/clock-imx35.c +1 -1
index d3af0fd..e619bdc 100644
@@ -485,10 +485,10 @@ static struct clk_lookup lookups[] = {
 
 int __init mx35_clocks_init()
 {
-	unsigned int ll = 0;
+	unsigned int cgr2 = 3 << 26, cgr3 = 0;
Quoted 4 lines

arch/arm/mach-mx3/clock-imx35.c static struct clk_lookup lookups[] = { · line 491

Quoted 3 lines
arch/arm/mach-mx3/clock-imx35.c +1 -1
 #if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_DEBUG_ICEDCC)
-	ll = (3 << 16);
+	cgr2 |= (3 << 16);

Please remove the parentheses here ...

I left these in to match the coding style of the rest. IMO, it makes it easier to read. Since you're going to replace all this with #defines, it doesn't matter anyway. But I won't argue. I remove them.

Quoted 20 lines
Quoted 9 lines
arch/arm/mach-mx3/clock-imx35.c +0 -1
 #endif
 
 	clkdev_add_table(lookups, ARRAY_SIZE(lookups));

@@ -499,8 +499,20 @@ int __init mx35_clocks_init()
 	__raw_writel((3 << 18), CCM_BASE + CCM_CGR0);
 	__raw_writel((3 << 2) | (3 << 4) | (3 << 6) | (3 << 8) | (3 << 16),
 			CCM_BASE + CCM_CGR1);
-	__raw_writel((3 << 26) | ll, CCM_BASE + CCM_CGR2);

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 510

Quoted 10 lines
arch/arm/mach-mx3/clock-imx35.c +9 -1
-	__raw_writel(0, CCM_BASE + CCM_CGR3);
+
+	/*
+	 * Check if we came up in internal boot mode. If yes, we need some
+	 * extra clocks turned on, otherwise the MX35 boot ROM code will
+	 * hang after a watchdog reset.
+	 */
+	if (!(__raw_readl(CCM_BASE + CCM_RCSR) & (3 << 10))) {
+		/* Additionally turn on UART0, SCC, and IIM clocks */
+		cgr2 |= (3 << 16) | (3 << 4);

... and here ...

Quoted 8 lines

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 511

Quoted 1 line
arch/arm/mach-mx3/clock-imx35.c +1 -0
+		cgr3 |= (3 << 2);

... and here.

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 515

Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +4 -0
+	}
+
+	__raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+	__raw_writel(cgr3, CCM_BASE + CCM_CGR3);

Note that my question concerning the UART clock was less about UART1 vs UART0 but more if the ROM really needs a UART clock.

In my tests on an mx35pdk board, I found these three clocks being the minimum set of additional clocks that need to be turned on. That means, if you turn off any of the three, it won't boot anymore.

Quoted 2 lines

And I prefer using
the name used in the manual, so UART1 please.

OK.

-v3 will follow soon.

Thanks,
Hans

author
#3 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

Quoted 11 lines · Hans J. Koch
Quoted 6 lines · Uwe Kleine-K?nig

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 515

Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +4 -0
+	}
+
+	__raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+	__raw_writel(cgr3, CCM_BASE + CCM_CGR3);

Note that my question concerning the UART clock was less about UART1 vs UART0 but more if the ROM really needs a UART clock.

In my tests on an mx35pdk board, I found these three clocks being the minimum set of additional clocks that need to be turned on. That means, if you turn off any of the three, it won't boot anymore.

For my tests, I didn't fully boot the kernel but inserted a while(1) after the clock init sequence. I had the wachdog already initialized in the bootloader, so after a few seconds it would boot again - or not.

If you fully boot the kernel, then the driver will probably switch on the UART1 clock, so you don't notice the effect anymore. But the kernel can hang _before_ drivers come up, and it should still work. That's what hardware watchdogs are for.

As a sidenote, it should be clear that this patch is a workaround for a serious chip bug of the MX35. The MX35 promises a watchdog that cannot be turned off once triggered. That's true, but of course, in case of a kernel crash, the watchdog has to be able to reliably reboot the system. That is not the case. Any driver going wild and writing a zero to the wrong register can easily turn the CPU into a desperate piece of silicon as soon as the watchdog reset occurs.

Also note that this also affects a normal system reboot using arch_reset() since that one also uses the hardware watchdog.

Thanks,
Hans

#4 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

Quoted 24 lines · Hans J. Koch
Quoted 11 lines · Hans J. Koch
Quoted 6 lines · Uwe Kleine-K?nig

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 515

Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +4 -0
+	}
+
+	__raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+	__raw_writel(cgr3, CCM_BASE + CCM_CGR3);

Note that my question concerning the UART clock was less about UART1 vs UART0 but more if the ROM really needs a UART clock.

In my tests on an mx35pdk board, I found these three clocks being the minimum set of additional clocks that need to be turned on. That means, if you turn off any of the three, it won't boot anymore.

For my tests, I didn't fully boot the kernel but inserted a while(1) after the clock init sequence. I had the wachdog already initialized in the bootloader, so after a few seconds it would boot again - or not.

If you fully boot the kernel, then the driver will probably switch on the UART1 clock, so you don't notice the effect anymore. But the kernel can hang _before_ drivers come up, and it should still work. That's what hardware watchdogs are for.

As a sidenote, it should be clear that this patch is a workaround for a serious chip bug of the MX35.

#5 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

Hi Uwe,

Le 13/08/2010 10:17, Uwe Kleine-K?nig a ?crit :

Quoted 9 lines · Hans J. Koch
Quoted 3 lines

As a sidenote, it should be clear that this patch is a workaround for a serious chip bug of the MX35.
From the things you wrote up to now it's possible that this is a

bootloader issue, isn't it.

I don't know exactly what "internal boot mode" means, but assuming the ROM code doesn't make any output to UART1 at least this clock smells like a bootloader problem.

internal bootmode is the mode where the CPU boots on the internal rom code which samples some GPIO to set the fuses and select which boot media to use. So ROM code is executed before loading the bootloader (barebox in my case) and that's where the CPU hangs.

I think we have the exact same problem using i.MX35 (and maybe also on i.MX51), but I need to find time to do more tests to confirm this.

Eric

author
#6 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

In reply to Uwe Kleine-König
Quoted 23 lines · Uwe Kleine-K?nig
Quoted 22 lines · Hans J. Koch
Quoted 11 lines · Hans J. Koch
Quoted 6 lines · Uwe Kleine-K?nig

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 515

Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +4 -0
+	}
+
+	__raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+	__raw_writel(cgr3, CCM_BASE + CCM_CGR3);

Note that my question concerning the UART clock was less about UART1 vs UART0 but more if the ROM really needs a UART clock.

In my tests on an mx35pdk board, I found these three clocks being the minimum set of additional clocks that need to be turned on. That means, if you turn off any of the three, it won't boot anymore.

For my tests, I didn't fully boot the kernel but inserted a while(1) after the clock init sequence. I had the wachdog already initialized in the bootloader, so after a few seconds it would boot again - or not.

If you fully boot the kernel, then the driver will probably switch on the UART1 clock, so you don't notice the effect anymore. But the kernel can hang _before_ drivers come up, and it should still work. That's what hardware watchdogs are for.

Quoted 4 lines
Quoted 2 lines

As a sidenote, it should be clear that this patch is a workaround for a serious chip bug of the MX35.

From the things you wrote up to now it's possible that this is a bootloader issue, isn't it.

I discussed that with John. It's at least very unlikely. Given the fact that it works in external boot mode, a bootloader bug had to be in the code path that's different for the two modes. That code path is very small, and there's no hint that the clocks in question are used there.

Quoted 4 lines

I don't know exactly what "internal boot mode" means, but assuming the ROM code doesn't make any output to UART1 at least this clock smells like a bootloader problem.

No. When the bootloader uses the UART for the first time it is already in code that's identical for both boot modes.

Quoted 2 lines

If you're conviced this is a chip problem,
did you consider to contact FSL about it?

Yes, of course. John does that at the moment.

Thanks,
Hans

#7 lore raw

PATCH v2 1/1 mx35: Fix boot ROM hang in internal boot mode

Hello Hans,

Quoted 39 lines · Hans J. Koch
Quoted 23 lines · Uwe Kleine-K?nig
Quoted 22 lines · Hans J. Koch
Quoted 11 lines · Hans J. Koch
Quoted 6 lines · Uwe Kleine-K?nig

arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 515

Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +4 -0
+	}
+
+	__raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+	__raw_writel(cgr3, CCM_BASE + CCM_CGR3);

Note that my question concerning the UART clock was less about UART1 vs UART0 but more if the ROM really needs a UART clock.

In my tests on an mx35pdk board, I found these three clocks being the minimum set of additional clocks that need to be turned on. That means, if you turn off any of the three, it won't boot anymore.

For my tests, I didn't fully boot the kernel but inserted a while(1) after the clock init sequence. I had the wachdog already initialized in the bootloader, so after a few seconds it would boot again - or not.

If you fully boot the kernel, then the driver will probably switch on the UART1 clock, so you don't notice the effect anymore. But the kernel can hang _before_ drivers come up, and it should still work. That's what hardware watchdogs are for.

Quoted 4 lines
Quoted 2 lines

As a sidenote, it should be clear that this patch is a workaround for a serious chip bug of the MX35.

From the things you wrote up to now it's possible that this is a bootloader issue, isn't it.

I discussed that with John. It's at least very unlikely. Given the fact that it works in external boot mode, a bootloader bug had to be in the code path that's different for the two modes. That code path is very small, and there's no hint that the clocks in question are used there.

Quoted 4 lines

I don't know exactly what "internal boot mode" means, but assuming the ROM code doesn't make any output to UART1 at least this clock smells like a bootloader problem.

Quoted 7 lines

No. When the bootloader uses the UART for the first time it is already in code that's identical for both boot modes.

Quoted 2 lines

If you're conviced this is a chip problem,
did you consider to contact FSL about it?

Yes, of course. John does that at the moment.

OK, great.

Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |