Rudolf just found a bug in the sb700 code:
u32 dword;
..
dword = pci_read_config8(dev, 0x64);
dword |= 1 << 10;
pci_write_config8(dev, 0x64, dword);
And I'm ranting now, because a pci_set8() macro/function could have
found this bug at compile time, and because I don't like these
constructs. (Compiler warnings would also have indicated a problem.
They're currently disabled for this code.)
Carl-Daniel is working on a patch, meanwhile consider this:
#define pci_set8(dev, reg, val) do { \
if ((a) & ~0xff) can_only_set_low_8_bits(); \
pci_set8_nocheck((dev), (reg), (val)); \
} while(0)
u8 pci_set8_nocheck(device_t dev, u32 reg, u8 val) {
u8 tmp = pci_read_config8(dev, reg);
tmp |= val;
pci_write_config8(dev, reg, tmp);
return tmp;
}
$ gcc -g -o a a.c
/tmp/cchpQWSr.o: In function `main':
/tmp/a.c:11: undefined reference to `can_only_set_low_8_bits'
collect2: ld returned 1 exit status
Now, this is ugly, and warnings is the only right way, but I still
very much think that one pci_set8() or pci_clear8() call is way
better than code to read+mod+write. The reason is that it's how I
think about these operations; "set bits xy in reg foo."
Read+mod+write is one lower level of abstraction, and not relevant
in the context of setting the bits. Ie. I don't want to write/see
*how* bits get set, I just want to write/see *that* bits get set.
I completely understand that everyone else may not have the same
mental model of these operations, but I hope many enough do..
(Yes, we've discussed before and it seemed not to be the case.)
Carl-Daniel mentioned that there may be a risk for confusion between
pci_set8() and pci_write_config8(), and this was also noted before.
Isn't it really reasonable to expect that people can actually keep
track of how those two functions are different?
I understand that my taste is more terse than most, and if consensus
is that _set8 _clear8 etc. names suck, then I'd be happy with other
fun names too, as long as read+mod+write blocks can be replaced with
single lines of code.
Like the CAR thing I think this really helps write- and readability,
and even thinkability, for our code. The latter helps make further
abstraction easier, which allows more refactoring. Bad goal?
//Peter
--
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
