* [PATCH] mx35: Fix boot ROM hang in internal boot mode @ 2010-08-11 0:19 Hans J. Koch 2010-08-11 5:27 ` Uwe Kleine-König 0 siblings, 1 reply; 4+ messages in thread From: Hans J. Koch @ 2010-08-11 0:19 UTC (permalink / raw) To: linux-arm-kernel 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 <hjk@linutronix.de> --- arch/arm/mach-mx3/clock-imx35.c | 20 +++++++++++++++++--- 1 files changed, 17 insertions(+), 3 deletions(-) diff --git a/arch/arm/mach-mx3/clock-imx35.c b/arch/arm/mach-mx3/clock-imx35.c index d3af0fd..671acb6 100644 --- a/arch/arm/mach-mx3/clock-imx35.c +++ b/arch/arm/mach-mx3/clock-imx35.c @@ -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); -- 1.7.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] mx35: Fix boot ROM hang in internal boot mode 2010-08-11 0:19 [PATCH] mx35: Fix boot ROM hang in internal boot mode Hans J. Koch @ 2010-08-11 5:27 ` Uwe Kleine-König 2010-08-11 23:20 ` Hans J. Koch 0 siblings, 1 reply; 4+ messages in thread From: Uwe Kleine-König @ 2010-08-11 5:27 UTC (permalink / raw) To: linux-arm-kernel Hello Hans, On Wed, Aug 11, 2010 at 02:19:33AM +0200, Hans J. Koch wrote: > 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 <hjk@linutronix.de> > --- > arch/arm/mach-mx3/clock-imx35.c | 20 +++++++++++++++++--- > 1 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-mx3/clock-imx35.c b/arch/arm/mach-mx3/clock-imx35.c > index d3af0fd..671acb6 100644 > --- a/arch/arm/mach-mx3/clock-imx35.c > +++ b/arch/arm/mach-mx3/clock-imx35.c > @@ -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 <nitpick> /* on an extra line please. </nitpick> > + * 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. > + if (__raw_readl(CCM_BASE + CCM_RCSR) & (3 << 10)) { Can you please introduce a symbol for (3 << 10)?. > + cgr2 = (3 << 26); > + cgr3 = 0; > + } else { > + /* Additionally turn on UART1, SCC, and IIM clocks */ really UART1? > + 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. ) > + > + __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); > + __raw_writel(cgr3, CCM_BASE + CCM_CGR3); > > mxc_timer_init(&gpt_clk, > MX35_IO_ADDRESS(MX35_GPT1_BASE_ADDR), MX35_INT_GPT); Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] mx35: Fix boot ROM hang in internal boot mode 2010-08-11 5:27 ` Uwe Kleine-König @ 2010-08-11 23:20 ` Hans J. Koch 2010-08-12 5:39 ` Uwe Kleine-König 0 siblings, 1 reply; 4+ messages in thread From: Hans J. Koch @ 2010-08-11 23:20 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 11, 2010 at 07:27:50AM +0200, Uwe Kleine-K?nig wrote: > Hello Hans, Hello Uwe, thanks for your review. A -v2 patch will follow soon. Some comments below. Thanks, Hans > > On Wed, Aug 11, 2010 at 02:19:33AM +0200, Hans J. Koch wrote: > > 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 <hjk@linutronix.de> > > --- > > arch/arm/mach-mx3/clock-imx35.c | 20 +++++++++++++++++--- > > 1 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm/mach-mx3/clock-imx35.c b/arch/arm/mach-mx3/clock-imx35.c > > index d3af0fd..671acb6 100644 > > --- a/arch/arm/mach-mx3/clock-imx35.c > > +++ b/arch/arm/mach-mx3/clock-imx35.c > > @@ -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 > <nitpick> /* on an extra line please. </nitpick> alright, if you like it... > > > + * 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. > > > + 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. > > > + 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. > > > + 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... > > 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. > ) > > > + > > + __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; > + } Well, OK, I like that, too. > + > + __raw_writel(cgr2, CCM_BASE + CCM_CGR2); > + __raw_writel(cgr3, CCM_BASE + CCM_CGR3); > > > > + __raw_writel(cgr3, CCM_BASE + CCM_CGR3); > > > > mxc_timer_init(&gpt_clk, > > MX35_IO_ADDRESS(MX35_GPT1_BASE_ADDR), MX35_INT_GPT); > > Thanks > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-K?nig | > Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] mx35: Fix boot ROM hang in internal boot mode 2010-08-11 23:20 ` Hans J. Koch @ 2010-08-12 5:39 ` Uwe Kleine-König 0 siblings, 0 replies; 4+ messages in thread From: Uwe Kleine-König @ 2010-08-12 5:39 UTC (permalink / raw) To: linux-arm-kernel Hello Hans, On Thu, Aug 12, 2010 at 01:20:14AM +0200, Hans J. Koch wrote: > > > diff --git a/arch/arm/mach-mx3/clock-imx35.c b/arch/arm/mach-mx3/clock-imx35.c > > > index d3af0fd..671acb6 100644 > > > --- a/arch/arm/mach-mx3/clock-imx35.c > > > +++ b/arch/arm/mach-mx3/clock-imx35.c > > > @@ -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 > > <nitpick> /* on an extra line please. </nitpick> > > alright, if you like it... > > > > > > + * 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. > > > > > > + 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. > > > > > > + 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. 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 -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-08-12 5:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-08-11 0:19 [PATCH] mx35: Fix boot ROM hang in internal boot mode Hans J. Koch 2010-08-11 5:27 ` Uwe Kleine-König 2010-08-11 23:20 ` Hans J. Koch 2010-08-12 5:39 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).