[U-Boot] ARM LED weak symbols
As discussed earlier today
http://lists.denx.de/pipermail/u-boot/2009-November/063711.html
This is the patch to conditionally compile the ARM board LED functions.
It was regression tested with MAKEALL arm using the Code Sourcery 2009 compiler and my roll-your-own gcc-4.4.2
Both patches were runtested with the Code Sourcery compiler on Zoom2.
Since this is a common patch, I would like to see it tested on other boards (hint).
I will wait till tomorrow for comments and then push it to arm/next.
Tom
The ARM board LED functions are defined as weak. They add a size overhead if they are not used.
Now they are only defined if CONFIG_STATUS_LED is also defined.
The arm920t and arm926ejs _start function calls these LED functions
bl coloured_LED_init bl red_LED_on
In general, what happens is they call into the weak stubs. Only if the cpu/board provides an overriding function do these calls cause anything meaningful to happen.
Now this noop case is removed and these LED functions are excuted only when CONFIG_STATUS_LED is defined
Signed-off-by: Tom Rix Tom.Rix@windriver.com --- cpu/arm920t/start.S | 4 ++-- cpu/arm926ejs/start.S | 4 ++-- lib_arm/board.c | 2 ++ 3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/cpu/arm920t/start.S b/cpu/arm920t/start.S index 114427a..e05ebf3 100644 --- a/cpu/arm920t/start.S +++ b/cpu/arm920t/start.S @@ -113,10 +113,10 @@ start_code: bic r0, r0, #0x1f orr r0, r0, #0xd3 msr cpsr, r0 - +#ifdef CONFIG_STATUS_LED bl coloured_LED_init bl red_LED_on - +#endif #if defined(CONFIG_AT91RM9200DK) || defined(CONFIG_AT91RM9200EK) /* * relocate exception table diff --git a/cpu/arm926ejs/start.S b/cpu/arm926ejs/start.S index 4421b6a..117ffb1 100644 --- a/cpu/arm926ejs/start.S +++ b/cpu/arm926ejs/start.S @@ -183,10 +183,10 @@ clbss_l:str r2, [r0] /* clear loop... */ add r0, r0, #4 cmp r0, r1 ble clbss_l - +#ifdef CONFIG_STATUS_LED bl coloured_LED_init bl red_LED_on - +#endif ldr pc, _start_armboot
_start_armboot: diff --git a/lib_arm/board.c b/lib_arm/board.c index 5e3d7f6..28b15da 100644 --- a/lib_arm/board.c +++ b/lib_arm/board.c @@ -87,6 +87,7 @@ extern void rtl8019_get_enetaddr (uchar * addr); #endif
+#ifdef CONFIG_STATUS_LED /************************************************************************ * Coloured LED functionality ************************************************************************ @@ -110,6 +111,7 @@ void inline __blue_LED_on(void) {} void inline blue_LED_on(void)__attribute__((weak, alias("__blue_LED_on"))); void inline __blue_LED_off(void) {} void inline blue_LED_off(void)__attribute__((weak, alias("__blue_LED_off"))); +#endif
/************************************************************************ * Init Utilities *
From: Abdoulaye Walsimou Gaye walsimou@walsimou.com
This patch fix build error with gcc-4.4.2 about inline function declared weak, see below: board.c:96: error: inline function 'coloured_LED_init' cannot be declared weak board.c:98: error: inline function 'red_LED_on' cannot be declared weak board.c:100: error: inline function 'red_LED_off' cannot be declared weak board.c:102: error: inline function 'green_LED_on' cannot be declared weak board.c:104: error: inline function 'green_LED_off' cannot be declared weak board.c:106: error: inline function 'yellow_LED_on' cannot be declared weak board.c:108: error: inline function 'yellow_LED_off' cannot be declared weak board.c:110: error: inline function 'blue_LED_on' cannot be declared weak board.c:112: error: inline function 'blue_LED_off' cannot be declared weak make[1]: *** [board.o] Error 1
Signed-off-by: Abdoulaye Walsimou Gaye walsimou@walsimou.com --- lib_arm/board.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/lib_arm/board.c b/lib_arm/board.c index 28b15da..2665093 100644 --- a/lib_arm/board.c +++ b/lib_arm/board.c @@ -94,23 +94,23 @@ extern void rtl8019_get_enetaddr (uchar * addr); * May be supplied by boards if desired */ void inline __coloured_LED_init (void) {} -void inline coloured_LED_init (void) __attribute__((weak, alias("__coloured_LED_init"))); +void coloured_LED_init(void)__attribute__((weak, alias("__coloured_LED_init"))); void inline __red_LED_on (void) {} -void inline red_LED_on (void) __attribute__((weak, alias("__red_LED_on"))); +void red_LED_on(void) __attribute__((weak, alias("__red_LED_on"))); void inline __red_LED_off(void) {} -void inline red_LED_off(void) __attribute__((weak, alias("__red_LED_off"))); +void red_LED_off(void)__attribute__((weak, alias("__red_LED_off"))); void inline __green_LED_on(void) {} -void inline green_LED_on(void) __attribute__((weak, alias("__green_LED_on"))); +void green_LED_on(void) __attribute__((weak, alias("__green_LED_on"))); void inline __green_LED_off(void) {} -void inline green_LED_off(void)__attribute__((weak, alias("__green_LED_off"))); +void green_LED_off(void)__attribute__((weak, alias("__green_LED_off"))); void inline __yellow_LED_on(void) {} -void inline yellow_LED_on(void)__attribute__((weak, alias("__yellow_LED_on"))); +void yellow_LED_on(void)__attribute__((weak, alias("__yellow_LED_on"))); void inline __yellow_LED_off(void) {} -void inline yellow_LED_off(void)__attribute__((weak, alias("__yellow_LED_off"))); +void yellow_LED_off(void)__attribute__((weak, alias("__yellow_LED_off"))); void inline __blue_LED_on(void) {} -void inline blue_LED_on(void)__attribute__((weak, alias("__blue_LED_on"))); +void blue_LED_on(void)__attribute__((weak, alias("__blue_LED_on"))); void inline __blue_LED_off(void) {} -void inline blue_LED_off(void)__attribute__((weak, alias("__blue_LED_off"))); +void blue_LED_off(void)__attribute__((weak, alias("__blue_LED_off"))); #endif
/************************************************************************
Hi,
I am trying to define multiple partitions using mtdparts variable. But I can't get it set automatically right.
I defined the set_bootargs as the following: set_bootargs=setenv bootargs root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=ph ysmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),7936k(root2);physmap-fl ash.1:16384k(data) console=ttyS0,115200n8
But when I run set_bootargs, I got the following error - # run set_bootargs Unknown command 'physmap-flash.1:16384k(data)' - try 'help' Can someone help me with the correct format on mtdparts? thanks a lot.
==================================================== mtdparts=mtdparts=physmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),793 6k(root2);physmap-flash.1:16384k(data) boot=run set_bootargs;chpart nor0,${boot_device};fsload boot/uImage;bootm mtdids=nor0=physmap-flash.0,nor1=physmap-flash.1 partition=nor0,2 mtddevnum=2 mtddevname=root1 set_bootargs=setenv bootargs root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=ph ysmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),7936k(root2);physmap-fl ash.1:16384k(data) console=ttyS0,115200n8 bootargs=root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=physmap-flash.0:256k(u -boot),256k(u-boot-env),7936k(root1),7936k(root2)
Environment size: 847/262140 bytes # run set_bootargs Unknown command 'physmap-flash.1:16384k(data)' - try 'help' #
Am Mittwoch 04 November 2009 01:34:53 schrieb myuboot@fastmail.fm:
Hi,
I am trying to define multiple partitions using mtdparts variable. But I can't get it set automatically right.
I defined the set_bootargs as the following: set_bootargs=setenv bootargs root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=ph ysmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),7936k(root2);physmap-fl ash.1:16384k(data) console=ttyS0,115200n8
But when I run set_bootargs, I got the following error - # run set_bootargs Unknown command 'physmap-flash.1:16384k(data)' - try 'help' Can someone help me with the correct format on mtdparts? thanks a lot.
I don't know the exact format but it looks like the ";" between (root2);physmap-flash1 causes your error?
Dieter
==================================================== mtdparts=mtdparts=physmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),793 6k(root2);physmap-flash.1:16384k(data) boot=run set_bootargs;chpart nor0,${boot_device};fsload boot/uImage;bootm mtdids=nor0=physmap-flash.0,nor1=physmap-flash.1 partition=nor0,2 mtddevnum=2 mtddevname=root1 set_bootargs=setenv bootargs root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=ph ysmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),7936k(root2);physmap-fl ash.1:16384k(data) console=ttyS0,115200n8 bootargs=root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=physmap-flash.0:256k(u -boot),256k(u-boot-env),7936k(root1),7936k(root2)
Environment size: 847/262140 bytes # run set_bootargs Unknown command 'physmap-flash.1:16384k(data)' - try 'help' # _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
I thought that is a legal definition according to u-boot document section "5.9.3.5. mtdparts - define a Linux compatible MTD partition scheme" at http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash
mtdparts=mtdparts=<mtd-def>[;<mtd-def>...]
Thanks.
On Wed, 04 Nov 2009 08:14 +0100, "Dieter Kiermaier" dk-arm-linux@gmx.de wrote:
Am Mittwoch 04 November 2009 01:34:53 schrieb myuboot@fastmail.fm:
Hi,
I am trying to define multiple partitions using mtdparts variable. But I can't get it set automatically right.
I defined the set_bootargs as the following: set_bootargs=setenv bootargs root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=ph ysmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),7936k(root2);physmap-fl ash.1:16384k(data) console=ttyS0,115200n8
But when I run set_bootargs, I got the following error - # run set_bootargs Unknown command 'physmap-flash.1:16384k(data)' - try 'help' Can someone help me with the correct format on mtdparts? thanks a lot.
I don't know the exact format but it looks like the ";" between (root2);physmap-flash1 causes your error?
Dieter
==================================================== mtdparts=mtdparts=physmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),793 6k(root2);physmap-flash.1:16384k(data) boot=run set_bootargs;chpart nor0,${boot_device};fsload boot/uImage;bootm mtdids=nor0=physmap-flash.0,nor1=physmap-flash.1 partition=nor0,2 mtddevnum=2 mtddevname=root1 set_bootargs=setenv bootargs root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=ph ysmap-flash.0:256k(u-boot),256k(u-boot-env),7936k(root1),7936k(root2);physmap-fl ash.1:16384k(data) console=ttyS0,115200n8 bootargs=root=/dev/mtdblock2 rootfs=cramfs,jffs2 mtdparts=physmap-flash.0:256k(u -boot),256k(u-boot-env),7936k(root1),7936k(root2)
Environment size: 847/262140 bytes # run set_bootargs Unknown command 'physmap-flash.1:16384k(data)' - try 'help' # _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear myuboot@fastmail.fm,
how about providing a realname?
And please read http://www.netmeister.org/news/learn2quote.html
Do not top post / full quote!
In message 1257348926.15792.1343548151@webmail.messagingengine.com you wrote:
I thought that is a legal definition according to u-boot document section "5.9.3.5. mtdparts - define a Linux compatible MTD partition scheme" at http://www.denx.de/wiki/view/DULG/UBootCmdGroupFlash
mtdparts=mtdparts=<mtd-def>[;<mtd-def>...]
Yes, it is. But the shell is interpreting the semicolon so you have to escape it.
Best regards,
Wolfgang Denk
Dear Tom Rix,
In message 1257292804-10612-3-git-send-email-Tom.Rix@windriver.com you wrote:
From: Abdoulaye Walsimou Gaye walsimou@walsimou.com
This patch fix build error with gcc-4.4.2 about inline function declared weak, see below: board.c:96: error: inline function 'coloured_LED_init' cannot be declared weak board.c:98: error: inline function 'red_LED_on' cannot be declared weak board.c:100: error: inline function 'red_LED_off' cannot be declared weak board.c:102: error: inline function 'green_LED_on' cannot be declared weak board.c:104: error: inline function 'green_LED_off' cannot be declared weak board.c:106: error: inline function 'yellow_LED_on' cannot be declared weak board.c:108: error: inline function 'yellow_LED_off' cannot be declared weak board.c:110: error: inline function 'blue_LED_on' cannot be declared weak board.c:112: error: inline function 'blue_LED_off' cannot be declared weak make[1]: *** [board.o] Error 1
Signed-off-by: Abdoulaye Walsimou Gaye walsimou@walsimou.com
What is this? Are you sure this patch is properly attributed? Was Abdoulaye Walsimou Gaye really the first to submit this patch?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Tom Rix,
In message 1257292804-10612-3-git-send-email-Tom.Rix@windriver.com you wrote:
From: Abdoulaye Walsimou Gaye walsimou@walsimou.com
This patch fix build error with gcc-4.4.2 about inline function declared weak, see below: board.c:96: error: inline function 'coloured_LED_init' cannot be declared weak board.c:98: error: inline function 'red_LED_on' cannot be declared weak board.c:100: error: inline function 'red_LED_off' cannot be declared weak board.c:102: error: inline function 'green_LED_on' cannot be declared weak board.c:104: error: inline function 'green_LED_off' cannot be declared weak board.c:106: error: inline function 'yellow_LED_on' cannot be declared weak board.c:108: error: inline function 'yellow_LED_off' cannot be declared weak board.c:110: error: inline function 'blue_LED_on' cannot be declared weak board.c:112: error: inline function 'blue_LED_off' cannot be declared weak make[1]: *** [board.o] Error 1
Signed-off-by: Abdoulaye Walsimou Gaye walsimou@walsimou.com
What is this? Are you sure this patch is properly attributed? Was Abdoulaye Walsimou Gaye really the first to submit this patch?
Best regards,
Wolfgang Denk
Would you like the older one ? As you said, they looked the same.
Tom
Dear Tom,
In message 4AF33815.4030101@windriver.com you wrote:
Signed-off-by: Abdoulaye Walsimou Gaye walsimou@walsimou.com
What is this? Are you sure this patch is properly attributed? Was Abdoulaye Walsimou Gaye really the first to submit this patch?
Best regards,
Wolfgang Denk
Would you like the older one ? As you said, they looked the same.
It does not matter what I like. What matters is who is to be credited for this patch, and in my opinion this has to be the first to submit it.
You don't get any credits any more today for inventing a light bulb, either.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Tom,
In message 4AF33815.4030101@windriver.com you wrote:
Signed-off-by: Abdoulaye Walsimou Gaye walsimou@walsimou.com
What is this? Are you sure this patch is properly attributed? Was Abdoulaye Walsimou Gaye really the first to submit this patch?
Best regards,
Wolfgang Denk
Would you like the older one ? As you said, they looked the same.
It does not matter what I like. What matters is who is to be credited for this patch, and in my opinion this has to be the first to submit it.
You don't get any credits any more today for inventing a light bulb, either.
Best regards,
Wolfgang Denk
I have pushed the earlier patch to arm/next. Tom
Author: Ron Lee ron@debian.org Date: Wed Aug 5 20:14:01 2009 +0200
ARM Don't inline weak symbols
------------------------------------------------------------------------
GCC 4.4 complains about this now.
Signed-off-by: Ron Lee ron@debian.org
Dear Tom,
In message 4AF9C04F.1020509@windriver.com you wrote:
I have pushed the earlier patch to arm/next.
Thnaks, but this being a clear bug fix I think we should include this in the upcoming release?
Best regards,
Wolfgang Denk
Wolfgang,
I have cherry picked the weak sym change into arm/master-sync This is the pull request. Tom
The following changes since commit 0f365273a6c210e0d82f6dca3994be5283e6bf82: Wolfgang Denk (1): Merge branch 'master-sync' of git://git.denx.de/u-boot-arm
are available in the git repository at:
git://git.denx.de/u-boot-arm master-sync
Ron Lee (1): ARM Don't inline weak symbols
lib_arm/board.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
Dear Tom,
In message 4AFB5A23.7030606@windriver.com you wrote:
I have cherry picked the weak sym change into arm/master-sync This is the pull request. Tom
The following changes since commit 0f365273a6c210e0d82f6dca3994be5283e6bf82: Wolfgang Denk (1): Merge branch 'master-sync' of git://git.denx.de/u-boot-arm
are available in the git repository at:
git://git.denx.de/u-boot-arm master-sync
Ron Lee (1): ARM Don't inline weak symbols
lib_arm/board.c | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Tom,
In message 4AF33815.4030101@windriver.com you wrote:
Signed-off-by: Abdoulaye Walsimou Gaye walsimou@walsimou.com
What is this? Are you sure this patch is properly attributed? Was Abdoulaye Walsimou Gaye really the first to submit this patch?
Best regards,
Wolfgang Denk
Would you like the older one ? As you said, they looked the same.
It does not matter what I like. What matters is who is to be credited for this patch, and in my opinion this has to be the first to submit it.
You don't get any credits any more today for inventing a light bulb, either.
Best regards,
Wolfgang Denk
Hello all, May be having tools such as patchwork will avoid people to submit the same patch. Recently linux-mips[1] adopted patchwork and it seems useful (for submitters and maintainers).
Best regards,
[1] http://www.linux-mips.org/archives/linux-mips/2009-11/msg00273.html
Dear Tom Rix,
In message 1257292804-10612-2-git-send-email-Tom.Rix@windriver.com you wrote:
The ARM board LED functions are defined as weak. They add a size overhead if they are not used.
Now they are only defined if CONFIG_STATUS_LED is also defined.
The arm920t and arm926ejs _start function calls these LED functions
bl coloured_LED_init bl red_LED_on
In general, what happens is they call into the weak stubs. Only if the cpu/board provides an overriding function do these calls cause anything meaningful to happen.
Now this noop case is removed and these LED functions are excuted only when CONFIG_STATUS_LED is defined
I don't get it. We use "weak" to avoid #ifdef's, and you insert new #ifdef's? That seems to be the wrong approach to me.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Tom Rix,
In message 1257292804-10612-2-git-send-email-Tom.Rix@windriver.com you wrote:
The ARM board LED functions are defined as weak. They add a size overhead if they are not used.
Now they are only defined if CONFIG_STATUS_LED is also defined.
The arm920t and arm926ejs _start function calls these LED functions
bl coloured_LED_init bl red_LED_on
In general, what happens is they call into the weak stubs. Only if the cpu/board provides an overriding function do these calls cause anything meaningful to happen.
Now this noop case is removed and these LED functions are excuted only when CONFIG_STATUS_LED is defined
I don't get it. We use "weak" to avoid #ifdef's, and you insert new #ifdef's? That seems to be the wrong approach to me.
The arguments for using weak are getting weak :P
Using weak is less relevant with the #ifdef's The benefit now is that boards that use the led functions do not have to define all of them.
I am open to ideas on how to kill weak off completely.
Has a general led driver layer ever been proposed ? Something to convert status led for a mixture of #defines and weak symbols to something that had a register function and some file_ops ?
Tom
Tom
Best regards,
Wolfgang Denk
Dear Tom,
In message 4AF339E1.9060809@windriver.com you wrote:
The arguments for using weak are getting weak :P
:-P
Using weak is less relevant with the #ifdef's
But it's the wrong direction your heading. We should get rid of #ifdef's, not add new ones.
With #ifdef's, you have different versions of the code, which increases the multitude of configurations that actually need to be tested. With weak, you have one version of the code only.
The benefit now is that boards that use the led functions do not have to define all of them.
That's just an indication of a broken implementation.
Normally you would provide the weak functions in a central place, where any board configuration which wants can overwrite them, or not.
I am open to ideas on how to kill weak off completely.
You got it wrong.
We want to kill off the #ifdef's.
Has a general led driver layer ever been proposed ? Something to convert status led for a mixture of #defines and weak symbols to something that had a register function and some file_ops ?
We use status LEDs on many boards, without real issues. It's only AT91 which suffers from this mess.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Tom,
In message 4AF339E1.9060809@windriver.com you wrote:
The arguments for using weak are getting weak :P
:-P
Using weak is less relevant with the #ifdef's
But it's the wrong direction your heading. We should get rid of #ifdef's, not add new ones.
With #ifdef's, you have different versions of the code, which increases the multitude of configurations that actually need to be tested. With weak, you have one version of the code only.
To use the status led api, you have to define CONFIG_STATUS_LED anyway. I did not think this added to the configuration space.
The benefit now is that boards that use the led functions do not have to define all of them.
That's just an indication of a broken implementation.
Normally you would provide the weak functions in a central place, where any board configuration which wants can overwrite them, or not.
I am open to ideas on how to kill weak off completely.
You got it wrong.
We want to kill off the #ifdef's.
My vector is obviously pointing in the wrong direction..
Has a general led driver layer ever been proposed ? Something to convert status led for a mixture of #defines and weak symbols to something that had a register function and some file_ops ?
We use status LEDs on many boards, without real issues. It's only AT91 which suffers from this mess.
I withdraw this patch. I will rethink this and come up with something better.
Tom
Best regards,
Wolfgang Denk
I withdraw this patch. I will rethink this and come up with something better.
I agree weak is better than ifdef. But the led situation on ARM isn't really pleasant when you look in lib_arm/board.c .
When I proposed a simplification of board.c back on Jul 22 ("[RFC] arm/board.c: avoid ifdef using weak default functions", I noticed the led approach and thought it would need cleanup (for example, by moving out of board.c to led.c or something). However, the patch was rejected by JC as he has initcalls as work in progress.
Since we still missing the initcalls (as missing JC), could that patch be reconsidered? I can rebase if there's any interest.
/alessandro
Alessandro Rubini wrote:
I withdraw this patch. I will rethink this and come up with something better.
I agree weak is better than ifdef. But the led situation on ARM isn't really pleasant when you look in lib_arm/board.c .
When I proposed a simplification of board.c back on Jul 22 ("[RFC] arm/board.c: avoid ifdef using weak default functions", I noticed the led approach and thought it would need cleanup (for example, by moving out of board.c to led.c or something). However, the patch was rejected by JC as he has initcalls as work in progress.
Since we still missing the initcalls (as missing JC), could that patch be reconsidered? I can rebase if there's any interest.
Yes I am interested. Please rebase the RFC patch.
Here is the link to the old RFC http://lists.denx.de/pipermail/u-boot/2009-July/057273.html
Thanks, Tom
/alessandro
participants (7)
-
Alessandro Rubini -
Dieter Kiermaier -
Gaye Abdoulaye Walsimou -
myuboot＠fastmail.fm -
Tom -
Tom Rix
-
Wolfgang Denk