[coreboot] PCI register read/mod/write code

Peter Stuge peter at stuge.se
Mon Oct 4 01:10:14 CEST 2010

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?


More information about the coreboot mailing list