Attention is currently required from: Furquan Shaikh, Subrata Banik, Patrick Rudolph.
Tim Wawrzynczak 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/bc2b2673_e93eac54
PS2, Line 9: 2017.00
> Does this also match the upcoming 2037 release? or is another update required?
Yes they match, `git diff upstream/ADL.2017.00 upstream/ADL.2037.00 MemInfoHob.h` shows nothing
--
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: Furquan Shaikh <furquan(a)google.com>
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 19:00:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment
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
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
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
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
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
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
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