diff mbox

[U-Boot] environmental "baudrate" not used at boot up

Message ID 20110119053445.GA9776@cslinux-build01.cyberswitching.local
State Rejected, archived
Headers show

Commit Message

Chris Verges Jan. 19, 2011, 5:34 a.m. UTC 
On Sun, Jan 16, 2011 at 06:01:22AM -0800, chrisv@cyberswitching.com wrote:
> ...
> Any advice on where to take it from here?

Hi everyone,

I investigated this a little further, and I'm wondering if the problem
is related to the initialization ordering in lib_arm/board.c.

Here's the sequence:

   start_armboot():
   1. init_baudrate() -> getenv_r() -> serial_setbrg()
   2. env_relocate()

Note that init_baudrate() calls getenv_r("baudrate") and passes either
the result (on success) or CONFIG_BAUDRATE (on error) to
serial_setbrg().  Only after this does the environment get relocated
using env_relocate().

Based on empirical testing, I've discovered that re-running
init_baudrate() after env_relocate() fixes everything.  The serial
console uses the baud rate stored in the "baudrate" variable now, and
some ordering of display outputs needs to be tweaked so that gibberish
isn't output in the interim.

The attached patch (below) should give a code-centric view.  Please
ignore the minor style changes, as I was hacking code pretty feverishly
to try and figure this out.  To be clear: DO NOT IMPORT THIS CODE
DIRECTLY INTO THE LATEST MASTER!  It will need to be reviewed, tweaked,
and the finally imported by someone who knows more of what they are
doing than I.  :-)

Hope this helps someone else out,

Chris

Comments

Wolfgang Denk Jan. 19, 2011, 6:59 a.m. UTC | #1 
Dear chrisv@cyberswitching.com,

In message <20110119053445.GA9776@cslinux-build01.cyberswitching.local> you wrote:
>
> I investigated this a little further, and I'm wondering if the problem
> is related to the initialization ordering in lib_arm/board.c.
> 
> Here's the sequence:
> 
>    start_armboot():
>    1. init_baudrate() -> getenv_r() -> serial_setbrg()
>    2. env_relocate()
> 
> Note that init_baudrate() calls getenv_r("baudrate") and passes either
> the result (on success) or CONFIG_BAUDRATE (on error) to
> serial_setbrg().  Only after this does the environment get relocated
> using env_relocate().

Right.  This is the intended sequence.

> Based on empirical testing, I've discovered that re-running
> init_baudrate() after env_relocate() fixes everything.  The serial
> console uses the baud rate stored in the "baudrate" variable now, and
> some ordering of display outputs needs to be tweaked so that gibberish
> isn't output in the interim.

You have diagnosed where the problem is, but you come to the wrong
conclusions and instead of fixing the problem you paint over it.

Obviously  getenv_r("baudrate")  is not returning the right value for
you.

You should first check, what exactly it returns.

Then you should check why it is not reading the correct data, as it is
supposed to do.

Then you should fix _that_ problem.

Best regards,

Wolfgang Denk
Chris Verges Jan. 19, 2011, 2:46 p.m. UTC | #2 
On Wed, Jan 19, 2011 at 07:59:10AM +0100, Wolfgang Denk wrote:
> > Based on empirical testing, I've discovered that re-running
> > init_baudrate() after env_relocate() fixes everything.  The serial
> > console uses the baud rate stored in the "baudrate" variable now, and
> > some ordering of display outputs needs to be tweaked so that gibberish
> > isn't output in the interim.
> 
> You have diagnosed where the problem is, but you come to the wrong
> conclusions and instead of fixing the problem you paint over it.
> 
> Obviously  getenv_r("baudrate")  is not returning the right value for
> you.
> 
> You should first check, what exactly it returns.
> 
> Then you should check why it is not reading the correct data, as it is
> supposed to do.
>
> Then you should fix _that_ problem.

Thanks for the feedback.  I hope that someone else can continue this
work now that I've identified the problem and have developed a solution
that works for my needs with no obvious side-effects.

Given that this is such base functionality, I'm surprised that no one
else has mentioned anything up to this point; I guess no one else
attempts to change the baud rate for subsequent reboots?  (Rhetorical,
no need to answer.)

All the best with the upcoming release,

Chris
Wolfgang Denk Jan. 19, 2011, 3:36 p.m. UTC | #3 
Dear chrisv@cyberswitching.com,

In message <20110119144640.GA8828@cslinux-build01.cyberswitching.local> you wrote:
>
> Thanks for the feedback.  I hope that someone else can continue this
> work now that I've identified the problem and have developed a solution
> that works for my needs with no obvious side-effects.

Who said there are no side-effects?  Just try setting a different baud
rate, save it, and reboot.

> Given that this is such base functionality, I'm surprised that no one
> else has mentioned anything up to this point; I guess no one else
> attempts to change the baud rate for subsequent reboots?  (Rhetorical,
> no need to answer.)

You are probably right - few people test such changes.

Best regards,

Wolfgang Denk
Reinhard Meyer Jan. 19, 2011, 7:49 p.m. UTC | #4 
Dear Wolfgang Denk,
> Dear chrisv@cyberswitching.com,
>
> In message<20110119144640.GA8828@cslinux-build01.cyberswitching.local>  you wrote:
>>
>> Thanks for the feedback.  I hope that someone else can continue this
>> work now that I've identified the problem and have developed a solution
>> that works for my needs with no obvious side-effects.
>
> Who said there are no side-effects?  Just try setting a different baud
> rate, save it, and reboot.
>
>> Given that this is such base functionality, I'm surprised that no one
>> else has mentioned anything up to this point; I guess no one else
>> attempts to change the baud rate for subsequent reboots?  (Rhetorical,
>> no need to answer.)
>
> You are probably right - few people test such changes.

I just want to point out that this works fine on our AT91SAM9260/9XE,
however the environment is read from I2C EEPROM. Not sure where
his ENV is coming from, whether it is read before use, or if there is
static data involved before relocation...

Best Regards,
Reinhard
Chris Verges Jan. 19, 2011, 7:50 p.m. UTC | #5 
On Wed, Jan 19, 2011 at 08:49:12PM +0100, Reinhard Meyer wrote:
> I just want to point out that this works fine on our AT91SAM9260/9XE,
> however the environment is read from I2C EEPROM. Not sure where
> his ENV is coming from, whether it is read before use, or if there is
> static data involved before relocation...

ENV is coming from nandflash.  Config is based on
at91sam9263ek_nandflash_config.

Chris
Scott Wood Jan. 19, 2011, 7:58 p.m. UTC | #6 
On Tue, 18 Jan 2011 21:34:45 -0800
<chrisv@cyberswitching.com> wrote:

> On Sun, Jan 16, 2011 at 06:01:22AM -0800, chrisv@cyberswitching.com wrote:
> > ...
> > Any advice on where to take it from here?
> 
> Hi everyone,
> 
> I investigated this a little further, and I'm wondering if the problem
> is related to the initialization ordering in lib_arm/board.c.
> 
> Here's the sequence:
> 
>    start_armboot():
>    1. init_baudrate() -> getenv_r() -> serial_setbrg()
>    2. env_relocate()
> 
> Note that init_baudrate() calls getenv_r("baudrate") and passes either
> the result (on success) or CONFIG_BAUDRATE (on error) to
> serial_setbrg().  Only after this does the environment get relocated
> using env_relocate().

The full NAND code only works after relocation.  So you cannot read out
the NAND environment, in the normal way, before serial init -- you get
the default environment instead.

If you are booting from NAND, I suggest using CONFIG_NAND_ENV_DST to
have the NAND SPL load the environment at the same time as it loads
U-Boot.  If you're using some preloader other than U-Boot's NAND SPL,
you'll need to see if it supports something similar.

-Scott
Aggrwal Poonam-B10812 Jan. 21, 2011, 4:45 a.m. UTC | #7 
> -----Original Message-----
> From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de]
> On Behalf Of Wood Scott-B07421
> Sent: Thursday, January 20, 2011 1:28 AM
> To: chrisv@cyberswitching.com
> Cc: u-boot@lists.denx.de
> Subject: Re: [U-Boot] environmental "baudrate" not used at boot up
> 
> On Tue, 18 Jan 2011 21:34:45 -0800
> <chrisv@cyberswitching.com> wrote:
> 
> > On Sun, Jan 16, 2011 at 06:01:22AM -0800, chrisv@cyberswitching.com
> wrote:
> > > ...
> > > Any advice on where to take it from here?
> >
> > Hi everyone,
> >
> > I investigated this a little further, and I'm wondering if the problem
> > is related to the initialization ordering in lib_arm/board.c.
> >
> > Here's the sequence:
> >
> >    start_armboot():
> >    1. init_baudrate() -> getenv_r() -> serial_setbrg()
> >    2. env_relocate()
> >
> > Note that init_baudrate() calls getenv_r("baudrate") and passes either
> > the result (on success) or CONFIG_BAUDRATE (on error) to
> > serial_setbrg().  Only after this does the environment get relocated
> > using env_relocate().
> 
> The full NAND code only works after relocation.  So you cannot read out
> the NAND environment, in the normal way, before serial init -- you get
> the default environment instead.
> 
> If you are booting from NAND, I suggest using CONFIG_NAND_ENV_DST to have
> the NAND SPL load the environment at the same time as it loads U-Boot.
> If you're using some preloader other than U-Boot's NAND SPL, you'll need
> to see if it supports something similar.
Hello Scott

Is this feature available/tested on any FSL platform. The code you pointed to does not seem to be used by FSL platforms.

It would be good idea to pull this in for P1/P2 RDB nand boot loader.

Regards
Poonam

> 
> -Scott
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Scott Wood Jan. 21, 2011, 6:58 p.m. UTC | #8 
On Thu, 20 Jan 2011 22:45:34 -0600
Aggrwal Poonam-B10812 <B10812@freescale.com> wrote:

> > -----Original Message-----
> > From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de]
> > On Behalf Of Wood Scott-B07421
> > Sent: Thursday, January 20, 2011 1:28 AM
> > To: chrisv@cyberswitching.com
> > Cc: u-boot@lists.denx.de
> > Subject: Re: [U-Boot] environmental "baudrate" not used at boot up
> > 
> > The full NAND code only works after relocation.  So you cannot read out
> > the NAND environment, in the normal way, before serial init -- you get
> > the default environment instead.
> > 
> > If you are booting from NAND, I suggest using CONFIG_NAND_ENV_DST to have
> > the NAND SPL load the environment at the same time as it loads U-Boot.
> > If you're using some preloader other than U-Boot's NAND SPL, you'll need
> > to see if it supports something similar.
> Hello Scott
> 
> Is this feature available/tested on any FSL platform. The code you pointed to does not seem to be used by FSL platforms.
> 
> It would be good idea to pull this in for P1/P2 RDB nand boot loader.

It is currently only supported in nand_boot.c, but it should be trivial
to add it to nand_boot_fsl_elbc.c.  Just copy the extra nand_boot()
invocations.

-Scott
diff mbox

Patch

diff -p -r -U 3 a/drivers/mtd/nand/nand.c b/drivers/mtd/nand/nand.c
--- a/drivers/mtd/nand/nand.c	2009-01-21 14:08:12.000000000 -0800
+++ b/drivers/mtd/nand/nand.c	2011-01-18 21:29:36.000000000 -0800
@@ -57,6 +57,17 @@  static void nand_init_chip(struct mtd_in
 
 }
 
