coreboot-gerrit March 2023

coreboot-gerrit@coreboot.org

1 participants
4880 discussions

[S] Change in coreboot[master]: [TEST] : Route I2S2 lines to Discrete BT
by Meera Ravindranath (Code Review) 03 Mar '23

03 Mar '23

Meera Ravindranath has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/73416 ) Change subject: [TEST] : Route I2S2 lines to Discrete BT ...................................................................... [TEST] : Route I2S2 lines to Discrete BT Signed-off-by: Meera Ravindranath <meera.ravindranath(a)intel.com> Change-Id: Ie274edf288531fe6c260275edaa695cf3b3605d3 --- M src/mainboard/google/brya/variants/skolas/fw_config.c 1 file changed, 11 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/73416/1 diff --git a/src/mainboard/google/brya/variants/skolas/fw_config.c b/src/mainboard/google/brya/variants/skolas/fw_config.c index c2b963f..e7c45f0 100644 --- a/src/mainboard/google/brya/variants/skolas/fw_config.c +++ b/src/mainboard/google/brya/variants/skolas/fw_config.c @@ -145,7 +145,7 @@ gpio_configure_pads(max98360_enable_pads, ARRAY_SIZE(max98360_enable_pads)); printk(BIOS_INFO, "BT offload enabled\n"); gpio_configure_pads(i2s0_enable_pads, ARRAY_SIZE(i2s0_enable_pads)); - gpio_configure_pads(bt_i2s_enable_pads, ARRAY_SIZE(bt_i2s_enable_pads)); + gpio_configure_pads(i2s2_enable_pads, ARRAY_SIZE(i2s2_enable_pads)); } } BOOT_STATE_INIT_ENTRY(BS_DEV_ENABLE, BS_ON_ENTRY, fw_config_handle, NULL); -- To view, visit https://review.coreboot.org/c/coreboot/+/73416 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie274edf288531fe6c260275edaa695cf3b3605d3 Gerrit-Change-Number: 73416 Gerrit-PatchSet: 1 Gerrit-Owner: Meera Ravindranath <meera.ravindranath(a)intel.com> Gerrit-MessageType: newchange

1 0

