On Sat, Oct 02, 2010 at 10:21:38PM +0200, Uwe Hermann wrote:
Yes, it scares me too (well, sort of :) It's one of the most complex and non-obvious pieces of code we have in coreboot. Which makes it even more important to make this code as clean and easily readable and understandable as we can. Every little bit helps here, e.g. not open-coding various asm snippets (in 3 or more slightly different variants) all over the place is one very good measure to make it less confusing for people trying to read the code. Less lines in cache_as_ram.inc is good. Readable macros such as "enable_l2_cache" instead of some open-coded, harder to understand assembler instructions is good.
I think the three lines of assembler is easier to understand than "enable_l2_cache". Assembler isn't C - the macros defined aren't free abstractions. (In particular, it's not clear they clobber %eax.)
Again, just my 2ct.
High-level "look, here we enable cache"-style code is much better than "look, here we set bit 2 in CR0, and clear bits 5-6 in some MSR and write magic value 456 to magic address 123, now please go find the right datasheet and look up what it actually does".
I think the bit definitions, msr addresses, port numbers, and special addresses should use definitions. For an example of this from seabios, see:
http://git.linuxtogo.org/?p=kevin/seabios.git;a=blob;f=src/romlayout.S;h=a46...
-Kevin