Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/23580 )
Change subject: soc/intel/cannonlake: Add basic CPU PM entry of FSP ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.h@89 PS3, Line 89: power limit What is the unit?
https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.h@89 PS3, Line 89: Package PL1 - Package long duration turbo mode power limit : * Package PL2 - Short Duration Turbo Mode : * Package PL1 - Package short duration turbo mode power limit */ These comments don't seem to match the fields below.
https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.h@92 PS3, Line 92: uint32_t PPL1Power; : uint8_t PPL2_enable; : uint32_t PPL2Power; I know this file is not very consistent about the naming, but it would be better to use: uint32_t package_pl1; uint8_t package_pl2_enable; uint32_t package_pl2;
Also, would it be safe to assume that package_pl2_enable should be set to 1 if package_pl2 is provided? i.e. do we need to accept a separate package_pl2_enable from mainboard?
https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.c File src/soc/intel/cannonlake/chip.c:
https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.c@183 PS3, Line 183: params_t Can we name this tparams or test_params? params_t looks like a typedef.
https://review.coreboot.org/#/c/23580/3/src/soc/intel/cannonlake/chip.c@273 PS3, Line 273: Powermanagement space after power?