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:
(3 comments)
File src/mainboard/google/brox/romstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/e71fb9c6_449aa959?usp... : PS19, Line 27: MAX_NONTURBO_PERFORMANCE
Yes Subrata
- In normal flow Bootfrequency is set to 2 by default and in ramstage turbo is enabled and I see the typical case of CPU brown out happening.
at which stage, you are seeing "CPU brown out" ? romstage or ramstage ? I assume you are seeing Bootfrequency=2?
- If bootfrequency is set to 1 then i see TurboMode is enabled but TurboModeDisable (MSR_MISC_ENABLES) is set
who is setting `TurboModeDisable` ? I assume croeboot can always override this.
- If bootfrequency is set to 2 then i see TurboMode is enabled but TurboModeDisable is not set
- In turbo.c we are enabling turbo based on IA32_MISC_ENABLE - 0x1a0
Yes, we are suppose to enable turbo for all cores.
- CPUstrap is read only to check if there is a mismatch in bootfreq and P1 and if reset is required
My point is always CPU strap and boot freq should be set to P1 unless we plan to override with some lower value to reduce the freq
File src/mainboard/google/brox/variants/baseboard/brox/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/12d63b80_20739062?usp... : PS19, Line 57: * PL4 calculations - For USB C charger (Max Power):
For PL4 writeback to work on all 3 different chargers it is required to change pl4 based on adapters.
can you please explain why kernel overriding PL4 value is dependent on how coreboot calculating USB-PD capacity and overriding the Pl4 limit in low/critical boot case.
I'm trying to convey that booting with lower Pl4 is still fine with an assumption that the kernel should be able to override the Pl4 after the system reaches at stable charging state.
File src/mainboard/google/brox/variants/brox/ramstage.c:
https://review.coreboot.org/c/coreboot/+/83752/comment/bf3210a8_db364ac1?usp... : PS19, Line 19: .pl1_min_power = 15000,
We had discussed that changes reverting psys calculations https://review.coreboot.org/c/coreboot/+/83087 will be taken up as part this patch. If its an issue to keep it here - i will take it as a separate patch. Please let me know
you are changing the PL1 limit from 6W to 15W and I expect this can be done as part of a separate CL. Unable to follow what is the relation between some psys revert CL.
if you wish to make some more clean up then do that in additional CL. I read this CL is to set the lower PL4 to meet the low/critical boot phase.
-
Subrata Banik (Code Review)