Attention is currently required from: Karthik Ramasubramanian, Matt DeVillier, Subrata Banik.
Eric Lai has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/84363?usp=email )
Change subject: mb/google/brox: Drop redundant entries of crashlog config
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/84363?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: Idcb03d13ee3943f188246663d47f47cb8afccbd9
Gerrit-Change-Number: 84363
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Eric Lai <ericllai(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Mon, 16 Sep 2024 03:45:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Arthur Heymans, Cliff Huang, Elyes Haouas, Hung-Te Lin, Julius Werner, Jérémy Compostella, Lance Zhao, Tim Wawrzynczak, Yidi Lin.
Yu-Ping Wu has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84208?usp=email )
Change subject: soc/mt6366: Work around GCC LTO build
......................................................................
Patch Set 8:
(1 comment)
File src/soc/mediatek/mt8186/mt6366.c:
https://review.coreboot.org/c/coreboot/+/84208/comment/4f27016a_4644548c?us… :
PS8, Line 10: #define NO_BUILDTIME_ASSERT
> just because one assert statement in a file had this issue the other statements in that file are somehow more likely to also have it
Well, that's exactly the argument I made in my previous comment. I still think this statement is true, at least for this file. By "robust" I meant a workaround solution which will continue to work despite future changes of other code (until GCC fixes the bug). Therefore I actually prefer PS5 the most (PS5 > per-file > per-statement).
Consider the situation where someone adds a new regulator type for mt6366, which breaks some of the existing `assert()` calls in this file. They probably wouldn't notice that until uploading the patch and then seeing the Jenkins build failure. Then, they will see the error message, probably spend some time trying to fix the problem, and then eventually figure out they'd need to just change those calls to `assertX()`. My question is, why not changing all of them to `assertX()` *right now*, so that all of these potential hassles could be avoided?
--
To view, visit https://review.coreboot.org/c/coreboot/+/84208?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: I6185e87a374f8722dba545d6bbce1c3a8de53e7e
Gerrit-Change-Number: 84208
Gerrit-PatchSet: 8
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Reviewer: Tim Wawrzynczak <inforichland(a)gmail.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: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Julius Werner <jwerner(a)chromium.org>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Mon, 16 Sep 2024 03:05:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Attention is currently required from: Balaji Manigandan, Jakub Czapiga, Jędrzej Ciupis, Matt DeVillier, Pratikkumar V Prajapati, Sowmya Aralguppe.
Subrata Banik has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/coreboot/+/84359?usp=email )
Change subject: mb/google/dedede: Select INTEL_CRASHLOG only for ChromeOS builds
......................................................................
Patch Set 2:
(2 comments)
Patchset:
PS2:
Hello Intel Team,
Can You please help to take a look into Crashlog error in JSL. Wondering if you have ever tested crashlog on JSL devices ?
File src/mainboard/google/dedede/Kconfig:
https://review.coreboot.org/c/coreboot/+/84359/comment/d40f9019_27a3d9a4?us… :
PS1, Line 39: select SOC_INTEL_CRASHLOG
> > to the problem that you faced while booting windows/linux on jsl Chromebook.
>
> actually, it seems to have nothing to do with OS or even payload; crashlog is just broken under JSL with upstream coreboot:
>
> [DEBUG] cmd_reg from pmc_make_ipc_cmd 1052838
> [ERROR] Null dereference at eip: 0x76ac8512
> [ERROR] Null dereference at eip: 0x76ac85ea
> [ERROR] Null dereference at eip: 0x76ac610c
> [DEBUG] PMC crashlog size in legacy mode = 0xd00
> [DEBUG] Read ID for Telemetry: 0x0 differs from expected: 0x23
> [ERROR] CPU crashlog capability not found.
> [DEBUG] PMC crashLog size in legacy mode : 0xD00
> [DEBUG] Invalid data 0x0 at offset 0x1300 from addr 0xfe010000 of PMC SRAM.
> [DEBUG] legacy mode PMC crashlog size adjusted to: 0x0
> [DEBUG] m_cpu_crashLog_size : 0x0 bytes
> [DEBUG] Skipping CPU crashLog collection. Data not present.
>
> unless this is expected behavior
CB:83884 tries to enable crashlog for JSL. I will talk to Intel team if they can take a look into it.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84359?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: I53be4befe9a04bdaece21f40f93af6599baa7e0b
Gerrit-Change-Number: 84359
Gerrit-PatchSet: 2
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Balaji Manigandan <balaji.bmg(a)gmail.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jędrzej Ciupis <jciupis(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Reviewer: Sowmya Aralguppe <sowmya.aralguppe(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Sowmya Aralguppe <sowmya.aralguppe(a)intel.com>
Gerrit-Attention: Jędrzej Ciupis <jciupis(a)google.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Balaji Manigandan <balaji.bmg(a)gmail.com>
Gerrit-Attention: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com>
Gerrit-Comment-Date: Mon, 16 Sep 2024 02:13:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Attention is currently required from: Dinesh Gehlot, Jayvik Desai, Kapil Porwal, Karthik Ramasubramanian, Matt DeVillier, Nick Vaccaro, Rishika Raj.
Subrata Banik has posted comments on this change by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/84365?usp=email )
Change subject: soc/intel/alderlake: Enable CRASHLOG for Chrome OS
......................................................................
Patch Set 1:
(1 comment)
File src/soc/intel/alderlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/84365/comment/5bc8f131_ca89852e?us… :
PS1, Line 87: MAINBOARD_HAS_CHROMEOS
> Fair enough. But currently there is no mechanism to disable the feature via Kconfig, and it doesn't appear to have been tested for the non-CHROMEOS use case.
I have guarded `MAINBOARD_HAS_CHROMEOS` to ensure atleast on ChromeOS devices, we have enabled and tested this feature started with ADL. Intel recently fixed some issues over Crashlog enablement in ADL as well.
This code will ensure crashlog config is only enabled for devices with CrOS.
--
To view, visit https://review.coreboot.org/c/coreboot/+/84365?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: Ia23ef1cbebdba9a3b724204eb25ee788afa3e8fd
Gerrit-Change-Number: 84365
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jayvik Desai <jayvik(a)google.com>
Gerrit-Attention: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
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: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Mon, 16 Sep 2024 02:12:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Shon Wang has posted comments on this change by Shon Wang. ( https://review.coreboot.org/c/coreboot/+/84333?usp=email )
Change subject: mb/google/brask/var/bujia: Fix PSYS voltage setting
......................................................................
Patch Set 3:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/84333/comment/c9027a57_387d497d?us… :
PS3, Line 2: Shon
> Please use your full name. […]
I will correct it next time. thanks
https://review.coreboot.org/c/coreboot/+/84333/comment/11b409b9_20069dc4?us… :
PS3, Line 15: TEST= cbmem -c | grep -i PsysPmax
> What is the output before and after?
before: psys_config default voltage value
after: adaptor voltage if exists
--
To view, visit https://review.coreboot.org/c/coreboot/+/84333?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: I848c92752b7a7b53f47c6296aad0bdda20e9b0bd
Gerrit-Change-Number: 84333
Gerrit-PatchSet: 3
Gerrit-Owner: Shon Wang <shon.wang(a)quanta.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: 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: Rishika Raj <rishikaraj(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Comment-Date: Mon, 16 Sep 2024 01:39:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>