Attention is currently required from: Shelley Chen, Felix Singer, Furquan Shaikh, Martin Roth, Tim Wawrzynczak, Paul Menzel, Julius Werner, Angel Pons, Kyösti Mälkki. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57861 )
Change subject: Documentation/RFC: Generalize PCI support in coreboot ......................................................................
Patch Set 9:
(4 comments)
Patchset:
PS9:
Hi Nico, I was wondering if you had any open issues with this design doc in it's current state? I b […]
Hi, sorry, didn't mean to stall this. I prefer to review/discuss things step-by-step which scales much better. This doc requires one to review everything untested, without visible context of the source code and at once which is a lot additional work (which I lack the time for and tbh. don't understand why we do it this way). I tried before and didn't spot an issue in the very first proposed change. Which, OTOH, was very obvious to me when I looked at the follow-up patch.
File Documentation/RFC/pci_config_access.md:
https://review.coreboot.org/c/coreboot/+/57861/comment/15dc3c57_ccd647ae PS9, Line 41: #if : CONFIG(MMCONFIG_SUPPORT) I think this should be done later when the necessary Kconfig option is introduced (in step 3.). Adding inconsistent conditions in one step just to fix them up later won't work well during review.
https://review.coreboot.org/c/coreboot/+/57861/comment/7cd92027_26a70f8d PS9, Line 54: 3. Add new Kconfigs to distinguish between ECAM and non-ECAM access mechanisms. If this happens after renaming another Kconfig to `PCI_ECAM` wouldn't there be two Kconfigs that distinguish between ECAM and non-ECAM?
I think it would be better if we reduce the need for Kconfig. For instance, we could move the current MMCONF_SUPPORT if into x86 scope, because it's really only there where we want to include `pci_mmio_cfg.h` and need a switch to disable it. I'll just try that...
https://review.coreboot.org/c/coreboot/+/57861/comment/04768188_ff394b34 PS9, Line 92: supported and it currently only covers ECAM. I'm still not convinced that anything is wrong with `MMCONF_SUPPORT`.
You mentioned that people were confused by it in CB:57333. If the motivation is to avoid confusion that should be mentioned. But first we need to understand why it was confusing. Did people mix it up with MMIO in general?
Why I was against changing the name from the very beginning is that it will likely _create_ confusion. We've been used to grep for MMCONF for more than a decade (maybe more than two, idk). If that term sud- denly vanishes, I'd be very confused.