Attention is currently required from: Arthur Heymans, Caveh Jalali, Christian Walter, Cliff Huang, Elyes Haouas, Felix Held, Forest Mittelberg, Fred Reitberger, Hung-Te Lin, Jakub Czapiga, Jason Glenesk, Johnny Lin, Jonathan Zhang, Jérémy Compostella, Lance Zhao, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Sean Rhodes, Shuo Liu, Tim Chu, Tim Wawrzynczak, Yidi Lin, Yu-Ping Wu.
Hello Arthur Heymans, Caveh Jalali, Christian Walter, Cliff Huang, Felix Held, Forest Mittelberg, Fred Reitberger, Hung-Te Lin, Jakub Czapiga, Jason Glenesk, Johnny Lin, Jonathan Zhang, Jérémy Compostella, Lance Zhao, Lean Sheng Tan, Matt DeVillier, Patrick Rudolph, Sean Rhodes, Shuo Liu, Tim Chu, Tim Wawrzynczak, Yidi Lin, Yu-Ping Wu, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/81830?usp=email
to look at the new patch set (#2).
Change subject: tree: Drop duplicated <device/{path,resource}.h>
......................................................................
tree: Drop duplicated <device/{path,resource}.h>
<device/pci.h> is supposed to provide <device/{path,resource}.h>
Change-Id: I2ef82c8fe30b1c1399a9f85c1734ce8ba16a1f88
Signed-off-by: Elyes Haouas <ehaouas(a)noos.fr>
---
M src/acpi/device.c
M src/arch/x86/cpu.c
M src/arch/x86/mpspec.c
M src/cpu/x86/mp_init.c
M src/device/device_const.c
M src/device/device_util.c
M src/device/oprom/yabel/device.c
M src/device/oprom/yabel/io.c
M src/device/oprom/yabel/mem.c
M src/drivers/generic/adau7002/adau7002.c
M src/drivers/generic/alc1015/alc1015.c
M src/drivers/generic/bayhub/bh720.c
M src/drivers/generic/bayhub_lv2/lv2.c
M src/drivers/generic/gpio_keys/gpio_keys.c
M src/drivers/generic/max98357a/max98357a.c
M src/drivers/generic/nau8315/nau8315.c
M src/drivers/genesyslogic/gl9755/gl9755.c
M src/drivers/genesyslogic/gl9763e/gl9763e.c
M src/drivers/i2c/cs35l53/cs35l53.c
M src/drivers/i2c/cs42l42/cs42l42.c
M src/drivers/i2c/da7219/da7219.c
M src/drivers/i2c/generic/generic.c
M src/drivers/i2c/gpiomux/bus/bus.c
M src/drivers/i2c/gpiomux/mux/mux.c
M src/drivers/i2c/max98373/max98373.c
M src/drivers/i2c/max98390/max98390.c
M src/drivers/i2c/max98396/max98396.c
M src/drivers/i2c/max98927/max98927.c
M src/drivers/i2c/nau8825/nau8825.c
M src/drivers/i2c/rt1011/rt1011.c
M src/drivers/i2c/rt5663/rt5663.c
M src/drivers/i2c/sx9310/sx9310.c
M src/drivers/i2c/sx9324/sx9324.c
M src/drivers/i2c/sx9360/sx9360.c
M src/drivers/i2c/tpm/chip.c
M src/drivers/intel/mipi_camera/camera.c
M src/drivers/intel/soundwire/soundwire.c
M src/drivers/intel/usb4/retimer/retimer.c
M src/drivers/nxp/uwb/uwb.c
M src/drivers/sof/sof.c
M src/drivers/soundwire/alc1308/alc1308.c
M src/drivers/soundwire/alc5682/alc5682.c
M src/drivers/soundwire/alc711/alc711.c
M src/drivers/soundwire/cs42l42/cs42l42.c
M src/drivers/soundwire/max98363/max98363.c
M src/drivers/soundwire/max98373/max98373.c
M src/drivers/spi/acpi/acpi.c
M src/drivers/uart/acpi/acpi.c
M src/drivers/usb/acpi/usb_acpi.c
M src/ec/google/chromeec/audio_codec/audio_codec.c
M src/ec/google/chromeec/ec.c
M src/ec/google/chromeec/i2c_tunnel/i2c_tunnel.c
M src/include/device/pci.h
M src/include/reg_script.h
M src/lib/reg_script.c
M src/soc/amd/common/block/root_complex/ioapic.c
M src/soc/amd/common/fsp/fsp_graphics.c
M src/soc/intel/apollolake/romstage.c
M src/soc/intel/cannonlake/graphics.c
M src/soc/intel/common/block/smbus/smbus.c
M src/soc/intel/common/block/usb4/pcie.c
M src/soc/intel/xeon_sp/include/soc/chip_common.h
M src/soc/intel/xeon_sp/spr/ioat.c
M src/soc/mediatek/common/pcie.c
M src/southbridge/intel/bd82x6x/smbus.c
M src/southbridge/intel/common/smbus_ops.c
M src/southbridge/intel/i82801gx/smbus.c
M src/southbridge/intel/i82801ix/smbus.c
M src/southbridge/intel/i82801jx/smbus.c
M src/southbridge/intel/ibexpeak/smbus.c
M src/southbridge/intel/lynxpoint/smbus.c
M tests/lib/bootmem-test.c
M tests/lib/memrange-test.c
M util/vgabios/device.c
74 files changed, 0 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/81830/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/81830?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: I2ef82c8fe30b1c1399a9f85c1734ce8ba16a1f88
Gerrit-Change-Number: 81830
Gerrit-PatchSet: 2
Gerrit-Owner: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Forest Mittelberg <bmbm(a)google.com>
Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Reviewer: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.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-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Cliff Huang <cliff.huang(a)intel.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Yidi Lin <yidilin(a)google.com>
Gerrit-Attention: Lance Zhao <lance.zhao(a)gmail.com>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems>
Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Tim Wawrzynczak <inforichland(a)gmail.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Shuo Liu <shuo.liu(a)intel.com>
Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com>
Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Attention: Forest Mittelberg <bmbm(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: newpatchset
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/libgfxinit/+/81852?usp=email )
Change subject: tgl plls: Disable warnings about unused variable
......................................................................
tgl plls: Disable warnings about unused variable
Looks like this is yet-to-be-implemented code. To be able to build-test
other changes, turn off some warnings about the `PLLs` variable.
TEST=Run this and make sure all builds pass:
for f in configs/*
do
make distclean
make DEBUG=1 cnf=$f -j$(nproc)
done
Change-Id: I51a14f7a9d6d6d930b9239ed5d0f61c45f2f123b
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M common/tigerlake/hw-gfx-gma-plls.adb
1 file changed, 7 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/libgfxinit refs/changes/52/81852/1
diff --git a/common/tigerlake/hw-gfx-gma-plls.adb b/common/tigerlake/hw-gfx-gma-plls.adb
index bf58f57..2823870 100644
--- a/common/tigerlake/hw-gfx-gma-plls.adb
+++ b/common/tigerlake/hw-gfx-gma-plls.adb
@@ -27,7 +27,14 @@
end record;
type PLL_State_Array is array (Configurable_DPLLs) of PLL_State;
+
+ pragma Warnings (Off, "unused variable ""PLLs""",
+ Reason => "Not yet implemented.");
+ pragma Warnings (Off, "variable ""PLLs"" is assigned but never read",
+ Reason => "Not yet implemented.");
PLLs : PLL_State_Array;
+ pragma Warnings (On, "variable ""PLLs"" is assigned but never read");
+ pragma Warnings (On, "unused variable ""PLLs""");
procedure Initialize is
begin
--
To view, visit https://review.coreboot.org/c/libgfxinit/+/81852?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: libgfxinit
Gerrit-Branch: main
Gerrit-Change-Id: I51a14f7a9d6d6d930b9239ed5d0f61c45f2f123b
Gerrit-Change-Number: 81852
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newchange
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/81627?usp=email )
Change subject: mb/google/brya: Create trulo variant
......................................................................
mb/google/brya: Create trulo variant
This patch adds a new variant trulo for the baseboard trulo.
BUG=b:333314089
TEST=abuild -a -x -p none -t google/brya
Change-Id: I91157d252ef56c8938bfc08ed0f734c5dc7e614d
Signed-off-by: Dinesh Gehlot <digehlot(a)google.com>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/81627
Reviewed-by: Subrata Banik <subratabanik(a)google.com>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Eric Lai <ericllai(a)google.com>
Reviewed-by: Kapil Porwal <kapilporwal(a)google.com>
Reviewed-by: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
---
M src/mainboard/google/brya/Kconfig
M src/mainboard/google/brya/Kconfig.name
A src/mainboard/google/brya/variants/trulo/include/variant/ec.h
A src/mainboard/google/brya/variants/trulo/include/variant/gpio.h
A src/mainboard/google/brya/variants/trulo/memory/Makefile.mk
A src/mainboard/google/brya/variants/trulo/memory/dram_id.generated.txt
A src/mainboard/google/brya/variants/trulo/memory/mem_parts_used.txt
A src/mainboard/google/brya/variants/trulo/overridetree.cb
8 files changed, 52 insertions(+), 0 deletions(-)
Approvals:
Eric Lai: Looks good to me, approved
Kapil Porwal: Looks good to me, approved
build bot (Jenkins): Verified
Sumeet R Pawnikar: Looks good to me, approved
Subrata Banik: Looks good to me, approved
diff --git a/src/mainboard/google/brya/Kconfig b/src/mainboard/google/brya/Kconfig
index 0df0ec1..7f6f7c7 100644
--- a/src/mainboard/google/brya/Kconfig
+++ b/src/mainboard/google/brya/Kconfig
@@ -494,6 +494,9 @@
select INTEL_GMA_HAVE_VBT
select SOC_INTEL_TWINLAKE
+config BOARD_GOOGLE_TRULO
+ select BOARD_GOOGLE_BASEBOARD_TRULO
+
config BOARD_GOOGLE_ULDREN
select BOARD_GOOGLE_BASEBOARD_NISSA
select CHROMEOS_WIFI_SAR if CHROMEOS
@@ -638,6 +641,7 @@
default 0x3 if BOARD_GOOGLE_TAEKO4ES
default 0x1 if BOARD_GOOGLE_TANIKS
default 0x0 if BOARD_GOOGLE_TIVVIKS
+ default 0x0 if BOARD_GOOGLE_TRULO
default 0x0 if BOARD_GOOGLE_ULDREN
default 0x1 if BOARD_GOOGLE_VELL
default 0x1 if BOARD_GOOGLE_VOLMAR
@@ -726,6 +730,7 @@
default "Taeko4ES" if BOARD_GOOGLE_TAEKO4ES
default "Taniks" if BOARD_GOOGLE_TANIKS
default "Tivviks" if BOARD_GOOGLE_TIVVIKS
+ default "Trulo" if BOARD_GOOGLE_TRULO
default "Uldren" if BOARD_GOOGLE_ULDREN
default "Vell" if BOARD_GOOGLE_VELL
default "Volmar" if BOARD_GOOGLE_VOLMAR
@@ -786,6 +791,7 @@
default "taeko" if BOARD_GOOGLE_TAEKO
default "taeko4es" if BOARD_GOOGLE_TAEKO4ES
default "taniks" if BOARD_GOOGLE_TANIKS
+ default "trulo" if BOARD_GOOGLE_TRULO
default "uldren" if BOARD_GOOGLE_ULDREN
default "vell" if BOARD_GOOGLE_VELL
default "volmar" if BOARD_GOOGLE_VOLMAR
diff --git a/src/mainboard/google/brya/Kconfig.name b/src/mainboard/google/brya/Kconfig.name
index a8d111a..0a22cad 100644
--- a/src/mainboard/google/brya/Kconfig.name
+++ b/src/mainboard/google/brya/Kconfig.name
@@ -140,6 +140,9 @@
config BOARD_GOOGLE_TIVVIKS
bool "-> Tivviks"
+config BOARD_GOOGLE_TRULO
+ bool "-> Trulo"
+
config BOARD_GOOGLE_ULDREN
bool "-> Uldren"
diff --git a/src/mainboard/google/brya/variants/trulo/include/variant/ec.h b/src/mainboard/google/brya/variants/trulo/include/variant/ec.h
new file mode 100644
index 0000000..7a2a6ff
--- /dev/null
+++ b/src/mainboard/google/brya/variants/trulo/include/variant/ec.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef __VARIANT_EC_H__
+#define __VARIANT_EC_H__
+
+#include <baseboard/ec.h>
+
+#endif
diff --git a/src/mainboard/google/brya/variants/trulo/include/variant/gpio.h b/src/mainboard/google/brya/variants/trulo/include/variant/gpio.h
new file mode 100644
index 0000000..c4fe342
--- /dev/null
+++ b/src/mainboard/google/brya/variants/trulo/include/variant/gpio.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef VARIANT_GPIO_H
+#define VARIANT_GPIO_H
+
+#include <baseboard/gpio.h>
+
+#endif
diff --git a/src/mainboard/google/brya/variants/trulo/memory/Makefile.mk b/src/mainboard/google/brya/variants/trulo/memory/Makefile.mk
new file mode 100644
index 0000000..850a62c
--- /dev/null
+++ b/src/mainboard/google/brya/variants/trulo/memory/Makefile.mk
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# This is an auto-generated file. Do not edit!!
+# Generated by:
+# /tmp/go-build204585115/b001/exe/part_id_gen ADL lp5 src/mainboard/google/brya/variants/trulo/memory src/mainboard/google/brya/variants/trulo/memory/mem_parts_used.txt
+
+SPD_SOURCES = placeholder
diff --git a/src/mainboard/google/brya/variants/trulo/memory/dram_id.generated.txt b/src/mainboard/google/brya/variants/trulo/memory/dram_id.generated.txt
new file mode 100644
index 0000000..8ce2480
--- /dev/null
+++ b/src/mainboard/google/brya/variants/trulo/memory/dram_id.generated.txt
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# This is an auto-generated file. Do not edit!!
+# Generated by:
+# /tmp/go-build204585115/b001/exe/part_id_gen ADL lp5 src/mainboard/google/brya/variants/trulo/memory src/mainboard/google/brya/variants/trulo/memory/mem_parts_used.txt
+
+DRAM Part Name ID to assign
diff --git a/src/mainboard/google/brya/variants/trulo/memory/mem_parts_used.txt b/src/mainboard/google/brya/variants/trulo/memory/mem_parts_used.txt
new file mode 100644
index 0000000..2499005
--- /dev/null
+++ b/src/mainboard/google/brya/variants/trulo/memory/mem_parts_used.txt
@@ -0,0 +1,11 @@
+# This is a CSV file containing a list of memory parts used by this variant.
+# One part per line with an optional fixed ID in column 2.
+# Only include a fixed ID if it is required for legacy reasons!
+# Generated IDs are dependent on the order of parts in this file,
+# so new parts must always be added at the end of the file!
+#
+# Generate an updated Makefile.mk and dram_id.generated.txt by running the
+# part_id_gen tool from util/spd_tools.
+# See util/spd_tools/README.md for more details and instructions.
+
+# Part Name
diff --git a/src/mainboard/google/brya/variants/trulo/overridetree.cb b/src/mainboard/google/brya/variants/trulo/overridetree.cb
new file mode 100644
index 0000000..ee86142
--- /dev/null
+++ b/src/mainboard/google/brya/variants/trulo/overridetree.cb
@@ -0,0 +1,4 @@
+chip soc/intel/alderlake
+ device domain 0 on
+ end
+end
--
To view, visit https://review.coreboot.org/c/coreboot/+/81627?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: I91157d252ef56c8938bfc08ed0f734c5dc7e614d
Gerrit-Change-Number: 81627
Gerrit-PatchSet: 20
Gerrit-Owner: 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: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: merged
Attention is currently required from: Dinesh Gehlot, Eric Lai, Nick Vaccaro, Sumeet R Pawnikar, Varshit Pandya.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81626?usp=email )
Change subject: mb/google/brya: Add new baseboard trulo
......................................................................
Patch Set 17:
(1 comment)
File src/mainboard/google/brya/Kconfig:
https://review.coreboot.org/c/coreboot/+/81626/comment/88c59460_77393b72 :
PS16, Line 111: SOC_INTEL_ALDERLAKE_PCH_N
> Any specific reason for this removal? We have this for Nissa which is the similar design like Trulo.
Nissa is with ADL-N and Trulo is with TWL
btw, we haven't removed the ADL-N config it just avoided the redundant selection as SOC_INTEL_TWINLAKE inherits SOC_INTEL_ALDERLAKE_PCH_N anyway,
```
config SOC_INTEL_TWINLAKE
bool
select SOC_INTEL_ALDERLAKE_PCH_N
help
Intel Twinlake support. Mainboards using TWL should select
SOC_INTEL_TWINLAKE.
```
--
To view, visit https://review.coreboot.org/c/coreboot/+/81626?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: Iad6230064c6b8359698d37c3e0440614cc7b073d
Gerrit-Change-Number: 81626
Gerrit-PatchSet: 17
Gerrit-Owner: 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: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Reviewer: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Eric Lai <ericllai(a)google.com>
Gerrit-Attention: Dinesh Gehlot <digehlot(a)google.com>
Gerrit-Attention: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Comment-Date: Thu, 11 Apr 2024 11:55:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sumeet R Pawnikar <sumeet.r.pawnikar(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Varshit Pandya.
Máté Kukri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81529?usp=email )
Change subject: mb/dell/optiplex_9020: Implement late HWM initialization
......................................................................
Patch Set 5:
(7 comments)
Patchset:
PS5:
Left some quick comments, I'll fix the issues late tonight, or tomorrow.
File src/mainboard/dell/optiplex_9020/mainboard.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/144a5c80_8907d081 :
PS5, Line 290: die("Unknown GPIO chassis type\n");
> Why not return some fallback default value instead? There's nothing stopping coreboot from continuin […]
I am fine with turning these into warning.
https://review.coreboot.org/c/coreboot/+/81529/comment/8e6f8503_b594f8fd :
PS5, Line 355: case 3:
> Would it make sense to use an enum? Or are these "type" numbers magic?
These are the same chassis type values the vendor firmware uses. I kept them to make it easier to cross reference in case any bugs pop up on the chassis types i dont have.
https://review.coreboot.org/c/coreboot/+/81529/comment/666812be_a92c9be7 :
PS5, Line 368: die("Unknown chassis type\n");
> Same here, is there any harm in not programming any table?
No table results in the fan running at a low constant speed.
https://review.coreboot.org/c/coreboot/+/81529/comment/0630ca2b_a6ba66bf :
PS5, Line 371: if (CONFIG_MAX_CPUS > 2) {
> This is a compile-time check. […]
The vendor firmware bases this check on "max core address MSR" > 2 (i dont remember the id off the top of my head). on my system that returns 7, despite having a quad core cpu.
I didn't check with a dual core, but it might be fine to remove this check as I don't think any supported cpu would return < 2
File src/mainboard/dell/optiplex_9020/sch5555_ec.h:
https://review.coreboot.org/c/coreboot/+/81529/comment/9cdfb485_13d732a4 :
PS5, Line 3: #pragma once
> Hmmm, I thought we were supposed to use traditional include guards. […]
Indeed, I missed that as this file copy pasted from my user-space utility.
File src/mainboard/dell/optiplex_9020/sch5555_ec.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/df098fcb_8088718b :
PS5, Line 51: for (size_t timeout = 0; timeout < 0xfff; ++timeout)
: if (inb(SCH555x_EMI_IOBASE + 1) & 1)
: break;
> No delay in the polling loop?
Vendor firmware doesn't use any delay either. LPC traffic shows a couple dozen iterations always being enough, so i think this is fine here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/81529?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: Ibdccd3fc7364e03e84ca606592928410624eed43
Gerrit-Change-Number: 81529
Gerrit-PatchSet: 5
Gerrit-Owner: Máté Kukri <kukri.mate(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 11 Apr 2024 11:50:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Máté Kukri, Varshit Pandya.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81529?usp=email )
Change subject: mb/dell/optiplex_9020: Implement late HWM initialization
......................................................................
Patch Set 5:
(6 comments)
File src/mainboard/dell/optiplex_9020/mainboard.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/93c6a2d1_db420594 :
PS5, Line 290: die("Unknown GPIO chassis type\n");
Why not return some fallback default value instead? There's nothing stopping coreboot from continuing to boot, even if the fans don't cooperate.
https://review.coreboot.org/c/coreboot/+/81529/comment/152a91f0_b3ace8b6 :
PS5, Line 355: case 3:
Would it make sense to use an enum? Or are these "type" numbers magic?
https://review.coreboot.org/c/coreboot/+/81529/comment/2aefdcb5_bead46be :
PS5, Line 368: die("Unknown chassis type\n");
Same here, is there any harm in not programming any table?
https://review.coreboot.org/c/coreboot/+/81529/comment/92684024_dba255a2 :
PS5, Line 371: if (CONFIG_MAX_CPUS > 2) {
This is a compile-time check. If this is about applying different values depending on the number of cores (or threads?) of the CPU, it would make more sense to query the information at runtime.
File src/mainboard/dell/optiplex_9020/sch5555_ec.h:
https://review.coreboot.org/c/coreboot/+/81529/comment/ccb820eb_39b954c1 :
PS5, Line 3: #pragma once
Hmmm, I thought we were supposed to use traditional include guards. Did the coding style change?
File src/mainboard/dell/optiplex_9020/sch5555_ec.c:
https://review.coreboot.org/c/coreboot/+/81529/comment/5add389b_b100fd0a :
PS5, Line 51: for (size_t timeout = 0; timeout < 0xfff; ++timeout)
: if (inb(SCH555x_EMI_IOBASE + 1) & 1)
: break;
No delay in the polling loop?
--
To view, visit https://review.coreboot.org/c/coreboot/+/81529?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: Ibdccd3fc7364e03e84ca606592928410624eed43
Gerrit-Change-Number: 81529
Gerrit-PatchSet: 5
Gerrit-Owner: Máté Kukri <kukri.mate(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-CC: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Varshit Pandya <pandyavarshit(a)gmail.com>
Gerrit-Attention: Máté Kukri <kukri.mate(a)gmail.com>
Gerrit-Comment-Date: Thu, 11 Apr 2024 11:38:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment