coreboot-gerrit February 2025

coreboot-gerrit@coreboot.org

1 participants
2886 discussions

[XS] Change in coreboot[main]: soc/mediatek/mt8196: Adjust thermal trip point parameters
by Yu-Ping Wu (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Hung-Te Lin, Jarried Lin, Zhaoqing Jiu. Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/86552?usp=email ) Change subject: soc/mediatek/mt8196: Adjust thermal trip point parameters ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86552?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: Ia7361edd7f75b82fff4241ec94488ed1ef07346f Gerrit-Change-Number: 86552 Gerrit-PatchSet: 3 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> 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-CC: Zhaoqing Jiu <zhaoqing.jiu(a)mediatek.corp-partner.google.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Zhaoqing Jiu <zhaoqing.jiu(a)mediatek.corp-partner.google.com> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Mon, 24 Feb 2025 04:14:48 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[S] Change in coreboot[main]: soc/mediatek/mt8196: Save HW protect temperature to SRAM
by Yu-Ping Wu (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Hung-Te Lin, Jarried Lin. Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/86551?usp=email ) Change subject: soc/mediatek/mt8196: Save HW protect temperature to SRAM ...................................................................... Patch Set 2: (5 comments) File src/soc/mediatek/mt8196/thermal.c: https://review.coreboot.org/c/coreboot/+/86551/comment/709a3bae_07ac3074?us… : PS2, Line 606: if (tc_num == LVTS_AP_CONTROLLER0) : tc_index = 0; : else if (tc_num == LVTS_AP_CONTROLLER1) : tc_index = 1; : : if (tc_index != 0xff) { : thermal_write_reboot_msr_sram(tc_index, raw_high); : if (tc_index == 0) : thermal_write_reboot_temp_sram(tc->reboot_temperature); : } > ``` […] I feel like the flow should be controlled by the `lvts_thermal_controller` struct, because that's how the remaining code works. Can we add a new bit field `flags` to indicate whether `thermal_write_reboot_*_sram` needs to be called? Right now only controllers 0 and 1 are enabled (`CTRL_ON`). However the code here should support controllers 2 and 3 as well. Should `thermal_write_reboot_msr_sram` and `thermal_write_reboot_temp_sram` be called for controllers 2 and 3 (if they are enabled)? File src/soc/mediatek/mt8196/thermal_sram.c: https://review.coreboot.org/c/coreboot/+/86551/comment/4734ff0a_80b8a36d?us… : PS2, Line 10: c C https://review.coreboot.org/c/coreboot/+/86551/comment/66205ec1_8591d7e0?us… : PS2, Line 66: int uint32_t https://review.coreboot.org/c/coreboot/+/86551/comment/07e087ce_37c27bda?us… : PS2, Line 71: int > unint32_t ? `unsigned int` IMO https://review.coreboot.org/c/coreboot/+/86551/comment/a6c053e9_adda6025?us… : PS2, Line 74: if ((idx * 4) < THERMAL_REBOOT_MSR_SRAM_LEN) { If `idx * 4 >= THERMAL_REBOOT_MSR_SRAM_LEN` is an unexpected case, can we use an assertion? -- To view, visit https://review.coreboot.org/c/coreboot/+/86551?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: Ib714c297871132907e286536c4b3aea1532f3869 Gerrit-Change-Number: 86551 Gerrit-PatchSet: 2 Gerrit-Owner: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> 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-CC: Zhaoqing Jiu <zhaoqing.jiu(a)mediatek.corp-partner.google.com> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Jarried Lin <jarried.lin(a)mediatek.com> Gerrit-Comment-Date: Mon, 24 Feb 2025 04:14:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Yidi Lin <yidilin(a)google.com>

1 0

