Attention is currently required from: Ashish Kumar Mishra, Caveh Jalali, Dinesh Gehlot, Eric Herrmann, Forest Mittelberg, Jayvik Desai, Kapil Porwal, Keith Short, Nick Vaccaro, Rishika Raj, Shelley Chen, Sowmya Aralguppe, Subrata Banik.
Karthik Ramasubramanian 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 11:
(9 comments)
File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/8230210a_3255b518?usp... : PS11, Line 15: #define SET_PSYSPL2(e, w) ((e) * (w) / 100) If not setting Psys please remove it.
https://review.coreboot.org/c/coreboot/+/83752/comment/9077036a_e1c32924?usp... : PS11, Line 16: SET_PL2 If not setting PL2, please remove it.
https://review.coreboot.org/c/coreboot/+/83752/comment/ddc91428_b5669365?usp... : PS11, Line 19: #define TENPOWERSIX 1000000 : #define TENPOWERTHREE 1000 : Instead of TENPOWERSIX and TENPOWERTHREE, please define MILLIVOLT_TO_VOLT and MILLIAMP_TO_AMP. That way it is easy to understand.
https://review.coreboot.org/c/coreboot/+/83752/comment/df80936f_6007a926?usp... : PS11, Line 82: MILLIWATTS_TO_WATTS); Indent it after the DIV_ROUND_UP open parenthesis.
https://review.coreboot.org/c/coreboot/+/83752/comment/360be0e4_0fe7ace0?usp... : PS11, Line 87: &volts_mv); Same comment as above.
https://review.coreboot.org/c/coreboot/+/83752/comment/0a320876_21f1d8ff?usp... : PS11, Line 98: soc_config->tdp_pl4 = SET_PL4(40, watts); Can you please add a table at the start of this function to provide an idea of what the PL4 will look like for different power adapter ratings?
https://review.coreboot.org/c/coreboot/+/83752/comment/557f7ed0_b4de5c8e?usp... : PS11, Line 102: watts); Same comment as above.
https://review.coreboot.org/c/coreboot/+/83752/comment/3b20ffc7_13e370f0?usp... : PS11, Line 146: void variant_update_psys_power_limits It seems not used anymore. If so, can we remove it. Also remove the function prototype in variants.h
File src/soc/intel/alderlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/23bfd33e_a085b684?usp... : PS9, Line 359: if (CONFIG(EC_GOOGLE_CHROMEEC) && (!google_chromeec_is_battery_present_and_above_critical_threshold())) {
for example how crashlog or platform debug consent is enabled in line 356 / 370 […]
How about moving inside mainboard_memory_init_params override in brox mainboard. That way it is mainboard specific. Also it will address batteryless scenario in chromebox form factor in other alderlake platforms.
Finally is MaxBootFrequency related to Trace Feature. As a casual reviewer, it does not look like so. But you might know the details more.