[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[Fabio Estevam]

Define CONFIG_BOARD_LATE_INIT so that the serial console messages can be redirected to the serial port.

This is needed because in mx53loco.c we have:

#ifdef CONFIG_BOARD_LATE_INIT
int board_late_init(void)
{
	setenv("stdout", "serial");

	return 0;
}
#endif

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 include/configs/mx53loco.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
index 0a25c7d..bd23387 100644
--- a/include/configs/mx53loco.h
+++ b/include/configs/mx53loco.h
@@ -41,6 +41,7 @@
 #define CONFIG_SYS_MALLOC_LEN		(10 * 1024 * 1024)
 
 #define CONFIG_BOARD_EARLY_INIT_F
+#define CONFIG_BOARD_LATE_INIT
 #define CONFIG_MXC_GPIO
 #define CONFIG_REVISION_TAG
 
-- 
1.7.1

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[Fabio Estevam]

On Tue, Jul 31, 2012 at 4:21 PM, Fabio Estevam
<fabio.estevam@freescale.com> wrote:
> Define CONFIG_BOARD_LATE_INIT so that the serial console messages can be redirected to the serial port.
>
> This is needed because in mx53loco.c we have:
>
> #ifdef CONFIG_BOARD_LATE_INIT
> int board_late_init(void)
> {
>         setenv("stdout", "serial");
>
>         return 0;
> }
> #endif
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  include/configs/mx53loco.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
> index 0a25c7d..bd23387 100644
> --- a/include/configs/mx53loco.h
> +++ b/include/configs/mx53loco.h
> @@ -41,6 +41,7 @@
>  #define CONFIG_SYS_MALLOC_LEN          (10 * 1024 * 1024)
>
>  #define CONFIG_BOARD_EARLY_INIT_F
> +#define CONFIG_BOARD_LATE_INIT
>  #define CONFIG_MXC_GPIO
>  #define CONFIG_REVISION_TAG

Or maybe this is a better approach?

diff --git a/board/freescale/mx53loco/mx53loco.c
b/board/freescale/mx53loco/mx53loco.c
index cbdcfad..4176d46 100644
--- a/board/freescale/mx53loco/mx53loco.c
+++ b/board/freescale/mx53loco/mx53loco.c
@@ -495,14 +495,12 @@ int print_cpuinfo(void)
 	return 0;
 }

-#ifdef CONFIG_BOARD_LATE_INIT
 int board_late_init(void)
 {
-	setenv("stdout", "serial");
+	setenv("preboot", "setenv stdout serial\0");

 	return 0;
 }
