Code Clean-up (weak functions)

This patch makes all definitions, declarations and usages of weak functions consistent.

Signed-off-by Graeme Russ <[email protected]>

- If the weak alias decleration exceeds 80 columns, the __attribute__ is placed on the following line, indented by one tab

All instances have been replaced by empty functions with an alias. e.g.

    void __do_something (args) {}
    do_something(args) __atttribute__((weak, alias("__do_something")));

- There is no purely weak functions and therfore no longer code like:

   if (do_something)
     do_somthing();

 board/incaip/incaip.c                              |    2 +-
 board/purple/purple.c                              |    2 +-
 board/tb0229/tb0229.c                              |    2 +-
 .../xilinx/ppc405-generic/xilinx_ppc405_generic.c  |    1 -
 common/cmd_boot.c                                  |    6 ++--
 common/cmd_bootm.c                                 |    6 +++-
 common/cmd_elf.c                                   |    4 +-
 common/cmd_ide.c                                   |    6 ++--
 common/cmd_log.c                                   |    3 +-
 common/main.c                                      |    3 +-
 common/serial.c                                    |    3 +-
 cpu/at32ap/cpu.c                                   |    6 +++-
 cpu/blackfin/cpu.h                                 |    2 +-
 cpu/blackfin/reset.c                               |    6 +++-
 cpu/mips/cpu.c                                     |    8 +++---
 cpu/mpc8xxx/ddr/util.c                             |    5 ++-
 cpu/ppc4xx/44x_spd_ddr.c                           |    3 +-
 cpu/ppc4xx/44x_spd_ddr2.c                          |    3 +-
 cpu/ppc4xx/4xx_pci.c                               |    3 +-
 cpu/ppc4xx/4xx_pcie.c                              |    2 +-
 cpu/ppc4xx/fdt.c                                   |    3 +-
 drivers/mtd/cfi_flash.c                            |   24 +++++++++++++------
 drivers/net/mcfmii.c                               |    3 +-
 include/asm-avr32/arch-at32ap700x/clk.h            |    2 +-
 include/asm-mips/reboot.h                          |    2 +-
 include/common.h                                   |    2 +-
 lib_arm/board.c                                    |   21 +++++++++++-----
 lib_ppc/board.c                                    |    6 +++-
 lib_ppc/interrupts.c                               |    4 +-
 29 files changed, 86 insertions(+), 57 deletions(-)

board/incaip/incaip.c +1 -1

index 3b30970..3ee3ac9 100644
@@ -31,7 +31,7 @@
 
 extern uint incaip_get_cpuclk(void);
 
-void _machine_restart(void)
+void machine_restart(void)
 {
       *INCA_IP_WDT_RST_REQ = 0x3f;
 }

board/purple/purple.c +1 -1

index 54bef65..c243487 100644
@@ -54,7 +54,7 @@ extern int    asc_serial_getc         (void);
 extern int     asc_serial_tstc         (void);
 extern void    asc_serial_setbrg       (void);
 
-void _machine_restart(void)
+void machine_restart(void)
 {
       void (*f)(void) = (void *) 0xbfc00000;
 

board/tb0229/tb0229.c +1 -1

index d3f05b2..f74573b 100644
@@ -16,7 +16,7 @@
 #include <asm/reboot.h>
 #include <pci.h>
 
-void _machine_restart(void)
+void machine_restart(void)
 {
       void (*f)(void) = (void *) 0xbfc00000;
 

board/xilinx/ppc405-generic/xilinx_ppc405_generic.c +0 -1

index 9bd1770..57ffdc8 100644
@@ -25,7 +25,6 @@ ulong __get_PCI_freq(void)
 {
       return 0;
 }
-
 ulong get_PCI_freq(void) __attribute__((weak, alias("__get_PCI_freq")));
 
 int __board_pre_init(void)

common/cmd_boot.c +3 -3

index 6024ffe..7d77b8d 100644
@@ -28,12 +28,12 @@
 #include <command.h>
 #include <net.h>
 
-/* Allow ports to override the default behavior */
-__attribute__((weak))
-unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[])
+unsigned long __do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[])
 {
       return entry (argc, argv);
 }
+unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[])
+       __attribute__((weak, alias("__do_go_exec")));
 
 int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 {

common/cmd_bootm.c +4 -2

index a8f85e9..137cf65 100644
@@ -159,13 +159,15 @@ void __board_lmb_reserve(struct lmb *lmb)
 {
       /* please define platform specific board_lmb_reserve() */
 }
-void board_lmb_reserve(struct lmb *lmb) __attribute__((weak, alias("__board_lmb_reserve")));
+void board_lmb_reserve(struct lmb *lmb)
+       __attribute__((weak, alias("__board_lmb_reserve")));
 
 void __arch_lmb_reserve(struct lmb *lmb)
 {
       /* please define platform specific arch_lmb_reserve() */
 }
-void arch_lmb_reserve(struct lmb *lmb) __attribute__((weak, alias("__arch_lmb_reserve")));
+void arch_lmb_reserve(struct lmb *lmb)
+       __attribute__((weak, alias("__arch_lmb_reserve")));
 
 #if defined(__ARM__)
  #define IH_INITRD_ARCH IH_ARCH_ARM

common/cmd_elf.c +2 -2

index 27a4b73..4cd2975 100644
@@ -27,8 +27,6 @@ DECLARE_GLOBAL_DATA_PTR;
 int valid_elf_image (unsigned long addr);
 unsigned long load_elf_image (unsigned long addr);
 
-/* Allow ports to override the default behavior */
-__attribute__((weak))
 unsigned long do_bootelf_exec (ulong (*entry)(int, char *[]), int argc, char *argv[])
 {
       unsigned long ret;

@@ -52,6 +50,8 @@ unsigned long do_bootelf_exec (ulong (*entry)(int, char *[]), int argc, char *ar
 
       return ret;
 }
+unsigned long do_bootelf_exec (ulong (*entry)(int, char *[]), int argc, char *argv[])
+       __attribute__((weak, alias("__do_bootelf_exec")));
 
 /* ======================================================================
 * Interpreter command to boot an arbitrary ELF image from memory.

common/cmd_ide.c +3 -3

index db05f76..0b890f0 100644
@@ -529,7 +529,7 @@ __ide_outb(int dev, int port, unsigned char val)
       outb(val, (ATA_CURR_BASE(dev)+CONFIG_SYS_ATA_PORT_ADDR(port)));
 }
 void inline ide_outb (int dev, int port, unsigned char val)
-               __attribute__((weak, alias("__ide_outb")));
+       __attribute__((weak, alias("__ide_outb")));
 
 unsigned char inline
 __ide_inb(int dev, int port)

@@ -541,7 +541,7 @@ __ide_inb(int dev, int port)
       return val;
 }
 unsigned char inline ide_inb(int dev, int port)
-                       __attribute__((weak, alias("__ide_inb")));
+       __attribute__((weak, alias("__ide_inb")));
 
 #ifdef CONFIG_TUNE_PIO
 int inline

@@ -550,7 +550,7 @@ __ide_set_piomode(int pio_mode)
       return 0;
 }
 int inline ide_set_piomode(int pio_mode)
-                       __attribute__((weak, alias("__ide_set_piomode")));
+       __attribute__((weak, alias("__ide_set_piomode")));
 #endif
 
 void ide_init (void)

common/cmd_log.c +2 -1

index febdb90..854c30d 100644
@@ -70,7 +70,8 @@ unsigned long __logbuffer_base(void)
 {
       return CONFIG_SYS_SDRAM_BASE + gd->bd->bi_memsize - LOGBUFF_LEN;
 }
-unsigned long logbuffer_base (void) __attribute__((weak, alias("__logbuffer_base")));
+unsigned long logbuffer_base (void)
+       __attribute__((weak, alias("__logbuffer_base")));
 
 void logbuff_init_ptrs (void)
 {

common/main.c +2 -1

index a999a5d..5b9da3b 100644
@@ -48,7 +48,8 @@ DECLARE_GLOBAL_DATA_PTR;
 * Board-specific Platform code can reimplement show_boot_progress () if needed
 */
 void inline __show_boot_progress (int val) {}
-void inline show_boot_progress (int val) __attribute__((weak, alias("__show_boot_progress")));
+void inline show_boot_progress (int val)
+       __attribute__((weak, alias("__show_boot_progress")));
 
 #if defined(CONFIG_BOOT_RETRY_TIME) && defined(CONFIG_RESET_TO_RETRY)
 extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);              /* for do_reset() prototype */

common/serial.c +2 -1

index b38d1e7..a7c6946 100644
@@ -75,7 +75,8 @@ struct serial_device *__default_serial_console (void)
 #endif
 }
 
