coreboot-gerrit February 2020

coreboot-gerrit@coreboot.org

1 participants
1797 discussions

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Sridhar Siricilla (Code Review) 20 Feb '20

20 Feb '20

Sridhar Siricilla has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 ) Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... Patch Set 64: (21 comments) https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/cse_bp.c: https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 32: boot_partition_info_entry > I think this can be just name cse_boot_partition_entry or cse_bp_entry? Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 116: cse_send_set_boot_partition_info_cmd > The command name is SET_BOOT_PARTITION_INFO. […] I will rename it as cse_send_set_bp_info() https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 161: cse_send_set_boot_partition_info_cmd > I am curious why CSE cannot just return a status in response to MKHI_SET_BOOT_PARTITION_CMD_REQ indi […] If response strcut's result field indicates success, then it means CSE updated the next boot partition. Line #170 to 175 are just a redundant. I removed the code. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 223: boot_partition > Same comment as above. […] Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 284: me_rw_status > In fact, why is there a need to introduce a new ME RW status? I think this function can simply be: […] The FW update driver checks for 3 conditions (ME_RW_VALID/INVALID/SIGN_MISMATCH). The partitcular function checks for RW_VALID/INVALID conditions. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 284: bool > Does this really need to be a bool? Can't we just return the state directly as the return value? Yes, it requires since there is a check for error condition. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 313: ME: %s version = %d.%d.%d.%d > Each SoC has its own way of printing ME version. […] All soc/cse_bp are logging cse version in same format(major.minor.hotfix.build). Currently, I see CSE version(one here, and another one in SoC) is being logged from 2 sources. I agree with you on having unified call which takes care of underlying ME Firmware SKU type, and logs firmware version. I will push new patchset accordingly. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… File src/soc/intel/common/block/include/intelblocks/cse.h: https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 214: BP_STATUS_MANIFEST_AND_INFO_OK > BP_STATUS_MANIFEST_AND_INFO_OK is no longer valid status code. I removed it. Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 216: I > nit: lowercase 'i'. Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 221: */ > It would be good to mentioned that this is returned in response to MKHI command for GET_PARTITION_IN […] Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 222: bp_info_flags > I think it would be good to be consistent with the other enums i.e. […] The flag represents value of cse_boot_partition_info struct. Hence the enum tag is named to match with it's struct. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 224: Boot Partition Info Flag > nit: All these are boot partition info flags and that is already indicated in the comment before the […] Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 232: RO > Done Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 239: BP_INFO_MIN_RECOV_MODE_EN > Why? Shouldn't this be BP_INFO_REDUNDANCY_EN? Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 253: ME_RW_SIGN_INVALID > ME_RW_INVALID/VALID are determined based on the boot partition status (BP_STATUS_GENERAL_FAILURE) Wh […] These status codes are not required to be part of this file. I will use CSE FW update file itself. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 257: partition_fw_version > BP2 and BP3 always have same versions.But RO(BP1) and RW(BP2/BP3) can have different versions. […] Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 264: * > I see a number of functions being exposed here. […] No, these functions won't be used outside of fw update driver. These functions will be used as helper functions by CSE FW Update driver. https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 267: > can/should? Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 270: Returns false on failure and true on success. > I agree with you on having common scheme for return values. […] Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 290: uint8_t > API returns member field "flags" of cse_boot_partition_info struct. Done https://review.coreboot.org/c/coreboot/+/35402/44/src/soc/intel/common/bloc… PS44, Line 316: bool > Error and disabled cases are handled differently in CSE FW Update library so modifying return values […] Done -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 64 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 20 Feb 2020 08:46:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by build bot (Jenkins) (Code Review) 20 Feb '20

