[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak
This patch defines all flash access functions as weak so that they can be overridden by board specific versions.
This will be used by the upcoming VCTH board support where the NOR FLASH unfortunately can't be accessed memory-mapped. Special accessor functions are needed here.
Signed-off-by: Stefan Roese sr@denx.de --- drivers/mtd/cfi_flash.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 24e9b9f..4d82a51 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -209,38 +209,38 @@ struct cfi_pri_hdr { u8 minor_version; } __attribute__((packed));
-static void flash_write8(u8 value, void *addr) +static void __flash_write8(u8 value, void *addr) { __raw_writeb(value, addr); }
-static void flash_write16(u16 value, void *addr) +static void __flash_write16(u16 value, void *addr) { __raw_writew(value, addr); }
-static void flash_write32(u32 value, void *addr) +static void __flash_write32(u32 value, void *addr) { __raw_writel(value, addr); }
-static void flash_write64(u64 value, void *addr) +static void __flash_write64(u64 value, void *addr) { /* No architectures currently implement __raw_writeq() */ *(volatile u64 *)addr = value; }
-static u8 flash_read8(void *addr) +static u8 __flash_read8(void *addr) { return __raw_readb(addr); }
-static u16 flash_read16(void *addr) +static u16 __flash_read16(void *addr) { return __raw_readw(addr); }
-static u32 flash_read32(void *addr) +static u32 __flash_read32(void *addr) { return __raw_readl(addr); } @@ -251,6 +251,13 @@ static u64 __flash_read64(void *addr) return *(volatile u64 *)addr; }
+void flash_write8(u8 value, void *addr)__attribute__((weak, alias("__flash_write8"))); +void flash_write16(u16 value, void *addr)__attribute__((weak, alias("__flash_write16"))); +void flash_write32(u32 value, void *addr)__attribute__((weak, alias("__flash_write32"))); +void flash_write64(u64 value, void *addr)__attribute__((weak, alias("__flash_write64"))); +u8 flash_read8(void *addr)__attribute__((weak, alias("__flash_read8"))); +u16 flash_read16(void *addr)__attribute__((weak, alias("__flash_read16"))); +u32 flash_read32(void *addr)__attribute__((weak, alias("__flash_read32"))); u64 flash_read64(void *addr)__attribute__((weak, alias("__flash_read64")));
/*-----------------------------------------------------------------------
Stefan Roese sr@denx.de wrote:
This patch defines all flash access functions as weak so that they can be overridden by board specific versions.
This will be used by the upcoming VCTH board support where the NOR FLASH unfortunately can't be accessed memory-mapped. Special accessor functions are needed here.
Signed-off-by: Stefan Roese sr@denx.de
How much overhead does this add? Those things used to be trivial inlines, but can't be if you turn them into weak aliases.
Haavard
On Thursday 13 November 2008, Haavard Skinnemoen wrote:
Stefan Roese sr@denx.de wrote:
This patch defines all flash access functions as weak so that they can be overridden by board specific versions.
This will be used by the upcoming VCTH board support where the NOR FLASH unfortunately can't be accessed memory-mapped. Special accessor functions are needed here.
Signed-off-by: Stefan Roese sr@denx.de
How much overhead does this add? Those things used to be trivial inlines, but can't be if you turn them into weak aliases.
Here the difference for a PPC4xx board (Kilauea):
Old version without weak aliases: text data bss dec hex filename 280964 20232 50788 351984 55ef0 ./u-boot
New version with weak aliases: text data bss dec hex filename 280520 20232 50788 351540 55d34 ./u-boot
So the difference is 444 bytes (with gcc 4.2.2). I have to admit that this is more than I thought. If necessary I could make this weak change conditionally of course. I just didn't want to "pollute" to the source with more #ifdef's.
Any comments?
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
On Monday 17 November 2008, Stefan Roese wrote:
Old version without weak aliases: text data bss dec hex filename 280964 20232 50788 351984 55ef0 ./u-boot
New version with weak aliases: text data bss dec hex filename 280520 20232 50788 351540 55d34 ./u-boot
It's the other way around of course. New version is bigger by 444 bytes.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Stefan Roese sr@denx.de wrote:
Old version without weak aliases: text data bss dec hex filename 280964 20232 50788 351984 55ef0 ./u-boot
New version with weak aliases: text data bss dec hex filename 280520 20232 50788 351540 55d34 ./u-boot
So the difference is 444 bytes (with gcc 4.2.2). I have to admit that this is more than I thought. If necessary I could make this weak change conditionally of course. I just didn't want to "pollute" to the source with more #ifdef's.
Looks like it's 444 bytes _smaller_ with weak functions...that's a bit surprising. From you comment, it sounds like it's really 444 bytes larger. Did you mix up the numbers?
If it's really 444 bytes larger, I think that's a bit much for something as simple as memory accessors. Ideally, they should boil down to a single instruction each.
Perhaps you could allow overriding them in the board header by doing something like
#ifndef flash_write8 # #define flash_write8(value, addr) __raw_writeb(value, addr) ...
in cfi_flash.c?
Haavard
On Monday 17 November 2008, Haavard Skinnemoen wrote:
Stefan Roese sr@denx.de wrote:
Old version without weak aliases: text data bss dec hex filename 280964 20232 50788 351984 55ef0 ./u-boot
New version with weak aliases: text data bss dec hex filename 280520 20232 50788 351540 55d34 ./u-boot
So the difference is 444 bytes (with gcc 4.2.2). I have to admit that this is more than I thought. If necessary I could make this weak change conditionally of course. I just didn't want to "pollute" to the source with more #ifdef's.
Looks like it's 444 bytes _smaller_ with weak functions...that's a bit surprising. From you comment, it sounds like it's really 444 bytes larger. Did you mix up the numbers?
Yes.
If it's really 444 bytes larger, I think that's a bit much for something as simple as memory accessors. Ideally, they should boil down to a single instruction each.
Perhaps you could allow overriding them in the board header by doing something like
#ifndef flash_write8 # #define flash_write8(value, addr) __raw_writeb(value, addr) ...
in cfi_flash.c?
I could do it this way, sure. But how about this version:
static void __flash_write8(u8 value, void *addr) { __raw_writeb(value, addr); } ...
#ifdef CONFIG_CFI_FLASH_USE_WEAK_ACCESSORS void flash_write8(u8 value, void *addr)__attribute__((weak, alias("__flash_write8"))); ... #else #define flash_read8 __flash_read8 ... #endif
We would still have the original accessor functions this way. And the resulting source code looks a little "better" to me (less #ifdef's).
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Stefan Roese sr@denx.de wrote:
I could do it this way, sure. But how about this version:
static void __flash_write8(u8 value, void *addr) { __raw_writeb(value, addr); } ...
#ifdef CONFIG_CFI_FLASH_USE_WEAK_ACCESSORS void flash_write8(u8 value, void *addr)__attribute__((weak, alias("__flash_write8"))); ... #else #define flash_read8 __flash_read8 ... #endif
We would still have the original accessor functions this way. And the resulting source code looks a little "better" to me (less #ifdef's).
Hmm...1 #ifdef vs. 1 #ifdef...I'd say that's pretty much the same ;-)
But sure, your way works too.
Haavard
On Monday 17 November 2008, Haavard Skinnemoen wrote:
Stefan Roese sr@denx.de wrote:
I could do it this way, sure. But how about this version:
static void __flash_write8(u8 value, void *addr) { __raw_writeb(value, addr); } ...
#ifdef CONFIG_CFI_FLASH_USE_WEAK_ACCESSORS void flash_write8(u8 value, void *addr)__attribute__((weak, alias("__flash_write8"))); ... #else #define flash_read8 __flash_read8 ... #endif
We would still have the original accessor functions this way. And the resulting source code looks a little "better" to me (less #ifdef's).
Hmm...1 #ifdef vs. 1 #ifdef...I'd say that's pretty much the same ;-)
Yes right. I first thought you wanted to override the functions on a per-function basis:
#ifndef flash_write8 # define flash_write8(value, addr) __raw_writeb(value, addr) #endif #ifndef flash_write16 # define flash_write16(value, addr) __raw_writew(value, addr) #endif ...
But sure, your way works too.
OK, I'll submit a new patch version later today.
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
participants (2)
-
Haavard Skinnemoen -
Stefan Roese