[U-Boot] [PATCH 0/2] edb93xx: two patches for boot code

[U-Boot] [PATCH 0/2] edb93xx: two patches for boot code

[Alessandro Rubini]

While working on a board similar to the EDB9315A, I had to fix two
more things as my board doesn't boot without them.  I already talked
with Matthias Kaehlcke who gave me his ack on those patches.

Alessandro Rubini (2):
  ep93xx leds: remove arrays in data section
  edb93xx sdram: fix initialization

 board/edb93xx/sdram_cfg.c |    7 ++++++-
 cpu/arm920t/ep93xx/led.c  |   29 +++++++++--------------------
 2 files changed, 15 insertions(+), 21 deletions(-)

[U-Boot] [PATCH 1/2] ep93xx leds: remove arrays in data section

[Alessandro Rubini]

This code is used at early boot, and using arrays for status
generates references to RAM addresses that are not working.
The patch avoids such structures using a preprocessor macro and
by reading status from hardware in the toggle function.
Meanwhile, inline functions are turned to static to save code space.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Matthias Kaehlcke <matthias@kaehlcke.net>
---
 cpu/arm920t/ep93xx/led.c |   29 +++++++++--------------------
 1 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/cpu/arm920t/ep93xx/led.c b/cpu/arm920t/ep93xx/led.c
index 7e2c897..8b2df04 100644
--- a/cpu/arm920t/ep93xx/led.c
+++ b/cpu/arm920t/ep93xx/led.c
@@ -25,24 +25,21 @@
 #include <config.h>
 #include <status_led.h>
 
-static uint8_t saved_state[2] = {STATUS_LED_OFF, STATUS_LED_OFF};
-static uint32_t gpio_pin[2] = {1 << STATUS_LED_GREEN,
-			       1 << STATUS_LED_RED};
+/* We can't use arrays in data segment@early boot, but we know n is 0-1 */
+#define GPIO_PIN(n) (1 << (n))
 
-inline void switch_LED_on(uint8_t led)
+static inline void switch_LED_on(uint8_t led)
 {
 	register struct gpio_regs *gpio = (struct gpio_regs *)GPIO_BASE;
 
-	writel(readl(&gpio->pedr) | gpio_pin[led], &gpio->pedr);
-	saved_state[led] = STATUS_LED_ON;
+	writel(readl(&gpio->pedr) | GPIO_PIN(led), &gpio->pedr);
 }
 
-inline void switch_LED_off(uint8_t led)
+static inline void switch_LED_off(uint8_t led)
 {
 	register struct gpio_regs *gpio = (struct gpio_regs *)GPIO_BASE;
 
-	writel(readl(&gpio->pedr) & ~gpio_pin[led], &gpio->pedr);
-	saved_state[led] = STATUS_LED_OFF;
+	writel(readl(&gpio->pedr) & ~GPIO_PIN(led), &gpio->pedr);
 }
 
 void red_LED_on(void)
@@ -72,17 +69,9 @@ void __led_init(led_id_t mask, int state)
 
 void __led_toggle(led_id_t mask)
 {
-	if (STATUS_LED_RED == mask) {
-		if (STATUS_LED_ON == saved_state[STATUS_LED_RED])
-			red_LED_off();
-		else
-			red_LED_on();
-	} else if (STATUS_LED_GREEN == mask) {
-		if (STATUS_LED_ON == saved_state[STATUS_LED_GREEN])
-			green_LED_off();
-		else
-			green_LED_on();
-	}
+	register struct gpio_regs *gpio = (struct gpio_regs *)GPIO_BASE;
+
+	writel(readl(&gpio->pedr) ^ GPIO_PIN(mask), &gpio->pedr);
 }
 
 void __led_set(led_id_t mask, int state)
-- 
1.6.0.2

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Alessandro Rubini]

The configuration of SDRAM needs two more writel() operations,
otherwise some boards won't be able to boot. These additional writes
are present in vendor assembly code but were forgotten during the
rewriting in C.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Matthias Kaehlcke <matthias@kaehlcke.net>
---
 board/edb93xx/sdram_cfg.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/board/edb93xx/sdram_cfg.c b/board/edb93xx/sdram_cfg.c
