coreboot-gerrit March 2023

coreboot-gerrit@coreboot.org

1 participants
4880 discussions

[M] Change in coreboot[master]: soc/amd/mendocino: Remove the SPL DPTC parameter
by Chris Wang (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Frank Wu, Jason Glenesk, Raul Rangel, Jason Nien, EricKY Cheng, Matt DeVillier, Isaac Lee, Martin Roth, Tim Van Patten, Fred Reitberger, Felix Held. Chris Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73124 ) Change subject: soc/amd/mendocino: Remove the SPL DPTC parameter ...................................................................... Patch Set 6: (1 comment) File src/mainboard/google/skyrim/variants/baseboard/devicetree.cb: https://review.coreboot.org/c/coreboot/+/73124/comment/26bae950_c7348e35 PS6, Line 263: # Remove the sustained_power_limit_mW when STT is enabled : register "sustained_power_limit_mW" = "15000" > I'm leaning towards Frank's suggestion, since the current comment is not entirely accurate. […] I submit another CL for moving `sustained_power_limit_mW` to `overridetree.cb` Would you help take a look ? https://review.coreboot.org/c/coreboot/+/73426. -- To view, visit https://review.coreboot.org/c/coreboot/+/73124 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8e6dad7d24883f8aadce83ebac401ecd4137d61a Gerrit-Change-Number: 73124 Gerrit-PatchSet: 6 Gerrit-Owner: Chris Wang <chris.wang(a)amd.corp-partner.google.com> Gerrit-Reviewer: Dtrain Hsu <dtrain_hsu(a)compal.corp-partner.google.com> Gerrit-Reviewer: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Isaac Lee <isaaclee(a)google.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Reviewer: Martin Roth <martin.roth(a)amd.corp-partner.google.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.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: Frank Wu <frank_wu(a)compal.corp-partner.google.com> Gerrit-CC: John Su <john_su(a)compal.corp-partner.google.com> Gerrit-CC: Van Chen <van_chen(a)compal.corp-partner.google.com> Gerrit-Attention: Frank Wu <frank_wu(a)compal.corp-partner.google.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jason Nien <jason.nien(a)amd.corp-partner.google.com> Gerrit-Attention: EricKY Cheng <ericky_cheng(a)compal.corp-partner.google.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Isaac Lee <isaaclee(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: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Fri, 03 Mar 2023 18:10:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Frank Wu <frank_wu(a)compal.corp-partner.google.com> Comment-In-Reply-To: Chris Wang <chris.wang(a)amd.corp-partner.google.com> Comment-In-Reply-To: Tim Van Patten <timvp(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/google/skyrim: Move SPL setting to variants
by Chris Wang (Code Review) 03 Mar '23

03 Mar '23

Chris Wang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73426 ) Change subject: mb/google/skyrim: Move SPL setting to variants ...................................................................... mb/google/skyrim: Move SPL setting to variants Move the sustained_power_limit_mW setting from the baseboard to variants. This setting will be needed before STT is enabled, but once STT is enabled, this setting should be removed. BUG=b:265267957 BRANCH=none TEST=Build/Boot to ChromeOS Signed-off-by: Chris Wang <chris.wang(a)amd.corp-partner.google.com> Change-Id: I7b9779600cfa8c7581732e936a714728fd618d20 --- M src/mainboard/google/skyrim/variants/baseboard/devicetree.cb M src/mainboard/google/skyrim/variants/crystaldrift/overridetree.cb M src/mainboard/google/skyrim/variants/markarth/overridetree.cb M src/mainboard/google/skyrim/variants/skyrim/overridetree.cb 4 files changed, 32 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/26/73426/1 diff --git a/src/mainboard/google/skyrim/variants/baseboard/devicetree.cb b/src/mainboard/google/skyrim/variants/baseboard/devicetree.cb index c92131c..6892742 100644 --- a/src/mainboard/google/skyrim/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/skyrim/variants/baseboard/devicetree.cb @@ -260,8 +260,6 @@ register "fast_ppt_limit_mW" = "30000" register "slow_ppt_time_constant_s" = "5" register "stapm_time_constant_s" = "275" - # Remove the sustained_power_limit_mW when STT is enabled - register "sustained_power_limit_mW" = "15000" register "thermctl_limit_degreeC" = "100" register "vrm_current_limit_mA" = "28000" register "vrm_maximum_current_limit_mA" = "50000" diff --git a/src/mainboard/google/skyrim/variants/crystaldrift/overridetree.cb b/src/mainboard/google/skyrim/variants/crystaldrift/overridetree.cb index 7a56f93..9f58641 100644 --- a/src/mainboard/google/skyrim/variants/crystaldrift/overridetree.cb +++ b/src/mainboard/google/skyrim/variants/crystaldrift/overridetree.cb @@ -1,5 +1,10 @@ # SPDX-License-Identifier: GPL-2.0-or-later chip soc/amd/mendocino + + # Set Package Power Parameters + # Remove the sustained_power_limit_mW when STT is enabled + register "sustained_power_limit_mW" = "15000" + device domain 0 on end end # chip soc/amd/mendocino diff --git a/src/mainboard/google/skyrim/variants/markarth/overridetree.cb b/src/mainboard/google/skyrim/variants/markarth/overridetree.cb index f50820b..b7132a8 100644 --- a/src/mainboard/google/skyrim/variants/markarth/overridetree.cb +++ b/src/mainboard/google/skyrim/variants/markarth/overridetree.cb @@ -2,6 +2,10 @@ chip soc/amd/mendocino + # Set Package Power Parameters + # Remove the sustained_power_limit_mW when STT is enabled + register "sustained_power_limit_mW" = "15000" + device domain 0 on register "dxio_tx_vboost_enable" = "1" diff --git a/src/mainboard/google/skyrim/variants/skyrim/overridetree.cb b/src/mainboard/google/skyrim/variants/skyrim/overridetree.cb index 0b9781e..3c33741 100644 --- a/src/mainboard/google/skyrim/variants/skyrim/overridetree.cb +++ b/src/mainboard/google/skyrim/variants/skyrim/overridetree.cb @@ -27,6 +27,11 @@ end chip soc/amd/mendocino + + # Set Package Power Parameters + # Remove the sustained_power_limit_mW when STT is enabled + register "sustained_power_limit_mW" = "15000" + device domain 0 on device ref gpp_bridge_a on # Internal GPP Bridge 0 to Bus A device ref xhci_1 on # XHCI1 controller -- To view, visit https://review.coreboot.org/c/coreboot/+/73426 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7b9779600cfa8c7581732e936a714728fd618d20 Gerrit-Change-Number: 73426 Gerrit-PatchSet: 1 Gerrit-Owner: Chris Wang <chris.wang(a)amd.corp-partner.google.com> Gerrit-MessageType: newchange

1 0

[XL] Change in coreboot[master]: soc/intel/xeon_sp/spr: Add header files and romstage code
by Johnny Lin (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Patrick Rudolph, Jonathan Zhang, TimLiu-SMCI, Paul Menzel, Christian Walter, Angel Pons, Jian-Ming Wang, Arthur Heymans, Srinidhi N Kaushik, Shelly Chang, Elyes Haouas, Tim Chu. Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/72442 ) Change subject: soc/intel/xeon_sp/spr: Add header files and romstage code ...................................................................... Patch Set 9: (8 comments) File src/soc/intel/xeon_sp/spr/ddr.c: https://review.coreboot.org/c/coreboot/+/72442/comment/a7c7450f_af6c0ac4 PS3, Line 5: get_ddr_voltage > Please add the unit to the function name. Done https://review.coreboot.org/c/coreboot/+/72442/comment/29ee950f_934274a0 PS3, Line 5: uint8_t > Ditto. Done https://review.coreboot.org/c/coreboot/+/72442/comment/d81871a8_1f202947 PS3, Line 5: uint32_t > Please do not fix the size [1]. […] If I understand correctly, we should use 'unsigned int' right? So I change both: uint16_t get_max_memory_speed(uint32_t commonTck) to unsigned int get_ddr_millivolt(unsigned int ddr_voltage) uint16_t get_max_memory_speed(uint32_t commonTck) to unsigned int get_max_memory_speed(unsigned int commonTck) https://review.coreboot.org/c/coreboot/+/72442/comment/5410eee0_c338aa43 PS3, Line 17: uint32_t > Ditto. Done https://review.coreboot.org/c/coreboot/+/72442/comment/5e80c339_77e04b81 PS3, Line 17: uint16_t > Ditto. Done File src/soc/intel/xeon_sp/spr/romstage.c: https://review.coreboot.org/c/coreboot/+/72442/comment/cf83017a_51c8fc2e PS2, Line 115: mupd->FspmConfig.DFXEnable = 0; > Maybe: […] Done https://review.coreboot.org/c/coreboot/+/72442/comment/5e069e07_9108fe05 PS2, Line 122: int port, socket; > Could be unsigned int? Done File src/soc/intel/xeon_sp/spr/romstage.c: https://review.coreboot.org/c/coreboot/+/72442/comment/f87d2e2a_023bc2ee PS8, Line 164: X2apic > m_cfg->X2apic = config->x2apic; […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/72442 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia022534e5206dbeec946d3e5f3c66bcb5628748f Gerrit-Change-Number: 72442 Gerrit-PatchSet: 9 Gerrit-Owner: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Jian-Ming Wang <jianmingW(a)supermicro.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Simon Chou <simonchou(a)supermicro.com.tw> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: TimLiu-SMCI <timliu(a)supermicro.com.tw> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shelly Chang <Shelly_Chang(a)wiwynn.com> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: TimLiu-SMCI <timliu(a)supermicro.com.tw> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Jian-Ming Wang <jianmingW(a)supermicro.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Attention: Shelly Chang <Shelly_Chang(a)wiwynn.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 17:59:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

[XL] Change in coreboot[master]: soc/intel/xeon_sp/spr: Add header files and romstage code
by Johnny Lin (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Jonathan Zhang, TimLiu-SMCI, Johnny Lin, Paul Menzel, Christian Walter, Angel Pons, Jian-Ming Wang, Arthur Heymans, Srinidhi N Kaushik, Shelly Chang, Elyes Haouas, Tim Chu. Johnny Lin has uploaded a new patch set (#9) to the change originally created by Jonathan Zhang. ( https://review.coreboot.org/c/coreboot/+/72442 ) Change subject: soc/intel/xeon_sp/spr: Add header files and romstage code ...................................................................... soc/intel/xeon_sp/spr: Add header files and romstage code Change-Id: Ia022534e5206dbeec946d3e5f3c66bcb5628748f Signed-off-by: Jonathan Zhang <jonzhang(a)meta.com> Signed-off-by: Johnny Lin <johnny_lin(a)wiwynn.com> --- A src/soc/intel/xeon_sp/spr/ddr.c A src/soc/intel/xeon_sp/spr/hob_display.c A src/soc/intel/xeon_sp/spr/include/soc/cpu.h A src/soc/intel/xeon_sp/spr/include/soc/crashlog.h A src/soc/intel/xeon_sp/spr/include/soc/ddr.h A src/soc/intel/xeon_sp/spr/include/soc/pci_devs.h A src/soc/intel/xeon_sp/spr/include/soc/soc_util.h A src/soc/intel/xeon_sp/spr/include/soc/xhci.h A src/soc/intel/xeon_sp/spr/romstage.c A src/soc/intel/xeon_sp/spr/soc_util.c A src/soc/intel/xeon_sp/spr/upd_display.c 11 files changed, 1,398 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/72442/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/72442 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia022534e5206dbeec946d3e5f3c66bcb5628748f Gerrit-Change-Number: 72442 Gerrit-PatchSet: 9 Gerrit-Owner: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: Jian-Ming Wang <jianmingW(a)supermicro.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Simon Chou <simonchou(a)supermicro.com.tw> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: TimLiu-SMCI <timliu(a)supermicro.com.tw> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-CC: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shelly Chang <Shelly_Chang(a)wiwynn.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: TimLiu-SMCI <timliu(a)supermicro.com.tw> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Jian-Ming Wang <jianmingW(a)supermicro.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Attention: Shelly Chang <Shelly_Chang(a)wiwynn.com> Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: mb/brya/variant/bb/{brya,hades}/include/ec: Add EC_HOST_EVENT_GPU
by Tarun Tuli (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Daisuke Nojiri, Caveh Jalali. Hello Daisuke Nojiri, build bot (Jenkins), Caveh Jalali, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/73217 to look at the new patch set (#8). Change subject: mb/brya/variant/bb/{brya,hades}/include/ec: Add EC_HOST_EVENT_GPU ...................................................................... mb/brya/variant/bb/{brya,hades}/include/ec: Add EC_HOST_EVENT_GPU EC_HOST_EVENT_GPU was renamed from EC_HOST_EVENT_USB_CHARGER and thought to no longer be used. It was subsequently removed in I9e3e0e9b45385766343489ae2d8fc43fb0954923 Add back the mask for this event as it is infact required on certain Brya (Agah) and Hades variants. Signed-off-by: Tarun Tuli <taruntuli(a)google.com> BUG=b:216485035,b:258126464,b:266631157 BRANCH=none TEST=D-notifier events are received again from EC Change-Id: I9d7bf52efa9572e1bbd2f307420e09a7398a1ca9 --- M src/mainboard/google/brya/variants/baseboard/brya/include/baseboard/ec.h M src/mainboard/google/brya/variants/baseboard/hades/include/baseboard/ec.h 2 files changed, 24 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/73217/8 -- To view, visit https://review.coreboot.org/c/coreboot/+/73217 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9d7bf52efa9572e1bbd2f307420e09a7398a1ca9 Gerrit-Change-Number: 73217 Gerrit-PatchSet: 8 Gerrit-Owner: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Daisuke Nojiri <dnojiri(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Daisuke Nojiri <dnojiri(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in coreboot[master]: mb/brya/include/ec: Add EC_HOST_EVENT_GPU
by Tarun Tuli (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Daisuke Nojiri, Caveh Jalali. Tarun Tuli has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73217 ) Change subject: mb/brya/include/ec: Add EC_HOST_EVENT_GPU ...................................................................... Patch Set 7: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/73217/comment/893df676_149b94c2 PS6, Line 15: required on certain Brya (Agah) variants > Why don't we change hades/include/.../ec. […] Hades isn't functional yet, but let me add it there too so we don't forget. -- To view, visit https://review.coreboot.org/c/coreboot/+/73217 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9d7bf52efa9572e1bbd2f307420e09a7398a1ca9 Gerrit-Change-Number: 73217 Gerrit-PatchSet: 7 Gerrit-Owner: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Daisuke Nojiri <dnojiri(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Daisuke Nojiri <dnojiri(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Mar 2023 17:50:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Daisuke Nojiri <dnojiri(a)google.com> Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: mb/brya/include/ec: Add EC_HOST_EVENT_GPU
by Daisuke Nojiri (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Tarun Tuli, Caveh Jalali. Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/73217 ) Change subject: mb/brya/include/ec: Add EC_HOST_EVENT_GPU ...................................................................... Patch Set 6: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/73217/comment/75c72087_8094460e PS6, Line 15: required on certain Brya (Agah) variants Why don't we change hades/include/.../ec.h? -- To view, visit https://review.coreboot.org/c/coreboot/+/73217 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I9d7bf52efa9572e1bbd2f307420e09a7398a1ca9 Gerrit-Change-Number: 73217 Gerrit-PatchSet: 6 Gerrit-Owner: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Caveh Jalali <caveh(a)chromium.org> Gerrit-Reviewer: Daisuke Nojiri <dnojiri(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Caveh Jalali <caveh(a)chromium.org> Gerrit-Comment-Date: Fri, 03 Mar 2023 17:44:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/amd/picasso/acpi: factor out get_cstate_info functionality
by Felix Held (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Jason Glenesk, Matt DeVillier, Fred Reitberger. Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73425 ) Change subject: soc/amd/picasso/acpi: factor out get_cstate_info functionality ...................................................................... soc/amd/picasso/acpi: factor out get_cstate_info functionality To be able to move generate_cpu_entries to the common AMD SoC code, the SoC-specific C-state info needs to be factored out into a separate get_cstate_info function that will remain in the SoC code. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I8200bb9a0ccacd3c1020cee58f866d2f11671070 --- M src/soc/amd/picasso/acpi.c 1 file changed, 59 insertions(+), 37 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/73425/1 diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index c876f6c..dc714a9 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -230,15 +230,55 @@ return pstate_count; } +static const acpi_cstate_t cstate_info[] = { + [0] = { + .ctype = 1, + .latency = 1, + .power = 0, + .resource = { + .space_id = ACPI_ADDRESS_SPACE_FIXED, + .bit_width = 2, + .bit_offset = 2, + .addrl = 0, + .addrh = 0, + }, + }, + [1] = { + .ctype = 2, + .latency = 400, + .power = 0, + .resource = { + .space_id = ACPI_ADDRESS_SPACE_IO, + .bit_width = 8, + .bit_offset = 0, + .addrl = ACPI_CPU_CONTROL + 1, + .addrh = 0, + .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, + }, + }, +}; + +static const acpi_cstate_t *get_cstate_info(size_t *cstate_count) +{ + uint32_t cstate_base_address = + rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK; + + if (cstate_base_address != ACPI_CPU_CONTROL) { + printk(BIOS_ERR, "C state IO port mismatch. Expected %x instead of %x.\n", + ACPI_CPU_CONTROL, cstate_base_address); + } + + *cstate_count = ARRAY_SIZE(cstate_info); + return cstate_info; +} + void generate_cpu_entries(const struct device *device) { int logical_cores; - size_t pstate_count, cpu; + size_t cstate_count, pstate_count, cpu; struct acpi_sw_pstate pstate_values[MAX_PSTATES] = { {0} }; struct acpi_xpss_sw_pstate pstate_xpss_values[MAX_PSTATES] = { {0} }; uint32_t threads_per_core; - uint32_t cstate_base_address = - rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK; const acpi_addr_t perf_ctrl = { .space_id = ACPI_ADDRESS_SPACE_FIXED, @@ -251,39 +291,7 @@ .addrl = PS_STS_REG, }; - const acpi_cstate_t cstate_info[] = { - [0] = { - .ctype = 1, - .latency = 1, - .power = 0, - .resource = { - .space_id = ACPI_ADDRESS_SPACE_FIXED, - .bit_width = 2, - .bit_offset = 2, - .addrl = 0, - .addrh = 0, - }, - }, - [1] = { - .ctype = 2, - .latency = 400, - .power = 0, - .resource = { - .space_id = ACPI_ADDRESS_SPACE_IO, - .bit_width = 8, - .bit_offset = 0, - .addrl = ACPI_CPU_CONTROL + 1, - .addrh = 0, - .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, - }, - }, - }; - - if (cstate_base_address != ACPI_CPU_CONTROL) { - printk(BIOS_ERR, "C state IO port mismatch. Expected %x, found %x.\n", - ACPI_CPU_CONTROL, cstate_base_address); - } - + const acpi_cstate_t *cstate_values = get_cstate_info(&cstate_count); threads_per_core = get_threads_per_core(); pstate_count = get_pstate_info(pstate_values, pstate_xpss_values); logical_cores = get_cpu_count(); @@ -305,7 +313,7 @@ acpigen_write_PPC(0); - acpigen_write_CST_package(cstate_info, ARRAY_SIZE(cstate_info)); + acpigen_write_CST_package(cstate_values, cstate_count); acpigen_write_CSD_package(cpu / threads_per_core, threads_per_core, CSD_HW_ALL, 0); -- To view, visit https://review.coreboot.org/c/coreboot/+/73425 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8200bb9a0ccacd3c1020cee58f866d2f11671070 Gerrit-Change-Number: 73425 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: soc/amd/picasso/acpi: only use constants in cstate_info
by Felix Held (Code Review) 03 Mar '23

03 Mar '23

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73424 ) Change subject: soc/amd/picasso/acpi: only use constants in cstate_info ...................................................................... soc/amd/picasso/acpi: only use constants in cstate_info To facilitate factoring out the C-state info retrieval into a function which would then allow to make generate_cpu_entries common, all fields of the cstate_info array of acpi_cstate_t structs need to be build-time constants. To achieve this, use the ACPI_CPU_CONTROL IO port address in the resource's address instead of the value retrieved from MSR_CSTATE_ADDRESS. earlier in ramstage, set_cstate_io_addr writes the ACPI_CPU_CONTROL IO port address used for C state control into MSR_CSTATE_ADDRESS and we don't expect this MSR value to have changed, but better check and print an error if the assumption turned out to not be true. In this case, the C-state IO address info in the _CST package will be wrong, but the rare case where this might happen should be caught during early development of the SoC support. Signed-off-by: Felix Held <felix-coreboot(a)felixheld.de> Change-Id: I202047a6632cd13e81ac3c93f5e108e269c2ce03 --- M src/soc/amd/picasso/acpi.c 1 file changed, 29 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/73424/1 diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c index 138e743..c876f6c 100644 --- a/src/soc/amd/picasso/acpi.c +++ b/src/soc/amd/picasso/acpi.c @@ -272,13 +272,18 @@ .space_id = ACPI_ADDRESS_SPACE_IO, .bit_width = 8, .bit_offset = 0, - .addrl = cstate_base_address + 1, + .addrl = ACPI_CPU_CONTROL + 1, .addrh = 0, .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, }, }, }; + if (cstate_base_address != ACPI_CPU_CONTROL) { + printk(BIOS_ERR, "C state IO port mismatch. Expected %x, found %x.\n", + ACPI_CPU_CONTROL, cstate_base_address); + } + threads_per_core = get_threads_per_core(); pstate_count = get_pstate_info(pstate_values, pstate_xpss_values); logical_cores = get_cpu_count(); -- To view, visit https://review.coreboot.org/c/coreboot/+/73424 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I202047a6632cd13e81ac3c93f5e108e269c2ce03 Gerrit-Change-Number: 73424 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-MessageType: newchange

1 0

[S] Change in coreboot[master]: soc/amd/picasso/include/iomap: add comment about ACPI IO assignment
by Felix Held (Code Review) 03 Mar '23

03 Mar '23

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2023