[U-Boot] [PATCH 1/3] board_r: move initr_serial to be called before initr_watchdog

[U-Boot] [PATCH 1/3] board_r: move initr_serial to be called before initr_watchdog

[Weijie Gao]

The initr_watchdog is currently placed before initr_serial. The
initr_watchdog calls printf and printf finally calls ops->putc of a serial
driver.

However, gd->cur_serial_dev points to a udevice allocated in board_f. The
gd->cur_serial_dev->driver->ops->putc points the the code region before
relocation.

Some serial drivers call WATCHDOG_RESET() in ops->putc. When DM is enabled
for watchdog, watchdog_reset() is called. watchdog_reset() calls get_timer
to get current timer.

On some platforms the timer driver is also a DM driver. initr_watchdog is
placed right after initr_dm, which means the timer driver hasn't been
initialized. So dm_timer_init() is called. To create a new udevice, calloc
is called.

However start from ops->putc, u-boot execution flow is redirected into the
memory region before relocation (board_f). In board_f, dlmalloc hasn't
been initialized. The call to calloc will fail, and this will cause DM to
print out an error message, and it will call printf again, causing
recursive error outputs.

This patch places initr_serial before initr_watchdog to solve this issue.

Cc: Stefan Roese <sr@denx.de>
Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
---
 common/board_r.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/board_r.c b/common/board_r.c
index 150e8cd424..a298146c2b 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -678,6 +678,7 @@ static init_fnc_t init_sequence_r[] = {
 #ifdef CONFIG_DM
 	initr_dm,
 #endif
+	initr_serial,
 #if defined(CONFIG_WDT)
 	initr_watchdog,
 #endif
@@ -698,7 +699,6 @@ static init_fnc_t init_sequence_r[] = {
 	efi_memory_init,
 #endif
 	stdio_init_tables,
-	initr_serial,
 	initr_announce,
 	INIT_FUNC_WATCHDOG_RESET
 #ifdef CONFIG_NEEDS_MANUAL_RELOC
-- 
2.18.0

[U-Boot] [PATCH 1/3] board_r: move initr_serial to be called before initr_watchdog

[Stefan Roese]

On 16.05.19 08:48, Weijie Gao wrote:
> The initr_watchdog is currently placed before initr_serial. The
> initr_watchdog calls printf and printf finally calls ops->putc of a serial
> driver.
> 
> However, gd->cur_serial_dev points to a udevice allocated in board_f. The
> gd->cur_serial_dev->driver->ops->putc points the the code region before
> relocation.
> 
> Some serial drivers call WATCHDOG_RESET() in ops->putc. When DM is enabled
> for watchdog, watchdog_reset() is called. watchdog_reset() calls get_timer
> to get current timer.
> 
> On some platforms the timer driver is also a DM driver. initr_watchdog is
> placed right after initr_dm, which means the timer driver hasn't been
> initialized. So dm_timer_init() is called. To create a new udevice, calloc
> is called.
> 
> However start from ops->putc, u-boot execution flow is redirected into the
> memory region before relocation (board_f). In board_f, dlmalloc hasn't
> been initialized. The call to calloc will fail, and this will cause DM to
> print out an error message, and it will call printf again, causing
> recursive error outputs.
> 
> This patch places initr_serial before initr_watchdog to solve this issue.
> 
> Cc: Stefan Roese <sr@denx.de>
> Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
> Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> ---
>   common/board_r.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/board_r.c b/common/board_r.c
> index 150e8cd424..a298146c2b 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -678,6 +678,7 @@ static init_fnc_t init_sequence_r[] = {
>   #ifdef CONFIG_DM
>   	initr_dm,
>   #endif
> +	initr_serial,
>   #if defined(CONFIG_WDT)
>   	initr_watchdog,
>   #endif
> @@ -698,7 +699,6 @@ static init_fnc_t init_sequence_r[] = {
>   	efi_memory_init,
>   #endif
>   	stdio_init_tables,
> -	initr_serial,
>   	initr_announce,

I'm not 100% sure, if moving initr_serial before stdio_init_tables and
other functions is safe. Perhaps its better to just move initr_watchdog
down a bit, perhaps after initr_announce?

What do you think?

Thanks,
Stefan

BTW: Somehow your Cc'ing me did not reach me directly. I only found the
patch on the list.

[U-Boot] [PATCH 1/3] board_r: move initr_serial to be called before initr_watchdog

[Weijie Gao]

On Thu, 2019-05-16 at 10:21 +0200, Stefan Roese wrote:
> On 16.05.19 08:48, Weijie Gao wrote:
> > The initr_watchdog is currently placed before initr_serial. The
> > initr_watchdog calls printf and printf finally calls ops->putc of a serial
> > driver.
> > 
> > However, gd->cur_serial_dev points to a udevice allocated in board_f. The
> > gd->cur_serial_dev->driver->ops->putc points the the code region before
> > relocation.
> > 
> > Some serial drivers call WATCHDOG_RESET() in ops->putc. When DM is enabled
> > for watchdog, watchdog_reset() is called. watchdog_reset() calls get_timer
> > to get current timer.
> > 
> > On some platforms the timer driver is also a DM driver. initr_watchdog is
> > placed right after initr_dm, which means the timer driver hasn't been
> > initialized. So dm_timer_init() is called. To create a new udevice, calloc
> > is called.
> > 
> > However start from ops->putc, u-boot execution flow is redirected into the
> > memory region before relocation (board_f). In board_f, dlmalloc hasn't
> > been initialized. The call to calloc will fail, and this will cause DM to
> > print out an error message, and it will call printf again, causing
> > recursive error outputs.
> > 
> > This patch places initr_serial before initr_watchdog to solve this issue.
> > 
> > Cc: Stefan Roese <sr@denx.de>
> > Reviewed-by: Ryder Lee <ryder.lee@mediatek.com>
> > Signed-off-by: Weijie Gao <weijie.gao@mediatek.com>
> > ---
> >   common/board_r.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/board_r.c b/common/board_r.c
> > index 150e8cd424..a298146c2b 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -678,6 +678,7 @@ static init_fnc_t init_sequence_r[] = {
> >   #ifdef CONFIG_DM
> >   	initr_dm,
> >   #endif
> > +	initr_serial,
> >   #if defined(CONFIG_WDT)
> >   	initr_watchdog,
> >   #endif
> > @@ -698,7 +699,6 @@ static init_fnc_t init_sequence_r[] = {
> >   	efi_memory_init,
> >   #endif
> >   	stdio_init_tables,
> > -	initr_serial,
> >   	initr_announce,
> 
> I'm not 100% sure, if moving initr_serial before stdio_init_tables and
> other functions is safe. Perhaps its better to just move initr_watchdog
> down a bit, perhaps after initr_announce?
> 
> What do you think?
> 
> Thanks,
> Stefan
> 
> BTW: Somehow your Cc'ing me did not reach me directly. I only found the
> patch on the list.

I'll change it to move initr_watchdog only.

My company's mail server reported delivery failure every time I Cc'ed
you and some others. I have no idea.

[U-Boot] [PATCH 1/3] board_r: move initr_serial to be called before initr_watchdog

[Suniel Mahesh]

Hi,

Is this patch under review ?

facing the same problem on TI Based AM33XX platform. 



--
Sent from: http://u-boot.10912.n7.nabble.com/