-struct serial_device *default_serial_console(void) __attribute__((weak, alias("__default_serial_console")));
+struct serial_device *default_serial_console(void)
+       __attribute__((weak, alias("__default_serial_console")));
 #endif
 
 int serial_register (struct serial_device *dev)

cpu/at32ap/cpu.c +4 -2

index f92d3e2..4e7eded 100644
@@ -43,6 +43,9 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+void inline __gclk_init(void) {}
+void inline gclk_init (void) __attribute__((weak, alias("__gclk_init")));
+
 int cpu_init(void)
 {
       extern void _evba(void);

@@ -65,8 +68,7 @@ int cpu_init(void)
       sysreg_write(EVBA, (unsigned long)&_evba);
       asm volatile("csrf      %0" : : "i"(SYSREG_EM_OFFSET));
 
-       if(gclk_init)
-               gclk_init();
+       gclk_init();
 
       return 0;
 }

cpu/blackfin/cpu.h +1 -1

index 0a13c28..5aa1787 100644
@@ -27,7 +27,7 @@
 
 #include <command.h>
 
-void board_reset(void) __attribute__((__weak__));
+void board_reset(void);
 void bfin_reset_or_hang(void) __attribute__((__noreturn__));
 void bfin_panic(struct pt_regs *reg);
 void dump(struct pt_regs *regs);

cpu/blackfin/reset.c +4 -2

index d1e34b3..e89b980 100644
@@ -65,6 +65,9 @@ void bfin_reset(void)
       }
 }
 
