Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35170 )
Change subject: soc/skylake: Write the P2SB IBDF and HBDF registers in coreboot ......................................................................
Patch Set 5:
(1 comment)
Patch Set 3:
(1 comment)
I don't see yet that this improves anything and it obviously doesn't fix anything either. I don't care enough, though, if somebody merges it as is.
This is just configuration of the P2SB for later use by the OS, nothing that coreboot depends on. Hence, it would preferably be done somewhere in the P2SB's device driver (I haven't found it yet, `soc/intel/common/block/p2sb/p2sb.c` doesn't seem to mention SKL) before FSP hides the device. That much of the code in soc/intel/ doesn't follow basic coreboot structures is no excuse to make it even messier. I would also prefer that such things are unified across all the newer platforms. That we have different directories and code structures for them seems more like an accident.
I am afraid of touching newer platforms as they can't be tested as easily. The idea of this change is to make the black box (FSP) weigh less (do less things) so that it can be carried away more easily where it belongs.
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/35170/3/src/soc/intel/skylake/bootb... PS3, Line 59: pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF);
Ack, will do. […]
After thinking about it for a while, I realized I am unconditionally setting these values when they only make sense if VT-d is supported and enabled, so I moved this to romstage vtd init. It also makes more sense because #include <soc/systemagent.h> is already there.