Nico Huber 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 8:
FSP MR2 does a check for ACPI mode and change the device mode. Supplying incorrect configuration will be corrected by FSP, but to be futher proof supply correct configuration to FSP.
I can't say that I understand this yet. You are saying FSP allows a value of 2 (PCH_ACPI_MODE). But then you say FSP would be correcting incorrect configuration, which translates to me as it reads the hardware state and ignores the diffe- rence between a 1 and a 2.
And about being future proof. That's generally a good idea, but how can we anticipate the future here? How can we tell that if FSP is being corrected, if the code will be made to match the header file or the other way around?
The goal is to supply correct mode settings to FSP. For this reason the modification is made. On the other hand FSP (MR2) will modify settings of the child devices if incorrect. What FSP is doing is not wrong, but for those without access to FSP sources, we should ensure that coreboot is configuring the correct mode.
Ah, there's that about child devices again. Sometimes you say it's corrected (see topmost quote here) and the change is only needed to be future proof, sometimes you say it's corrected for child devices (only?). That's confusing.
But let's back up a little. How does the device hierarchy look like here? I couldn't find anything about child devices and actually nothing about the DMA devices in general. Can you point to a datasheet that explains this a little?
I think the first thing to do in such a case (documentation / comments in header files don't match the code) is to file a bug report for FSP. Was that done yet? And in case we can't get the official documentation fixed, we have to document things on our own, e.g. by adding a reasonable enum to our code and comment that the documentation is wrong.