Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29284 )
Change subject: soc/intel/braswell/chip.c: Configure LPSS devices in correct mode ......................................................................
Patch Set 9:
Patch Set 9:
If LPSS (SIO DMA1) pci device 0/30/0 is configured in ACPI mode, PWM, HS2UARTS, and SPI controller needs to be configured in ACPI mode. If LPSS (SIO DMA2) pci device 0/24/0 is configured in ACPI mode, I2C controller needs to be configured in ACPI mode. In other words: If function > 1 in PCI mode, function 0 must be PCI mode to be PCI-enumerated. (See 33.1 of Intel Braswell BIOS Writes Guide) (See 24.3 of Intel Braswell External Design Guide Volume 2)
This is NOT a bug in FSP. I expect it's responsibility of coreboot/developer to provide correct parameters API of FSP.
IMO it is still a BUG in FSP docs. The FSP docs do not document ACPI mode for DMAs and the dependency for child devices following the parent device setting. How a developer can integrate the binary correctly when the documentation and API have bugs? Not everyone has access to FSP source code, let's keep that in mind.
I (now) realize that not everyone has access to all Intel document/FSP source code.
On the other hand providing a patch and having code-review score of -1, I don't feel much support for supplying for this kit of fixes/update to coreboot.
A -1 merely states that somebody doesn't like the current state of a patch, not the patch itself (it should automa- tically vanish once you push a new version). Your help here is much appreciated.
When you write some code that conflicts an API documentation, that's ok when you know better. But most developers (no matter if they have access to FSP sources) will probably trust the API documentation and think that your code is wrong. So please add a code comment in such cases (or try to get the documentation fixed).
But before you continue, I would like to know in detail how you tested this. The interaction of coreboot and FSP is most fragile, especially when coreboot and FSP both offer options to configure the same thing (ACPI mode in this case). I fear, in case FSP hides the PCI device too early, dev_enable_acpi_mode() in core- boot might not work correctly anymore (e.g. fail to report the correct BARs).
Thanks for clarification. (My assumption was that having a -1 prevent other reviewers from reviewing)
This 'issue' is discovered during bringup on the Intel CherryHill (CRB). It has not influence on actuel Braswell platforms where the LPSS is configured in ACPI mode. This patch will be 'used' when board configuration is incorrect only.
We noticed that PCI devices were not visible on the PCI bus, since 'lpss_mode' was set to ACPI. Can not describe details of testing, since PCI/ACPI modes has been corrected in devicetree.cb. If all devicetree.cb setting are correct this fix would not influence behavoir of actual code. So there should not be any fear about other influences. If this influence exist, it exist in actual code already.