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.

---
 drivers/mtd/cfi_flash.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

index 24e9b9f..4d82a51 100644
@@ -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

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

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
=====================================================================

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
=====================================================================

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

Yes.

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
=====================================================================

Hmm...1 #ifdef vs. 1 #ifdef...I'd say that's pretty much the same ;-)

But sure, your way works too.

Haavard

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
...

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
=====================================================================