+void display_nand_config(void)
+{
+	int i;
+	unsigned int size = 0;
+	printf("NAND:  ");
+	for (i = 0; i < CONFIG_SYS_MAX_NAND_DEVICE; i++) {
+		size += nand_info[i].size / 1024;
+	}
+	printf("%u MiB\n", size / 1024);
+}
+
 void nand_init(void)
 {
 	int i;
@@ -67,7 +78,6 @@  void nand_init(void)
 		if (nand_curr_device == -1)
 			nand_curr_device = i;
 	}
-	printf("%u MiB\n", size / 1024);
 
 #ifdef CONFIG_SYS_NAND_SELECT_DEVICE
 	/*
diff -p -r -U 3 a/include/nand.h b/include/nand.h
--- a/include/nand.h	2009-01-21 14:08:12.000000000 -0800
+++ b/include/nand.h	2011-01-18 21:29:43.000000000 -0800
@@ -25,6 +25,7 @@ 
 #define _NAND_H_
 
 extern void nand_init(void);
+extern void display_nand_config(void);
 
 #ifndef CONFIG_NAND_LEGACY
 #include <linux/mtd/compat.h>
diff -p -r -U 3 a/lib_arm/board.c b/lib_arm/board.c
--- a/lib_arm/board.c	2009-01-21 14:08:12.000000000 -0800
+++ b/lib_arm/board.c	2011-01-18 21:29:24.000000000 -0800
@@ -148,10 +148,15 @@  void inline yellow_LED_off(void)__attrib
 static int init_baudrate (void)
 {
 	char tmp[64];	/* long enough for environment variables */
-	int i = getenv_r ("baudrate", tmp, sizeof (tmp));
-	gd->bd->bi_baudrate = gd->baudrate = (i > 0)
-			? (int) simple_strtoul (tmp, NULL, 10)
-			: CONFIG_BAUDRATE;
+	int i;
+	int baudrate = CONFIG_BAUDRATE;
+
+	i = getenv_r ("baudrate", tmp, sizeof (tmp));
+	if (i > 0)
+		baudrate = (int) simple_strtoul(tmp, NULL, 10);
+
+	gd->baudrate = baudrate;
+	gd->bd->bi_baudrate = baudrate;
 
 	return (0);
 }
@@ -256,6 +261,14 @@  init_fnc_t *init_sequence[] = {
 	init_baudrate,		/* initialze baudrate settings */
 	serial_init,		/* serial communications setup */
 	console_init_f,		/* stage 1 init of console */
+#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
+	init_func_i2c,
+#endif
+	dram_init,		/* configure available RAM banks */
+	NULL,
+};
+
+init_fnc_t *display_sequence[] = {
 	display_banner,		/* say that we are here */
 #if defined(CONFIG_DISPLAY_CPUINFO)
 	print_cpuinfo,		/* display cpu info (and speed) */
@@ -263,10 +276,9 @@  init_fnc_t *init_sequence[] = {
 #if defined(CONFIG_DISPLAY_BOARDINFO)
 	checkboard,		/* display board info */
 #endif
-#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
-	init_func_i2c,
+#if defined(CONFIG_CMD_NAND)
+	display_nand_config,
 #endif
-	dram_init,		/* configure available RAM banks */
 	display_dram_config,
 	NULL,
 };
@@ -340,7 +352,6 @@  void start_armboot (void)
 	mem_malloc_init (_armboot_start - CONFIG_SYS_MALLOC_LEN);
 
 #if defined(CONFIG_CMD_NAND)
-	puts ("NAND:  ");
 	nand_init();		/* go init the NAND */
 #endif
 
@@ -396,6 +407,18 @@  void start_armboot (void)
 #endif
 	}
 
+	/* Reinitialize the baud rate since the environment wasn't ready */
+	init_baudrate();
+	serial_setbrg();
+	udelay(50000);
+
+	/* Now display the information to the serial console */
+	for (init_fnc_ptr = display_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
+		if ((*init_fnc_ptr)() != 0) {
+			hang ();
+		}
+	}
+
 	devices_init ();	/* get the devices list going. */
 
 #ifdef CONFIG_CMC_PU2