Change in ...coreboot[master]: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configura...
Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31246
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration
This implementation moves saving ITSS IPCx regsister snaphsot after the GPIO pad configuration has configured the polarity bits for active low interrupts.
BUG=b:123315212 TEST=Verify the ITSS polarities set in ITSS IPCx registers are correctly restored for active low interrupts ater FSP-S call.
Change-Id: Id7b6f732759538ca41c872308727b1d87c2c5d85 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/31246/1
diff --git a/src/soc/intel/cannonlake/chip.c b/src/soc/intel/cannonlake/chip.c index 4604d80..541155d 100644 --- a/src/soc/intel/cannonlake/chip.c +++ b/src/soc/intel/cannonlake/chip.c @@ -142,10 +142,6 @@
void soc_init_pre_device(void *chip_info) { - /* Snapshot the current GPIO IRQ polarities. FSP is setting a - * default policy that doesn't honor boards' requirements. */ - itss_snapshot_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END); - /* Perform silicon specific init. */ fsp_silicon_init(romstage_handoff_is_resume());
diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c index 866d9c8..595069b 100644 --- a/src/soc/intel/cannonlake/fsp_params.c +++ b/src/soc/intel/cannonlake/fsp_params.c @@ -19,8 +19,10 @@ #include <device/pci.h> #include <fsp/api.h> #include <fsp/util.h> +#include <intelblocks/itss.h> #include <intelblocks/xdci.h> #include <soc/intel/common/vbt.h> +#include <soc/itss.h> #include <soc/pci_devs.h> #include <soc/ramstage.h> #include <string.h> @@ -87,6 +89,9 @@ }
mainboard_silicon_init_params(params); + /* Snapshot the current GPIO IRQ polarities just after GPIO pad configuration. + * FSP is setting a default policy that doesn't honor boards' requirements. */ + itss_snapshot_irq_polarities(GPIO_IRQ_START, GPIO_IRQ_END);
/* Unlock upper 8 bytes of RTC RAM */ params->PchLockDownRtcMemoryLock = 0;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31246/1/src/soc/intel/cannonlake/fsp_params.... File src/soc/intel/cannonlake/fsp_params.c:
https://review.coreboot.org/#/c/31246/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 92: /* Snapshot the current GPIO IRQ polarities just after GPIO pad configuration. line over 80 characters
https://review.coreboot.org/#/c/31246/1/src/soc/intel/cannonlake/fsp_params.... PS1, Line 93: * FSP is setting a default policy that doesn't honor boards' requirements. */ line over 80 characters
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31246
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration
This implementation moves saving ITSS IPCx regsister snaphsot after the GPIO pad configuration has configured the polarity bits for active low interrupts.
BUG=b:123315212 TEST=Verify the ITSS polarities set in ITSS IPCx registers are correctly restored for active low interrupts ater FSP-S call.
Change-Id: Id7b6f732759538ca41c872308727b1d87c2c5d85 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/fsp_params.c 2 files changed, 6 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/31246/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 2:
this should be across all soc then ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 2:
Patch Set 2:
this should be across all soc then ?
Yes, it looks like this will be required for all SoCs. I am wondering if we should just move snapshot/restore to do_silicon_init() in silicon_init.c
APL/GLK is special since it calls gpio config differently than what KBL/CNL do. Hence, this issue was never seen on platforms using APL/GLK.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31246/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31246/2//COMMIT_MSG@9 PS2, Line 9: This implementation moves saving ITSS IPCx regsister snaphsot after Just one space before ITSS please.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 2:
Patch Set 2:
this should be across all soc then ?
Yes, it looks like this will be required for all SoCs. I am wondering if we should just move snapshot/restore to do_silicon_init() in silicon_init.c
APL/GLK is special since it calls gpio config differently than what KBL/CNL do. Hence, this issue was never seen on platforms using APL/GLK.
do_silicon_init() in silicon_init.c is in fsp driver (which is common between small and big cores) i believe then we might need to make use of common PCH Kconfig option to achieve this.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
this should be across all soc then ?
Yes, it looks like this will be required for all SoCs. I am wondering if we should just move snapshot/restore to do_silicon_init() in silicon_init.c
APL/GLK is special since it calls gpio config differently than what KBL/CNL do. Hence, this issue was never seen on platforms using APL/GLK.
do_silicon_init() in silicon_init.c is in fsp driver (which is common between small and big cores) i believe then we might need to make use of common PCH Kconfig option to achieve this.
Yes, This has to be common across SOCs. Will check and revise the implementation.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
this should be across all soc then ?
Yes, it looks like this will be required for all SoCs. I am wondering if we should just move snapshot/restore to do_silicon_init() in silicon_init.c
APL/GLK is special since it calls gpio config differently than what KBL/CNL do. Hence, this issue was never seen on platforms using APL/GLK.
do_silicon_init() in silicon_init.c is in fsp driver (which is common between small and big cores) i believe then we might need to make use of common PCH Kconfig option to achieve this.
Yes, This has to be common across SOCs. Will check and revise the implementation.
Thinking about this again: Its okay to just do this in every SoC like it is done right now. However, we should fix FSP to not touch the GPIO IRQ polarities. Subrata, can you please follow up on this?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31246 )
Change subject: soc/intel/cannonlake: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Patch Set 2:
this should be across all soc then ?
Yes, it looks like this will be required for all SoCs. I am wondering if we should just move snapshot/restore to do_silicon_init() in silicon_init.c
APL/GLK is special since it calls gpio config differently
than what
KBL/CNL do. Hence, this issue was never seen on platforms
using
APL/GLK.
do_silicon_init() in silicon_init.c is in fsp driver (which is
common between small and big cores) i believe then we might need to make use of common PCH Kconfig option to achieve this.
Yes, This has to be common across SOCs. Will check and revise the
implementation.
Thinking about this again: Its okay to just do this in every SoC like it is done right now. However, we should fix FSP to not touch the GPIO IRQ polarities. Subrata, can you please follow up on this?
i think its been taken care in ICL as i know where only IPC0 is something that FSP should bother rest should be don't care for FSP. I can check if same CL need to be ported for CNL/CFL/CML
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31246
to look at the new patch set (#3).
Change subject: soc/intel/{cannonlake,skylake}: Take ITSS polarity snaphot after GPIO configuration ......................................................................
soc/intel/{cannonlake,skylake}: Take ITSS polarity snaphot after GPIO configuration
This implementation moves saving ITSS IPCx regsister snaphsot after the GPIO pad configuration has configured the polarity bits for active low interrupts.
BUG=b:123315212 TEST=Verify the ITSS polarities set in ITSS IPCx registers are correctly restored for active low interrupts ater FSP-S call.
Change-Id: Id7b6f732759538ca41c872308727b1d87c2c5d85 Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/chip.c M src/soc/intel/cannonlake/fsp_params.c M src/soc/intel/skylake/chip_fsp20.c 3 files changed, 14 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/31246/3
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31246?usp=email )
Change subject: soc/intel/{cannonlake,skylake}: Take ITSS polarity snaphot after GPIO configuration ......................................................................
Abandoned
-
Aamir Bohra (Code Review) -
build bot (Jenkins) (Code Review)
-
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)
-
Subrata Banik (Code Review)