Attention is currently required from: Alexander Couzens, Anastasios Koutian, Angel Pons.
Nico Huber has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83280?usp=email )
Change subject: mb/lenovo/t420: Use vendor default power limits
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
Do we know if Lenovo always used the same numbers? i.e. irrespective
of the installed CPU model?
--
To view, visit https://review.coreboot.org/c/coreboot/+/83280?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: Ia187b67ae28fbcda7d5d0e35ec64a3b21d97a21b
Gerrit-Change-Number: 83280
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com>
Gerrit-Reviewer: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Anastasios Koutian <akoutian2(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alexander Couzens <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Fri, 05 Jul 2024 15:17:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Anastasios Koutian, Angel Pons.
Nico Huber has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83271?usp=email )
Change subject: cpu/intel/model_206ax: Allow turbo boost ratio limit configuration
......................................................................
Patch Set 5: Code-Review+1
(3 comments)
File src/cpu/intel/model_206ax/chip.h:
https://review.coreboot.org/c/coreboot/+/83271/comment/4e1a817f_e1408818?us… :
PS5, Line 63: int turbo_ratio_limit_4c; /* Turbo Ratio Limit for 4 cores acrive */
An array might look better?
File src/cpu/intel/model_206ax/model_206ax.h:
https://review.coreboot.org/c/coreboot/+/83271/comment/101ab793_7f0894e0?us… :
PS5, Line 49:
The tabs are spurious at this point, please remove.
Alternatively, how about shortening the name? e.g.
`PLATFORM_INFO_SET_TURBO_LIMITS` seems to fit.
File src/cpu/intel/model_206ax/model_206ax_init.c:
https://review.coreboot.org/c/coreboot/+/83271/comment/0bd80844_233a284d?us… :
PS5, Line 333: BIOS_NOTICE
It's expected to be like this on most processors, isn't it? Then it should
be `BIOS_INFO` at most.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83271?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: I1c65a129478e8ac2c4f66eb3c6aa2507358f82ad
Gerrit-Change-Number: 83271
Gerrit-PatchSet: 5
Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasios Koutian <akoutian2(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Fri, 05 Jul 2024 15:16:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Nico Huber has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/13044?usp=email )
Change subject: Make Ada a first class citizen
......................................................................
Patch Set 16:
(1 comment)
File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/13044/comment/b9864717_1c68a715?us… :
PS16, Line 362: +=
> isn't `ADAFLAGS_common := -gnatg -gnatp`?
I don't understand the question.
--
To view, visit https://review.coreboot.org/c/coreboot/+/13044?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: master
Gerrit-Change-Id: I70df9adbd467ecd2dc7c5c1cf418b7765aca4e93
Gerrit-Change-Number: 13044
Gerrit-PatchSet: 16
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Edward O'Callaghan <edward.ocallaghan(a)koparo.com>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Georgi
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Fri, 05 Jul 2024 14:51:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Elyes Haouas <ehaouas(a)noos.fr>
Attention is currently required from: Felix Singer, Maciej Pijanowski, Michał Żygowski, Piotr Król.
Nico Huber has posted comments on this change by Michał Żygowski. ( https://review.coreboot.org/c/coreboot/+/80501?usp=email )
Change subject: mb/protectli/vault_adl_p: Add initial support for VP6630/VP6650/VP6670
......................................................................
Patch Set 8: Code-Review+1
(2 comments)
File src/mainboard/protectli/vault_adl_p/bootblock.c:
https://review.coreboot.org/c/coreboot/+/80501/comment/3f791baa_3ca990e8?us… :
PS7, Line 41: polarity << pin
> Prepared a patch with a generic ITE SIo GPIO driver: CB:83355
Acknowledged
File src/mainboard/protectli/vault_adl_p/mainboard.c:
https://review.coreboot.org/c/coreboot/+/80501/comment/1a3f907b_c1febd63?us… :
PS7, Line 29: }
> Exported it to a header file as static inline function
Why inline, though? We can add as many C files as we want. Inline only bloats
the binary.
--
To view, visit https://review.coreboot.org/c/coreboot/+/80501?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: If4ae5b14b69806b6b0727d1ca1dcf56f47cfcd8e
Gerrit-Change-Number: 80501
Gerrit-PatchSet: 8
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.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: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com>
Gerrit-Attention: Piotr Król <piotr.krol(a)3mdeb.com>
Gerrit-Comment-Date: Fri, 05 Jul 2024 14:50:38 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Attention is currently required from: Dinesh Gehlot, Kapil Porwal, Michał Żygowski, Nick Vaccaro, Subrata Banik.
Nico Huber has posted comments on this change by Michał Żygowski. ( https://review.coreboot.org/c/coreboot/+/83356?usp=email )
Change subject: soc/intel/alderlake/tcss: Add definition of IOM_READY bit
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
Any idea where this is documented? I wonder why nobody else ran into this yet.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83356?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: If868a77852468ebb73526b1571191cbdeb1804b9
Gerrit-Change-Number: 83356
Gerrit-PatchSet: 1
Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Comment-Date: Fri, 05 Jul 2024 14:38:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
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>