coreboot-gerrit June 2024

coreboot-gerrit@coreboot.org

1 participants
3527 discussions

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow package power limit clamping
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Angel Pons. Anastasios Koutian has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83270?usp=email ) Change subject: cpu/intel/model_206ax: Allow package power limit clamping ...................................................................... Patch Set 2: (1 comment) File src/cpu/intel/model_206ax/model_206ax_init.c: https://review.coreboot.org/c/coreboot/+/83270/comment/69d6534f_6b16ccf7?us… : PS2, Line 160: if(conf->pl2_clamp) { > ```suggestion […] Sorry for these formatting errors, is there an auto-formatting hook that is used by coreboot? This is my first contribution -- To view, visit https://review.coreboot.org/c/coreboot/+/83270?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: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Gerrit-Change-Number: 83270 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 14:05:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasios Koutian <akoutian2(a)gmail.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow turbo boost ratio limit configuration
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Angel Pons. Anastasios Koutian has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83271?usp=email ) Change subject: cpu/intel/model_206ax: Allow turbo boost ratio limit configuration ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: > Also see CB:42547 which admittedly I haven't really taken care of. Thank you, I saw your change but assumed it to be stale so I decided to start fresh. Commit Message: https://review.coreboot.org/c/coreboot/+/83271/comment/3b610378_4431a537?us… : PS2, Line 9: Tested on ThinkPad T420 with the i7-3940XM. > Hmmm, I don't see any changes to the T420. […] This branch is intended to enable setting the values in devicetree.cb. I haven't included the changes to T420 devicetree.cb in this branch, since I wanted to keep it single-topic, but I have tested it locally and it works. I have a separate branch with changes to T420 devicetree.cb which use the vendor default values for TCC offset, PL1/PL2 and current limits. I intend to push this for review after this branch is merged. That being said, I also like the idea of making these CMOS options. That way users don't need to recompile and flash in order to change power limits. Perhaps we can think about it. -- To view, visit https://review.coreboot.org/c/coreboot/+/83271?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: I1c65a129478e8ac2c4f66eb3c6aa2507358f82ad Gerrit-Change-Number: 83271 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 14:03:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>

1 0

[XS] Change in coreboot[main]: nb/intel/sandybridge/chipset.cb: Add alias for `cpu_cluster`
by Angel Pons (Code Review) 30 Jun '24

30 Jun '24

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/83275?usp=email ) Change subject: nb/intel/sandybridge/chipset.cb: Add alias for `cpu_cluster` ...................................................................... nb/intel/sandybridge/chipset.cb: Add alias for `cpu_cluster` Define a devicetree alias for `cpu_cluster` so that it can be referenced in C code as `DEV_PTR(cpu_bus)`. Change-Id: Id6ead3d98d8fc17cab44ecf0b2af60a23187e036 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M src/northbridge/intel/sandybridge/chipset.cb 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/83275/1 diff --git a/src/northbridge/intel/sandybridge/chipset.cb b/src/northbridge/intel/sandybridge/chipset.cb index 22a0e7b..ea917a9 100644 --- a/src/northbridge/intel/sandybridge/chipset.cb +++ b/src/northbridge/intel/sandybridge/chipset.cb @@ -2,7 +2,7 @@ chip northbridge/intel/sandybridge chip cpu/intel/model_206ax - device cpu_cluster 0 on ops sandybridge_cpu_bus_ops end + device cpu_cluster 0 alias cpu_bus on ops sandybridge_cpu_bus_ops end register "acpi_c1" = "CPU_ACPI_C1" register "acpi_c2" = "CPU_ACPI_C3" -- To view, visit https://review.coreboot.org/c/coreboot/+/83275?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: Id6ead3d98d8fc17cab44ecf0b2af60a23187e036 Gerrit-Change-Number: 83275 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow PL1/PL2 configuration
by Angel Pons (Code Review) 30 Jun '24

