Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34724 )
Change subject: soc/intel/common: Implement power-failure-state handling
......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34724/5/src/soc/intel/common/block…
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/34724/5/src/soc/intel/common/block…
PS5, Line 583: void pmc_set_power_failure_state(const pci_devfn_t dev)
Why does the caller need to provide a dev pointer here? SoC knows how to get to the GEN_PMCON* reg on its own i.e. MMIO or PCI config. And this function doesn't care about it anyways.
--
To view, visit https://review.coreboot.org/c/coreboot/+/34724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic6970a79d9b95373c2855f4c92232d2aa05963bb
Gerrit-Change-Number: 34724
Gerrit-PatchSet: 5
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 07 Aug 2019 22:37:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34724 )
Change subject: soc/intel/common: Implement power-failure-state handling
......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/34724/5/src/soc/intel/common/block…
File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/34724/5/src/soc/intel/common/block…
PS5, Line 222: ,
no param name?
https://review.coreboot.org/c/coreboot/+/34724/5/src/soc/intel/common/block…
PS5, Line 223: )
same here?
https://review.coreboot.org/c/coreboot/+/34724/5/src/soc/intel/common/block…
PS5, Line 223: pmc_set_power_failure_state
Can you please add description to each of the functions as to how they differ? I believe one is a callback that SoC is supposed to provide? Also, it would be good to provide information about what params are expected.
--
To view, visit https://review.coreboot.org/c/coreboot/+/34724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic6970a79d9b95373c2855f4c92232d2aa05963bb
Gerrit-Change-Number: 34724
Gerrit-PatchSet: 5
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 07 Aug 2019 22:26:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34726 )
Change subject: soc/intel/{cnl,icl}: Use new power-failure-state API
......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34726/3/src/soc/intel/icelake/pmc.c
File src/soc/intel/icelake/pmc.c:
https://review.coreboot.org/c/coreboot/+/34726/3/src/soc/intel/icelake/pmc.…
PS3, Line 39: 1
> same as skylake; symbolic constant
Done
https://review.coreboot.org/c/coreboot/+/34726/3/src/soc/intel/icelake/pmc.…
PS3, Line 41: 1
> same as skylake; symbolic constant
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/34726
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib13eac00002232d4377f683ad92b04a0907529f3
Gerrit-Change-Number: 34726
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Wed, 07 Aug 2019 21:46:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34725 )
Change subject: soc/intel/skylake: Use new power-failure-state API
......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34725/3/src/soc/intel/skylake/pmc.c
File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/34725/3/src/soc/intel/skylake/pmc.…
PS3, Line 60: 1
> A symbolic constant would be nice
Interestingly, it was already defined but never used :) thanks.
https://review.coreboot.org/c/coreboot/+/34725/3/src/soc/intel/skylake/pmc.…
PS3, Line 62: 1
> same
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/34725
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie72753764ecd876e6cb999fa0074d1114ae5efcf
Gerrit-Change-Number: 34725
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Wed, 07 Aug 2019 21:46:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34724 )
Change subject: soc/intel/common: Implement power-failure-state handling
......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34724/4/src/soc/intel/common/block…
File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/34724/4/src/soc/intel/common/block…
PS4, Line 602: state == MAINBOARD_POWER_STATE_ON
> It makes sense to squash the two CLs together since it is easier to see what the overall logic is.
Hmmm, already worked on the next round when you wrote this. Should I still
squash it?
--
To view, visit https://review.coreboot.org/c/coreboot/+/34724
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic6970a79d9b95373c2855f4c92232d2aa05963bb
Gerrit-Change-Number: 34724
Gerrit-PatchSet: 5
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Wed, 07 Aug 2019 21:45:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34728 )
Change subject: soc/intel/common: Implement MAINBOARD_POWER_STATE_PREVIOUS
......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34728/3/src/soc/intel/apollolake/p…
File src/soc/intel/apollolake/pmc.c:
https://review.coreboot.org/c/coreboot/+/34728/3/src/soc/intel/apollolake/p…
PS3, Line 125: PCI_DEV_INVALID
> Please add a comment here as to why you're using PCI_DEV_INVALID
I think that would go way too far. FSP is hiding PCI devices from coreboot.
So the device is invalid. We cannot comment every single line that is affected
by an FSP deficiency. Otherwise we'd have much more comments than code.
--
To view, visit https://review.coreboot.org/c/coreboot/+/34728
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I22bd71e4f3b67309c1aa0cb6faeb5959521bf656
Gerrit-Change-Number: 34728
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Comment-Date: Wed, 07 Aug 2019 21:40:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: comment
Nico Huber has uploaded a new patch set (#9) to the change originally created by Thomas Heijligen. ( https://review.coreboot.org/c/coreboot/+/31352 )
Change subject: soc/intel/apl: Implement power-failure-state API
......................................................................
soc/intel/apl: Implement power-failure-state API
The PMC is not represented as a PCI device on this platform,
so we pass PCI_DEV_INVALID, and ignore the device argument to
pmc_soc_set_afterg3_en().
Needed some Makefile changes to be able to compile in SMM.
TEST=Manually tested all four cases with Kontron/mAL10,
at the end of this patch series.
Change-Id: Ibf218b90088a45349c54f4b881e895bb852e88bb
Signed-off-by: Nico Huber <nico.huber(a)secunet.com>
---
M src/soc/intel/apollolake/Makefile.inc
M src/soc/intel/apollolake/include/soc/pm.h
M src/soc/intel/apollolake/pmc.c
M src/soc/intel/common/block/fast_spi/Makefile.inc
4 files changed, 23 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/31352/9
--
To view, visit https://review.coreboot.org/c/coreboot/+/31352
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ibf218b90088a45349c54f4b881e895bb852e88bb
Gerrit-Change-Number: 31352
Gerrit-PatchSet: 9
Gerrit-Owner: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: newpatchset
Hello Patrick Rudolph, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34726
to look at the new patch set (#4).
Change subject: soc/intel/{cnl,icl}: Use new power-failure-state API
......................................................................
soc/intel/{cnl,icl}: Use new power-failure-state API
The PMC is not represented as a PCI device on these platforms,
so we pass PCI_DEV_INVALID, and ignore the device argument to
pmc_soc_set_afterg3_en().
pmc_soc_restore_power_failure() is only called from SMM, so add
`pmc.c` to the `smm` class. Once all platforms moved to the new
API, it can be implemented in a central place, avoiding the weak-
function trap.
TEST=Confirmed register states for MAINBOARD_POWER_STATE_OFF
and MAINBOARD_POWER_STATE_ON on Cannonlake PCH-H, at the
end of this patch series.
Change-Id: Ib13eac00002232d4377f683ad92b04a0907529f3
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
---
M src/mainboard/google/sarien/chromeos.c
M src/soc/intel/cannonlake/Makefile.inc
M src/soc/intel/cannonlake/pmc.c
M src/soc/intel/icelake/Makefile.inc
M src/soc/intel/icelake/pmc.c
5 files changed, 23 insertions(+), 113 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/34726/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/34726
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ib13eac00002232d4377f683ad92b04a0907529f3
Gerrit-Change-Number: 34726
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: newpatchset