[coreboot-gerrit] Change in coreboot[master]: soc/intel/cannonlake: Add basic CPU PM entry of FSP

[Furquan Shaikh (Code Review)]

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?



-- 
To view, visit https://review.coreboot.org/23580
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81bd04f5de05543ad00ce2562839a51a5c0ae047
Gerrit-Change-Number: 23580
Gerrit-PatchSet: 3
Gerrit-Owner: Lijian Zhao <lijian.zhao at intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Comment-Date: Sat, 03 Feb 2018 05:50:57 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180203/e8030f82/attachment.html>