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)
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);
Right, I could do that. Should I then place that function on the common P2SB intelblock?
Sounds good.
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.
I assume, it doesn't matter when it is done. The FSP source is probably just not organized around the P2SB device. HPET is a CPU thing, IOAPIC a PCH thing. For instance, we could also decide to put the HPET BDF writing in p2sb_configure_hpet()? Just an example, please ignore.
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.
The soc/intel/common/ code uses #includes as abstraction mechanism. It would just have to #include <soc/systemagent.h> and would get the correct numbers for the SoC it is built for.
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.
Yeah, your comments are actually just rephrasing the code (the worst kind of comments, beside lies, imho). What is really needed is the information that it's done early because FSP hides the device. Either in a comment or at least the commit message.