Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31534 )
Change subject: soc/intel/cannonlake: Set FSP-S Enable8254ClockGating using clock_gate_8254 devicetree parameter ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31534/2/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31534/2/src/soc/intel/cannonlake/fsp_params.... PS2, Line 132: params->Enable8254ClockGating = config->clock_gate_8254; : params->Enable8254ClockGatingOnS3 = config->clock_gate_8254;
Default value for both Enable8254ClockGating and Enable8254ClockGatingOnS3 is 1. […]
Phew, I guess this means the surrounding code in coreboot (e.g. itss_clock_gate8254()) is dead and was never tested? I don't under- stand why we keep adding or copying dead code (that is redundant to FSP).
Jeremy, I think the best thing to do is to drop `clock_gate_8254` and the code around it. And introduce a Kconfig option that defaults to enabling the 8254 if the payload is SeaBIOS (IIRC, this was why you wanted to enable it?).
Whether or not this clock should be gated isn't a hardware design decision anyway, so why have such a setting the the devicetree in the first place? We could also remove the devicetree option for other platforms (that don't use it anyway) and give the Kconfig a nice default for each platform that reflects the current state?