Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44355 )
Change subject: soc/intel/tigerlake: Allow fine grained control of S0iX states ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44355/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44355/2//COMMIT_MSG@12 PS2, Line 12:
BUG=?
Done
https://review.coreboot.org/c/coreboot/+/44355/2/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/44355/2/src/soc/intel/tigerlake/chi... PS2, Line 94: uint8_t
enum lpm_state_mask
Done
https://review.coreboot.org/c/coreboot/+/44355/1/src/soc/intel/tigerlake/chi... File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/44355/1/src/soc/intel/tigerlake/chi... PS1, Line 82:
If you are open to introducing a new element "LpmStateEnable", then its default value of '0' (when n […]
I see. I have considered making a special case for the value 0, (as it would not make sense to disable every single substate, while keeping s0ix_enable). However, there is a theoretical problem, as I am also introducing code that will programmatically override bit 7 of the LpsStateEnableMask before passing it to the FSP. In the extremelty unlikely case that a board wants to disable every substate except S03.4, and hence sets LpmStateEnableMask to "0x80" in devicetree, and then the cr50 firmware is too old, causing my logic to clear the bit7, in order to disable S03.4 until the cr50 firmware has been upgraded, then it instead has the unintended effect of enabling every other substate.
I will reverse the meaning of this field, calling it LpmStateDisableMask. That way, the default of 0 will naturally result in new boards having every substate enabled, unless they request otherwise, without any special handling or surprises.
https://review.coreboot.org/c/coreboot/+/44355/2/src/soc/intel/tigerlake/fsp... File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/44355/2/src/soc/intel/tigerlake/fsp... PS2, Line 212: params->LpmStateEnableMask = config->LpmStateEnableMask;
Just a note: This change should land with the mainboard change as a single unit. […]
I see. I have decided to negate the meaning of this field, such that by default, every substate will be enabled (as was the case before this CL.)