coreboot-gerrit April 2024

coreboot-gerrit@coreboot.org

1 participants
3292 discussions

[L] Change in coreboot[main]: acpi: Add acpigen_write_OSC_pci_domain
by Shuo Liu (Code Review) 03 Apr '24

03 Apr '24

Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81375?usp=email ) Change subject: acpi: Add acpigen_write_OSC_pci_domain ...................................................................... Patch Set 19: (2 comments) Patchset: PS19: > Please consider moving this back to `soc/intel/`. The code seems to be tailored to […] I see. These codes are initially raised for xeon_sp and then through discussion they are moved to acpigen layer. I will make another patch so that discussion could be made to see whether we will put it back to xeon_sp again. File src/acpi/acpigen_pci.c: https://review.coreboot.org/c/coreboot/+/81375/comment/1dfb52f4_05935b0b : PS12, Line 348: __weak unsigned long get_granted_pci_features(const struct device *domain) : { : return 0; : } : : __weak unsigned long get_granted_cxl_features(const struct device *domain) : { : return 0; : } > This explains what would happen not why it would make sense to generate all the code […] Pass parameters instead of using weak functions looks good. Let us discuss together in new patch later. -- To view, visit https://review.coreboot.org/c/coreboot/+/81375?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: I711ce5350d718e47feb2912555108801ad7f918d Gerrit-Change-Number: 81375 Gerrit-PatchSet: 19 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Wed, 03 Apr 2024 01:39:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Shuo Liu <shuo.liu(a)intel.com> Gerrit-MessageType: comment

1 0

