Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42440 )
Change subject: soc/intel/cannonlake: Add PchPmPwrCycDur to chip options ......................................................................
Patch Set 12:
(12 comments)
Patch Set 10:
also thank you Sridhar for adding the checks đ
Thanks for review đ đ
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/9/src/soc/intel/cannonlake/fs... PS9, Line 536: if (config->PchPmPwrCycDur)
If PchPmPwrCycDur is configured (or not at all configured) to 0 , then FSP sets the default value(4s [âŚ]
Done
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 37: MinAssrtDur0s = 0, : MinAssrtDur60us = 60, : MinAssrtDur1ms = 1000, : MinAssrtDur50ms = 50000, : MinAssrtDur98ms = 98000, : MinAssrtDur500ms = 500000, : MinAssrtDur1s = 1000000, : MinAssrtDur2s = 2000000, : MinAssrtDur3s = 3000000, : MinAssrtDur4s = 4000000,
I think you need another tab before `=` on everything that's not 500ms
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 93: struct
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 110:
extra blank line
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 116: enum min_assrt_dur slp_s4_asst_dur_list[] = {
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 121: enum
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 126: enum
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 131: enum
const
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 155: .slp_a = MinAssrtDur2s, : .slp_s4 = MinAssrtDur4s, : .slp_s3 = MinAssrtDur50ms, : .pm_pwr_cyc_dur = MinAssrtDur4s
line up the right side, please
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 164:
nit: register-encoded
Done
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 164: Converts register encoded values into assertion durations(in microseconds) and get : * the configured values.
You could just say something like "Convert assertion durations from register-encoded to microseconds [âŚ]
Ack
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 177: "Set PmPwrCycDur to 4s as given PmPwrCycDur(%d) violates PCH EDS spec\n",
this line is 1 char too wide
Done