Change in ...coreboot[master]: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map
Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31245
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map
This implementation corrects the GPIO IRQ start map used for ITSS polarity configuration. The lowest GPIO IRQ mapped in SKL/KBL, CNL, and ICL is 24.
BUG=b:123315212 TEST=[TESTED on CNL-ONLY] Verify the ITSS polarity configuration for GPIOs are correctly getting restored to original coreboot configuration, after fsp-s call in itss_restore_irq_polarities() call.
Change-Id: I3c12e6ca01453da92259f077771c3f4d887aa03d Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/include/soc/itss.h M src/soc/intel/icelake/include/soc/itss.h M src/soc/intel/skylake/include/soc/itss.h 3 files changed, 3 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31245/1
diff --git a/src/soc/intel/cannonlake/include/soc/itss.h b/src/soc/intel/cannonlake/include/soc/itss.h index 0d8b2ca..09245e7 100644 --- a/src/soc/intel/cannonlake/include/soc/itss.h +++ b/src/soc/intel/cannonlake/include/soc/itss.h @@ -16,7 +16,7 @@ #ifndef SOC_INTEL_CNL_ITSS_H #define SOC_INTEL_CNL_ITSS_H
-#define GPIO_IRQ_START 50 +#define GPIO_IRQ_START 24 #define GPIO_IRQ_END ITSS_MAX_IRQ
#define ITSS_MAX_IRQ 119 diff --git a/src/soc/intel/icelake/include/soc/itss.h b/src/soc/intel/icelake/include/soc/itss.h index d846ce0..c27a2ea 100644 --- a/src/soc/intel/icelake/include/soc/itss.h +++ b/src/soc/intel/icelake/include/soc/itss.h @@ -16,7 +16,7 @@ #ifndef SOC_INTEL_ICL_ITSS_H #define SOC_INTEL_ICL_ITSS_H
-#define GPIO_IRQ_START 50 +#define GPIO_IRQ_START 24 #define GPIO_IRQ_END ITSS_MAX_IRQ
#define ITSS_MAX_IRQ 119 diff --git a/src/soc/intel/skylake/include/soc/itss.h b/src/soc/intel/skylake/include/soc/itss.h index e6eb8b0..a987cab 100644 --- a/src/soc/intel/skylake/include/soc/itss.h +++ b/src/soc/intel/skylake/include/soc/itss.h @@ -16,7 +16,7 @@ #ifndef SOC_INTEL_SKL_ITSS_H #define SOC_INTEL_SKL_ITSS_H
-#define GPIO_IRQ_START 50 +#define GPIO_IRQ_START 24 #define GPIO_IRQ_END ITSS_MAX_IRQ
#define ITSS_MAX_IRQ 119
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31245 )
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
Patch Set 1: Code-Review+1
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31245 )
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/itss.h:
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... PS1, Line 19: 24 the purpose of making it 50 is that to ensure we are taking snap shot between IPC1-IPCn and excluding IPC0 register entry.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31245 )
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/itss.h:
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... PS1, Line 19: 24
the purpose of making it 50 is that to ensure we are taking snap shot between IPC1-IPCn and excludin […]
Ok, for that it had to be 32. The GPIO IRQ map starts from 24 to 119 with 0-23 reserved for internal PCH devices. For taking the GPIO IRQ snap shot we can keep the start index to 24.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31245 )
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31245/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31245/1//COMMIT_MSG@14 PS1, Line 14: [TESTED on CNL-ONLY] Can you please test this on some KBL board?
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/itss.h:
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... PS1, Line 19: 24
Ok, for that it had to be 32. […]
restoring of polarities already takes care of updating only the right bits based on start offset.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31245 )
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/itss.h:
https://review.coreboot.org/#/c/31245/1/src/soc/intel/cannonlake/include/soc... PS1, Line 19: 24
restoring of polarities already takes care of updating only the right bits based on start offset.
Sounds like there needs to be come comments put down explaining the implicit assumptions.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Paul Menzel, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31245
to look at the new patch set (#2).
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map
This implementation corrects the GPIO IRQ start map used for ITSS polarity configuration. The lowest GPIO IRQ mapped in SKL/KBL, CNL, and ICL is 24. However the PCI controller devices are mapped upto 34, hence setting the gpio irq snaphot restore from irq#35.
BUG=b:123315212 TEST=[TESTED on CNP(hatch) and SPT(soraka)] Verify the ITSS polarity configuration for GPIOs are correctly getting restored to original coreboot configuration, after fsp-s call in itss_restore_irq_polarities() call.
Change-Id: I3c12e6ca01453da92259f077771c3f4d887aa03d Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/include/soc/itss.h M src/soc/intel/icelake/include/soc/itss.h M src/soc/intel/skylake/include/soc/itss.h 3 files changed, 21 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/31245/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31245 )
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31245/2/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/itss.h:
https://review.coreboot.org/#/c/31245/2/src/soc/intel/cannonlake/include/soc... PS2, Line 21: PCI devices are reserving IRQ till 34 This will be violating the interrupt controller requirements that IRQ > 29 should not have more than one interrupt source. GPIO controller already claims INTSEL 29-34. If PCI devices are also doing that, won't that lead to issues?
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31245?usp=email )
Change subject: soc/intel/{skylake,cannonlake,icelake}: Correct GPIO IRQ start map ......................................................................
Abandoned
-
Aamir Bohra (Code Review) -
Aaron Durbin (Code Review)
-
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)
-
Subrata Banik (Code Review)