[U-Boot] [PATCH] Export redesign
Simon Glass
sjg at chromium.org
Tue Nov 25 14:33:04 CET 2014
Hi Martin,
On 25 November 2014 at 03:28, Martin Dorwig <dorwig at tetronik.com> wrote:
> this is an atempt to make the export of functions typesafe.
> I replaced the jumptable void ** by a struct with function pointers.
> The EXPORT_FUNC macro now has 2 fixed parameters and one
> variadic parameter
> The first is the return type of the function,
> the second the name of the function,
> and the third is the list of parameters the function takes.
> The Concept of using enums as indexes for the stubs is keept.
> the EXPORT_FUNC macros are now expanded four times,
> 1. to generate the enums
> 2. to declare the members of the struct
> 3. to initialize the structmember pointers
> 4. to call the functions in stubs.c
>
> Signed-off-by: Martin Dorwig <dorwig at tetronik.com>
Looks great! Just a few comments from me, but others might chime in.
(Note when you send v2 you should add a change log - if using patman
then there are instructions there)
I tested this works on x86 (with a few tweaks as below), compatible
with the existing ABI.
Also what are the XF_... enums used for now?
> ---
>
> arch/blackfin/cpu/cpu.c | 2 +-
> board/BuS/eb_cpux9k2/cpux9k2.c | 2 +-
> common/cmd_load.c | 2 +-
> common/console.c | 20 +++++-----
> common/exports.c | 51 ++++++++++++++----------
> examples/standalone/stubs.c | 30 +++++++-------
> include/_exports.h | 84 +++++++++++++++++++++++++--------------
> include/asm-generic/global_data.h | 2 +-
> include/exports.h | 20 +++++++---
> 9 files changed, 129 insertions(+), 84 deletions(-)
You should also update doc/README.standalone a little.
>
> diff --git a/arch/blackfin/cpu/cpu.c b/arch/blackfin/cpu/cpu.c
> index b7f1188..59c470f 100644
> --- a/arch/blackfin/cpu/cpu.c
> +++ b/arch/blackfin/cpu/cpu.c
> @@ -121,7 +121,7 @@ static void display_global_data(void)
> printf(" |-ram_size: %lx\n", gd->ram_size);
> printf(" |-env_addr: %lx\n", gd->env_addr);
> printf(" |-env_valid: %lx\n", gd->env_valid);
> - printf(" |-jt(%p): %p\n", gd->jt, *(gd->jt));
> + printf(" |-jt(%p): %p\n", gd->jt, gd->jt->get_version);
> printf(" \\-bd: %p\n", gd->bd);
> printf(" |-bi_boot_params: %lx\n", bd->bi_boot_params);
> printf(" |-bi_memstart: %lx\n", bd->bi_memstart);
> diff --git a/board/BuS/eb_cpux9k2/cpux9k2.c b/board/BuS/eb_cpux9k2/cpux9k2.c
> index 5e4778e..76ad7c4 100644
> --- a/board/BuS/eb_cpux9k2/cpux9k2.c
> +++ b/board/BuS/eb_cpux9k2/cpux9k2.c
> @@ -98,7 +98,7 @@ int misc_init_r(void)
> puts("Error: invalid MAC at EEPROM\n");
> }
> }
> - gd->jt[XF_do_reset] = (void *) do_reset;
> + gd->jt->do_reset = do_reset;
>
> #ifdef CONFIG_STATUS_LED
> status_led_set(STATUS_LED_BOOT, STATUS_LED_BLINKING);
> diff --git a/common/cmd_load.c b/common/cmd_load.c
> index f6e522c..d043e6d 100644
> --- a/common/cmd_load.c
> +++ b/common/cmd_load.c
> @@ -222,7 +222,7 @@ static int read_record(char *buf, ulong len)
> }
>
> /* Check for the console hangup (if any different from serial) */
> - if (gd->jt[XF_getc] != getc) {
> + if (gd->jt->getc != getc) {
> if (ctrlc()) {
> return (-1);
> }
> diff --git a/common/console.c b/common/console.c
> index 5a2f411..08cf188 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -124,13 +124,13 @@ static int console_setfile(int file, struct stdio_dev * dev)
> */
> switch (file) {
> case stdin:
> - gd->jt[XF_getc] = dev->getc;
> - gd->jt[XF_tstc] = dev->tstc;
> + gd->jt->getc = getc;
> + gd->jt->tstc = tstc;
> break;
> case stdout:
> - gd->jt[XF_putc] = dev->putc;
> - gd->jt[XF_puts] = dev->puts;
> - gd->jt[XF_printf] = printf;
> + gd->jt->putc = putc;
> + gd->jt->puts = puts;
> + gd->jt->printf = printf;
> break;
> }
> break;
> @@ -722,11 +722,11 @@ int console_init_r(void)
> #endif
>
> /* set default handlers at first */
> - gd->jt[XF_getc] = serial_getc;
> - gd->jt[XF_tstc] = serial_tstc;
> - gd->jt[XF_putc] = serial_putc;
> - gd->jt[XF_puts] = serial_puts;
> - gd->jt[XF_printf] = serial_printf;
> + gd->jt->getc = serial_getc;
> + gd->jt->tstc = serial_tstc;
> + gd->jt->putc = serial_putc;
> + gd->jt->puts = serial_puts;
> + gd->jt->printf = serial_printf;
>
> /* stdin stdout and stderr are in environment */
> /* scan for it */
> diff --git a/common/exports.c b/common/exports.c
> index b97ca48..a15d871 100644
> --- a/common/exports.c
> +++ b/common/exports.c
> @@ -1,11 +1,14 @@
> #include <common.h>
> #include <exports.h>
> #include <spi.h>
> +#include <i2c.h>
>
> DECLARE_GLOBAL_DATA_PTR;
>
> -__attribute__((unused)) static void dummy(void)
> -{
> +#define NOT_EXPORTED(x) \
> +void dummy_ ## x(void)\
> +{\
> + printf("%s not exported\n", #x);\
> }
>
> unsigned long get_version(void)
> @@ -13,31 +16,39 @@ unsigned long get_version(void)
> return XF_VERSION;
> }
>
> -/* Reuse _exports.h with a little trickery to avoid bitrot */
> -#define EXPORT_FUNC(sym) gd->jt[XF_##sym] = (void *)sym;
> +#ifndef CONFIG_CMD_I2C
> +NOT_EXPORTED(i2c_read)
> +NOT_EXPORTED(i2c_write)
> +#endif
>
> -#if !defined(CONFIG_X86) && !defined(CONFIG_PPC)
> -# define install_hdlr dummy
> -# define free_hdlr dummy
> -#else /* kludge for non-standard function naming */
> -# define install_hdlr irq_install_handler
> -# define free_hdlr irq_free_handler
> +void install_hdlr(int irq, interrupt_handler_t handler, void *p)
> +{
> +#if defined(CONFIG_X86) || defined(CONFIG_PPC)
> + irq_install_handler(irq, handler, p);
> #endif
> -#ifndef CONFIG_CMD_I2C
> -# define i2c_write dummy
> -# define i2c_read dummy
> +}
> +
> +void free_hdlr(int i)
> +{
> +#if defined(CONFIG_X86) || defined(CONFIG_PPC)
> + irq_free_handler(i);
> #endif
> +}
> +
> #ifndef CONFIG_CMD_SPI
> -# define spi_init dummy
> -# define spi_setup_slave dummy
> -# define spi_free_slave dummy
> -# define spi_claim_bus dummy
> -# define spi_release_bus dummy
> -# define spi_xfer dummy
> +NOT_EXPORTED(spi_init)
> +NOT_EXPORTED(spi_setup_slave)
> +NOT_EXPORTED(spi_free_slave)
> +NOT_EXPORTED(spi_claim_bus)
> +NOT_EXPORTED(spi_release_bus)
> +NOT_EXPORTED(spi_xfer)
This needs a rebase to mainline.
> #endif
>
> +
> +#define EXPORT_FUNC(a, x, ...) gd->jt->x = x;
> +
> void jumptable_init(void)
> {
> - gd->jt = malloc(XF_MAX * sizeof(void *));
> + gd->jt = malloc(sizeof(struct jt_funcs));
> #include <_exports.h>
> }
> diff --git a/examples/standalone/stubs.c b/examples/standalone/stubs.c
> index 0bf690e..2f14c55 100644
> --- a/examples/standalone/stubs.c
> +++ b/examples/standalone/stubs.c
> @@ -13,7 +13,7 @@
> static void **jt;
> gd_t *global_data;
>
In this file there are a few strange x86 things hidden behind
CONFIG_X86 - I get type mismatch warnings. Very simple to fix up.
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -26,7 +26,7 @@ gd_t *global_data;
> * r2 holds the pointer to the global_data, r11 is a call-clobbered
> * register
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -41,7 +41,7 @@ gd_t *global_data;
> * x18 holds the pointer to the global_data, x9 is a call-clobbered
> * register
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -54,7 +54,7 @@ gd_t *global_data;
> * r9 holds the pointer to the global_data, ip is a call-clobbered
> * register
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -70,7 +70,7 @@ gd_t *global_data;
> * it; however, GCC/mips generates an additional `nop' after each asm
> * statement
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -82,7 +82,7 @@ gd_t *global_data;
> /*
> * gp holds the pointer to the global_data, r8 is call-clobbered
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -98,7 +98,7 @@ gd_t *global_data;
> * d7 holds the pointer to the global_data, a0 is a call-clobbered
> * register
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -113,7 +113,7 @@ gd_t *global_data;
> /*
> * r31 holds the pointer to the global_data. r5 is a call-clobbered.
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -126,7 +126,7 @@ gd_t *global_data;
> * P3 holds the pointer to the global_data, P0 is a call-clobbered
> * register
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl _" #x "\n_" \
> #x ":\n" \
> @@ -138,7 +138,7 @@ gd_t *global_data;
> /*
> * r6 holds the pointer to the global_data. r8 is call clobbered.
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile( \
> " .globl\t" #x "\n" \
> #x ":\n" \
> @@ -151,7 +151,7 @@ gd_t *global_data;
> /*
> * r13 holds the pointer to the global_data. r1 is a call clobbered.
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .align 2\n" \
> " .globl " #x "\n" \
> @@ -169,7 +169,7 @@ gd_t *global_data;
> /*
> * g7 holds the pointer to the global_data. g1 is call clobbered.
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile( \
> " .globl\t" #x "\n" \
> #x ":\n" \
> @@ -185,7 +185,7 @@ gd_t *global_data;
> * r16 holds the pointer to the global_data. gp is call clobbered.
> * not support reduced register (16 GPR).
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -198,7 +198,7 @@ gd_t *global_data;
> * r10 holds the pointer to the global_data, r13 is a call-clobbered
> * register
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile ( \
> " .globl " #x "\n" \
> #x ":\n" \
> @@ -211,7 +211,7 @@ gd_t *global_data;
> /*
> * r25 holds the pointer to the global_data. r10 is call clobbered.
> */
> -#define EXPORT_FUNC(x) \
> +#define EXPORT_FUNC(a, x, ...) \
> asm volatile( \
> " .align 4\n" \
> " .globl " #x "\n" \
> diff --git a/include/_exports.h b/include/_exports.h
> index 349a3c5..7f18645 100644
> --- a/include/_exports.h
> +++ b/include/_exports.h
> @@ -1,32 +1,58 @@
> /*
> - * You do not need to use #ifdef around functions that may not exist
> + * You need to use #ifdef around functions that may not exist
> * in the final configuration (such as i2c).
> + * define a dummy function here, and implement it in exports.c
> + * As an example see the CONFIG_CMD_I2C sections below
> */
> -EXPORT_FUNC(get_version)
> -EXPORT_FUNC(getc)
> -EXPORT_FUNC(tstc)
> -EXPORT_FUNC(putc)
> -EXPORT_FUNC(puts)
> -EXPORT_FUNC(printf)
> -EXPORT_FUNC(install_hdlr)
> -EXPORT_FUNC(free_hdlr)
> -EXPORT_FUNC(malloc)
> -EXPORT_FUNC(free)
> -EXPORT_FUNC(udelay)
> -EXPORT_FUNC(get_timer)
> -EXPORT_FUNC(vprintf)
> -EXPORT_FUNC(do_reset)
> -EXPORT_FUNC(getenv)
> -EXPORT_FUNC(setenv)
> -EXPORT_FUNC(simple_strtoul)
> -EXPORT_FUNC(strict_strtoul)
> -EXPORT_FUNC(simple_strtol)
> -EXPORT_FUNC(strcmp)
> -EXPORT_FUNC(i2c_write)
> -EXPORT_FUNC(i2c_read)
> -EXPORT_FUNC(spi_init)
> -EXPORT_FUNC(spi_setup_slave)
> -EXPORT_FUNC(spi_free_slave)
> -EXPORT_FUNC(spi_claim_bus)
> -EXPORT_FUNC(spi_release_bus)
> -EXPORT_FUNC(spi_xfer)
> + EXPORT_FUNC(unsigned long, get_version, void)
> + EXPORT_FUNC(int, getc, void)
> + EXPORT_FUNC(int, tstc, void)
> + EXPORT_FUNC(void, putc, const char)
> + EXPORT_FUNC(void, puts, const char *)
> + EXPORT_FUNC(int, printf, const char*, ...)
> + EXPORT_FUNC(void, install_hdlr,
> + int, interrupt_handler_t, void*)
> + EXPORT_FUNC(void, free_hdlr, int)
> + EXPORT_FUNC(void *, malloc, size_t)
> + EXPORT_FUNC(void, free, void *)
> + EXPORT_FUNC(void, udelay, unsigned long)
> + EXPORT_FUNC(unsigned long, get_timer, unsigned long)
> + EXPORT_FUNC(int, vprintf, const char *, va_list)
> + EXPORT_FUNC(int, do_reset, cmd_tbl_t *, int , int , char * const [])
> + EXPORT_FUNC(char *, getenv, const char*)
> + EXPORT_FUNC(int, setenv, const char *, const char *)
> + EXPORT_FUNC(unsigned long, simple_strtoul,
> + const char *, char **, unsigned int)
> + EXPORT_FUNC(int, strict_strtoul,
> + const char *, unsigned int , unsigned long *)
> + EXPORT_FUNC(long, simple_strtol, const char *, char **, unsigned int)
> + EXPORT_FUNC(int, strcmp, const char *cs, const char *ct)
> +#ifdef CONFIG_CMD_I2C
> + EXPORT_FUNC(int, i2c_write, uchar, uint, int , uchar * , int)
> + EXPORT_FUNC(int, i2c_read, uchar, uint, int , uchar * , int)
> +#else
> + EXPORT_FUNC(void, dummy_i2c_write, void)
> + EXPORT_FUNC(void, dummy_i2c_read, void)
> +#endif
> +
> +#ifdef CONFIG_CMD_SPI
> + EXPORT_FUNC(void, spi_init, void)
> + EXPORT_FUNC(struct spi_slave *, spi_setup_slave,
> + unsigned int, unsigned int, unsigned int, unsigned int)
> + EXPORT_FUNC(void, spi_free_slave, struct spi_slave *)
> + EXPORT_FUNC(int, spi_claim_bus, struct spi_slave *)
> + EXPORT_FUNC(void, spi_release_bus, struct spi_slave *)
> + EXPORT_FUNC(int, spi_xfer, struct spi_slave *,
> + unsigned int, const void *, void *, unsigned long)
> +#else
> + EXPORT_FUNC(void, dummy_spi_init, void)
> + EXPORT_FUNC(void, dummy_spi_setup_slave, void)
> + EXPORT_FUNC(void, dummy_spi_free_slave, void)
> + EXPORT_FUNC(void, dummy_spi_claim_bus, void)
> + EXPORT_FUNC(void, dummy_spi_release_bus, void)
> + EXPORT_FUNC(void, dummy_spi_xfer, void)
> +#endif
> + EXPORT_FUNC(unsigned long, ustrtoul,
> + const char *, char **, unsigned int)
> + EXPORT_FUNC(unsigned long long, ustrtoull,
> + const char *, char **, unsigned int)
> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> index 74df210..f8b1919 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -73,7 +73,7 @@ typedef struct global_data {
> const void *fdt_blob; /* Our device tree, NULL if none */
> void *new_fdt; /* Relocated FDT */
> unsigned long fdt_size; /* Space reserved for relocated FDT */
> - void **jt; /* jump table */
> + struct jt_funcs *jt; /* jump table */
> char env_buf[32]; /* buffer for getenv() before reloc. */
> #ifdef CONFIG_TRACE
> void *trace_buff; /* The trace buffer */
> diff --git a/include/exports.h b/include/exports.h
> index 41d5085..5211aed 100644
> --- a/include/exports.h
> +++ b/include/exports.h
> @@ -10,19 +10,19 @@ int tstc(void);
> void putc(const char);
> void puts(const char*);
> int printf(const char* fmt, ...);
> -void install_hdlr(int, void (*interrupt_handler_t)(void *), void*);
> +void install_hdlr(int, interrupt_handler_t, void*);
> void free_hdlr(int);
> void *malloc(size_t);
> void free(void*);
> void __udelay(unsigned long);
> unsigned long get_timer(unsigned long);
> int vprintf(const char *, va_list);
> -unsigned long simple_strtoul(const char *cp,char **endp,unsigned int base);
> +unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base);
> int strict_strtoul(const char *cp, unsigned int base, unsigned long *res);
> char *getenv (const char *name);
> int setenv (const char *varname, const char *varvalue);
> -long simple_strtol(const char *cp,char **endp,unsigned int base);
> -int strcmp(const char * cs,const char * ct);
> +long simple_strtol(const char *cp, char **endp, unsigned int base);
> +int strcmp(const char *cs, const char *ct);
> unsigned long ustrtoul(const char *cp, char **endp, unsigned int base);
> unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base);
> #if defined(CONFIG_CMD_I2C)
> @@ -35,13 +35,21 @@ void app_startup(char * const *);
> #endif /* ifndef __ASSEMBLY__ */
>
> enum {
> -#define EXPORT_FUNC(x) XF_ ## x ,
> +#define EXPORT_FUNC(res, func, ...) XF_##func,
> #include <_exports.h>
> -#undef EXPORT_FUNC
> +#undef EXPORT_FUNC
>
> XF_MAX
> };
>
> +
> +struct jt_funcs {
> +#define EXPORT_FUNC(res, func, ...) res(*func)(__VA_ARGS__);
> +#include <_exports.h>
> +#undef EXPORT_FUNC
> +};
> +
> +
> #define XF_VERSION 6
>
> #if defined(CONFIG_X86)
> --
> 1.8.1.4
>
Regards,
Simon
More information about the U-Boot
mailing list