Attention is currently required from: Ashish Kumar Mishra, Caveh Jalali, Dinesh Gehlot, Eric Herrmann, Forest Mittelberg, Jayvik Desai, Karthik Ramasubramanian, Keith Short, Krishna P Bhat D, Nick Vaccaro, Paul Menzel, Rishika Raj, Ronak Kanabar, Shelley Chen, Sowmya Aralguppe, Sowmya Aralguppe.
Subrata Banik has posted comments on this change by Sowmya Aralguppe. ( https://review.coreboot.org/c/coreboot/+/83752?usp=email )
Change subject: mb/google/brox: Fix booting to kernel without battery ......................................................................
Patch Set 27:
(4 comments)
File src/mainboard/google/brox/Kconfig:
https://review.coreboot.org/c/coreboot/+/83752/comment/922aeb94_db8d3d17?usp... : PS27, Line 124: Select this if the variant has to boot for low battery, battery at critical threshold : or when battery is disconnected physically. PL4 which is processor Instataneous Power : or Absolute Peak Power controls the highest power draw from processor at any given : instant of time within Pl1Tau duration. Hence PL4 acts as a fail safe mechanism to set an : upper threshold limit for processor instantaneous power draw. For a 30W adapter, the : maximum peak power is set at 9 watts which is 30W multiplied by 32 percent efficiency. : This default pl4 value is set for 30W or any higher adapter rating such as 45W or 65W. Select this if the variant has to boot even with low battery, critical battery threshold, or when the battery is physically disconnected. PL4, which stands for Processor Instantaneous Power or Absolute Peak Power, controls the highest power draw from the processor at any given moment within the Pl1Tau duration. Therefore, PL4 acts as a failsafe mechanism to set an upper threshold limit for the processor's instantaneous power draw. For a 30W adapter, the maximum peak power is set at 9 watts, which is 30W multiplied by 32% efficiency. This default pl4 value is set for 30W or any higher adapter rating, such as 45W or 65W.
File src/mainboard/google/brox/romstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/e47e2212_52505f77?usp... : PS27, Line 16: FSP_M_CONFIG *mem_cfg = &memupd->FspmConfig; you can move this line inside `if` case in line 27
File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/a6b638c3_e2ac8d30?usp... : PS27, Line 74: else : soc_config->tdp_pl4 = CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT; please use braces as we have multi-line `if` case as well.
https://review.coreboot.org/c/coreboot/+/83752/comment/fd780543_3014e55f?usp... : PS27, Line 70: if (google_chromeec_is_battery_present_and_above_critical_threshold()) { : if (soc_config->tdp_pl4 == 0) : soc_config->tdp_pl4 = DIV_ROUND_UP(limits[brox_idx].pl4_power, : MILLIWATTS_TO_WATTS); : } else : soc_config->tdp_pl4 = CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT; : can we move this into a local function like `variant_pl4_override(soc_config)`
``` static void variant_pl4_override(const struct soc_power_limits_config *config) { if (google_chromeec_is_battery_present_and_above_critical_threshold()) { if (config->tdp_pl4 == 0) config->tdp_pl4 = DIV_ROUND_UP(limits[brox_idx].pl4_power, MILLIWATTS_TO_WATTS); } else { if (CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT) config->tdp_pl4 = CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT; } } ```