Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31752 )
Change subject: device/pci: Rewrite PCI MMCONF with symbol reference ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/31752/4/src/include/device/pci_mmio_cfg.h File src/include/device/pci_mmio_cfg.h:
https://review.coreboot.org/#/c/31752/4/src/include/device/pci_mmio_cfg.h@29 PS4, Line 29: };
Doesn't the same apply to `arch/mmio.h`?
Just wondering if that's not the better place. And we could use readxx()/writexx() below.
I guess I would be looking at ton of different callers of readX()/writeX() should I change those signatures away from void *.
In any case, it doesn't seem very obvious why we have that union. So I'd expect a comment here with the same reasoning as in the commit message.
Far from obvious to me as well, I'll replicate commit message in code.
Wait... do we need the union? I would assume now, that a struct, e.g. `struct pci8 { uint8_t reg[4096]; };` would produce the same result. I wouldn't have a preference, though.
You would cast thru u16* or u32*, respectively.
https://review.coreboot.org/#/c/31752/4/src/include/device/pci_mmio_cfg.h@36 PS4, Line 36: volatile union pci_bank *cfg = (void *)&pci_mmconf[dev]; Adding u8* pci_mmconf is very much related. I don't know if I can split it.
At least I don't want to split it, as the current form seems to create picture-perfect assembly code on x86.