Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
[WIP] intel/skylake: Refactor IRQ assignments
Change-Id: Ifc4795245187f8d70650242a56e6ce771ef2167a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/skylake/include/soc/interrupt.h M src/soc/intel/skylake/irq.c 2 files changed, 44 insertions(+), 86 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35735/1
diff --git a/src/soc/intel/skylake/include/soc/interrupt.h b/src/soc/intel/skylake/include/soc/interrupt.h index 015ea76..3bf3ca8 100644 --- a/src/soc/intel/skylake/include/soc/interrupt.h +++ b/src/soc/intel/skylake/include/soc/interrupt.h @@ -31,9 +31,9 @@ #define PCH_PHRC 7 #define PCH_MAX_IRQ_CONFIG 8
-#define DEVICE_INT_CONFIG(dev, func, line, irqno) {\ - .Device = dev, \ - .Function = func, \ +#define DEVICE_INT_CONFIG(devfn, line, irqno) {\ + .Device = PCI_SLOT(devfn), \ + .Function = PCI_FUNC(devfn), \ .IntX = line, \ .Irq = irqno }
diff --git a/src/soc/intel/skylake/irq.c b/src/soc/intel/skylake/irq.c index ddaffda..6e6d655 100644 --- a/src/soc/intel/skylake/irq.c +++ b/src/soc/intel/skylake/irq.c @@ -28,194 +28,152 @@ * cAVS(Audio, Voice, Speech), INTA is default, programmed in * PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_HDA), int_A, cAVS_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_HDA, int_A, cAVS_INTA_IRQ), /* * SMBus Controller, no default value, programmed in * PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_SMBUS), int_A, SMBUS_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SMBUS, int_A, SMBUS_INTA_IRQ), /* GbE Controller, INTA is default, programmed in PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_GBE), int_A, GbE_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_GBE, int_A, GbE_INTA_IRQ), /* TraceHub, INTA is default, RO register */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_TRACEHUB), int_A, - TRACE_HUB_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_TRACEHUB, int_A, TRACE_HUB_INTA_IRQ), /* * SerialIo: UART #0, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[7] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_UART0), int_A, LPSS_UART0_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_UART0, int_A, LPSS_UART0_IRQ), /* * SerialIo: UART #1, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[8] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_UART1), int_B, LPSS_UART1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_UART1, int_B, LPSS_UART1_IRQ), /* * SerialIo: SPI #0, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[10] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_GSPI0), int_C, LPSS_SPI0_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_GSPI0, int_C, LPSS_SPI0_IRQ), /* * SerialIo: SPI #1, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[11] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_GSPI1), int_D, LPSS_SPI1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_GSPI1, int_D, LPSS_SPI1_IRQ), /* SCS: eMMC (SKL PCH-LP Only) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_EMMC), int_B, eMMC_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_EMMC, int_B, eMMC_IRQ), /* SCS: SDIO (SKL PCH-LP Only) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_SDIO), int_C, SDIO_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SDIO, int_C, SDIO_IRQ), /* SCS: SDCard (SKL PCH-LP Only) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_SDCARD), int_D, SD_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SDCARD, int_D, SD_IRQ), /* PCI Express Port, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE9), int_A, PCIE_9_IRQ), - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE10), int_B, PCIE_10_IRQ), - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE11), int_C, PCIE_11_IRQ), - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE12), int_D, PCIE_12_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE9, int_A, PCIE_9_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE10, int_B, PCIE_10_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE11, int_C, PCIE_11_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE12, int_D, PCIE_12_IRQ), /* * PCI Express Port 1, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE1), int_A, PCIE_1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE1, int_A, PCIE_1_IRQ), /* * PCI Express Port 2, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE2), int_B, PCIE_2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE2, int_B, PCIE_2_IRQ), /* * PCI Express Port 3, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE3), int_C, PCIE_3_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE3, int_C, PCIE_3_IRQ), /* * PCI Express Port 4, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE4), int_D, PCIE_4_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE4, int_D, PCIE_4_IRQ), /* * PCI Express Port 5, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE5), int_A, PCIE_5_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE5, int_A, PCIE_5_IRQ), /* * PCI Express Port 6, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE6), int_B, PCIE_6_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE6, int_B, PCIE_6_IRQ), /* * PCI Express Port 7, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE7), int_C, PCIE_7_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE7, int_C, PCIE_7_IRQ), /* * PCI Express Port 8, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE8), int_D, PCIE_8_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE8, int_D, PCIE_8_IRQ), /* * SerialIo UART Controller #2, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[9] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO2, - PCI_FUNC(PCH_DEVFN_UART2), int_A, LPSS_UART2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_UART2, int_A, LPSS_UART2_IRQ), /* * SerialIo UART Controller #5, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[6] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO2, - PCI_FUNC(PCH_DEVFN_I2C5), int_B, LPSS_I2C5_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C5, int_B, LPSS_I2C5_IRQ), /* * SerialIo UART Controller #4, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[5] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO2, - PCI_FUNC(PCH_DEVFN_I2C4), int_C, LPSS_I2C4_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C4, int_C, LPSS_I2C4_IRQ), /* * SATA Controller, INTA is default, * programmed in PciCfgSpace + 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SATA, - PCI_FUNC(PCH_DEVFN_SATA), int_A, SATA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SATA, int_A, SATA_IRQ), /* CSME: HECI #1 */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE), int_A, HECI_1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE, int_A, HECI_1_IRQ), /* CSME: HECI #2 */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_2), int_B, HECI_2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_2, int_B, HECI_2_IRQ), /* CSME: IDE-Redirection (IDE-R) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_IDER), int_C, IDER_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_IDER, int_C, IDER_IRQ), /* CSME: Keyboard and Text (KT) Redirection */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_KT), int_D, KT_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_KT, int_D, KT_IRQ), /* CSME: HECI #3 */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_3), int_A, HECI_3_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_3, int_A, HECI_3_IRQ), /* * SerialIo I2C Controller #0, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[1] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C0), int_A, LPSS_I2C0_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C0, int_A, LPSS_I2C0_IRQ), /* * SerialIo I2C Controller #1, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[2] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C1), int_B, LPSS_I2C1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C1, int_B, LPSS_I2C1_IRQ), /* * SerialIo I2C Controller #2, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[3] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C2), int_C, LPSS_I2C2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C2, int_C, LPSS_I2C2_IRQ), /* * SerialIo I2C Controller #3, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[4] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C3), int_D, LPSS_I2C3_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C3, int_D, LPSS_I2C3_IRQ), /* * USB 3.0 xHCI Controller, no default value, * programmed in PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_XHCI), int_A, XHCI_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_XHCI, int_A, XHCI_IRQ), /* USB Device Controller (OTG) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_USBOTG), int_B, OTG_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_USBOTG, int_B, OTG_IRQ), /* Thermal Subsystem */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_THERMAL), int_C, THERMAL_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_THERMAL, int_C, THERMAL_IRQ), /* Camera IO Host Controller */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_CIO), int_A, CIO_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CIO, int_A, CIO_INTA_IRQ), /* Integrated Sensor Hub */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_ISH, - PCI_FUNC(PCH_DEVFN_ISH), int_A, ISH_IRQ) + DEVICE_INT_CONFIG(PCH_DEVFN_ISH, int_A, ISH_IRQ) };
void soc_irq_settings(FSP_SIL_UPD *params)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1:
(2 comments)
Is there some quirk with SI_PCH_DEVICE_INTERRUPT_CONFIG that would require entries where arguments device and function would not originate from same PCI BDF?
Also, I do not see fsp2_0/skylake/FspUpdVpd.h anywhere, how does this code work from chip_fsp20.c?
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c File src/soc/intel/skylake/irq.c:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 192: sizeof(SI_PCH_DEVICE_INTERRUPT_CONFIG)); Who allocates target buffer?
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 212: memcpy(params->PxRcConfig, irq_config, PCH_MAX_IRQ_CONFIG); Who allocates target buffer? FSP? But PCH_MAX_IRQ_CONFIG is not defined in FSP headers.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1:
Also, I do not see fsp2_0/skylake/FspUpdVpd.h anywhere, how does this code work from chip_fsp20.c?
FSP headers pulled from FSP submodule (3rdparty/fsp/KabylakeFspBinPkg/Include/)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Patch Set 1:
(2 comments)
Is there some quirk with SI_PCH_DEVICE_INTERRUPT_CONFIG that would require entries where arguments device and function would not originate from same PCI BDF?
Also, I do not see fsp2_0/skylake/FspUpdVpd.h anywhere, how does this code work from chip_fsp20.c?
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/interrupt.h:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/inclu... PS1, Line 37: line Can we add parens around these macro parameters while we'er here?
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c File src/soc/intel/skylake/irq.c:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 192: sizeof(SI_PCH_DEVICE_INTERRUPT_CONFIG));
Who allocates target buffer?
It's used directly from the fsp-s memory.
fsp_silicon_init -> do_silicon_init -> platform_fsp_silicon_init_params_cb -> soc_irq_settings
params are embeddedin the FspsConfig object within the FSPS_UPD object.
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 212: memcpy(params->PxRcConfig, irq_config, PCH_MAX_IRQ_CONFIG);
Who allocates target buffer? FSP? But PCH_MAX_IRQ_CONFIG is not defined in FSP headers.
$ grep -r PxRcConfig 3rdparty/fsp/KabylakeFspBinPkg/ 3rdparty/fsp/KabylakeFspBinPkg/Include/FspsUpd.h: UINT8 PxRcConfig[8];
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c File src/soc/intel/skylake/irq.c:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 192: sizeof(SI_PCH_DEVICE_INTERRUPT_CONFIG));
It's used directly from the fsp-s memory. […]
Some weird assymetry there?
src/soc/intel/skylake/chip_fsp20.c: params->GraphicsConfigPtr = (u32) vbt_data;
I don't see params->DevIntConfigPtr initialised anywhere. And if it was, how amny entries can it hold?
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 212: memcpy(params->PxRcConfig, irq_config, PCH_MAX_IRQ_CONFIG);
$ grep -r PxRcConfig 3rdparty/fsp/KabylakeFspBinPkg/ […]
Yes, it's sort of fine, except made assumption ARRAY_SIZE(PxRcConfig)<=PCH_MAX_IRQ_CONFIG.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c File src/soc/intel/skylake/irq.c:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 192: sizeof(SI_PCH_DEVICE_INTERRUPT_CONFIG));
Some weird assymetry there? […]
Was this supposed to be just:
params->DevIntConfigPtr = (uintptr_t)(void *)devintconfig;
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c File src/soc/intel/skylake/irq.c:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 192: sizeof(SI_PCH_DEVICE_INTERRUPT_CONFIG));
Was this supposed to be just: […]
I wasn't looking that closely. Sorry. I was thinking this is the same sequence as the PxRcConfig below. Yes, that seems quite problematic. And it appears we're just using the NULL pointer as the config (if FSP is honoring it sitting at address 0). Or it's all ignored.
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 212: memcpy(params->PxRcConfig, irq_config, PCH_MAX_IRQ_CONFIG);
Yes, it's sort of fine, except made assumption ARRAY_SIZE(PxRcConfig)<=PCH_MAX_IRQ_CONFIG.
Correct. Static analysis tools will totally catch it as well as the compiler. The assumption is actually flipped, though:
PCH_MAX_IRQ_CONFIG <= ARRAY_SIZE(PxRcConfig)
Ideally we wouldn't duplicate things, but then we'd have to carry the dependency from fsp header file all the way everywhere within coreboot code.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: [WIP] intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c File src/soc/intel/skylake/irq.c:
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 192: sizeof(SI_PCH_DEVICE_INTERRUPT_CONFIG));
I wasn't looking that closely. Sorry. […]
Well I won't fix this. Subrata?
https://review.coreboot.org/c/coreboot/+/35735/1/src/soc/intel/skylake/irq.c... PS1, Line 212: memcpy(params->PxRcConfig, irq_config, PCH_MAX_IRQ_CONFIG);
Correct. Static analysis tools will totally catch it as well as the compiler. […]
Ack
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Matt DeVillier, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35735
to look at the new patch set (#2).
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
intel/skylake: Refactor IRQ assignments
When creating the IRQ routing, referenced device and function number always come from the same pci_devfn_t identifier.
Change-Id: Ifc4795245187f8d70650242a56e6ce771ef2167a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/skylake/include/soc/interrupt.h M src/soc/intel/skylake/irq.c 2 files changed, 46 insertions(+), 88 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35735/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG@10 PS2, Line 10: pci_devfn_t But this isn't really pci_devfn_t, right? It's the pci struct device_path encoding. I know we intend to converge those, but I think we should be accurate in the current implementation.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG@10 PS2, Line 10: pci_devfn_t
But this isn't really pci_devfn_t, right? It's the pci struct device_path encoding. […]
I could rephrase this as... 'are always of the same PCI device'.
Too early for me to try converge pci_devfn_t encodings right now.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG@10 PS2, Line 10: pci_devfn_t
I could rephrase this as... 'are always of the same PCI device'. […]
Yes, please reword. I was working on some patches to fix up some that divergence as well -- helper functions and not open coding direct access. Once we have that we should be able to target convergence.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Matt DeVillier, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35735
to look at the new patch set (#3).
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
intel/skylake: Refactor IRQ assignments
When creating the IRQ routing, referenced device and function number are always of the same PCI device.
Change-Id: Ifc4795245187f8d70650242a56e6ce771ef2167a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/skylake/include/soc/interrupt.h M src/soc/intel/skylake/irq.c 2 files changed, 46 insertions(+), 88 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/35735/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 3: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35735/2//COMMIT_MSG@10 PS2, Line 10: pci_devfn_t
Yes, please reword. […]
Done
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35735 )
Change subject: intel/skylake: Refactor IRQ assignments ......................................................................
intel/skylake: Refactor IRQ assignments
When creating the IRQ routing, referenced device and function number are always of the same PCI device.
Change-Id: Ifc4795245187f8d70650242a56e6ce771ef2167a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35735 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/skylake/include/soc/interrupt.h M src/soc/intel/skylake/irq.c 2 files changed, 46 insertions(+), 88 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/soc/intel/skylake/include/soc/interrupt.h b/src/soc/intel/skylake/include/soc/interrupt.h index 015ea76..bc65498 100644 --- a/src/soc/intel/skylake/include/soc/interrupt.h +++ b/src/soc/intel/skylake/include/soc/interrupt.h @@ -31,11 +31,11 @@ #define PCH_PHRC 7 #define PCH_MAX_IRQ_CONFIG 8
-#define DEVICE_INT_CONFIG(dev, func, line, irqno) {\ - .Device = dev, \ - .Function = func, \ - .IntX = line, \ - .Irq = irqno } +#define DEVICE_INT_CONFIG(devfn, line, irqno) {\ + .Device = PCI_SLOT(devfn), \ + .Function = PCI_FUNC(devfn), \ + .IntX = (line), \ + .Irq = (irqno) }
#define no_int 0 #define int_A 1 diff --git a/src/soc/intel/skylake/irq.c b/src/soc/intel/skylake/irq.c index ddaffda..6e6d655 100644 --- a/src/soc/intel/skylake/irq.c +++ b/src/soc/intel/skylake/irq.c @@ -28,194 +28,152 @@ * cAVS(Audio, Voice, Speech), INTA is default, programmed in * PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_HDA), int_A, cAVS_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_HDA, int_A, cAVS_INTA_IRQ), /* * SMBus Controller, no default value, programmed in * PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_SMBUS), int_A, SMBUS_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SMBUS, int_A, SMBUS_INTA_IRQ), /* GbE Controller, INTA is default, programmed in PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_GBE), int_A, GbE_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_GBE, int_A, GbE_INTA_IRQ), /* TraceHub, INTA is default, RO register */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_LPC, - PCI_FUNC(PCH_DEVFN_TRACEHUB), int_A, - TRACE_HUB_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_TRACEHUB, int_A, TRACE_HUB_INTA_IRQ), /* * SerialIo: UART #0, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[7] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_UART0), int_A, LPSS_UART0_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_UART0, int_A, LPSS_UART0_IRQ), /* * SerialIo: UART #1, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[8] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_UART1), int_B, LPSS_UART1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_UART1, int_B, LPSS_UART1_IRQ), /* * SerialIo: SPI #0, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[10] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_GSPI0), int_C, LPSS_SPI0_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_GSPI0, int_C, LPSS_SPI0_IRQ), /* * SerialIo: SPI #1, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[11] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_GSPI1), int_D, LPSS_SPI1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_GSPI1, int_D, LPSS_SPI1_IRQ), /* SCS: eMMC (SKL PCH-LP Only) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_EMMC), int_B, eMMC_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_EMMC, int_B, eMMC_IRQ), /* SCS: SDIO (SKL PCH-LP Only) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_SDIO), int_C, SDIO_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SDIO, int_C, SDIO_IRQ), /* SCS: SDCard (SKL PCH-LP Only) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_STORAGE, - PCI_FUNC(PCH_DEVFN_SDCARD), int_D, SD_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SDCARD, int_D, SD_IRQ), /* PCI Express Port, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE9), int_A, PCIE_9_IRQ), - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE10), int_B, PCIE_10_IRQ), - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE11), int_C, PCIE_11_IRQ), - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE_1, - PCI_FUNC(PCH_DEVFN_PCIE12), int_D, PCIE_12_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE9, int_A, PCIE_9_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE10, int_B, PCIE_10_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE11, int_C, PCIE_11_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE12, int_D, PCIE_12_IRQ), /* * PCI Express Port 1, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE1), int_A, PCIE_1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE1, int_A, PCIE_1_IRQ), /* * PCI Express Port 2, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE2), int_B, PCIE_2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE2, int_B, PCIE_2_IRQ), /* * PCI Express Port 3, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE3), int_C, PCIE_3_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE3, int_C, PCIE_3_IRQ), /* * PCI Express Port 4, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE4), int_D, PCIE_4_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE4, int_D, PCIE_4_IRQ), /* * PCI Express Port 5, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE5), int_A, PCIE_5_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE5, int_A, PCIE_5_IRQ), /* * PCI Express Port 6, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE6), int_B, PCIE_6_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE6, int_B, PCIE_6_IRQ), /* * PCI Express Port 7, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE7), int_C, PCIE_7_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE7, int_C, PCIE_7_IRQ), /* * PCI Express Port 8, INT is default, * programmed in PciCfgSpace + FCh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_PCIE, - PCI_FUNC(PCH_DEVFN_PCIE8), int_D, PCIE_8_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_PCIE8, int_D, PCIE_8_IRQ), /* * SerialIo UART Controller #2, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[9] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO2, - PCI_FUNC(PCH_DEVFN_UART2), int_A, LPSS_UART2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_UART2, int_A, LPSS_UART2_IRQ), /* * SerialIo UART Controller #5, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[6] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO2, - PCI_FUNC(PCH_DEVFN_I2C5), int_B, LPSS_I2C5_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C5, int_B, LPSS_I2C5_IRQ), /* * SerialIo UART Controller #4, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[5] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO2, - PCI_FUNC(PCH_DEVFN_I2C4), int_C, LPSS_I2C4_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C4, int_C, LPSS_I2C4_IRQ), /* * SATA Controller, INTA is default, * programmed in PciCfgSpace + 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SATA, - PCI_FUNC(PCH_DEVFN_SATA), int_A, SATA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_SATA, int_A, SATA_IRQ), /* CSME: HECI #1 */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE), int_A, HECI_1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE, int_A, HECI_1_IRQ), /* CSME: HECI #2 */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_2), int_B, HECI_2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_2, int_B, HECI_2_IRQ), /* CSME: IDE-Redirection (IDE-R) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_IDER), int_C, IDER_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_IDER, int_C, IDER_IRQ), /* CSME: Keyboard and Text (KT) Redirection */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_KT), int_D, KT_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_KT, int_D, KT_IRQ), /* CSME: HECI #3 */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_CSE, - PCI_FUNC(PCH_DEVFN_CSE_3), int_A, HECI_3_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CSE_3, int_A, HECI_3_IRQ), /* * SerialIo I2C Controller #0, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[1] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C0), int_A, LPSS_I2C0_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C0, int_A, LPSS_I2C0_IRQ), /* * SerialIo I2C Controller #1, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[2] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C1), int_B, LPSS_I2C1_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C1, int_B, LPSS_I2C1_IRQ), /* * SerialIo I2C Controller #2, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[3] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C2), int_C, LPSS_I2C2_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C2, int_C, LPSS_I2C2_IRQ), /* * SerialIo I2C Controller #3, INTA is default, * programmed in PCR[SERIALIO] + PCICFGCTRL[4] */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_SIO1, - PCI_FUNC(PCH_DEVFN_I2C3), int_D, LPSS_I2C3_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_I2C3, int_D, LPSS_I2C3_IRQ), /* * USB 3.0 xHCI Controller, no default value, * programmed in PciCfgSpace 3Dh */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_XHCI), int_A, XHCI_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_XHCI, int_A, XHCI_IRQ), /* USB Device Controller (OTG) */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_USBOTG), int_B, OTG_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_USBOTG, int_B, OTG_IRQ), /* Thermal Subsystem */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_THERMAL), int_C, THERMAL_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_THERMAL, int_C, THERMAL_IRQ), /* Camera IO Host Controller */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_XHCI, - PCI_FUNC(PCH_DEVFN_CIO), int_A, CIO_INTA_IRQ), + DEVICE_INT_CONFIG(PCH_DEVFN_CIO, int_A, CIO_INTA_IRQ), /* Integrated Sensor Hub */ - DEVICE_INT_CONFIG(PCH_DEV_SLOT_ISH, - PCI_FUNC(PCH_DEVFN_ISH), int_A, ISH_IRQ) + DEVICE_INT_CONFIG(PCH_DEVFN_ISH, int_A, ISH_IRQ) };
void soc_irq_settings(FSP_SIL_UPD *params)
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Matt DeVillier (Code Review)