+void inline __board_reset(void) {}
+void inline board_reset (void) __attribute__((weak, alias("__board_reset")));
+
 /* We need to trampoline ourselves up into L1 since our linker
 * does not have relaxtion support and will only generate a
 * PC relative call with a 25 bit immediate.  This is not enough

@@ -73,8 +76,7 @@ void bfin_reset(void)
 __attribute__ ((__noreturn__))
 static inline void bfin_reset_trampoline(void)
 {
-       if (board_reset)
-               board_reset();
+       board_reset();
       while (1)
               asm("jump (%0);" : : "a" (bfin_reset));
 }

cpu/mips/cpu.c +4 -4

index b7180b0..84c4730 100644
@@ -38,13 +38,13 @@
       :                                                               \
       : "i" (op), "R" (*(unsigned char *)(addr)))
 
-void __attribute__((weak)) _machine_restart(void)
-{
-}
+void inline __machine_restart(void) {}
+void inline machine_restart (void)
+       __attribute__((weak, alias("__machine_restart")));
 
 int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 {
-       _machine_restart();
+       machine_restart();
 
       fprintf(stderr, "*** reset failed ***\n");
       return 0;

cpu/mpc8xxx/ddr/util.c +3 -2

index 27c135b..56de559 100644
@@ -97,7 +97,8 @@ __fsl_ddr_set_lawbar(const common_timing_params_t *memctl_common_params,
       }
 }
 
-__attribute__((weak, alias("__fsl_ddr_set_lawbar"))) void
+void
 fsl_ddr_set_lawbar(const common_timing_params_t *memctl_common_params,
                        unsigned int memctl_interleaved,
-                        unsigned int ctrl_num);
+                        unsigned int ctrl_num)
+       __attribute__((weak, alias("__fsl_ddr_set_lawbar")));

cpu/ppc4xx/44x_spd_ddr.c +2 -1

index 153391e..79976a3 100644
@@ -79,7 +79,8 @@ void __spd_ddr_init_hang (void)
 {
       hang ();
 }
-void spd_ddr_init_hang (void) __attribute__((weak, alias("__spd_ddr_init_hang")));
+void spd_ddr_init_hang (void)
+       __attribute__((weak, alias("__spd_ddr_init_hang")));
 
 /*-----------------------------------------------------------------------------+
  |  General Definition

cpu/ppc4xx/44x_spd_ddr2.c +2 -1

index b40e4b1..d456b97 100644
@@ -171,7 +171,8 @@ void __spd_ddr_init_hang (void)
 {
       hang ();
 }
-void spd_ddr_init_hang (void) __attribute__((weak, alias("__spd_ddr_init_hang")));
+void spd_ddr_init_hang (void)
+       __attribute__((weak, alias("__spd_ddr_init_hang")));
 
 /*
 * To provide an interface for board specific config values in this common

cpu/ppc4xx/4xx_pci.c +2 -1

index e8871fc..b1f90b8 100644
@@ -89,7 +89,8 @@ int __pci_pre_init(struct pci_controller *hose)
 {
       return 1;
 }
-int pci_pre_init(struct pci_controller *hose) __attribute__((weak, alias("__pci_pre_init")));
+int pci_pre_init(struct pci_controller *hose)
+       __attribute__((weak, alias("__pci_pre_init")));
 
 #if defined(CONFIG_405GP) || defined(CONFIG_405EP)
 

cpu/ppc4xx/4xx_pcie.c +1 -1

index fd40d8a..970932b 100644
@@ -729,7 +729,7 @@ int __ppc4xx_init_pcie_port_hw(int port, int rootport)
 #endif /* CONFIG_405EX */
 
 int ppc4xx_init_pcie_port_hw(int port, int rootport)
-__attribute__((weak, alias("__ppc4xx_init_pcie_port_hw")));
+       __attribute__((weak, alias("__ppc4xx_init_pcie_port_hw")));
 
 /*
 * We map PCI Express configuration access into the 512MB regions

cpu/ppc4xx/fdt.c +2 -1

index c55e1cf..cd6ed58 100644
@@ -72,7 +72,8 @@ void __ft_board_setup(void *blob, bd_t *bd)
                      fdt_strerror(rc));
       }
 }
-void ft_board_setup(void *blob, bd_t *bd) __attribute__((weak, alias("__ft_board_setup")));
+void ft_board_setup(void *blob, bd_t *bd
+       __attribute__((weak, alias("__ft_board_setup")));
 
 /*
 * Fixup all PCIe nodes by setting the device_type property

drivers/mtd/cfi_flash.c +16 -8

index e8afe99..5055274 100644
@@ -251,14 +251,22 @@ static u64 __flash_read64(void *addr)
 }
 
 #ifdef CONFIG_CFI_FLASH_USE_WEAK_ACCESSORS
-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")));
+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")));
 #else
 #define flash_write8   __flash_write8
 #define flash_write16  __flash_write16

drivers/net/mcfmii.c +1 -2

index 2b733c6..41e1c94 100644
@@ -218,8 +218,6 @@ int mii_discover_phy(struct eth_device *dev)
 }
 #endif                         /* CONFIG_SYS_DISCOVER_PHY */
 
-void mii_init(void) __attribute__((weak,alias("__mii_init")));
-
 void __mii_init(void)
 {
       FEC_INFO_T *info;

@@ -269,6 +267,7 @@ void __mii_init(void)
       info->dup_spd = miiphy_duplex(dev->name, info->phy_addr) << 16;
       info->dup_spd |= miiphy_speed(dev->name, info->phy_addr);
 }
+void mii_init(void) __attribute__((weak,alias("__mii_init")));
 
 /*
 * Read and write a MII PHY register, routines used by MII Utilities

include/asm-avr32/arch-at32ap700x/clk.h +1 -1

index 7817572..7a23fd8 100644
@@ -82,7 +82,7 @@ static inline unsigned long get_spi_clk_rate(unsigned int dev_id)
 #endif
 
 extern void clk_init(void);
-extern void gclk_init(void) __attribute__((weak));
+extern void gclk_init(void);
 
 /* Board code may need the SDRAM base clock as a compile-time constant */
 #define SDRAMC_BUS_HZ  (MAIN_CLK_RATE >> CONFIG_SYS_CLKDIV_HSB)

include/asm-mips/reboot.h +1 -1

index 978d206..26885c0 100644
@@ -9,6 +9,6 @@
 #ifndef _ASM_REBOOT_H
 #define _ASM_REBOOT_H
 
-extern void _machine_restart(void);
+extern void machine_restart(void);
 
 #endif /* _ASM_REBOOT_H */

include/common.h +1 -1

index 5968036..6583f00 100644
@@ -693,7 +693,7 @@ int pcmcia_init (void);
 /*
 * Board-specific Platform code can reimplement show_boot_progress () if needed
 */
-void __attribute__((weak)) show_boot_progress (int val);
+void show_boot_progress (int val);
 
 #ifdef CONFIG_INIT_CRITICAL
 #error CONFIG_INIT_CRITICAL is deprecated!

lib_arm/board.c +14 -7

index 2358beb..67b9469 100644
@@ -123,19 +123,26 @@ void *sbrk (ptrdiff_t increment)
 * 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 inline 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 inline 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 inline 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 inline 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 inline 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 inline 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 inline yellow_LED_off(void)
+       __attribute__((weak, alias("__yellow_LED_off")));
 
 /************************************************************************
 * Init Utilities                                                      *

lib_ppc/board.c +4 -2

index 289a32a..caebc2b 100644
@@ -206,7 +206,8 @@ void __board_add_ram_info(int use_default)
 {
       /* please define platform specific board_add_ram_info() */
 }
-void board_add_ram_info(int) __attribute__((weak, alias("__board_add_ram_info")));
+void board_add_ram_info(int)
+       __attribute__((weak, alias("__board_add_ram_info")));
 
 
 static int init_func_ram (void)

@@ -643,7 +644,8 @@ int __is_sata_supported(void)
        * board have such issue.*/
       return 1;
 }
-int is_sata_supported(void) __attribute__((weak, alias("__is_sata_supported")));
+int is_sata_supported(void)
+       __attribute__((weak, alias("__is_sata_supported")));
 
 /************************************************************************
 *

lib_ppc/interrupts.c +2 -3

index f603170..696259b 100644
@@ -32,12 +32,12 @@
 #endif
 
 #ifdef CONFIG_SHOW_ACTIVITY
-void board_show_activity (ulong) __attribute__((weak, alias("__board_show_activity")));
-
 void __board_show_activity (ulong dummy)
 {
       return;
 }
+void board_show_activity (ulong)
+       __attribute__((weak, alias("__board_show_activity")));
 #endif /* CONFIG_SHOW_ACTIVITY */
 
 #ifndef CONFIG_SYS_WATCHDOG_FREQ
--

1.6.0.2.GIT

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Hello Graeme,

2008/12/13 Graeme Russ <[email protected]>:

Quoted 3 lines

This patch makes all definitions, declarations and usages of weak functions consistent.

Quoted 1 line

Signed-off-by Graeme Russ <[email protected]>

Just curious:
What is the relation of this patch to the problem discussed earlier:
http://www.mail-archive.com/u-boot at lists.denx.de/msg05367.html Does this patch repair anything, or could it maybe break things?

I ask this because the weak linking seemed not work as many expected, and I guess you cannot test all architectures/boards...

Quoted 3 lines

- If the weak alias decleration exceeds 80 columns, the __attribute__ is placed on the following line, indented by one tab

The benefit on 1 line would be that it is easy recognisable with grep

which instance of a function is actually being used (or in fact should be used due to the fuzzy behaviour of weak in U-boot...) without having to go through all the code. That benefit is gone when it is moved across different lines.

