Attention is currently required from: Alper Nebi Yasak, Arthur Heymans, Jianjun Wang, Julius Werner, Ron Minnich, Shelley Chen, Yu-Ping Wu.
Nico Huber has posted comments on this change by Alper Nebi Yasak. ( https://review.coreboot.org/c/coreboot/+/80372?usp=email )
Change subject: arch/io.h: Add port I/O functions to other architectures ......................................................................
Patch Set 5:
(1 comment)
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/80372/comment/fa3e2e7d_0b91f337?usp... : PS5, Line 532: CPU communicate with peripheral devices over PCI I/O space.
Okay, sorry, I guess I meant `struct resource`. I was thinking of stuff like what `src/drivers/emulation/qemu/bochs.c` seems to do, where it gets a `struct resource` from the platform to represent its base address and whenever it wants to write to some register it uses a little helper that checks whether `res->flags & IORESOURCE_IO`, and uses that to decide whether to use I/O or MMIO accessors.
Oh, I should have looked closer. That's not a good role model. This is uncommon. When you have a PCI device, usually it's either IO or MMIO. And you know that beforehand, because it's always the same for this device model. This bochs driver seems to abstract because it supports different device models and different (emulated) hardware revisions. ``` /* MMIO bar supported since qemu 3.0+ */ ``` So coreboot is either confronted with this 3.0+ hardware or not. If the driver was written for a single device model/revision, we wouldn't see such abstraction.
With that said, I'm not sure if we have a strong use case here for PIO. Unless somebody uses a sadly old QEMU, they could emulate something where the driver would use MMIO, and stubbing out PIO would work. For QEMU's firmware-config interface, we could just pretend it's MMIO (assuming the translation offset is always the same and we can do the calculation by hand).
I was kinda assuming that that's how that whole coreboot device framework (which I don't really have any experience with because the non-PCI Arm drivers I usually worked on don't use it) works... that you have your `devicetree.cb` thing which says the platform has an instance of device X which is mapped at MMIO address Y (or I/O address Y), and then that somehow gets translated into a `struct resource` and passed to the X driver which can then use the address and flags in that struct to figure out how to talk to it. And if you're on an x86 device you can write your `devicetree.cb` such that it represents an I/O address, and if you're on an Arm device you write it such that it represents an MMIO address (and I guess you need to do the math to see where in your SoC's PCI MMIO window that thing falls by hand then when writing `devicetree.cb`, or maybe build some magic into sconfig that allows you to define the different PCI roots and have it do that math for you).
Is that not how it works?
No. It could be done that way, but isn't. Two major points: 1. PCI was designed for systems where everything was pluggable. Consequently, our static devicetree might not even mention the devices. This requires dynamic resource allocation. And because we have that implemented anyway, we also use it for devices soldered on-board. At least some address calculations would have to be done at runtime. 2. At least for PCI, `struct resource` describes the physical address that the device decodes. Because of all the identity mapping on x86, that's conveniently the same address that the CPU sees. So we don't have any mechanism for address translation.
Having to translate PIO-MMIO isn't new. However, this used to be a singleton (where this `arch/io.h` implementation with a single Kconfig offset would work). If we encounter or want to prepare for hardware where we can't use a single offset, we need to design something new. For instance, our IORESOURCE_BRIDGE is just a flag, w/o any concept of offsets or different address spaces on both sides of the bridge. If we'd add the missing information there, implementing a generic `read_res(struct resource *, offset)` / `write_res(...)` would be straight forward. I still doubt, though, if we'll ever need it.
-
Nico Huber (Code Review)