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 24:
(4 comments)
Patchset:
PS24: Thanks for accommodating the review feedback. Appreciate your dedication to understand the problem and go deep to resolve it.
File src/mainboard/google/brox/romstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/aaab83ef_706ecd4b?usp... : PS14, Line 32: FSPM_MAX_NONTURBO_FREQ
I have added comment with respect to this in the partnerbug Subrata - please take a look
Acknowledged
File src/mainboard/google/brox/romstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/21e5da29_2a5fdb5d?usp... : PS19, Line 27: MAX_NONTURBO_PERFORMANCE
at which stage, you are seeing "CPU brown out" ? romstage or ramstage ? I assume you are seeing Bootfrequency=2? -It happens in ramstage ( FspMultiPhaseSiInit)
The BSP always boots at the HFM, or P1 frequency, which is aligned with the CPU soft strap. I don't believe we need to file a bug to align with this expectation. You can dump the perf MSR to check the current frequency and compare it to the HFM.
Later, during the ramstage, when we enable Turbo and try to configure Turbo for all APs, we need to draw more power to switch to the supported turbo max.
Since the battery is unable to draw enough power, the CPU browns out.
The best solution to this situation would be to ensure that the BIOS is not enabling Turbo or the BSP frequency is limited to HFM/P1 max.
What I am saying is that the boot frequency override only makes sense at the ramstage and not at the romstage because we are already booting using P1/HFM until the romstage. This UPD is used to prevent the BSP from switching to the Turbo frequency, knowing that the power envelope is not sufficient to keep Turbo enabled.
who is setting TurboModeDisable ? I assume croeboot can always override this.
- FSP - in function VOID SetBootFrequency ( IN SI_POLICY_PPI *SiPolicyPpi )
If bootfreq is set 2 - I believe the boot will start in HPM and then switch to turbo mode (by looking at the code and logs) ..If this is not the expected behavior then we should follow it up a separate bug
Please read my explanation above, I'm insisting this UPD is more like ramstage specific UPD rather getting used in romstage. Because turbo freq is what ideally we wish to avoid to switch for BSP unless we have ample supply.
I will move this discussion for FSP arch sync. Hence, marking it resolved here
File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/a2982c6f_a96502cf?usp... : PS24, Line 77: 9 can you please move this into a Kconfig and keep a default value `9`. If any variant wish to override a better number, they can do that easily.
``` if (CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT) soc_config->tdp_pl4 = CONFIG_PL4_LIMIT_FOR_CRITICAL_BAT_BOOT; ```