Maybe it is an idea to move the attribute(weak,...) construction to a macro, like in Linux:
#define __weak __attribute__((weak))

In that case it can be changed easier when a non-GCC compiler is being used, and would keep the lines shorter, such that it still fits on 1 line?

Quoted 4 lines

All instances have been replaced by empty functions with an alias. e.g.

    void __do_something (args) {}
    do_something(args) __atttribute__((weak, alias("__do_something")));

Notice that in Linux, the 'alias' construction is not being used massively. Can it be removed here also, or is it somehow mandatory here?

Removing it would keep the lines short as well...

Kind Regards,

Remy

This patch makes all definitions, declarations and usages of weak functions consistent.

Quoted 2 lines · On Saturday 13 December 2008 00:26:26 Graeme Russ wrote:

This patch makes all definitions, declarations and usages of weak functions consistent.

a quick glance shows that it breaks things (the ELF and Blackfin stuff certainly appears to be wrong). i'm guessing you focused on style for the RFC part rather than the result actually being correct ...
-mike

Quoted 4 lines · Mike Frysinger

Quoted 2 lines · On Saturday 13 December 2008 00:26:26 Graeme Russ wrote:

This patch makes all definitions, declarations and usages of weak functions consistent.

Quoted 4 lines

a quick glance shows that it breaks things (the ELF and Blackfin stuff certainly appears to be wrong). i'm guessing you focused on style for the RFC part rather than the result actually being correct ... -mike

Thanks for having a look at this. Would it be possible for you to be a bit more specific about 'wrong' if for nothing other that me gaining a better understanding of how it works, and how it breaks

Quoted 10 lines · On Monday 22 December 2008 04:16:33 Graeme Russ wrote:

Quoted 8 lines · Mike Frysinger

Quoted 2 lines · On Saturday 13 December 2008 00:26:26 Graeme Russ wrote:

This patch makes all definitions, declarations and usages of weak functions consistent.

a quick glance shows that it breaks things (the ELF and Blackfin stuff certainly appears to be wrong). i'm guessing you focused on style for the RFC part rather than the result actually being correct ... -mike

Quoted 3 lines

Thanks for having a look at this. Would it be possible for you to be a bit more specific about 'wrong' if for nothing other that me gaining a better understanding of how it works, and how it breaks

you set the aliases to functions that do not exist -mike

Quoted 11 lines · Mike Frysinger

Quoted 10 lines · On Monday 22 December 2008 04:16:33 Graeme Russ wrote:

Quoted 8 lines · Mike Frysinger

Quoted 2 lines · On Saturday 13 December 2008 00:26:26 Graeme Russ wrote:

This patch makes all definitions, declarations and usages of weak functions consistent.

a quick glance shows that it breaks things (the ELF and Blackfin stuff certainly appears to be wrong). i'm guessing you focused on style for the RFC part rather than the result actually being correct ... -mike

Quoted 5 lines

Quoted 3 lines

Thanks for having a look at this. Would it be possible for you to be a bit more specific about 'wrong' if for nothing other that me gaining a better understanding of how it works, and how it breaks

you set the aliases to functions that do not exist

Ack - I can see that for the ELF - the main function needs to be renamed __do_bootelf_exec ()

I cannot see the problem with Blackfin - I will freely admit that Blackfin has been fundamentally changed (any therefore needs thorough testing), but unless the Blackfin toolchain treats weak function linking differently, it _should_ "just still work"(tm)

Maybe I am not seeing the obvious?

This patch makes all definitions, declarations and usages of weak functions consistent.

Signed-off-by Graeme Russ <[email protected]>

---

WARNING: This patch hits a _lot_ of arches - Please

The following patch applies the following rules:

- All functions are defined without __attribute__((weak)) in header files

- All weak functions are declared as __function() in the source file with funtion() __attribute__((weak, alias("function"))); on the line immediately after the closing brace of __function() - for example:

     void __do_something (args)
     {
     ...some code...
     }
     do_something(args) __atttribute__((weak, alias("__do_something")));

