Change in coreboot[master]: soc/intel/common: Add Kconfig for CSE RW firmware version

[V Sowmya (Code Review)]

V Sowmya has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

soc/intel/common: Add Kconfig for CSE RW firmware version

This patch adds a kconfig SOC_INTEL_CSE_RW_VERSION to pass the
CSE RW firmware version from the maiboard. This will be extracted
by makefile to update the cse_rw_metadata structure.

BUG=b:169077783

Change-Id: I62691ee3ede7d4cd21f821381f5d1519f9061fd9
Signed-off-by: V Sowmya 
---
M src/soc/intel/common/block/cse/Kconfig
1 file changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47430/1

diff --git a/src/soc/intel/common/block/cse/Kconfig b/src/soc/intel/common/block/cse/Kconfig
index a651198..9d781f9 100644
--- a/src/soc/intel/common/block/cse/Kconfig
+++ b/src/soc/intel/common/block/cse/Kconfig
@@ -40,3 +40,10 @@
    default ""
    help
     Intel CSE CBFS RW blob path and file name
+
+config SOC_INTEL_CSE_RW_VERSION
+	string "Intel CSE RW firmware version"
+	depends on SOC_INTEL_CSE_LITE_SKU
+	default ""
+	help
+	 Intel CSE RW firmware version

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/Kconfig:

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
PS4, Line 22: 
What do you think about adding a Kconfig `SOC_INTEL_CSE_RW_UPDATE` which enables support for RW update. I know previously we decided not to add a separate Kconfig and only use the presence of SOC_INTEL_CSE_RW_FILE as an indication. But, now that we have multiple Kconfigs for update, maybe it makes sense to have that switch?

It will also allow us to add compile-time checks in Makefile to ensure all required configs are provided by mainboard.

i.e. if SOC_INTEL_CSE_RW_UPDATE is selected, but SOC_INTEL_CSE_RW_FILE or SOC_INTEL_CSE_RW_VERSION is not provided, then throw an error asking the user to set both configs.

Also, you can use the switch to decide if the configs below need to be made user-visible.

i.e.
if SOC_INTEL_CSE_RW_UPDATE

config SOC_INTEL_CSE_FMAP_NAME
...
...
...

endif

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
PS4, Line 49: firmware
nit: Rest of the Kconfigs refer to this as blob. Maybe use the same here?

Also, it would be good to add text indicating that this is really the version for the file provided by SOC_INTEL_CSE_RW_FILE.

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/Kconfig:

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
PS4, Line 49: firmware
> nit: Rest of the Kconfigs refer to this as blob. Maybe use the same here? […]

I see that CSE RW version is populated in the first few bytes of CSE_RW_FILE. Can the version be extracted from there?

I am concerned that we are manually inputting CSE RW version in multiple places. Once while generating the CSE RW File and then later we have to update this kconfig.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/Kconfig:

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
PS4, Line 49: firmware
> I see that CSE RW version is populated in the first few bytes of CSE_RW_FILE. […]

Actually, with this change, CSE RW version will not be prepended to the CSE_RW_FILE anymore. It will only be added to Kconfig. This will remove the immediate overhead on partners when they are re-generating FIT images.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/Kconfig:

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
PS4, Line 49: firmware
> Also, it would be good to add text indicating that this is really the version for the file provided by SOC_INTEL_CSE_RW_FILE.

In addition to that, can you please document the format of the expected string as well i.e. major.minor.hotfix.build

