Attention is currently required from: Hung-Te Lin, Paul Menzel, Ruihai Zhou, Xuxin Xiong, Yang Wu, Yidi Lin.
Eric Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81584?usp=email )
Change subject: mb/google/corsola: Add new board variant Wugtrio
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/81584?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I84cb86bf4eb2762b3072cb030b3c34a08c6b869d
Gerrit-Change-Number: 81584
Gerrit-PatchSet: 7
Gerrit-Owner: Yang Wu <wuyang5(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Ruihai Zhou <zhouruihai(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Xuxin Xiong <xuxinxiong(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Ruihai Zhou <zhouruihai(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Xuxin Xiong <xuxinxiong(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Yang Wu <wuyang5(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Comment-Date: Tue, 02 Apr 2024 01:53:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Andrey Petrov, Felix Held, Fred Reitberger, Jason Glenesk, Matt DeVillier, Ronak Kanabar, Sean Rhodes, Subrata Banik.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81618?usp=email )
Change subject: lib: Use platform-independent types in bmp_load_logo()
......................................................................
Patch Set 1:
(2 comments)
File src/lib/bmp_logo.c:
https://review.coreboot.org/c/coreboot/+/81618/comment/f173b881_bf26f6e4 :
PS1, Line 19: void bmp_load_logo(uintptr_t *logo_ptr, size_t *logo_size)
Why does this function take two out-pointers and have no return value, anyway? I think that's a very weird design, and it's actually problematic because the function has no way to return errors. Functions like `fsp_convert_bmp_to_gop_blt()` check if `logo_ptr` is `NULL` afterwards to check for error, but that's not actually correct because this function doesn't set it to `NULL` before returning (and `fsp_convert_bmp_to_gop_blt()` didn't initialize the pointer it passes in).
I think it would make much more sense if this function returns a void pointer and only takes the `size_t *logo_size` argument, and if the returned pointer is `NULL` that signals error. If the caller needs a `uintptr_t` (some of them actually don't, e.g. `fsp_convert_bmp_to_gop_blt()` would be better off with a `void *` anyway), it can do the cast itself.
File src/soc/amd/mendocino/fsp_s_params.c:
https://review.coreboot.org/c/coreboot/+/81618/comment/d08b89c0_c9a3d1e2 :
PS1, Line 59: (uintptr_t *)
You can't do these kinds of things. You can't take a pointer to `uint32_t`, cast it to a pointer to `uintptr_t`, and then have another function write into that. If this code is running in 64-bit mode, then `uintptr_t` will be 8 bytes long and writing into the `uint32_t` location you're passing in here will overwrite the next 4 bytes after it.
If you want to do something like this, you'll need to put an intermediate variable of the right size on the stack to pass in, and do a narrowing copy later (same applies to `size_t` vs. `uint32_t`, of course):
```
uintptr_t logo_buffer;
bmp_load_logo(&logo_buffer, ...);
supd->FspsConfig.logo_bmp_buffer = logo_buffer;
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/81618?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I14bc54670a67980ec93bc366b274832d1f959e50
Gerrit-Change-Number: 81618
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Ronak Kanabar <ronak.kanabar(a)intel.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 02 Apr 2024 00:51:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Eric Lai, Hung-Te Lin, Paul Menzel, Ruihai Zhou, Xuxin Xiong, Yidi Lin.
Yang Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81584?usp=email )
Change subject: mb/google/corsola: Add new board variant Wugtrio
......................................................................
Patch Set 7:
(1 comment)
Patchset:
PS6:
> please rebase to ToT
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/81584?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I84cb86bf4eb2762b3072cb030b3c34a08c6b869d
Gerrit-Change-Number: 81584
Gerrit-PatchSet: 7
Gerrit-Owner: Yang Wu <wuyang5(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Ruihai Zhou <zhouruihai(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Xuxin Xiong <xuxinxiong(a)huaqin.corp-partner.google.com>
Gerrit-Reviewer: Yidi Lin <yidilin(a)google.com>
Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Ruihai Zhou <zhouruihai(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Xuxin Xiong <xuxinxiong(a)huaqin.corp-partner.google.com>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Comment-Date: Tue, 02 Apr 2024 00:48:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Paul Menzel, Tarun, Zhuohao Lee.
Tony Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81564?usp=email )
Change subject: mb/google/rex/var/deku: Swap LAN device indices for correct MAC address
......................................................................
Patch Set 5:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81564/comment/bc01ed17_f3aa0a60 :
PS4, Line 7: Swap LAN device indices
> Maybe even: […]
Done
https://review.coreboot.org/c/coreboot/+/81564/comment/b9f95761_60eb3493 :
PS4, Line 9: Deku has 2 ethernet ports, due to the LAN devices indices were swapped.
: So set vpd ethernet_mac0 will affect the eth1 and set vpd ethernet_mac1 will affect the eth0 from ifconfig.
> Maybe use: […]
Done
https://review.coreboot.org/c/coreboot/+/81564/comment/d6923809_0ed408fb :
PS4, Line 12: This CL is to correct the device indices for LAN devices so ethernet_mac[0-1] in vpd can apply to the correct ethernet ports.
> Please use 72 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/81564/comment/8afcb841_7f6902cd :
PS4, Line 12: This CL is to correct
> Correct
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/81564?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Id1508104cbb5cf0a234f34f9db19cc535fdb634b
Gerrit-Change-Number: 81564
Gerrit-PatchSet: 5
Gerrit-Owner: Tony Huang <tony-huang(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Derek Huang <derekhuang(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kevin Chiu <kevin.chiu.17802(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-Comment-Date: Tue, 02 Apr 2024 00:34:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-MessageType: comment
Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Tarun, Tony Huang, Zhuohao Lee.
Hello Derek Huang, Dinesh Gehlot, Eran Mitrani, Eric Lai, Jakub Czapiga, Kapil Porwal, Subrata Banik, Tarun, Zhuohao Lee, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/81564?usp=email
to look at the new patch set (#5).
Change subject: mb/google/rex/var/deku: Swap LAN device indices for correct MAC address
......................................................................
mb/google/rex/var/deku: Swap LAN device indices for correct MAC address
Deku has two Ethernet ports. Currently both get assigned the wrong
MAC address due to the LAN devices indices being swapped and
vpd ethernet_mac0() affects device eth1 and vpd ethernet_mac1() affects
device eth0.
Correct the device indices for LAN devices so ethernet_mac[0-1] in vpd
can apply to the correct ethernet ports.
BUG=b:320203629
BRANCH=firmware-rex-15709.B
TEST=vpd -s ethernet_mac0=<mac address0>
vpd -s ethernet_mac1=<mac address1>
reboot the system and check ifconfig
eth0 and eth1 MAC addresses are fetched correctly
Change-Id: Id1508104cbb5cf0a234f34f9db19cc535fdb634b
Signed-off-by: Tony Huang <tony-huang(a)quanta.corp-partner.google.com>
---
M src/mainboard/google/rex/variants/deku/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/81564/5
--
To view, visit https://review.coreboot.org/c/coreboot/+/81564?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Id1508104cbb5cf0a234f34f9db19cc535fdb634b
Gerrit-Change-Number: 81564
Gerrit-PatchSet: 5
Gerrit-Owner: Tony Huang <tony-huang(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Derek Huang <derekhuang(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tarun <tstuli(a)gmail.com>
Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Kevin Chiu <kevin.chiu.17802(a)gmail.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Eran Mitrani <mitrani(a)google.com>
Gerrit-Attention: Tony Huang <tony-huang(a)quanta.corp-partner.google.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Zhuohao Lee <zhuohao(a)google.com>
Gerrit-Attention: Tarun <tstuli(a)gmail.com>
Gerrit-MessageType: newpatchset