Attention is currently required from: David Wu, Ren Kuo, Shelley Chen, Subrata Banik, Tyler Wang.
Karthik Ramasubramanian has posted comments on this change by Ren Kuo. ( https://review.coreboot.org/c/coreboot/+/83212?usp=email )
Change subject: mb/google/brox: Create jubilant variant
......................................................................
Patch Set 4:
(6 comments)
File src/mainboard/google/brox/variants/jubilant/Makefile.mk:
https://review.coreboot.org/c/coreboot/+/83212/comment/c1df3133_2b3f7d93?us… :
PS4, Line 3: bootblock-y += gpio.c
Nit: Leave a blank line between each stages for clarity.
File src/mainboard/google/brox/variants/jubilant/fw_config.c:
https://review.coreboot.org/c/coreboot/+/83212/comment/6aa79186_7f3ff451?us… :
PS4, Line 15: if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_NVME))) {
: printk(BIOS_INFO, "Configure GPIOs, device config for NVME.\n");
: }
:
: if (fw_config_probe(FW_CONFIG(STORAGE, STORAGE_UNKNOWN))) {
: printk(BIOS_INFO, "Configure GPIOs, device config for UFS and NVME.\n");
: }
These things do nothing. Remove them now and add them later when you configure the GPIOs.
File src/mainboard/google/brox/variants/jubilant/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/83212/comment/c061b4af_3bab7ba2?us… :
PS4, Line 11: #define WWAN_RST GPP_E16
: #define WWAN_PERST GPP_E0
These don't look correct. WWAN_PERST does not exist in GPIO config.
File src/mainboard/google/brox/variants/jubilant/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/83212/comment/0486f0e9_c3ab88dc?us… :
PS4, Line 2: field RETIMER 0 1
: option RETIMER_UNKNOWN 0
: option RETIMER_BYPASS 1
: end
: field STORAGE 2 3
: option STORAGE_UNKNOWN 0
: option STORAGE_UFS 1
: option STORAGE_NVME 2
: end
: field WIFI_BT 4 4
: option WIFI_BT_CNVI 0
: option WIFI_BT_PCIE 1
: end
: field AUDIO 5 7
: option AUDIO_UNKNOWN 0
: option AUDIO_REALTEK_ALC256 1
: end
: field UFC 8 9
: option UFC_NONE 0
: option UFC_USB 1
: end
: field FPMCU 17 18
: option FP_ABSENT 0
: option FP_MCU_NUVOTON 1
: end
: field ISH 21
: option ISH_DISABLE 0
: option ISH_ENABLE 1
: end
The entire devicetree seems copypasted from another devicetree. Please acknowledge that you have to tune them at a later point in time. Ideally I would recommend you to start fresh instead of copy-paste, so that you can enable only the required things - eg. ISH is not required.
https://review.coreboot.org/c/coreboot/+/83212/comment/d3f27eb9_49022e6f?us… :
PS4, Line 39: WWLAN
Nit: WWAN
https://review.coreboot.org/c/coreboot/+/83212/comment/124e828d_554dffe6?us… :
PS4, Line 294: DB_USB DB_1C_LTE
This mask is not defined in the FW Config. This is another reason, I recommend you to start a fresh devicetree instead of copy-pasting from another devicetree.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83212?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: Ic54437697058f8bce2167093bd88c0880d1b7cac
Gerrit-Change-Number: 83212
Gerrit-PatchSet: 4
Gerrit-Owner: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: Shelley Chen <shchen(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Shelley Chen <shchen(a)google.com>
Gerrit-Attention: David Wu <david_wu(a)quanta.corp-partner.google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Ren Kuo <ren.kuo(a)quanta.corp-partner.google.com>
Gerrit-Attention: Tyler Wang <tyler.wang(a)quanta.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 26 Jul 2024 17:02:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Bao Zheng, Jason Nien, Martin Roth, Matt DeVillier, Zheng Bao.
Tim Van Patten has posted comments on this change by Bao Zheng. ( https://review.coreboot.org/c/coreboot/+/83646?usp=email )
Change subject: mb/google/skyrim: Combine the variants function
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
CC'ing Jon since he's done work in this area.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83646?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: I981e9c52c8e5fa32296e2e43be47411557133691
Gerrit-Change-Number: 83646
Gerrit-PatchSet: 1
Gerrit-Owner: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Zheng Bao
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jon Murphy <jpmurphy(a)google.com>
Gerrit-CC: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Tim Van Patten <timvp(a)google.com>
Gerrit-Attention: Bao Zheng <fishbaozi(a)gmail.com>
Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Zheng Bao
Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com>
Gerrit-Comment-Date: Fri, 26 Jul 2024 15:14:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Dinesh Gehlot, Jayvik Desai.
Subrata Banik has posted comments on this change by Dinesh Gehlot. ( https://review.coreboot.org/c/coreboot/+/83667?usp=email )
Change subject: src: Enable config to determine eSOL status
......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
ideally you can split this into three cls
1. CROS adding SOL Kconfig
2. libgfx select eSOL
3. MTL selects eSOL
File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/83667/comment/d1327dae_1f886aa1?us… :
PS1, Line 55: MAINBOARD_HAS_EARLY_SIGN_OF_LIFE
eSOL is a chromeos feature then why this is here ? i would request to have the config added as part of the ChromeOS directory like `CROS_ENABLES_ESOL` which should be only enabled with CROS build Kconfig and not directly like what you did from line 68 below
--
To view, visit https://review.coreboot.org/c/coreboot/+/83667?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: I68aca8033cf843e8a569339ab1af85fab104b36a
Gerrit-Change-Number: 83667
Gerrit-PatchSet: 1
Gerrit-Owner: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Reviewer: Eran Mitrani <mitrani(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jayvik Desai <jayvik(a)google.com>
Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jayvik Desai <jayvik(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Comment-Date: Fri, 26 Jul 2024 13:53:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No