[S] Change in coreboot[main]: refactor: Consolidate bmp_logo_filename() logic
by Subrata Banik (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Paz Zcharya. Subrata Banik has posted comments on this change by Paz Zcharya. ( https://review.coreboot.org/c/coreboot/+/86566?usp=email ) Change subject: refactor: Consolidate bmp_logo_filename() logic ...................................................................... Patch Set 1: (1 comment) File src/lib/bmp_logo.c: https://review.coreboot.org/c/coreboot/+/86566/comment/347d7c0c_ebaed254?us… : PS1, Line 37: if (platform_is_low_battery_shutdown_needed()) : logo_name = "low_battery.bmp"; > The only issue is that `platform_is_low_battery_shutdown_needed` is defined in `#include <fsp/api.h>`. with refactoring CL, everything will move into lib/bmp -- To view, visit https://review.coreboot.org/c/coreboot/+/86566?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: Ifd91af411aff6ccd6f5974a0b6c849794b9a2794 Gerrit-Change-Number: 86566 Gerrit-PatchSet: 1 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Comment-Date: Mon, 24 Feb 2025 04:02:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>

1 0

[S] Change in coreboot[main]: util/cbfstool: Add missing \n in debug messages
by Shuo Liu (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Intel coreboot Reviewers, Jincheng Li, Patrick Rudolph. Hello Angel Pons, Arthur Heymans, Christian Walter, Intel coreboot Reviewers, Jincheng Li, Patrick Rudolph, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/86567?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: util/cbfstool: Add missing \n in debug messages ...................................................................... util/cbfstool: Add missing \n in debug messages Find all potential missings by below script and apply manual check and fixes. grep -nE "(DEBUG|ERROR)\(\".+[^\\n]\"" util/cbfstool/ -r Change-Id: I3e2c225dc16a65470f9f94db89d8ec3711e781c8 Signed-off-by: Shuo Liu <shuo.liu(a)intel.com> --- M util/cbfstool/cbfs_image.c M util/cbfstool/fit.c 2 files changed, 11 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/86567/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/86567?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: I3e2c225dc16a65470f9f94db89d8ec3711e781c8 Gerrit-Change-Number: 86567 Gerrit-PatchSet: 3 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com>

1 0

[XS] Change in coreboot[main]: util/cbfstool: Handle unaligned component adding by do_cbfs_locate
by Shuo Liu (Code Review) 23 Feb '25

23 Feb '25

Shuo Liu has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/86568?usp=email ) Change subject: util/cbfstool: Handle unaligned component adding by do_cbfs_locate ...................................................................... Abandoned abandon the patch now, due to conflict with the latest upstreaming logic -- To view, visit https://review.coreboot.org/c/coreboot/+/86568?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: Ifbf9017170833d7786eb95922554ddc86d2a5f8b Gerrit-Change-Number: 86568 Gerrit-PatchSet: 2 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in coreboot[main]: refactor: Consolidate bmp_logo_filename() logic
by Paz Zcharya (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Subrata Banik. Paz Zcharya has posted comments on this change by Paz Zcharya. ( https://review.coreboot.org/c/coreboot/+/86566?usp=email ) Change subject: refactor: Consolidate bmp_logo_filename() logic ...................................................................... Patch Set 1: (1 comment) File src/lib/bmp_logo.c: https://review.coreboot.org/c/coreboot/+/86566/comment/a51ea416_6c8dcd3f?us… : PS1, Line 37: if (platform_is_low_battery_shutdown_needed()) : logo_name = "low_battery.bmp"; > > Oh, I see. […] The only issue is that `platform_is_low_battery_shutdown_needed` is defined in `#include <fsp/api.h>`. -- To view, visit https://review.coreboot.org/c/coreboot/+/86566?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: Ifd91af411aff6ccd6f5974a0b6c849794b9a2794 Gerrit-Change-Number: 86566 Gerrit-PatchSet: 1 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Comment-Date: Mon, 24 Feb 2025 02:35:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>

1 0

[S] Change in coreboot[main]: refactor: Consolidate bmp_logo_filename() logic
by Subrata Banik (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Paz Zcharya. Subrata Banik has posted comments on this change by Paz Zcharya. ( https://review.coreboot.org/c/coreboot/+/86566?usp=email ) Change subject: refactor: Consolidate bmp_logo_filename() logic ...................................................................... Patch Set 1: (1 comment) File src/lib/bmp_logo.c: https://review.coreboot.org/c/coreboot/+/86566/comment/ee08189b_eb2d5afc?us… : PS1, Line 37: if (platform_is_low_battery_shutdown_needed()) : logo_name = "low_battery.bmp"; > Oh, I see. > > Okay so should I re-upload that CL but with the safeguards in this file? I don't think this CL is necessary. You probably want to move the low-battery logo to cros vendor code to keep the generic bmp_logo implementation clean. However, as I mentioned before, the idea is to decouple the cros splash from low-battery to allow non-cros device to use low-battery logo, so nothing needs to be done at this point imo. -- To view, visit https://review.coreboot.org/c/coreboot/+/86566?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: Ifd91af411aff6ccd6f5974a0b6c849794b9a2794 Gerrit-Change-Number: 86566 Gerrit-PatchSet: 1 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Comment-Date: Mon, 24 Feb 2025 02:34:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paz Zcharya <pazz(a)google.com> Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>

1 0

[S] Change in coreboot[main]: util/cbfstool: Add missing \n in debug messages
by Shuo Liu (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Intel coreboot Reviewers, Jincheng Li, Patrick Rudolph. Hello Angel Pons, Arthur Heymans, Christian Walter, Intel coreboot Reviewers, Jincheng Li, Patrick Rudolph, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/86567?usp=email to look at the new patch set (#2). Change subject: util/cbfstool: Add missing \n in debug messages ...................................................................... util/cbfstool: Add missing \n in debug messages Find all potential missings by below script and apply manual check and fixes. grep -nE "(DEBUG|ERROR)\(\".+[^\\n]\"" util/cbfstool/ -r Change-Id: I3e2c225dc16a65470f9f94db89d8ec3711e781c8 Signed-off-by: Shuo Liu <shuo.liu(a)intel.com> --- M util/cbfstool/cbfs_image.c M util/cbfstool/fit.c 2 files changed, 11 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/86567/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/86567?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: I3e2c225dc16a65470f9f94db89d8ec3711e781c8 Gerrit-Change-Number: 86567 Gerrit-PatchSet: 2 Gerrit-Owner: Shuo Liu <shuo.liu(a)intel.com> Gerrit-Reviewer: Angel Pons <angel.pons(a)9elements.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Reviewer: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Jincheng Li <jincheng.li(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Angel Pons <angel.pons(a)9elements.com>

1 0

[S] Change in coreboot[main]: refactor: Consolidate bmp_logo_filename() logic
by Paz Zcharya (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Subrata Banik. Paz Zcharya has posted comments on this change by Paz Zcharya. ( https://review.coreboot.org/c/coreboot/+/86566?usp=email ) Change subject: refactor: Consolidate bmp_logo_filename() logic ...................................................................... Patch Set 1: (1 comment) File src/lib/bmp_logo.c: https://review.coreboot.org/c/coreboot/+/86566/comment/1ead43d1_f94f1739?us… : PS1, Line 37: if (platform_is_low_battery_shutdown_needed()) : logo_name = "low_battery.bmp"; > This existed to allow low-battery rendering by non-ChromeOS devices as well. […] Oh, I see. Okay so should I re-upload that CL but with the safeguards in this file? -- To view, visit https://review.coreboot.org/c/coreboot/+/86566?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: Ifd91af411aff6ccd6f5974a0b6c849794b9a2794 Gerrit-Change-Number: 86566 Gerrit-PatchSet: 1 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Comment-Date: Mon, 24 Feb 2025 02:31:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>

1 0

[S] Change in coreboot[main]: refactor: Consolidate bmp_logo_filename() logic
by Subrata Banik (Code Review) 23 Feb '25

23 Feb '25

Attention is currently required from: Paz Zcharya. Subrata Banik has posted comments on this change by Paz Zcharya. ( https://review.coreboot.org/c/coreboot/+/86566?usp=email ) Change subject: refactor: Consolidate bmp_logo_filename() logic ...................................................................... Patch Set 1: (1 comment) File src/lib/bmp_logo.c: https://review.coreboot.org/c/coreboot/+/86566/comment/f41f21f9_1fcf1eee?us… : PS1, Line 37: if (platform_is_low_battery_shutdown_needed()) : logo_name = "low_battery.bmp"; This existed to allow low-battery rendering by non-ChromeOS devices as well. Since there's no relation between low-battery and ChromeOS vendor code, the reviewers suggested keeping the low-battery logo inside bmp_logo.c, while the ChromeOS splash screen is ChromeOS-specific code that should be in the vendor code. -- To view, visit https://review.coreboot.org/c/coreboot/+/86566?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: Ifd91af411aff6ccd6f5974a0b6c849794b9a2794 Gerrit-Change-Number: 86566 Gerrit-PatchSet: 1 Gerrit-Owner: Paz Zcharya <pazz(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Paz Zcharya <pazz(a)google.com> Gerrit-Comment-Date: Mon, 24 Feb 2025 02:20:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2025