Go to file
Ronald G. Minnich 026bbda071 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)
2013-03-04 19:43:19 +01:00
3rdparty@dac1a18d18 Update 3rdparty mark to latest repository 2013-02-22 09:22:41 +01:00
documentation sconfig: rename lapic_cluster -> cpu_cluster 2013-02-14 07:07:20 +01:00
payloads bump SeaBIOS to 1.7.2.1 2013-03-04 11:00:17 +01:00
src ARM: remove code that is IMHO a dangerous design 2013-03-04 19:43:19 +01:00
util GPLv2 notice: Unify all files to just use one space in »MA 02110-1301« 2013-03-01 10:16:08 +01:00
.gitignore add a few entries to .gitignore 2013-01-10 22:51:20 +01:00
.gitmodules Add 3rdparty as submodule 2012-05-01 00:08:37 +02:00
.gitreview add .gitreview 2012-11-01 23:13:39 +01:00
COPYING update license template. 2006-08-12 22:03:36 +00:00
Makefile build system: Retire REQUIRES_BLOB 2013-02-19 11:00:41 +01:00
Makefile.inc build system: Retire REQUIRES_BLOB 2013-02-19 11:00:41 +01:00
README Update README with newer version of the text from the web page 2011-06-15 10:16:33 +02:00

README

-------------------------------------------------------------------------------
coreboot README
-------------------------------------------------------------------------------

coreboot is a Free Software project aimed at replacing the proprietary BIOS
(firmware) found in most computers.  coreboot performs a little bit of
hardware initialization and then executes additional boot logic, called a
payload.

With the separation of hardware initialization and later boot logic,
coreboot can scale from specialized applications that run directly
firmware, run operating systems in flash, load custom
bootloaders, or implement firmware standards, like PC BIOS services or
UEFI. This allows for systems to only include the features necessary
in the target application, reducing the amount of code and flash space
required.

coreboot was formerly known as LinuxBIOS.


Payloads
--------

After the basic initialization of the hardware has been performed, any
desired "payload" can be started by coreboot.

See http://www.coreboot.org/Payloads for a list of supported payloads.


Supported Hardware
------------------

coreboot supports a wide range of chipsets, devices, and mainboards.

For details please consult:

 * http://www.coreboot.org/Supported_Motherboards
 * http://www.coreboot.org/Supported_Chipsets_and_Devices


Build Requirements
------------------

 * gcc / g++
 * make

Optional:

 * doxygen (for generating/viewing documentation)
 * iasl (for targets with ACPI support)
 * gdb (for better debugging facilities on some targets)
 * ncurses (for 'make menuconfig')
 * flex and bison (for regenerating parsers)


Building coreboot
-----------------

Please consult http://www.coreboot.org/Build_HOWTO for details.


Testing coreboot Without Modifying Your Hardware
------------------------------------------------

If you want to test coreboot without any risks before you really decide
to use it on your hardware, you can use the QEMU system emulator to run
coreboot virtually in QEMU.

Please see http://www.coreboot.org/QEMU for details.


Website and Mailing List
------------------------

Further details on the project, a FAQ, many HOWTOs, news, development
guidelines and more can be found on the coreboot website:

  http://www.coreboot.org

You can contact us directly on the coreboot mailing list:

  http://www.coreboot.org/Mailinglist


Copyright and License
---------------------

The copyright on coreboot is owned by quite a large number of individual
developers and companies. Please check the individual source files for details.

coreboot is licensed under the terms of the GNU General Public License (GPL).
Some files are licensed under the "GPL (version 2, or any later version)",
and some files are licensed under the "GPL, version 2". For some parts, which
were derived from other projects, other (GPL-compatible) licenses may apply.
Please check the individual source files for details.

This makes the resulting coreboot images licensed under the GPL, version 2.