- There is no purely weak functions and therfore no longer code like:

    if (do_something)
      do_somthing();
  All instances have been replaced by empty functions with an alias. e.g.
     void __do_something (args) {}
     do_something(args) __atttribute__((weak, alias("__do_something")));

board/purple/purple.c +1 -1

index 54bef65..c243487 100644
@@ -54,7 +54,7 @@ extern int	asc_serial_getc		(void);
 extern int	asc_serial_tstc		(void);
 extern void	asc_serial_setbrg	(void);
 
-void _machine_restart(void)
+void machine_restart(void)
 {
 	void (*f)(void) = (void *) 0xbfc00000;
 

board/tb0229/tb0229.c +0 -0

index d3f05b2..f74573b 100644

cpu/mips/cpu.c +1 -3

index b7180b0..84c4730 100644
@@ -38,13 +38,13 @@
 	:								\
 	: "i" (op), "R" (*(unsigned char *)(addr)))
 
-void __attribute__((weak)) _machine_restart(void)
-{
-}
+void inline __machine_restart(void) {}

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

All instances have been replaced by empty functions with an alias. e.g.

    void __do_something (args) {}
    do_something(args) __atttribute__((weak, alias("__do_something")));

Notice that in Linux, the 'alias' construction is not being used massively. Can it be removed here also, or is it somehow mandatory here?

I don't think it is mandatory but it is in the majority in u-boot.

Quoted 7 lines

- There is no purely weak functions and therfore no longer code like:

    if (do_something)
      do_somthing();
  All instances have been replaced by empty functions with an alias. e.g.
     void __do_something (args) {}
     do_something(args) __atttribute__((weak, alias("__do_something")));

Quoted 6 lines

Quoted 6 lines

- There is no purely weak functions and therfore no longer code like:

    if (do_something)
      do_somthing();
  All instances have been replaced by empty functions with an alias. e.g.
     void __do_something (args) {}
     do_something(args) __atttribute__((weak, alias("__do_something")));

Curious as to why you removed such code? Was it because it didn't work? If so I might have an answer for that. See my post:
"Re: [U-Boot] [PATCH V4] cmd_bdinfo: move implementation to arch instead of common"

Jocke

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

All instances have been replaced by empty functions with an alias. e.g.

     void __do_something (args) {}
     do_something(args) __atttribute__((weak, alias("__do_something")));

Good to know. This doc also helps:
http://docs.sun.com/app/docs/doc/817-1984/chapter2-11?l=en&a=view
Then,
- we must try to not leave undefined weak symbols at all,
- or check the symbol before invocation for the safety in case of NULL dereference.

Create a default fallback routine that can be used if there is no strong implementation:
-------------------------------------------------------------------------------------------
__attribute__((weak)) unsigned long long printk_clock(void)

{
        return sched_clock();
}

-------------------------------------------------------------------------------------------
and here is an example of the strong implementation (from the ARM architecture):
-------------------------------------------------------------------------------------------
unsigned long long printk_clock(void)
{
return (unsigned long long)(jiffies - INITIAL_JIFFIES) *
(1000000000 / HZ);
}
-------------------------------------------------------------------------------------------

If the strong implementation is available, the weak is simply discarded during linking, if the strong is omitted, the weak is used as fallback. This is a clean, lean and mean example without complex/superfluous aliases or checks for NULL pointers. It should not get any harder than this...

Quoted 17 lines · Remy Bohmer

Create a default fallback routine that can be used if there is no strong implementation:
-------------------------------------------------------------------------------------------
__attribute__((weak)) unsigned long long printk_clock(void)

{
        return sched_clock();
}

-------------------------------------------------------------------------------------------
and here is an example of the strong implementation (from the ARM architecture):
-------------------------------------------------------------------------------------------
unsigned long long printk_clock(void)
{
return (unsigned long long)(jiffies - INITIAL_JIFFIES) *
(1000000000 / HZ);
}
-------------------------------------------------------------------------------------------

