Tim Wawrzynczak 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 10:
(11 comments)
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
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 93: struct const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 110: extra blank line
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
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 121: enum const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 126: enum const
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 131: enum const
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
https://review.coreboot.org/c/coreboot/+/42440/10/src/soc/intel/cannonlake/f... PS10, Line 164: nit: register-encoded
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."
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