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.
Sowmya Aralguppe 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 30:
(7 comments)
File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/8e6ee19a_f470f608?usp... : PS28, Line 61: if (!num_entries) : return;
why are we skipping this check now ?
added back - basically this is array size and I thought it might not be of much value add .Instead i added num_entries print while searching for sku .But anyway i have reverted the change
https://review.coreboot.org/c/coreboot/+/83752/comment/c77d7a1c_328001d7?usp... : PS28, Line 81: 1000
ideally this is better to modify in a separate CL. […]
Acknowledged - but this was part of the review comment for this patchthat i addressed
https://review.coreboot.org/c/coreboot/+/83752/comment/383bf5b7_d1f3f9a5?usp... : PS28, Line 141: if (CONFIG(EC_GOOGLE_CHROMEEC) && google_chromeec_is_battery_present_and_above_critical_threshold()) {
was it more than > 96 ?
yes - please dont review variant_update_psys_power_limits - i will remove this module in a separate patch .Other variants were calling this function so had to keep this temporarily to avoid build errors
https://review.coreboot.org/c/coreboot/+/83752/comment/e58aba39_ab11403d?usp... : PS28, Line 45: printk(BIOS_ERR, "Cannot find brox sku index (mchid = %u) in %lu entries\n", : mchid, num_entries);
submit a separate CL for this
reverted the change
https://review.coreboot.org/c/coreboot/+/83752/comment/0950e416_3788ed5b?usp... : PS28, Line 56: if (config->tdp_pl4 == 0) {
nit […]
Done
https://review.coreboot.org/c/coreboot/+/83752/comment/e6a70f31_87beb8f4?usp... : PS28, Line 59: (CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT)
u don't need additional braces for this config check
Done
https://review.coreboot.org/c/coreboot/+/83752/comment/4e034e00_d6a25fb1?usp... : PS28, Line 79: policy_dev = DEV_PTR(dptf_policy); : if (!policy_dev) { : printk(BIOS_INFO, "DPTF policy not set\n"); : return; : }
why are we not keeping the previous code ? […]
I just put dptf policy check above pl4 override - thought that would be right thing to do .but i have reverted this as its causing more confusion by making patch set changes non readable