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 3:
(1 comment)
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 […]
Right, I could do that. Should I then place that function on the common P2SB intelblock?
I'd move this elsewhere, but where? I know the P2SB device is working fine here. Plus, if I were to move this elsewhere, the HPET write seems to be done by FSP-M and the IOAPIC write is done by FSP-S... Not sure why these are done on different phases, but I don't think Intel would want to tell me the reasons to do so.
And since the B:D.F numbers aren't the same for all platforms (cannonlake uses different stuffs, for instance), I think the p2sb_configure_vtd_originators() function would then have to take the B:D.F values as arguments... Not sure if a function to do two specific register writes is really worth it.
But I do agree that the comments for this function deserve a different comment treatment. The comments explain what the code does, but do not mention why.