Attention is currently required from: Felix Singer, Martin L Roth, Nico Huber.
Elyes Haouas has posted comments on this change by Felix Singer. ( https://review.coreboot.org/c/coreboot/+/83347?usp=email )
Change subject: xcompile: Add -Wextra with temporary exceptions
......................................................................
Patch Set 5: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/83347?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I60915cb66581dc2c9b6807335fd0e214b45e76d6
Gerrit-Change-Number: 83347
Gerrit-PatchSet: 5
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Comment-Date: Fri, 05 Jul 2024 13:14:55 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Bharath N, Julius Werner, Paul Menzel, Shelley Chen.
Felix Singer has posted comments on this change by Bharath N. ( https://review.coreboot.org/c/qc_blobs/+/83305?usp=email )
Change subject: sc7180/qtiseclib: Update qtiseclib blobs binaries and release notes from 69 to 71
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/qc_blobs/+/83305/comment/f3f9c9c7_d37daeaa?us… :
PS2, Line 7: blobs binaries
I would remove "binaries". Then it fits into the line
--
To view, visit https://review.coreboot.org/c/qc_blobs/+/83305?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: qc_blobs
Gerrit-Branch: main
Gerrit-Change-Id: Ia390035cdd591c1d31fd2e28ad53e63d16e91a37
Gerrit-Change-Number: 83305
Gerrit-PatchSet: 2
Gerrit-Owner: Bharath N <quic_bharn(a)quicinc.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: mturney mturney <quic_mturney(a)quicinc.com>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Bharath N <quic_bharn(a)quicinc.com>
Gerrit-Comment-Date: Fri, 05 Jul 2024 13:03:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Felix Singer has submitted this change. ( https://review.coreboot.org/c/coreboot/+/83081?usp=email )
Change subject: payloads/external: don't prevent parallel build of iPXE
......................................................................
payloads/external: don't prevent parallel build of iPXE
When starting a nested instance Make communicates information on the
number of jobs and how to synchronize difference instances via MAKEFLAGS
variable. Explicitly overwriting it when invoking
payloads/external/iPXE/Makefile ends up forcing serial build of iPXE.
iPXE builds hundreds of files and its dependency generation is done
separately from compilation making the whole process take couple minutes
on a single CPU (which becomes several seconds if large enough number of
CPUs is available).
iPXE seems to have Make-based build system that has no problems with
parallel build and not utilizing that effectively turns it into a
bottleneck when building a coreboot image in parallel.
It's unclear whether MAKEFLAGS= was even added for any particular
purpose. It doesn't prevent child instances from using variables of
parents, nor it prevents child instance from running in parallel
(because it's still passed as an environment variable that's processed
prior of variable assignments on command-line), but it does prevent
grandchild instance from running in parallel (actual iPXE's Makefile).
MFLAGS contains flags from MAKEFLAGS and isn't used implicitly by Make,
so no need to clear it either because iPXE doesn't use it.
Change-Id: Iac00e2f86d160793d3217e00ddc5012202b3196a
Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/83081
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Reviewed-by: Krystian Hebel <krystian.hebel(a)3mdeb.com>
---
M payloads/external/Makefile.mk
1 file changed, 1 insertion(+), 2 deletions(-)
Approvals:
build bot (Jenkins): Verified
Felix Singer: Looks good to me, approved
Krystian Hebel: Looks good to me, approved
diff --git a/payloads/external/Makefile.mk b/payloads/external/Makefile.mk
index 4a17791..73b600b 100644
--- a/payloads/external/Makefile.mk
+++ b/payloads/external/Makefile.mk
@@ -381,8 +381,7 @@
CONFIG_HAS_SCRIPT=$(CONFIG_IPXE_ADD_SCRIPT) \
CONFIG_IPXE_NO_PROMPT=$(CONFIG_IPXE_NO_PROMPT) \
CONFIG_IPXE_HAS_HTTPS=$(CONFIG_IPXE_HAS_HTTPS) \
- CONFIG_PXE_TRUST_CMD=$(CONFIG_PXE_TRUST_CMD) \
- MFLAGS= MAKEFLAGS=
+ CONFIG_PXE_TRUST_CMD=$(CONFIG_PXE_TRUST_CMD)
# LinuxBoot
LINUXBOOT_CROSS_COMPILE_ARCH-$(CONFIG_LINUXBOOT_X86) = x86_32
--
To view, visit https://review.coreboot.org/c/coreboot/+/83081?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iac00e2f86d160793d3217e00ddc5012202b3196a
Gerrit-Change-Number: 83081
Gerrit-PatchSet: 2
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Martin L Roth, Sergii Dmytruk.
Felix Singer has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/coreboot/+/83081?usp=email )
Change subject: payloads/external: don't prevent parallel build of iPXE
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83081?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iac00e2f86d160793d3217e00ddc5012202b3196a
Gerrit-Change-Number: 83081
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 05 Jul 2024 12:55:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Martin L Roth, Sergii Dmytruk.
Krystian Hebel has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/coreboot/+/83081?usp=email )
Change subject: payloads/external: don't prevent parallel build of iPXE
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83081?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Iac00e2f86d160793d3217e00ddc5012202b3196a
Gerrit-Change-Number: 83081
Gerrit-PatchSet: 1
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Krystian Hebel <krystian.hebel(a)3mdeb.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 05 Jul 2024 12:34:14 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Elyes Haouas, Martin L Roth.
Hello Martin L Roth, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83347?usp=email
to look at the new patch set (#5).
Change subject: xcompile: Add -Wextra with temporary exceptions
......................................................................
xcompile: Add -Wextra with temporary exceptions
In order to detect more issues in our code, make GCC more picky by
enabling -Wextra. Disable a couple of warnings turned on by -Wextra
temporarily in order to keep everything compiling and working for now.
The warnings may be enabled step by step later.
Since xcompiles applies to coreboot and libpayload, add Wextra here
instead of the top-level Makefile.mk.
Change-Id: I60915cb66581dc2c9b6807335fd0e214b45e76d6
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M util/xcompile/xcompile
1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/83347/5
--
To view, visit https://review.coreboot.org/c/coreboot/+/83347?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I60915cb66581dc2c9b6807335fd0e214b45e76d6
Gerrit-Change-Number: 83347
Gerrit-PatchSet: 5
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Attention is currently required from: Elyes Haouas, Martin L Roth.
Hello Martin L Roth, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83347?usp=email
to look at the new patch set (#4).
Change subject: xcompile: Add -Wextra with temporary exceptions
......................................................................
xcompile: Add -Wextra with temporary exceptions
In order to detect more issues in our code, make GCC more picky by
enabling -Wextra when GCC is used. Disable a couple of warnings turned
on by -Wextra temporarily in order to keep everything compiling and
working for now. The warnings may be enabled step by step later.
Since xcompiles applies to coreboot and libpayload, add Wextra here
instead of the top-level Makefile.mk.
Change-Id: I60915cb66581dc2c9b6807335fd0e214b45e76d6
Signed-off-by: Felix Singer <felixsinger(a)posteo.net>
---
M util/xcompile/xcompile
1 file changed, 33 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/83347/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/83347?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I60915cb66581dc2c9b6807335fd0e214b45e76d6
Gerrit-Change-Number: 83347
Gerrit-PatchSet: 4
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Attention is currently required from: Elyes Haouas, Martin L Roth.
Felix Singer has posted comments on this change by Felix Singer. ( https://review.coreboot.org/c/coreboot/+/83347?usp=email )
Change subject: xcompile: Add -Wextra with temporary exceptions
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
> I would have done the opposite.
> "Wextra" may evolve with new options which may generate compilation errors in the future.
I wouldn't worry about new warnings in Wextra. We test new toolchains long enough before we introduce them. So we can just disable or even fix new warnings beforehand.
> On the other hand, activating the options individually seems more appropriate to me.
> Once you have covered all of the "Wextra" options, you can activate it.
The advantage here is that all other options from Wextra are already active, which wouldn't be the case with enabling each option separately. So only some problematic ones are disabled and we know exactly which are failing.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83347?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I60915cb66581dc2c9b6807335fd0e214b45e76d6
Gerrit-Change-Number: 83347
Gerrit-PatchSet: 3
Gerrit-Owner: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Fri, 05 Jul 2024 12:14:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr>
Attention is currently required from: Michał Żygowski.
Nicholas Sudsgaard has posted comments on this change by Michał Żygowski. ( https://review.coreboot.org/c/coreboot/+/83355?usp=email )
Change subject: superio/ite/common: Add common driver for GPIO and LED configuration
......................................................................
Patch Set 1:
(3 comments)
File src/superio/ite/common/gpio.c:
https://review.coreboot.org/c/coreboot/+/83355/comment/ea72be36_1657117d?us… :
PS1, Line 16: This ITE SIO GPIO driver support up to 10 GPIO sets
"ITE SIO GPIO drivers only support up to 10 GPIO sets" sounds more natural as an error message.
File src/superio/ite/common/ite_gpio.h:
https://review.coreboot.org/c/coreboot/+/83355/comment/677e4c21_f2491b8b?us… :
PS1, Line 15: ITE_GPIO_PULLUP_DIS,
: ITE_GPIO_PULLUP_EN
To keep the naming consistent with the other enums.
```suggestion
ITE_GPIO_PULLUP_DISABLE,
ITE_GPIO_PULLUP_ENABLE
```
https://review.coreboot.org/c/coreboot/+/83355/comment/3f2300ff_47864a97?us… :
PS1, Line 63: ITE_LED_FREQ_2Hz_DUTY_50 = 3,
: ITE_LED_FREQ_0P25HZ_DUTY_25 = 4,
: ITE_LED_FREQ_0P25HZ_DUTY_75 = 5,
: ITE_LED_FREQ_0P125HZ_DUTY_25 = 6,
: ITE_LED_FREQ_0P125Hz_DUTY_75 = 7,
: ITE_LED_FREQ_0P4Hz_DUTY_20 = 8,
: ITE_LED_FREQ_0P5Hz_DUTY_50 = 16,
: ITE_LED_FREQ_0P125Hz_DUTY_50 = 24,
Is there a reason the 'z' is not capitalized for some of these enums?
```suggestion
ITE_LED_FREQ_2HZ_DUTY_50 = 3,
ITE_LED_FREQ_0P25HZ_DUTY_25 = 4,
ITE_LED_FREQ_0P25HZ_DUTY_75 = 5,
ITE_LED_FREQ_0P125HZ_DUTY_25 = 6,
ITE_LED_FREQ_0P125HZ_DUTY_75 = 7,
ITE_LED_FREQ_0P4HZ_DUTY_20 = 8,
ITE_LED_FREQ_0P5HZ_DUTY_50 = 16,
ITE_LED_FREQ_0P125HZ_DUTY_50 = 24,
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/83355?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: If610d2809b56c63444c3406c26fad412c94136a5
Gerrit-Change-Number: 83355
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Nicholas Sudsgaard <devel+coreboot(a)nsudsgaard.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 05 Jul 2024 11:48:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No