Change in ...coreboot[master]: stage_cache: Add into stage_cache if !CONFIG_NO_STAGE_CACHE

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33394

Change subject: stage_cache: Add into stage_cache if !CONFIG_NO_STAGE_CACHE
......................................................................

stage_cache: Add into stage_cache if !CONFIG_NO_STAGE_CACHE

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
---
M src/drivers/intel/fsp1_1/ramstage.c
M src/drivers/intel/fsp2_0/silicon_init.c
M src/soc/intel/baytrail/refcode.c
M src/soc/intel/broadwell/refcode.c
4 files changed, 8 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33394/1

diff --git a/src/drivers/intel/fsp1_1/ramstage.c b/src/drivers/intel/fsp1_1/ramstage.c
index 814bddf..a55df95 100644
--- a/src/drivers/intel/fsp1_1/ramstage.c
+++ b/src/drivers/intel/fsp1_1/ramstage.c
@@ -165,7 +165,8 @@
    	return;
    }

-	stage_cache_add(STAGE_REFCODE, fsp);
+	if (!CONFIG(NO_STAGE_CACHE))
+		stage_cache_add(STAGE_REFCODE, fsp);
 }

static int fsp_find_and_relocate(struct prog *fsp)
diff --git a/src/drivers/intel/fsp2_0/silicon_init.c b/src/drivers/intel/fsp2_0/silicon_init.c
index e72e4ac..ab9951d 100644
--- a/src/drivers/intel/fsp2_0/silicon_init.c
+++ b/src/drivers/intel/fsp2_0/silicon_init.c
@@ -128,7 +128,8 @@

prog_set_area(&fsps, dest, size);

-	stage_cache_add(STAGE_REFCODE, &fsps);
+	if (!CONFIG(NO_STAGE_CACHE))
+		stage_cache_add(STAGE_REFCODE, &fsps);

/* Signal that FSP component has been loaded. */
    prog_segment_loaded(hdr->image_base, hdr->image_size, SEG_FINAL);
diff --git a/src/soc/intel/baytrail/refcode.c b/src/soc/intel/baytrail/refcode.c
index 24469ea..daf3d1a 100644
--- a/src/soc/intel/baytrail/refcode.c
+++ b/src/soc/intel/baytrail/refcode.c
@@ -65,7 +65,8 @@
    }

/* Cache loaded reference code. */
-	stage_cache_add(STAGE_REFCODE, &prog);
+	if (!CONFIG(NO_STAGE_CACHE))
+		stage_cache_add(STAGE_REFCODE, &prog);

return prog_entry(&prog);
 }
diff --git a/src/soc/intel/broadwell/refcode.c b/src/soc/intel/broadwell/refcode.c
index 6d192cc..63decaf 100644
--- a/src/soc/intel/broadwell/refcode.c
+++ b/src/soc/intel/broadwell/refcode.c
@@ -61,7 +61,8 @@
    }

/* Cache loaded reference code. */
-	stage_cache_add(STAGE_REFCODE, &prog);
+	if (!CONFIG(NO_STAGE_CACHE))
+		stage_cache_add(STAGE_REFCODE, &prog);

return (pei_wrapper_entry_t)prog_entry(&prog);
 }

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33394 )

Change subject: stage_cache: Add into stage_cache if !CONFIG_NO_STAGE_CACHE
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33394/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33394/1//COMMIT_MSG@7 
PS1, Line 7: Add into stage_cache if !CONFIG_NO_STAGE_CACHE
Why? This never the case on those platforms afaict.

[Subrata Banik (Code Review)]

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

Change subject: stage_cache: Add into stage_cache if !CONFIG_NO_STAGE_CACHE
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33394/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33394/1//COMMIT_MSG@7 
PS1, Line 7: Add into stage_cache if !CONFIG_NO_STAGE_CACHE
> Why? This never the case on those platforms afaict.

stage_cache_add() exist inside ext_stage_cache.c or cbmem_stage_cache.c, do you think it make sense to call stage_cache_add() function in case of !CONFIG_NO_STAGE_CACHE ?
it will eventually call __weak implementation ?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33394 )

Change subject: stage_cache: Add into stage_cache if !CONFIG_NO_STAGE_CACHE
......................................................................

Patch Set 1:

This is just making the call sites noisy. If you want to tease all that out put empty inline functions in the header file based on the various Kconfigs. The way things were constructed before is to not make the call sites noisy.

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

This patch removes CONFIG_NO_STAGE_CACHE check from caller function
and add empty inline function incase CONFIG_NO_STAGE_CACHE is enable.

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
---
M src/arch/x86/postcar_loader.c
M src/include/stage_cache.h
M src/lib/prog_loaders.c
3 files changed, 12 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33394/2

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

Patch Set 2:
> Patch Set 1:
>
> This is just making the call sites noisy. If you want to tease all that out put empty inline functions in the header file based on the various Kconfigs. The way things were constructed before is to not make the call sites noisy.

Done

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Furquan Shaikh (Code Review)]

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/33394/2/src/include/stage_cache.h 
File src/include/stage_cache.h:

https://review.coreboot.org/#/c/33394/2/src/include/stage_cache.h@36 
PS2, Line 36: __weak
Why is weak required now?

https://review.coreboot.org/#/c/33394/2/src/include/stage_cache.h@38 
PS2, Line 38: __weak
same here.

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

This patch removes CONFIG_NO_STAGE_CACHE check from caller function
and add empty inline function incase CONFIG_NO_STAGE_CACHE is enable.

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
---
M src/arch/x86/postcar_loader.c
M src/include/stage_cache.h
M src/lib/prog_loaders.c
3 files changed, 10 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33394/3

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

This patch removes CONFIG_NO_STAGE_CACHE check from caller function
and add empty inline function incase CONFIG_NO_STAGE_CACHE is enable.

Also make NO_STAGE_CACHE depends on !CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
to avoid multiple select of stage_cache_<> functions.

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
---
M src/Kconfig
M src/arch/x86/postcar_loader.c
M src/include/stage_cache.h
M src/lib/prog_loaders.c
4 files changed, 11 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33394/4

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

This patch removes CONFIG_NO_STAGE_CACHE check from caller function
and add empty inline function incase CONFIG_NO_STAGE_CACHE is enable.

Also make NO_STAGE_CACHE depends on !CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
to avoid multiple select of stage_cache_<> functions.

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
---
M src/Kconfig
M src/arch/x86/postcar_loader.c
M src/include/stage_cache.h
M src/lib/prog_loaders.c
4 files changed, 11 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33394/5

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

This patch removes CONFIG_NO_STAGE_CACHE check from caller function
and add empty inline function incase CONFIG_NO_STAGE_CACHE is enable.

Also make NO_STAGE_CACHE enable if !CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
and !RELOCATABLE_RAMSTAGE to avoid multiple select of stage_cache_<> functions.

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
---
M src/Kconfig
M src/arch/x86/postcar_loader.c
M src/include/stage_cache.h
M src/lib/prog_loaders.c
4 files changed, 11 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33394/6

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

Hello Kyösti Mälkki, Aaron Durbin, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

This patch removes CONFIG_NO_STAGE_CACHE check from caller function
and add empty inline function incase CONFIG_NO_STAGE_CACHE is enable.

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
---
M src/arch/x86/postcar_loader.c
M src/include/stage_cache.h
M src/lib/prog_loaders.c
3 files changed, 11 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/33394/7

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Furquan Shaikh (Code Review)]

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h 
File src/include/stage_cache.h:

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h@35 
PS7, Line 35: CONFIG(CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) \
            : 		|| CONFIG(RELOCATABLE_RAMSTAGE
Why are these configs used here? Shouldn't it be !CONFIG(NO_STAGE_CACHE)? Or just flip it around and:

#if CONFIG(NO_STAGE_CACHE)
static inline ...

#else

#endif

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33394 )

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

Patch Set 7: Code-Review+2

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33394 )

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h 
File src/include/stage_cache.h:

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h@35 
PS7, Line 35: CONFIG(CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) \
            : 		|| CONFIG(RELOCATABLE_RAMSTAGE
> Why are these configs used here? Shouldn't it be !CONFIG(NO_STAGE_CACHE)? Or just flip it around and […]

I just went through the same thought process. This logic is mimicking the current set up. There currently isn't any dependencies tying these Kconfigs together. I think the logic is just as good (or bad) as prior to this change. It could use a clean up.

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Furquan Shaikh (Code Review)]

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

Patch Set 7: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h 
File src/include/stage_cache.h:

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h@35 
PS7, Line 35: CONFIG(CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) \
            : 		|| CONFIG(RELOCATABLE_RAMSTAGE
> I just went through the same thought process. This logic is mimicking the current set up. […]

Humm okay. Makes sense.

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

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

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h 
File src/include/stage_cache.h:

https://review.coreboot.org/#/c/33394/7/src/include/stage_cache.h@35 
PS7, Line 35: CONFIG(CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) \
            : 		|| CONFIG(RELOCATABLE_RAMSTAGE
> Humm okay. Makes sense.

yes furquan, i wish to do the same but you must watch those build failure. Looks like stage cache kconfig is in deep mess :(

Logically NO_STAGE_CACHE and other internal/external stage cache should be mutually exclusive but its not the case for many platform hence adding check based on NO_STAGE_CACHE cause redefinition of function.

I will clean stage_cache issue and create logic as per your suggestion here https://review.coreboot.org/c/coreboot/+/33116/7

Change in ...coreboot[master]: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33394 )

Change subject: stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable
......................................................................

stage_cache: Make empty inline function if CONFIG_NO_STAGE_CACHE enable

This patch removes CONFIG_NO_STAGE_CACHE check from caller function
and add empty inline function incase CONFIG_NO_STAGE_CACHE is enable.

Change-Id: I8e10ef2d261f9b204cecbeae6f65fda037753534
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33394
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
Reviewed-by: Furquan Shaikh 
---
M src/arch/x86/postcar_loader.c
M src/include/stage_cache.h
M src/lib/prog_loaders.c
3 files changed, 11 insertions(+), 11 deletions(-)

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

diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c
index e5d0cea..732b767 100644
--- a/src/arch/x86/postcar_loader.c
+++ b/src/arch/x86/postcar_loader.c
@@ -154,8 +154,7 @@

finalize_load(rsl.params, pcf->stack);

-	if (!CONFIG(NO_STAGE_CACHE))
-		stage_cache_add(STAGE_POSTCAR, prog);
+	stage_cache_add(STAGE_POSTCAR, prog);
 }

void run_postcar_phase(struct postcar_frame *pcf)
diff --git a/src/include/stage_cache.h b/src/include/stage_cache.h
index 28cb859..192cfb9 100644
--- a/src/include/stage_cache.h
+++ b/src/include/stage_cache.h
@@ -32,14 +32,21 @@
    STAGE_S3_DATA,
 };

+#if CONFIG(CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) \
+		|| CONFIG(RELOCATABLE_RAMSTAGE)
 /* Cache the loaded stage provided according to the parameters. */
 void stage_cache_add(int stage_id, const struct prog *stage);
+/* Load the cached stage at given location returning the stage entry point. */
+void stage_cache_load_stage(int stage_id, struct prog *stage);
+#else /* CONFIG_NO_STAGE_CACHE */
+static inline void stage_cache_add(int stage_id, const struct prog *stage) {}
+static inline void stage_cache_load_stage(int stage_id, struct prog *stage) {}
+#endif
+
 /* Cache non-specific data or code. */
 void stage_cache_add_raw(int stage_id, const void *base, const size_t size);
 /* Get a pointer to cached raw data and its size. */
 void stage_cache_get_raw(int stage_id, void **base, size_t *size);
-/* Load the cached stage at given location returning the stage entry point. */
-void stage_cache_load_stage(int stage_id, struct prog *stage);
 /* Fill in parameters for the external stage cache, if utilized. */
 void stage_cache_external_region(void **base, size_t *size);

diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index a21663f..81ec2ec 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -74,11 +74,6 @@
    halt();
 }

-void __weak stage_cache_add(int stage_id,
-						const struct prog *stage) {}
-void __weak stage_cache_load_stage(int stage_id,
-							struct prog *stage) {}
-
 static void ramstage_cache_invalid(void)
 {
    printk(BIOS_ERR, "ramstage cache invalid.\n");
@@ -155,8 +150,7 @@
    } else if (load_nonrelocatable_ramstage(&ramstage))
    	goto fail;

-	if (!CONFIG(NO_STAGE_CACHE))
-		stage_cache_add(STAGE_RAMSTAGE, &ramstage);
+	stage_cache_add(STAGE_RAMSTAGE, &ramstage);

timestamp_add_now(TS_END_COPYRAM);