Attention is currently required from: Michał Żygowski.
Nico Huber 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: Code-Review+1
(9 comments)
Patchset:
PS1:
Thanks, I know it's a lot of work, much appreciated!
I don't have all the datasheets available, in particular nothing with
GPIO sets > 8 or the 5-bit frequency selection. Otherwise, it looks
pretty good, given the number of datasheets and oddities.
Commit Message:
https://review.coreboot.org/c/coreboot/+/83355/comment/0bcd8a85_5d7aa388?us… :
PS1, Line 22: - Refactors the mainboards' code to use the new driver where possible
When a patch has hundreds of lines and a list of things it does
that's usually a sign that it could be done easier in smaller
patches. In this case the last two points would have to be in a
single commit, but the others don't.
I'm fine reviewing this one at once, but please keep an eye on
such cases in the future. There's also a chance to get important
parts merged more quickly. For instance, updating the IT8772F
code is a very nice bonus. OTOH, should reviewing if that is
regression free hold up a new board port?
File src/mainboard/google/beltino/variants/mccloud/led.c:
https://review.coreboot.org/c/coreboot/+/83355/comment/f3f443f1_89d4c11e?us… :
PS1, Line 18: ITE_LED_SHORT_PULSE_DISABLE,
: ITE_LED_PINMAP_CLEAR_DISABLE,
: ITE_LED_OUTPUT_LOW_DISABLE,
: ITE_LED_FREQ_1HZ);
Random thought: Making the enum values masks (i.e. with the 1 shifted already)
would allow a single parameter. A caller that wants to set more of the bits
wouldn't look much different, e.g.
```
ite_gpio_setup_led(IT8772F_GPIO_DEV, 10, ITE_GPIO_LED_1,
ITE_LED_SHORT_PULSE |
ITE_OUTPUT_LOW |
ITE_LED_FREQ_1HZ);
```
And callers that don't want to enable a feature can omit it, i.e. this call
here could just be
```
ite_gpio_setup_led(IT8772F_GPIO_DEV, 10, ITE_GPIO_LED_1, ITE_LED_FREQ_1HZ);
```
I think it might also reduce some of the boilerplate in the implementation.
And things like the `do we have pinmap-clear?' could be implemented by masking
the respective bit.
File src/mainboard/samsung/stumpy/smihandler.c:
https://review.coreboot.org/c/coreboot/+/83355/comment/01ec0b47_f5b36cd5?us… :
PS1, Line 25: ite_reg_write(GPIO_DEV, IT8772F_GPIO_REG_SELECT(3), 0x20);
Now that these are seperate steps, the redundancy becomes obvious: We already
set this during boot. So we could drop these mux settings here.
https://review.coreboot.org/c/coreboot/+/83355/comment/493eafa2_0d0f7bee?us… :
PS1, Line 39: ite_reg_write(GPIO_DEV, IT8772F_GPIO_REG_SELECT(3), 0x20);
Same.
https://review.coreboot.org/c/coreboot/+/83355/comment/dd103b1d_3567b53f?us… :
PS1, Line 49: ITE_LED_FREQ_1HZ);
This looks very odd now, configuring blink when it's not used? Traced
this to CB:12797. We could just drop this call.
File src/superio/ite/common/gpio.c:
https://review.coreboot.org/c/coreboot/+/83355/comment/6a5be451_d985c222?us… :
PS1, Line 130: */
It looks like IT8718F doesn't have pull-up set 2?
Same for IT8720F, plus sets 7, 8?
IT8783EF not set 6?
File src/superio/ite/it8772f/it8772f.h:
https://review.coreboot.org/c/coreboot/+/83355/comment/3c303899_49dc5dd0?us… :
PS1, Line 16:
: /* GPIO Polarity Select: 1: Inverting, 0: Non-inverting */
: #define IT8772F_GPIO_REG_POLARITY(x) (0xb0 + (x))
:
: /* GPIO Internal Pull-up: 1: Enable, 0: Disable */
: #define IT8772F_GPIO_REG_PULLUP(x) (0xb8 + (x))
:
: /* GPIO Function Select: 1: Simple I/O, 0: Alternate function */
: #define IT8772F_GPIO_REG_ENABLE(x) (0xc0 + (x))
:
: /* GPIO Mode: 0: input mode, 1: output mode */
: #define IT8772F_GPIO_REG_OUTPUT(x) (0xc8 + (x))
:
Why keep them?
File src/superio/ite/it8786e/Kconfig:
https://review.coreboot.org/c/coreboot/+/83355/comment/27bab9c1_0368f9c7?us… :
PS1, Line 16: default 10
I only see 8?
--
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: Nico Huber <nico.h(a)gmx.de>
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 13:32:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/83286?usp=email
to look at the new patch set (#2).
Change subject: autoport: Factor out GPIO config generation
......................................................................
autoport: Factor out GPIO config generation
Intel chipsets from ICH7 through Lynxpoint use the same GPIO register
format and thus mainboards using using these platforms have similar
gpio.c files. Factor out the code to generate gpio.c from bd82x6x.go so
that it other chipsets added to autoport can use it.
This was originally written by Iru Cai in his Haswell autoport patch in
CB:30890; I have simply split out the code to a separate commit as it is
a separate logical change.
TEST=Generated output is identical before and after this patch when run
against logs from a Dell Latitude E6430
Change-Id: If1f506f6ad10144bd6acc42505592426bb7193b7
Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com>
---
M util/autoport/bd82x6x.go
A util/autoport/gpio_common.go
2 files changed, 115 insertions(+), 110 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/83286/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83286?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: If1f506f6ad10144bd6acc42505592426bb7193b7
Gerrit-Change-Number: 83286
Gerrit-PatchSet: 2
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
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>