Attention is currently required from: Jérémy Compostella, Shuo Liu, yuchi.chen(a)intel.com.
Felix Singer has posted comments on this change by yuchi.chen(a)intel.com. ( https://review.coreboot.org/c/coreboot/+/83315?usp=email )
Change subject: src/soc/intel/common/block/include/intelblocks/gpio.h: add new field
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83315/comment/ad5456f1_05bb3363?us… :
PS2, Line 7: src/soc/intel/common/block/include/intelblocks/gpio.h
This prefix is not supposed to be a full path to the file that is changed, but it could be though. However, it's more meant to be a topic so that people get a rough idea what this change is about. Also, we don't add "src" if the changes are limited to that directory, since most changes happen there.
How about:
```
soc/intel/common/intelblocks/gpio.h: Allow specifying the pad ownership
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/83315?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: I30a934fd00a7a42cb156341da1954e4e4b1231d8
Gerrit-Change-Number: 83315
Gerrit-PatchSet: 2
Gerrit-Owner: yuchi.chen(a)intel.com
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: yuchi.chen(a)intel.com
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 10:54:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Jérémy Compostella, Shuo Liu, yuchi.chen(a)intel.com.
Felix Singer has posted comments on this change by yuchi.chen(a)intel.com. ( https://review.coreboot.org/c/coreboot/+/83314?usp=email )
Change subject: add CPU and PCIe definitions for SNR
......................................................................
Patch Set 1:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83314/comment/b27e317e_ee6247a6?us… :
PS1, Line 7: add CPU and PCIe definitions for SNR
Suggestion:
```
soc/intel/common: add CPU and PCIe IDs for Snow Ridge platform
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/83314?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: I3f5d612765bbe9adffe0b6c7a4151f32b33e88b4
Gerrit-Change-Number: 83314
Gerrit-PatchSet: 1
Gerrit-Owner: yuchi.chen(a)intel.com
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: yuchi.chen(a)intel.com
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 10:45:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Elyes Haouas has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/81907?usp=email )
Change subject: util/lint: Add lint rule to avoid duplicated includes
......................................................................
Abandoned
--
To view, visit https://review.coreboot.org/c/coreboot/+/81907?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: abandon
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Ia14c73f0bffa6ee4b73254d71836f793af8771c4
Gerrit-Change-Number: 81907
Gerrit-PatchSet: 13
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
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-CC: David Hendricks <david.hendricks(a)gmail.com>
Elyes Haouas has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/82713?usp=email )
Change subject: util/lint: Warn if {stddef,stdint,string}.h included but not used
......................................................................
Abandoned
--
To view, visit https://review.coreboot.org/c/coreboot/+/82713?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: abandon
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I94104df7a2e25c0197b4027eb7fddd52cbc4c6ad
Gerrit-Change-Number: 82713
Gerrit-PatchSet: 3
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.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: Jérémy Compostella, Shuo Liu, yuchi.chen(a)intel.com.
Felix Singer has posted comments on this change by yuchi.chen(a)intel.com. ( https://review.coreboot.org/c/coreboot/+/83322?usp=email )
Change subject: src/mainboard/intel/frost_creek: add a new CRB Frost Creek for SNR
......................................................................
Patch Set 3:
(1 comment)
File src/mainboard/intel/frost_creek/COPYING-NOTICE:
PS3:
This probably doesn't belong here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83322?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: If3b387a6a4a567415aef21e520056c23b8cfa013
Gerrit-Change-Number: 83322
Gerrit-PatchSet: 3
Gerrit-Owner: yuchi.chen(a)intel.com
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: yuchi.chen(a)intel.com
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Comment-Date: Wed, 03 Jul 2024 10:42:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Arthur Heymans, Elyes Haouas, Julius Werner, Martin L Roth, Nico Huber.
Angel Pons has posted comments on this change by Elyes Haouas. ( https://review.coreboot.org/c/coreboot/+/81907?usp=email )
Change subject: util/lint: Add lint rule to avoid duplicated includes
......................................................................
Patch Set 13: Code-Review-1
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81907/comment/137c3af1_9ce3e097?us… :
PS13, Line 7: util/lint: Add lint rule to avoid duplicated includes
If I assume the commit message is correct, I would expect a lint rule for *duplicated* includes to check if the same file is included multiple times within a single file. But the warning message is completely different:
> echo "Warning: $expected_file is supposed to provide $pattern"
Please accurately describe what this commit does. It does not seem to check for duplicated includes in the way I initially believed it would.
File util/lint/lint-stable-031-duplicated-includes:
https://review.coreboot.org/c/coreboot/+/81907/comment/1a9b9a8c_dc5e1582?us… :
PS13, Line 48: check_duplicates '<commonlib/bsd/cb_err.h>\|<limits.h>\|<stdbool.h>\|<stdint.h>\|<stddef.h>' '<types.h>'
This is enforcing code style. And not everything can use `types.h` (e.g. utils).
https://review.coreboot.org/c/coreboot/+/81907/comment/db68ff38_41eed89b?us… :
PS13, Line 46: check_duplicates '<device/\(path\|resource\).h>' '<device/device.h>'
: check_duplicates '<device/pci_\(def\|type\).h>' '<device/pci.h>'
: check_duplicates '<commonlib/bsd/cb_err.h>\|<limits.h>\|<stdbool.h>\|<stdint.h>\|<stddef.h>' '<types.h>'
: check_duplicates '<arch/cpuid.h>' '<arch/cpu.h>'
: check_duplicates '<commonlib/bsd/cbmem_id.h>' '<cbmem.h>'
: check_duplicates '<commonlib/loglevel.h>' '<console/console.h>'
: check_duplicates '<arch/cpu.h>' '<cpu/cpu.h>'
: check_duplicates '<cpu/x86/msr_access.h>' '<cpu/x86/msr.h>'
: check_duplicates '<arch/mmio.h>' '<device/mmio.h>'
: check_duplicates '<arch/pci_ops.h>' '<device/pci_ops.h>'
: check_duplicates '<soc/gpio.h>' '<gpio.h>'
: check_duplicates '<commonlib/bsd/cbfs_serialized.h>' '<cbfs.h>'
There do not seem to be any facilities to exclude false positives for the files themselves considered by the first argument.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81907?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: Ia14c73f0bffa6ee4b73254d71836f793af8771c4
Gerrit-Change-Number: 81907
Gerrit-PatchSet: 13
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
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-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Wed, 03 Jul 2024 09:44:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Attention is currently required from: Elyes Haouas, Martin L Roth.
Angel Pons has posted comments on this change by Elyes Haouas. ( https://review.coreboot.org/c/coreboot/+/82713?usp=email )
Change subject: util/lint: Warn if {stddef,stdint,string}.h included but not used
......................................................................
Patch Set 3: Code-Review-1
(2 comments)
Patchset:
PS3:
I fail to see why this change would be worth it.
This assumes that indirect header inclusion is never a thing. However, there is at least one header that indirectly includes some of the headers here. Maintaining a list of excluded headers would only add to the maintenance burden.
IMHO, the maintenance burden alone outweighs any benefits of automatically detecting unnecessary includes of a few headers. coreboot compilation times are relatively short (especially with ccache).
The few cases where I've seen coreboot rebuild times go up noticeably (still being well below 30 seconds on my machine) is when changing stuff that impacts the devicetree on newer Intel platforms (nearly all of the SoC code needs to be recompiled) or headers that get included from many places (but that's kind of expected).
File util/lint/lint-stable-032-includes:
https://review.coreboot.org/c/coreboot/+/82713/comment/5147e2b8_8035a6d5?us… :
PS3, Line 19: HEADERS["<stddef.h>"]="size_t\b\|ptrdiff_t\b\|NULL\b\|offsetof\b\|wchar_t\b"
: HEADERS["<stdint.h>"]="int8_t\b\|int16_t\b\|int32_t\b\|int64_t\b\|uint8_t\b\|uint16_t\b\|uint32_t\b\|uint64_t\b\|intptr_t\b\|uintptr_t\b\|u8\b\|u16\b\|u32\b\|u64\b\|INT8_MIN\b\|INT16_MIN\b\|INT32_MIN\b\|INT64_MIN\b\|UINT8_MAX\b\|UINT16_MAX\b\|UINT32_MAX\b\|UINT64_MAX"
: HEADERS["<string.h>"]="memcpy\b\|memmove\b\|memset\b\|memcmp\b\|memchr\b\|strdup\b\|strconcat\b\|strnlen\b\|strlen\b\|strchr\b\|strncpy\b\|strcpy\b\|strcmp\b\|strncmp\b\|strspn\b\|strcspn\b\|strstr\b\|strtok_r\b\|strtok\b\|atol\b\|strrchr\b\|skip_atoi"
IMHO, having to maintain these lists manually is wasteful. It's true that they don't change often, but any missing elements from this list would produce a false positive.
And if there's multiple versions of these headers with the same name (e.g. coreboot's `string.h`, vs. the host's `string.h` used in `util/`), this can result in irreconcilable misdetections (either false positives or false negatives).
--
To view, visit https://review.coreboot.org/c/coreboot/+/82713?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: I94104df7a2e25c0197b4027eb7fddd52cbc4c6ad
Gerrit-Change-Number: 82713
Gerrit-PatchSet: 3
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.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: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Wed, 03 Jul 2024 09:33:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes