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 10:
(11 comments)
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
File src/soc/intel/common/block/cse/cse.c:
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 89: MKHI_BUP_COMMON_GROUP_ID
> Can you please ensure that the naming is consistent. […]
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 92: #define BUP_COMMON_GET_BOOT_PARTITION_INFO_CMD_REQ 0x1C
: #define BUP_COMMON_SET_BOOT_PARTITION_CMD_REQ 0x1D
> can we move these commands IDs and ones above into a separate header file and refer it from there? N […]
I think moving command ids to header file is not required!
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 731: int
> unsigned int?
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 760: set_cse_next_boot_partition
> What exactly is this function doing? Under what conditions can this function be called? What are the […]
It sends HECI command to notify CSE about it's next boot partition to boot from on next reset.
Whenever coreboot wants CSE to boot from certain partition(CSE valid boot partition ids are RO_BP1 (0), RW_BP2(1)), then this function be used. Global Reset( Global Reset or CSE Reset) should be issued after marking next boot partition of ME.
This function must be used before EOP.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 760: int
> same.
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 774: .group_id = MKHI_BUP_COMMON_GROUP_ID,
> Move this to next line.
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 779: +1
> Why add 1?
Boot Partition indexing starts from 0.
We are referring partition names as BP1,BP2 & BP3, so as to align with the naming, I'm logging partition names accordingly.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 143: #define RO_BP1 0
: #define RW_BP2 1
: #define RW_BP3 2
> can we have it as enum as well would help it bundle under Boot partition context or add comment here […]
Done
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 175: mkhi_hdr
> This should be added as a separate CL. […]
Agreed.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 183: /* Boot Partition Info Flag Redundancy Enabled */
: #define BP_INFO_REDUNDANCY_EN (1 << 0)
:
: /* Boot Partition Info Flag Min. Recovery Mode Enabled */
: #define BP_INFO_MIN_RECO_MODE_EN (1 << 1)
:
: /* Boot Partition Info Flag Read only Config Enabled */
: #define BP_INFO_READ_ONLY_CFG (1 << 2)
> It is not clear to me from the comments what the flags really mean.
In order to support CSE FW update functionality with resilience, the CSE FW logically divided into RO, and RW partitions. The RO will contain minimum firmware to boot the platform.
RW will contain boot capability and other functionality.The minimal boot functionality is redundant in RO and RW. This is indicated by BP_INFO_REDUNDANCY_EN, and BP_INFO_MIN_RECO_MODE_EN.
BP_INFO_READ_ONLY_CFG indicates HW protection to RO region.
https://review.coreboot.org/c/coreboot/+/35402/6/src/soc/intel/common/block…
PS6, Line 212: /* Retrieves boot partitions Info CSE */
> Can you please add more details as to what this function really does. […]
The get_boot_partition_info_resp is utilized in multiple places at different points in the update flow, and having the response info available saves sending same command multiple times.
--
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: 10
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: 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: 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: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Mon, 23 Sep 2019 16:12:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Aamir Bohra <aamir.bohra(a)intel.com>
Comment-In-Reply-To: Furquan Shaikh <furquan(a)google.com>
Gerrit-MessageType: comment
Hello Patrick Rudolph, Subrata Banik, Balaji Manigandan, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35402
to look at the new patch set (#10).
Change subject: soc/intel/common/block/cse: Add boot partition related APIs
......................................................................
soc/intel/common/block/cse: Add boot partition related APIs
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 the next boot partition to boot for CSE.
With the HECI API, firmware can notify CSE to boot from BP1 or BP2 on next boot.
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/Kconfig
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
3 files changed, 197 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35402/10
--
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: 10
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: 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: 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: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
Hello Patrick Rudolph, Subrata Banik, Balaji Manigandan, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35402
to look at the new patch set (#9).
Change subject: soc/intel/common/block/cse: Add boot partition related APIs
......................................................................
soc/intel/common/block/cse: Add boot partition related APIs
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 the next boot partition to boot for CSE.
With the HECI API, firmware can notify CSE to boot from BP1 or BP2 on next boot.
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/Kconfig
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
3 files changed, 196 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35402/9
--
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: 9
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: 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: 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: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset
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 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35402/8/src/soc/intel/common/block…
File src/soc/intel/common/block/include/intelblocks/cse.h:
https://review.coreboot.org/c/coreboot/+/35402/8/src/soc/intel/common/block…
PS8, Line 242: * It sends HECI command to notify CSE about it's next boot partition to boot from on next reset.
line over 96 characters
https://review.coreboot.org/c/coreboot/+/35402/8/src/soc/intel/common/block…
PS8, Line 244: * are BP1 (0), BP2(1)), then this function be used. Global Reset( Global Reset or CSE Reset) should
line over 96 characters
--
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: 8
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: 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: 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: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Comment-Date: Mon, 23 Sep 2019 15:51:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Hello Patrick Rudolph, Subrata Banik, Balaji Manigandan, Aamir Bohra, Rizwan Qureshi, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35402
to look at the new patch set (#8).
Change subject: soc/intel/common/block/cse: Add boot partition related APIs
......................................................................
soc/intel/common/block/cse: Add boot partition related APIs
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 the next boot partition to boot for CSE.
With the HECI API, firmware can notify CSE to boot from BP1 or BP2 on next boot.
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/Kconfig
M src/soc/intel/common/block/cse/cse.c
M src/soc/intel/common/block/include/intelblocks/cse.h
3 files changed, 202 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/02/35402/8
--
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: 8
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: 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: 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: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-MessageType: newpatchset