20 Feb '20

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35402 ) Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... Patch Set 64: (14 comments) https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/cse_bp.c: https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 215: if(!cse_get_bp_entry(bp, &cse_bp)) space required before the open parenthesis '(' https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 229: if(!cse_get_bp_entry(bp, &cse_bp)) space required before the open parenthesis '(' https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 243: if(!cse_get_bp_entry(bp, &cse_bp)) space required before the open parenthesis '(' https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 310: if(!cse_get_bp_entry(bp, &cse_bp)) space required before the open parenthesis '(' https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 323: if(!cse_get_bp_entry(bp, &cse_bp)) space required before the open parenthesis '(' https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… File src/soc/intel/common/block/include/intelblocks/cse.h: https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 298: /* ME RW region is valid */ code indent should use tabs where possible https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 299: ME_RW_VALID = 0, code indent should use tabs where possible https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 299: ME_RW_VALID = 0, please, no spaces at the start of a line https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 301: /* ME RW Update is required */ code indent should use tabs where possible https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 302: ME_RW_INVALID = 1, code indent should use tabs where possible https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 302: ME_RW_INVALID = 1, please, no spaces at the start of a line https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 304: /* ME RW Signature is invalid */ code indent should use tabs where possible https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 305: ME_RW_SIGN_INVALID = 2, code indent should use tabs where possible https://review.coreboot.org/c/coreboot/+/35402/64/src/soc/intel/common/bloc… PS64, Line 305: ME_RW_SIGN_INVALID = 2, please, no spaces at the start of a line -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 64 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Comment-Date: Thu, 20 Feb 2020 08:45:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/block/cse: Add boot partition related APIs
by Sridhar Siricilla (Code Review) 20 Feb '20

20 Feb '20

Hello Patrick Rudolph, Subrata Banik, Balaji Manigandan, Aamir Bohra, Sridhar Siricilla, Rizwan Qureshi, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, V Sowmya, Nico Huber, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/35402 to look at the new patch set (#64). Change subject: soc/intel/common/block/cse: Add boot partition related APIs ...................................................................... soc/intel/common/block/cse: Add boot partition related APIs The CSE region is logically divided into 3 boot partitions when redundancy is enabled. These boot partitions are represented by BP1, BP2 and BP3. In chrome platforms, CSE can boot from either BP1 or BP2. The CSE image layout appears as below.. ------------- -------------------- -------------------------- |CSE REGION | => | RO | RW | DATA | => | BP1 | BP2 + BP3 | DATA | ------------- -------------------- -------------------------- In order to support CSE FW update to RW region, below APIs help coreboot to get info about the boot partitions, and allows coreboot to set CSE to boot from required boot partition (either BP1(RO) or BP2). GET_BOOT_PARTITION_INFO - provides info on available partitions in the CSE region. The API provides info on boot partitions like start/end offsets of a partition within CSE region, and their version and partition status. SET_BOOT_PARTITION_INFO - Sets next boot partition to boot for CSE. With the HECI API, firmware can notify CSE to boot from BP1 or BP2 on next boot. BUG=b:145809764 Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Signed-off-by: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Signed-off-by: Sridhar Siricilla <sridhar.siricilla(a)intel.com> --- M src/soc/intel/common/block/cse/Makefile.inc A src/soc/intel/common/block/cse/cse_bp.c M src/soc/intel/common/block/include/intelblocks/cse.h 3 files changed, 496 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35402/64 -- To view, visit https://review.coreboot.org/c/coreboot/+/35402 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Iaa62409c0616d5913d21374a8a6804f82258eb4f Gerrit-Change-Number: 35402 Gerrit-PatchSet: 64 Gerrit-Owner: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.com> Gerrit-Reviewer: Sridhar Siricilla <sridhar.siricilla(a)intel.corp-partner.google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Andrey Petrov <anpetrov(a)fb.com> Gerrit-CC: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/common/block: Move smihandler common functions into common ...
by Subrata Banik (Code Review) 20 Feb '20

20 Feb '20

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/26138 ) Change subject: soc/intel/common/block: Move smihandler common functions into common code ...................................................................... Patch Set 41: (1 comment) https://review.coreboot.org/c/coreboot/+/26138/40/src/soc/intel/apollolake/… File src/soc/intel/apollolake/include/soc/pm.h: https://review.coreboot.org/c/coreboot/+/26138/40/src/soc/intel/apollolake/… PS40, Line 111: PMC_OCP_SMI_STS > Ack please refer this https://review.coreboot.org/c/coreboot/+/39022 -- To view, visit https://review.coreboot.org/c/coreboot/+/26138 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic082bc5d556dd19617d83ab86f93a53574b5bc03 Gerrit-Change-Number: 26138 Gerrit-PatchSet: 41 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com> Gerrit-Reviewer: Krzysztof M Sywula <krzysztof.m.sywula(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 20 Feb 2020 07:07:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Subrata Banik <subrata.banik(a)intel.com> Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/block: Move cse common functions into block/cse
by Subrata Banik (Code Review) 20 Feb '20

20 Feb '20

