Michael Niewöhner 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 6:
(1 comment)
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.
The idea of this change is to make the black box (FSP) weigh less (do less things)
Full ACK! It's a small step in the right direction :-)
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);
After thinking about it for a while, I realized I am unconditionally setting these values when they […]
FSP hides P2SB but recently a CB got merged unhiding it again after FSP-S, so this is not true anymore