Attention is currently required from: Raul Rangel, Furquan Shaikh, Matt DeVillier, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52493 )
Change subject: [RFC] soc/intel/skylake: Introduce new method for setting device states
......................................................................
Patch Set 11:
(3 comments)
File src/soc/intel/skylake/fspdevmap.h:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-118135):
https://review.coreboot.org/c/coreboot/+/52493/comment/dd87f7a4_e6eba445
PS11, Line 12: void set_dev_state_by_devicetree(const struct device_fspoption_map *const devmap, uint8_t arrsize);
line over 96 characters
File src/soc/intel/skylake/fspdevmap.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-118135):
https://review.coreboot.org/c/coreboot/+/52493/comment/ee487a9b_2241d195
PS11, Line 6: void set_dev_state_by_devicetree(const struct device_fspoption_map *const devmap, uint8_t arrsize)
line over 96 characters
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-118135):
https://review.coreboot.org/c/coreboot/+/52493/comment/463592cf_27a1785a
PS11, Line 13: if (dev) {
braces {} are not necessary for single statement blocks
--
To view, visit https://review.coreboot.org/c/coreboot/+/52493
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70fde306c65a8881f565c5f923be20f380ea64d3
Gerrit-Change-Number: 52493
Gerrit-PatchSet: 11
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 20:35:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Raul Rangel, Furquan Shaikh, Matt DeVillier, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held.
Hello build bot (Jenkins), Raul Rangel, Nico Huber, Furquan Shaikh, Matt DeVillier, Tim Wawrzynczak, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/52493
to look at the new patch set (#11).
Change subject: [RFC] soc/intel/skylake: Introduce new method for setting device states
......................................................................
[RFC] soc/intel/skylake: Introduce new method for setting device states
We mostly use the same mechanism for setting the state of internal
devices, but we copy it over again and again. Thus, introduce a new
method trying to reduce redundant code.
Basically, this uses an array of structs containing the DEVFN number,
its corresponding FSP option and an indicator if the FSP option is
inverted. The struct maps the device to its related FSP option and
`set_dev_state_by_devicetree()` iterates over the array using the usual
mechanism.
Unresolved issues:
- Where should fspdevmap{.c,.h} be implemented?
Change-Id: I70fde306c65a8881f565c5f923be20f380ea64d3
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M src/soc/intel/skylake/Makefile.inc
M src/soc/intel/skylake/chip.c
A src/soc/intel/skylake/fspdevmap.c
A src/soc/intel/skylake/fspdevmap.h
M src/soc/intel/skylake/romstage/Makefile.inc
M src/soc/intel/skylake/romstage/fsp_params.c
6 files changed, 67 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/52493/11
--
To view, visit https://review.coreboot.org/c/coreboot/+/52493
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70fde306c65a8881f565c5f923be20f380ea64d3
Gerrit-Change-Number: 52493
Gerrit-PatchSet: 11
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Michael Niewöhner, Patrick Rudolph, Felix Held.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48934 )
Change subject: [RFC] device/pnp: do not warn for generic unassigned ressources
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> I still don't see what this warning really is for... Sombody ports a board and adds a SIO driver. This person can check what registers "need" to be configured. So, would they want to warn themselves (which is not very sensible ofc) or others? What if a SIO gets used by a different board later but the register does *not* need to be set there because the board doesn't use that function? Then they get a warning, which is meaningless for that board.
I can't say that I know all SIOs. It seems possible that there are LDNs that if
enabled need a specific register set.
If an LDN is disabled, the resource assignment shouldn't run, no warning would
be printed.
> Nico, I read your comment again and now wonder, if I got it right or not. Is your idea to first remove all PNP_MSC that are not needed and then, if none is left, drop the warning, which is dead code then?
Not my idea, but a valid conclusion if all PNP_MSC* declarations look spurious
(wrt. to the warning).
--
To view, visit https://review.coreboot.org/c/coreboot/+/48934
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic23852d525a3f9c95118b113d419f815cac002d3
Gerrit-Change-Number: 48934
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 20:31:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52776 )
Change subject: soc/intel/cannonlake: Rename `SOC_INTEL_COMETLAKE`
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS1:
> Well, I don't care that much, but it would be consistent imo. […]
SOC_INTEL_COMETLAKE is still a valid option AFAICS. We just wouldn't know
what FSP to take. But we can't guarantee that any of the existing FSPs
actually work, so it's all vague anyway. If you want to enforce it, you
could add a dependency to SOC_INTEL_COMETLAKE. e.g.
config SOC_INTEL_COMETLAKE
def_bool y
depends on SOC_INTEL_COMETLAKE_1 || ...
and drop the selects.
--
To view, visit https://review.coreboot.org/c/coreboot/+/52776
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I993f96f032c3b76a464474c83fee4442968be152
Gerrit-Change-Number: 52776
Gerrit-PatchSet: 2
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Fri, 30 Apr 2021 20:23:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak, Patrick Rudolph.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52808 )
Change subject: soc/intel/*: Update data types for variables holding PCH_DEVFN_* macros
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/52808
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic8880de984e6eceda4cbe141e118f3a5fdd672a2
Gerrit-Change-Number: 52808
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Fri, 30 Apr 2021 20:18:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Raul Rangel, Furquan Shaikh, Matt DeVillier, Angel Pons, Michael Niewöhner, Patrick Rudolph, Felix Held.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52493 )
Change subject: [RFC] soc/intel/skylake: Introduce new method for setting device states
......................................................................
Patch Set 10:
(1 comment)
File src/soc/intel/skylake/fspdevmap.h:
https://review.coreboot.org/c/coreboot/+/52493/comment/33d387aa_1148638e
PS9, Line 13: const
> You mean, the first `const` in the forward declaration and both in the function definition or none a […]
The former.
The currently present, first `const` is part of the type (pointer to const
struct device_fspoption_map), it's correct and required. The other `const`
are a qualification of the parameter and only meaningful where the para-
meter is used. (should we start writing `uint8_t const arrsize` to make
it more clear?)
--
To view, visit https://review.coreboot.org/c/coreboot/+/52493
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I70fde306c65a8881f565c5f923be20f380ea64d3
Gerrit-Change-Number: 52493
Gerrit-PatchSet: 10
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 20:16:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment
Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/52808 )
Change subject: soc/intel/*: Update data types for variables holding PCH_DEVFN_* macros
......................................................................
soc/intel/*: Update data types for variables holding PCH_DEVFN_* macros
The usage of `pci_devfn_t` here is misleading, as these intentionally
store the `PCH_DEVFN_*` macros so they can be used across `smm` and
`ramstage` without requiring the device model. Update to `unsigned int`
instead, as `pci_devfn_t` implies the data is an MMCONF-compatible PCI
devfn offset.
Change-Id: Ic8880de984e6eceda4cbe141e118f3a5fdd672a2
Signed-off-by: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
---
M src/soc/intel/alderlake/elog.c
M src/soc/intel/jasperlake/elog.c
M src/soc/intel/skylake/elog.c
M src/soc/intel/tigerlake/elog.c
4 files changed, 4 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/52808/1
diff --git a/src/soc/intel/alderlake/elog.c b/src/soc/intel/alderlake/elog.c
index 84765d9..7b0fd87 100644
--- a/src/soc/intel/alderlake/elog.c
+++ b/src/soc/intel/alderlake/elog.c
@@ -12,7 +12,7 @@
#include <types.h>
struct pme_map {
- pci_devfn_t devfn;
+ unsigned int devfn;
unsigned int wake_source;
};
diff --git a/src/soc/intel/jasperlake/elog.c b/src/soc/intel/jasperlake/elog.c
index 858b371..61b8fb8 100644
--- a/src/soc/intel/jasperlake/elog.c
+++ b/src/soc/intel/jasperlake/elog.c
@@ -12,7 +12,7 @@
#include <types.h>
struct pme_map {
- pci_devfn_t devfn;
+ unsigned int devfn;
unsigned int wake_source;
};
diff --git a/src/soc/intel/skylake/elog.c b/src/soc/intel/skylake/elog.c
index c6be670..1c80517 100644
--- a/src/soc/intel/skylake/elog.c
+++ b/src/soc/intel/skylake/elog.c
@@ -26,7 +26,7 @@
}
struct pme_status_info {
- pci_devfn_t devfn;
+ unsigned int devfn;
uint8_t reg_offset;
uint32_t elog_event;
};
diff --git a/src/soc/intel/tigerlake/elog.c b/src/soc/intel/tigerlake/elog.c
index 0fccf84..93174be 100644
--- a/src/soc/intel/tigerlake/elog.c
+++ b/src/soc/intel/tigerlake/elog.c
@@ -12,7 +12,7 @@
#include <types.h>
struct pme_map {
- pci_devfn_t devfn;
+ unsigned int devfn;
unsigned int wake_source;
};
--
To view, visit https://review.coreboot.org/c/coreboot/+/52808
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic8880de984e6eceda4cbe141e118f3a5fdd672a2
Gerrit-Change-Number: 52808
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-MessageType: newchange
Attention is currently required from: Nico Huber, Furquan Shaikh, Tim Wawrzynczak, Patrick Rudolph, Felix Held.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48934 )
Change subject: [RFC] device/pnp: do not warn for generic unassigned ressources
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> I still don't see what this warning really is for... Sombody ports a board and adds a SIO driver. […]
Nico, I read your comment again and now wonder, if I got it right or not. Is your idea to first remove all PNP_MSC that are not needed and then, if none is left, drop the warning, which is dead code then?
--
To view, visit https://review.coreboot.org/c/coreboot/+/48934
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ic23852d525a3f9c95118b113d419f815cac002d3
Gerrit-Change-Number: 48934
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Fri, 30 Apr 2021 20:00:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-MessageType: comment