Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79984?usp=email )
Change subject: soc/amd/phoenix/Makefile: conditionally add fsp_[m,s]_params.c
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/79984?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ife38ca6a548d7c3c2e765d9c9f30e0a4057bb373
Gerrit-Change-Number: 79984
Gerrit-PatchSet: 3
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:57:47 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79983?usp=email )
Change subject: soc/amd/phoenix/Kconfig: factor out FSP-specific options
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/79983?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I5e95fbfd9d16930ba3e6cc497557d61adba5a6fa
Gerrit-Change-Number: 79983
Gerrit-PatchSet: 2
Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:57:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Erik van den Bogaert, Felix Singer, Frans Hendriks, Jan Samek, Jonathon Hall, Michał Żygowski, Piotr Król.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79958?usp=email )
Change subject: intel skl mainboards: Move PcieRpEnable option below dt entries
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> Heh? What? Why lots of manual work? Where? […]
Please look at the results of your command. You'll
have things like
```
device ref pcie_rp3 on
# Ethernet controller
end
```
instead of the cannonical
```
device ref pcie_rp3 on end # Ethernet controller
```
Such things need to be fixed up. Hence I'd like to see the result
before we agree that the job is done.
You also forget about cases where we currently have an `on`
without matching FSP setting. These need to be turned `off`.
That's also manual work.
You wanted to do things your way. Now please see it through.
--
To view, visit https://review.coreboot.org/c/coreboot/+/79958?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I397497118d6868e4a6d4086e97901081da7d5fda
Gerrit-Change-Number: 79958
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:57:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Arthur Heymans, Chen, Gang C, Felix Held, Fred Reitberger, Jason Glenesk, Jincheng Li, Jérémy Compostella, Martin L Roth, Matt DeVillier.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78328?usp=email )
Change subject: device/device.h: Drop multiple links
......................................................................
Patch Set 8:
(1 comment)
File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/78328/comment/6081d485_3702125e :
PS8, Line 1837: if (device_probes_match(base_child, override_child))
This seems like another mechanism to add duplicate device nodes with the same
path. But for a different purpose. And to the same bus even. So I think that's
not to worry about for this patch, but may be worth keeping in mind.
The basic idea is to have different nodes with different settings that can't
both (all) be enabled (present) in an actual device unit at once, cf. CB:57111
for details.
--
To view, visit https://review.coreboot.org/c/coreboot/+/78328?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iab6fe269faef46ae77ed1ea425440cf5c7dbd49b
Gerrit-Change-Number: 78328
Gerrit-PatchSet: 8
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Chen, Gang C <gang.c.chen(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:47:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Erik van den Bogaert, Frans Hendriks, Jan Samek, Jonathon Hall, Michał Żygowski, Nico Huber, Piotr Król.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79958?usp=email )
Change subject: intel skl mainboards: Move PcieRpEnable option below dt entries
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> I looked through this now, and I fear this will be a lot of manual […]
Heh? What? Why lots of manual work? Where?
The options can be simply removed with:
```
dt_files=$( \
find src/mainboard -type f -name '*.cb' | \
xargs grep -l "soc/intel/skylake" \
) && sed -i'' '/.*PcieRpEnable.*/d' $dt_files
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/79958?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I397497118d6868e4a6d4086e97901081da7d5fda
Gerrit-Change-Number: 79958
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:44:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: comment
Attention is currently required from: Erik van den Bogaert, Frans Hendriks, Jan Samek, Jonathon Hall, Michał Żygowski, Piotr Król.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79958?usp=email )
Change subject: intel skl mainboards: Move PcieRpEnable option below dt entries
......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS6:
> Uderstood. Isn't it then better to just add the missing dt entries in response to CB:79917 and not duplicate work done there? Or if you really insist on preserving the state, suggesting incorporating the missing device dt entries into CB:79917?
Then how do people review this patch pretending that all devicetrees have been checked? Right... They need to go through all .cb files from that platform and compare the option with the devicetree, which is what I am doing here by moving all matching options from a given platform and at the same time I am making the review for others easier. The review is just trivial then. I could yet provide a command that prints out all files containing this option, so that people can compare their local tree against the list of modified files here.
I don't see where the work is duplicated. I am making sure that all affecting boards stay working and I am making the review quite trivial.
> Or if you really insist on preserving the state, suggesting incorporating the missing device dt entries into CB:79917?
Um, yes.. I do insist on preserving the device states. Otherwise this very likely breaks working configurations?
Both changes are two logically separate changes. One fixes something and prepares devicetrees, the other one switches over to a different interface. Not sure how this could be merged.
--
To view, visit https://review.coreboot.org/c/coreboot/+/79958?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I397497118d6868e4a6d4086e97901081da7d5fda
Gerrit-Change-Number: 79958
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:38:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Comment-In-Reply-To: Jan Samek <jan.samek(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Erik van den Bogaert, Felix Singer, Frans Hendriks, Jonathon Hall, Michał Żygowski, Piotr Król.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79958?usp=email )
Change subject: intel skl mainboards: Move PcieRpEnable option below dt entries
......................................................................
Patch Set 6:
(3 comments)
Patchset:
PS6:
I looked through this now, and I fear this will be a lot of manual
work for next patch as I expected. So please don't merge before you
rebased the next patch and we agreed on the final state.
File src/mainboard/google/eve/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/79958/comment/3888a49c_09e098e6 :
PS6, Line 128: Enable
Some of the comments look odd now. Maybe just remove the `Enable ` where
appropriate. You could also move the whole RP settings into their proper
place (would also avoid having to move comments and `end` keywords around
in the next patch).
File src/mainboard/protectli/vault_kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/79958/comment/985c040c_f0f356fc :
PS6, Line 188: device ref pcie_rp1 on
`# LAN`
plus the next 5 RPs
--
To view, visit https://review.coreboot.org/c/coreboot/+/79958?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I397497118d6868e4a6d4086e97901081da7d5fda
Gerrit-Change-Number: 79958
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Reviewer: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Reviewer: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Reviewer: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jan Samek <jan.samek(a)siemens.com>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Erik van den Bogaert <ebogaert(a)eltan.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Frans Hendriks <fhendriks(a)eltan.com>
Gerrit-Attention: Jonathon Hall <jonathon.hall(a)puri.sm>
Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:31:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Marek Maślanka, Subrata Banik.
Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79909?usp=email )
Change subject: soc/intel/common/block: Add support for watchdog
......................................................................
Patch Set 9: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/79909?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iaf7971f8407920a553fd91d2ed04193c882e08f1
Gerrit-Change-Number: 79909
Gerrit-PatchSet: 9
Gerrit-Owner: Marek Maślanka <mmaslanka(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Marek Maślanka <mmaslanka(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Comment-Date: Thu, 18 Jan 2024 16:05:32 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment