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 4:
(3 comments)
Patch Set 2:
(1 comment)
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
+1
The latter is really optional, in the best case a no-op and in the worst a regression (e.g. if FSP misbehaves) and will probably get some more bikeshedding about where/when it should be done.
Unrelated, what's with the comment style? I'm generally not op- posed to //, but seeing it added to a file that consistently uses the other style makes me freak out a little.
Took care of all the comments, I hope.
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;
makes sense, but the comment and the code don't without that context. […]
Ack
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
Not public, in BIOS Writer's Guides mostly. But doesn't matter really, they […]
Ack
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;
ah, I wasn't looking carefully enough at the UPDs. My comment for the one chip_fsp20. […]
Ack