Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Subrata Banik, Michael Niewöhner, EricR Lai. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48340 )
Change subject: soc/intel/alderlake: Refactor PCIE port config ......................................................................
Patch Set 51:
(2 comments)
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/48340/comment/67f11e0d_dbe84d83 PS47, Line 34: die("Unsupported pcie_rp_type!"); If die() fixes the compiler warning, probably only because it has attribute((noreturn)) or something like that. It's just deferring the construct that turns the warning off. But that's not needed, how about a switch/case with default?
switch (type) { case PCH_PCIE_RP: return rp_number; case CPU_PCIE_RP: default: return 0x40 + rp_number; }
This way you could avoid an `else` path and still have `CPU_PCIE_RP` mentioned explicitly.
I only stumbled over this because people seem to be concerned about code sizes lately. The die() statement probably adds 40B of nothing.
Well, assert() is not a drop-in replacement for die(). I'm not sure what should the assertion be checking, though.
The assertion would be:
assert(type == PCH_PCIE_RP || type == CPU_PCIE_RP);
Assertions are mere documentation (unless you have static code analyzation check them), so doesn't change anything about the compiler warning. It is also redundant because we already specify type constraints. Alas, those are not enforced in C :-/
File src/soc/intel/common/block/include/intelblocks/pcie_rp.h:
https://review.coreboot.org/c/coreboot/+/48340/comment/9b3d82ae_4bc9ee99 PS47, Line 107: };
Because I don't want to touch get_l1_substate_control() and change it... […]
What I meant is that we have now the flags declaration for board config on top, then a bunch of unrelated function prototypes and then board configuration again. I'm also wondering why these things are mixed in a single header file (IIRC, I already commented elsewhere about that).