Change in coreboot[master]: mb/google/volteer: fmd update for CSE Lite Firmware Update

[Jamie Ryu (Code Review)]

Jamie Ryu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: fmd update for CSE Lite Firmware Update
......................................................................

mb/google/volteer: fmd update for CSE Lite Firmware Update

Updated fmd to include CSE Lite RW partition binary to FW_MAIN_A
and FW_MAIN_B for CSE Lite firmware update.

BUG=b:140448618
TEST=build with me_rw binary blob for volteer and boot to kernel.

Signed-off-by: Jamie Ryu 
Change-Id: Ie3c2b657f0426d206dfe3729829ec34ff57812c7
---
M src/mainboard/google/volteer/chromeos.fmd
1 file changed, 7 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/43790/1

diff --git a/src/mainboard/google/volteer/chromeos.fmd b/src/mainboard/google/volteer/chromeos.fmd
index 60ea3de..07a5464 100644
--- a/src/mainboard/google/volteer/chromeos.fmd
+++ b/src/mainboard/google/volteer/chromeos.fmd
@@ -7,16 +7,16 @@
    	# Place RW_LEGACY at the start of BIOS region such that the rest
    	# of BIOS regions start at 16MiB boundary. Since this is a 32MiB
    	# SPI flash only the top 16MiB actually gets memory mapped.
-		RW_LEGACY(CBFS)@0x0 0xf00000
-		RW_SECTION_A@0xf00000 0x3e0000 {
+		RW_LEGACY(CBFS)@0x0 0xb00000
+		RW_SECTION_A@0xb00000 0x5e0000 {
    		VBLOCK_A@0x0 0x10000
-			FW_MAIN_A(CBFS)@0x10000 0x3cffc0
-			RW_FWID_A@0x3dffc0 0x40
+			FW_MAIN_A(CBFS)@0x10000 0x5cffc0
+			RW_FWID_A@0x5dffc0 0x40
    	}
-		RW_SECTION_B@0x12e0000 0x3e0000 {
+		RW_SECTION_B@0x10e0000 0x5e0000 {
    		VBLOCK_B@0x0 0x10000
-			FW_MAIN_B(CBFS)@0x10000 0x3cffc0
-			RW_FWID_B@0x3dffc0 0x40
+			FW_MAIN_B(CBFS)@0x10000 0x5cffc0
+			RW_FWID_B@0x5dffc0 0x40
    	}
    	RW_MISC@0x16c0000 0x40000 {
    		UNIFIED_MRC_CACHE(PRESERVE)@0x0 0x30000 {

[Paul Menzel (Code Review)]

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

Change subject: mb/google/volteer: fmd update for CSE Lite Firmware Update
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@7 
PS1, Line 7: mb/google/volteer: fmd update for CSE Lite Firmware Update
Please make it a statement by using a verb (in imperative mood):
> Update fmd for …

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@8 
PS1, Line 8: 
Please add a problem description in the beginning.

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@9 
PS1, Line 9: Updated
Present tense: Update

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@9 
PS1, Line 9: fmd
flash map descriptor?

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Jamie Ryu (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

To support CSE Lite firmware update, CSE RW partition is extracted from
CSE blob binary and added to FW_MAIN_A and FW_MAIN_B.

CSE RW size for TGL is close to 2.3MB; hence, the size of FW_MAIN_A and
FW_MAIN_B is increased to avoid an overflow.

BUG=b:140448618
TEST=build with me_rw binary blob for volteer and boot to kernel.

Signed-off-by: Jamie Ryu 
Change-Id: Ie3c2b657f0426d206dfe3729829ec34ff57812c7
---
M src/mainboard/google/volteer/chromeos.fmd
1 file changed, 7 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/43790/2

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Jamie Ryu (Code Review)]

Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

Patch Set 2:

(4 comments)

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@7 
PS1, Line 7: mb/google/volteer: fmd update for CSE Lite Firmware Update
> Please make it a statement by using a verb (in imperative mood): […]

Done

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@8 
PS1, Line 8:
> Please add a problem description in the beginning.

Done

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@9 
PS1, Line 9: Updated
> Present tense: Update

Done

https://review.coreboot.org/c/coreboot/+/43790/1//COMMIT_MSG@9 
PS1, Line 9: fmd
> flash map descriptor?

Thanks! I updated the commit message.

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Nick Vaccaro (Code Review)]

Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

Patch Set 2: Code-Review+2

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

Patch Set 2:

This breaks the update flow, right? Any coordination needed here?

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Jamie Ryu (Code Review)]

Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

Patch Set 2:
> Patch Set 2:
>
> This breaks the update flow, right? Any coordination needed here?

Can you share more information why this breaks the update flow? Thanks.

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> >
> > This breaks the update flow, right? Any coordination needed here?
>
> Can you share more information why this breaks the update flow? Thanks.

firmware updates overwrite one of the RW_SECTION_[AB] regions, keeping everything else intact but that requires that they keep their location and size.

I'm not sure at which state this device is, and if there are any such incremential updates already. If not (and all updates are always full rewrites) there's no issue, otherwise we might have to look into it.

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Jamie Ryu (Code Review)]

Jamie Ryu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > >
> > > This breaks the update flow, right? Any coordination needed here?
> >
> > Can you share more information why this breaks the update flow? Thanks.
>
> firmware updates overwrite one of the RW_SECTION_[AB] regions, keeping everything else intact but that requires that they keep their location and size.
>
> I'm not sure at which state this device is, and if there are any such incremential updates already. If not (and all updates are always full rewrites) there's no issue, otherwise we might have to look into it.

Thanks for comments. Can I have comments from others reviewers regarding this?
To enable CSE FW update, I think FMD update is required anyway to reserve enough space for RW_SECTION_A/B, and it is better FMD change is done sooner. Any comments would be appreciated.

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> > > Patch Set 2:
> > > > Patch Set 2:
> > > >
> > > > This breaks the update flow, right? Any coordination needed here?
> > >
> > > Can you share more information why this breaks the update flow? Thanks.
> >
> > firmware updates overwrite one of the RW_SECTION_[AB] regions, keeping everything else intact but that requires that they keep their location and size.
> >
> > I'm not sure at which state this device is, and if there are any such incremential updates already. If not (and all updates are always full rewrites) there's no issue, otherwise we might have to look into it.
>
> Thanks for comments. Can I have comments from others reviewers regarding this?
> To enable CSE FW update, I think FMD update is required anyway to reserve enough space for RW_SECTION_A/B, and it is better FMD change is done sooner. Any comments would be appreciated.

It should be ok right now since we're still unlocked futility will do an RO update of the full image.  Agreed that we should do this before it gets too late.

Change in coreboot[master]: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

[Duncan Laurie (Code Review)]

Duncan Laurie has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43790 )

