Matt DeVillier 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)
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?
based on the commit subject, the former IMO
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;
The FSP has four UPDs for this: The trio for Bus:Dev.Fun values, and a fourth "Valid" UPD. […]
makes sense, but the comment and the code don't without that context. I'd argue a comment such as: "// APIC Bus:Dev.Fun set in coreboot, so ignore UPDs" would be more useful to anyone reading the code outside of this review :)
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
IBDF values come from chip_fsp20.c […]
right, but they appear to be common across multiple Intel SoC's, so I assume it's documented somewhere publicly?
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;
FSP-M writes the Bus:Dev.Fun for the HPET (done here), whereas FSP-S writes the Bus:Dev. […]
ah, I wasn't looking carefully enough at the UPDs. My comment for the one chip_fsp20.c applies here as well, since it's unclear what coreboot sets or how setting the UPD to zero relates