Attention is currently required from: Martin Roth, Chiranjeevi Rapolu, Tim Wawrzynczak, John Zhao, Duncan Laurie, Brandon Breitenstein, Patrick Rudolph.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51194 )
Change subject: soc/intel/tigerlake: Enable TCSS Muxes to disconnect mode during boot
......................................................................
Patch Set 6:
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51194/comment/de126bc3_a2347183
PS6, Line 8:
Commit message lines need to be max 75 characters: https://www.coreboot.org/Git#Commit_messages
File src/soc/intel/tigerlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/51194/comment/b5875222_4dfdc4a7
PS6, Line 243: bool "Enable early TCSS display"
bool "Enable early TCSS display" if EARLY_TCSS
so that the option is not visible in menuconfig if EARLY_TCSS is not selected. Also, can you please add EARLY_TCSS before EARLY_TCSS_DISPLAY here?
https://review.coreboot.org/c/coreboot/+/51194/comment/3f6211dc_6b71c457
PS6, Line 244: depends on RUN_FSP_GOP
depends on EARLY_TCSS & RUN_FSP_GOP
File src/soc/intel/tigerlake/early_tcss.c:
https://review.coreboot.org/c/coreboot/+/51194/comment/d141a7cf_75dc0be3
PS6, Line 61:
Unrelated change. Can be dropped.
https://review.coreboot.org/c/coreboot/+/51194/comment/13063bdf_0ecf371f
PS6, Line 326: single_port
nit: Since this is used in a single location, you can use `&port_map[i]` directly here.
https://review.coreboot.org/c/coreboot/+/51194/comment/50477bb2_cf7e1b87
PS6, Line 329: if(CONFIG(EARLY_TCSS_DISPLAY))
> space required before the open parenthesis '('
^^^ what Jenkins said.
File src/soc/intel/tigerlake/include/soc/early_tcss.h:
https://review.coreboot.org/c/coreboot/+/51194/comment/8117ef50_ff87d962
PS6, Line 152: Weak
That is not correct. Same for the above function.
--
To view, visit https://review.coreboot.org/c/coreboot/+/51194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4352072a4a7d6ccb1364b38377831f3c22ae8fb4
Gerrit-Change-Number: 51194
Gerrit-PatchSet: 6
Gerrit-Owner: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-Reviewer: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(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: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: John Zhao <john.zhao(a)intel.com>
Gerrit-Attention: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Attention: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 03 Mar 2021 20:37:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-MessageType: comment
Attention is currently required from: Marc Jones, Jonathan Zhang, Johnny Lin, Christian Walter, Subrata Banik, Patrick Rudolph, Deomid "rojer" Ryabkov.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51230 )
Change subject: soc/intel/xeon_sp/cpx: Set the MRC "cold boot required" status bit
......................................................................
Patch Set 5: Code-Review+1
(2 comments)
Patchset:
PS4:
> I don't think anyone particularly likes communicating via random CMOS locations but at the moment th […]
Sounds good. I'll let others have a look and share their thoughts, then will +2 if there are no objections. 😊
File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/51230/comment/3beec846_133ff24a
PS4, Line 132: uint8_t new_mrc_status = (mrc_status & 0xfe) | ((uint8_t) cold_boot_required);
> agree, but for whatever reason it is what it is.
Yeah... I'd suggest bringing this up with Intel, if possible.
--
To view, visit https://review.coreboot.org/c/coreboot/+/51230
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9c4191d2fa2e0203b3464dcf40d845ede5f14c6b
Gerrit-Change-Number: 51230
Gerrit-PatchSet: 5
Gerrit-Owner: Deomid "rojer" Ryabkov <rojer9(a)fb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Attention: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Deomid "rojer" Ryabkov <rojer9(a)fb.com>
Gerrit-Comment-Date: Wed, 03 Mar 2021 20:18:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Comment-In-Reply-To: Deomid "rojer" Ryabkov <rojer9(a)fb.com>
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Martin Roth, Chiranjeevi Rapolu, Tim Wawrzynczak, John Zhao, Duncan Laurie, Patrick Rudolph.
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51194 )
Change subject: soc/intel/tigerlake: Enable TCSS Muxes to disconnect mode during boot
......................................................................
Patch Set 6:
(7 comments)
File src/soc/intel/tigerlake/early_tcss.c:
https://review.coreboot.org/c/coreboot/+/51194/comment/6d930b8a_d685d033
PS5, Line 109: res
> The caller doesn't even care about res. […]
Done
https://review.coreboot.org/c/coreboot/+/51194/comment/7ae745dc_c7ce145c
PS5, Line 115: PMC_IPC_TCSS_DISC_REQ_RES,
: port_map.usb3_port,
: port_map.usb2_port,
: 0, 0, 0, 0);
> Can you please reflow this to fill in the 96-column limit?
Done
https://review.coreboot.org/c/coreboot/+/51194/comment/af4090da_1ddcdd63
PS5, Line 268: struct tcss_port_map port_map
> Instead of passing in the entire structure here, you can pass in a pointer to the entry? […]
Done
https://review.coreboot.org/c/coreboot/+/51194/comment/1011400e_11da2a75
PS5, Line 270: *rbuf = NULL
> going to add that patch to this patch chain
Done
https://review.coreboot.org/c/coreboot/+/51194/comment/19b17c82_a182cc24
PS5, Line 271: = 0
> Not required. This is set on line 273.
Done
https://review.coreboot.org/c/coreboot/+/51194/comment/c996b463_994a3844
PS5, Line 319: if (!display_init_required())
> if (!CONFIG(EARLY_TCSS_DISPLAY) || !display_init_required()) […]
Done
File src/soc/intel/tigerlake/include/soc/early_tcss.h:
https://review.coreboot.org/c/coreboot/+/51194/comment/06ad0b4b_f98f37de
PS5, Line 129: uint8_t usb3_port; /* USB2 Port Number */
: uint8_t usb2_port; /* USB3 Port Number */
> This can be dropped now that there is a separate structure for the port map?
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/51194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4352072a4a7d6ccb1364b38377831f3c22ae8fb4
Gerrit-Change-Number: 51194
Gerrit-PatchSet: 6
Gerrit-Owner: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-Reviewer: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(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: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: John Zhao <john.zhao(a)intel.com>
Gerrit-Attention: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 03 Mar 2021 20:16:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Martin Roth, Chiranjeevi Rapolu, Tim Wawrzynczak, John Zhao, Duncan Laurie, Patrick Rudolph.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51194 )
Change subject: soc/intel/tigerlake: Enable TCSS Muxes to disconnect mode during boot
......................................................................
Patch Set 6:
(1 comment)
File src/soc/intel/tigerlake/early_tcss.c:
https://review.coreboot.org/c/coreboot/+/51194/comment/12715476_f0f5f421
PS6, Line 329: if(CONFIG(EARLY_TCSS_DISPLAY))
space required before the open parenthesis '('
--
To view, visit https://review.coreboot.org/c/coreboot/+/51194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4352072a4a7d6ccb1364b38377831f3c22ae8fb4
Gerrit-Change-Number: 51194
Gerrit-PatchSet: 6
Gerrit-Owner: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-Reviewer: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(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: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: John Zhao <john.zhao(a)intel.com>
Gerrit-Attention: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 03 Mar 2021 20:14:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Martin Roth, Chiranjeevi Rapolu, Tim Wawrzynczak, John Zhao, Duncan Laurie, Patrick Rudolph.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh, Chiranjeevi Rapolu, Tim Wawrzynczak, John Zhao, Duncan Laurie, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/51194
to look at the new patch set (#6).
Change subject: soc/intel/tigerlake: Enable TCSS Muxes to disconnect mode during boot
......................................................................
soc/intel/tigerlake: Enable TCSS Muxes to disconnect mode during boot
TCSS muxes being left uninitialized during boot is causing some USB3 devices
to downgrade to USB2 speed. To properly configure the Type C ports the muxes
should be set to disconnected state during boot so that the port mapping of
USB2/3 devices is properly setup prior to Kernel initializing devices.
BUG=b:180426950
BRANCH=firmware-volteer-13672.B
TEST= Connected USB3 storage device and rebooted the system multiple times
to verify that the devices were no longer downgrading to USB2 speed.
Change-Id: I4352072a4a7d6ccb1364b38377831f3c22ae8fb4
Signed-off-by: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/Makefile.inc
M src/soc/intel/tigerlake/early_tcss.c
M src/soc/intel/tigerlake/fsp_params.c
M src/soc/intel/tigerlake/include/soc/early_tcss.h
5 files changed, 122 insertions(+), 52 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/51194/6
--
To view, visit https://review.coreboot.org/c/coreboot/+/51194
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4352072a4a7d6ccb1364b38377831f3c22ae8fb4
Gerrit-Change-Number: 51194
Gerrit-PatchSet: 6
Gerrit-Owner: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-Reviewer: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(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: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Chiranjeevi Rapolu <chiranjeevi.rapolu(a)intel.com>
Gerrit-Attention: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Attention: John Zhao <john.zhao(a)intel.com>
Gerrit-Attention: Duncan Laurie <dlaurie(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Furquan Shaikh, Brandon Breitenstein, Patrick Rudolph.
Brandon Breitenstein has uploaded a new patch set (#2) to the change originally created by Furquan Shaikh. ( https://review.coreboot.org/c/coreboot/+/51232 )
Change subject: soc/intel/tigerlake: Fix NULL being passed for response buffer
......................................................................
soc/intel/tigerlake: Fix NULL being passed for response buffer
`pmc_send_ipc_cmd()` expects the caller to pass in a pointer to a valid
request and response buffer. However, early_tcss driver was passing in
a NULL pointer for response buffer which would result in invalid
access by `pmc_send_ipc_cmd()`.
Currently, the response buffer is not used in `update_tcss_mux()`. So,
this change drops the passing of `rbuf` parameter to `send_pmc*`
helpers and instead uses a local `rsp` variable in the respective
functions. All the PMC functions used in early_tcss driver return some
kind of response. These should be checked to return appropriate
response code back to the caller. However, this needs to be done as a
separate change.
Change-Id: I215af85feed60b6beee17f28e3d65daa9ad4ae69
Signed-off-by: Furquan Shaikh <furquan(a)google.com>
---
M src/soc/intel/tigerlake/early_tcss.c
1 file changed, 14 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/32/51232/2
--
To view, visit https://review.coreboot.org/c/coreboot/+/51232
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I215af85feed60b6beee17f28e3d65daa9ad4ae69
Gerrit-Change-Number: 51232
Gerrit-PatchSet: 2
Gerrit-Owner: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Brandon Breitenstein <brandon.breitenstein(a)intel.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: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Marc Jones, Jonathan Zhang, Johnny Lin, Christian Walter, Angel Pons, Subrata Banik, Patrick Rudolph.
Deomid "rojer" Ryabkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51230 )
Change subject: soc/intel/xeon_sp/cpx: Set the MRC "cold boot required" status bit
......................................................................
Patch Set 5:
(1 comment)
Patchset:
PS4:
> Uh, FSP shouldn't be using a hardcoded offset from CMOS... […]
I don't think anyone particularly likes communicating via random CMOS locations but at the moment this is needed to fix boot time regression. I'll let Intel folks clarify future plans around this.
--
To view, visit https://review.coreboot.org/c/coreboot/+/51230
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9c4191d2fa2e0203b3464dcf40d845ede5f14c6b
Gerrit-Change-Number: 51230
Gerrit-PatchSet: 5
Gerrit-Owner: Deomid "rojer" Ryabkov <rojer9(a)fb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com>
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: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 03 Mar 2021 20:08:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Jonathan Zhang <jonzhang(a)fb.com>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Martin Roth, Mariusz Szafrański, Michal Motyl.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51180 )
Change subject: util/sconfig: Fix for multidomain support sconfig/devicetree.cb
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/51180/comment/415f13a2_2645b761
PS2, Line 7: multidomain
> We can do it over email or chat or meeting if you prefer that. […]
If it's okay with you, I'd be interested in knowing the use-cases for multi-domain support. My email is th3fanbus(a)gmail.com
--
To view, visit https://review.coreboot.org/c/coreboot/+/51180
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idd639d0cb3ed1a49ed7c7b1c77ac747ba6f77672
Gerrit-Change-Number: 51180
Gerrit-PatchSet: 2
Gerrit-Owner: Mariusz Szafrański <mariuszx.szafranski(a)intel.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Michal Motyl <michalx.motyl(a)intel.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: Suresh Bellampalli <suresh.bellampalli(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Mariusz Szafrański <mariuszx.szafranski(a)intel.com>
Gerrit-Attention: Michal Motyl <michalx.motyl(a)intel.com>
Gerrit-Comment-Date: Wed, 03 Mar 2021 20:02:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Mariusz Szafrański <mariuszx.szafranski(a)intel.com>
Gerrit-MessageType: comment
Attention is currently required from: Marc Jones, Jonathan Zhang, Johnny Lin, Christian Walter, Angel Pons, Subrata Banik, Patrick Rudolph.
Deomid "rojer" Ryabkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/51230 )
Change subject: soc/intel/xeon_sp/cpx: Set the MRC "cold boot required" status bit
......................................................................
Patch Set 4:
(5 comments)
File src/soc/intel/xeon_sp/cpx/chip.h:
https://review.coreboot.org/c/coreboot/+/51230/comment/672ef06e_9855f717
PS4, Line 107: #define CMOS_MRC_STATUS_ADDR 0x47
> Let's make this more robust. […]
Done
File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/51230/comment/580ce63f_574a1558
PS4, Line 128:
> Since the CMOS offset is only used in this file, I'd move its definition right here. […]
Done
https://review.coreboot.org/c/coreboot/+/51230/comment/0aa69ca3_49938c98
PS4, Line 132: ((uint8_t)
> cast should not be necessary
Done
https://review.coreboot.org/c/coreboot/+/51230/comment/6c5f2571_c09967eb
PS4, Line 132: uint8_t new_mrc_status = (mrc_status & 0xfe) | ((uint8_t) cold_boot_required);
> Wouldn't it make more sense if cold_boot_required == 0? This way, if a machine loses RTC power, it w […]
agree, but for whatever reason it is what it is.
https://review.coreboot.org/c/coreboot/+/51230/comment/25a70a31_670b63ad
PS4, Line 207: set_cmos_mrc_cold_boot_flag(cold_boot_required);
> Simpler: […]
not as readable imo, but ok.
--
To view, visit https://review.coreboot.org/c/coreboot/+/51230
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9c4191d2fa2e0203b3464dcf40d845ede5f14c6b
Gerrit-Change-Number: 51230
Gerrit-PatchSet: 4
Gerrit-Owner: Deomid "rojer" Ryabkov <rojer9(a)fb.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Christian Walter <christian.walter(a)9elements.com>
Gerrit-Reviewer: Johnny Lin <Johnny_Lin(a)wiwynn.com>
Gerrit-Reviewer: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Jonathan Zhang <jonzhang(a)fb.com>
Gerrit-Attention: Marc Jones <marc(a)marcjonesconsulting.com>
Gerrit-Attention: Jonathan Zhang <jonzhang(a)fb.com>
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: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Comment-Date: Wed, 03 Mar 2021 20:00:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment