coreboot-gerrit January 2024

coreboot-gerrit@coreboot.org

1 participants
3043 discussions

[XS] Change in coreboot[main]: mb/prodrive/hermes: Change UART from hidden to on
by Christian Walter (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Patrick Rudolph. Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79786?usp=email ) Change subject: mb/prodrive/hermes: Change UART from hidden to on ...................................................................... Patch Set 3: (1 comment) Patchset: PS2: > Does uart_fill_ssdt() still generate valid ACPI code for UART2? […] You are actually right. UART2 is not exposed anymore. -- To view, visit https://review.coreboot.org/c/coreboot/+/79786?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: If24e1bf920c0b3edaefb4e20af66e33f4ddc1f3f Gerrit-Change-Number: 79786 Gerrit-PatchSet: 3 Gerrit-Owner: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Comment-Date: Thu, 04 Jan 2024 20:04:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: device/pciexp_device.c: Fix setting Max Payload Size
by Krystian Hebel (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Kyösti Mälkki, Michał Żygowski, Nico Huber, Patrick Rudolph. Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77338?usp=email ) Change subject: device/pciexp_device.c: Fix setting Max Payload Size ...................................................................... Patch Set 11: (1 comment) File src/device/pciexp_device.c: https://review.coreboot.org/c/coreboot/+/77338/comment/3aafabfa_77bf440d : PS10, Line 645: pciexp_limit_max_payload_ctl(dev->bus->dev, pciexp_configured_max_payload(dev), cap); > @Krystian, since this is your comment, could you please take another look and verify whether your co […] Acknowledged -- To view, visit https://review.coreboot.org/c/coreboot/+/77338?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: I24386dc208363b7d94fea46dec25c231a3968225 Gerrit-Change-Number: 77338 Gerrit-PatchSet: 11 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 04 Jan 2024 19:43:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com> Comment-In-Reply-To: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[main]: device/pciexp_device.c: Fix setting Max Payload Size
by Krystian Hebel (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Angel Pons, Arthur Heymans, Felix Held, Kyösti Mälkki, Michał Żygowski, Nico Huber, Patrick Rudolph. Krystian Hebel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77338?usp=email ) Change subject: device/pciexp_device.c: Fix setting Max Payload Size ...................................................................... Patch Set 11: (4 comments) File src/device/pciexp_device.c: https://review.coreboot.org/c/coreboot/+/77338/comment/34f0ccc4_7b202c1b : PS11, Line 594: root_devcap The name no longer describes what this variable holds. https://review.coreboot.org/c/coreboot/+/77338/comment/a4dee813_1d3a4cd3 : PS11, Line 598: awlays always https://review.coreboot.org/c/coreboot/+/77338/comment/059ccb24_7eeb1a46 : PS11, Line 608: & PCI_EXP_DEVCAP_PAYLOAD No need to mask it here, it was already masked right after reading. https://review.coreboot.org/c/coreboot/+/77338/comment/6e5a16dd_9e217dc7 : PS11, Line 630: root->max_payload_set = 1; I think this still doesn't catch case where two endpoints have different max payload sizes, for example: - root has max size = 5, there are two endpoints directly below root, A and B (they may be whole chains, but let's keep it simple), scanned in that order - endpoint A has max size = 4, the code sees it and limits root's payload size to 4 - endpoint B has max size = 3, the code sees it and limits root's payload size to 3 At this point endpoint A and root have different max payload sizes configured. I think this would be fixed by restarting the loop in `pciexp_scan_bus()` (in fact, `pciexp_configure_max_payload_size()` would be enough) `if ((root->max_payload_set == 1) && (max_payload < root_max_payload))`. Best not to do this recurrently though, with many devices a lot of stack space could be wasted. -- To view, visit https://review.coreboot.org/c/coreboot/+/77338?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: I24386dc208363b7d94fea46dec25c231a3968225 Gerrit-Change-Number: 77338 Gerrit-PatchSet: 11 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Krystian Hebel <krystian.hebel(a)3mdeb.com> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Thu, 04 Jan 2024 19:43:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: arch/x86/include/smm: use inline asm from drivers/smmstore/ramstage
by Felix Held (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Jérémy Compostella. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79827?usp=email ) Change subject: arch/x86/include/smm: use inline asm from drivers/smmstore/ramstage ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/79827/comment/720f0de5_f6fa5d66 : PS1, Line 10: easier : to understand there's also some function difference in the different outb parameters -- To view, visit https://review.coreboot.org/c/coreboot/+/79827?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: I73837cab75429014897486b38a5c56f93a850f96 Gerrit-Change-Number: 79827 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Comment-Date: Thu, 04 Jan 2024 18:38:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: vendorcode/google/chromeos: Use unsigned int for "factory_config"
by Julius Werner (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Eric Lai, Kapil Porwal, Shelley Chen, Subrata Banik, Tim Van Patten. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79769?usp=email ) Change subject: vendorcode/google/chromeos: Use unsigned int for "factory_config" ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/79769?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: I3021b8646de4750b4c8e2a2981f42500894fa2d0 Gerrit-Change-Number: 79769 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-Comment-Date: Thu, 04 Jan 2024 18:09:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: vendorcode/google/chromeos: Use unsigned int for "factory_config"
by Subrata Banik (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Eric Lai, Julius Werner, Kapil Porwal, Shelley Chen, Tim Van Patten. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79769?usp=email ) Change subject: vendorcode/google/chromeos: Use unsigned int for "factory_config" ...................................................................... Patch Set 4: (1 comment) File src/vendorcode/google/chromeos/chromeos.h: https://review.coreboot.org/c/coreboot/+/79769/comment/2ed0305a_e0e627c4 : PS3, Line 20: CHROMEOS_ERROR > But it is not something that should be used more widely? It is specifically only for functions that […] Acknowledged -- To view, visit https://review.coreboot.org/c/coreboot/+/79769?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: I3021b8646de4750b4c8e2a2981f42500894fa2d0 Gerrit-Change-Number: 79769 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-Comment-Date: Thu, 04 Jan 2024 17:57:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: vendorcode/google/chromeos: Use unsigned int for "factory_config"
by Subrata Banik (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Eric Lai, Julius Werner, Kapil Porwal, Shelley Chen, Subrata Banik, Tim Van Patten. Hello Eric Lai, Julius Werner, Kapil Porwal, Shelley Chen, Tim Van Patten, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/79769?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review+2 by Julius Werner, Code-Review+2 by Tim Van Patten, Verified+1 by build bot (Jenkins) Change subject: vendorcode/google/chromeos: Use unsigned int for "factory_config" ...................................................................... vendorcode/google/chromeos: Use unsigned int for "factory_config" This patch ensures `chromeos_get_factory_config()` returns an unsigned integer value because factory config represents bit-fields to determine the Chromebook Plus branding. Additionally, introduced safety measures to catch future "factory_config" bit-field exhaustion. BUG=b:317880956 TEST=Able to verify that google/screebo is branded as Chromebook Plus. Change-Id: I3021b8646de4750b4c8e2a2981f42500894fa2d0 Signed-off-by: Subrata Banik <subratabanik(a)google.com> --- M src/vendorcode/google/chromeos/chromeos.h M src/vendorcode/google/chromeos/tpm_factory_config.c 2 files changed, 15 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/79769/4 -- To view, visit https://review.coreboot.org/c/coreboot/+/79769?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: I3021b8646de4750b4c8e2a2981f42500894fa2d0 Gerrit-Change-Number: 79769 Gerrit-PatchSet: 4 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Shelley Chen <shchen(a)google.com> Gerrit-Attention: Eric Lai <ericllai(a)google.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-MessageType: newpatchset

1 0

[XS] Change in coreboot[main]: [WIP] cpu/x86/smi_trigger: use call_smm
by Felix Held (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Jérémy Compostella. Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79828?usp=email ) Change subject: [WIP] cpu/x86/smi_trigger: use call_smm ...................................................................... [WIP] cpu/x86/smi_trigger: use call_smm Use call_smm instead of writing the command number directly to the APMC SMI command IO port. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: Iefbdb3d17932d6db6a17b5771436ede220c714fb --- M src/cpu/x86/smi_trigger.c 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/79828/1 diff --git a/src/cpu/x86/smi_trigger.c b/src/cpu/x86/smi_trigger.c index 56c7d44..7357dc5 100644 --- a/src/cpu/x86/smi_trigger.c +++ b/src/cpu/x86/smi_trigger.c @@ -38,7 +38,7 @@ apmc_log(__func__, cmd); /* Now raise the SMI. */ - outb(cmd, pm_acpi_smi_cmd_port()); + call_smm(cmd, 0, NULL); printk(BIOS_DEBUG, "APMC done.\n"); return 0; -- To view, visit https://review.coreboot.org/c/coreboot/+/79828?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: Iefbdb3d17932d6db6a17b5771436ede220c714fb Gerrit-Change-Number: 79828 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: newchange

1 0

[XS] Change in coreboot[main]: arch/x86/include/smm: use inline asm from drivers/smmstore/ramstage
by Felix Held (Code Review) 04 Jan '24

04 Jan '24

Attention is currently required from: Jérémy Compostella. Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/79827?usp=email ) Change subject: arch/x86/include/smm: use inline asm from drivers/smmstore/ramstage ...................................................................... arch/x86/include/smm: use inline asm from drivers/smmstore/ramstage call_smm is currently unused and the inline assembly code for more or less the same functionality in drivers/smmstore/ramstage is both easier to understand and also tells the compiler that this piece of code might change global memory. Also having too much in the clobber list might only have some performance impact, which should however be negligible compared to the SMI handler being called, while missing something in the clobber list might cause hard to debug problems. This is a preparation to make drivers/smmstore/ramstage use call_smm instead of having its own inline assembly implementation for this. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I73837cab75429014897486b38a5c56f93a850f96 --- M src/arch/x86/include/smm.h 1 file changed, 5 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/79827/1 diff --git a/src/arch/x86/include/smm.h b/src/arch/x86/include/smm.h index e6db9dc..c0d96c0 100644 --- a/src/arch/x86/include/smm.h +++ b/src/arch/x86/include/smm.h @@ -13,8 +13,11 @@ { u32 res = 0; __asm__ __volatile__ ( - "outb %b0, %3" + "outb %%al, %%dx" : "=a" (res) - : "a" ((subcmd << 8) | cmd), "b" (arg), "i" (APM_CNT)); + : "a" ((subcmd << 8) | cmd), + "b" (arg), + "d" (APM_CNT) + : "memory"); return res; } -- To view, visit https://review.coreboot.org/c/coreboot/+/79827?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: I73837cab75429014897486b38a5c56f93a850f96 Gerrit-Change-Number: 79827 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[main]: drivers/smmstore/ramstage: use call_smm
by Felix Held (Code Review) 04 Jan '24

04 Jan '24

Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/79766?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: drivers/smmstore/ramstage: use call_smm ...................................................................... drivers/smmstore/ramstage: use call_smm Use call_smm instead of open-coding the same in inline assembly functionality in init_store. The local ebx variable is dropped, since call_smm takes a pointer to the argument instead of an integer, and the local eax variable is renamed to res to make the code a bit clearer, since the EAX register is used for both passing the command and subcommand to the APMC SMI handler and to get the return value from the handler. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: Ib14de0d120ae5c7db3bb7a529837ababe653e1a2 --- M src/drivers/smmstore/ramstage.c 1 file changed, 3 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/79766/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/79766?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: Ib14de0d120ae5c7db3bb7a529837ababe653e1a2 Gerrit-Change-Number: 79766 Gerrit-PatchSet: 2 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit January 2024