Hello Patrick Rudolph, Aaron Durbin, Angel Pons, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Duncan Laurie, Paul Menzel, build bot (Jenkins), Hannah Williams, Furquan Shaikh, Patrick Georgi, Pratikkumar V Prajapati, V Sowmya, Lance Zhao, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/26133 to look at the new patch set (#39). Change subject: soc/intel/common/block: Move cse common functions into block/cse ...................................................................... soc/intel/common/block: Move cse common functions into block/cse This patch cleans soc/intel/{cnl, icl, tgl} by moving common soc code into common/block/cse. Supported SoC can select existing HECI_DISABLE_USING_SMM option to select common cse code block to make heci function disable using sideband interface during SMM mode at preboot envionment. BUG=b:78109109 TEST=Able to make HECI disable in SMM mode successfully without any hang or errors in CNL, ICL and TGL platform. Change-Id: I22a4cc05d3967c7653d2abe2c829b4876516d179 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> Signed-off-by: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> --- M src/soc/intel/cannonlake/smihandler.c M src/soc/intel/common/block/cse/Kconfig M src/soc/intel/common/block/cse/Makefile.inc A src/soc/intel/common/block/cse/disable_heci.c M src/soc/intel/common/block/include/intelblocks/cse.h M src/soc/intel/icelake/smihandler.c M src/soc/intel/tigerlake/smihandler.c 7 files changed, 76 insertions(+), 117 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/26133/39 -- To view, visit https://review.coreboot.org/c/coreboot/+/26133 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22a4cc05d3967c7653d2abe2c829b4876516d179 Gerrit-Change-Number: 26133 Gerrit-PatchSet: 39 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/common/block: Move smihandler common functions into common ...
by Subrata Banik (Code Review) 20 Feb '20

20 Feb '20

Hello Aaron Durbin, Patrick Rudolph, Angel Pons, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, Jeremy Soller, Duncan Laurie, Paul Menzel, Shelley Chen, build bot (Jenkins), Hannah Williams, Furquan Shaikh, Patrick Georgi, Pratikkumar V Prajapati, V Sowmya, Lance Zhao, Krzysztof M Sywula, Martin Roth, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/26138 to look at the new patch set (#41). Change subject: soc/intel/common/block: Move smihandler common functions into common code ...................................................................... soc/intel/common/block: Move smihandler common functions into common code This patch cleans soc/intel/{apl/cnl/skl/icl/tgl} by moving common soc code into common/block/smihandler.c BUG=b:78109109 TEST=Build and boot KBL/CNL/APL/ICL/TGL platform. Change-Id: Ic082bc5d556dd19617d83ab86f93a53574b5bc03 Signed-off-by: Subrata Banik <subrata.banik(a)intel.com> --- M src/soc/intel/apollolake/smihandler.c M src/soc/intel/cannonlake/smihandler.c M src/soc/intel/common/block/include/intelblocks/smihandler.h M src/soc/intel/common/block/smm/smihandler.c M src/soc/intel/icelake/smihandler.c M src/soc/intel/skylake/smihandler.c M src/soc/intel/tigerlake/smihandler.c 7 files changed, 44 insertions(+), 178 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/26138/41 -- To view, visit https://review.coreboot.org/c/coreboot/+/26138 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic082bc5d556dd19617d83ab86f93a53574b5bc03 Gerrit-Change-Number: 26138 Gerrit-PatchSet: 41 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com> Gerrit-Reviewer: Krzysztof M Sywula <krzysztof.m.sywula(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: newpatchset

1 0

Change in coreboot[master]: soc/intel/common/block: Move smihandler common functions into common ...
by Subrata Banik (Code Review) 20 Feb '20

20 Feb '20

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/26138 ) Change subject: soc/intel/common/block: Move smihandler common functions into common code ...................................................................... Patch Set 40: (1 comment) https://review.coreboot.org/c/coreboot/+/26138/40/src/soc/intel/apollolake/… File src/soc/intel/apollolake/include/soc/pm.h: https://review.coreboot.org/c/coreboot/+/26138/40/src/soc/intel/apollolake/… PS40, Line 111: PMC_OCP_SMI_STS > I think it would be good to change all these macros to be consistent i.e. ending with _SMI_STS_BIT. Ack -- To view, visit https://review.coreboot.org/c/coreboot/+/26138 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ic082bc5d556dd19617d83ab86f93a53574b5bc03 Gerrit-Change-Number: 26138 Gerrit-PatchSet: 40 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Jeremy Soller <jeremy(a)system76.com> Gerrit-Reviewer: Krzysztof M Sywula <krzysztof.m.sywula(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Shelley Chen <shchen(a)google.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 20 Feb 2020 07:06:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: soc/intel/common/block: Move cse common functions into block/cse
by Subrata Banik (Code Review) 20 Feb '20

20 Feb '20

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/26133 ) Change subject: soc/intel/common/block: Move cse common functions into block/cse ...................................................................... Patch Set 38: (2 comments) https://review.coreboot.org/c/coreboot/+/26133/37//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/26133/37//COMMIT_MSG@17 PS37, Line 17: Build and boot CNL, ICL and TGL platform. > and HECI was disabled in SMM successfully without hangs or errors? Ack https://review.coreboot.org/c/coreboot/+/26133/37/src/soc/intel/common/bloc… File src/soc/intel/common/block/cse/disable_heci.c: https://review.coreboot.org/c/coreboot/+/26133/37/src/soc/intel/common/bloc… PS37, Line 54: && > Shouldn't this be ||? i.e. […] make sense -- To view, visit https://review.coreboot.org/c/coreboot/+/26133 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I22a4cc05d3967c7653d2abe2c829b4876516d179 Gerrit-Change-Number: 26133 Gerrit-PatchSet: 38 Gerrit-Owner: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: Aamir Bohra <aamir.bohra(a)intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com> Gerrit-Reviewer: Lance Zhao <lance.zhao(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Maulik V Vaghela <maulik.v.vaghela(a)intel.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Pratikkumar V Prajapati <pratikkumar.v.prajapati(a)intel.com> Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com> Gerrit-Reviewer: V Sowmya <v.sowmya(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Thu, 20 Feb 2020 07:04:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com> Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/intel/tglrvp: add Tiger Lake memory initialization support
by Nick Vaccaro (Code Review) 20 Feb '20

20 Feb '20

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38998 ) Change subject: mb/intel/tglrvp: add Tiger Lake memory initialization support ...................................................................... Patch Set 4: (2 comments) https://review.coreboot.org/c/coreboot/+/38998/4//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/38998/4//COMMIT_MSG@15 PS4, Line 15: BUG=b:none BUG=none https://review.coreboot.org/c/coreboot/+/38998/4/src/mainboard/intel/tglrvp… File src/mainboard/intel/tglrvp/variants/tglrvp_up3/memory.c: https://review.coreboot.org/c/coreboot/+/38998/4/src/mainboard/intel/tglrvp… PS4, Line 54: /* Rcomp resistor */ Are these (rcomp_resistor and RcompTarget) needed given all three memory options are lpddr4x? -- To view, visit https://review.coreboot.org/c/coreboot/+/38998 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7248862efd1dcd5a0df0e17d39b44c168caa200e Gerrit-Change-Number: 38998 Gerrit-PatchSet: 4 Gerrit-Owner: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Comment-Date: Thu, 20 Feb 2020 04:38:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in coreboot[master]: mb/intel/tglrvp: add Tiger Lake memory initialization support
by Nick Vaccaro (Code Review) 20 Feb '20

20 Feb '20

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38998 ) Change subject: mb/intel/tglrvp: add Tiger Lake memory initialization support ...................................................................... Patch Set 4: (2 comments) https://review.coreboot.org/c/coreboot/+/38998/4/src/mainboard/intel/tglrvp… File src/mainboard/intel/tglrvp/romstage_fsp_params.c: https://review.coreboot.org/c/coreboot/+/38998/4/src/mainboard/intel/tglrvp… PS4, Line 21: #include "board_id.h" nit - please move local double-quoted includes to bottom of includes list https://review.coreboot.org/c/coreboot/+/38998/4/src/mainboard/intel/tglrvp… File src/mainboard/intel/tglrvp/spd/Makefile.inc: https://review.coreboot.org/c/coreboot/+/38998/4/src/mainboard/intel/tglrvp… PS4, Line 18: += first setting of SPD_SOURCES should use "=" -- To view, visit https://review.coreboot.org/c/coreboot/+/38998 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I7248862efd1dcd5a0df0e17d39b44c168caa200e Gerrit-Change-Number: 38998 Gerrit-PatchSet: 4 Gerrit-Owner: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com> Gerrit-Reviewer: Ravishankar Sarawadi <ravishankar.sarawadi(a)intel.com> Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com> Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com> Gerrit-Reviewer: Wonkyu Kim <wonkyu.kim(a)intel.com> Gerrit-Comment-Date: Thu, 20 Feb 2020 04:32:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

coreboot-gerrit February 2020