diff mbox series

[06/16] arm: stm32mp: spl: display error in board_init_f

Message ID 20200331180330.6.I41a641a07fd12da45b392920fc3407e608926396@changeid
State Superseded
Delegated to: Patrick Delaunay
Headers show
Series [01/16] arm: stm32mp: update dependency for STM32_ETZPC | expand

Commit Message

Patrick Delaunay March 31, 2020, 4:04 p.m. UTC
Update board_init_f and try to display error message
when console is available.

This patch adds trace to debug a spl boot issue when DEBUG
and DEBUG_UART is not activated, after uart probe.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 arch/arm/mach-stm32mp/spl.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

Patrice CHOTARD April 1, 2020, 7:43 a.m. UTC | #1
Hi Patrick

On 3/31/20 6:04 PM, Patrick Delaunay wrote:
> Update board_init_f and try to display error message
> when console is available.
>
> This patch adds trace to debug a spl boot issue when DEBUG
> and DEBUG_UART is not activated, after uart probe.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  arch/arm/mach-stm32mp/spl.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
> index ca4231cd0d..dfdb5bb7e9 100644
> --- a/arch/arm/mach-stm32mp/spl.c
> +++ b/arch/arm/mach-stm32mp/spl.c
> @@ -79,37 +79,36 @@ void spl_display_print(void)
>  void board_init_f(ulong dummy)
>  {
>  	struct udevice *dev;
> -	int ret;
> +	int ret, clk, reset, pinctrl;
>  
>  	arch_cpu_init();
>  
>  	ret = spl_early_init();
>  	if (ret) {
> -		debug("spl_early_init() failed: %d\n", ret);
> +		debug("%s: spl_early_init() failed: %d\n", __func__, ret);
>  		hang();
>  	}
>  
> -	ret = uclass_get_device(UCLASS_CLK, 0, &dev);
> -	if (ret) {
> -		debug("Clock init failed: %d\n", ret);
> -		return;
> -	}
> +	clk = uclass_get_device(UCLASS_CLK, 0, &dev);
> +	if (clk)
> +		debug("%s: Clock init failed: %d\n", __func__, clk);
>  
> -	ret = uclass_get_device(UCLASS_RESET, 0, &dev);
> -	if (ret) {
> -		debug("Reset init failed: %d\n", ret);
> -		return;
> -	}
> +	reset = uclass_get_device(UCLASS_RESET, 0, &dev);
> +	if (reset)
> +		debug("%s: Reset init failed: %d\n", __func__, reset);
>  
> -	ret = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> -	if (ret) {
> -		debug("%s: Cannot find pinctrl device\n", __func__);
> -		return;
> -	}
> +	pinctrl = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> +	if (pinctrl)
> +		debug("%s: Cannot find pinctrl device: %d\n",
> +		      __func__, pinctrl);
>  
>  	/* enable console uart printing */
>  	preloader_console_init();
>  
> +	if (clk || reset || pinctrl)
> +		printf("%s: probe failed clk=%d reset=%d pinctrl=%d\n",
> +		       __func__, clk, reset, pinctrl);
> +
>  	ret = uclass_get_device(UCLASS_RAM, 0, &dev);
>  	if (ret) {
>  		printf("DRAM init failed: %d\n", ret);

Reviewed-by: Patrice Chotard <patrice.chotard@st.com>

Thanks
Wolfgang Denk April 1, 2020, 11:30 a.m. UTC | #2
Dear Patrick Delaunay,

In message <20200331180330.6.I41a641a07fd12da45b392920fc3407e608926396@changeid> you wrote:
> Update board_init_f and try to display error message
> when console is available.
>
> This patch adds trace to debug a spl boot issue when DEBUG
> and DEBUG_UART is not activated, after uart probe.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  arch/arm/mach-stm32mp/spl.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
> index ca4231cd0d..dfdb5bb7e9 100644
> --- a/arch/arm/mach-stm32mp/spl.c
> +++ b/arch/arm/mach-stm32mp/spl.c
> @@ -79,37 +79,36 @@ void spl_display_print(void)
>  void board_init_f(ulong dummy)
>  {
>  	struct udevice *dev;
> -	int ret;
> +	int ret, clk, reset, pinctrl;
>  
>  	arch_cpu_init();
>  
>  	ret = spl_early_init();
>  	if (ret) {
> -		debug("spl_early_init() failed: %d\n", ret);
> +		debug("%s: spl_early_init() failed: %d\n", __func__, ret);
>  		hang();
>  	}
>  
> -	ret = uclass_get_device(UCLASS_CLK, 0, &dev);
> -	if (ret) {
> -		debug("Clock init failed: %d\n", ret);
> -		return;
> -	}
> +	clk = uclass_get_device(UCLASS_CLK, 0, &dev);
> +	if (clk)
> +		debug("%s: Clock init failed: %d\n", __func__, clk);
>  
> -	ret = uclass_get_device(UCLASS_RESET, 0, &dev);
> -	if (ret) {
> -		debug("Reset init failed: %d\n", ret);
> -		return;
> -	}
> +	reset = uclass_get_device(UCLASS_RESET, 0, &dev);
> +	if (reset)
> +		debug("%s: Reset init failed: %d\n", __func__, reset);
>  
> -	ret = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> -	if (ret) {
> -		debug("%s: Cannot find pinctrl device\n", __func__);
> -		return;
> -	}
> +	pinctrl = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> +	if (pinctrl)
> +		debug("%s: Cannot find pinctrl device: %d\n",
> +		      __func__, pinctrl);
>  
>  	/* enable console uart printing */
>  	preloader_console_init();
>  
> +	if (clk || reset || pinctrl)
> +		printf("%s: probe failed clk=%d reset=%d pinctrl=%d\n",
> +		       __func__, clk, reset, pinctrl);
> +

This change makes little sense to me/  If you want error messages,
then just turn above debug() into printf(), and be done with it.
As an additional benefit so see at once which step failed.

Best regards,

Wolfgang Denk
Patrick Delaunay April 21, 2020, 4:05 p.m. UTC | #3
Dear Wolfgang,

> From: Wolfgang Denk <wd@denx.de>
> Sent: mercredi 1 avril 2020 13:30
> 
> Dear Patrick Delaunay,
> 
> In message
> <20200331180330.6.I41a641a07fd12da45b392920fc3407e608926396@changeid>
> you wrote:
> > Update board_init_f and try to display error message when console is
> > available.
> >
> > This patch adds trace to debug a spl boot issue when DEBUG and
> > DEBUG_UART is not activated, after uart probe.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> >  arch/arm/mach-stm32mp/spl.c | 33 ++++++++++++++++-----------------
> >  1 file changed, 16 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
> > index ca4231cd0d..dfdb5bb7e9 100644
> > --- a/arch/arm/mach-stm32mp/spl.c
> > +++ b/arch/arm/mach-stm32mp/spl.c
> > @@ -79,37 +79,36 @@ void spl_display_print(void)  void
> > board_init_f(ulong dummy)  {
> >  	struct udevice *dev;
> > -	int ret;
> > +	int ret, clk, reset, pinctrl;
> >
> >  	arch_cpu_init();
> >
> >  	ret = spl_early_init();
> >  	if (ret) {
> > -		debug("spl_early_init() failed: %d\n", ret);
> > +		debug("%s: spl_early_init() failed: %d\n", __func__, ret);
> >  		hang();
> >  	}
> >
> > -	ret = uclass_get_device(UCLASS_CLK, 0, &dev);
> > -	if (ret) {
> > -		debug("Clock init failed: %d\n", ret);
> > -		return;
> > -	}
> > +	clk = uclass_get_device(UCLASS_CLK, 0, &dev);
> > +	if (clk)
> > +		debug("%s: Clock init failed: %d\n", __func__, clk);
> >
> > -	ret = uclass_get_device(UCLASS_RESET, 0, &dev);
> > -	if (ret) {
> > -		debug("Reset init failed: %d\n", ret);
> > -		return;
> > -	}
> > +	reset = uclass_get_device(UCLASS_RESET, 0, &dev);
> > +	if (reset)
> > +		debug("%s: Reset init failed: %d\n", __func__, reset);
> >
> > -	ret = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> > -	if (ret) {
> > -		debug("%s: Cannot find pinctrl device\n", __func__);
> > -		return;
> > -	}
> > +	pinctrl = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> > +	if (pinctrl)
> > +		debug("%s: Cannot find pinctrl device: %d\n",
> > +		      __func__, pinctrl);
> >
> >  	/* enable console uart printing */
> >  	preloader_console_init();
> >
> > +	if (clk || reset || pinctrl)
> > +		printf("%s: probe failed clk=%d reset=%d pinctrl=%d\n",
> > +		       __func__, clk, reset, pinctrl);
> > +
> 
> This change makes little sense to me/  If you want error messages, then just turn
> above debug() into printf(), and be done with it.
> As an additional benefit so see at once which step failed.

In this patch, I try to display error as soon as console is available
(after preloader_console_init) , if after one driver initialization
(clk, reset, pincontrol) failing.

Change debug to printf only works only if CONFIG_DEBUG_UART 
is activated (not by default) and board_debug_uart_init() exist to configure
the needed UART TX pins.

At least I need to remove the return and change them to hang() to interrupt SPL
execution if one probe failed to detect issue

I spent some time for this kind of issue: clock probe failed without any trace.
 

> Best regards,
> 
> Wolfgang Denk
> 
> --

Regards
Patrick
Wolfgang Denk April 23, 2020, 8:39 p.m. UTC | #4
Dear Patrick,

In message <8970fb86c1374d1999ff656c2a3272da@SFHDAG6NODE3.st.com> you wrote:
> 
> > > -	ret = uclass_get_device(UCLASS_CLK, 0, &dev);
> > > -	if (ret) {
> > > -		debug("Clock init failed: %d\n", ret);
> > > -		return;
> > > -	}
> > > +	clk = uclass_get_device(UCLASS_CLK, 0, &dev);
> > > +	if (clk)
> > > +		debug("%s: Clock init failed: %d\n", __func__, clk);
> > >
> > > -	ret = uclass_get_device(UCLASS_RESET, 0, &dev);
> > > -	if (ret) {
> > > -		debug("Reset init failed: %d\n", ret);
> > > -		return;
> > > -	}
> > > +	reset = uclass_get_device(UCLASS_RESET, 0, &dev);
> > > +	if (reset)
> > > +		debug("%s: Reset init failed: %d\n", __func__, reset);
> > >
> > > -	ret = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> > > -	if (ret) {
> > > -		debug("%s: Cannot find pinctrl device\n", __func__);
> > > -		return;
> > > -	}
> > > +	pinctrl = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
> > > +	if (pinctrl)
> > > +		debug("%s: Cannot find pinctrl device: %d\n",
> > > +		      __func__, pinctrl);
> > >
> > >  	/* enable console uart printing */
> > >  	preloader_console_init();
> > >
> > > +	if (clk || reset || pinctrl)
> > > +		printf("%s: probe failed clk=%d reset=%d pinctrl=%d\n",
> > > +		       __func__, clk, reset, pinctrl);
> > > +
> > 
> > This change makes little sense to me/  If you want error messages, then just turn
> > above debug() into printf(), and be done with it.
> > As an additional benefit so see at once which step failed.
> 
> In this patch, I try to display error as soon as console is available
> (after preloader_console_init) , if after one driver initialization
> (clk, reset, pincontrol) failing.
> 
> Change debug to printf only works only if CONFIG_DEBUG_UART 
> is activated (not by default) and board_debug_uart_init() exist to configure
> the needed UART TX pins.

Maybe you can remember an error code so you can tell the user which
of the steps failed? That would be more useful, then.

Best regards,

Wolfgang Denk
Patrick Delaunay April 24, 2020, 8:28 a.m. UTC | #5
Dear Wolfgang,

> From: Wolfgang Denk <wd@denx.de>
> Sent: jeudi 23 avril 2020 22:40
> 
> Dear Patrick,
> 
> In message <8970fb86c1374d1999ff656c2a3272da@SFHDAG6NODE3.st.com>
> you wrote:
> >

[...]

> > > >  	/* enable console uart printing */
> > > >  	preloader_console_init();
> > > >
> > > > +	if (clk || reset || pinctrl)
> > > > +		printf("%s: probe failed clk=%d reset=%d pinctrl=%d\n",
> > > > +		       __func__, clk, reset, pinctrl);
> > > > +
> > >
> > > This change makes little sense to me/  If you want error messages,
> > > then just turn above debug() into printf(), and be done with it.
> > > As an additional benefit so see at once which step failed.
> >
> > In this patch, I try to display error as soon as console is available
> > (after preloader_console_init) , if after one driver initialization
> > (clk, reset, pincontrol) failing.
> >
> > Change debug to printf only works only if CONFIG_DEBUG_UART is
> > activated (not by default) and board_debug_uart_init() exist to
> > configure the needed UART TX pins.
> 
> Maybe you can remember an error code so you can tell the user which of the steps
> failed? That would be more useful, then.

After check, I agree with you first comment.

Because console over serial can work only if the driver required for uart driver (clk, reset, pincontrol) can probe.

So CONFIG_DEBUG_UART is mandatory to identify the  failing step and debug() is enough in this case.

In fact my initial issue was only the simple return (and SPL boot continue) 
when a error was detected: I replace them by hang() calls.

That simplify the implementation in V2:
 
[PATCH v2 02/12] arm: stm32mp: spl: update error management in board_init_f
http://patchwork.ozlabs.org/project/uboot/patch/20200422142834.v2.2.I703cbd885066981e3bab374021d5578dce7cb035@changeid/

Regards
 
 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> I know engineers. They love to change things.             - Dr. McCoy

Patrick
diff mbox series

Patch

diff --git a/arch/arm/mach-stm32mp/spl.c b/arch/arm/mach-stm32mp/spl.c
index ca4231cd0d..dfdb5bb7e9 100644
--- a/arch/arm/mach-stm32mp/spl.c
+++ b/arch/arm/mach-stm32mp/spl.c
@@ -79,37 +79,36 @@  void spl_display_print(void)
 void board_init_f(ulong dummy)
 {
 	struct udevice *dev;
-	int ret;
+	int ret, clk, reset, pinctrl;
 
 	arch_cpu_init();
 
 	ret = spl_early_init();
 	if (ret) {
-		debug("spl_early_init() failed: %d\n", ret);
+		debug("%s: spl_early_init() failed: %d\n", __func__, ret);
 		hang();
 	}
 
-	ret = uclass_get_device(UCLASS_CLK, 0, &dev);
-	if (ret) {
-		debug("Clock init failed: %d\n", ret);
-		return;
-	}
+	clk = uclass_get_device(UCLASS_CLK, 0, &dev);
+	if (clk)
+		debug("%s: Clock init failed: %d\n", __func__, clk);
 
-	ret = uclass_get_device(UCLASS_RESET, 0, &dev);
-	if (ret) {
-		debug("Reset init failed: %d\n", ret);
-		return;
-	}
+	reset = uclass_get_device(UCLASS_RESET, 0, &dev);
+	if (reset)
+		debug("%s: Reset init failed: %d\n", __func__, reset);
 
-	ret = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
-	if (ret) {
-		debug("%s: Cannot find pinctrl device\n", __func__);
-		return;
-	}
+	pinctrl = uclass_get_device(UCLASS_PINCTRL, 0, &dev);
+	if (pinctrl)
+		debug("%s: Cannot find pinctrl device: %d\n",
+		      __func__, pinctrl);
 
 	/* enable console uart printing */
 	preloader_console_init();
 
+	if (clk || reset || pinctrl)
+		printf("%s: probe failed clk=%d reset=%d pinctrl=%d\n",
+		       __func__, clk, reset, pinctrl);
+
 	ret = uclass_get_device(UCLASS_RAM, 0, &dev);
 	if (ret) {
 		printf("DRAM init failed: %d\n", ret);