30 Jun '24

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow turbo boost ratio limit configuration
by Angel Pons (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Anastasios Koutian. Angel Pons has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83271?usp=email ) Change subject: cpu/intel/model_206ax: Allow turbo boost ratio limit configuration ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: Also see CB:42547 which admittedly I haven't really taken care of. Commit Message: https://review.coreboot.org/c/coreboot/+/83271/comment/badb8fe0_42df4d8c?us… : PS2, Line 9: Tested on ThinkPad T420 with the i7-3940XM. Hmmm, I don't see any changes to the T420. If these settings are meant to be user-configurable, perhaps they should be Kconfig options and/or CMOS options. The option API's "get" functions take a fallback value as parameter, which is returned when the selected option backend fails to read the option's value (the "null" backend always fails). This fallback value can be a Kconfig option, so a common pattern is the following: ``` const unsigned int some_value = get_uint_option("some_option", CONFIG_SOME_OPTION); ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/83271?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: I1c65a129478e8ac2c4f66eb3c6aa2507358f82ad Gerrit-Change-Number: 83271 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 13:50:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow PL1/PL2 configuration
by Nico Huber (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Anastasios Koutian, Angel Pons, Patrick Rudolph. Nico Huber has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83269?usp=email ) Change subject: cpu/intel/model_206ax: Allow PL1/PL2 configuration ...................................................................... Patch Set 1: (1 comment) File src/cpu/intel/model_206ax/model_206ax_init.c: https://review.coreboot.org/c/coreboot/+/83269/comment/3b7ff531_c1c38d20?us… : PS1, Line 99: dev->upstream->dev->upstream->children->chip_info; > I'm not too fond of walking the hierarchy like this but I don't know of a better way to handle it `dev->chip_info` should work as well as it's all one chip, northbridge & CPU. -- To view, visit https://review.coreboot.org/c/coreboot/+/83269?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: I064af25ec4805fae755eea52c4c9c6d4386c0aee Gerrit-Change-Number: 83269 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 13:40:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow package power limit clamping
by Angel Pons (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Anastasios Koutian. Angel Pons has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83270?usp=email ) Change subject: cpu/intel/model_206ax: Allow package power limit clamping ...................................................................... Patch Set 2: (3 comments) File src/cpu/intel/model_206ax/model_206ax_init.c: https://review.coreboot.org/c/coreboot/+/83270/comment/f7e63cf6_dc32d1a6?us… : PS2, Line 144: if(conf->pl2_clamp) { > > `space required before the open parenthesis '('` […] ```suggestion if (conf->pl2_clamp) { ``` https://review.coreboot.org/c/coreboot/+/83270/comment/a24924ab_de1d884f?us… : PS2, Line 153: if (conf->pl2){ > > `space required before the open brace '{'` […] ```suggestion if (conf->pl2) { ``` https://review.coreboot.org/c/coreboot/+/83270/comment/d03213ed_55335a01?us… : PS2, Line 160: if(conf->pl2_clamp) { > > `space required before the open parenthesis '('` […] ```suggestion if (conf->pl2_clamp) { ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/83270?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: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Gerrit-Change-Number: 83270 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 13:39:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasios Koutian <akoutian2(a)gmail.com>

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow PL1/PL2 configuration
by Angel Pons (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Anastasios Koutian, Nico Huber, Patrick Rudolph. Angel Pons has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83269?usp=email ) Change subject: cpu/intel/model_206ax: Allow PL1/PL2 configuration ...................................................................... Patch Set 1: (8 comments) File src/cpu/intel/model_206ax/chip.h: https://review.coreboot.org/c/coreboot/+/83269/comment/77813df1_b9d86afc?us… : PS1, Line 46: int pl1; /* Long-term power limit*/ : int pl2; /* Short-term power limit*/ nit: space before comment end Also, which units are these values in? I would at least make these unsigned, and possibly suffix the units: ```suggestion unsigned int pl1; /* Long-term power limit */ unsigned int pl2; /* Short-term power limit */ ``` File src/cpu/intel/model_206ax/model_206ax.h: https://review.coreboot.org/c/coreboot/+/83269/comment/79ed9a71_039e0706?us… : PS1, Line 132: void set_power_limits(u8 power_limit_1_time, struct device *); ```suggestion void set_power_limits(u8 power_limit_1_time, struct device *dev); ``` File src/cpu/intel/model_206ax/model_206ax_init.c: https://review.coreboot.org/c/coreboot/+/83269/comment/b4d6c47d_e78c8ae0?us… : PS1, Line 99: dev->upstream->dev->upstream->children->chip_info; I'm not too fond of walking the hierarchy like this but I don't know of a better way to handle it https://review.coreboot.org/c/coreboot/+/83269/comment/6ddccf02_78ffccad?us… : PS1, Line 137: if(conf->pl1) { ```suggestion if (conf->pl1) { ``` https://review.coreboot.org/c/coreboot/+/83269/comment/f533289d_47fc89f5?us… : PS1, Line 138: dev_path(dev) This is a bit confusing because it would print the northbridge's path from CPU code. https://review.coreboot.org/c/coreboot/+/83269/comment/e158f1b0_2d406066?us… : PS1, Line 139: limit.lo |= ((conf->pl1 * power_unit) / 1000000) & PKG_POWER_LIMIT_MASK; This could theoretically overflow, and the resulting value would then be truncated. Not sure what happens if power limits are too low (e.g. zero or one). https://review.coreboot.org/c/coreboot/+/83269/comment/916879ff_a21941a6?us… : PS1, Line 149: if(conf->pl2){ ```suggestion if (conf->pl2) { ``` File src/northbridge/intel/sandybridge/northbridge.c: https://review.coreboot.org/c/coreboot/+/83269/comment/d92cf5f5_e2433a5e?us… : PS1, Line 354: 28 Would it make sense to turn this into another devicetree setting? Doesn't have to be in this change. -- To view, visit https://review.coreboot.org/c/coreboot/+/83269?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: I064af25ec4805fae755eea52c4c9c6d4386c0aee Gerrit-Change-Number: 83269 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 13:37:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow package power limit clamping
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Anastasios Koutian has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83270?usp=email ) Change subject: cpu/intel/model_206ax: Allow package power limit clamping ...................................................................... Patch Set 2: (3 comments) File src/cpu/intel/model_206ax/model_206ax_init.c: https://review.coreboot.org/c/coreboot/+/83270/comment/5226d85e_d63f61d3?us… : PS2, Line 144: if(conf->pl2_clamp) { > `space required before the open parenthesis '('` Please fix. https://review.coreboot.org/c/coreboot/+/83270/comment/2e88f71f_41198222?us… : PS2, Line 153: if (conf->pl2){ > `space required before the open brace '{'` Please fix. https://review.coreboot.org/c/coreboot/+/83270/comment/bf3841ab_b8beb5b2?us… : PS2, Line 160: if(conf->pl2_clamp) { > `space required before the open parenthesis '('` Please fix. -- To view, visit https://review.coreboot.org/c/coreboot/+/83270?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: Id0c0aedc29aca121d0fd1d8f8826089e13a026be Gerrit-Change-Number: 83270 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Sun, 30 Jun 2024 13:30:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in coreboot[main]: cpu/intel/model_206ax: Allow PL1/PL2 configuration
by Anastasios Koutian (Code Review) 30 Jun '24

30 Jun '24

Attention is currently required from: Angel Pons, Patrick Rudolph. Anastasios Koutian has posted comments on this change by Anastasios Koutian. ( https://review.coreboot.org/c/coreboot/+/83269?usp=email ) Change subject: cpu/intel/model_206ax: Allow PL1/PL2 configuration ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Hi! This branch contains some features that I have been using in my T420 and have found them to be useful. In particular, when using the i7-3940XM CPU, which is quite powerful, it is necessary to set power and current limits in order to avoid system instability. I am sharing these patches hoping that they will be useful for other ThinkPad users. -- To view, visit https://review.coreboot.org/c/coreboot/+/83269?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: I064af25ec4805fae755eea52c4c9c6d4386c0aee Gerrit-Change-Number: 83269 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasios Koutian <akoutian2(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 30 Jun 2024 13:28:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

← Newer
1
...
4
5
6
7
8
9
10
...
353
Older →

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2024