Attention is currently required from: Michael Niewöhner. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52627 )
Change subject: soc/intel/skylake: Clean up root port structs ......................................................................
Patch Set 3:
(1 comment)
File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/52627/comment/ad648ba3_668d66d6 PS2, Line 35: static const struct pcie_rp_group pch_rp_groups[] = { : #if CONFIG(SKYLAKE_SOC_PCH_H) : { .slot = PCH_DEV_SLOT_PCIE, .count = 8 }, : { .slot = PCH_DEV_SLOT_PCIE_1, .count = 8 }, : /* Sunrise Point PCH-H actually only has 4 ports in the : third group. But that would require a runtime check : and probing 4 non-existent ports shouldn't hurt. */ : { .slot = PCH_DEV_SLOT_PCIE_2, .count = 8 }, : #else : { .slot = PCH_DEV_SLOT_PCIE, .count = 8 }, : { .slot = PCH_DEV_SLOT_PCIE_1, .count = 4 }, : #endif :
why confusing? the third controller is only there on PCH-H
I am not talking about the third controller (the count is untouched there) but the second. In your example, you moved it out of the if-else-condition. Sure it wouldn't hurt probing just 4 ports more, but it makes me feel we are misusing it. So we would have to add a comment there too (in addition to the existing one), because we want to save two lines of code. Workarounds are confusing. Then why even care about the root port count?
However, as Patrick mentioned, the compiler should optimize it anyway. So maybe the code is just fine as it is.