Bora Guvendik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32992
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/cannonlake/romstage/fsp_params.c 3 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32992/1
diff --git a/src/soc/intel/cannonlake/chip.h b/src/soc/intel/cannonlake/chip.h index 0d51c1c..6bea8fe 100644 --- a/src/soc/intel/cannonlake/chip.h +++ b/src/soc/intel/cannonlake/chip.h @@ -421,6 +421,7 @@ * Bit 0: MISCCFG_GPDLCGEN */ uint8_t gpio_pm[TOTAL_GPIO_COMM]; + uint8_t Heci1Disabled; };
typedef struct soc_intel_cannonlake_config config_t; diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index dd93882..7dc1dd0 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -341,6 +341,7 @@ params->ScsUfsEnabled = dev->enabled;
params->Heci3Enabled = config->Heci3Enabled; + params->Heci1Disabled = config->Heci1Disabled; params->Device4Enable = config->Device4Enable;
/* VrConfig Settings for 5 domains diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 6e492bb..365fb9f 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -25,7 +25,6 @@ #include <vendorcode/google/chromeos/chromeos.h>
#include "../chip.h" - static void soc_memory_init_params(FSP_M_CONFIG *m_cfg, const config_t *config) { unsigned int i;
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32992
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M src/soc/intel/cannonlake/chip.h M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32992/2
Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 2:
I don't expect it to compile here yet but it should work in firmware-sarien-12200.B branch
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 2:
Need to upload the updated FSP header files.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h@216 PS4, Line 216: /* HeciEnabled decides the state of Heci1 at end of boot : * Setting to 0 (default) disables Heci1 and hides the device from OS */ : uint8_t HeciEnabled; Why are we not using this config?
Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h@216 PS4, Line 216: /* HeciEnabled decides the state of Heci1 at end of boot : * Setting to 0 (default) disables Heci1 and hides the device from OS */ : uint8_t HeciEnabled;
Why are we not using this config?
I forgot about that but it seems HeciEnabled is being used at smihandler_soc_at_finalize().
if (config->HeciEnabled == 0) pch_disable_heci();
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h@216 PS4, Line 216: /* HeciEnabled decides the state of Heci1 at end of boot : * Setting to 0 (default) disables Heci1 and hides the device from OS */ : uint8_t HeciEnabled;
I forgot about that but it seems HeciEnabled is being used at smihandler_soc_at_finalize(). […]
In my opinion we should get rid of the SMM way of disabling HECI and just use the FSP UPD.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h@216 PS4, Line 216: /* HeciEnabled decides the state of Heci1 at end of boot : * Setting to 0 (default) disables Heci1 and hides the device from OS */ : uint8_t HeciEnabled;
In my opinion we should get rid of the SMM way of disabling HECI and just use the FSP UPD.
I will fix this today, this UPD is not added for all platform so far, for an example: right now UPD is enable on WHL and CML where SMM mode won't have IOSF access. CNL and ICL still relies on SMM mode. i will add CONFIG for now to skip as we have common code for CNL, WHl and CML
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/chip.h@424 PS5, Line 424: Heci1Disabled make use of config HeciEnabled line 218
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 344: Heci1Disabled i guess you can use !config->HeciEnabled to assign FSP UPD, in that way you don't need another one
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h@216 PS4, Line 216: /* HeciEnabled decides the state of Heci1 at end of boot : * Setting to 0 (default) disables Heci1 and hides the device from OS */ : uint8_t HeciEnabled;
I will fix this today, this UPD is not added for all platform so far, for an example: right now UPD […]
CL https://review.coreboot.org/c/coreboot/+/33193
Hello Subrata Banik, Aamir Bohra, Duncan Laurie, build bot (Jenkins), Lijian Zhao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32992
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32992/6
Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/chip.h@424 PS5, Line 424: Heci1Disabled
make use of config HeciEnabled line 218
Done
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h File src/soc/intel/cannonlake/chip.h:
https://review.coreboot.org/#/c/32992/4/src/soc/intel/cannonlake/chip.h@216 PS4, Line 216: /* HeciEnabled decides the state of Heci1 at end of boot : * Setting to 0 (default) disables Heci1 and hides the device from OS */ : uint8_t HeciEnabled;
CL https://review.coreboot. […]
Done
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/5/src/soc/intel/cannonlake/fsp_params.... PS5, Line 344: Heci1Disabled
i guess you can use !config->HeciEnabled to assign FSP UPD, in that way you don't need another one
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/32992/10/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/10/src/soc/intel/cannonlake/fsp_params... PS10, Line 344: Disabled Just wanted to point out how this option is "negative" compared to everything around it. Would it make sense to invert it so that it becomes "Heci1Enabled"?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/32992/10/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/10/src/soc/intel/cannonlake/fsp_params... PS10, Line 344: Disabled
Just wanted to point out how this option is "negative" compared to everything around it. […]
I have been told changing this is not trivial, so please disregard. Sorry for the noise.
Hello Subrata Banik, Aamir Bohra, Selma Bensaid, Duncan Laurie, build bot (Jenkins), Lijian Zhao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32992
to look at the new patch set (#11).
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32992/11
Hello Subrata Banik, Aamir Bohra, Selma Bensaid, Duncan Laurie, build bot (Jenkins), Lijian Zhao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32992
to look at the new patch set (#12).
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32992/12
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 12: Code-Review+1
tested on out-of-tree WHL-U board, HECI1 PCI device is not visible/accessible to OS after boot
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 12: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 12:
i guess headers has to merge first then this CL.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE) I am a bit confused about the two ways of disabling Heci1: 1. Using FSP 2. Using SMM
From a previous change that was pushed, it looks like for CNL, SMM was being used: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/cannonlake/...
With this change, it looks like CNL is using FSP to disable Heci1? Also, why is this done for all cannonlake SoCs except cometlake? Is that just a matter of FSP header being udpated?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
I am a bit confused about the two ways of disabling Heci1: […]
As i know CNL is not using FSP's way to make HECI disable, as this is not requested and timeline doesn't matches. Now do we know if WHL and CNL is using same FSP then, my understanding is wrong.
In general CNL and ICL will use SMM way to make HECI disable and WHL, CFL and CML should use FSP ways to make HECI disable
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
As i know CNL is not using FSP's way to make HECI disable, as this is not requested and timeline doe […]
In that case, the check here does not look right to me i.e. if !CONFIG(SOC_INTEL_COMETLAKE)?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13: -Code-Review
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
In that case, the check here does not look right to me i.e. […]
yes, if its due to FSP UPD doesn't exist today for CML ?
#if CONFIG(SOC_INTEL_WHISKEYLAKE)
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
yes, if its due to FSP UPD doesn't exist today for CML ?
#if CONFIG(SOC_INTEL_WHISKEYLAKE)
but then you're dropping CFL
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
yes, if its due to FSP UPD doesn't exist today for CML ? […]
Is there an ETA for the CML FSP UPD update? If we get that in, then this can just be set to #if !CONFIG(HECI_DISABLE_USING_SMM)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
Is there an ETA for the CML FSP UPD update? If we get that in, then this can just be set to #if !CONFIG(HECI_DISABLE_USING_SMM)
@Aamir, do you know ETA ?
but then you're dropping CFL
So far this requirement is only applicable for chrome platform and there are no CFL based chrome platform.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
So far this requirement is only applicable for chrome platform and there are no CFL based chrome platform.
but we don't want to have the situation where setting HeciEnabled=0 doesn't produce the expected result on CFL, esp when CFL and WHL use the same FSP binary
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
So far this requirement is only applicable for chrome platform and there are no CFL based chrome p […]
my question was would you also like to set HeciEnabled = 0 for CFL platform with non-chrome OS ? if yes then you need to set that UPD as well
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/32992/13/src/soc/intel/cannonlake/fsp_params... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
my question was would you also like to set HeciEnabled = 0 for CFL platform with non-chrome OS ? if yes then you need to set that UPD as well
I don't see any reason why we would assume that UPD to be ChromeOS-specific. So guarding against platforms which use SMM method to disable seems to make the most sense
Hello Subrata Banik, Aamir Bohra, Selma Bensaid, Matt DeVillier, Duncan Laurie, build bot (Jenkins), Lijian Zhao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32992
to look at the new patch set (#14).
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32992/14
Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32992/13/src/soc/intel/cannonlake/f... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/32992/13/src/soc/intel/cannonlake/f... PS13, Line 344: !CONFIG(SOC_INTEL_COMETLAKE)
my question was would you also like to set HeciEnabled = 0 for CFL platform with non-chrome OS ? i […]
Changed it as per Furquan's suggestion since Aamir's CML FSP UPD patch is available as of yesterday.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32992/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32992/15//COMMIT_MSG@12 PS15, Line 12: TEST=Boot to OS, verify if Heci1 is disabled What device was this tested on? What FSP version was used to test this?
Hello Patrick Rudolph, Subrata Banik, Aamir Bohra, Selma Bensaid, Matt DeVillier, Duncan Laurie, build bot (Jenkins), Lijian Zhao,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32992
to look at the new patch set (#16).
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled on hatch system using FSP 1344.
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/32992/16
Bora Guvendik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/32992/15//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32992/15//COMMIT_MSG@12 PS15, Line 12: TEST=Boot to OS, verify if Heci1 is disabled
What device was this tested on? What FSP version was used to test this?
Sorry about late reply, I was on vacation.
Tested with FSP 1344 on hatch, updated the comment.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 16: Code-Review+2
Kindly note, making HECI disable using PMC API has ~20ms of boot time penalties. so to make sure we are not surprise to see ReadToBoot time increases by ~20ms now onwards
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
Patch Set 16:
Patch Set 16: Code-Review+2
Kindly note, making HECI disable using PMC API has ~20ms of boot time penalties. so to make sure we are not surprise to see ReadToBoot time increases by ~20ms now onwards
kindly ignore this msg, it was some FSP issue which causes 20ms regression. we are good now once FSP boot time regression got fixed, this CL is good now. with ~2ms of extra time taken..
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/32992 )
Change subject: soc/intel/cannonlake: Add ability to disable Heci1 ......................................................................
soc/intel/cannonlake: Add ability to disable Heci1
Decide if HECI1 should be hidden prior to boot to OS.
BUG=none TEST=Boot to OS, verify if Heci1 is disabled on hatch system using FSP 1344.
Change-Id: I7c63316c8b04fb101d34064daac5ba4fdc05a63c Signed-off-by: Bora Guvendik bora.guvendik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32992 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/fsp_params.c 1 file changed, 3 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Subrata Banik: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 5cdddc6..06c556c 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -349,6 +349,9 @@ params->ScsUfsEnabled = dev->enabled;
params->Heci3Enabled = config->Heci3Enabled; +#if !CONFIG(HECI_DISABLE_USING_SMM) + params->Heci1Disabled = !config->HeciEnabled; +#endif params->Device4Enable = config->Device4Enable;
/* VrConfig Settings for 5 domains
-
Aamir Bohra (Code Review) -
Angel Pons (Code Review)
-
Bora Guvendik (Code Review)
-
Furquan Shaikh (Code Review)
-
Matt DeVillier (Code Review)
-
Subrata Banik (Code Review)