Attention is currently required from: Nicholas Chin.
Nico Huber has posted comments on this change by Nicholas Chin. ( https://review.coreboot.org/c/coreboot/+/83977?usp=email )
Change subject: Doc/mb/starlabs: Rename starlite_adl.md to lite_adl.md
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/83977?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: I1922940fd18cc806d9647cbe05ad11b2a70e0d08
Gerrit-Change-Number: 83977
Gerrit-PatchSet: 1
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 13:52:25 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Angel Pons, Benjamin Doron, David Milosevic, Patrick Rudolph.
Arthur Heymans has posted comments on this change by David Milosevic. ( https://review.coreboot.org/c/coreboot/+/79108?usp=email )
Change subject: mb/emulation/qemu-sbsa: Generate PPTT ACPI table
......................................................................
Patch Set 7:
(1 comment)
File src/mainboard/emulation/qemu-sbsa/pptt.c:
https://review.coreboot.org/c/coreboot/+/79108/comment/10c3d559_9110590a?us… :
PS7, Line 19: static struct pptt_cache l2 = {
> Why is this static? For lifetime reasons, I imagine?
>
> If so: I'm not too fond of this. I'm not sure if there are any drawbacks but I'd consider having these as static globals outside the function instead.
Yes for lifetime purposes.
Why increase the scope? Do you expect some other function inside this file to access them?
--
To view, visit https://review.coreboot.org/c/coreboot/+/79108?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: Iabeb138c1257488bcb364fe7b522f02c041745e2
Gerrit-Change-Number: 79108
Gerrit-PatchSet: 7
Gerrit-Owner: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 13:45:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Attention is currently required from: Arthur Heymans, Benjamin Doron, David Milosevic, Patrick Rudolph.
Angel Pons has posted comments on this change by David Milosevic. ( https://review.coreboot.org/c/coreboot/+/79108?usp=email )
Change subject: mb/emulation/qemu-sbsa: Generate PPTT ACPI table
......................................................................
Patch Set 7:
(4 comments)
File src/mainboard/emulation/qemu-sbsa/pptt.c:
https://review.coreboot.org/c/coreboot/+/79108/comment/d1e28884_f1e8f2b0?us… :
PS7, Line 9: #define CACHE_NODE_FLAGS 0xd7 // everything valid except, write-policy and allocation type
: #define CLUSTER_FLAGS 0x11 // physical package, ID invalid, no thread, no leaf, identical impl.
: #define CORE_FLAGS 0x1a // no physical package, ID valid, no thread, leaf, identical impl.
:
: #define CACHE_ATTR_TYPE_DATA (0)
: #define CACHE_ATTR_TYPE_INSTRUCTION (0x1 << 2)
: #define CACHE_ATTR_TYPE_UNIFIED (0x1 << 3)
> > Done […]
+1, please use the structs/bitfields if possible. If bitfields cannot possibly work properly because of implementation defined behaviour, then the bitfields are misleading and should not exist (or be fixed somehow, if possible)
https://review.coreboot.org/c/coreboot/+/79108/comment/3aaf8402_c4ab68fb?us… :
PS7, Line 19: static struct pptt_cache l2 = {
Why is this static? For lifetime reasons, I imagine?
If so: I'm not too fond of this. I'm not sure if there are any drawbacks but I'd consider having these as static globals outside the function instead.
https://review.coreboot.org/c/coreboot/+/79108/comment/2e690bb3_0952e04b?us… :
PS7, Line 47: static struct pptt_topology root_topology = {
: .flags.raw = CLUSTER_FLAGS,
: .resources = NULL,
: .sibling = NULL,
: .child = NULL // updated in runtime
: };
You can move this below `static struct pptt_topology entries[CONFIG_MAX_CPUS] = {};` and directly set `.child = entries,` in the struct initialiser.
https://review.coreboot.org/c/coreboot/+/79108/comment/182af4bf_33f93b8d?us… :
PS7, Line 54: struct cache_info info;
:
: /* update cache information (L1I) */
:
: cpu_get_cache_info(CACHE_L1, CACHE_INSTRUCTION, &info);
:
: l1i.size = info.size;
: l1i.associativity = info.associativity;
: l1i.numsets = info.numsets;
: l1i.line_size = info.line_bytes;
Would it make sense to encapsulate this in a function?
```
static void fill_pptt_cache_info(const enum cache_level level, const enum cache_type type, struct pptt_cache *cache)
{
struct cache_info info;
cpu_get_cache_info(level, type, &info);
cache->size = info.size;
cache->associativity = info.associativity;
cache->numsets = info.numsets;
cache->line_size = info.line_bytes;
}
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/79108?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: Iabeb138c1257488bcb364fe7b522f02c041745e2
Gerrit-Change-Number: 79108
Gerrit-PatchSet: 7
Gerrit-Owner: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: David Milosevic <David.Milosevic(a)9elements.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 13:35:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: David Milosevic <David.Milosevic(a)9elements.com>
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/83933?usp=email )
Change subject: commonlib/bsd: Optimize strnlen()
......................................................................
commonlib/bsd: Optimize strnlen()
This patch changes the strnlen() implementation to fix a small issue
where we would dereference once more byte than intended when not finding
a NUL-byte within the specified amount of characters. It also changes
the implementation to rely on a pre-calculated end pointer rather than a
running counter, since this seems to lead to slightly better assembly
(one less instruction in the inner loop) on most architectures.
Change-Id: Ic36768fd3a26e2b64143904e78cd0b52ba66898d
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/83933
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Yu-Ping Wu <yupingso(a)google.com>
---
M src/commonlib/bsd/string.c
1 file changed, 13 insertions(+), 4 deletions(-)
Approvals:
build bot (Jenkins): Verified
Yu-Ping Wu: Looks good to me, approved
diff --git a/src/commonlib/bsd/string.c b/src/commonlib/bsd/string.c
index 56670e8..6482279 100644
--- a/src/commonlib/bsd/string.c
+++ b/src/commonlib/bsd/string.c
@@ -15,10 +15,19 @@
size_t strnlen(const char *str, size_t maxlen)
{
- size_t len = 0;
- while (*str++ && len < maxlen)
- len++;
- return len;
+ const char *ptr = str;
+ const char *end = str + maxlen;
+
+ if (!maxlen)
+ return 0;
+
+ while (*ptr++) {
+ /* Make sure this checks for ==, not >=, because the calculation
+ for `end` may overflow in some edge cases. */
+ if (ptr == end)
+ return maxlen;
+ }
+ return ptr - str - 1;
}
char *strcat(char *dst, const char *src)
--
To view, visit https://review.coreboot.org/c/coreboot/+/83933?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: Ic36768fd3a26e2b64143904e78cd0b52ba66898d
Gerrit-Change-Number: 83933
Gerrit-PatchSet: 3
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Arthur Heymans, Martin Roth.
Felix Singer has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/83973?usp=email )
Change subject: toolchain: Add support for sccache
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
Interesting. Thanks for bringing it up! I will look at it.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83973?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: Ia28e696dfe9eab0fc73ba8c7c6bdfc90cbdb790e
Gerrit-Change-Number: 83973
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-CC: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Comment-Date: Mon, 19 Aug 2024 13:24:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/83792?usp=email )
(
6 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: mb/google/nissa/var/sundance: Adjust WWAN GPIO sequence
......................................................................
mb/google/nissa/var/sundance: Adjust WWAN GPIO sequence
This patch removes WWAN configuration from the bootblock.
It appears that setting it up in the bootblock may not be necessary.
Configure in bootblock,the seq will be triggered at the same time.
The customer would like us to leave some buffer for EN to RST.
BUG=b:357764679
TEST=Build and verified test result by EE team
Change-Id: I2c0e789c0bec293f4bca711e53644d62f4f83551
Signed-off-by: Roger Wang <roger2.wang(a)lcfc.corp-partner.google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/83792
Reviewed-by: Subrata Banik <subratabanik(a)google.com>
Reviewed-by: Eric Lai <ericllai(a)google.com>
Reviewed-by: Dinesh Gehlot <digehlot(a)google.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M src/mainboard/google/brya/variants/sundance/gpio.c
1 file changed, 0 insertions(+), 2 deletions(-)
Approvals:
Subrata Banik: Looks good to me, approved
Dinesh Gehlot: Looks good to me, approved
build bot (Jenkins): Verified
Eric Lai: Looks good to me, approved
diff --git a/src/mainboard/google/brya/variants/sundance/gpio.c b/src/mainboard/google/brya/variants/sundance/gpio.c
index bd36f05..107fa77 100644
--- a/src/mainboard/google/brya/variants/sundance/gpio.c
+++ b/src/mainboard/google/brya/variants/sundance/gpio.c
@@ -66,8 +66,6 @@
PAD_CFG_GPO(GPP_D6, 0, DEEP),
/* E12 : THC0_SPI1_IO1 ==> SOC_WP_OD */
PAD_CFG_GPI_GPIO_DRIVER(GPP_E12, NONE, DEEP),
- /* F12 : WWAN_RST_L */
- PAD_CFG_GPO(GPP_F12, 0, DEEP),
/* F18 : THC1_SPI2_INT# ==> EC_IN_RW_OD */
PAD_CFG_GPI(GPP_F18, NONE, DEEP),
/* H4 : I2C0_SDA ==> SOC_I2C_GSC_SDA */
--
To view, visit https://review.coreboot.org/c/coreboot/+/83792?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: I2c0e789c0bec293f4bca711e53644d62f4f83551
Gerrit-Change-Number: 83792
Gerrit-PatchSet: 8
Gerrit-Owner: Roger Wang <roger2.wang(a)lcfc.corp-partner.google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Reviewer: Shou-Chieh Hsu <shouchieh(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Leo Chou <leo.chou(a)lcfc.corp-partner.google.com>
Gerrit-CC: Melo Chuang <melo.chuang(a)lcfc.corp-partner.google.com>