[coreboot-gerrit] Change in coreboot[master]: [WIP]soc/intel/common/block: Add Intel PMC support

[Shaunak Saha (Code Review)]

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/intelblocks/pmc.h
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/intelblocks/pmc.h
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/Kconfig
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


-- 
To view, visit https://review.coreboot.org/19349
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3d96fc23a98c30e8ea0969a7be09d217eeaa889
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Shaunak Saha <shaunak.saha at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik at intel.com>
Gerrit-Reviewer: Brandon Breitenstein <brandon.breitenstein at intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Shaunak Saha <shaunak.saha at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes