On Sat, Oct 02, 2010 at 12:42:51PM -0700, ron minnich wrote:
On Sat, Oct 2, 2010 at 12:27 PM, Warren Turkal wt@penguintechs.org wrote:
I think that making arguments against this code on grounds that it doesn't feel like other common code is a little shortsighted. When bugs are fixed in one bit of code, we should be able to take advantage of those in other parts of code. For instance, I just had to copy/paste some CPP logic to fix the via/vt8454c CAR init due to a tiny bootblock. That would have already been fixed had the via and amd logic been unified since they are basically identical bits of code.
well, you have to be careful. We've had more than one problem with "common" code that was not so common. CAR is so tricky, and one platform changes may break another platform in ways we don't expect. CAR, in fact, scares me.
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.
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".
Of course I'm exagerrating a bit here, but I'm very convinced that readability is of utmost importance in coreboot code, especially so in CAR code.
about the idea of unifying via and amd code.
Yeah, that should be considered very carefully indeed, and only be done after sufficient testing on hardware (if it's feasible and makes sense to do it al all, i.e., if the two implementations are >= 90% similar).
But the general idea of unifying similar code (especially if it stems from copy-pasted code from elsewhere with only minimal customizations) is very good and very necessary indeed. Yeah, things may break, that's life. Someone will test it, we'll fix it, problem solved. No reason to _not_ unify code which can be reasonably unified.
I'm actually planning to unify model_6ex, model_6fx, and model_106cx CAR implementations myself, but that can be done relatively easily, as the diff between all three files is just 2-3 simple lines.
Uwe.