Change in ...coreboot[master]: Fix fsp post-init validation

[Matthew Garrett (Code Review)]

Matthew Garrett has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32530

Change subject: Fix fsp post-init validation
......................................................................

Fix fsp post-init validation

Part of this checks whether tolum_base and cbmem_top are the same - however,
cbmem_top hasn't been initialised at the point where this call occurs.
Change the ordering to fix that.

Signed-off-by: Matthew Garrett 
Change-Id: Ib89e0513bdc35c3751a9d4c2a2789a2836046789
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 2 insertions(+), 2 deletions(-)

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

diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 985ee3a..e021f52 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -310,8 +310,6 @@
    post_code(POST_FSP_MEMORY_EXIT);
    timestamp_add_now(TS_FSP_MEMORY_INIT_END);

-	fsp_debug_after_memory_init(status);
-
    /* Handle any errors returned by FspMemoryInit */
    fsp_handle_reset(status);
    if (status != FSP_SUCCESS) {
@@ -320,6 +318,8 @@
    }

do_fsp_post_memory_init(s3wake, fsp_version);
+
+	fsp_debug_after_memory_init(status);
 }

/* Load the binary into the memory specified by the info header. */

[Patrick Rudolph (Code Review)]

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

Change subject: Fix fsp post-init validation
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/32530/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32530/4//COMMIT_MSG@7 
PS4, Line 7: Fix fsp post-init validation
please add a prefix

https://review.coreboot.org/#/c/32530/4//COMMIT_MSG@9 
PS4, Line 9: Part of this checks whether tolum_base and cbmem_top are the same - however,
true, cbmem is initilaized in do_fsp_post_memory_init()

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Matthew Garrett (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

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

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

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

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

drivers/intel/fsp2_0: Fix fsp post-init validation

Part of this checks whether tolum_base and cbmem_top are the same - however,
cbmem_top hasn't been initialised at the point where this call occurs.
Change the ordering to fix that.

Signed-off-by: Matthew Garrett 
Change-Id: Ib89e0513bdc35c3751a9d4c2a2789a2836046789
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/32530/7

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Matthew Garrett (Code Review)]

Matthew Garrett has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32530 )

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/#/c/32530/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/32530/4//COMMIT_MSG@7 
PS4, Line 7: Fix fsp post-init validation
> please add a prefix

Done

https://review.coreboot.org/#/c/32530/4//COMMIT_MSG@9 
PS4, Line 9: Part of this checks whether tolum_base and cbmem_top are the same - however,
> true, cbmem is initilaized in do_fsp_post_memory_init()

Ack

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Patrick Rudolph (Code Review)]

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

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 7: Code-Review+1

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32530 )

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32530 )

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 7:

i don't understand what is broken ?

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Furquan Shaikh (Code Review)]

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

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 7:
> Patch Set 7:
>
> i don't understand what is broken ?

fsp_debug_after_memory_init() calls into fsp_verify_memory_init_hobs() which as its last step checks tolum end is equal to cbmem top. cbmem_top() relies on cbmem_top_init() to fill_ebda_area() which is done as part of do_fsp_post_memory_init(). This change seems to be fixing that order to ensure do_fsp_post_memory_init() gets called before fsp_debug_after_memory_init()

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Furquan Shaikh (Code Review)]

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

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/32530/7/src/drivers/intel/fsp2_0/memory_init... 
File src/drivers/intel/fsp2_0/memory_init.c:

https://review.coreboot.org/#/c/32530/7/src/drivers/intel/fsp2_0/memory_init... 
PS7, Line 322: fsp_debug_after_memory_init
Maybe add a comment here indicating why this is required after do_fsp_post_memory_init() so future change does not end up moving it again?

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Matthew Garrett (Code Review)]

Matthew Garrett has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32530 )

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/32530/7/src/drivers/intel/fsp2_0/memory_init... 
File src/drivers/intel/fsp2_0/memory_init.c:

https://review.coreboot.org/#/c/32530/7/src/drivers/intel/fsp2_0/memory_init... 
PS7, Line 322: fsp_debug_after_memory_init
> Maybe add a comment here indicating why this is required after do_fsp_post_memory_init() so future c […]

Done

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Matthew Garrett (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Rizwan Qureshi, build bot (Jenkins),

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

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

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

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

drivers/intel/fsp2_0: Fix fsp post-init validation

Part of this checks whether tolum_base and cbmem_top are the same - however,
cbmem_top hasn't been initialised at the point where this call occurs.
Change the ordering to fix that.

Signed-off-by: Matthew Garrett 
Change-Id: Ib89e0513bdc35c3751a9d4c2a2789a2836046789
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 7 insertions(+), 2 deletions(-)

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

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Furquan Shaikh (Code Review)]

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

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 8: Code-Review+2

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32530 )

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

Patch Set 8: Code-Review+2

Change in ...coreboot[master]: drivers/intel/fsp2_0: Fix fsp post-init validation

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32530 )

Change subject: drivers/intel/fsp2_0: Fix fsp post-init validation
......................................................................

drivers/intel/fsp2_0: Fix fsp post-init validation

Part of this checks whether tolum_base and cbmem_top are the same - however,
cbmem_top hasn't been initialised at the point where this call occurs.
Change the ordering to fix that.

Signed-off-by: Matthew Garrett 
Change-Id: Ib89e0513bdc35c3751a9d4c2a2789a2836046789
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32530
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Subrata Banik 
---
M src/drivers/intel/fsp2_0/memory_init.c
1 file changed, 7 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Subrata Banik: Looks good to me, approved

diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c
index 985ee3a..b3afb98 100644
--- a/src/drivers/intel/fsp2_0/memory_init.c
+++ b/src/drivers/intel/fsp2_0/memory_init.c
@@ -310,8 +310,6 @@
    post_code(POST_FSP_MEMORY_EXIT);
    timestamp_add_now(TS_FSP_MEMORY_INIT_END);

-	fsp_debug_after_memory_init(status);
-
    /* Handle any errors returned by FspMemoryInit */
    fsp_handle_reset(status);
    if (status != FSP_SUCCESS) {
@@ -320,6 +318,13 @@
    }

do_fsp_post_memory_init(s3wake, fsp_version);
+
+	/*
+	 * fsp_debug_after_memory_init() checks whether the end of the tolum
+	 * region is the same as the top of cbmem, so must be called here
+	 * after cbmem has been initialised in do_fsp_post_memory_init().
+	 */
+	fsp_debug_after_memory_init(status);
 }

/* Load the binary into the memory specified by the info header. */