Attention is currently required from: Ashish Kumar Mishra, Caveh Jalali, Dinesh Gehlot, Eric Herrmann, Forest Mittelberg, Jayvik Desai, Kapil Porwal, 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 19:
(2 comments)
File src/mainboard/google/brox/romstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/8b21c153_184572f6?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.
File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/d8a25daf_9ce1efd7?usp... : PS19, Line 57: * PL4 calculations - For USB C charger (Max Power):
I am referring PL4 writeback in depthcharge not in the kernel. are you seeing any feature gap or issue with the functionality in this implementation ?
I am repeating my questions. Why do you believe that overriding the PL4 at depthcharge would be sufficient to restore the original PL4 limit? Our boot time is less than or equal to 1 second, which is far too brief for the system to reach a stable charged state and restore the original PL4 limit.
I would prefer to avoid overriding PL4 from DC and rely on the kernel to restore the PL4's original value after we have reached a stable charge state.
Overriding PL4 from depthcharge may cause the same CPI brownout problem if there is insufficient fuel (which I would like to avoid). I believe I mentioned this during the call as well.
``` Low battery - Working fine with power button press Able to write back PL4 after exiting depthcharge (needs to validated on different boards) - seems to be sporadic ```
this is what you have mentioned during the previous call that overriding PL4 from depthcharge has a corner case as well.
-
Subrata Banik (Code Review)