mx35: Fix boot ROM hang in internal boot mode
Subsystems (3)
PATCH 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.
--- arch/arm/mach-mx3/clock-imx35.c | 20 +++++++++++++++++--- 1 files changed, 17 insertions(+), 3 deletions(-)
arch/arm/mach-mx3/clock-imx35.c +17 -3
@@ -485,7 +485,7 @@ static struct clk_lookup lookups[] = {int __init mx35_clocks_init () { -unsigned int ll =0 ; +unsigned int ll =0 , cgr2, cgr3;#if defined(CONFIG_DEBUG_LL ) && !defined(CONFIG_DEBUG_ICEDCC ) ll = (3 <<16 );
@@ -499,8 +499,22 @@ 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 )) { + cgr2 = (3 <<26 ); + cgr3 =0 ; + }else { +/* Additionally turn on UART1, SCC, and IIM clocks */ + cgr2 = (3 <<26 ) | (3 <<16 ) | (3 <<4 ); + cgr3 = (3 <<2 ); + } + +__raw_writel (cgr2 | ll,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
PATCH 1/1 mx35: Fix boot ROM hang in internal boot mode
Hello Hans,
Quoted 24 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, 17 insertions(+), 3 deletions(-)arch/arm/mach-mx3/clock-imx35.c +1
-1@@ -485,7 +485,7 @@ static struct clk_lookup lookups[] = {int __init mx35_clocks_init () { -unsigned int ll =0 ; +unsigned int ll =0 , cgr2, cgr3;#if defined(CONFIG_DEBUG_LL ) && !defined(CONFIG_DEBUG_ICEDCC ) ll = (3 <<16 );
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 503
Quoted 7 lines
arch/arm/mach-mx3/clock-imx35.c +2
-2@@ -499,8 +499,22 @@ 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
<nitpick> /* on an extra line please. </nitpick>
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 506
Quoted 3 lines
arch/arm/mach-mx3/clock-imx35.c +3
-0+* extra clocks turned on, otherwise the MX35 boot ROM code will +* hang after a watchdog reset. +*/
Better move the comment into the else branch below.
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 507
Quoted 1 line
arch/arm/mach-mx3/clock-imx35.c +1
-0+if (__raw_readl (CCM_BASE +CCM_RCSR ) & (3 <<10 )) {
Can you please introduce a symbol for (3 << 10)?.
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 511
Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +4
-0+ cgr2 = (3 <<26 ); + cgr3 =0 ; + }else { +/* Additionally turn on UART1, SCC, and IIM clocks */
really UART1?
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 514
Quoted 3 lines
arch/arm/mach-mx3/clock-imx35.c +3
-0+ cgr2 = (3 <<26 ) | (3 <<16 ) | (3 <<4 ); + cgr3 = (3 <<2 ); + }
I would prefer having symbolic names here, too, but this affects not only your patch. Will put it on my todo list.
Is it sensible to assume that the machine will boot in internal boot mode again iff it came up this time in internal boot mode? (I assume yes, still I would reword the comment. Something like the following?:
In internal boot mode the MX35 boot ROM code seems to assume that UART1, SCC, and IIM clocks are on and hangs if they are not. These are enabled by hardware after a power-on reset, but not after a watchdog reset. So enable them here.
)
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 516
Quoted 2 lines
arch/arm/mach-mx3/clock-imx35.c +2
-0+ +__raw_writel (cgr2 | ll,CCM_BASE +CCM_CGR2 );
hmm, can you make this:
- unsigned int ll = 0;
+ unsigned int cgr2 = 3 << 26, cgr3 = 0;
#if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_DEBUG_ICEDCC)
cgr2 |= 3 << 16;
#endif
...
-...
+ if (!__raw_readl(CCM_BASE + CCM_RCSR) & (3 << 10)) {
+ /*
+ * ...
+ */
+ cgr2 |= 3 << 16 | 3 << 4;
+ cgr3 |= 3 << 2;
+ }
+
+ __raw_writel(cgr2, CCM_BASE + CCM_CGR2);
+ __raw_writel(cgr3, CCM_BASE + CCM_CGR3);
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 520
Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +1
-0+__raw_writel (cgr3,CCM_BASE +CCM_CGR3 );mxc_timer_init (&gpt_clk,MX35_IO_ADDRESS (MX35_GPT1_BASE_ADDR ),MX35_INT_GPT );
Thanks
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 1/1 mx35: Fix boot ROM hang in internal boot mode
Quoted 1 line · Uwe Kleine-K?nig
Hello Hans,
Hello Uwe,
thanks for your review. A -v2 patch will follow soon. Some comments below.
Thanks,
Hans
Quoted 23 lines · Hans J. Koch
Quoted 21 lines
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, 17 insertions(+), 3 deletions(-)arch/arm/mach-mx3/clock-imx35.c +1
-1@@ -485,7 +485,7 @@ static struct clk_lookup lookups[] = {int __init mx35_clocks_init () { -unsigned int ll =0 ; +unsigned int ll =0 , cgr2, cgr3;
Quoted 11 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 503
Quoted 10 lines
arch/arm/mach-mx3/clock-imx35.c +2
-2#if defined(CONFIG_DEBUG_LL ) && !defined(CONFIG_DEBUG_ICEDCC ) ll = (3 <<16 );@@ -499,8 +499,22 @@ 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 <nitpick> /* on an extra line please. </nitpick>
alright, if you like it...
Quoted 5 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 506
Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +3
-0+* extra clocks turned on, otherwise the MX35 boot ROM code will +* hang after a watchdog reset. +*/ Better move the comment into the else branch below.
There's no else branch anymore. Before the "if" the comment says what's being checked, and in the if branch the comment says what's being done.
Quoted 3 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 507
Quoted 2 lines
arch/arm/mach-mx3/clock-imx35.c +1
-0+if (__raw_readl (CCM_BASE +CCM_RCSR ) & (3 <<10 )) {Can you please introduce a symbol for (3 << 10)?.
I just wanted to follow the style of the rest of the file ;-) Since you say you've got those #defines on your todo list, I better leave that to you.
Quoted 6 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 511
Quoted 5 lines
arch/arm/mach-mx3/clock-imx35.c +4
-0+ cgr2 = (3 <<26 ); + cgr3 =0 ; + }else { +/* Additionally turn on UART1, SCC, and IIM clocks */ really UART1?
Well, stupid reference manual. They name the UARTs 1,2,3 instead of 0,1,2. That also leads to lines like:
_REGISTER_CLOCK("imx-uart.0", NULL, uart1_clk)
I renamed it UART0 in the comment to make it more clear.
Quoted 6 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 514
Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +3
-0+ cgr2 = (3 <<26 ) | (3 <<16 ) | (3 <<4 ); + cgr3 = (3 <<2 ); + }I would prefer having symbolic names here, too, but this affects not only your patch. Will put it on my todo list.
Good idea, thank you. It's a PITA to need that >3000 pages reference manual to read the code...
Quoted 20 lines
Is it sensible to assume that the machine will boot in internal boot mode again iff it came up this time in internal boot mode? (I assume yes, still I would reword the comment. Something like the following?:
In internal boot mode the MX35 boot ROM code seems to assume that UART1, SCC, and IIM clocks are on and hangs if they are not. These are enabled by hardware after a power-on reset, but not after a watchdog reset. So enable them here.
)arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 516
Quoted 2 lines
arch/arm/mach-mx3/clock-imx35.c +2
-0+ +__raw_writel (cgr2 | ll,CCM_BASE +CCM_CGR2 );hmm, can you make this:
(quoted diff) +1
-1- unsigned int ll = 0; + unsigned int cgr2 = 3 << 26, cgr3 = 0; #if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_DEBUG_ICEDCC) cgr2 |= 3 << 16;
Review
Quoted 10 lines
(quoted diff) +7
-1#endif -... + if (!__raw_readl(CCM_BASE + CCM_RCSR) & (3 << 10)) { + /* + * ... + */ + cgr2 |= 3 << 16 | 3 << 4; + cgr3 |= 3 << 2; + }
Well, OK, I like that, too.
Quoted 16 lines
arch/arm/mach-mx3/clock-imx35.c +3
-0+ +__raw_writel (cgr2,CCM_BASE +CCM_CGR2 ); +__raw_writel (cgr3,CCM_BASE +CCM_CGR3 );arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 520
Quoted 6 lines
arch/arm/mach-mx3/clock-imx35.c +1
-0+__raw_writel (cgr3,CCM_BASE +CCM_CGR3 );mxc_timer_init (&gpt_clk,MX35_IO_ADDRESS (MX35_GPT1_BASE_ADDR ),MX35_INT_GPT );Thanks
UweSignature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
PATCH 1/1 mx35: Fix boot ROM hang in internal boot mode
Hello Hans,
Quoted 31 lines · Hans J. Koch
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-mx3/clock-imx35.c +1
-1@@ -485,7 +485,7 @@ static struct clk_lookup lookups[] = {int __init mx35_clocks_init () { -unsigned int ll =0 ; +unsigned int ll =0 , cgr2, cgr3;Quoted 11 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 503
Quoted 10 lines
arch/arm/mach-mx3/clock-imx35.c +2
-2#if defined(CONFIG_DEBUG_LL ) && !defined(CONFIG_DEBUG_ICEDCC ) ll = (3 <<16 );@@ -499,8 +499,22 @@ 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 <nitpick> /* on an extra line please. </nitpick>
alright, if you like it...
Quoted 5 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 506
Quoted 4 lines
arch/arm/mach-mx3/clock-imx35.c +3
-0+* extra clocks turned on, otherwise the MX35 boot ROM code will +* hang after a watchdog reset. +*/ Better move the comment into the else branch below.
Quoted 8 lines
There's no else branch anymore. Before the "if" the comment says what's being checked, and in the if branch the comment says what's being done.
Quoted 3 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 507
Quoted 2 lines
arch/arm/mach-mx3/clock-imx35.c +1
-0+if (__raw_readl (CCM_BASE +CCM_RCSR ) & (3 <<10 )) {Can you please introduce a symbol for (3 << 10)?.
I just wanted to follow the style of the rest of the file ;-)
:-)
Quoted 10 lines
Since you say you've got those #defines on your todo list, I better leave that to you.
Quoted 6 lines
arch/arm/mach-mx3/clock-imx35.c int __init mx35_clocks_init() · line 511
Quoted 5 lines
arch/arm/mach-mx3/clock-imx35.c +4
-0+ cgr2 = (3 <<26 ); + cgr3 =0 ; + }else { +/* Additionally turn on UART1, SCC, and IIM clocks */ really UART1?
Quoted 6 lines
Well, stupid reference manual. They name the UARTs 1,2,3 instead of 0,1,2. That also leads to lines like:
_REGISTER_CLOCK("imx-uart.0", NULL, uart1_clk)I renamed it UART0 in the comment to make it more clear.
Actually I already thought about fixing that. (On the software side obviously.) There are only little problems and one bigger to make them match. The bigger one is that it breaks people's kernel commandlines that usually say:
console=ttymxc0...
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |