Attention is currently required from: Werner Zeh.
Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63547 )
Change subject: soc/intel/elkhartlake/systemagent: Disable RAPL based on Kconfig
......................................................................
Patch Set 6:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63547/comment/2c436583_a76c1bd4
PS2, Line 7: soc/intel/elkhartlake/systemagent: possibility of deactivate RAPL
> soc/intel/elkhartlake: Disable RAPL based on Kconfig
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/1347ec41_51e98d9a
PS2, Line 9: deactivate
> disable
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/47c15a4a_18303a15
PS2, Line 10: as in APL based boards
> SOC_INTEL_DISABLE_POWER_LIMITS
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/ca56c2d9_16198939
PS2, Line 10: The only
: difference in EHL is the necessary usage of an MCHBAR register instead
: the relevant MSR (Intel changes EDS at the moment).
> Other than on previouse SOCs this needs to be done in an MCHBAR mapped register rather than via MSR […]
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/5452dc6a_9cb84495
PS2, Line 15: On siemens/mc_ehl1 checking the MCHBAR register with and without the
: relevant config switch.
> Check MCHBAR mapped registers (MCH_PKG_POWER_LIMIT) on mc_ehl1.
Done
File src/soc/intel/elkhartlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/63547/comment/e3da2248_71789467
PS2, Line 51: u32
> elkhartlake code usually uses uint{8,16,32}_t types. I would do it here, too to stay consistent.
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/3db96762_b0016772
PS2, Line 61: Skip setting RAPL per configuration\n
> Here you actually disable RAPL directly, so maybe "Disable RAPL" would be more clear here?
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/63547
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5be6632b15ab8e14a21b5cd35152f82fec919d9f
Gerrit-Change-Number: 63547
Gerrit-PatchSet: 6
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Mon, 02 May 2022 07:00:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Uwe Poeche, Werner Zeh, Andrey Petrov.
Uwe Poeche has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63546 )
Change subject: intel/common/block: Provide RAPL and min clock ratio switches in common
......................................................................
Patch Set 6:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63546/comment/95f405cf_902658be
PS2, Line 7: provide config switches from APL specific
> This could be formulated more clear, e.g.: […]
Done
https://review.coreboot.org/c/coreboot/+/63546/comment/a9f9c0a1_227c2111
PS2, Line 10: APL_SKIP_SET_POWER_LIMITS
> APL_SET_MIN_CLOCK_RATIO
Done
https://review.coreboot.org/c/coreboot/+/63546/comment/c220a50f_e4952352
PS2, Line 12: Therefore they moved here with other name to common/block.
> Maybe 'Move them to common code instead of having them just for one SOC'
Done
https://review.coreboot.org/c/coreboot/+/63546/comment/11221b66_f5e6078a
PS2, Line 14: mainboards mc_apl1/4/5: compare cpu clock via MSR 0x198 and RAPL
: settings via MSR 0x610 before and after the change.
> Maybe: […]
Done
File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/63546/comment/bed210f6_2e1fb4cd
PS2, Line 144: Some Apollo Lake mainboards do not need the Running Average Power
: Limits (RAPL) algorithm for a constant power management.
: Set this config option to skip the RAPL configuration.
> I would rephrase this text as it is not APL specific anymore
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/63546
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3d63d1b9b6c96586a3c20bf8c1d8001b1d7c4bed
Gerrit-Change-Number: 63546
Gerrit-PatchSet: 6
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Attention: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Comment-Date: Mon, 02 May 2022 07:00:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Uwe Poeche.
Hello build bot (Jenkins), Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/63548
to look at the new patch set (#7).
Change subject: mb/siemens/mc_ehl: Disable RAPL
......................................................................
mb/siemens/mc_ehl: Disable RAPL
Disable RAPL for all mainboards based on mc_ehl.
Test: Boot mc_ehl1 with this patch and ensure the bits in the MCBAR
register are cleared.
Change-Id: Ie58a4b6444d5be088ac2b25ff0a2f5cd33120ace
Signed-off-by: Uwe Poeche <uwe.poeche(a)siemens.com>
---
M src/mainboard/siemens/mc_ehl/Kconfig
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/63548/7
--
To view, visit https://review.coreboot.org/c/coreboot/+/63548
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie58a4b6444d5be088ac2b25ff0a2f5cd33120ace
Gerrit-Change-Number: 63548
Gerrit-PatchSet: 7
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Anjaneya "Reddy" Chagam, Nico Huber, Jonathan Zhang, Johnny Lin, Christian Walter, Tim Wawrzynczak, Michael Niewöhner, Tim Chu.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60543 )
Change subject: soc/intel/skylake: Move FSP_HYPERTHREADING to common Intel Kconfig
......................................................................
Patch Set 6:
(2 comments)
File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/60543/comment/7a5fe0e3_d0678c88
PS6, Line 144: if HAVE_HYPERTHREADING
use "depends on".
File src/soc/intel/xeon_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/60543/comment/e8c649e5_a4320891
PS6, Line 38: HAVE_HYPERTHREADING
Can you select this in patches that hook it up? It looks like only skylake consumes CONFIG_FSP_HYPERTHREADING
--
To view, visit https://review.coreboot.org/c/coreboot/+/60543
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I892d48b488cbf828057f0e9be9edc4352c58bbe7
Gerrit-Change-Number: 60543
Gerrit-PatchSet: 6
Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Anjaneya "Reddy" Chagam <anjaneya.chagam(a)intel.com>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Mon, 02 May 2022 06:55:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Uwe Poeche.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63548 )
Change subject: mb/siemens/mc_ehl: Disable RAPL
......................................................................
Patch Set 6:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63548/comment/d5b63bb2_371b6c55
PS2, Line 9: With that patch RAPL for the mentioned mainboards is deactivated.
> Maybe better: […]
Done
https://review.coreboot.org/c/coreboot/+/63548/comment/3c9b91d0_271f5fcc
PS2, Line 11: mc_ehl1 with and without this patch and check the mchbar
: register.
> Boot mc_ehl1 with this patch and ensure the bits in the MCBAR register are cleared.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/63548
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie58a4b6444d5be088ac2b25ff0a2f5cd33120ace
Gerrit-Change-Number: 63548
Gerrit-PatchSet: 6
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Comment-Date: Mon, 02 May 2022 06:50:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Uwe Poeche.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63547 )
Change subject: soc/intel/elkhartlake/systemagent: Disable RAPL based on Kconfig
......................................................................
Patch Set 6: Code-Review+1
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63547/comment/7ade0dc2_c173feac
PS2, Line 7: soc/intel/elkhartlake/systemagent: possibility of deactivate RAPL
> soc/intel/elkhartlake: Disable RAPL based on Kconfig
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/417e4b05_0d26627a
PS2, Line 9: deactivate
> disable
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/511662e1_98a426cb
PS2, Line 10: as in APL based boards
> SOC_INTEL_DISABLE_POWER_LIMITS
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/a9162613_7a80ec11
PS2, Line 10: The only
: difference in EHL is the necessary usage of an MCHBAR register instead
: the relevant MSR (Intel changes EDS at the moment).
> Other than on previouse SOCs this needs to be done in an MCHBAR mapped register rather than via MSR […]
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/3534ea2c_bde7baff
PS2, Line 15: On siemens/mc_ehl1 checking the MCHBAR register with and without the
: relevant config switch.
> Check MCHBAR mapped registers (MCH_PKG_POWER_LIMIT) on mc_ehl1.
Done
File src/soc/intel/elkhartlake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/63547/comment/48d5c834_8f2d64b3
PS2, Line 51: u32
> elkhartlake code usually uses uint{8,16,32}_t types. I would do it here, too to stay consistent.
Done
https://review.coreboot.org/c/coreboot/+/63547/comment/79c53972_f6eec466
PS2, Line 61: Skip setting RAPL per configuration\n
> Here you actually disable RAPL directly, so maybe "Disable RAPL" would be more clear here?
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/63547
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5be6632b15ab8e14a21b5cd35152f82fec919d9f
Gerrit-Change-Number: 63547
Gerrit-PatchSet: 6
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Comment-Date: Mon, 02 May 2022 06:49:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment
Attention is currently required from: Uwe Poeche, Andrey Petrov.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63546 )
Change subject: intel/common/block: Provide RAPL and min clock ratio switches in common
......................................................................
Patch Set 6: Code-Review+1
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/63546/comment/bb9f7338_dc66c8a8
PS2, Line 7: provide config switches from APL specific
> This could be formulated more clear, e.g.: […]
Done
https://review.coreboot.org/c/coreboot/+/63546/comment/e7b31104_a5822464
PS2, Line 10: APL_SKIP_SET_POWER_LIMITS
> APL_SET_MIN_CLOCK_RATIO
Done
https://review.coreboot.org/c/coreboot/+/63546/comment/92ff3fa1_1020a3b5
PS2, Line 12: Therefore they moved here with other name to common/block.
> Maybe 'Move them to common code instead of having them just for one SOC'
Done
https://review.coreboot.org/c/coreboot/+/63546/comment/63b95ca0_7460ced8
PS2, Line 14: mainboards mc_apl1/4/5: compare cpu clock via MSR 0x198 and RAPL
: settings via MSR 0x610 before and after the change.
> Maybe: […]
Done
File src/soc/intel/common/block/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/63546/comment/c61250b1_b8978d0d
PS2, Line 144: Some Apollo Lake mainboards do not need the Running Average Power
: Limits (RAPL) algorithm for a constant power management.
: Set this config option to skip the RAPL configuration.
> I would rephrase this text as it is not APL specific anymore
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/63546
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3d63d1b9b6c96586a3c20bf8c1d8001b1d7c4bed
Gerrit-Change-Number: 63546
Gerrit-PatchSet: 6
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Comment-Date: Mon, 02 May 2022 06:47:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-MessageType: comment
Hello build bot (Jenkins), Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/63548
to look at the new patch set (#6).
Change subject: mb/siemens/mc_ehl: Disable RAPL
......................................................................
mb/siemens/mc_ehl: Disable RAPL
Disable RAPL for all mainboards based on mc_ehl.
Test: Boot mc_ehl1 with this patch and ensure the bits in the MCBAR
register are cleared.
Change-Id: Ie58a4b6444d5be088ac2b25ff0a2f5cd33120ace
Signed-off-by: Uwe Poeche <uwe.poeche(a)siemens.com>
---
M src/mainboard/siemens/mc_ehl/Kconfig
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/63548/6
--
To view, visit https://review.coreboot.org/c/coreboot/+/63548
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie58a4b6444d5be088ac2b25ff0a2f5cd33120ace
Gerrit-Change-Number: 63548
Gerrit-PatchSet: 6
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Uwe Poeche.
Hello build bot (Jenkins), Werner Zeh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/63547
to look at the new patch set (#6).
Change subject: soc/intel/elkhartlake/systemagent: Disable RAPL based on Kconfig
......................................................................
soc/intel/elkhartlake/systemagent: Disable RAPL based on Kconfig
This patch provides the possibility for EHL based boards to disable
RAPL settings via SOC_INTEL_DISABLE_POWER_LIMITS config switch.
Other than on previouse SOCs this needs to be done in an MCHBAR mapped
register rather than via MSR on Elkhart Lake.
Test:
Check MCHBAR mapped registers (MCH_PKG_POWER_LIMIT) on mc_ehl1.
Change-Id: I5be6632b15ab8e14a21b5cd35152f82fec919d9f
Signed-off-by: Uwe Poeche <uwe.poeche(a)siemens.com>
---
M src/soc/intel/elkhartlake/include/soc/systemagent.h
M src/soc/intel/elkhartlake/systemagent.c
2 files changed, 15 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/63547/6
--
To view, visit https://review.coreboot.org/c/coreboot/+/63547
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I5be6632b15ab8e14a21b5cd35152f82fec919d9f
Gerrit-Change-Number: 63547
Gerrit-PatchSet: 6
Gerrit-Owner: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Uwe Poeche <uwe.poeche(a)siemens.com>
Gerrit-MessageType: newpatchset