Attention is currently required from: Shreesh Chhabbi. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49766 )
Change subject: soc/intel/tgl: Enable/Disable S0ix substates ......................................................................
Patch Set 11:
(8 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49766/comment/09e55669_edf706d6 PS9, Line 7: Disable S0i3.2 & S0i3.3 substates : : S0i3.2 and S0i3.3 are applicable only if wake on voice is : disabled. As per Platform Design Guide, S0i3.2 and S0i3.3 : substates need to be disabled for Tigerlake. This needs update
https://review.coreboot.org/c/coreboot/+/49766/comment/4a4f0e1e_be107afc PS9, Line 12: Can you please check what is the lowest power state reported by coreboot and actually observed on volteer devices? I see you have commented that you need to update volteer and tglrvp devicetrees. It would be good to validate the entire series.
File src/soc/intel/tigerlake/chip.h:
https://review.coreboot.org/c/coreboot/+/49766/comment/78878b4e_dad500f2 PS9, Line 502: enabled Rather than saying "enabled", I think it should say "Mainboard design uses external clock gating" since this is very much dependent on the hardware design. Same for external phy gating and external bypass rails.
https://review.coreboot.org/c/coreboot/+/49766/comment/03df9929_a518f390 PS9, Line 506: ExternalClkGated nit: use external_clk_gated instead of ExternalClkGated. I know this file uses a mix of both styles, but the former is preferred. Also, the latter is mostly used for FSP UPDs. Same for configs below.
File src/soc/intel/tigerlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/49766/comment/2dae88e3_2af37d56 PS9, Line 60: and UP4 I don't think this is correct for UP4.
https://review.coreboot.org/c/coreboot/+/49766/comment/41a822b2_f9873292 PS9, Line 67: recommended It would be good to capture in commit message what "recommended" means i.e. it is possible to achieve this state, but is known to provide lower savings than shallower states.
https://review.coreboot.org/c/coreboot/+/49766/comment/f65d0c3f_f953247e PS9, Line 71: /* If external phy gating is not implemented, S0i3.3/S0i3.4/S0i2.2 are not recommended. */
line over 96 characters
Can you please split this into multi-line comment?
https://review.coreboot.org/c/coreboot/+/49766/comment/17db7814_fbff986c PS9, Line 76: if (is_dev_enabled(pcidev_path_on_root(PCH_DEVFN_CNVI_BT)) || is_dev_enabled(pcidev_path_on_root(PCH_DEVFN_CNVI_WIFI)) || is_dev_enabled(pcidev_path_on_root(PCH_DEVFN_ISH)))
line over 96 characters
split into multiple lines:
if (is_dev_enabled() || is_dev_enabled() || is_dev_enabled())
or you can add a helper:
if (is_cnvi_or_ish_enabled())
and implement is_cnvi_or_ish_enabled() such that:
bool is_cnvi_or_ish_enabled(void) { return is_dev_enabled() || is_dev_enabled() || is_dev_enabled(); }