Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/19349 )
Change subject: [WIP]soc/intel/common/block: Add Intel PMC support ......................................................................
Patch Set 4:
(13 comments)
https://review.coreboot.org/#/c/19349/1//COMMIT_MSG Commit Message:
PS1, Line 7: [WIP]
remove this
Done
https://review.coreboot.org/#/c/19349/2/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/pmc.h:
PS2, Line 4: * Copyright 2017 Intel Corporation.
i guess one (C) i missing
Done
PS2, Line 27: void pmc_enable_pm1_control(uint32_t mask); : void pmc_disable_pm1_control(uint32_t mask);
can we have single function with bool argument as enable/disable
Will fix.
https://review.coreboot.org/#/c/19349/3/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/pmc.h:
Line 18:
#include the headers that provide the types you use in this header.
Done
Line 19: /* SMI */
Provide some documentation for these functions. If they are returning a val
Done
Line 36: void pmc_enable_gpe(uint32_t mask);
something to think about. GPE block is typically longer than a 32-bit value
Agreed. Will try to name these in more meaningful way and add comments.
https://review.coreboot.org/#/c/19349/1/src/soc/intel/common/block/pmc/Kconf... File src/soc/intel/common/block/pmc/Kconfig:
PS1, Line 4: PMC
please describe what PMC stands for
Done
https://review.coreboot.org/#/c/19349/1/src/soc/intel/common/block/pmc/pmc.c File src/soc/intel/common/block/pmc/pmc.c:
PS1, Line 31:
use tabs
Done
https://review.coreboot.org/#/c/19349/3/src/soc/intel/common/block/pmc/pmc.c File src/soc/intel/common/block/pmc/pmc.c:
Line 44: }
This whole file is not consistent with the proper indention of using tabs.
Done
PS3, Line 49: #define SMI_STS_DEF : #include <soc/bit_def.h>
What is this? Just provide a function that provides the array and its size.
OK. Will implement the function way. I did it this way so that the arrays are replaced by proper SOC bit defines during compile time.
Line 85: sts |= (1 << FAKE_PM1_SMI_STS);
This is specific to apollolake's brokenness. It shouldn't be in "common" wi
Agreed. Will fix.
PS3, Line 140: #define PM1_STS_DEF : #include <soc/bit_def.h>
same as above
Done
PS3, Line 232: #define GPE_STS_DEF : #include <soc/bit_def.h>
ditto
Done