[XL] Change in coreboot[main]: lib/device_tree: Add some FDT helper functions
by Julius Werner (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email ) Change subject: lib/device_tree: Add some FDT helper functions ...................................................................... Patch Set 18: (8 comments) File src/include/device_tree.h: https://review.coreboot.org/c/coreboot/+/81081/comment/c20e785d_52a59bf7 : PS18, Line 203: nit: Please add at least a line of documentation, particularly to clarify here what the return value is. File src/lib/device_tree.c: https://review.coreboot.org/c/coreboot/+/81081/comment/3890d52e_cafa86d8 : PS12, Line 200: if (!strncmp(*path, node_name, path_sub_len)) { > I changed it now to use a 'path_array' instead of the path directly (although personally I find the […] Ack, that works too. (I think the code should get quite a bit simpler still with some of my other suggestions, e.g. pull `find_next_node_name()` into the loop condition and reduce the number of arguments.) https://review.coreboot.org/c/coreboot/+/81081/comment/97f6c220_a7cc00b8 : PS12, Line 214: ince address-cells and size-cells are not inherited > I am not sure what to do now. According to the issue, we should not treat them as inheritable. I mean sounds like they're acknowledging that Linux is intentionally doing something else to improve compatibility. I would say in that case following Linux would make it more likely that our code works with stuff than following the spec. People who generate device trees test against implementations, not against an abstract spec. Either way, I think it's important that this library is internally consistent. All the non-flattened APIs (e.g. `dt_find_node()` currently do what Linux does. You should either match that behavior or change all those function to follow the spec. File src/lib/device_tree.c: https://review.coreboot.org/c/coreboot/+/81081/comment/1396a001_0d9f13c0 : PS18, Line 175: "" I don't really understand the `""` here at the start, what does that represent? There shouldn't be an extra string for the root node, that's where you start after all (i.e. where node_offset already is). If someone does `fdt_find_node_by_path("/")`, that should get translated into `fdt_find_node(blob, root_node_offset, { /* empty array */ }, 0, 0, ...)` (or, with my suggestion below, `fdt_find_node(blob, root_node_offset, { NULL }, ...)`). https://review.coreboot.org/c/coreboot/+/81081/comment/f583e281_d1571979 : PS18, Line 188: size_t path_count I think you should match `dt_find_node()` with this API for consistency (e.g. no `path_index`, no `path_count` and instead terminate the path array with `NULL`). https://review.coreboot.org/c/coreboot/+/81081/comment/b52b9b93_88fb4caa : PS18, Line 251: #define PATH_ARRAY_MAX 10 I think this should go at the top of the file (maybe call it FDT_PATH_MAX_DEPTH to clarify that it doesn't count for the unflattened DT functions). https://review.coreboot.org/c/coreboot/+/81081/comment/697322b7_56f00672 : PS18, Line 255: 128 This should also be a named constant (e.g. FDT_PATH_MAX_LEN). https://review.coreboot.org/c/coreboot/+/81081/comment/36643a45_f6a4aae6 : PS18, Line 265: } I think this would be cleaner with `memcpy()` and `strtok_r()`: ``` char path_copy[128]; memcpy(path_copy, path + 1, path_size); char *cur = path_copy; for (int i = 0; i < PATH_ARRAY_MAX; i++) { path_array[i] = strtok_r(NULL, '/', &cur); if (!path_array[i]) break; } assert(i < PATH_ARRAY_MAX); ``` That way you also get the `NULL` terminator for the path array right away (for my suggestion on how to change the `fdt_find_node()` signature), and I think `strtok_r()` automatically ignores the trailing slash. -- To view, visit https://review.coreboot.org/c/coreboot/+/81081?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: I2fb1d93c5b3e1cb2f7d9584db52bbce3767b63d8 Gerrit-Change-Number: 81081 Gerrit-PatchSet: 18 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alper Nebi Yasak <alpernebiyasak(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 01:35:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com> Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: lib: Refactor bmp_load_logo() implementation
by Subrata Banik (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Andrey Petrov, Felix Held, Fred Reitberger, Jason Glenesk, Julius Werner, Matt DeVillier, Ronak Kanabar, Sean Rhodes. Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81618?usp=email ) Change subject: lib: Refactor bmp_load_logo() implementation ...................................................................... Patch Set 3: (2 comments) File src/drivers/intel/fsp1_1/ramstage.c: https://review.coreboot.org/c/coreboot/+/81618/comment/88ae012c_345579d5 : PS3, Line 81: uint32_t > I don't think this would compile for 64-bit. Better make this a cast to `uintptr_t`. (You can leave the narrowing conversion from there to `uint32_t` implicit or add another cast in front of that.) > > Same issue at most other call sites. is this what you meant ? `PcdLogoPtr` is UINT32 type. ``` silicon_init_params.PcdLogoPtr = (uint32_t)(uintptr_t)bmp_load_logo(&logo_size); ``` File src/drivers/intel/fsp2_0/fsp_gop_blt.c: https://review.coreboot.org/c/coreboot/+/81618/comment/f5ba1a03_ce87ebdd : PS3, Line 241: logo_size_t > `_t` is usually a suffix for types, not variable identifiers. sorry for not being thoughtful with the variable name. I will do the change to make the variable name meaningful here,. > Why not just change `logo_ptr_size` to a `size_t`? https://review.coreboot.org/c/coreboot/+/81615/7/src/drivers/intel/fsp2_0/f… CL is dropping the temp variable. just keeping CLs meaningful -- 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: 3 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: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> 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: Wed, 03 Apr 2024 01:10:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: drivers/mipi: Fine tune clock for BOE_NV110WUM_L60
by Xuxin Xiong (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Paul Menzel, Ruihai Zhou, Yidi Lin. Xuxin Xiong has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81438?usp=email ) Change subject: drivers/mipi: Fine tune clock for BOE_NV110WUM_L60 ...................................................................... Patch Set 6: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/81438/comment/25b38be2_05f45660 : PS4, Line 11: Just keep consistent with the Linux kernel panel driver : configuration. > Please add a URL then. https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/… -- To view, visit https://review.coreboot.org/c/coreboot/+/81438?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: Ic44c86f062d4e836f403ee97f2fc6370fff02797 Gerrit-Change-Number: 81438 Gerrit-PatchSet: 6 Gerrit-Owner: Ruihai Zhou <zhouruihai(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-Reviewer: cong yang <yangcong5(a)huaqin.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Xuxin Xiong <xuxinxiong(a)huaqin.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Ruihai Zhou <zhouruihai(a)huaqin.corp-partner.google.com> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 00:47:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Comment-In-Reply-To: Ruihai Zhou <zhouruihai(a)huaqin.corp-partner.google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: riscv/opensbi: make the opensbi payload a flat binary
by ron minnich (Code Review) 03 Apr '24

03 Apr '24

ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/81632?usp=email ) Change subject: riscv/opensbi: make the opensbi payload a flat binary ...................................................................... riscv/opensbi: make the opensbi payload a flat binary opensbi is now self-relocating and, further, has strict requirements on the alignment of the data segment -- it has to be power of 2. As opposed to trying to play too many games with ELF files and ldscripts, taking them beyond what's possible, It's simplest just to unpack the opensbi elf into a flat binary and bspecify its location in cbfs. Change-Id: I31525125e87ff5925ca82f4e48fe1191ba6326fd Signed-off-by: Ronald G Minnich <rminnich(a)gmail.com> --- M src/arch/riscv/Makefile.mk M src/mainboard/emulation/qemu-riscv/memlayout.ld 2 files changed, 7 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/81632/1 diff --git a/src/arch/riscv/Makefile.mk b/src/arch/riscv/Makefile.mk index d5defea..7e00711 100644 --- a/src/arch/riscv/Makefile.mk +++ b/src/arch/riscv/Makefile.mk @@ -149,7 +149,7 @@ OPENSBI_SOURCE := $(top)/3rdparty/opensbi OPENSBI_BUILD := $(abspath $(obj)/3rdparty/opensbi) OPENSBI_TARGET := $(OPENSBI_BUILD)/platform/$(CONFIG_OPENSBI_PLATFORM)/firmware/fw_dynamic.elf -OPENSBI := $(obj)/opensbi.elf +OPENSBI := $(obj)/opensbi.bin # TODO: Building with clang has troubles finding the proper linker. # Always use GCC for now. @@ -170,16 +170,15 @@ FW_TEXT_START=$(CONFIG_OPENSBI_TEXT_START) $(OPENSBI): $(OPENSBI_TARGET) - cp $< $@ + $(OBJCOPY_ramstage) -v -O binary $(OPENSBI_TARGET) $(OPENSBI) OPENSBI_CBFS := $(CONFIG_CBFS_PREFIX)/opensbi $(OPENSBI_CBFS)-file := $(OPENSBI) -$(OPENSBI_CBFS)-type := payload -$(OPENSBI_CBFS)-compression := $(CBFS_COMPRESS_FLAG) +$(OPENSBI_CBFS)-type := flat-binary +$(OPENSBI_CBFS)-compression := none +$(OPENSBI_CBFS)-options := -l $(CONFIG_OPENSBI_TEXT_START) -e $(CONFIG_OPENSBI_TEXT_START) cbfs-files-y += $(OPENSBI_CBFS) -check-ramstage-overlap-files += $(OPENSBI_CBFS) - CPPFLAGS_common += -I$(OPENSBI_SOURCE)/include ramstage-y += opensbi.c diff --git a/src/mainboard/emulation/qemu-riscv/memlayout.ld b/src/mainboard/emulation/qemu-riscv/memlayout.ld index 047597b..fc86f05 100644 --- a/src/mainboard/emulation/qemu-riscv/memlayout.ld +++ b/src/mainboard/emulation/qemu-riscv/memlayout.ld @@ -5,8 +5,8 @@ #include <mainboard/addressmap.h> #define BOOTBLOCKSIZE 128K -#define OPENSBISIZE 512K -#define ROMSTAGESIZE 256K +#define OPENSBISIZE 512K+128K +#define ROMSTAGESIZE 0 #define RAMSTAGESIZE 2M #define CONSOLESIZE 8K #define FMAPSIZE 2K -- To view, visit https://review.coreboot.org/c/coreboot/+/81632?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: I31525125e87ff5925ca82f4e48fe1191ba6326fd Gerrit-Change-Number: 81632 Gerrit-PatchSet: 1 Gerrit-Owner: ron minnich <rminnich(a)gmail.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[main]: mb/google/brya/var/sundance: Add GPIO table
by Leo Chou (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Derek Huang, Dinesh Gehlot, Kapil Porwal, Nick Vaccaro, Paul Menzel, Shou-Chieh Hsu, Subrata Banik, Varshit Pandya. Leo Chou has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81613?usp=email ) Change subject: mb/google/brya/var/sundance: Add GPIO table ...................................................................... Patch Set 5: (1 comment) File src/mainboard/google/brya/variants/sundance/Makefile.inc: https://review.coreboot.org/c/coreboot/+/81613/comment/8047e56d_1e38bc88 : PS5, Line 1: # SPDX-License-Identifier: GPL-2.0-only > empty line after this line ? NO, There's no empty line after this line. -- To view, visit https://review.coreboot.org/c/coreboot/+/81613?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: I53ed5874347006985ca5231d1531fa519088f796 Gerrit-Change-Number: 81613 Gerrit-PatchSet: 5 Gerrit-Owner: Leo Chou <leo.chou(a)lcfc.corp-partner.google.com> Gerrit-Reviewer: Derek Huang <derekhuang(a)google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(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: 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: Melo Chuang <melo.chuang(a)lcfc.corp-partner.google.com> Gerrit-CC: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Derek Huang <derekhuang(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: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Shou-Chieh Hsu <shouchieh(a)google.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 00:35:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Varshit Pandya <pandyavarshit(a)gmail.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[main]: mb/google/nissa/var/anraggar: Set EN_SPK_PA to low
by Eric Lai (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Dinesh Gehlot, Jianeng Ceng, Kapil Porwal, Nick Vaccaro, Paul Menzel, Weimin Wu. Eric Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81629?usp=email ) Change subject: mb/google/nissa/var/anraggar: Set EN_SPK_PA to low ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/81629?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: I5982be5a8d965086b46861f4c2c758d9bdee6e75 Gerrit-Change-Number: 81629 Gerrit-PatchSet: 4 Gerrit-Owner: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com> Gerrit-Reviewer: Eric Lai <ericllai(a)google.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Weimin Wu <wuweimin(a)huaqin.corp-partner.google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Dolan Liu <liuyong5(a)huaqin.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> 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: Nick Vaccaro <nvaccaro(a)chromium.org> Gerrit-Attention: Weimin Wu <wuweimin(a)huaqin.corp-partner.google.com> Gerrit-Attention: Jianeng Ceng <cengjianeng(a)huaqin.corp-partner.google.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 00:34:43 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[main]: lib: Refactor bmp_load_logo() implementation
by Julius Werner (Code Review) 03 Apr '24

03 Apr '24

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: Refactor bmp_load_logo() implementation ...................................................................... Patch Set 3: (2 comments) File src/drivers/intel/fsp1_1/ramstage.c: https://review.coreboot.org/c/coreboot/+/81618/comment/422d7bee_f37735d7 : PS3, Line 81: uint32_t I don't think this would compile for 64-bit. Better make this a cast to `uintptr_t`. (You can leave the narrowing conversion from there to `uint32_t` implicit or add another cast in front of that.) Same issue at most other call sites. File src/drivers/intel/fsp2_0/fsp_gop_blt.c: https://review.coreboot.org/c/coreboot/+/81618/comment/c9e7633a_929f3651 : PS3, Line 241: logo_size_t `_t` is usually a suffix for types, not variable identifiers. Why not just change `logo_ptr_size` to a `size_t`? -- 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: 3 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: Wed, 03 Apr 2024 00:00:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XL] Change in coreboot[main]: lib/device_tree: Add some FDT helper functions
by Maximilian Brune (Code Review) 02 Apr '24

02 Apr '24

Attention is currently required from: Jakub Czapiga, Julius Werner, Martin L Roth. Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email ) Change subject: lib/device_tree: Add some FDT helper functions ...................................................................... Patch Set 18: (2 comments) File src/lib/device_tree.c: https://review.coreboot.org/c/coreboot/+/81081/comment/70004b13_3e7000bb : PS12, Line 200: if (!strncmp(*path, node_name, path_sub_len)) { > Don't we? It's used in acpigen and vboot/ec_sync, for example. […] I changed it now to use a 'path_array' instead of the path directly (although personally I find the solutions are similar regarding complexity). I used an upper bound instead of `alloca`, because otherwise GCC complains and I don't want to add an extra pragma like in `src/acpi/acpigen.c`. https://review.coreboot.org/c/coreboot/+/81081/comment/69301c61_d0563510 : PS12, Line 214: ince address-cells and size-cells are not inherited > I asked in this thread: […] I am not sure what to do now. According to the issue, we should not treat them as inheritable. -- To view, visit https://review.coreboot.org/c/coreboot/+/81081?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: I2fb1d93c5b3e1cb2f7d9584db52bbce3767b63d8 Gerrit-Change-Number: 81081 Gerrit-PatchSet: 18 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alper Nebi Yasak <alpernebiyasak(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Julius Werner <jwerner(a)chromium.org> Gerrit-Comment-Date: Tue, 02 Apr 2024 23:10:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org> Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: comment

1 0

[XL] Change in coreboot[main]: lib/device_tree: Add some FDT helper functions
by Maximilian Brune (Code Review) 02 Apr '24

02 Apr '24

Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune. Hello Jakub Czapiga, Julius Werner, Martin L Roth, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/81081?usp=email to look at the new patch set (#18). Change subject: lib/device_tree: Add some FDT helper functions ...................................................................... lib/device_tree: Add some FDT helper functions This adds some helper functions for FDT, since more and more mainboards seem to need FDT nowadays. For example our QEMU boards need it in order to know how much RAM is available. Also all RISC-V boards in our tree need FDT. This also adds some tests in order to test said functions. Signed-off-by: Maximilian Brune <maximilian.brune(a)9elements.com> Change-Id: I2fb1d93c5b3e1cb2f7d9584db52bbce3767b63d8 --- M src/include/device_tree.h M src/lib/device_tree.c M tests/lib/Makefile.mk A tests/lib/device_tree-test.c A tests/lib/tegra30-ouya.dtb.xxd A tests/lib/virt.dtb.xxd M util/lint/lint-000-license-headers 7 files changed, 14,252 insertions(+), 47 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/81/81081/18 -- To view, visit https://review.coreboot.org/c/coreboot/+/81081?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: I2fb1d93c5b3e1cb2f7d9584db52bbce3767b63d8 Gerrit-Change-Number: 81081 Gerrit-PatchSet: 18 Gerrit-Owner: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alper Nebi Yasak <alpernebiyasak(a)gmail.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com> Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com> Gerrit-MessageType: newpatchset

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit April 2024