Nico Huber 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:
(3 comments)
https://review.coreboot.org/#/c/31752/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31752/4//COMMIT_MSG@19 PS4, Line 19: value from a 'struct device *'. Trying to read this without the OOB communication of the past days in mind: We should somehow mention that this applies to multiple writes to the same device.
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.
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.
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.
https://review.coreboot.org/#/c/31752/4/src/include/device/pci_mmio_cfg.h@31 PS4, Line 31: extern u8 *const pci_mmconf; Adding this variable seems unrelated now (not covered by the commit message any more). Maybe make it two changes?