Change subject: mb/google/volteer: Update flashmap descriptor for CSE Lite FW update
......................................................................

mb/google/volteer: Update flashmap descriptor for CSE Lite FW update

To support CSE Lite firmware update, CSE RW partition is extracted from
CSE blob binary and added to FW_MAIN_A and FW_MAIN_B.

CSE RW size for TGL is close to 2.3MB; hence, the size of FW_MAIN_A and
FW_MAIN_B is increased to avoid an overflow.

BUG=b:140448618
TEST=build with me_rw binary blob for volteer and boot to kernel.

Signed-off-by: Jamie Ryu 
Change-Id: Ie3c2b657f0426d206dfe3729829ec34ff57812c7
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43790
Reviewed-by: Nick Vaccaro 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/volteer/chromeos.fmd
1 file changed, 7 insertions(+), 7 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nick Vaccaro: Looks good to me, approved

diff --git a/src/mainboard/google/volteer/chromeos.fmd b/src/mainboard/google/volteer/chromeos.fmd
index 60ea3de..07a5464 100644
--- a/src/mainboard/google/volteer/chromeos.fmd
+++ b/src/mainboard/google/volteer/chromeos.fmd
@@ -7,16 +7,16 @@
    	# Place RW_LEGACY at the start of BIOS region such that the rest
    	# of BIOS regions start at 16MiB boundary. Since this is a 32MiB
    	# SPI flash only the top 16MiB actually gets memory mapped.
-		RW_LEGACY(CBFS)@0x0 0xf00000
-		RW_SECTION_A@0xf00000 0x3e0000 {
+		RW_LEGACY(CBFS)@0x0 0xb00000
+		RW_SECTION_A@0xb00000 0x5e0000 {
    		VBLOCK_A@0x0 0x10000
-			FW_MAIN_A(CBFS)@0x10000 0x3cffc0
-			RW_FWID_A@0x3dffc0 0x40
+			FW_MAIN_A(CBFS)@0x10000 0x5cffc0
+			RW_FWID_A@0x5dffc0 0x40
    	}
-		RW_SECTION_B@0x12e0000 0x3e0000 {
+		RW_SECTION_B@0x10e0000 0x5e0000 {
    		VBLOCK_B@0x0 0x10000
-			FW_MAIN_B(CBFS)@0x10000 0x3cffc0
-			RW_FWID_B@0x3dffc0 0x40
+			FW_MAIN_B(CBFS)@0x10000 0x5cffc0
+			RW_FWID_B@0x5dffc0 0x40
    	}
    	RW_MISC@0x16c0000 0x40000 {
    		UNIFIED_MRC_CACHE(PRESERVE)@0x0 0x30000 {