Attention is currently required from: Arthur Heymans, Christian Walter, Dinesh Gehlot, Elyes Haouas, Eran Mitrani, Felix Held, Jeff Daly, Johnny Lin, Kapil Porwal, Martin L Roth, Nico Huber, Subrata Banik, Tarun, Tim Chu, Vanessa Eusebio.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80193?usp=email )
Change subject: soc/intel: Use write{64,32,16,8}p and read{64,32,16,8}p ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Basically, I think when you're trying to call write32() with an integer address then in 99% of the […]
Are you arguing that we can't do byte-wise arithmetic with `void *`, or more generally that `void *` shouldn't point to something that's not allocated by the C runtime because of language minutiae in the standard? Either way, I don't think it's a good reason not to use it (and I believe we had the `void *` arithmetic argument before... in fact, we've now put in the coding style that it's fine: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/main/Docum...). The C standard was written 50 years ago and mainly targeted at userspace development, not systems. For firmware development we have to do some things that "violate" it anyway, and we are using other GCC-extensions (which clang also supports) anyway, so I don't think there's a point in clinging too strictly to the letter of the standard for this.
Adding a new type specifically to represent MMIO space means yet another coreboot-specific style detail for new contributors to learn, and I'm not convinced it really adds value to justify it. I think people are used to using `void *` to represent arbitrary addresses (and we also have many instances of MMIO space represented by non-void types, like all the struct overlays that are common on our Arm platforms (e.g. https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/main/src/s...). And we have `void *` arithmetic in other cases as well anyway, that's not specific to just MMIO.
(I think the main concern about lack of type safety is that people confuse the order of the address and value arguments, especially since we used to have `writel(val, addr)` functions in the past that had it exactly the other way around, and some other firmware projects still do. Using `void *` at least solves that problem. I think accidentally using some other random pointer is not a common mistake.)