Ronald G. Minnich (rminnich@gmail.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/2582
-gerrit
commit 860bbd9ce42e6037b1bbfb5492a2235df55adb3a Author: Ronald G. Minnich rminnich@gmail.com Date: Mon Mar 4 09:46:31 2013 -0800
ARM: remove code that is IMHO a dangerous design
OK, this is tl;dr. But I need to write this in hopes we make sure we don't put code like this into coreboot. Ever.
Our excuse in this case is that it was imported, not obviously wrong, and easily changed. It made sense to get it in, make it work, then do a cleanup pass, because changing everything up front is almost impossible to debug.
The exynos code has bunch of base register values, e.g.
These are base addresses of things that look like a memory-mapped struct. To get these to a pointer, they created the following macro, which creates an inline function.
static inline unsigned int samsung_get_base_##device(void) \ { \ return cpu_is_exynos5() ? EXYNOS5_##base : 0; \ }
And then invoke it 31 times in a .h file, e.g.: SAMSUNG_BASE(clock, CLOCK_BASE)
to create 31 functions.
And then use it: struct exynos5_clock *clk = (struct exynos5_clock *)samsung_get_base_clock();
OK, what's wrong with this? It's easier to ask what's right with it. Answer: nothing.
I have a long list of what's wrong, and I may leave some things out, but here goes: 1. the "function" can return a NULL if we're not on exynos5. Most uses of the code don't check the return value. 2. And why would this function be running, if we're not on an exynos5? Why compile it in? 3. Note the cast everywhere a samsung_get_base_xxx is used. The function returns an untyped variable, requiring the *user* to get two things right: the cast, and the function invocation. One can replace that _clock(); with _power(); in the code above, and they will be referencing the wrong registers, and they'll never get an error! We have a C compiler; use it to type data. 4. You're generating 31 functions using cpp each and every time the file is included. The C compiler has to parse these each time. It's not at all like a simple cpp macro which is only generated on use. 5. You can't tags or etags this code 6. In fact, any kind of analysis tool will be unable to do anything with this cpp magic.
That's only a partial list.
So what's the right way to do it? Just make typed constants, viz:
Or, since I expect people will want the lower case function syntax, I've left it that way:
Now we've got something that is efficient, and we don't even need to protect with any more.
Hence this change. We've got something that is type checked, does not require users to cast on each use, will catch simple programming errors, can be analyzed with standard tools, and builds faster.
So if we make a mistake: struct exynos5_clock *clk = samsung_get_base_adc();
We'll see it: src/cpu/samsung/exynos5250/clock.c: In function 'get_pll_clk': src/cpu/samsung/exynos5250/clock.c:183:3: error: initialization from incompatible pointer type [-Werror]
which we would not have seen before.
As a minor benefit, it shaves most of a second off the compilation.
Change-Id: Ie67bc4bc038a8dd1837b977d07332d7d7fd6be1f Signed-off-by: Ronald G. Minnich rminnich@gmail.com --- src/cpu/samsung/exynos5-common/pwm.c | 10 ++-- src/cpu/samsung/exynos5-common/sromc.c | 2 +- src/cpu/samsung/exynos5-common/timer.c | 2 +- src/cpu/samsung/exynos5-common/wdt.c | 4 +- src/cpu/samsung/exynos5250/clock.c | 20 ++++---- src/cpu/samsung/exynos5250/cpu.h | 85 +++++++++++++--------------------- src/cpu/samsung/exynos5250/power.c | 22 ++++----- 7 files changed, 62 insertions(+), 83 deletions(-)
diff --git a/src/cpu/samsung/exynos5-common/pwm.c b/src/cpu/samsung/exynos5-common/pwm.c index da465d9..0fc571b 100644 --- a/src/cpu/samsung/exynos5-common/pwm.c +++ b/src/cpu/samsung/exynos5-common/pwm.c @@ -32,7 +32,7 @@ int pwm_enable(int pwm_id) { const struct s5p_timer *pwm = - (struct s5p_timer *)samsung_get_base_timer(); + samsung_get_base_timer(); unsigned long tcon;
tcon = readl(&pwm->tcon); @@ -46,7 +46,7 @@ int pwm_enable(int pwm_id) int pwm_check_enabled(int pwm_id) { const struct s5p_timer *pwm = - (struct s5p_timer *)samsung_get_base_timer(); + samsung_get_base_timer(); const unsigned long tcon = readl(&pwm->tcon);
return tcon & TCON_START(pwm_id); @@ -55,7 +55,7 @@ int pwm_check_enabled(int pwm_id) void pwm_disable(int pwm_id) { const struct s5p_timer *pwm = - (struct s5p_timer *)samsung_get_base_timer(); + samsung_get_base_timer(); unsigned long tcon;
tcon = readl(&pwm->tcon); @@ -84,7 +84,7 @@ static unsigned long pwm_calc_tin(int pwm_id, unsigned long freq) int pwm_config(int pwm_id, int duty_ns, int period_ns) { const struct s5p_timer *pwm = - (struct s5p_timer *)samsung_get_base_timer(); + samsung_get_base_timer(); unsigned int offset; unsigned long tin_rate; unsigned long tin_ns; @@ -143,7 +143,7 @@ int pwm_init(int pwm_id, int div, int invert) { u32 val; const struct s5p_timer *pwm = - (struct s5p_timer *)samsung_get_base_timer(); + samsung_get_base_timer(); unsigned long ticks_per_period; unsigned int offset, prescaler;
diff --git a/src/cpu/samsung/exynos5-common/sromc.c b/src/cpu/samsung/exynos5-common/sromc.c index 091e8d1..7bc93e7 100644 --- a/src/cpu/samsung/exynos5-common/sromc.c +++ b/src/cpu/samsung/exynos5-common/sromc.c @@ -36,7 +36,7 @@ void s5p_config_sromc(u32 srom_bank, u32 srom_bw_conf, u32 srom_bc_conf) { u32 tmp; struct s5p_sromc *srom = - (struct s5p_sromc *)samsung_get_base_sromc(); + samsung_get_base_sromc();
/* Configure SMC_BW register to handle proper SROMC bank */ tmp = srom->bw; diff --git a/src/cpu/samsung/exynos5-common/timer.c b/src/cpu/samsung/exynos5-common/timer.c index 6436da2..6cd5f5d 100644 --- a/src/cpu/samsung/exynos5-common/timer.c +++ b/src/cpu/samsung/exynos5-common/timer.c @@ -39,7 +39,7 @@ static unsigned long lastinc; /* macro to read the 16 bit timer */ static inline struct s5p_timer *s5p_get_base_timer(void) { - return (struct s5p_timer *)samsung_get_base_timer(); + return samsung_get_base_timer(); }
/** diff --git a/src/cpu/samsung/exynos5-common/wdt.c b/src/cpu/samsung/exynos5-common/wdt.c index 6f435e5..dbeefec 100644 --- a/src/cpu/samsung/exynos5-common/wdt.c +++ b/src/cpu/samsung/exynos5-common/wdt.c @@ -30,7 +30,7 @@ void wdt_stop(void) { struct s5p_watchdog *wdt = - (struct s5p_watchdog *)samsung_get_base_watchdog(); + samsung_get_base_watchdog(); unsigned int wtcon;
wtcon = readl(&wdt->wtcon); @@ -42,7 +42,7 @@ void wdt_stop(void) void wdt_start(unsigned int timeout) { struct s5p_watchdog *wdt = - (struct s5p_watchdog *)samsung_get_base_watchdog(); + samsung_get_base_watchdog(); unsigned int wtcon;
wdt_stop(); diff --git a/src/cpu/samsung/exynos5250/clock.c b/src/cpu/samsung/exynos5250/clock.c index 115d40d..6b79272 100644 --- a/src/cpu/samsung/exynos5250/clock.c +++ b/src/cpu/samsung/exynos5250/clock.c @@ -180,7 +180,7 @@ static struct st_epll_con_val epll_div[] = { unsigned long get_pll_clk(int pllreg) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); unsigned long r, m, p, s, k = 0, mask, fout; unsigned int freq;
@@ -246,7 +246,7 @@ unsigned long get_pll_clk(int pllreg) unsigned long clock_get_periph_rate(enum periph_id peripheral) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); struct clk_bit_info *bit_info = &clk_bit_info[peripheral]; unsigned long sclk, sub_clk; unsigned int src, div, sub_div; @@ -341,7 +341,7 @@ unsigned long clock_get_periph_rate(enum periph_id peripheral) unsigned long get_arm_clk(void) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); unsigned long div; unsigned long armclk; unsigned int arm_ratio; @@ -378,7 +378,7 @@ struct arm_clk_ratios *get_arm_clk_ratios(void) void set_mmc_clk(int dev_index, unsigned int div) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); unsigned int addr; unsigned int val;
@@ -404,7 +404,7 @@ void set_mmc_clk(int dev_index, unsigned int div) void clock_ll_set_pre_ratio(enum periph_id periph_id, unsigned divisor) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); unsigned shift; unsigned mask = 0xff; u32 *reg; @@ -449,7 +449,7 @@ void clock_ll_set_pre_ratio(enum periph_id periph_id, unsigned divisor) void clock_ll_set_ratio(enum periph_id periph_id, unsigned divisor) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); unsigned shift; unsigned mask = 0xff; u32 *reg; @@ -574,7 +574,7 @@ int clock_set_rate(enum periph_id periph_id, unsigned int rate) int clock_set_mshci(enum periph_id peripheral) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); u32 *addr; unsigned int clock; unsigned int tmp; @@ -635,7 +635,7 @@ int clock_epll_set_rate(unsigned long rate) unsigned int lockcnt; unsigned int start; struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock();
epll_con = readl(&clk->epll_con0); epll_con &= ~((EPLL_CON0_LOCK_DET_EN_MASK << @@ -685,7 +685,7 @@ int clock_epll_set_rate(unsigned long rate) void clock_select_i2s_clk_source(void) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock();
clrsetbits_le32(&clk->src_peric1, AUDIO1_SEL_MASK, (CLK_SRC_SCLK_EPLL)); @@ -694,7 +694,7 @@ void clock_select_i2s_clk_source(void) int clock_set_i2s_clk_prescaler(unsigned int src_frq, unsigned int dst_frq) { struct exynos5_clock *clk = - (struct exynos5_clock *)samsung_get_base_clock(); + samsung_get_base_clock(); unsigned int div ;
if ((dst_frq == 0) || (src_frq == 0)) { diff --git a/src/cpu/samsung/exynos5250/cpu.h b/src/cpu/samsung/exynos5250/cpu.h index b859823..772e591 100644 --- a/src/cpu/samsung/exynos5250/cpu.h +++ b/src/cpu/samsung/exynos5250/cpu.h @@ -79,59 +79,38 @@ /* Distance between each Trust Zone PC register set */ #define TZPC_BASE_OFFSET 0x10000
-#ifndef __ASSEMBLER__ - -/* FIXME(dhendrix): cpu_is_exynos5() seems broken atm... */ -#if 0 -#define SAMSUNG_BASE(device, base) \ -static inline unsigned int samsung_get_base_##device(void) \ -{ \ - return cpu_is_exynos5() ? EXYNOS5_##base : 0; \ -} -#endif -#define SAMSUNG_BASE(device, base) \ -static inline unsigned int samsung_get_base_##device(void) \ -{ \ - return EXYNOS5_##base; \ -} - -SAMSUNG_BASE(adc, ADC_BASE) -SAMSUNG_BASE(clock, CLOCK_BASE) -SAMSUNG_BASE(ace_sfr, ACE_SFR_BASE) -SAMSUNG_BASE(dsim, MIPI_DSI1_BASE) -SAMSUNG_BASE(disp_ctrl, DISP1_CTRL_BASE) -SAMSUNG_BASE(fimd, FIMD_BASE) -SAMSUNG_BASE(gpio_part1, GPIO_PART1_BASE) -SAMSUNG_BASE(gpio_part2, GPIO_PART2_BASE) -SAMSUNG_BASE(gpio_part3, GPIO_PART3_BASE) -SAMSUNG_BASE(gpio_part4, GPIO_PART4_BASE) -SAMSUNG_BASE(gpio_part5, GPIO_PART5_BASE) -SAMSUNG_BASE(gpio_part6, GPIO_PART6_BASE) -SAMSUNG_BASE(pro_id, PRO_ID) - -#ifndef CONFIG_OF_CONTROL -SAMSUNG_BASE(mmc, MMC_BASE) -SAMSUNG_BASE(mshci, MSHC_BASE) -#endif - -SAMSUNG_BASE(modem, MODEM_BASE) -SAMSUNG_BASE(sromc, SROMC_BASE) -SAMSUNG_BASE(swreset, SWRESET) -SAMSUNG_BASE(sysreg, SYSREG_BASE) -SAMSUNG_BASE(timer, PWMTIMER_BASE) -SAMSUNG_BASE(uart, UART_BASE) -SAMSUNG_BASE(usb_phy, USBPHY_BASE) -SAMSUNG_BASE(usb_otg, USBOTG_BASE) -SAMSUNG_BASE(watchdog, WATCHDOG_BASE) -SAMSUNG_BASE(power, POWER_BASE) -SAMSUNG_BASE(i2s, I2S_BASE) -SAMSUNG_BASE(spi1, SPI1_BASE) -#ifndef CONFIG_OF_CONTROL -SAMSUNG_BASE(i2c, I2C_BASE) -SAMSUNG_BASE(spi, SPI_BASE) -SAMSUNG_BASE(spi_isp, SPI_ISP_BASE) -#endif -#endif +#define samsung_get_base_adc() ((struct exynos5_adc *)EXYNOS5_ADC_BASE) +#define samsung_get_base_clock() ((struct exynos5_clock *)EXYNOS5_CLOCK_BASE) +#define samsung_get_base_ace_sfr() ((struct exynos5_ace_sfr *)EXYNOS5_ACE_SFR_BASE) +#define samsung_get_base_dsim() ((struct exynos5_dsim *)EXYNOS5_MIPI_DSI1_BASE) +#define samsung_get_base_disp_ctrl() ((struct exynos5_disp_ctrl *)EXYNOS5_DISP1_CTRL_BASE) +#define samsung_get_base_fimd() ((struct exynos5_fimd *)EXYNOS5_FIMD_BASE) +#define samsung_get_base_gpio_part1() ((struct exynos5_gpio_part1 *)EXYNOS5_GPIO_PART1_BASE) +#define samsung_get_base_gpio_part2() ((struct exynos5_gpio_part2 *)EXYNOS5_GPIO_PART2_BASE) +#define samsung_get_base_gpio_part3() ((struct exynos5_gpio_part3 *)EXYNOS5_GPIO_PART3_BASE) +#define samsung_get_base_gpio_part4() ((struct exynos5_gpio_part4 *)EXYNOS5_GPIO_PART4_BASE) +#define samsung_get_base_gpio_part5() ((struct exynos5_gpio_part5 *)EXYNOS5_GPIO_PART5_BASE) +#define samsung_get_base_gpio_part6() ((struct exynos5_gpio_part6 *)EXYNOS5_GPIO_PART6_BASE) +#define samsung_get_base_pro_id() ((struct exynos5_pro_id *)EXYNOS5_PRO_ID) + +#define samsung_get_base_mmc() ((struct exynos5_mmc *)EXYNOS5_MMC_BASE) +#define samsung_get_base_mshci() ((struct exynos5_mshci *)EXYNOS5_MSHC_BASE) + +#define samsung_get_base_modem() ((struct exynos5_modem *)EXYNOS5_MODEM_BASE) +#define samsung_get_base_sromc() ((struct exynos5_sromc *)EXYNOS5_SROMC_BASE) +#define samsung_get_base_swreset() ((struct exynos5_swreset *)EXYNOS5_SWRESET) +#define samsung_get_base_sysreg() ((struct exynos5_sysreg *)EXYNOS5_SYSREG_BASE) +#define samsung_get_base_timer() ((struct s5p_timer *)EXYNOS5_PWMTIMER_BASE) +#define samsung_get_base_uart() ((struct exynos5_uart *)EXYNOS5_UART_BASE) +#define samsung_get_base_usb_phy() ((struct exynos5_usb_phy *)EXYNOS5_USBPHY_BASE) +#define samsung_get_base_usb_otg() ((struct exynos5_usb_otg *)EXYNOS5_USBOTG_BASE) +#define samsung_get_base_watchdog() ((struct exynos5_watchdog *)EXYNOS5_WATCHDOG_BASE) +#define samsung_get_base_power() ((struct exynos5_power *)EXYNOS5_POWER_BASE) +#define samsung_get_base_i2s() ((struct exynos5_i2s *)EXYNOS5_I2S_BASE) +#define samsung_get_base_spi1() ((struct exynos5_spi1 *)EXYNOS5_SPI1_BASE) +#define samsung_get_base_i2c() ((struct exynos5_i2c *)EXYNOS5_I2C_BASE) +#define samsung_get_base_spi() ((struct exynos5_spi *)EXYNOS5_SPI_BASE) +#define samsung_get_base_spi_isp() ((struct exynos5_spi_isp *)EXYNOS5_SPI_ISP_BASE)
#define EXYNOS5_SPI_NUM_CONTROLLERS 5 #define EXYNOS_I2C_MAX_CONTROLLERS 8 diff --git a/src/cpu/samsung/exynos5250/power.c b/src/cpu/samsung/exynos5250/power.c index 41e1e1b..953bf5c 100644 --- a/src/cpu/samsung/exynos5250/power.c +++ b/src/cpu/samsung/exynos5250/power.c @@ -38,7 +38,7 @@ static void ps_hold_setup(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
/* Set PS-Hold high */ setbits_le32(&power->ps_hold_ctrl, POWER_PS_HOLD_CONTROL_DATA_HIGH); @@ -47,7 +47,7 @@ static void ps_hold_setup(void) void power_reset(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
/* Clear inform1 so there's no change we think we've got a wake reset */ power->inform1 = 0; @@ -59,7 +59,7 @@ void power_reset(void) void power_shutdown(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
clrbits_le32(&power->ps_hold_ctrl, POWER_PS_HOLD_CONTROL_DATA_HIGH);
@@ -69,7 +69,7 @@ void power_shutdown(void) void power_enable_dp_phy(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
setbits_le32(&power->dptx_phy_control, DPTX_PHY_ENABLE); } @@ -77,9 +77,9 @@ void power_enable_dp_phy(void) void power_enable_usb_phy(void) { struct exynos5_sysreg *sysreg = - (struct exynos5_sysreg *)samsung_get_base_sysreg(); + samsung_get_base_sysreg(); struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power(); unsigned int phy_cfg;
/* Setting USB20PHY_CONFIG register to USB 2.0 HOST link */ @@ -98,7 +98,7 @@ void power_enable_usb_phy(void) void power_disable_usb_phy(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
/* Disabling USBHost_PHY */ clrbits_le32(&power->usb_host_phy_ctrl, POWER_USB_HOST_PHY_CTRL_EN); @@ -107,7 +107,7 @@ void power_disable_usb_phy(void) void power_enable_hw_thermal_trip(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
/* Enable HW thermal trip */ setbits_le32(&power->ps_hold_ctrl, POWER_ENABLE_HW_TRIP); @@ -116,7 +116,7 @@ void power_enable_hw_thermal_trip(void) uint32_t power_read_reset_status(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
return power->inform1; } @@ -124,7 +124,7 @@ uint32_t power_read_reset_status(void) void power_exit_wakeup(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power(); typedef void (*resume_func)(void);
((resume_func)power->inform0)(); @@ -193,7 +193,7 @@ int power_init(void) void power_enable_xclkout(void) { struct exynos5_power *power = - (struct exynos5_power *)samsung_get_base_power(); + samsung_get_base_power();
/* use xxti for xclk out */ clrsetbits_le32(&power->pmu_debug, PMU_DEBUG_CLKOUT_SEL_MASK,