Change in coreboot[master]: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package
Aamir Bohra has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package
PIRX ACPI package is dynamically genarated PCI IRQ routing information based on SOC_INTEL_COMMON_ITSS_INTERRUPT_OVERRIDE config selection. This implementation returns PIRX package under _PRT method for IOAPIC mode of interrupt.
Change-Id: Ib23e57d3ce58406f8faff18e912b78a911e50d3d Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/acpi/pci_irqs.asl M src/soc/intel/icelake/acpi/pci_irqs.asl 2 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/34710/1
diff --git a/src/soc/intel/cannonlake/acpi/pci_irqs.asl b/src/soc/intel/cannonlake/acpi/pci_irqs.asl index accfdb9..f18fa19 100644 --- a/src/soc/intel/cannonlake/acpi/pci_irqs.asl +++ b/src/soc/intel/cannonlake/acpi/pci_irqs.asl @@ -17,6 +17,12 @@
#include <soc/irq.h>
+/* PCI IRQ routing package that gets generated dynamically + * if config SOC_INTEL_COMMON_ITSS_INTERRUPT_OVERRIDE is + * selected. + */ +External(PIRX) + Name (PICP, Package () { /* PCI Bridge */ /* cAVS, SMBus, GbE, Nothpeak */ @@ -138,6 +144,9 @@ Method (_PRT) { If (PICM) { + if (CondRefOf(PIRX)) { + Return(PIRX) + } Return (^PICP) } Else { Return (^PICN) diff --git a/src/soc/intel/icelake/acpi/pci_irqs.asl b/src/soc/intel/icelake/acpi/pci_irqs.asl index 79c9927..1f07d1d 100644 --- a/src/soc/intel/icelake/acpi/pci_irqs.asl +++ b/src/soc/intel/icelake/acpi/pci_irqs.asl @@ -17,6 +17,12 @@
#include <soc/irq.h>
+/* PCI IRQ routing package that gets generated dynamically + * if config SOC_INTEL_COMMON_ITSS_INTERRUPT_OVERRIDE is + * selected. + */ +External(PIRX) + Name (PICP, Package () { /* PCI Bridge */ /* cAVS, SMBus, GbE, Nothpeak */ @@ -132,6 +138,9 @@ Method (_PRT) { If (PICM) { + if(CondRefOf(PIRX)) { + Return(PIRX) + } Return (^PICP) } Else { Return (^PICN)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 1:
why do you keep the old static IRQ entries?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 2:
Patch Set 1:
why do you keep the old static IRQ entries?
PIRX is only created when SOC would select the IRQ configuration config. IF PIRX is not present the IRQ config is not created using irq common driver and needs to fallback to static package.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 2: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/34710/2/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/2/src/soc/intel/cannonlake/ac... PS2, Line 149: Return nit: space after Return
https://review.coreboot.org/c/coreboot/+/34710/2/src/soc/intel/icelake/acpi/... File src/soc/intel/icelake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/2/src/soc/intel/icelake/acpi/... PS2, Line 142: ( nit: space after if
https://review.coreboot.org/c/coreboot/+/34710/2/src/soc/intel/icelake/acpi/... PS2, Line 143: Return nit: space after return
Hello Patrick Rudolph, Tim Wawrzynczak, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34710
to look at the new patch set (#3).
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package
PIRX ACPI package is dynamically genarated PCI IRQ routing information based on SOC_INTEL_COMMON_ITSS_INTERRUPT_OVERRIDE config selection. This implementation returns PIRX package under _PRT method for IOAPIC mode of interrupt.
Change-Id: Ib23e57d3ce58406f8faff18e912b78a911e50d3d Signed-off-by: Aamir Bohra aamir.bohra@intel.com --- M src/soc/intel/cannonlake/acpi/pci_irqs.asl M src/soc/intel/icelake/acpi/pci_irqs.asl 2 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/34710/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... File src/soc/intel/icelake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... PS4, Line 25: External(PIRX) So if you change the scope to _SB.PCI0, I think this declaration needs to include that as well.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... File src/soc/intel/icelake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... PS4, Line 25: External(PIRX)
So if you change the scope to _SB.PCI0, I think this declaration needs to include that as well.
Done
Lean Sheng Tan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 4: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... File src/soc/intel/icelake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... PS4, Line 141: If (PICM) { : if (CondRefOf (PIRX)) { : Return (PIRX) : } : Return (^PICP) : } Else { : Return (^PICN) : } Why not always generate the ACPI table in SSDT and drop the static ones?
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... File src/soc/intel/icelake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... PS4, Line 141: If (PICM) { : if (CondRefOf (PIRX)) { : Return (PIRX) : } : Return (^PICP) : } Else { : Return (^PICN) : }
Why not always generate the ACPI table in SSDT and drop the static ones?
the dynamic generation is tied up with a Kconfig(in common driver CL in same patch train), if Kconfig is not selected , then need a fallback package. The idea was to allow a provision to on/off the feature if required.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... File src/soc/intel/icelake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... PS4, Line 141: If (PICM) { : if (CondRefOf (PIRX)) { : Return (PIRX) : } : Return (^PICP) : } Else { : Return (^PICN) : }
the dynamic generation is tied up with a Kconfig(in common driver CL in same patch train), if Kconfig is not selected , then need a fallback package. The idea was to allow a provision to on/off the feature if required.
Then always select the dynamic generation? If it works I don't see a reason to provide an option to not use it.
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34710 )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... File src/soc/intel/icelake/acpi/pci_irqs.asl:
https://review.coreboot.org/c/coreboot/+/34710/4/src/soc/intel/icelake/acpi/... PS4, Line 141: If (PICM) { : if (CondRefOf (PIRX)) { : Return (PIRX) : } : Return (^PICP) : } Else { : Return (^PICN) : }
the dynamic generation is tied up with a Kconfig(in common driver CL in same patch train), if Kcon […]
cnl and icl would select it: https://review.coreboot.org/c/coreboot/+/34517 Just wanted to keep it a option for upcoming SOCs, let me know your final thoughts, I am ok to do that changes.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34710?usp=email )
Change subject: soc/intel/{cnl,icl}: Add provision to use PIRX PCI IRQ acpi package ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Aamir Bohra (Code Review) -
Arthur Heymans (Code Review)
-
Lean Sheng Tan (Code Review)
-
Martin L Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Tim Wawrzynczak (Code Review)