-#endif

 int board_init(void)
 {
-- 
1.7.1

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[Wolfgang Denk]

Dear Fabio Estevam,

In message <CAOMZO5CBpA4kVC+J6Yp8RwmL-EbLwK1PcYWC_RLX=SwWRnqsyg@mail.gmail.com> you wrote:
...
> 
> -#ifdef CONFIG_BOARD_LATE_INIT
>  int board_late_init(void)
>  {
> -	setenv("stdout", "serial");
> +	setenv("preboot", "setenv stdout serial\0");

NAK.  Please never, never ever mandatorily overwrite environment
variables!  The user who sets it to a different value and cannot find
out why his settings don;t work and always get overwritten would be
seriously frustrated.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If A equals success, then the formula is A = X + Y + Z. X is work.  Y
is play. Z is keep your mouth shut.                 - Albert Einstein

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[Wolfgang Denk]

Dear Fabio Estevam,

In message <1343762513-5574-1-git-send-email-fabio.estevam@freescale.com> you wrote:
> Define CONFIG_BOARD_LATE_INIT so that the serial console messages can be redirected to the serial port.
> 
> This is needed because in mx53loco.c we have:
> 
> #ifdef CONFIG_BOARD_LATE_INIT
> int board_late_init(void)
> {
> 	setenv("stdout", "serial");
> 
> 	return 0;
> }
> #endif

Why would that beneeded?  And why must it be done mandatorily, without
a chance for the user to configure different behaviour?

Please feel fre to define defualt settings, but please always give the
user an option to select different behaviour.  This is what the
environment variables are made for - if we would always sent them
mandatorily in the code, we would nbot need any representation in the
environment.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A failure will not appear until a unit has passed final inspection.

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[stefano babic]

Am 31/07/2012 21:21, schrieb Fabio Estevam:
> Define CONFIG_BOARD_LATE_INIT so that the serial console messages can be redirected to the serial port.
> 
> This is needed because in mx53loco.c we have:
> 
> #ifdef CONFIG_BOARD_LATE_INIT
> int board_late_init(void)
> {
> 	setenv("stdout", "serial");
> 
> 	return 0;
> }
> #endif
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---

Hi Fabio,

>  include/configs/mx53loco.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
> index 0a25c7d..bd23387 100644
> --- a/include/configs/mx53loco.h
> +++ b/include/configs/mx53loco.h
> @@ -41,6 +41,7 @@
>  #define CONFIG_SYS_MALLOC_LEN		(10 * 1024 * 1024)
>  
>  #define CONFIG_BOARD_EARLY_INIT_F
> +#define CONFIG_BOARD_LATE_INIT

I see, commit eae08eb2b53ffb87f3342e45ab422d8625659fcd dropped it.

Acked-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[Wolfgang Denk]

Dear Stefano,

In message <501842D4.6060309@denx.de> you wrote:
>
> > #ifdef CONFIG_BOARD_LATE_INIT
> > int board_late_init(void)
> > {
> > 	setenv("stdout", "serial");
> > 
> > 	return 0;
> > }
> > #endif
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> 
> Hi Fabio,
> 
> >  include/configs/mx53loco.h |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
> > index 0a25c7d..bd23387 100644
> > --- a/include/configs/mx53loco.h
> > +++ b/include/configs/mx53loco.h
> > @@ -41,6 +41,7 @@
> >  #define CONFIG_SYS_MALLOC_LEN		(10 * 1024 * 1024)
> >  
> >  #define CONFIG_BOARD_EARLY_INIT_F
> > +#define CONFIG_BOARD_LATE_INIT
> 
> I see, commit eae08eb2b53ffb87f3342e45ab422d8625659fcd dropped it.
> 
> Acked-by: Stefano Babic <sbabic@denx.de>

Please see my previous message - I dislike the first part of the
patch, the unconditional "setenv stdout serial".

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If that makes any sense to you, you have a big problem."
                                  -- C. Durance, Computer Science 234

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[stefano babic]

Am 31/07/2012 21:56, schrieb Wolfgang Denk:
> Dear Fabio Estevam,
> 

Hi Fabio,

> In message <CAOMZO5CBpA4kVC+J6Yp8RwmL-EbLwK1PcYWC_RLX=SwWRnqsyg@mail.gmail.com> you wrote:
> ...
>>
>> -#ifdef CONFIG_BOARD_LATE_INIT
>>  int board_late_init(void)
>>  {
>> -	setenv("stdout", "serial");
>> +	setenv("preboot", "setenv stdout serial\0");
> 
> NAK.  Please never, never ever mandatorily overwrite environment
> variables!  The user who sets it to a different value and cannot find
> out why his settings don;t work and always get overwritten would be
> seriously frustrated.

Maybe a better way is to use CONFIG_PREBOOT as you do, but without
hard-coding the variable. You can add "preboot" to the default
environment (CONFIG_EXTRA_ENV_SETTINGS), and the user can still
overwrite the behavior setting the variable in the u-boot shell.

But I see I did not follow the same approach for the mx51evk (blame on
me !).

Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[stefano babic]

Am 31/07/2012 22:50, schrieb Wolfgang Denk:
> Dear Stefano,
> 
> In message <501842D4.6060309@denx.de> you wrote:
>>
>>> #ifdef CONFIG_BOARD_LATE_INIT
>>> int board_late_init(void)
>>> {
>>> 	setenv("stdout", "serial");
>>>
>>> 	return 0;
>>> }
>>> #endif
>>>
>>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>>> ---
>>
>> Hi Fabio,
>>
>>>  include/configs/mx53loco.h |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
>>> index 0a25c7d..bd23387 100644
>>> --- a/include/configs/mx53loco.h
>>> +++ b/include/configs/mx53loco.h
>>> @@ -41,6 +41,7 @@
>>>  #define CONFIG_SYS_MALLOC_LEN		(10 * 1024 * 1024)
>>>  
>>>  #define CONFIG_BOARD_EARLY_INIT_F
>>> +#define CONFIG_BOARD_LATE_INIT
>>
>> I see, commit eae08eb2b53ffb87f3342e45ab422d8625659fcd dropped it.
>>
>> Acked-by: Stefano Babic <sbabic@denx.de>
> 
> Please see my previous message - I dislike the first part of the
> patch, the unconditional "setenv stdout serial".

Right - but this is not part of the patch, it is only in the commit
message and regards code that is already mainlined. The patch sets only
CONFIG_BOARD_LATE_INIT. The part you dislike should be fixed by a
separate patch.

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[Stefano Babic]

On 31/07/2012 21:21, Fabio Estevam wrote:
> Define CONFIG_BOARD_LATE_INIT so that the serial console messages can be redirected to the serial port.
> 
> This is needed because in mx53loco.c we have:
> 
> #ifdef CONFIG_BOARD_LATE_INIT
> int board_late_init(void)
> {
> 	setenv("stdout", "serial");
> 
> 	return 0;
> }
> #endif
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  include/configs/mx53loco.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
> index 0a25c7d..bd23387 100644
> --- a/include/configs/mx53loco.h
> +++ b/include/configs/mx53loco.h
> @@ -41,6 +41,7 @@
>  #define CONFIG_SYS_MALLOC_LEN		(10 * 1024 * 1024)
>  
>  #define CONFIG_BOARD_EARLY_INIT_F
> +#define CONFIG_BOARD_LATE_INIT


Applied to u-boot-imx, thank.

This is a real fix - board does not run without it. According to our
discussion in the thread, dropping setenv("stdout", "serial") in code
should be done in a separate patch.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] mx53loco: Define CONFIG_BOARD_LATE_INIT

[Stefano Babic]

On 05/08/2012 09:34, Stefano Babic wrote:
> On 31/07/2012 21:21, Fabio Estevam wrote:
>> Define CONFIG_BOARD_LATE_INIT so that the serial console messages can be redirected to the serial port.
>>
>> This is needed because in mx53loco.c we have:
>>
>> #ifdef CONFIG_BOARD_LATE_INIT
>> int board_late_init(void)
>> {
>> 	setenv("stdout", "serial");
>>
>> 	return 0;
>> }
>> #endif
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>> ---
>>  include/configs/mx53loco.h |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h
>> index 0a25c7d..bd23387 100644
>> --- a/include/configs/mx53loco.h
>> +++ b/include/configs/mx53loco.h
>> @@ -41,6 +41,7 @@
>>  #define CONFIG_SYS_MALLOC_LEN		(10 * 1024 * 1024)
>>  
>>  #define CONFIG_BOARD_EARLY_INIT_F
>> +#define CONFIG_BOARD_LATE_INIT
> 
> 
> Applied to u-boot-imx, thank.
> 
> This is a real fix - board does not run without it. According to our
> discussion in the thread, dropping setenv("stdout", "serial") in code
> should be done in a separate patch.

Sorry Fabio - I was too fast. I thnked about and there is a better
solution. I drop your patch from u-boot-imx and I send a new on for
review, fixing the same issue.

Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================