[U-Boot] [PATCH 0/2] edb93xx: two patches for boot code
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(-)
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 at 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)
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
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
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
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
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
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
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&s...)
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
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
participants (3)
-
Alessandro Rubini -
Alessandro Rubini -
Matthias Kaehlcke