[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[John Ogness]

On 2010-08-13, Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:
>> 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.
>> 
>> As a sidenote, it should be clear that this patch is a workaround for
>> a serious chip bug of the MX35.
>
> If you're conviced this is a chip problem, did you consider to contact
> FSL about it?

Freescale has responded to my inqueries about the boot ROM issue:

MX51EVK, MX508ARM2, MX25PDK boards also do this. Freescale has no plans
to change the MX35 ROM code.

John

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Hans J. Koch]

On Fri, Sep 24, 2010 at 10:13:36AM +0200, John Ogness wrote:
> On 2010-08-13, Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:
> >> 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.
> >> 
> >> As a sidenote, it should be clear that this patch is a workaround for
> >> a serious chip bug of the MX35.
> >
> > If you're conviced this is a chip problem, did you consider to contact
> > FSL about it?
> 
> Freescale has responded to my inqueries about the boot ROM issue:
> 
> MX51EVK, MX508ARM2, MX25PDK boards also do this. Freescale has no plans
> to change the MX35 ROM code.

Sad thing. A hardware watchdog is there to protect you in situations where
things go wrong, so it must never assume that certain registers have certain
values.

If a hardware developer chose one of these CPUs for the following reasons

1) he needs internal bootmode to verify the bootloader
2) he needs the hardware watchdog to ensure reliable operation

he can simply not use these SoCs, because there are cases where the boot ROM
will hang until the next hardware reset or powercycle. If he uses such a CPU,
he needs additional hardware for an external watchdog.

Thanks,
Hans

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[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 <hjk@linutronix.de>
---
 arch/arm/mach-mx3/clock-imx35.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-mx3/clock-imx35.c b/arch/arm/mach-mx3/clock-imx35.c
index d3af0fd..e619bdc 100644
--- a/arch/arm/mach-mx3/clock-imx35.c
+++ b/arch/arm/mach-mx3/clock-imx35.c
@@ -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);
-- 
1.7.1

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Uwe Kleine-König]

Hello Hans,

On Thu, Aug 12, 2010 at 01:37:21AM +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, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-mx3/clock-imx35.c b/arch/arm/mach-mx3/clock-imx35.c
> index d3af0fd..e619bdc 100644
> --- a/arch/arm/mach-mx3/clock-imx35.c
> +++ b/arch/arm/mach-mx3/clock-imx35.c
> @@ -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 ...

>  #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);
... and here ...
> +		cgr3 |= (3 << 2);
... and here.
> +	}
> +
> +	__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

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

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Hans J. Koch]

On Thu, Aug 12, 2010 at 07:57:53AM +0200, Uwe Kleine-K?nig wrote:
> Hello Hans,
> 
> On Thu, Aug 12, 2010 at 01:37:21AM +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, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mx3/clock-imx35.c b/arch/arm/mach-mx3/clock-imx35.c
> > index d3af0fd..e619bdc 100644
> > --- a/arch/arm/mach-mx3/clock-imx35.c
> > +++ b/arch/arm/mach-mx3/clock-imx35.c
> > @@ -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 ...

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.

> 
> >  #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);
> ... and here ...
> > +		cgr3 |= (3 << 2);
> ... and here.
> > +	}
> > +
> > +	__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.

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

OK.

-v3 will follow soon.

Thanks,
Hans

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Hans J. Koch]

On Thu, Aug 12, 2010 at 01:29:40PM +0200, Hans J. Koch wrote:
> On Thu, Aug 12, 2010 at 07:57:53AM +0200, Uwe Kleine-K?nig wrote:
> > > +	}
> > > +
> > > +	__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

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Uwe Kleine-König]

On Thu, Aug 12, 2010 at 02:15:04PM +0200, Hans J. Koch wrote:
> On Thu, Aug 12, 2010 at 01:29:40PM +0200, Hans J. Koch wrote:
> > On Thu, Aug 12, 2010 at 07:57:53AM +0200, Uwe Kleine-K?nig wrote:
> > > > +	}
> > > > +
> > > > +	__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.

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Eric Bénard]

Hi Uwe,

Le 13/08/2010 10:17, Uwe Kleine-K?nig a ?crit :
> On Thu, Aug 12, 2010 at 02:15:04PM +0200, Hans J. Koch wrote:
>> 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

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Hans J. Koch]

On Fri, Aug 13, 2010 at 10:17:20AM +0200, Uwe Kleine-K?nig wrote:
> On Thu, Aug 12, 2010 at 02:15:04PM +0200, Hans J. Koch wrote:
> > On Thu, Aug 12, 2010 at 01:29:40PM +0200, Hans J. Koch wrote:
> > > On Thu, Aug 12, 2010 at 07:57:53AM +0200, Uwe Kleine-K?nig wrote:
> > > > > +	}
> > > > > +
> > > > > +	__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.
> 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.

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

> 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

[PATCH v2] mx35: Fix boot ROM hang in internal boot mode

[Uwe Kleine-König]

Hello Hans,

On Fri, Aug 13, 2010 at 12:04:29PM +0200, Hans J. Koch wrote:
> On Fri, Aug 13, 2010 at 10:17:20AM +0200, Uwe Kleine-K?nig wrote:
> > On Thu, Aug 12, 2010 at 02:15:04PM +0200, Hans J. Koch wrote:
> > > On Thu, Aug 12, 2010 at 01:29:40PM +0200, Hans J. Koch wrote:
> > > > On Thu, Aug 12, 2010 at 07:57:53AM +0200, Uwe Kleine-K?nig wrote:
> > > > > > +	}
> > > > > > +
> > > > > > +	__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.
> > 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.
> 
> > 
> > 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.
> 
> > 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

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