Attention is currently required from: Ivy Jian, Nick Vaccaro, Shelley Chen.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79355?usp=email )
Change subject: mb/google/brox: Fix memory config
......................................................................
Patch Set 3:
(1 comment)
File src/mainboard/google/brox/variants/baseboard/brox/gpio.c:
https://review.coreboot.org/c/coreboot/+/79355/comment/5523b359_b273f4cb :
PS3, Line 404: const struct pad_config *__weak variant_romstage_gpio_table(size_t *num)
: {
I am afraid the weak override here will be overridden by the table here - https://review.coreboot.org/c/coreboot/+/79295/4/src/mainboard/google/brox/…. It makes sense to move this config over to the other table.
--
To view, visit https://review.coreboot.org/c/coreboot/+/79355?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: I17615cda7df10e73e49fb49f736728787ef7625d
Gerrit-Change-Number: 79355
Gerrit-PatchSet: 3
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Comment-Date: Thu, 30 Nov 2023 23:16:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Shelley Chen has uploaded a new patch set (#3). ( https://review.coreboot.org/c/coreboot/+/79355?usp=email )
Change subject: mb/google/brox: Fix memory config
......................................................................
mb/google/brox: Fix memory config
Fix up the memory config for brox based on the schematics. Also,
since memory training needs to happen in romstage, initializing the
MEM_STRAP & MEM_CH_SEL gpios for use in romstage.
BUG=b:300690448
BRANCH=None
TEST=emerge-brox coreboot
Change-Id: I17615cda7df10e73e49fb49f736728787ef7625d
Signed-off-by: Shelley Chen <shchen(a)google.com>
---
M src/mainboard/google/brox/variants/baseboard/brox/gpio.c
M src/mainboard/google/brox/variants/baseboard/brox/memory.c
2 files changed, 58 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/79355/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/79355?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: I17615cda7df10e73e49fb49f736728787ef7625d
Gerrit-Change-Number: 79355
Gerrit-PatchSet: 3
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Jérémy Compostella, Simon Glass.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79317?usp=email )
Change subject: Introduce a coreboot Control Block (CCB)
......................................................................
Patch Set 2: Code-Review-2
(5 comments)
Patchset:
PS2:
Sorry but I do not understand why we are suddenly proceeding to implementation with this. Everywhere this was discussed before in both the leadership meetings and on the mailing list, you have repeatedly received feedback from multiple people that we shouldn't add yet another configuration backend and instead just add serial control to any of the existing ones. I feel like you have consistently just ignored that feedback and kept bringing up the same proposal again and again without even really engaging the objections.
I still maintain that this is not good for coreboot, the added complexity to firmware layout and cbfstool is in no way worth the benefit, this interacts badly with edge cases on certain platforms or other feature configurations (e.g. verification), and we should focus on consolidating the existing configuration backends we have rather than adding yet another one that is super narrow and will in practice only be used for a single thing (especially when it is so invasive). People have proposed plenty of alternatives that could be used for post-build serial control including option table (CMOS), VPD, FW_CONFIG (CBI) and a CBFS file — in fact, the option table mechanism for this has always existed, and for CBFS files we have an almost finished patch series which would just need to be rebased and cleaned up (CB:45208). The only real argument for adding all this extra stuff instead is to control serial output in a very narrow time window in the early bootblock before CBFS access is available, and it seems incredibly unlikely that something goes wrong in that tiny early window during a development phase where you don't still have serial console enabled by default anyway.
If you are adamant to keep pushing this approach then I guess maybe we'll need to have another big discussion and force a decision in the leadership meeting or something. But I don't think this should be decided by just continuously pushing the same thing until the people with objections get tired or happen to be on vacation. So I'll -2 this until I'm convinced there is a majority in favor.
File Documentation/technotes/ccb.md:
https://review.coreboot.org/c/coreboot/+/79317/comment/d2670d71_88bd445f :
PS2, Line 79: Some boards used a signed bootblock which cannot easily be modified after
: building, since it requires resigning parts of the image. To address this, the
: CCB could be stored in the romstage instead. This means it is unable to affect
: the operation of bootblock, of course. This could be controlled by a new
: ROMSTAGE_CCB option.
This would literally invalidate the only reason you've used to justify this approach (over other post-build configuration options like CBFS files).
File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/79317/comment/53d43d0b_7ad2e5f2 :
PS2, Line 45: CCB(., 0x10)
This file only affects x86 devices (excluding AMD). Expanding this to other platforms puts the burden of finding room for this in the memlayout (and remembering to do so) on every single platform owner, which will lead to a fragmented landscape where only a random selection of platforms supports this. That's another good reason to reject this solution over the alternatives that don't need to pass the setting forward between stages.
File src/arch/x86/postcar.c:
https://review.coreboot.org/c/coreboot/+/79317/comment/c2ec411e_cea94f82 :
PS2, Line 33: console_init();
Moving console_init() down here makes it impossible to debug CBMEM initialization handlers, of which there are many and which can sometimes be non-trivial.
File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/79317/comment/dc0e3bfe_6f107e3b :
PS2, Line 433: default y
Any feature for runtime control of console behavior should probably be based on loglevel as it was suggested back in CB:45208, not create yet another orthogonal setting.
--
To view, visit https://review.coreboot.org/c/coreboot/+/79317?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: I1f6a7a3e136067859e4e8c2043cf39f2a1771c0f
Gerrit-Change-Number: 79317
Gerrit-PatchSet: 2
Gerrit-Owner: Simon Glass <sjg(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Simon Glass <sjg(a)chromium.org>
Gerrit-Comment-Date: Thu, 30 Nov 2023 22:18:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Arthur Heymans, Cliff Huang, David Milosevic, Lance Zhao, Martin L Roth, Maximilian Brune, Patrick Rudolph, Tim Wawrzynczak.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78071?usp=email )
Change subject: acpi: Add PPTT support
......................................................................
Patch Set 22: Code-Review+1
(1 comment)
File src/acpi/acpi_pptt.c:
https://review.coreboot.org/c/coreboot/+/78071/comment/21a942a0_5df9e9f3 :
PS17, Line 131: cache_reference_t cache_refs[CONFIG_ACPI_PPTT_MAX_CACHES];
> Done
Sorry for the delay. We'll have to see how this works once there are real-hardware
ports, I guess. And then we might have to adjust. But nothing to do right now.
--
To view, visit https://review.coreboot.org/c/coreboot/+/78071?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: Ia119e1ba15756704668116bdbc655190ec94ff10
Gerrit-Change-Number: 78071
Gerrit-PatchSet: 22
Gerrit-Owner: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Reviewer: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-CC: Martin L Roth <gaumless(a)gmail.com>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-Comment-Date: Thu, 30 Nov 2023 22:17:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-MessageType: comment
Shelley Chen has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/79355?usp=email )
Change subject: mb/google/brox: Fix memory config
......................................................................
mb/google/brox: Fix memory config
Fix up the memory config for brox based on the schematics. Also,
since memory training needs to happen in romstage, initializing the
MEM_STRAP gpios in for romstage.
BUG=b:300690448
BRANCH=None
TEST=emerge-brox coreboot
Change-Id: I17615cda7df10e73e49fb49f736728787ef7625d
Signed-off-by: Shelley Chen <shchen(a)google.com>
---
M src/mainboard/google/brox/variants/baseboard/brox/gpio.c
M src/mainboard/google/brox/variants/baseboard/brox/memory.c
2 files changed, 56 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/79355/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/79355?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: I17615cda7df10e73e49fb49f736728787ef7625d
Gerrit-Change-Number: 79355
Gerrit-PatchSet: 2
Gerrit-Owner: Shelley Chen <shchen(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Bao Zheng, Cliff Huang, Felix Held, Fred Reitberger, Jason Glenesk, Lance Zhao, Matt DeVillier, Tim Wawrzynczak, Zheng Bao.
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79174?usp=email )
Change subject: soc/amd: Add DBG2 ACPI table
......................................................................
Patch Set 11: Code-Review+1
(1 comment)
Patchset:
PS11:
LGTM. I was hoping to test this today but did not get time.
You can try it on skyrim using altfw. Make sure you sync to the latest source. Use the *non-serial* image. Then if U-Boot shows a serial console on boot, all is well.
Thank you for doing this!
--
To view, visit https://review.coreboot.org/c/coreboot/+/79174?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: I3c97a78d1889549421baf0bc1a2e8f959a0f47e2
Gerrit-Change-Number: 79174
Gerrit-PatchSet: 11
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Cliff Huang <cliff.huang(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: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Simon Glass <sjg(a)chromium.org>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Thu, 30 Nov 2023 21:19:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Benjamin Doron, Martin L Roth, Maximilian Brune, Patrick Rudolph.
Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/78913?usp=email )
Change subject: payloads: Add uefistub payload
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> I struggle to understand the goal here. […]
BTW if the concern is the size, U-Boot can now be built without the cmdline and at some point it will be possible to boot EFI apps in that mode. It makes U-Boot very small
--
To view, visit https://review.coreboot.org/c/coreboot/+/78913?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: I4093378e89c3cb43fb0846666de80a7da36b03f1
Gerrit-Change-Number: 78913
Gerrit-PatchSet: 4
Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Simon Glass <sjg(a)chromium.org>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Comment-Date: Thu, 30 Nov 2023 21:17:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org>
Gerrit-MessageType: comment