Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 2:
(3 comments)
Patch Set 2:
Looks good, but it's actually two unrelated changes:
- what the summary says
- switching from FSP configuration to direct register writes
I could split this into two patches, one that just hardcodes the DRHD values, and another that "eases" FSP's job. Should I reuse this change ID for DRHD hardcoding or register writes?
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... PS2, Line 487: // We set that in coreboot already : params->PchIoApicBdfValid = 0;
if we set it in coreboot already, then why are we setting it to zero here for FSP?
The FSP has four UPDs for this: The trio for Bus:Dev.Fun values, and a fourth "Valid" UPD. The FSP code checks this "Valid" flag and, if true, will then write the Bus:Dev.Fun UPD values to the P2SB register.
Since I am now writing the values with coreboot code, I don't need FSP to write them again. So I just tell FSP that the Bus:Dev.Fun UPD values are not valid, and FSP will not program them.
And if FSP misbehaves and programs them anyway, then things will break and I will blame the FSP for it >:)
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... PS2, Line 65: Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET
source?
IBDF values come from chip_fsp20.c params->PchIoApicBusNumber = 250; params->PchIoApicDeviceNumber = 31; params->PchIoApicFunctionNumber = 0;
HBDF values come from romstage_fsp20.c m_cfg->PchHpetBusNumber = 250; m_cfg->PchHpetDeviceNumber = 15; m_cfg->PchHpetFunctionNumber = 0;
To be exact, the same places in which you left your other two comments :-)
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... PS2, Line 291: // We set that in coreboot already : m_cfg->PchHpetBdfValid = 0;
same as before. […]
FSP-M writes the Bus:Dev.Fun for the HPET (done here), whereas FSP-S writes the Bus:Dev.Fun for the IOAPIC (done in chip_fsp20.c). It's not doing it twice, it's just very similar.