[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Stefan Roese]

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")));
 
 /*-----------------------------------------------------------------------
-- 
1.6.0.4

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Haavard Skinnemoen]

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

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Haavard Skinnemoen]

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

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Haavard Skinnemoen]

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

[U-Boot] [PATCH] cfi_flash: Make all flash access functions weak

[Stefan Roese]

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 at denx.de
=====================================================================