Quoted 5 lines

If the strong implementation is available, the weak is simply discarded during linking, if the strong is omitted, the weak is used as fallback. This is a clean, lean and mean example without complex/superfluous aliases or checks for NULL pointers. It should not get any harder than this...

+1. I like this one, and U-Boot/MIPS _machine_restart() is implemented in the same manner.

-----Original Message-----
From: Shinya Kuribayashi [mailto:skuribay at ruby.dti.ne.jp]
Sent: den 24 december 2008 17:55
To: Joakim Tjernlund
Cc: 'Shinya Kuribayashi'; 'Graeme Russ'; u-boot at lists.denx.de
Subject: Re: [U-Boot] [RFC][PATCH] Code Clean-up (weak functions)

Quoted 7 lines · Joakim Tjernlund

Quoted 6 lines

Quoted 6 lines

- There is no purely weak functions and therfore no longer code like:

    if (do_something)
      do_somthing();
  All instances have been replaced by empty functions with an alias. e.g.
     void __do_something (args) {}
     do_something(args) __atttribute__((weak, alias("__do_something")));

Quoted 5 lines

Curious as to why you removed such code? Was it because it didn't work? If so I might have an answer for that. See my post:
"Re: [U-Boot] [PATCH V4] cmd_bdinfo: move implementation to arch instead of common"

Jocke

Good to know. This doc also helps:

http://docs.sun.com/app/docs/doc/817-1984/chapter2-11?l=en&a=view

Then,
- we must try to not leave undefined weak symbols at all,
- or check the symbol before invocation for the safety in case of NULL dereference.

Checking for NULL before invocation won't work unless you fix the relocation routine not to relocate NULL values. That is what I have been trying to say several times now. That fix should be performed regardless so no nasty surprises will happen in the future. Perhaps there are other cases besides weak symbols too.
You can either fix the assembler routine in all start.S or make all relevant boards use the C-versions I posted.

Jocke

Quoted 1 line · Graeme Russ

Quoted 9 lines

- All weak functions are declared as __function() in the source file with funtion() __attribute__((weak, alias("function"))); on the line
immediately
after the closing brace of __function() - for example:

    void __do_something (args)
    {
    ...some code...
    }
    do_something(args) __atttribute__((weak, alias("__do_something")));

Why do we need to being consistent? I don't see real/technical benefits to doing so. Using alias or not should be each developer's business. I completely disagree with forcing alias use here.

Quoted 8 lines

board/incaip/incaip.c +0 -0

index 3b30970..3ee3ac9 100644
@@ -31,7 +31,7 @@
 
 extern uint incaip_get_cpuclk(void);
 

board/incaip/incaip.c line 37

Quoted 5 lines

board/incaip/incaip.c +1 -1

-void _machine_restart(void)
+void machine_restart(void)
 {
       *INCA_IP_WDT_RST_REQ = 0x3f;
 }

Why change the function name? It's derived from Linux/MIPS, and I'd like to keep consistent with with him. Please don't touch it.

Quoted 21 lines

cpu/mips/cpu.c +4 -4

index b7180b0..84c4730 100644
@@ -38,13 +38,13 @@
       :                                                               \
       : "i" (op), "R" (*(unsigned char *)(addr)))
 
-void __attribute__((weak)) _machine_restart(void)
-{
-}
+void inline __machine_restart(void) {}
+void inline machine_restart (void)
+       __attribute__((weak, alias("__machine_restart")));
 
 int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 {
-       _machine_restart();
+       machine_restart();
 

cpu/mips/cpu.c line 50

Quoted 2 lines

cpu/mips/cpu.c +0 -0

       fprintf(stderr, "*** reset failed ***\n");
       return 0;

Why inline? It seems totally wrong to me.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Notice that in Linux, the 'alias' construction is not being used

massively. Can it be removed here also, or is it somehow mandatory here?

I don't think it is mandatory but it is in the majority in u-boot.

If it's not mandatory, please drop all aliases. That saves source code size (not generated u-boot image size) a little bit. Majority or not does not make sense here.