Karthik Ramasubramanian has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
soc/intel/jasperlake: Add ISST override support
Add support for mainboard to override Intel Speed Shift Technology (ISST). This will allow the mainboards to override default ISST configuration during the phases where it is not supported.
BUG=b:151281860 TEST=Build and boot the mainboard.
Change-Id: I811cb2a3d4ee3a7e8689601a1d440547633f5885 Signed-off-by: Karthikeyan Ramasubramanian kramasub@google.com --- M src/soc/intel/jasperlake/cpu.c M src/soc/intel/jasperlake/include/soc/cpu.h 2 files changed, 9 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/41429/1
diff --git a/src/soc/intel/jasperlake/cpu.c b/src/soc/intel/jasperlake/cpu.c index 2533fe0..30023e9 100644 --- a/src/soc/intel/jasperlake/cpu.c +++ b/src/soc/intel/jasperlake/cpu.c @@ -29,6 +29,7 @@ config_t *conf = config_of_soc(); msr_t msr;
+ mainboard_isst_override(); if (conf->speed_shift_enable) { /* * Kernel driver checks CPUID.06h:EAX[Bit 7] to determine if HWP @@ -247,3 +248,8 @@ if (mp_init_with_smm(cpu_bus, &mp_ops)) printk(BIOS_ERR, "MP initialization failure.\n"); } + +__weak void mainboard_isst_override(void) +{ + /* Nothing to override. */ +} diff --git a/src/soc/intel/jasperlake/include/soc/cpu.h b/src/soc/intel/jasperlake/include/soc/cpu.h index 58e9a8f..b4faaac 100644 --- a/src/soc/intel/jasperlake/include/soc/cpu.h +++ b/src/soc/intel/jasperlake/include/soc/cpu.h @@ -33,4 +33,7 @@ /* Configure power limits for turbo mode */ void set_power_limits(u8 power_limit_1_time);
+/* Mainboard Intel Speed Shift Technology override */ +void mainboard_isst_override(void); + #endif
Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
suggestion: In Hatch, we used a variant_devtree_update() function, (see ramstage.c in hatch). It's slightly more generic-sounding and if variants need to make changes later (and they probably will 😊) they can all go in there, instead of making a separate callback for each change.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41429/1/src/soc/intel/jasperlake/in... File src/soc/intel/jasperlake/include/soc/cpu.h:
https://review.coreboot.org/c/coreboot/+/41429/1/src/soc/intel/jasperlake/in... PS1, Line 37: mainboard_isst_override Do we really need this additional callback into mainboard? Can't this be handled as part of mainboard chip_init?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
Patch Set 1:
(1 comment)
Couple of reasons I added this callback: 1) It happens as part of CPU initialization. It definitely happens before mainboard Silicon initialization(as pointed out by Tim). I have to check if mainboard Chip initialization. If this happens before CPU initialization, then we can update the configuration there. 2) If all mainboard initialization happens only after CPU initialization, then there will be a small window for which ISST will be enabled before getting disabled. I am not sure if there is a side-effect of because of that. Also we have to export configure_isst to mainboard and invoke that again to disable it.
These are the reasons I added the callback.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
Couple of reasons I added this callback:
- It happens as part of CPU initialization. It definitely happens before mainboard Silicon initialization(as pointed out by Tim). I have to check if mainboard Chip initialization. If this happens before CPU initialization, then we can update the configuration there.
This is the sequence in which different bootstates are executed: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/bootstate.h?i.... IIUC, CPU initialization is being done as part of BS_DEV_INIT, whereas mainboard chip initialization happens in BS_DEV_INIT_CHIPS.
- If all mainboard initialization happens only after CPU initialization, then there will be a small window for which ISST will be enabled before getting disabled. I am not sure if there is a side-effect of because of that. Also we have to export configure_isst to mainboard and invoke that again to disable it.
That shouldn't be required. As long as we can make sure that config->speed_shift_enable is updated before BS_DEV_INIT, it should work.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
Patch Set 1:
Couple of reasons I added this callback:
- It happens as part of CPU initialization. It definitely happens before mainboard Silicon initialization(as pointed out by Tim). I have to check if mainboard Chip initialization. If this happens before CPU initialization, then we can update the configuration there.
This is the sequence in which different bootstates are executed: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/bootstate.h?i.... IIUC, CPU initialization is being done as part of BS_DEV_INIT, whereas mainboard chip initialization happens in BS_DEV_INIT_CHIPS.
I see that CPUs are getting initialized also at BS_DEV_INIT_CHIPS boot state - https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...
The test also indicates the ISST is getting enabled if moved to mainboard chip init.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Couple of reasons I added this callback:
- It happens as part of CPU initialization. It definitely happens before mainboard Silicon initialization(as pointed out by Tim). I have to check if mainboard Chip initialization. If this happens before CPU initialization, then we can update the configuration there.
This is the sequence in which different bootstates are executed: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/bootstate.h?i.... IIUC, CPU initialization is being done as part of BS_DEV_INIT, whereas mainboard chip initialization happens in BS_DEV_INIT_CHIPS.
I see that CPUs are getting initialized also at BS_DEV_INIT_CHIPS boot state - https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...
The test also indicates the ISST is getting enabled if moved to mainboard chip init.
From the BIOS logs, I can see that the CPU initialization are done on entry callback to BS_DEV_INIT_CHIPS stage. The mainboard chip init and Silicon Init are done as part of BS_DEV_INIT_CHIPS i.e. run part of BS_DEV_INIT_CHIPS. So the only option to avoid the callback in cpu.c is to add a callback on entry or exit stage of BS_PRE_DEVICE stage. Please find the BIOS logs for reference:
Initializing CPU #0 ... CPU #0 initialized Initializing CPU #2 Initializing CPU #3 Initializing CPU #1 ... CPU #3 initialized ... CPU #1 initialized ... CPU #2 initialized bsp_do_flight_plan done after 248 msecs. CPU: frequency set to 1900 MHz Enabling SMIs. Locking SMM. BS: BS_DEV_INIT_CHIPS entry times (exec / console): 177 / 217 ms ... WEAK: src/soc/intel/jasperlake/fsp_params.c/mainboard_silicon_init_params called Detected 4 core, 4 thread CPU. ... IPC3: 0xffffffff BS: BS_DEV_INIT_CHIPS run times (exec / console): 510 / 134 ms RTC Init ... Disabling Deep S5 BS: BS_DEV_INIT_CHIPS exit times (exec / console): 3 / 19 ms
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Couple of reasons I added this callback:
- It happens as part of CPU initialization. It definitely happens before mainboard Silicon initialization(as pointed out by Tim). I have to check if mainboard Chip initialization. If this happens before CPU initialization, then we can update the configuration there.
This is the sequence in which different bootstates are executed: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/bootstate.h?i.... IIUC, CPU initialization is being done as part of BS_DEV_INIT, whereas mainboard chip initialization happens in BS_DEV_INIT_CHIPS.
I see that CPUs are getting initialized also at BS_DEV_INIT_CHIPS boot state - https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...
The test also indicates the ISST is getting enabled if moved to mainboard chip init.
From the BIOS logs, I can see that the CPU initialization are done on entry callback to BS_DEV_INIT_CHIPS stage. The mainboard chip init and Silicon Init are done as part of BS_DEV_INIT_CHIPS i.e. run part of BS_DEV_INIT_CHIPS. So the only option to avoid the callback in cpu.c is to add a callback on entry or exit stage of BS_PRE_DEVICE stage. Please find the BIOS logs for reference:
Initializing CPU #0 ... CPU #0 initialized Initializing CPU #2 Initializing CPU #3 Initializing CPU #1 ... CPU #3 initialized ... CPU #1 initialized ... CPU #2 initialized bsp_do_flight_plan done after 248 msecs. CPU: frequency set to 1900 MHz Enabling SMIs. Locking SMM. BS: BS_DEV_INIT_CHIPS entry times (exec / console): 177 / 217 ms ... WEAK: src/soc/intel/jasperlake/fsp_params.c/mainboard_silicon_init_params called Detected 4 core, 4 thread CPU. ... IPC3: 0xffffffff BS: BS_DEV_INIT_CHIPS run times (exec / console): 510 / 134 ms RTC Init ... Disabling Deep S5 BS: BS_DEV_INIT_CHIPS exit times (exec / console): 3 / 19 ms
If the requirement is that this should be done before FSP-S runs, then what Tim suggested makes sense. Basically, add a variant_devtree_update() as part of mainboard_silicon_init_params(). That guarantees the right ordering.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Couple of reasons I added this callback:
- It happens as part of CPU initialization. It definitely happens before mainboard Silicon initialization(as pointed out by Tim). I have to check if mainboard Chip initialization. If this happens before CPU initialization, then we can update the configuration there.
This is the sequence in which different bootstates are executed: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/bootstate.h?i.... IIUC, CPU initialization is being done as part of BS_DEV_INIT, whereas mainboard chip initialization happens in BS_DEV_INIT_CHIPS.
I see that CPUs are getting initialized also at BS_DEV_INIT_CHIPS boot state - https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...
The test also indicates the ISST is getting enabled if moved to mainboard chip init.
From the BIOS logs, I can see that the CPU initialization are done on entry callback to BS_DEV_INIT_CHIPS stage. The mainboard chip init and Silicon Init are done as part of BS_DEV_INIT_CHIPS i.e. run part of BS_DEV_INIT_CHIPS. So the only option to avoid the callback in cpu.c is to add a callback on entry or exit stage of BS_PRE_DEVICE stage. Please find the BIOS logs for reference:
Initializing CPU #0 ... CPU #0 initialized Initializing CPU #2 Initializing CPU #3 Initializing CPU #1 ... CPU #3 initialized ... CPU #1 initialized ... CPU #2 initialized bsp_do_flight_plan done after 248 msecs. CPU: frequency set to 1900 MHz Enabling SMIs. Locking SMM. BS: BS_DEV_INIT_CHIPS entry times (exec / console): 177 / 217 ms ... WEAK: src/soc/intel/jasperlake/fsp_params.c/mainboard_silicon_init_params called Detected 4 core, 4 thread CPU. ... IPC3: 0xffffffff BS: BS_DEV_INIT_CHIPS run times (exec / console): 510 / 134 ms RTC Init ... Disabling Deep S5 BS: BS_DEV_INIT_CHIPS exit times (exec / console): 3 / 19 ms
If the requirement is that this should be done before FSP-S runs, then what Tim suggested makes sense. Basically, add a variant_devtree_update() as part of mainboard_silicon_init_params(). That guarantees the right ordering.
I had misunderstood the comment before. Thanks Karthik for the pointers. So, basically, it looks like even though we have init_one_cpu() set as device_ops.init, mp_init really happens in BS_DEV_INIT_CHIPS state which triggers init_one_cpu() in BS_DEV_INIT_CHIPS.
Since the current change is just a temporary one required for some early versions of the boards, we can restrict the change the mainboard by adding a callback in BS_PRE_DEVICE and ensure that isst is setup correctly there.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Couple of reasons I added this callback:
- It happens as part of CPU initialization. It definitely happens before mainboard Silicon initialization(as pointed out by Tim). I have to check if mainboard Chip initialization. If this happens before CPU initialization, then we can update the configuration there.
This is the sequence in which different bootstates are executed: https://review.coreboot.org/cgit/coreboot.git/tree/src/include/bootstate.h?i.... IIUC, CPU initialization is being done as part of BS_DEV_INIT, whereas mainboard chip initialization happens in BS_DEV_INIT_CHIPS.
I see that CPUs are getting initialized also at BS_DEV_INIT_CHIPS boot state - https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...
The test also indicates the ISST is getting enabled if moved to mainboard chip init.
From the BIOS logs, I can see that the CPU initialization are done on entry callback to BS_DEV_INIT_CHIPS stage. The mainboard chip init and Silicon Init are done as part of BS_DEV_INIT_CHIPS i.e. run part of BS_DEV_INIT_CHIPS. So the only option to avoid the callback in cpu.c is to add a callback on entry or exit stage of BS_PRE_DEVICE stage. Please find the BIOS logs for reference:
Initializing CPU #0 ... CPU #0 initialized Initializing CPU #2 Initializing CPU #3 Initializing CPU #1 ... CPU #3 initialized ... CPU #1 initialized ... CPU #2 initialized bsp_do_flight_plan done after 248 msecs. CPU: frequency set to 1900 MHz Enabling SMIs. Locking SMM. BS: BS_DEV_INIT_CHIPS entry times (exec / console): 177 / 217 ms ... WEAK: src/soc/intel/jasperlake/fsp_params.c/mainboard_silicon_init_params called Detected 4 core, 4 thread CPU. ... IPC3: 0xffffffff BS: BS_DEV_INIT_CHIPS run times (exec / console): 510 / 134 ms RTC Init ... Disabling Deep S5 BS: BS_DEV_INIT_CHIPS exit times (exec / console): 3 / 19 ms
If the requirement is that this should be done before FSP-S runs, then what Tim suggested makes sense. Basically, add a variant_devtree_update() as part of mainboard_silicon_init_params(). That guarantees the right ordering.
I had misunderstood the comment before. Thanks Karthik for the pointers. So, basically, it looks like even though we have init_one_cpu() set as device_ops.init, mp_init really happens in BS_DEV_INIT_CHIPS state which triggers init_one_cpu() in BS_DEV_INIT_CHIPS.
Since the current change is just a temporary one required for some early versions of the boards, we can restrict the change the mainboard by adding a callback in BS_PRE_DEVICE and ensure that isst is setup correctly there.
Added a BS_PRE_DEVICE callback instead of CPU specific callback. All the changes are self-contained in MB - https://review.coreboot.org/c/coreboot/+/39477/5. This patch can be abandoned.
Karthik Ramasubramanian has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41429 )
Change subject: soc/intel/jasperlake: Add ISST override support ......................................................................
Abandoned
Abandoning this CL due to everything self-contained in https://review.coreboot.org/c/coreboot/+/39477/5
-
Furquan Shaikh (Code Review) -
Justin TerAvest (Code Review)
-
Karthik Ramasubramanian (Code Review)
-
Tim Wawrzynczak (Code Review)