coreboot-gerrit June 2023

coreboot-gerrit@coreboot.org

1 participants
5387 discussions

[S] Change in coreboot[master]: include/cpu/x86: Simplify en/dis cache functions
by Felix Singer (Code Review) 02 Jun '23

02 Jun '23

Felix Singer has submitted this change. ( https://review.coreboot.org/c/coreboot/+/75552?usp=email ) Change subject: include/cpu/x86: Simplify en/dis cache functions ...................................................................... include/cpu/x86: Simplify en/dis cache functions Implementation of enable/disable cache functions aren't complex, simply drop cr0 variable usage, still maintains good readablity. Signed-off-by: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Change-Id: I81688e8bbb073e1d09ecf63f3f33e1651dbd778e Reviewed-on: https://review.coreboot.org/c/coreboot/+/75552 Reviewed-by: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Jérémy Compostella <jeremy.compostella(a)intel.com> Reviewed-by: Subrata Banik <subratabanik(a)google.com> --- M src/include/cpu/x86/cache.h 1 file changed, 2 insertions(+), 8 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved Jérémy Compostella: Looks good to me, but someone else must approve Subrata Banik: Looks good to me, approved diff --git a/src/include/cpu/x86/cache.h b/src/include/cpu/x86/cache.h index d4d9160..dcc96b6 100644 --- a/src/include/cpu/x86/cache.h +++ b/src/include/cpu/x86/cache.h @@ -47,10 +47,7 @@ */ static __always_inline void enable_cache(void) { - CRx_TYPE cr0; - cr0 = read_cr0(); - cr0 &= ~(CR0_CD | CR0_NW); - write_cr0(cr0); + write_cr0(read_cr0() & ~(CR0_CD | CR0_NW)); } /* @@ -66,10 +63,7 @@ static __always_inline void disable_cache(void) { /* Disable and write back the cache */ - CRx_TYPE cr0; - cr0 = read_cr0(); - cr0 |= CR0_CD; - write_cr0(cr0); + write_cr0(read_cr0() | CR0_CD); if (!self_snooping_supported()) wbinvd(); } -- To view, visit https://review.coreboot.org/c/coreboot/+/75552?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I81688e8bbb073e1d09ecf63f3f33e1651dbd778e Gerrit-Change-Number: 75552 Gerrit-PatchSet: 4 Gerrit-Owner: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-MessageType: merged

1 0

[S] Change in coreboot[master]: include/cpu/x86: Simplify en/dis cache functions
by Felix Singer (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: Arthur Heymans, Himanshu Sahdev, Lean Sheng Tan. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75552?usp=email ) Change subject: include/cpu/x86: Simplify en/dis cache functions ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/75552?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I81688e8bbb073e1d09ecf63f3f33e1651dbd778e Gerrit-Change-Number: 75552 Gerrit-PatchSet: 3 Gerrit-Owner: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de> Gerrit-Reviewer: Jérémy Compostella <jeremy.compostella(a)intel.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Himanshu Sahdev <himanshu.sahdev(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 01:49:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: mb/google/kukui: change Juniper/Willow RAM table offset to 0x30
by Sheng-Liang Pan (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: Hung-Te Lin, Paul Menzel, Yidi Lin, Yu-Ping Wu. Sheng-Liang Pan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75563?usp=email ) Change subject: mb/google/kukui: change Juniper/Willow RAM table offset to 0x30 ...................................................................... Patch Set 3: Code-Review+1 (4 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/75563/comment/22e53e11_93336a96 : PS2, Line 7: Ram > RAM Done https://review.coreboot.org/c/coreboot/+/75563/comment/171280e7_4190ffde : PS2, Line 9: all memory ID > What do you mean by “all memory ID”? The memory ID in each RAM table. https://review.coreboot.org/c/coreboot/+/75563/comment/9991946b_c9c548a2 : PS2, Line 9: for > For Done https://review.coreboot.org/c/coreboot/+/75563/comment/c01f3cef_c1018a80 : PS2, Line 11: > What is the source? memory 2nd source -- To view, visit https://review.coreboot.org/c/coreboot/+/75563?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I92740275dcc27061a94b7db7ce095655c0bd7cf5 Gerrit-Change-Number: 75563 Gerrit-PatchSet: 3 Gerrit-Owner: Sheng-Liang Pan <sheng-liang.pan(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Reviewer: Sheng-Liang Pan <sheng-liang.pan(a)quanta.corp-partner.google.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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 01:45:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: mb/google/kukui: change Juniper/Willow RAM table offset to 0x30
by Sheng-Liang Pan (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: Hung-Te Lin, Sheng-Liang Pan, Yidi Lin, Yu-Ping Wu. Hello Hung-Te Lin, Yidi Lin, Yu-Ping Wu, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/75563?usp=email to look at the new patch set (#3). Change subject: mb/google/kukui: change Juniper/Willow RAM table offset to 0x30 ...................................................................... mb/google/kukui: change Juniper/Willow RAM table offset to 0x30 All the memory source for Juniper/Willow can leverage same RAMID in offset 0x30 table, so change Juniper/Willow RAM table default as 0x30 for further memory 2nd source. BUG=b:284423187 BRANCH=kukui TEST=emerge-jacuzzi coreboot Signed-off-by: Sheng-Liang Pan <sheng-liang.pan(a)quanta.corp-partner.google.com> Change-Id: I92740275dcc27061a94b7db7ce095655c0bd7cf5 --- M src/mainboard/google/kukui/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/75563/3 -- To view, visit https://review.coreboot.org/c/coreboot/+/75563?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I92740275dcc27061a94b7db7ce095655c0bd7cf5 Gerrit-Change-Number: 75563 Gerrit-PatchSet: 3 Gerrit-Owner: Sheng-Liang Pan <sheng-liang.pan(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org> 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-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org> Gerrit-Attention: Sheng-Liang Pan <sheng-liang.pan(a)quanta.corp-partner.google.com> Gerrit-Attention: Yidi Lin <yidilin(a)google.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: dedede/variants/dibbi: Update power limit on Dibbi
by Reka Norman (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: ChiaLing, Reka Norman, Ryan Lin, Zhuohao Lee. Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75404?usp=email ) Change subject: dedede/variants/dibbi: Update power limit on Dibbi ...................................................................... Patch Set 4: (4 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/75404/comment/cf2a76d9_07bd33ec : PS4, Line 7: dedede/variants/dibbi: Update power limit on Dibbi nit: `mb/google/dedede/var/dibbi: Update power limits` https://review.coreboot.org/c/coreboot/+/75404/comment/4a32588f_0337902c : PS4, Line 9: by ramstage nit: `in ramstage.` Patchset: PS4: This is failing to build too. You need to upload it with CB:75396 as its parent. File src/mainboard/google/dedede/variants/dibbi/ramstage.c: https://review.coreboot.org/c/coreboot/+/75404/comment/dc01e67e_32f3e178 : PS2, Line 16: //N4500 > Done I meant please move each comment up one line. E.g. ``` // N4500 { PCI_DEVICE_ID_INTEL_JSL_ID_1, 6, 6000, 6000, 20000, 20000, 60000 }, ``` -- To view, visit https://review.coreboot.org/c/coreboot/+/75404?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic3571ff850cc2ec1a1ffecf39dded004c8cf89e6 Gerrit-Change-Number: 75404 Gerrit-PatchSet: 4 Gerrit-Owner: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Ryan Lin <ryan.lin(a)intel.com> Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com> Gerrit-CC: Reka Norman <rekanorman(a)google.com> Gerrit-CC: Super Ni <super.ni(a)intel.com> Gerrit-CC: Super Ni <super.ni(a)intel.corp-partner.google.com> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Ryan Lin <ryan.lin(a)intel.com> Gerrit-Attention: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-Attention: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 01:36:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Reka Norman <rekanorman(a)google.com> Comment-In-Reply-To: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/rex: Enable ISH support
by Bernardo Perez Priego (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: Boris Mittelberg, Caveh Jalali, Gwendal Grignou, Harsha B R, Jamie Ryu, Jay Patel, Kapil Porwal, Li1 Feng, Paul Menzel, Subrata Banik, Tarun Tuli, Vijay P Hiremath, Wonkyu Kim. Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75039?usp=email ) Change subject: mb/google/rex: Enable ISH support ...................................................................... Patch Set 15: (1 comment) File src/mainboard/google/rex/Kconfig: https://review.coreboot.org/c/coreboot/+/75039/comment/0322d714_e4fcdd8c : PS13, Line 111: default "variants/\$(CONFIG_VARIANT_DIR)/overridetree_ec_ish.cb" if DRIVERS_INTEL_ISH > > I would think so. […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/75039?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ibe0e1b8ce2c9b08ac6b1e6fef9bd19afc9b4f59f Gerrit-Change-Number: 75039 Gerrit-PatchSet: 15 Gerrit-Owner: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-Reviewer: Boris Mittelberg <bmbm(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Gwendal Grignou <gwendal(a)chromium.org> Gerrit-Reviewer: Harsha B R <harsha.b.r(a)intel.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Jay Patel <jay2.patel(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Li1 Feng <li1.feng(a)intel.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tanu Malhotra <tanu.malhotra(a)gmail.com> Gerrit-Reviewer: Tanu Malhotra <tanu.malhotra(a)intel.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Vijay P Hiremath <vijay.p.hiremath(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Attention: Li1 Feng <li1.feng(a)intel.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Jay Patel <jay2.patel(a)intel.com> Gerrit-Attention: Gwendal Grignou <gwendal(a)chromium.org> Gerrit-Attention: Vijay P Hiremath <vijay.p.hiremath(a)intel.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Harsha B R <harsha.b.r(a)intel.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Boris Mittelberg <bmbm(a)google.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 01:29:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subratabanik(a)google.com> Comment-In-Reply-To: Bernardo Perez Priego <bernardo.perez.priego(a)intel.com> Gerrit-MessageType: comment

1 0

[XS] Change in coreboot[master]: mb/google/myst: Enable S0ix
by Eric Lai (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: Jon Murphy, Mark Hasemeyer, Martin Roth, Tim Van Patten. Eric Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74279?usp=email ) Change subject: mb/google/myst: Enable S0ix ...................................................................... Patch Set 20: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/74279?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3cabc2c3ba75f4490da18b861ef2b82ce240860d Gerrit-Change-Number: 74279 Gerrit-PatchSet: 20 Gerrit-Owner: Jon Murphy <jpmurphy(a)google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Karthik Ramasubramanian <kramasub(a)google.com> Gerrit-Reviewer: Mark Hasemeyer <markhas(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Tim Van Patten <timvp(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-CC: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: Jon Murphy <jpmurphy(a)google.com> Gerrit-Attention: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Attention: Tim Van Patten <timvp(a)google.com> Gerrit-Attention: Mark Hasemeyer <markhas(a)google.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 01:20:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: [RFC] security/vboot/misc.h: Always return slot A if RW_AB not enabled
by Julius Werner (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: Michał Żygowski, Yu-Ping Wu. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75462?usp=email ) Change subject: [RFC] security/vboot/misc.h: Always return slot A if RW_AB not enabled ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I'm not sure this really makes sense? You won't affect vboot bookkeeping with this, so vboot will still think it needs to reboot and try slot B first before going into recovery mode and all those kinds of things. So I don't think this actually makes the extra reboots go away. A proper solution for this I think would be to add a new VB2_CONTEXT_ONLY_ONE_SLOT flag in https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/ref…, to let vboot know properly that it is supposed to run in single slot mode. And then modify vb2_select_fw_slot() and vb2api_fail() (in https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/ref…) accordingly, so that it will behave correctly in slot failure cases (not try to reboot into the other slot first but go straight into recovery). -- To view, visit https://review.coreboot.org/c/coreboot/+/75462?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie123881a2f9f766ae65e4ac7c36bc2a8fce8d100 Gerrit-Change-Number: 75462 Gerrit-PatchSet: 1 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org> Gerrit-Reviewer: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 01:19:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: mb/google/dedede: Support variant specific power limits
by Reka Norman (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: ChiaLing, Paul Menzel, Reka Norman, Ryan Lin, Zhuohao Lee. Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75396?usp=email ) Change subject: mb/google/dedede: Support variant specific power limits ...................................................................... Patch Set 11: (2 comments) File src/mainboard/google/dedede/variants/baseboard/ramstage.c: https://review.coreboot.org/c/coreboot/+/75396/comment/0de791d1_0208e908 : PS10, Line 134: > Pl4 will be 60w. It updated on CB:75395. On brask, we set PL4 to the same value as psyspl2 for Type-C, i.e. limit it to 97% of the charger rating. Do we need to do the same here? Currently PL4 will currently exceed the charger rating for 45W chargers. File src/mainboard/google/dedede/variants/baseboard/ramstage.c: https://review.coreboot.org/c/coreboot/+/75396/comment/a540a3c8_6464539b : PS11, Line 106: psyspl1, not used -- To view, visit https://review.coreboot.org/c/coreboot/+/75396?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: If38a9b7923f19885f6d47b83c92b68ef6dfa88db Gerrit-Change-Number: 75396 Gerrit-PatchSet: 11 Gerrit-Owner: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Ryan Lin <ryan.lin(a)intel.com> Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Reka Norman <rekanorman(a)google.com> Gerrit-CC: Super Ni <super.ni(a)intel.com> Gerrit-CC: Super Ni <super.ni(a)intel.corp-partner.google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Ryan Lin <ryan.lin(a)intel.com> Gerrit-Attention: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-Attention: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 01:16:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: ChiaLing <chia-ling.hou(a)intel.com> Comment-In-Reply-To: Zhuohao Lee <zhuohao(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/intel/jasperlake: Add per-SKU power limits
by Reka Norman (Code Review) 02 Jun '23

02 Jun '23

Attention is currently required from: Arthur Heymans, ChiaLing, Reka Norman, Ryan Lin, Zhuohao Lee. Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/75395?usp=email ) Change subject: soc/intel/jasperlake: Add per-SKU power limits ...................................................................... Patch Set 5: (9 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/75395/comment/0d52e37a_5bf4e623 : PS5, Line 12: check psys on DUT Please add details of how you tested this specific CL since it's not related to PSYS. How did you check the power limits are being set correctly? Patchset: PS5: The build is failing. You can look at the logs which the Jenkins bot linked: https://qa.coreboot.org/job/coreboot-gerrit/237558/ It looks like the PCI ID variable names are wrong. In general please make sure all your CLs build when you upload them. You should get a verified+1 from the bot. File src/mainboard/google/dedede/variants/blipper/overridetree.cb: https://review.coreboot.org/c/coreboot/+/75395/comment/49b5a5b6_b243bf62 : PS5, Line 62: register "power_limits_config[JSL_N4500_6W_CORE]" = "{ : .tdp_pl1_override = 6, : .tdp_pl2_override = 20, : .tdp_pl4 = 60, : }" : : register "power_limits_config[JSL_N5100_6W_CORE]" = "{ : .tdp_pl1_override = 6, : .tdp_pl2_override = 20, : .tdp_pl4 = 60, : }" For the ones which are the same as the baseboard, you can just delete them instead. https://review.coreboot.org/c/coreboot/+/75395/comment/95af07a8_b9e2803b : PS5, Line 73: Why not override `JSL_N6000_6W_CORE` too? Are you sure there are no dedede variants using this SKU? File src/soc/intel/jasperlake/chip.h: https://review.coreboot.org/c/coreboot/+/75395/comment/b43f05b1_9fb5d63b : PS5, Line 7: #include <device/pci_ids.h> up one line File src/soc/intel/jasperlake/chip.h: https://review.coreboot.org/c/coreboot/+/75395/comment/3891c0dd_e89ff3db : PS3, Line 45: cpu_id > need to be cpu_id Why? File src/soc/intel/jasperlake/systemagent.c: https://review.coreboot.org/c/coreboot/+/75395/comment/539a7c29_e205d400 : PS3, Line 66: /* Get System Agent PCI ID */ : sa = pcidev_path_on_root(SA_DEVFN_ROOT); : sa_pci_id = sa ? pci_read_config16(sa, PCI_DEVICE_ID) : 0xFFFF; > Acknowledged He means `sa` and `dev` are the same device, so you don't need the `sa = pcidev_path_on_root(SA_DEVFN_ROOT);` line, you can just use `dev` instead. (And in general please don't mark comments as resolved until you've addressed them, or given an explanation for why you won't address them.) https://review.coreboot.org/c/coreboot/+/75395/comment/04182ee9_b112b0c3 : PS3, Line 84: printk(BIOS_ERR, "unknown SA ID: 0x%4x, " : "skipped power limits configuration.\n", > There's no SA ID, so can't search corresponding TDP He means print the value of `tdp`. We're trying to find an entry which matches both the SA ID and TDP. If we can't find a matching entry, printing both will help us figure out why. File src/soc/intel/jasperlake/systemagent.c: https://review.coreboot.org/c/coreboot/+/75395/comment/b64d8e96_7355b4a8 : PS5, Line 4: #include <chip.h> up one line -- To view, visit https://review.coreboot.org/c/coreboot/+/75395?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I74affa6bc6ca8203459820f2530b6f20793cf857 Gerrit-Change-Number: 75395 Gerrit-PatchSet: 5 Gerrit-Owner: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-Reviewer: Reka Norman <rekanorman(a)chromium.org> Gerrit-Reviewer: Ryan Lin <ryan.lin(a)intel.com> Gerrit-Reviewer: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-CC: Reka Norman <rekanorman(a)google.com> Gerrit-CC: Super Ni <super.ni(a)intel.com> Gerrit-CC: Super Ni <super.ni(a)intel.corp-partner.google.com> Gerrit-Attention: Reka Norman <rekanorman(a)chromium.org> Gerrit-Attention: Ryan Lin <ryan.lin(a)intel.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-Attention: Zhuohao Lee <zhuohao(a)google.com> Gerrit-Comment-Date: Fri, 02 Jun 2023 00:49:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz> Comment-In-Reply-To: ChiaLing <chia-ling.hou(a)intel.com> Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit June 2023