Skip to contentSkip to conversation

remove (double) LED initialization in arm920t start.s

PATCH New Jens Scharsig 5 participants 8 messages Dec 18, 2010 – Jan 20, 2011 Arm (32-bit)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

* remove LED initialization in front of relocation and bss init

---

* prevents run C function on an uninitialized environment

 arch/arm/cpu/arm920t/start.S |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
arch/arm/cpu/arm920t/start.S +0 -3
index 08f178d..764a06a 100644
@@ -119,9 +119,6 @@ start_code:
 	orr	r0, r0, #0xd3
 	msr	cpsr, r0
 
-	bl	coloured_LED_init
-	bl	red_LED_on
-
 #if	defined(CONFIG_AT91RM9200DK) || defined(CONFIG_AT91RM9200EK)
 	/*
 	 * relocate exception table

-- 1.7.1

#1 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

Dear Jens Scharsig,

Am 18.12.2010 um 13:08 schrieb Jens Scharsig:

Quoted 6 lines

* remove LED initialization in front of relocation and bss init

---

* prevents run C function on an uninitialized environment

You are right, we do not have stack/bss setup when we call these two c-functions.

But in that case it is ok to do so cause we do just pc relative accesses here, do not use bss variables and last we do not use the stack.

Beside the fact that we could use it here as it is done since atmel posted their at91rm9200 code we could ask if we need it or not.

I think it is a nice helper for initialisation debugging and I used it from time to time for my relocation investigations. But I think we do _not_ need it in every arm920t/at91 board for every bootup. It should be
a) enclosed in some ifdef to enable them conditionally on compile time
b) documented in a README and completely removed (some lines later too)

Beside that one could state
c) completely removed (the whole coloured_LED stuff) as this is atmel specific and other SoC use another interface to switch LED
if we decide to not use these LED features for bootup debugging.

AFAIK this is also used in arm926ejs (at91sam only), Reinhard can you please comment on that?

regards

Andreas Bie?mann

#2 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

Quoted 22 lines · Andreas Bie?mann

Dear Jens Scharsig,

Am 18.12.2010 um 13:08 schrieb Jens Scharsig:

Quoted 6 lines

* remove LED initialization in front of relocation and bss init

---

* prevents run C function on an uninitialized environment

You are right, we do not have stack/bss setup when we call these two c-functions.

But in that case it is ok to do so cause we do just pc relative accesses here, do not use bss variables and last we do not use the stack.

Beside the fact that we could use it here as it is done since atmel posted their at91rm9200 code we could ask if we need it or not.

I think it is a nice helper for initialisation debugging and I used it from time to time for my relocation investigations. But I think we do _not_ need it in every arm920t/at91 board for every bootup. It should be
a) enclosed in some ifdef to enable them conditionally on compile time
b) documented in a README and completely removed (some lines later too)

Quoted 5 lines

Beside that one could state
c) completely removed (the whole coloured_LED stuff) as this is atmel specific and other SoC use another interface to switch LED
if we decide to not use these LED features for bootup debugging.

AFAIK this is also used in arm926ejs (at91sam only), Reinhard can you please comment on that?

Quite funny, incidentally right this morning I tried to figure out how to use LEDs to assist a "blackbox" sort of system that has only 3 RG LEDs to signal u-boot phases and errors before kernel and application are ultimately started.

This part of u-boot is quite messy, look at "status_led.h" for example...

I agree the AT91 colored LED specialty should be removed, once we can agree on a useful common LED approach.

And I think that should be working the other way.
It is pointless to try to factorize it into a few LEDs with specific meanings.

We should have one function like "show_status(int status)"
with various statuses defined like:
LED_STATUS_RESET, _BEFORE_RELOC, _AFTER_RELOC, _SENT_DHCP, _STARTING_KERNEL, _CRASHED, ...

Then it is up to the board specific code to drive multicolored LEDs, an LCDisplay, some seven-segment displays or even relais.

But I guess that's too futuristic, and as always there is a lot of existing code that does it in various different ways :(

Best Regards,
Reinhard

#3 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

Dear Reinhard Meyer,

Quoted 5 lines · In message <[email protected]> you wrote:

We should have one function like "show_status(int status)"
with various statuses defined like:
LED_STATUS_RESET, _BEFORE_RELOC, _AFTER_RELOC, _SENT_DHCP, _STARTING_KERNEL, _CRASHED, ...

Keep the existing code, and just turn the current set of numbers into an enum.

Best regards,

Wolfgang Denk

Signature · 4 lines
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
Anyone who isn't confused here doesn't really know what's going on.
#4 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

Dear Wolfgang Denk,

Quoted 11 lines

Dear Reinhard Meyer,

Quoted 5 lines · In message<[email protected]> you wrote:

We should have one function like "show_status(int status)"
with various statuses defined like:
LED_STATUS_RESET, _BEFORE_RELOC, _AFTER_RELOC, _SENT_DHCP, _STARTING_KERNEL, _CRASHED, ...

Keep the existing code, and just turn the current set of numbers into an enum.

What current code?

1. show_boot_progress() is quite close, but has hundreds of values... Not sure a enum would be useful there, the values have gaps; rather an errno.h like implementation makes sense there.

2. the LED current code (in some non-ARM) places
has LED types (BOOT, ...) and LED modes (OFF, BLINK, ON). That is totally different and quite useless for seven segment and simple LCDisplays.

Best Regards,
Reinhard

#5 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

Dear Reinhard Meyer,

Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 3 lines

Keep the existing code, and just turn the current set of numbers into an enum.

What current code?

I meant the existing show_boot_progress() code.

Quoted 3 lines

1. show_boot_progress() is quite close, but has hundreds of values... Not sure a enum would be useful there, the values have gaps; rather an errno.h like implementation makes sense there.

I think it's just an implementation detail wether you use an enum or #defines here.

Quoted 4 lines

2. the LED current code (in some non-ARM) places
has LED types (BOOT, ...) and LED modes (OFF, BLINK, ON). That is totally different and quite useless for seven segment and simple LCDisplays.

Right.

Note that there is more code when it comes to seven segment and character based LCD displays - please see doc/README.LED_display, include/led-display.h, common/cmd_display.c and drivers/misc/pdsp188x.c on one side, or the highly board specific display code using the same framework in board/a4m072/a4m072.c on the other side.

Best regards,

Wolfgang Denk

Signature · 6 lines
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
The main thing is the play itself. I swear that greed for  money  has
nothing  to  do with it, although heaven knows I am sorely in need of
money.                                           - Feodor Dostoyevsky
#6 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

In reply to Jens Scharsig

To explain the background

1st:

I've compiled my code for easier debugging without -Os option. But the non optimized code needs a valid stack at this position. It's only as secondary problem

2nd:

The functions will called later again.

3rd:

Some soc are not fully intitialized here (e.g Clock aso). So, we also increase the boot speed.

regards

Jens

#7 lore raw

PATCH 1/1 remove (double) LED initialization in arm920t start.s

In reply to Jens Scharsig

Le 18/12/2010 13:08, Jens Scharsig a ?crit :

Quoted 15 lines

* remove LED initialization in front of relocation and bss init

---

* prevents run C function on an uninitialized environment

  arch/arm/cpu/arm920t/start.S |    3 ---
  1 files changed, 0 insertions(+), 3 deletions(-)
arch/arm/cpu/arm920t/start.S +0 -0
index 08f178d..764a06a 100644

arch/arm/cpu/arm920t/start.S start_code:

Quoted 10 lines
arch/arm/cpu/arm920t/start.S +0 -4
@@ -119,9 +119,6 @@ start_code:
  	orr	r0, r0, #0xd3
  	msr	cpsr, r0
 
-	bl	coloured_LED_init
-	bl	red_LED_on
-
  #if	defined(CONFIG_AT91RM9200DK) || defined(CONFIG_AT91RM9200EK)
  	/*
  	 * relocate exception table
-- 1.7.1

Applied to U-boot-arm, thanks.

Amicalement,

Signature · 1 line
Albert.