Change in coreboot[master]: soc/intel/jasperlake: Allow mainboard to override chip configuration
Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
soc/intel/jasperlake: Allow mainboard to override chip configuration
Add a weak override function to allow mainboard to override chip configuration like GPIO PM.
BUG=None TEST=Build and boot waddledee to OS. Ensure that the suspend/resume sequence works fine.
Change-Id: I40fa655b0324dc444182b988f0089587e3877a47 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/jasperlake/fsp_params.c M src/soc/intel/jasperlake/include/soc/ramstage.h 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/56/45856/1
diff --git a/src/soc/intel/jasperlake/fsp_params.c b/src/soc/intel/jasperlake/fsp_params.c index cdd088e..d140213 100644 --- a/src/soc/intel/jasperlake/fsp_params.c +++ b/src/soc/intel/jasperlake/fsp_params.c @@ -78,6 +78,11 @@ sizeof(config->SerialIoUartMode)); }
+__weak void mainboard_update_soc_chip_config(struct soc_intel_jasperlake_config *config) +{ + /* Override settings per board. */ +} + /* UPD parameters to be initialized before SiliconInit */ void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd) { @@ -86,6 +91,9 @@ FSP_S_CONFIG *params = &supd->FspsConfig; struct soc_intel_jasperlake_config *config = config_of_soc();
+ /* Allow mainboard to override any chip config */ + mainboard_update_soc_chip_config(config); + /* Parse device tree and fill in FSP UPDs */ parse_devicetree(params);
diff --git a/src/soc/intel/jasperlake/include/soc/ramstage.h b/src/soc/intel/jasperlake/include/soc/ramstage.h index 8188fbd..1de8e37 100644 --- a/src/soc/intel/jasperlake/include/soc/ramstage.h +++ b/src/soc/intel/jasperlake/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_jasperlake_config *config); void soc_init_pre_device(void *chip_info);
#endif
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override Shouldn't this be run at the end so that it can actually override something? This way it's more like the board can provide defaults and the code below would have the last word.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override
Shouldn't this be run at the end so that it can actually override something? […]
Well, this can override the chip config, not the UPD parameters directly.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override
Well, this can override the chip config, not the UPD parameters directly.
oh, right. thanks :)
I've decided that the devicetree `chip` term is wrong so long ago... It's just hard to follow. We use the "chip config" to configure software to configure software to configure... a chip :)
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override
oh, right. thanks :) […]
Completely agree, somehow the terminology ended up sounding very strange. Maybe a treewide rename (+ proper docs) is appropriate sometime in the nearish future?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override
Completely agree, somehow the terminology ended up sounding very strange. […]
Maybe. I wouldn't know how to call it, though.
Btw. now that I understand what this change is doing, I realized that it's not needed. The `chip` config can be altered by the mainboard code at any time before silicon init, e.g. the mainboard's `chip` driver's `.init`.
We already have a lot of flexibility in coreboot. Most of these calls to other entities I see in soc/intel/ are not needed. Sometimes because we already have a generic mechanism in place, sometimes they only exist because a function parameter is missing (i.e. instead of passing some information, let the callee directly call back to ask for its parameters; don't know what that is, it's not procedural programming).
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override
Maybe. I wouldn't know how to call it, though. […]
I don't have any better suggestions ATM either 😞
You are right, I think the reason is that the mainboard chip driver often gets forgotten. chip init is called in BS_DEV_INIT_CHIPS, and I believe it's guaranteed by the devtree model that the mainboard is the (one of anyway) first real device in `all_devices`, therefore this should be OK. FSP init is also called from chip init (of the SoC), which may be the reason why we've used these `mainboard_update` functions (otherwise you're relying on the fact that the mainboard is first in all_devices).
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override
(otherwise you're relying on the fact that the mainboard is first in all_devices).
AFAIR, we often make that assumption. It's a bit implicit, but with the current code it's guaranteed (`all_devices` points to `dev_root` which is the mainboard).
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
(1 comment)
Karthik, Nico raises a good point; have you considered putting this in the mainboard chip init?
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... File src/soc/intel/jasperlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/45856/1/src/soc/intel/jasperlake/fs... PS1, Line 94: override
(otherwise you're relying on the fact that the mainboard is first in all_devices). […]
That is true, just pointing it out, the fewer assumptions we have to make, the better IMO 😊
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
Karthik, Nico raises a good point; have you considered putting this in the mainboard chip init?
I am fine with putting in the mainboard chip init. I will try it out and update the CL.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
(1 comment)
Karthik, Nico raises a good point; have you considered putting this in the mainboard chip init?
I am fine with putting in the mainboard chip init. I will try it out and update the CL.
Moved everything into CB:45857. This CL can be abandoned once that is accepted.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45856 )
Change subject: soc/intel/jasperlake: Allow mainboard to override chip configuration ......................................................................
soc/intel/jasperlake: Allow mainboard to override chip configuration
Add a weak override function to allow mainboard to override chip configuration like GPIO PM.
BUG=None TEST=Build and boot waddledee to OS. Ensure that the suspend/resume sequence works fine.
Change-Id: I40fa655b0324dc444182b988f0089587e3877a47 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45856 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/soc/intel/jasperlake/fsp_params.c M src/soc/intel/jasperlake/include/soc/ramstage.h 2 files changed, 9 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/jasperlake/fsp_params.c b/src/soc/intel/jasperlake/fsp_params.c index 1919936..bf279ec 100644 --- a/src/soc/intel/jasperlake/fsp_params.c +++ b/src/soc/intel/jasperlake/fsp_params.c @@ -78,6 +78,11 @@ sizeof(config->SerialIoUartMode)); }
+__weak void mainboard_update_soc_chip_config(struct soc_intel_jasperlake_config *config) +{ + /* Override settings per board. */ +} + /* UPD parameters to be initialized before SiliconInit */ void platform_fsp_silicon_init_params_cb(FSPS_UPD *supd) { @@ -86,6 +91,9 @@ FSP_S_CONFIG *params = &supd->FspsConfig; struct soc_intel_jasperlake_config *config = config_of_soc();
+ /* Allow mainboard to override any chip config */ + mainboard_update_soc_chip_config(config); + /* Parse device tree and fill in FSP UPDs */ parse_devicetree(params);
diff --git a/src/soc/intel/jasperlake/include/soc/ramstage.h b/src/soc/intel/jasperlake/include/soc/ramstage.h index 8188fbd..1de8e37 100644 --- a/src/soc/intel/jasperlake/include/soc/ramstage.h +++ b/src/soc/intel/jasperlake/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_jasperlake_config *config); void soc_init_pre_device(void *chip_info);
#endif
-
Furquan Shaikh (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Tim Wawrzynczak (Code Review)