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>
Reviewed-on: http://review.coreboot.org/2582
Tested-by: build bot (Jenkins)

4 files changed, 9 insertions, 9 deletions

diff --git a/src/cpu/samsung/exynos5-common/pwm.c b/src/cpu/samsung/exynos5-common/pwm.c
index da465d93c7..0fc571b220 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 091e8d18ab..7bc93e7862 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 6436da2770..6cd5f5dd35 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 6f435e5857..dbeefec49a 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();