index 6155f0e..418959a 100644
--- a/board/edb93xx/sdram_cfg.c
+++ b/board/edb93xx/sdram_cfg.c
@@ -59,13 +59,15 @@ void sdram_cfg(void)
 
 static void force_precharge(void)
 {
+	struct sdram_regs *sdram = (struct sdram_regs *)SDRAM_BASE;
+
+	writel(GLCONFIG_INIT | GLCONFIG_CKE, &sdram->glconfig);
 	/*
 	 * Errata most EP93xx revisions say that PRECHARGE ALL isn't always
 	 * issued.
 	 *
 	 * Do a read from each bank to make sure they're precharged
 	 */
-
 	PRECHARGE_BANK(0);
 	PRECHARGE_BANK(1);
 	PRECHARGE_BANK(2);
@@ -101,6 +103,9 @@ static void setup_refresh_timer(void)
 
 static void program_mode_registers(void)
 {
+	struct sdram_regs *sdram = (struct sdram_regs *)SDRAM_BASE;
+
+	writel(GLCONFIG_MRS | GLCONFIG_CKE, &sdram->glconfig);
 	/*
 	 * The mode registers are programmed by performing a read from each
 	 * SDRAM bank. The value of the address that is read defines the value
-- 
1.6.0.2

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Matthias Kaehlcke]

Hi Alessandro,

El Thu, Feb 11, 2010 at 09:46:50PM +0100 Alessandro Rubini ha dit:

> The configuration of SDRAM needs two more writel() operations,
> otherwise some boards won't be able to boot. These additional writes
> are present in vendor assembly code but were forgotten during the
> rewriting in C.
> 
> Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
> Acked-by: Matthias Kaehlcke <matthias@kaehlcke.net>

i gave my ack after a visual review of the patch, without having
tested it. i just installed a patched u-boot on one of my boards and
it doesn't boot :(

at a first glance the offender seems to be the write to the GlConfig
register in force_precharge(), after commenting this line the board
comes up.

comparing with the initial SDRAM initialization written in
assembly i noticed that you set the INIT bit in force_precharge(),
while the assembly code doesn't. according to the user manual the INIT
must be set to issue PRECHARGE ALL, but if i don't get it wrong
this isn't our case, as we assume that PRECHARGE ALL doesn't work (see
errata), thus we precharge manually by reading from the RAM.

but even after clearing the INIT bit my board refuses to boot. i hope
i'll find time this weekend to track this down. if you want to have a
look at the initial sequence in assembly, it can be found here:

http://lists.denx.de/pipermail/u-boot/2009-December/065133.html

cu

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

          We can't solve problems by using the same kind
             of thinking we used when we created them
                         (Albert Einstein)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Alessandro Rubini]

> i gave my ack after a visual review of the patch, without having
> tested it. i just installed a patched u-boot on one of my boards and
> it doesn't boot :(

Oh. The opposite of my board.

Then, since I don't have a 9315A but only a similar one, it's better
to drop the patch.  I'll have a different sdram_cfg file for mine,
then.

Thanks for testing, Matthias

/alessandro

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Matthias Kaehlcke]

El Fri, Feb 12, 2010 at 08:01:26AM +0100 Alessandro Rubini ha dit:

> > i gave my ack after a visual review of the patch, without having
> > tested it. i just installed a patched u-boot on one of my boards and
> > it doesn't boot :(
> 
> Oh. The opposite of my board.
> 
> Then, since I don't have a 9315A but only a similar one, it's better
> to drop the patch.  I'll have a different sdram_cfg file for mine,
> then.

i'm pretty sure we'll be able to track this down, the purpose of your
patch is correct, i actually wonder why the current code works ...

if we can't solve the issue in the next days, you could still send a
patch which only writes the GlConfig register in
program_mode_registers(). this would fix at least on of the issues and
make your code less different from the one in the tree

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

              Comunicar bichos a <bug-coreutils@gnu.org>
                     (LANG=es_ES uname --help)
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Matthias Kaehlcke]

hi alessandro,

El Fri, Feb 12, 2010 at 10:23:58AM +0100 Matthias Kaehlcke ha dit:

> El Fri, Feb 12, 2010 at 08:01:26AM +0100 Alessandro Rubini ha dit:
> 
> > > i gave my ack after a visual review of the patch, without having
> > > tested it. i just installed a patched u-boot on one of my boards and
> > > it doesn't boot :(
> > 
> > Oh. The opposite of my board.
> > 
> > Then, since I don't have a 9315A but only a similar one, it's better
> > to drop the patch.  I'll have a different sdram_cfg file for mine,
> > then.
> 
> i'm pretty sure we'll be able to track this down, the purpose of your
> patch is correct, i actually wonder why the current code works ...
> 
> if we can't solve the issue in the next days, you could still send a
> patch which only writes the GlConfig register in
> program_mode_registers(). this would fix at least on of the issues and
> make your code less different from the one in the tree

maybe i have found a clue: i think the original assembly code wasn't
correct either. the comment talks about reading from each bank, but
then code like this is excuted:

ldr r0, =0x00000000 /* A[22:21] = b00 */
str r0, [r0]

which *writes* to the bank, but doesn't read from it.

according to the errata note a bank is precharged by reading from
it. the current code does this, but before the bank is configured
properly. i suspect that is the cause for the hang i see on my
boards. i've put together some code which does the precharge after
setting up the mode registers. on my boards based on the edb9301 and
edb9307a design u-boot boots with this code.

below you find a patch, could you give it a try on your board, when
you find some time and feel well enough?

thanks

Matthias

---

diff --git a/board/edb93xx/sdram_cfg.c b/board/edb93xx/sdram_cfg.c
index 6155f0e..f3db750 100644
--- a/board/edb93xx/sdram_cfg.c
+++ b/board/edb93xx/sdram_cfg.c
@@ -47,25 +47,27 @@ void sdram_cfg(void)
 
 	early_udelay(200);
 
-	force_precharge();
+	/*
+	 * Errata of most EP93xx revisions say that PRECHARGE ALL isn't always
+	 * issued, so we omit it at this point. Instead we force a precharge
+	 * after having programmed the mode registers
+	 */
 
 	setup_refresh_timer();
 
 	program_mode_registers();
 
-	/* Select normal operation mode */
-	writel(GLCONFIG_CKE, &sdram->glconfig);
+	force_precharge();
 }
 
 static void force_precharge(void)
 {
-	/*
-	 * Errata most EP93xx revisions say that PRECHARGE ALL isn't always
-	 * issued.
-	 *
-	 * Do a read from each bank to make sure they're precharged
-	 */
+	struct sdram_regs *sdram = (struct sdram_regs *)SDRAM_BASE;
+
+	/* Select normal operation mode */
+	writel(GLCONFIG_CKE, &sdram->glconfig);
 
+	/* Do a read from each bank to make sure they're precharged */
 	PRECHARGE_BANK(0);
 	PRECHARGE_BANK(1);
 	PRECHARGE_BANK(2);
@@ -101,6 +103,11 @@ static void setup_refresh_timer(void)
 
 static void program_mode_registers(void)
 {
+	struct sdram_regs *sdram = (struct sdram_regs *)SDRAM_BASE;
+
+	/* Select mode register update mode */
+	writel(GLCONFIG_MRS | GLCONFIG_CKE, &sdram->glconfig);
+
 	/*
 	 * The mode registers are programmed by performing a read from each
 	 * SDRAM bank. The value of the address that is read defines the value

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Alessandro Rubini]

Hello Matthias.
I'm sorry I can't test before wednesday as I'll be offline.

I am doubtful, as usually precharge is done before setting mode, but
your chips are clearly different from mine, I now expect that no
sequence works for both of them.

/alessandro

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Matthias Kaehlcke]

Hi Alessandro,

El Sat, Feb 13, 2010 at 12:01:34AM +0100 Alessandro Rubini ha dit:

> I'm sorry I can't test before wednesday as I'll be offline.
> 
> I am doubtful, as usually precharge is done before setting mode, but
> your chips are clearly different from mine, I now expect that no
> sequence works for both of them.

i found some more information about the precharge workaround: Cirrus
added it to the RedBoot of v1.4.3 of its BSP, but commented in the
most recent version (v1.4.5) after people started complaining that
it causes their boards to hang
(http://arm.cirrus.com/forum/viewtopic.php?t=48&highlight=precharge&sid=e13913b8baa37aabbb6c73a911935080)

conclusion: their workaround doesn't seem to work :/

please test the below patch on your board when you get a chance. it
should be pretty much the same sequence that worked for you, i just
removed the *workaround* that makes my boards hang and restructured
the code a little

---

diff --git a/board/edb93xx/sdram_cfg.c b/board/edb93xx/sdram_cfg.c
index 6155f0e..9c0fa9a 100644
--- a/board/edb93xx/sdram_cfg.c
+++ b/board/edb93xx/sdram_cfg.c
@@ -29,10 +29,7 @@
 #define PROGRAM_MODE_REG(bank)		(*(volatile uint32_t *)		\
 		(SDRAM_BASE_ADDR | SDRAM_BANK_SEL_##bank | SDRAM_MODE_REG_VAL))
 
-#define PRECHARGE_BANK(bank)		(*(volatile uint32_t *)		\
-				(SDRAM_BASE_ADDR | SDRAM_BANK_SEL_##bank))
-
-static void force_precharge(void);
+static void precharge_all_banks(void);
 static void setup_refresh_timer(void);
 static void program_mode_registers(void);
 
@@ -47,7 +44,7 @@ void sdram_cfg(void)
 
 	early_udelay(200);
 
-	force_precharge();
+	precharge_all_banks();
 
 	setup_refresh_timer();
 
@@ -57,19 +54,25 @@ void sdram_cfg(void)
 	writel(GLCONFIG_CKE, &sdram->glconfig);
 }
 
-static void force_precharge(void)
+static void precharge_all_banks(void)
 {
+	struct sdram_regs *sdram = (struct sdram_regs *)SDRAM_BASE;
+
+	/* Issue PRECHARGE ALL commands */
+	writel(GLCONFIG_INIT | GLCONFIG_CKE, &sdram->glconfig);
+
 	/*
-	 * Errata most EP93xx revisions say that PRECHARGE ALL isn't always
-	 * issued.
+	 * Errata of most EP93xx revisions say that PRECHARGE ALL isn't always
+	 * issued
+	 *
+	 * Cirrus proposes a workaround consisting in performing a read from
+	 * each bank to force the precharge. Unfortunately this causes the board
+	 * to hang. Cirrus added this workaround to the RedBoot bootloader they
+	 * deliver, but had to remove it one version later after problems were
+	 * reported
 	 *
-	 * Do a read from each bank to make sure they're precharged
+	 * Thus it seems the only workaround currently available is hope ...
 	 */
-
-	PRECHARGE_BANK(0);
-	PRECHARGE_BANK(1);
-	PRECHARGE_BANK(2);
-	PRECHARGE_BANK(3);
 }
 
 static void setup_refresh_timer(void)
@@ -101,6 +104,11 @@ static void setup_refresh_timer(void)
 
 static void program_mode_registers(void)
 {
+	struct sdram_regs *sdram = (struct sdram_regs *)SDRAM_BASE;
+
+	/* Select mode register update mode */
+	writel(GLCONFIG_MRS | GLCONFIG_CKE, &sdram->glconfig);
+
 	/*
 	 * The mode registers are programmed by performing a read from each
 	 * SDRAM bank. The value of the address that is read defines the value

-- 
Matthias Kaehlcke
Embedded Linux Developer
Barcelona

             "The only important thing Windows does better
            than Debian is implementing the win32 platform"
                                                                 .''`.
    using free software / Debian GNU/Linux | http://debian.org  : :'  :
                                                                `. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4                  `-

[U-Boot] [PATCH 2/2] edb93xx sdram: fix initialization

[Alessandro Rubini]

Hello Matthias.

> i found some more information about the precharge workaround:
> [...]
> please test the below patch on your board when you get a chance. it
> should be pretty much the same sequence that worked for you, i just
> removed the *workaround* that makes my boards hang and restructured
> the code a little

It boots, but it takes one second more than my code -- don't know why.
And neither the ethernet nor usb work properly any more.

I suggest it's best if you leave your previous code here, and I'll
have mine for my board in a different directory.

/alessandro