[M] Change in coreboot[main]: mb/google/brox: Fix booting to kernel without battery

[Subrata Banik (Code Review)]

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 28:

(10 comments)

File src/mainboard/google/brox/Kconfig:

https://review.coreboot.org/c/coreboot/+/83752/comment/da243306_5370a35d?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 th […]

Acknowledged

File src/mainboard/google/brox/romstage.c:

https://review.coreboot.org/c/coreboot/+/83752/comment/1685f8fa_bb46e342?usp... :
PS27, Line 16: 	FSP_M_CONFIG *mem_cfg = &memupd->FspmConfig;
> you can move this line inside `if` case in line 27

Acknowledged

File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:

https://review.coreboot.org/c/coreboot/+/83752/comment/fd0a38ae_4d6dbc6e?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)` […]

Acknowledged

File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:

https://review.coreboot.org/c/coreboot/+/83752/comment/cc940155_360d6b10?usp... :
PS28, Line 61: 	if (!num_entries)
             : 		return;
why are we skipping this check now ?

https://review.coreboot.org/c/coreboot/+/83752/comment/3e81e8ae_4136de90?usp... :
PS28, Line 81: 1000
ideally this is better to modify in a separate CL.

note: patch is intangible hence, don;t feel shy to submit more than one CL if that makes things meaningful rather combine everything into one.

https://review.coreboot.org/c/coreboot/+/83752/comment/c88cf4a4_5d6bd4cb?usp... :
PS28, Line 141: 	if (CONFIG(EC_GOOGLE_CHROMEEC) && google_chromeec_is_battery_present_and_above_critical_threshold()) {
was it more than > 96 ?

https://review.coreboot.org/c/coreboot/+/83752/comment/ad8faf6f_a7375010?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

https://review.coreboot.org/c/coreboot/+/83752/comment/19897376_d3765971?usp... :
PS28, Line 56: 	if (config->tdp_pl4 == 0) {
nit

```
if (!config->tdp_pl4)
   return;
```

that way you can save a tab space in below code.

https://review.coreboot.org/c/coreboot/+/83752/comment/26a709f4_97a53edc?usp... :
PS28, Line 59: (CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT)
u don't need additional braces for this config check

https://review.coreboot.org/c/coreboot/+/83752/comment/c41c2196_bf43eaf1?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 ?

https://review.coreboot.org/c/coreboot/+/83752/28/src/mainboard/google/brox/...

if needed modify in separate CL