coreboot-gerrit February 2021

coreboot-gerrit@coreboot.org

1 participants
7190 discussions

Change in coreboot[master]: soc/intel/tgl: Add configurable value for ConfigTdpLevel
by Nick Vaccaro (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Derek Huang, Patrick Rudolph. Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50104 ) Change subject: soc/intel/tgl: Add configurable value for ConfigTdpLevel ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/50104 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I4242575807caac172b6cbe667839bf6c9241f3c5 Gerrit-Change-Number: 50104 Gerrit-PatchSet: 4 Gerrit-Owner: Derek Huang <derek.huang(a)intel.corp-partner.google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Derek Huang <derek.huang(a)intel.corp-partner.google.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 18:52:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/common/block/acpi/pm_state: add missing include
by Kyösti Mälkki (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50266 ) Change subject: soc/amd/common/block/acpi/pm_state: add missing include ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/coreboot/+/50266 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22862c2d29f130c741b4817dac00287ecfc71fa2 Gerrit-Change-Number: 50266 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Comment-Date: Wed, 03 Feb 2021 18:49:49 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/skylake/acpi: Add PEP table
by Michael Niewöhner (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Benjamin Doron. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49140 ) Change subject: soc/intel/skylake/acpi: Add PEP table ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/49140/comment/6c90f6d4_3c3e7061 PS3, Line 11: > > > > > > Awesome, then at least S0ix states work just fine. > > > > > > > > > > > > That error message comes from the kernel SlpS0 self-test. So, let's test something. I assume `cat /sys/power/mem_sleep` has `s2idle` selected? (With s0ix_enable=1 it should. If not, we have another problem.) > > > > > > > > > > Yes. (I think that's based on the FADT) > > > > > > > > Right, there is some bit in FADT for that. > > > > > > > > > > > > > > > Does your system stay in sleep after executing `echo freeze >/sys/power/state`? > > > > > > > > > > Is the alternative that it wakes immediately? Then yes, it stays in sleep. However, when it does wake, it has always been unlocked. I don't know if that's relevant. > > > > > > > > Unlocked vs. locked is user-space and depends on the desktop environment. XFCE has an option for locking before going to sleep but that only works when sleep was initiated from the DE. > > > > > > That makes sense; I thought it might have been a possibility. > > > > > > > If it wakes immediately with that error, the self-test failed. I'm surprised to see the self-test fail *without* immediate wake, though.... Maybe we have to exclude the SlpS0 state from the table LPIT, when s0ix is not supported on a board :S Could you test this, please? Insert a "return" right before the comment "System (Slp_S0) residency counter" in `src/soc/intel/common/block/acpi/lpit.c` and see if the error disappears. > > > > > > Based on https://01.org/blogs/qwang59/2020/linux-s0ix-troubleshooting, PC10 without S0ix residency is a possible case. If I understand correctly, that situation would be what I see with this board: No S0ix/failed self-test, but PC10 residency/no immediate wake. I will attempt to troubleshoot that sometime by disabling devices, I suppose, following that link. > > > > > > I'm also not certain what you mean. If S0ix is disabled, would LPIT factor? Besides, if S0ix does relate to hardware state, but not board-design, shouldn't LPIT always advertise both PC10 residency and system residency to warn about broken S0ix support for a board? > > > > Well, yes PC10 without S0ix is possible. s0ix_enable means means "PC10 and/or SlpS0 possible". Note that SlpS0 is a substate of s0ix but s0ix != SlpS0. > > > > The LPIT table contains two entries, one for PC10 (the first one) and SlpS0 (the second one. Based on these entries, the OS decides what *actually* is possible. When PC10 and SlpS0 is advertised but SlpS0 doesn't work, that error triggers. So, I want to test what happens, when SlpS0 is not advertised, because it doesn't apply to your board. > > I see the same error. Strange.... > > Additionally, I noticed while looking over the vendor firmware's LPIT table that it defines the PC10 subtable twice, if I'm reading it correctly (two copies of address 0x632 as "FunctionalFixedHW" - the PKG_C10_RESIDENCY MSR?). Even so, the kernel warns that the system was unable to enter PC10. Oh yeah, I've seen that too somewhere but I doubt that's correct. > > So, while I was initially surprised to hear that SlpS0 might not apply to my board, perhaps that is the case. I can always try to troubleshoot some other time. > > > > But regardless: `return` or `return current`? The PC10 residency counter portion does increment "current" > > > > Yes, you're right, it should be `return current` ofc :-) > > Was this only for testing, or do you think that the S0ix subtable should only be advertised if S0ix is known to work? Very good question. S0ix isn't documented very good but I wouldn't advertise something that isn't usable. However, that will require us to introduce a dt option `slps0_supported`. That's already planned but I currently have higher-prio stuff. I'll come back to that asap. If you're curious, have a look on my WIP patches on s0ix. Since it doesn't seem to make a difference irt that error/warning, let's keep it as-is for now and resolve that as soon as I get back to these patches. Wdyt? > If S0ix can be temperamental, I think that the OS should always be able to warn about errors. Well, when the board does not support SlpS0 (= SlpS0 is unconnected) I wouldn't want the OS to warn about it. That warning/error is triggered by the S0ix self-test, which may also be buggy btw. > > Sorry it took me so long to get back to you. No problem :-) So... last question - I hope I didn't ask that already, but I haven't seen it above. Is this patch the cause for that error message or does the error message appear even without it? -- To view, visit https://review.coreboot.org/c/coreboot/+/49140 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I08d8c1fde4f447e9292a0508649f802fdc2721e1 Gerrit-Change-Number: 49140 Gerrit-PatchSet: 3 Gerrit-Owner: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Reviewer: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Benjamin Doron <benjamin.doron00(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 18:47:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Benjamin Doron <benjamin.doron00(a)gmail.com> Comment-In-Reply-To: Michael Niewöhner <foss(a)mniewoehner.de> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/amd/picasso: clean up and re-sort UPD table
by Furquan Shaikh (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: chris wang. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50212 ) Change subject: soc/amd/picasso: clean up and re-sort UPD table ...................................................................... Patch Set 7: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/50212/comment/031ce649_da3f05b5 PS7, Line 32: For such changes, it is important to add Cq-Depend to ensure that when it lands in chromium tree, the corresponding FSP change lands along with it to avoid any compatibility issues. -- To view, visit https://review.coreboot.org/c/coreboot/+/50212 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I655af08e2f86398d088e30d330f49e71cf7e1275 Gerrit-Change-Number: 50212 Gerrit-PatchSet: 7 Gerrit-Owner: chris wang <Chris.Wang(a)amd.com> Gerrit-Reviewer: Chris Wang <chris.wang(a)amd.corp-partner.google.com> Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.com> Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de> Gerrit-Reviewer: Jason Glenesk <jason.glenesk(a)gmail.com> Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: 9elements QA <hardwaretestrobot(a)gmail.com> Gerrit-CC: Frank Wu <frank_wu(a)compal.corp-partner.google.com> Gerrit-CC: Furquan Shaikh <furquan(a)google.com> Gerrit-CC: John Su <john_su(a)compal.corp-partner.google.com> Gerrit-Attention: chris wang <Chris.Wang(a)amd.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 18:15:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: vc/intel/fsp/fsp2_0/alderlake: Add required macros into MemInfoHob.h
by Furquan Shaikh (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Subrata Banik, Patrick Rudolph. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50254 ) Change subject: vc/intel/fsp/fsp2_0/alderlake: Add required macros into MemInfoHob.h ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/50254/comment/0ce92280_522b8f4e PS2, Line 12: BUG=b:178846328 -- To view, visit https://review.coreboot.org/c/coreboot/+/50254 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I18370edca481bac5fdd483680cd7b05b216d10fc Gerrit-Change-Number: 50254 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 18:10:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: vc/intel/fsp/fsp2_0/alderlake: Add required macros into MemInfoHob.h
by Furquan Shaikh (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Subrata Banik, Patrick Rudolph. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50254 ) Change subject: vc/intel/fsp/fsp2_0/alderlake: Add required macros into MemInfoHob.h ...................................................................... Patch Set 2: (1 comment) Commit Message: https://review.coreboot.org/c/coreboot/+/50254/comment/a20c6c67_6a40ee73 PS2, Line 9: 2017.00 Does this also match the upcoming 2037 release? or is another update required? -- To view, visit https://review.coreboot.org/c/coreboot/+/50254 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I18370edca481bac5fdd483680cd7b05b216d10fc Gerrit-Change-Number: 50254 Gerrit-PatchSet: 2 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 18:10:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/emulation/qemu-q35: Mark TSEG region as reserved
by Angel Pons (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50267 ) Change subject: mb/emulation/qemu-q35: Mark TSEG region as reserved ...................................................................... Patch Set 2: (1 comment) File src/mainboard/emulation/qemu-i440fx/northbridge.c: https://review.coreboot.org/c/coreboot/+/50267/comment/5eaa2172_552c5bd8 PS1, Line 109: CONFIG(BOARD_EMULATION_QEMU_X86_Q35) > Done Argh, looks like the builds fail because smm_region() is not defined for i440fx... You could move this to mb/emulation/qemu-q35/mainboard.c function qemu_nb_read_resources() -- To view, visit https://review.coreboot.org/c/coreboot/+/50267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3ae3659349d2a88bc3575fe9675433c054e28832 Gerrit-Change-Number: 50267 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 18:03:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Patrick Rudolph <patrick.rudolph(a)9elements.com> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/emulation/qemu-q35: Mark TSEG region as reserved
by Patrick Rudolph (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Angel Pons. Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50267 ) Change subject: mb/emulation/qemu-q35: Mark TSEG region as reserved ...................................................................... Patch Set 1: (3 comments) Commit Message: https://review.coreboot.org/c/coreboot/+/50267/comment/2334fcf7_d2a4c34a PS1, Line 7: Fix crash in CorebootPayloadPkg > Mark TSEG region as reserved Done File src/mainboard/emulation/qemu-i440fx/northbridge.c: https://review.coreboot.org/c/coreboot/+/50267/comment/d3a2a73e_82de1f19 PS1, Line 109: CONFIG(BOARD_EMULATION_QEMU_X86_Q35) > There's the `q35` variable already Done https://review.coreboot.org/c/coreboot/+/50267/comment/a5889cb8_d2b12b4c PS1, Line 111: size_t > uintptr_t Done -- To view, visit https://review.coreboot.org/c/coreboot/+/50267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3ae3659349d2a88bc3575fe9675433c054e28832 Gerrit-Change-Number: 50267 Gerrit-PatchSet: 1 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 03 Feb 2021 17:53:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/emulation/qemu-q35: Mark TSEG region as reserved
by Patrick Rudolph (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Patrick Rudolph. Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/50267 to look at the new patch set (#2). Change subject: mb/emulation/qemu-q35: Mark TSEG region as reserved ...................................................................... mb/emulation/qemu-q35: Mark TSEG region as reserved Mark TSEG as reserved, which is done on other platforms as well. For some reason CorebootPayloadPkg crashes when using the region where TSEG typically resides, which is basically RAM. UefiPayloadPkg doesn't show this issue. Change-Id: I3ae3659349d2a88bc3575fe9675433c054e28832 Signed-off-by: Patrick Rudolph <patrick.rudolph(a)9elements.com> --- M src/mainboard/emulation/qemu-i440fx/northbridge.c 1 file changed, 8 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/50267/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/50267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I3ae3659349d2a88bc3575fe9675433c054e28832 Gerrit-Change-Number: 50267 Gerrit-PatchSet: 2 Gerrit-Owner: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/broadwell: Convert some CONFIG(CHROMEOS) preprocessor
by Kyösti Mälkki (Code Review) 03 Feb '21

03 Feb '21

Attention is currently required from: Matt DeVillier, Duncan Laurie, Patrick Rudolph. Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50252 ) Change subject: soc/intel/broadwell: Convert some CONFIG(CHROMEOS) preprocessor ...................................................................... Patch Set 1: (1 comment) File src/soc/intel/broadwell/gma.c: https://review.coreboot.org/c/coreboot/+/50252/comment/b6746107_2a36c66a PS1, Line 518: !CONFIG(CHROMEOS) > might be able to just drop this and call display_init_required()? I think originally these function […] See CB:48787 unresolved comment. -- To view, visit https://review.coreboot.org/c/coreboot/+/50252 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie3feee0448175db2b6ed4e8e37d92de3af9be371 Gerrit-Change-Number: 50252 Gerrit-PatchSet: 1 Gerrit-Owner: Kyösti Mälkki <kyosti.malkki(a)gmail.com> Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com> Gerrit-Attention: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Wed, 03 Feb 2021 17:40:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-MessageType: comment

1 0

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2021