Nico Huber 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 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.
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); Looks like this would align better if you'd put it into something like
void p2sb_configure_vtd_originators(void);
called from here. It would also make the implementation available to other platforms (if they ever need it).
Also, as such configuration usually isn't done in the bootblock, we should at least comment, why. Maybe even move it to a more reasonable place before FSP hides the P2SB?