[V Sowmya (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Tim Wawrzynczak, Rizwan Qureshi, Sridhar Siricilla, Balaji Manigandan, Raj Astekar, Patrick Rudolph, Karthik Ramasubramanian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47430

to look at the new patch set (#5).

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

soc/intel/common: Add Kconfig for CSE RW firmware version

This patch adds a kconfig SOC_INTEL_CSE_RW_VERSION to pass the
CSE RW firmware version from the maiboard. This will be extracted
by makefile to update the cse_rw_metadata structure.

BUG=b:169077783

Change-Id: I62691ee3ede7d4cd21f821381f5d1519f9061fd9
Signed-off-by: V Sowmya 
---
M src/soc/intel/common/block/cse/Kconfig
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47430/5

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/Kconfig:

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
PS4, Line 22:
> What do you think about adding a Kconfig `SOC_INTEL_CSE_RW_UPDATE` which enables support for RW upda […]

Ack

https://review.coreboot.org/c/coreboot/+/47430/4/src/soc/intel/common/block/... 
PS4, Line 49: firmware
> > Also, it would be good to add text indicating that this is really the version for the file provide […]

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 7: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47430/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47430/7//COMMIT_MSG@10 
PS7, Line 10: maiboard
mainboard

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47430/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47430/7//COMMIT_MSG@12 
PS7, Line 12: 
Can you document, that the firmware version is not stored in the blob itself? (Maybe it should be in the future, so it can be extracted.)

https://review.coreboot.org/c/coreboot/+/47430/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/Kconfig:

https://review.coreboot.org/c/coreboot/+/47430/7/src/soc/intel/common/block/... 
PS7, Line 51: 	 major.minor.hotfix.build.
Can you give one example?

[V Sowmya (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Tim Wawrzynczak, Rizwan Qureshi, Sridhar Siricilla, Balaji Manigandan, Raj Astekar, Patrick Rudolph, Karthik Ramasubramanian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47430

to look at the new patch set (#8).

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

soc/intel/common: Add Kconfig for CSE RW firmware version

This patch adds a kconfig SOC_INTEL_CSE_RW_VERSION to pass the
CSE RW firmware version from the mainboard. This will be extracted
by makefile to update the cse_rw_metadata structure.
Right now the required tool to extract the CSE RW version from
the blob is still under development and after the official version
of the tool is released, version will be extracted by parsing the
CSE RW blob.

BUG=b:169077783

Change-Id: I62691ee3ede7d4cd21f821381f5d1519f9061fd9
Signed-off-by: V Sowmya 
---
M src/soc/intel/common/block/cse/Kconfig
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47430/8

[V Sowmya (Code Review)]

V Sowmya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 8:

(3 comments)

https://review.coreboot.org/c/coreboot/+/47430/7//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47430/7//COMMIT_MSG@10 
PS7, Line 10: maiboard
> mainboard

Done

https://review.coreboot.org/c/coreboot/+/47430/7//COMMIT_MSG@12 
PS7, Line 12:
> Can you document, that the firmware version is not stored in the blob itself? (Maybe it should be in […]

Done

https://review.coreboot.org/c/coreboot/+/47430/7/src/soc/intel/common/block/... 
File src/soc/intel/common/block/cse/Kconfig:

https://review.coreboot.org/c/coreboot/+/47430/7/src/soc/intel/common/block/... 
PS7, Line 51: 	 major.minor.hotfix.build.
> Can you give one example?

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 10: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 10: Code-Review+1

Changing to +1 so that we can land all the coreboot and chromium tree changes together.

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 10: Code-Review+1

I will stop with +1 so that all the CLs can be landed together.

[V Sowmya (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Jamie Ryu, Tim Wawrzynczak, Rizwan Qureshi, Sridhar Siricilla, Balaji Manigandan, Raj Astekar, Patrick Rudolph, Karthik Ramasubramanian,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47430

to look at the new patch set (#11).

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

soc/intel/common: Add Kconfig for CSE RW firmware version

This patch adds a kconfig SOC_INTEL_CSE_RW_VERSION to pass the
CSE RW firmware version from the mainboard. This will be extracted
by makefile to update the cse_rw_metadata structure.
Right now the required tool to extract the CSE RW version from
the blob is still under development and after the official version
of the tool is released, version will be extracted by parsing the
CSE RW blob.

BUG=b:169077783

Cq-Depend: chrome-internal:3402224, chrome-internal:3397863,
chromium:2473603, chromium:2473603, chromium:2535950
Change-Id: I62691ee3ede7d4cd21f821381f5d1519f9061fd9
Signed-off-by: V Sowmya 
---
M src/soc/intel/common/block/cse/Kconfig
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/47430/11

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 11:

All required changes except this one are +2ed. I will wait until end of day to see if there are any major concerns from anyone. If not, I will go ahead and merge these changes and cherry-pick to chromium tree with appropriate Cq-Depends.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

Patch Set 11: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47430 )

Change subject: soc/intel/common: Add Kconfig for CSE RW firmware version
......................................................................

soc/intel/common: Add Kconfig for CSE RW firmware version

This patch adds a kconfig SOC_INTEL_CSE_RW_VERSION to pass the
CSE RW firmware version from the mainboard. This will be extracted
by makefile to update the cse_rw_metadata structure.
Right now the required tool to extract the CSE RW version from
the blob is still under development and after the official version
of the tool is released, version will be extracted by parsing the
CSE RW blob.

BUG=b:169077783

Cq-Depend: chrome-internal:3402224, chrome-internal:3397863,
chromium:2473603, chromium:2473603, chromium:2535950
Change-Id: I62691ee3ede7d4cd21f821381f5d1519f9061fd9
Signed-off-by: V Sowmya 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47430
Reviewed-by: Furquan Shaikh 
Reviewed-by: Karthik Ramasubramanian 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/common/block/cse/Kconfig
1 file changed, 9 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Karthik Ramasubramanian: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/common/block/cse/Kconfig b/src/soc/intel/common/block/cse/Kconfig
index a651198..5b52bfd 100644
--- a/src/soc/intel/common/block/cse/Kconfig
+++ b/src/soc/intel/common/block/cse/Kconfig
@@ -40,3 +40,12 @@
    default ""
    help
     Intel CSE CBFS RW blob path and file name
+
+config SOC_INTEL_CSE_RW_VERSION
+	string "Intel CSE RW firmware version"
+	depends on SOC_INTEL_CSE_LITE_SKU
+	default ""
+	help
+	 This config contains the Intel CSE RW version of the blob that is provided by
+	 SOC_INTEL_CSE_RW_FILE config and the version must be set in the format
+	 major.minor.hotfix.build (ex: 14.0.40.1209).