Attention is currently required from: Felix Singer, Angel Pons, Arthur Heymans, Patrick Rudolph.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49287 )
Change subject: nb/intel/sandybridge: Fix programming command timings
......................................................................
Patch Set 1: Code-Review-1
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49287/comment/324ac70a_1eb834dc
PS1, Line 11:
That's basically an empty commit message. If you want to avoid documenting
what the interim code did wrong, how about a revert and then let people
review against the original code?
Patchset:
PS1:
Looks like this is mixing refactorings with a fix for a
commit that mixed so many refactorings together that it
broke things unnoticed... please stop this mess.
I've also left some `resolved` comments that would make
it much easier to review if they were handled in advance.
File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/49287/comment/d21c1302_c2c4d89c
PS1, Line 943: FOR_ALL_POPULATED_RANKS {
Please consider making these macros readable before any further reviews.
https://review.coreboot.org/c/coreboot/+/49287/comment/c48d5364_1ed01386
PS1, Line 939: /*
: * Compute command phase shift as the most negative CCC setting
: * across all ranks. Use zero if none of the values is negative.
: */
: FOR_ALL_POPULATED_RANKS {
: cmd_delay = MAX(cmd_delay, -ctrl->timings[channel][slotrank].pi_coding);
: }
:
This part looks like a mere refactoring. What's the fix? did I miss something?
https://review.coreboot.org/c/coreboot/+/49287/comment/337a02bc_ad161a8b
PS1, Line 1000: gdcr_cmd_pi_coding_reg
This type is only used once. If there is no intention to change that, please
please please kill it. The type information is very valuable to the reader
and should only be hidden in a header file as a last resort (if shared by
multiple compilation units). You can also declare it right here as anonymous
union.
--
To view, visit https://review.coreboot.org/c/coreboot/+/49287
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I33b2271803e6fa3d8e625bfe5a830152cab2b9d4
Gerrit-Change-Number: 49287
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Mon, 11 Jan 2021 23:26:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Evan Green, Tim Wawrzynczak, Karthik Ramasubramanian.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48623 )
Change subject: mb/google/dedede/var/boten: Update LTE GPIO configuration
......................................................................
Patch Set 3:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/48623/comment/572bc16e_02dbabb6
PS3, Line 9: LTE module is not expected to be powered off during warm reset
Is there any expectation w.r.t. LTE firmware reload after a warm reset? (OS updates trigger a warm reset).
--
To view, visit https://review.coreboot.org/c/coreboot/+/48623
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I8676da6186559288aabe078b6158fc01075c7b41
Gerrit-Change-Number: 48623
Gerrit-PatchSet: 3
Gerrit-Owner: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Reviewer: Evan Green <evgreen(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Allen Cheng <allen.cheng(a)lcfc.corp-partner.google.com>
Gerrit-CC: Jerry2 Huang <jerry2.huang(a)lcfc.corp-partner.google.com>
Gerrit-CC: Kevin Chang <kevin.chang(a)lcfc.corp-partner.google.com>
Gerrit-CC: Rasheed Hsueh <rasheed.hsueh(a)lcfc.corp-partner.google.com>
Gerrit-CC: Stanley Wu <stanley1.wu(a)lcfc.corp-partner.google.com>
Gerrit-Attention: Evan Green <evgreen(a)chromium.org>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)google.com>
Gerrit-Attention: Karthik Ramasubramanian <kramasub(a)google.com>
Gerrit-Comment-Date: Mon, 11 Jan 2021 22:44:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak, Subrata Banik.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49301 )
Change subject: soc/intel/alderlake: Add PCH ID 0x5182
......................................................................
Patch Set 1:
(1 comment)
File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/49301/comment/6f77ae65_a04b5428
PS1, Line 308: PCI_DEVICE_ID_INTEL_ADP_P_ESPI_32,
: PCI_DEVICE_ID_INTEL_ADP_P_ESPI_33,
nit: Place this up with rest of the ADP_P_ESPI above?
--
To view, visit https://review.coreboot.org/c/coreboot/+/49301
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia331998b46abcf10e939078dea992589f09139bd
Gerrit-Change-Number: 49301
Gerrit-PatchSet: 1
Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: EricR Lai <ericr_lai(a)compal.corp-partner.google.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: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Comment-Date: Mon, 11 Jan 2021 22:42:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Marc Jones, Patrick Rudolph, Johnny Lin, Jingle Hsu, Angel Pons, Morgan Jang, Tim Chu.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49235 )
Change subject: ipmi/ocp: Move common OCP/Facebook IPMI OEM codes into drivers/ipmi/ocp
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit https://review.coreboot.org/c/coreboot/+/49235
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idd116a89239273fd5cc7b06c7768146085a3ed69
Gerrit-Change-Number: 49235
Gerrit-PatchSet: 4
Gerrit-Owner: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Jingle Hsu <jingle_hsu(a)wiwynn.com>
Gerrit-Reviewer: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Reviewer: Morgan Jang <Morgan_Jang(a)wiwynn.com>
Gerrit-Reviewer: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Reviewer: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Attention: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Attention: Patrick Rudolph <patrick.rudolph(a)9elements.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Jingle Hsu <jingle_hsu(a)wiwynn.com>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Morgan Jang <Morgan_Jang(a)wiwynn.com>
Gerrit-Attention: Tim Chu <Tim.Chu(a)quantatw.com>
Gerrit-Comment-Date: Mon, 11 Jan 2021 22:32:50 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Tim Wawrzynczak, Nick Vaccaro, Srinidhi N Kaushik, Raj Astekar, Patrick Rudolph.
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49244 )
Change subject: soc/intel/tigerlake: Enable TC Cold support
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
This change looks good, but I'm running into an unrelated flashrom failure that is currently keeping me from being able to verify that my volteer2 boots with this change.
Has anyone checked this on a volteer2?
--
To view, visit https://review.coreboot.org/c/coreboot/+/49244
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0ad0322693b4f8fbf1000b24eb21dddcebec686b
Gerrit-Change-Number: 49244
Gerrit-PatchSet: 3
Gerrit-Owner: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Attention: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com>
Gerrit-Attention: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Mon, 11 Jan 2021 22:27:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Michael Niewöhner.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48922 )
Change subject: util/superiotool: add IT5570E registers
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Patchset:
PS3:
sorry this one got list in my inbox
--
To view, visit https://review.coreboot.org/c/coreboot/+/48922
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idc764c6180e235298835d7639fcb0b562a2c21a4
Gerrit-Change-Number: 48922
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Michael Niewöhner <foss(a)mniewoehner.de>
Gerrit-Comment-Date: Mon, 11 Jan 2021 21:56:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment