Change in coreboot[master]: soc/intel/tigerlake: Add mainboard hook for overriding SoC config

[Jes Klinke (Code Review)]

Hello Jes Klinke,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

soc/intel/tigerlake: Add mainboard hook for overriding SoC config

TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
BUG=b:154333137

Change-Id: Iff28e4a29fab5c22c410cdc743d0402134c4ac56
Signed-off-by: [e-mail]
---
M src/soc/intel/tigerlake/chip.h
M src/soc/intel/tigerlake/fsp_params.c
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44914/1

diff --git a/src/soc/intel/tigerlake/chip.h b/src/soc/intel/tigerlake/chip.h
index 2da63ed..bcc57c8 100644
--- a/src/soc/intel/tigerlake/chip.h
+++ b/src/soc/intel/tigerlake/chip.h
@@ -395,4 +395,7 @@

typedef struct soc_intel_tigerlake_config config_t;

+/* Override settings per board. */
+void mainboard_config_update(config_t *config);
+
 #endif
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index 0a5fbe7..faaff88 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -82,6 +82,11 @@
    PCH_DEVFN_UART2
 };

+void __weak mainboard_config_update(struct soc_intel_tigerlake_config *config)
+{
+	/* Override settings per board. */
+}
+
 /* UPD parameters to be initialized before SiliconInit */
 void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
 {
@@ -92,6 +97,7 @@
    struct device *dev;
    struct soc_intel_tigerlake_config *config;
    config = config_of_soc();
+	mainboard_config_update(config);

/* Parse device tree and enable/disable Serial I/O devices */
    parse_devicetree(params);

[Jes Klinke (Code Review)]

Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44914 )

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 1:

Introducing weak function in Tiger Lake as suggested in https://review.coreboot.org/c/coreboot/+/44359 .

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/44914/1/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/44914/1/src/soc/intel/tigerlake/chi... 
PS1, Line 398: settings
SoC chip config

https://review.coreboot.org/c/coreboot/+/44914/1/src/soc/intel/tigerlake/chi... 
PS1, Line 399: void mainboard_config_update(config_t *config);
nit: Add this to soc/intel/tigerlake/include/soc/ramstage.h Also, maybe name: mainboard_update_soc_chip_config()

[Jes Klinke (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Jes Klinke, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

soc/intel/tigerlake: Add mainboard hook for overriding SoC config

TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
BUG=b:154333137

Change-Id: Iff28e4a29fab5c22c410cdc743d0402134c4ac56
Signed-off-by: [e-mail]
---
M src/soc/intel/tigerlake/fsp_params.c
M src/soc/intel/tigerlake/include/soc/ramstage.h
2 files changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44914/2

[Jes Klinke (Code Review)]

Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44914 )

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/44914/1/src/soc/intel/tigerlake/chi... 
File src/soc/intel/tigerlake/chip.h:

https://review.coreboot.org/c/coreboot/+/44914/1/src/soc/intel/tigerlake/chi... 
PS1, Line 398: settings
> SoC chip config

Changed the name of the method as suggested, eliminating the need for additional comment.

https://review.coreboot.org/c/coreboot/+/44914/1/src/soc/intel/tigerlake/chi... 
PS1, Line 399: void mainboard_config_update(config_t *config);
> nit: Add this to soc/intel/tigerlake/include/soc/ramstage. […]

Done

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 2: Code-Review+1

Thanks Jes! Let's just wait a little more to see if anyone else has any comments. Else I can +2 on Monday.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44914 )

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44914/2/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44914/2/src/soc/intel/tigerlake/fsp... 
PS2, Line 84: 
            : void __weak
nit: we typically do __weak void although I do see both

[Jes Klinke (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Jes Klinke, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

soc/intel/tigerlake: Add mainboard hook for overriding SoC config

TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
BUG=b:154333137

Change-Id: Iff28e4a29fab5c22c410cdc743d0402134c4ac56
Signed-off-by: [e-mail]
---
M src/soc/intel/tigerlake/fsp_params.c
M src/soc/intel/tigerlake/include/soc/ramstage.h
2 files changed, 7 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/44914/3

[Jes Klinke (Code Review)]

Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44914 )

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44914/2/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44914/2/src/soc/intel/tigerlake/fsp... 
PS2, Line 84: 
            : void __weak
> nit: we typically do __weak void although I do see both

Done

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 3: Code-Review+2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44914 )

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44914 )

Change subject: soc/intel/tigerlake: Add mainboard hook for overriding SoC config
......................................................................

soc/intel/tigerlake: Add mainboard hook for overriding SoC config

TEST=util/abuild/abuild -t GOOGLE_VOLTEER -c max -x
BUG=b:154333137

Change-Id: Iff28e4a29fab5c22c410cdc743d0402134c4ac56
Signed-off-by: [e-mail]
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44914
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Tim Wawrzynczak 
---
M src/soc/intel/tigerlake/fsp_params.c
M src/soc/intel/tigerlake/include/soc/ramstage.h
2 files changed, 7 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index 0a5fbe7..1601c2c 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -82,6 +82,11 @@
    PCH_DEVFN_UART2
 };

+__weak void mainboard_update_soc_chip_config(struct soc_intel_tigerlake_config *config)
+{
+	/* Override settings per board. */
+}
+
 /* UPD parameters to be initialized before SiliconInit */
 void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd)
 {
@@ -92,6 +97,7 @@
    struct device *dev;
    struct soc_intel_tigerlake_config *config;
    config = config_of_soc();
+	mainboard_update_soc_chip_config(config);

/* Parse device tree and enable/disable Serial I/O devices */
    parse_devicetree(params);
diff --git a/src/soc/intel/tigerlake/include/soc/ramstage.h b/src/soc/intel/tigerlake/include/soc/ramstage.h
index 8188fbd..ec64eec 100644
--- a/src/soc/intel/tigerlake/include/soc/ramstage.h
+++ b/src/soc/intel/tigerlake/include/soc/ramstage.h
@@ -9,6 +9,7 @@
 #include <soc/soc_chip.h>

void mainboard_silicon_init_params(FSP_S_CONFIG *params);
+void mainboard_update_soc_chip_config(struct soc_intel_tigerlake_config *config);
 void soc_init_pre_device(void *chip_info);

#endif