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=a4695963cd4b10d1369273aef36562eb7f00dd65;hb=94dc9c49c283cd576c25692d17567035557a2505#l231 -Kevin -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
