Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33941 )
Change subject: src/soc/intel/cannonlake: Fix PMC and GPIO block values for PCH-H ......................................................................
Patch Set 2: Code-Review+1
(5 comments)
https://review.coreboot.org/#/c/33941/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@9 PS2, Line 9: In order for GPEs to work properly this part doesn't really fit the following sentence?
https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@13 PS2, Line 13: GPP_E : GPP_F : GPP_H : GPP_I : GPP_J : GPP_K : GPD This is visible from the diff, no need to list them here.
What I do miss however is where you got the numbers from. And please mention that the datasheet is not correct for the PMC_ numbers.
https://review.coreboot.org/#/c/33941/2//COMMIT_MSG@23 PS2, Line 23: after applying this change. Please adhere to the 72 char line limit in commit messages.
https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/gpio_soc_defs_cnp_h.h:
https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... PS2, Line 36: #define GPD 5 Please either use hex or decimal for all numbers. Please keep them sorted by value (makes it easier to review with the datasheet).
https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/pmc.h:
https://review.coreboot.org/#/c/33941/2/src/soc/intel/cannonlake/include/soc... PS2, Line 131: #define PMC_GPD 0x7 If these really differ from the datasheet, that must be commented (otherwise, somebody could "correct" these values by accident). Also, if you haven't already, please report the documentation error to Intel.