Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56275 )
Change subject: soc/amd/picasso: implement and use mca_is_valid_bank
......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/56275/comment/bbb7c42f_117d5a32
PS1, Line 142: ""
> Does that mean this should be NULL instead?
all MSRs we want to access exist for this bank, so no need to do this. setting it to NULL is basically only to work around the problem on stoneyridge where we'd be getting general protection faults. so it's still a valid MCAX bank; it just won't do anything if i read the docs correctly
--
To view, visit https://review.coreboot.org/c/coreboot/+/56275
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0c7f3066afd220e6b8bf8308a321189d7a2679f6
Gerrit-Change-Number: 56275
Gerrit-PatchSet: 1
Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Tue, 13 Jul 2021 21:41:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56275 )
Change subject: soc/amd/picasso: implement and use mca_is_valid_bank
......................................................................
Patch Set 1:
(2 comments)
File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/56275/comment/95efcdb2_daf4f13b
PS1, Line 142: ""
Does that mean this should be NULL instead?
https://review.coreboot.org/c/coreboot/+/56275/comment/a43904f4_08f23f02
PS1, Line 197: continue
> at least on stoneyridge doing that would cause false warnings. see https://review.coreboot. […]
Ack
--
To view, visit https://review.coreboot.org/c/coreboot/+/56275
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0c7f3066afd220e6b8bf8308a321189d7a2679f6
Gerrit-Change-Number: 56275
Gerrit-PatchSet: 1
Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.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: Tue, 13 Jul 2021 21:34:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Comment-In-Reply-To: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56275 )
Change subject: soc/amd/picasso: implement and use mca_is_valid_bank
......................................................................
Patch Set 1:
(1 comment)
File src/soc/amd/picasso/mca.c:
https://review.coreboot.org/c/coreboot/+/56275/comment/91396989_6454bbb8
PS1, Line 197: continue
> Should we print a warning saying that bank is not valid?
at least on stoneyridge doing that would cause false warnings. see https://review.coreboot.org/c/coreboot/+/56277/1/src/soc/amd/stoneyridge/mc… where the same mechanism is used to be able to skip one mca bank that's not fully implemented and will cause a general protection fault if at least some registers are accessed.
haven't finished the full patch series yer, but basically my plan is to implement mca_is_valid_bank and mca_get_bank_name to access mca_bank_name[] for each soc and have one common non-x mca implementation and one common mcax implementation with some common mca code that is common for both mca and mcax
--
To view, visit https://review.coreboot.org/c/coreboot/+/56275
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I0c7f3066afd220e6b8bf8308a321189d7a2679f6
Gerrit-Change-Number: 56275
Gerrit-PatchSet: 1
Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Jason Glenesk <jason.glenesk(a)gmail.com>
Gerrit-Attention: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Comment-Date: Tue, 13 Jul 2021 21:29:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Raul Rangel <rrangel(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Jason Glenesk, Marshall Dawson, Felix Held.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56273 )
Change subject: soc/amd/picasso: add missing banks to mca_bank_name array
......................................................................
Patch Set 1:
(10 comments)
File src/soc/amd/picasso/mca.c:
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/0faa3506_12479aae
PS1, Line 138: [ 0] = "Load-store unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/29b2316e_0a7c5a3b
PS1, Line 139: [ 1] = "Instruction fetch unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/4bde885a_2c9e6a14
PS1, Line 140: [ 2] = "L2 cache unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/6b464250_d8d74735
PS1, Line 141: [ 3] = "Decode unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/c7e972b5_61661208
PS1, Line 142: [ 4] = "",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/8aa102da_a4bf6c77
PS1, Line 143: [ 5] = "Execution unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/9c14e8f5_46145f7d
PS1, Line 144: [ 6] = "Floating point unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/74e2d4a8_9d89e8ac
PS1, Line 145: [ 7] = "L3 cache unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/32e761fe_66f97713
PS1, Line 146: [ 8] = "L3 cache unit",
space prohibited after that open square bracket '['
Robot Comment from checkpatch (run ID jenkins-coreboot-checkpatch-124051):
https://review.coreboot.org/c/coreboot/+/56273/comment/f26b5a10_c865c622
PS1, Line 147: [ 9] = "L3 cache unit",
space prohibited after that open square bracket '['
--
To view, visit https://review.coreboot.org/c/coreboot/+/56273
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I10a65210da73e64b67d613609fcc0f9a245a81fb
Gerrit-Change-Number: 56273
Gerrit-PatchSet: 1
Gerrit-Owner: 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: Raul Rangel <rrangel(a)chromium.org>
Gerrit-CC: 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: Tue, 13 Jul 2021 21:22:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Furquan Shaikh, Michał Żygowski, Marshall Dawson, Subrata Banik, Nikolai Vyssotski, Andrey Petrov, Patrick Rudolph, Nathaniel L Desimone.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56190 )
Change subject: src/drivers/intel/fsp2_0: allow larger FSP 2.0 header
......................................................................
Patch Set 3:
(1 comment)
File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/56190/comment/0c277a7b_b5a1ec02
PS2, Line 13: however, some FSP 2.0 variants have it as well.
> > The FSP header has a length field. […]
Nick, do we meet all the other requirements for FSP 2.2? I thought AGESA FSP was only FSP 2.0 compliant? If that's the case, I'm not entirely sure we want it to advertise that it's a 2.2 compliant FSP.
If other people think that's the best solution, I won't argue, but I think it might lead to other issues in the long run.
--
To view, visit https://review.coreboot.org/c/coreboot/+/56190
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ie8422447b2cff0a6c536e13014905ffa15c70586
Gerrit-Change-Number: 56190
Gerrit-PatchSet: 3
Gerrit-Owner: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com>
Gerrit-Reviewer: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Reviewer: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.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: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-CC: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Gerrit-Attention: Marshall Dawson <marshalldawson3rd(a)gmail.com>
Gerrit-Attention: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Attention: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com>
Gerrit-Attention: Andrey Petrov <andrey.petrov(a)gmail.com>
Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Attention: Nathaniel L Desimone <nathaniel.l.desimone(a)intel.com>
Gerrit-Comment-Date: Tue, 13 Jul 2021 21:15:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Comment-In-Reply-To: Martin Roth <martinroth(a)google.com>
Comment-In-Reply-To: Michał Żygowski <michal.zygowski(a)3mdeb.com>
Comment-In-Reply-To: Nikolai Vyssotski <nikolai.vyssotski(a)amd.corp-partner.google.com>
Gerrit-MessageType: comment
Attention is currently required from: Martin Roth, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56278 )
Change subject: src/device: Remove DEVICE_PATH_ESPI & DEVICE_PATH_LPC
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/56278
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I9ae7817bb63d69ee272103b2d1186f125e188950
Gerrit-Change-Number: 56278
Gerrit-PatchSet: 1
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 13 Jul 2021 21:10:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Martin Roth, Felix Held.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56197 )
Change subject: util/sconfig: Remove unused devicetree keywords ESPI & LPC
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/56197
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3a78afc55477d62eac8056e2ca4bcdd3ab12ea47
Gerrit-Change-Number: 56197
Gerrit-PatchSet: 2
Gerrit-Owner: Martin Roth <martinroth(a)google.com>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Raul Rangel <rrangel(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Martin Roth <martinroth(a)google.com>
Gerrit-Attention: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Comment-Date: Tue, 13 Jul 2021 21:10:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment