Attention is currently required from: Jérémy Compostella, Nicholas Chin, Subrata Banik.
Julius Werner has posted comments on this change by Nicholas Chin. ( https://review.coreboot.org/c/coreboot/+/85905?usp=email )
Change subject: drivers/option: Add CBFS file based option backend
......................................................................
Patch Set 13:
(1 comment)
Patchset:
PS4:
> First, we are forcing all previous and future generations of chromeos devices to bind with USE_CBFS_FILE_OPTION_BACKEND.
I know but I don't think there's really a drawback to enabling this. This also allows controlling other things (e.g. coreboot loglevel) via options, and it's cleaner to have that enabled for all devices than to only have it work on a few with no clear reason for the difference.
> Second, what about non-CrOS devices? They might also wish to use the USE_CBFS_FILE_OPTION_BACKEND feature. The current model of relying on HAVE_ would allow any SoC/Mainboard to select this feature as per their need.
Yes, but selecting this from the mainboard Kconfig is fundamentally the wrong way to go about it. Mainboard Kconfigs are only supposed to describe hardware, not decide policy. Whether this feature is enabled or not is clearly a policy decision and not a property of the hardware (same as enabling timestamps and those kinds of options). That means it should be decided by the user in menuconfig instead (in the ChromeOS case, that means our downstream ebuilds), and there's no good reason why some hardware should have different defaults than other here.
--
To view, visit https://review.coreboot.org/c/coreboot/+/85905?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: Ifc0439ee42f13f49ae54d4855d1d9333c39b01f5
Gerrit-Change-Number: 85905
Gerrit-PatchSet: 13
Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Julius Werner <jwerner(a)chromium.org>
Gerrit-CC: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-CC: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Jérémy Compostella <jeremy.compostella(a)intel.com>
Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com>
Gerrit-Comment-Date: Thu, 16 Jan 2025 22:51:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com>
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com>
Attention is currently required from: Ariel Chang, Matt DeVillier, Nick Vaccaro, Paul Menzel, Paul Yang, Wayne3 Wang.
Ingo Reitz has posted comments on this change by Ingo Reitz. ( https://review.coreboot.org/c/coreboot/+/86026?usp=email )
Change subject: mb/google/volteer/drobit: fix power_limits_config
......................................................................
Patch Set 4:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/86026/comment/c6bd280b_24f3c7d5?us… :
PS4, Line 9: Drobit shows little power usage and very low clock speeds under load,
: despite being at very low temperatures. It turns out that
: power_limits_config is set to the lower end of the dptf power limit
: ranges as opposed to baseboard and other variants. This seems to
: prevent the device from using the intended power limits.
> Commit a5761efd14a2 (mb/google/volteer/variants/drobit: Update DPTF parameters), decreased `. […]
i was unable to find any documentation on the changed values, but from what i observed the pl1 max value in line 82 (and 87 for pl2) seems to be overruled by power_limits_config, which made me question these 2 values in lines 82 and 87
--
To view, visit https://review.coreboot.org/c/coreboot/+/86026?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: Id0478c713b51db4972e7d93ec597a30fa885c22b
Gerrit-Change-Number: 86026
Gerrit-PatchSet: 4
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Gerrit-Reviewer: Ariel Chang <ariel_chang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Paul Yang <paul.f.yang(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Wayne3 Wang <wayne3_wang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Wayne3 Wang <wayne3_wang(a)pegatron.corp-partner.google.com>
Gerrit-Attention: Paul Yang <paul.f.yang(a)intel.corp-partner.google.com>
Gerrit-Attention: Ariel Chang <ariel_chang(a)pegatron.corp-partner.google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Comment-Date: Thu, 16 Jan 2025 22:45:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org>
Attention is currently required from: Ariel Chang, Ingo Reitz, Matt DeVillier, Nick Vaccaro, Paul Yang, Wayne3 Wang.
Paul Menzel has posted comments on this change by Ingo Reitz. ( https://review.coreboot.org/c/coreboot/+/86026?usp=email )
Change subject: mb/google/volteer/drobit: fix power_limits_config
......................................................................
Patch Set 4:
(9 comments)
Patchset:
PS4:
How to access the Chromium issue `BUG=b:177777472`?
Commit Message:
https://review.coreboot.org/c/coreboot/+/86026/comment/fd1adec2_17132e6c?us… :
PS4, Line 7: volteer/drobit
Add variants in between?
https://review.coreboot.org/c/coreboot/+/86026/comment/21eed1ff_674d831f?us… :
PS4, Line 9: little power usage and very low clock speeds under load
Concrete values would be nice.
https://review.coreboot.org/c/coreboot/+/86026/comment/aaa229be_289c1d95?us… :
PS4, Line 9: Drobit shows little power usage and very low clock speeds under load,
: despite being at very low temperatures. It turns out that
: power_limits_config is set to the lower end of the dptf power limit
: ranges as opposed to baseboard and other variants. This seems to
: prevent the device from using the intended power limits.
Commit a5761efd14a2 (mb/google/volteer/variants/drobit: Update DPTF parameters), decreased `.tdp_pl1_override` from 13 to 9. Do you know if there is another commit in some Chromium branch changes the values? I’d have thought, Google has good test coverage that they would have detected such a problem.
https://review.coreboot.org/c/coreboot/+/86026/comment/f88afac2_27574989?us… :
PS4, Line 15: correct power usage, clock speeds temperature
It’d be great if you added the specific values.
https://review.coreboot.org/c/coreboot/+/86026/comment/74085b38_dba82488?us… :
PS4, Line 15: temperature
It‘d be great if you could be more specific.
https://review.coreboot.org/c/coreboot/+/86026/comment/28953e5b_b4ac031d?us… :
PS4, Line 15: speeds
Comma after *speeds*?
https://review.coreboot.org/c/coreboot/+/86026/comment/01d44531_62e68d51?us… :
PS4, Line 15: Tested
No idea if there are different CPU models in drobit, but it’d be great if you listed yours.
https://review.coreboot.org/c/coreboot/+/86026/comment/b5f9e5db_82e024a6?us… :
PS4, Line 16: 100% load
How do you create this load?
--
To view, visit https://review.coreboot.org/c/coreboot/+/86026?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: Id0478c713b51db4972e7d93ec597a30fa885c22b
Gerrit-Change-Number: 86026
Gerrit-PatchSet: 4
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Gerrit-Reviewer: Ariel Chang <ariel_chang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Paul Yang <paul.f.yang(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Wayne3 Wang <wayne3_wang(a)pegatron.corp-partner.google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Wayne3 Wang <wayne3_wang(a)pegatron.corp-partner.google.com>
Gerrit-Attention: Paul Yang <paul.f.yang(a)intel.corp-partner.google.com>
Gerrit-Attention: Ariel Chang <ariel_chang(a)pegatron.corp-partner.google.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Ingo Reitz <9l(a)9lo.re>
Gerrit-Comment-Date: Thu, 16 Jan 2025 22:37:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Angel Pons, Filip Brozovic.
Matt DeVillier has posted comments on this change by Filip Brozovic. ( https://review.coreboot.org/c/coreboot/+/85987?usp=email )
Change subject: CFR: add dependencies based on specific option values
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/85987?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: Iaf7965551490969052eb27c207fa524470d4dd6a
Gerrit-Change-Number: 85987
Gerrit-PatchSet: 1
Gerrit-Owner: Filip Brozovic <fbrozovic(a)gmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Filip Brozovic <fbrozovic(a)gmail.com>
Gerrit-Comment-Date: Thu, 16 Jan 2025 22:08:16 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Attention is currently required from: Nick Vaccaro.
Hello Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/86026?usp=email
to look at the new patch set (#4).
Change subject: mb/google/volteer/drobit: fix power_limits_config
......................................................................
mb/google/volteer/drobit: fix power_limits_config
Drobit shows little power usage and very low clock speeds under load,
despite being at very low temperatures. It turns out that
power_limits_config is set to the lower end of the dptf power limit
ranges as opposed to baseboard and other variants. This seems to
prevent the device from using the intended power limits.
Tested: Boot and confirm correct power usage, clock speeds temperature
and stability under 100% load
Change-Id: Id0478c713b51db4972e7d93ec597a30fa885c22b
Signed-off-by: Ingo Reitz <9l(a)9lo.re>
---
M src/mainboard/google/volteer/variants/drobit/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/86026/4
--
To view, visit https://review.coreboot.org/c/coreboot/+/86026?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: Id0478c713b51db4972e7d93ec597a30fa885c22b
Gerrit-Change-Number: 86026
Gerrit-PatchSet: 4
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Attention is currently required from: Nick Vaccaro.
Hello Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/86026?usp=email
to look at the new patch set (#3).
Change subject: mb/google/volteer/drobit: fix power_limits_config
......................................................................
mb/google/volteer/drobit: fix power_limits_config
Drobit shows little power usage and very low clock speeds under load,
despite being at very low temperatures. It turns out that
power_limits_config is set to the lower end of the dptf power limit
ranges as opposed to baseboard and other variants. This seems to prevent
the device from using the intended power limits.
Tested: Boot and confirm correct power usage, clock speeds temperature and stability under 100% load
Change-Id: Id0478c713b51db4972e7d93ec597a30fa885c22b
Signed-off-by: Ingo Reitz <9l(a)9lo.re>
---
M src/mainboard/google/volteer/variants/drobit/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/86026/3
--
To view, visit https://review.coreboot.org/c/coreboot/+/86026?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: Id0478c713b51db4972e7d93ec597a30fa885c22b
Gerrit-Change-Number: 86026
Gerrit-PatchSet: 3
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Attention is currently required from: Nick Vaccaro.
Hello Nick Vaccaro,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/86026?usp=email
to look at the new patch set (#2).
Change subject: mb/google/volteer/drobit: fix power_limits_config
......................................................................
mb/google/volteer/drobit: fix power_limits_config
Drobit showed little power usage and very low clock speeds under load,
despite being at very low temperatures. It turns out that
power_limits_config was set to the lower end of the dptf power limit
ranges as opposed to baseboard and other variants. This seems to prevent
the device from using the intended power limits.
Tested: Booted and confirmed correct power usage, clock speeds temperature and stability under 100% load
Change-Id: Id0478c713b51db4972e7d93ec597a30fa885c22b
Signed-off-by: Ingo Reitz <9l(a)9lo.re>
---
M src/mainboard/google/volteer/variants/drobit/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/86026/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/86026?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: Id0478c713b51db4972e7d93ec597a30fa885c22b
Gerrit-Change-Number: 86026
Gerrit-PatchSet: 2
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Ingo Reitz has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/86026?usp=email )
Change subject: mb/google/volteer/drobit> fix power_limits_config
......................................................................
mb/google/volteer/drobit> fix power_limits_config
Drobit showed little power usage and very low clock speeds under load,
despite being at very low temperatures. It turns out that
power_limits_config was set to the lower end of the dptf power limit
ranges as opposed to baseboard and other variants. This seems to prevent
the device from using the intended power limits.
Tested: Booted and confirmed correct power usage, clock speeds temperature and stability under 100% load
Change-Id: Id0478c713b51db4972e7d93ec597a30fa885c22b
Signed-off-by: Ingo Reitz <9l(a)9lo.re>
---
M src/mainboard/google/volteer/variants/drobit/overridetree.cb
1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/86026/1
diff --git a/src/mainboard/google/volteer/variants/drobit/overridetree.cb b/src/mainboard/google/volteer/variants/drobit/overridetree.cb
index c791079..148af6e 100644
--- a/src/mainboard/google/volteer/variants/drobit/overridetree.cb
+++ b/src/mainboard/google/volteer/variants/drobit/overridetree.cb
@@ -12,8 +12,8 @@
register "tcc_offset" = "8"
register "power_limits_config[POWER_LIMITS_U_4_CORE]" = "{
- .tdp_pl1_override = 9,
- .tdp_pl2_override = 28,
+ .tdp_pl1_override = 17,
+ .tdp_pl2_override = 64,
.tdp_pl4 = 105,
}"
--
To view, visit https://review.coreboot.org/c/coreboot/+/86026?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: Id0478c713b51db4972e7d93ec597a30fa885c22b
Gerrit-Change-Number: 86026
Gerrit-PatchSet: 1
Gerrit-Owner: Ingo Reitz <9l(a)9lo.re>
Attention is currently required from: Angel Pons, Intel coreboot Reviewers.
Riku Viitanen has posted comments on this change by Riku Viitanen. ( https://review.coreboot.org/c/coreboot/+/85793?usp=email )
Change subject: nb/sandybridge: Implement adjustable DRAM voltages
......................................................................
Patch Set 12:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/coreboot/+/85793?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: I1a8857deee85fd635429afd3cbf93cad7a7d589b
Gerrit-Change-Number: 85793
Gerrit-PatchSet: 12
Gerrit-Owner: Riku Viitanen <riku.viitanen(a)protonmail.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Intel coreboot Reviewers <intel_coreboot_reviewers(a)intel.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Thu, 16 Jan 2025 21:51:58 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No