[M] Change in coreboot[master]: mb/google/rex: Update Rex Flash Layout
by Subrata Banik (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Subrata Banik. Hello build bot (Jenkins), Tarun Tuli, YH Lin, Jamie Ryu, Kapil Porwal, Ivy Jian, Eric Lai, Tim Wawrzynczak, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/66288 to look at the new patch set (#14). Change subject: mb/google/rex: Update Rex Flash Layout ...................................................................... mb/google/rex: Update Rex Flash Layout This patch updates the Rex flash layout to allow CSE Lite FW update and accommodate multiple ESx SoC stepping blobs. SI_BIOS: RW_SECTION_A/B: Increased by ~1.9MB. RW_LEGACY: Reduce to 1MB. RW_MISC: Reduce to 152KB. - Drop RW_SPD_CACHE - Optimize other sections Additionally, moved RW_LEGACY under extended BIOS region. BUG=b:262868089 TEST=Able to enable CSE update on google/rex and have free space to add one more PUNIT FW for support different SoC stepping. Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: I6146b36c4ce2c0141277eeb906d6ad1f503f3c78 --- M src/mainboard/google/rex/chromeos.fmd 1 file changed, 38 insertions(+), 18 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/66288/14 -- To view, visit https://review.coreboot.org/c/coreboot/+/66288 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I6146b36c4ce2c0141277eeb906d6ad1f503f3c78 Gerrit-Change-Number: 66288 Gerrit-PatchSet: 14 Gerrit-Owner: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Eric Lai <eric_lai(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Ivy Jian <ivy.jian(a)quanta.corp-partner.google.com> Gerrit-Reviewer: Jamie Ryu <jamie.m.ryu(a)intel.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com> Gerrit-Reviewer: YH Lin <yueherngl(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newpatchset

1 0

[L] Change in coreboot[master]: soc/intel/xeon_sp/spr: Add Sapphire Rapids ramstage code
by Johnny Lin (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Jonathan Zhang, TimLiu-SMCI, Paul Menzel, Christian Walter, Angel Pons, Jian-Ming Wang, Arthur Heymans, Srinidhi N Kaushik. Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/72443 ) Change subject: soc/intel/xeon_sp/spr: Add Sapphire Rapids ramstage code ...................................................................... Patch Set 9: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/72443/comment/6c030b7d_a2e441e0 PS3, Line 8: > Please elaborate. Done -- To view, visit https://review.coreboot.org/c/coreboot/+/72443 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I128fdc6e58c49fb5abf911d6ffa91e7411f6d1e2 Gerrit-Change-Number: 72443 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: TimLiu-SMCI <timliu(a)supermicro.com.tw> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-CC: Tim Chu <Tim.Chu(a)quantatw.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-Comment-Date: Fri, 03 Mar 2023 09:23:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-MessageType: comment

1 0

[L] Change in coreboot[master]: soc/intel/xeon_sp/spr: Add Sapphire Rapids ramstage code
by Johnny Lin (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: TimLiu-SMCI, Johnny Lin, Christian Walter, Angel Pons, Jian-Ming Wang, Arthur Heymans, Srinidhi N Kaushik. Johnny Lin has uploaded a new patch set (#9) to the change originally created by Jonathan Zhang. ( https://review.coreboot.org/c/coreboot/+/72443 ) Change subject: soc/intel/xeon_sp/spr: Add Sapphire Rapids ramstage code ...................................................................... soc/intel/xeon_sp/spr: Add Sapphire Rapids ramstage code It implements SPR ramstage including silicon initialization, MSR programming, MP init and certain registers locking before booting to payload. Change-Id: I128fdc6e58c49fb5abf911d6ffa91e7411f6d1e2 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/Makefile.inc A src/soc/intel/xeon_sp/spr/chip.c A src/soc/intel/xeon_sp/spr/chip.h M src/soc/intel/xeon_sp/spr/chipset.cb A src/soc/intel/xeon_sp/spr/cpu.c A src/soc/intel/xeon_sp/spr/crashlog.c A src/soc/intel/xeon_sp/spr/numa.c A src/soc/intel/xeon_sp/spr/ramstage.c A src/soc/intel/xeon_sp/spr/reset.c A src/soc/intel/xeon_sp/spr/xhci.c 10 files changed, 876 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/72443/9 -- To view, visit https://review.coreboot.org/c/coreboot/+/72443 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I128fdc6e58c49fb5abf911d6ffa91e7411f6d1e2 Gerrit-Change-Number: 72443 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: TimLiu-SMCI <timliu(a)supermicro.com.tw> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Shuming Chu (Shuming) <s1218944(a)gmail.com> Gerrit-CC: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Attention: TimLiu-SMCI <timliu(a)supermicro.com.tw> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> 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-MessageType: newpatchset

1 0

[M] Change in coreboot[master]: src: Add and use POST_BOOTBLOCK_C_ENTRY
by Werner Zeh (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Raul Rangel, Jeff Daly, Matt DeVillier, Jonathan Zhang, Arthur Heymans, Jason Glenesk, Sean Rhodes, Tarun Tuli, Martin L Roth, Subrata Banik, Johnny Lin, Kapil Porwal, Christian Walter, Vanessa Eusebio, Lean Sheng Tan, Fred Reitberger, Felix Held, Tim Chu. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/71593 ) Change subject: src: Add and use POST_BOOTBLOCK_C_ENTRY ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/71593 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I45a86ece016e098ca42a31207fdedda5c49708d7 Gerrit-Change-Number: 71593 Gerrit-PatchSet: 2 Gerrit-Owner: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com> Gerrit-Reviewer: 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: Jeff Daly <jeffd(a)silicom-usa.com> Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Reviewer: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Reviewer: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-CC: Elyes Haouas <ehaouas(a)noos.fr> Gerrit-CC: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org> Gerrit-Attention: Jeff Daly <jeffd(a)silicom-usa.com> Gerrit-Attention: Matt DeVillier <matt.devillier(a)amd.corp-partner.google.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Kapil Porwal <kapilporwal(a)google.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Vanessa Eusebio <vanessa.f.eusebio(a)intel.com> Gerrit-Attention: Lean Sheng Tan <sheng.tan(a)9elements.com> Gerrit-Attention: Fred Reitberger <reitbergerfred(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 09:19:38 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/*/include/soc/pmc.h: Add missing periodic SMI rate bits
by Werner Zeh (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Michał Żygowski, Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Tim Chu. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69871 ) Change subject: soc/intel/*/include/soc/pmc.h: Add missing periodic SMI rate bits ...................................................................... Patch Set 3: Code-Review+1 (1 comment) File src/soc/intel/apollolake/include/soc/pm.h: https://review.coreboot.org/c/coreboot/+/69871/comment/295cc3d0_249e9983 PS3, Line 168: define GEN_PMCON_B GEN_PMCON2 This is merged already. -- To view, visit https://review.coreboot.org/c/coreboot/+/69871 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic600d39d49135808dd1f571c9eff3cdb98682796 Gerrit-Change-Number: 69871 Gerrit-PatchSet: 3 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.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 <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 09:05:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/*/include/soc/pmc.h: Add missing periodic SMI rate bits
by Werner Zeh (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Michał Żygowski, Jonathan Zhang, Johnny Lin, Christian Walter, Arthur Heymans, Tim Chu. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69871 ) Change subject: soc/intel/*/include/soc/pmc.h: Add missing periodic SMI rate bits ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/69871 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic600d39d49135808dd1f571c9eff3cdb98682796 Gerrit-Change-Number: 69871 Gerrit-PatchSet: 3 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.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 <jon.zhixiong.zhang(a)gmail.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sean Rhodes <sean(a)starlabs.systems> Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Jonathan Zhang <jon.zhixiong.zhang(a)gmail.com> Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com> Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 09:01:58 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in coreboot[master]: soc/intel/alderlake: Hook up the OC watchdog
by Werner Zeh (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Tarun Tuli, Michał Żygowski, Subrata Banik, Paul Menzel, Michał Kopeć. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68945 ) Change subject: soc/intel/alderlake: Hook up the OC watchdog ...................................................................... Patch Set 15: (1 comment) File src/soc/intel/alderlake/romstage/romstage.c: https://review.coreboot.org/c/coreboot/+/68945/comment/332945dd_2e8fef55 PS15, Line 137: is_wdt_enabled(); This looks strange. I guess your intention is to see the current OC_WDT status in the log. If so, then move the printk() to this place and let the is_wdt_enabled() function be silent. Otherwise I would rename this function to 'show_oc_wdt_status' or the like. -- To view, visit https://review.coreboot.org/c/coreboot/+/68945 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I1c2c640d48b7e03ad8cd8d6cdf6aac447e93cd86 Gerrit-Change-Number: 68945 Gerrit-PatchSet: 15 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: Tarun Tuli <taruntuli(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Attention: Tarun Tuli <taruntuli(a)google.com> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Comment-Date: Fri, 03 Mar 2023 09:00:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in coreboot[master]: soc/intel/common/block/oc_wdt: Add OC watchdog common block
by Werner Zeh (Code Review) 03 Mar '23

03 Mar '23

1 0

[M] Change in coreboot[master]: soc/intel/common/block/oc_wdt: Add OC watchdog common block
by Werner Zeh (Code Review) 03 Mar '23

03 Mar '23

Attention is currently required from: Nico Huber, Michał Żygowski, Martin L Roth, Michał Kopeć, Angel Pons, Arthur Heymans. Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68944 ) Change subject: soc/intel/common/block/oc_wdt: Add OC watchdog common block ...................................................................... Patch Set 14: (15 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/68944/comment/1270ba83_6243b163 PS14, Line 18: driveless driverless File src/soc/intel/common/block/include/intelblocks/oc_wdt.h: https://review.coreboot.org/c/coreboot/+/68944/comment/085ea05b_2c7da7b4 PS14, Line 8: void wdt_reload_and_start(unsigned int timeout); : void wdt_disable(void); : bool is_wdt_enabled(void); : unsigned int wdt_get_current_timeout(void); To avoid confusions with the TCO watchdog, would it make sense to add '_oc' to the function names? File src/soc/intel/common/block/oc_wdt/Kconfig: https://review.coreboot.org/c/coreboot/+/68944/comment/a6bd1a8e_9f2e1740 PS14, Line 24: timeout the timeout https://review.coreboot.org/c/coreboot/+/68944/comment/b4e2eb8b_418d3503 PS14, Line 24: reload maybe 'preload'? https://review.coreboot.org/c/coreboot/+/68944/comment/2054b0e4_f1d567a1 PS14, Line 27: platform the platform https://review.coreboot.org/c/coreboot/+/68944/comment/2d6ddf1d_c52cce94 PS14, Line 30: Feed Maybe better 'Reload'? File src/soc/intel/common/block/oc_wdt/Makefile.inc: https://review.coreboot.org/c/coreboot/+/68944/comment/52ae59be_90063928 PS14, Line 1: bootblock-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : verstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : romstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : postcar-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : ramstage-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c : smm-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c Since you want this module being available in all stages, you could use: all-$(CONFIG_SOC_INTEL_COMMON_BLOCK_OC_WDT) += oc_wdt.c instead to improve readability. File src/soc/intel/common/block/oc_wdt/oc_wdt.c: https://review.coreboot.org/c/coreboot/+/68944/comment/89c9c456_58bf36fa PS14, Line 10: (ACPI_BASE_ADDRESS + 0x54) nit: At least my documents for Apollo Lake does not mention neither the OC watchdog nor this register at all. But I can see it on Elkhart Lake. https://review.coreboot.org/c/coreboot/+/68944/comment/732142d6_2ae8806a PS14, Line 25: uint32_t readback; I would move this right at the start of the function. And maybe give it better name, e.g. oc_wdt_ctrl or the like? https://review.coreboot.org/c/coreboot/+/68944/comment/b2493565_547672fc PS14, Line 27: timeout - 1) > PCH_OC_WDT_CTL_TOV_MASK || timeout == 0 Who about: ``` if ((timeout == 0) || (timeout > (PCH_OC_WDT_CTL_TOV_MASK + 1))) {``` This will avoid the bad '-1' case for timeout values of 0. https://review.coreboot.org/c/coreboot/+/68944/comment/55cce624_33a7e02c PS14, Line 29: Watchdog Here again: to avoid confusions witht he old watchdog (TCO), how about a 'OC' prefix? https://review.coreboot.org/c/coreboot/+/68944/comment/1d385829_289c60d1 PS14, Line 40: & PCH_OC_WDT_CTL_TOV_MASK nit: Not really necessary here since you have made sure already that the range fits into 10 bits at the start of this function. https://review.coreboot.org/c/coreboot/+/68944/comment/3d150a86_29466e05 PS14, Line 48: readback nit: Same register name argument here. File src/soc/intel/common/block/smm/smihandler.c: https://review.coreboot.org/c/coreboot/+/68944/comment/3bf0c386_1bedd0a5 PS14, Line 217: if (CONFIG(SOC_INTEL_COMMON_OC_WDT_ENABLE)) : wdt_disable(); This reads strange. If OC_WDT is enabled then disable it? Is this your real intention here? If yes, an explaining comment would be helpful. https://review.coreboot.org/c/coreboot/+/68944/comment/f5b6a7dc_44756459 PS14, Line 488: if (CONFIG(SOC_INTEL_COMMON_OC_WDT_RELOAD_IN_PERIODIC_SMI)) : wdt_reload_and_start(CONFIG_SOC_INTEL_COMMON_OC_WDT_TIMEOUT_SECONDS); It seems like the oc_wdt is only enabled when SOC_INTEL_COMMON_OC_WDT_RELOAD_IN_PERIODIC_SMI is selected. What about the described possibility to let the driver/SW do the reload? Where is it started in this case? -- To view, visit https://review.coreboot.org/c/coreboot/+/68944 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ib494aa0c7581351abca8b496fc5895b2c7cbc5bc Gerrit-Change-Number: 68944 Gerrit-PatchSet: 14 Gerrit-Owner: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Maciej Pijanowski <maciej.pijanowski(a)3mdeb.com> Gerrit-CC: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com> Gerrit-Attention: Martin L Roth <gaumless(a)gmail.com> Gerrit-Attention: Michał Kopeć <michal.kopec(a)3mdeb.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Comment-Date: Fri, 03 Mar 2023 08:55:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit March 2023