Change in coreboot[master]: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table
Piotr Kleinschmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table
MP Table and IRQ routing tables were buggy, which results in Linux kernel panic when 'acpi=noirq' flag was added during boot.
TEST=boot Linux debian 4.19.0-9 on PC Engines apu4 platform and see if IRQs are correctly assigned to devices analyzing 'cat proc/interrupts' and `lspci -v` outputs.
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I9eadba65eb5f8f66d1d28c89c7c29d0745c93119 --- M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/mptable.c M src/northbridge/amd/pi/00730F01/pci_devs.h 3 files changed, 34 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/42097/1
diff --git a/src/mainboard/pcengines/apu2/mainboard.c b/src/mainboard/pcengines/apu2/mainboard.c index 170c912..43a27ea 100644 --- a/src/mainboard/pcengines/apu2/mainboard.c +++ b/src/mainboard/pcengines/apu2/mainboard.c @@ -70,7 +70,7 @@ [0x20] = 0x05,0x1F,0x1F,0x1F,0x1F,0x1F,0x00,0x00, [0x28] = 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, /* USB Devs 18/19/20/22 INTA-C */ - [0x30] = 0x12,0x1f,0x12,0x1F,0x12,0x1F,0x1F,0x00, + [0x30] = 0x1F,0x12,0x1F,0x12,0x1F,0x12,0x1F,0x00, [0x38] = 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, /* SATA */ [0x40] = 0x1f,0x13,0x00,0x00,0x00,0x00,0x00,0x00, @@ -92,22 +92,22 @@ */ static const struct pirq_struct mainboard_pirq_data[] = { /* {PCI_devfn, {PIN A, PIN B, PIN C, PIN D}}, */ + {IOMMU_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* IOMMU: 00.2 */ {GFX_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* VGA: 01.0 */ {ACTL_DEVFN,{PIRQ_NC, PIRQ_B, PIRQ_NC, PIRQ_NC}}, /* Audio: 01.1 */ - {NB_PCIE_PORT1_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* x4 PCIe: 02.1 */ - {NB_PCIE_PORT2_DEVFN, {PIRQ_B, PIRQ_C, PIRQ_D, PIRQ_A}}, /* mPCIe: 02.2 */ + {NB_PCIE_PORT1_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* NIC: 02.1 */ + {NB_PCIE_PORT2_DEVFN, {PIRQ_B, PIRQ_C, PIRQ_D, PIRQ_A}}, /* NIC: 02.2 */ {NB_PCIE_PORT3_DEVFN, {PIRQ_C, PIRQ_D, PIRQ_A, PIRQ_B}}, /* NIC: 02.3 */ + {NB_PCIE_PORT4_DEVFN, {PIRQ_D, PIRQ_A, PIRQ_B, PIRQ_C}}, /* NIC: 02.4 */ + {NB_PCIE_PORT5_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* mPCIe slot 1: 02.5 */ {XHCI_DEVFN, {PIRQ_C, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* XHCI: 10.0 */ - {SATA_DEVFN, {PIRQ_SATA, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* SATA: 11.0 */ - {OHCI1_DEVFN, {PIRQ_OHCI1, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* OHCI1: 12.0 */ - {EHCI1_DEVFN, {PIRQ_NC, PIRQ_EHCI1, PIRQ_NC, PIRQ_NC}}, /* EHCI1: 12.2 */ - {OHCI2_DEVFN, {PIRQ_OHCI2, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* OHCI2: 13.0 */ - {EHCI2_DEVFN, {PIRQ_NC, PIRQ_EHCI2, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 13.2 */ - {SMBUS_DEVFN, {PIRQ_SMBUS, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* SMBUS: 14.0 */ - {HDA_DEVFN, {PIRQ_HDA, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* HDA: 14.2 */ - {SD_DEVFN, {PIRQ_SD, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* SD: 14.7 */ - {OHCI3_DEVFN, {PIRQ_OHCI3, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* OHCI3: 16.0 (same device as xHCI 10.0) */ - {EHCI3_DEVFN, {PIRQ_NC, PIRQ_EHCI3, PIRQ_NC, PIRQ_NC}}, /* EHCI3: 16.2 (same device as xHCI 10.1) */ + {SATA_DEVFN, {PIRQ_D, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* SATA: 11.0 */ + {EHCI1_DEVFN, {PIRQ_C, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI0: 12.0 */ + {EHCI2_DEVFN, {PIRQ_C, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI1: 13.0 */ + {SMBUS_DEVFN, {PIRQ_NC, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* SMBUS: 14.0 */ + {LPC_DEVFN, {PIRQ_NC, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* LPC: 14.3 */ + {SD_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* SD: 14.7 */ + {EHCI3_DEVFN, {PIRQ_C, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 16.0 (same device as xHCI 10.1) */ };
/* PIRQ Setup */ diff --git a/src/mainboard/pcengines/apu2/mptable.c b/src/mainboard/pcengines/apu2/mptable.c index 8cfec5d..cca3cc7 100644 --- a/src/mainboard/pcengines/apu2/mptable.c +++ b/src/mainboard/pcengines/apu2/mptable.c @@ -54,6 +54,8 @@ * Source Bus IRQ, Dest APIC ID, Dest PIN# */
+ ioapic_id = (io_apic_read(VIO_APIC_VADDR, 0x00) >> 24); + mptable_add_isa_interrupts(mc, bus_isa, ioapic_id, 0);
/* PCI interrupts are level triggered, and are @@ -62,40 +64,31 @@ #define PCI_INT(bus, dev, int_sign, pin) \ smp_write_intsrc(mc, mp_INT, MP_IRQ_TRIGGER_LEVEL|MP_IRQ_POLARITY_LOW, (bus), (((dev)<<2)|(int_sign)), ioapic_id, (pin))
+ /* IOMMU */ + //ioapic_id = (io_apic_read((u8 *)IO_APIC2_ADDR, 0x00) >> 24); + PCI_INT(0x0, 0x0, 0x2, 0x10);
/* SMBUS / ACPI */ PCI_INT(0x0, 0x14, 0x0, intr_data_ptr[PIRQ_SMBUS]);
/* SD card */ - PCI_INT(0x0, 0x14, 0x1, intr_data_ptr[PIRQ_SD]); + PCI_INT(0x0, 0x14, 0x7, intr_data_ptr[PIRQ_SD]);
/* USB */ - PCI_INT(0x0, 0x12, 0x0, intr_data_ptr[PIRQ_OHCI1]); - PCI_INT(0x0, 0x12, 0x1, intr_data_ptr[PIRQ_EHCI1]); - PCI_INT(0x0, 0x13, 0x0, intr_data_ptr[PIRQ_OHCI2]); - PCI_INT(0x0, 0x13, 0x1, intr_data_ptr[PIRQ_EHCI2]); - PCI_INT(0x0, 0x16, 0x0, intr_data_ptr[PIRQ_OHCI3]); - PCI_INT(0x0, 0x16, 0x1, intr_data_ptr[PIRQ_EHCI3]); - PCI_INT(0x0, 0x14, 0x2, intr_data_ptr[PIRQ_OHCI4]); + PCI_INT(0x0, 0x12, 0x0, intr_data_ptr[PIRQ_EHCI1]); + PCI_INT(0x0, 0x13, 0x0, intr_data_ptr[PIRQ_EHCI2]); + PCI_INT(0x0, 0x16, 0x0, intr_data_ptr[PIRQ_EHCI3]); + PCI_INT(0x0, 0x10, 0x0, intr_data_ptr[PIRQ_EHCI3]);
/* SATA */ PCI_INT(0x0, 0x11, 0x0, intr_data_ptr[PIRQ_SATA]);
/* on board NIC & Slot PCIE */ - PCI_INT(0x1, 0x0, 0x0, intr_data_ptr[PIRQ_E]); - PCI_INT(0x2, 0x0, 0x0, intr_data_ptr[PIRQ_F]); - - - /* GPP0 */ - PCI_INT(0x0, 0x2, 0x0, 0x10); // Network 3 - /* GPP1 */ - PCI_INT(0x0, 0x2, 0x1, 0x11); // Network 2 - /* GPP2 */ - PCI_INT(0x0, 0x2, 0x2, 0x12); // Network 1 - /* GPP3 */ - PCI_INT(0x0, 0x2, 0x3, 0x13); // mPCI - /* GPP4 */ - PCI_INT(0x0, 0x2, 0x4, 0x14); // mPCI + PCI_INT(0x1, 0x0, 0x0, 0x10); + PCI_INT(0x2, 0x0, 0x0, 0x11); + PCI_INT(0x3, 0x0, 0x0, 0x12); + PCI_INT(0x4, 0x0, 0x0, 0x13); + PCI_INT(0x5, 0x0, 0x0, 0x10);
IO_LOCAL_INT(mp_ExtINT, 0x0, MP_APIC_ALL, 0x0); IO_LOCAL_INT(mp_NMI, 0x0, MP_APIC_ALL, 0x1); diff --git a/src/northbridge/amd/pi/00730F01/pci_devs.h b/src/northbridge/amd/pi/00730F01/pci_devs.h index 888e54c..91f7274 100644 --- a/src/northbridge/amd/pi/00730F01/pci_devs.h +++ b/src/northbridge/amd/pi/00730F01/pci_devs.h @@ -5,6 +5,12 @@
#define BUS0 0
+/* IOMMU */ +#define IOMMU_DEV 0x00 +#define IOMMU_FUNC 2 +#define IOMMU_DEVID 0x1567 +#define IOMMU_DEVFN PCI_DEVFN(IOMMU_DEV,IOMMU_FUNC) + /* Graphics and Display */ #define GFX_DEV 0x1 #define GFX_FUNC 0
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 1:
(6 comments)
Patch Set 1:
(1 comment)
Can't this be autogenerated?
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... PS1, Line 73: [0x30] = 0x1F,0x12,0x1F,0x12,0x1F,0x12,0x1F,0x00,
This is wrong, you are trying to set reserved values at offsets 0x31, 0x33 and 0x35, but the interru […]
Okay, I'll revert to 0x30, 0x32 and 0x34 configuration. I made this change because definitions in amd_oci_int_defs suggest that 0x30 is PIRQ_OHCI1, 0x32 is PIRQ_OHCI2... Whereas 0x31 is PIRQ_EHCI1 and so on. Therefore those names should be used in mptable.c file and MP Table configuration routine which may be misleading with OHCI names.
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... PS1, Line 95: {IOMMU_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* IOMMU: 00.2 */
As indicated in other comment, not sure about IOMMU interrupt.
I suggested myself by output from 'lspci -v' with acpi enabled. IOMMU has IRQ assigned there.
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... PS1, Line 107: {SMBUS_DEVFN, {PIRQ_NC, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* SMBUS: 14.0 */
Why no interrupt for SMBus?
As in case of IOMMU I suggested myself by 'lspci -v' with acpi enabled and SMBus doesn't have any IRQ assigned.
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mptable.c:
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... PS1, Line 89: /* GPP0 */ : PCI_INT(0x0, 0x2, 0x0, 0x10); // Network 3 : /* GPP1 */ : PCI_INT(0x0, 0x2, 0x1, 0x11); // Network 2 : /* GPP2 */ : PCI_INT(0x0, 0x2, 0x2, 0x12); // Network 1 : /* GPP3 */ : PCI_INT(0x0, 0x2, 0x3, 0x13); // mPCI : /* GPP4 */ : PCI_INT(0x0, 0x2, 0x4, 0x14); // mPCI :
The bridges should also have their entries in MP table.
Ok, I'll revert this configuration
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... PS1, Line 75: PCI_INT(0x0, 0x14, 0x7, intr_data_ptr[PIRQ_SD]);
The third parameter does not indicate the PCI function number, but the interrupt lane, i.e. […]
Ack
https://review.coreboot.org/c/coreboot/+/42097/1/src/mainboard/pcengines/apu... PS1, Line 81: PCI_INT(0x0, 0x10, 0x0, intr_data_ptr[PIRQ_EHCI3]);
xHCI controller seems to be using PIRQ_C in mainboard_pirq_data. […]
If I understand correctly, XHCI uses PIRQC, but EHCIs controllers use PIRQC too. I have changed 'mainboard_pirq_data' structure according to that, so now every EHCI and XHCI controller uses PIRQC. Is it wrong?
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 2:
This change is ready for review.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... PS2, Line 95: {GFX_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* VGA: 01.0 */ : {ACTL_DEVFN,{PIRQ_NC, PIRQ_B, PIRQ_NC, PIRQ_NC}}, /* Audio: 01.1 */ We do not have the VGA and Audio. GX-412TC lacks the graphics and its audio device as well
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... PS2, Line 101: {NB_PCIE_PORT5_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* mPCIe slot 1: 02.5 */
trailing whitespace
This whitespace should be removed
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mptable.c:
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... PS2, Line 90: /* GPP0 */ : PCI_INT(0x0, 0x2, 0x0, 0x10); // Network 3 : /* GPP1 */ : PCI_INT(0x0, 0x2, 0x1, 0x11); // Network 2 : /* GPP2 */ : PCI_INT(0x0, 0x2, 0x2, 0x12); // Network 1 : /* GPP3 */ : PCI_INT(0x0, 0x2, 0x3, 0x13); // mPCI : /* GPP4 */ : PCI_INT(0x0, 0x2, 0x0, 0x10); // mPCI Aren't the PCIe bridges BDF from 2.1 to 2.5?
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... PS2, Line 99: PCI_INT(0x0, 0x2, 0x0, 0x10); // mPCI What is the reason of change here?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1:
(1 comment)
Can't this be autogenerated?
What do you mean by autogenerated?
I honestly don't recall. I guess I was thinking that the information here is also available elsewhere. If that is the case, then we have to manually keep both implementations in sync, which is cumbersome.
Hello build bot (Jenkins), Michał Żygowski, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42097
to look at the new patch set (#3).
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table
MP Table and IRQ routing tables were buggy, which results in Linux kernel panic when 'acpi=noirq' flag was added during boot.
TEST=boot Linux debian 4.19.0-9 on PC Engines apu4 platform and see if IRQs are correctly assigned to devices analyzing 'cat proc/interrupts' and `lspci -v` outputs.
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I9eadba65eb5f8f66d1d28c89c7c29d0745c93119 --- M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/mptable.c M src/northbridge/amd/pi/00730F01/pci_devs.h 3 files changed, 30 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/42097/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 3:
(16 comments)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 95: {IOMMU_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 96: {NB_PCIE_PORT1_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* NIC: 02.1 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 97: {NB_PCIE_PORT2_DEVFN, {PIRQ_B, PIRQ_C, PIRQ_D, PIRQ_A}}, /* NIC: 02.2 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 99: {NB_PCIE_PORT4_DEVFN, {PIRQ_D, PIRQ_A, PIRQ_B, PIRQ_C}}, /* NIC: 02.4 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 100: {NB_PCIE_PORT5_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* mPCIe slot 1: 02.5 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 103: {EHCI1_DEVFN, {PIRQ_OHCI1, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI0: 12.0 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 104: {EHCI2_DEVFN, {PIRQ_OHCI2, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI1: 13.0 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 106: {EHCI3_DEVFN, {PIRQ_OHCI3, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 16.0 (same device as xHCI 10.0) */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 106: {EHCI3_DEVFN, {PIRQ_OHCI3, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 16.0 (same device as xHCI 10.0) */ space required after that close brace '}'
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 3:
(3 comments)
I revert back to IOMMU interrupt configuration and deleted SMBus interrupt. I suggested myself by Linux kernel which seems to configure those devices such way.
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... PS2, Line 95: {GFX_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* VGA: 01.0 */ : {ACTL_DEVFN,{PIRQ_NC, PIRQ_B, PIRQ_NC, PIRQ_NC}}, /* Audio: 01.1 */
We do not have the VGA and Audio. […]
I deleted both entries
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... PS2, Line 101: {NB_PCIE_PORT5_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* mPCIe slot 1: 02.5 */
This whitespace should be removed
Done
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mptable.c:
https://review.coreboot.org/c/coreboot/+/42097/2/src/mainboard/pcengines/apu... PS2, Line 99: PCI_INT(0x0, 0x2, 0x0, 0x10); // mPCI
What is the reason of change here?
In the other comment, you mentioned that this function actually configure PCI device line, not function interrupt. Am I correct if there are 4 interrupt lines only, so there is no point in trying to configure line 0x4?
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 3:
This change is ready for review.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 3: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG@9 PS3, Line 9: were I’d use present tense.
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG@10 PS3, Line 10: was is
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 3: -Code-Review
(2 comments)
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG@11 PS3, Line 11: Please roughly mention, that there is no audio device, and that OHCI is outdated.
Was the table copy-pasted from some other board?
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, This should be a separate commit. There are more occurrences.
$ git grep 0x1f src/mainboard/pcengines/apu2/mainboard.c src/mainboard/pcengines/apu2/mainboard.c: [0x10] = 0x09,0x1F,0x1F,0x1F,0x1F,0x1f,0x1F,0x10, src/mainboard/pcengines/apu2/mainboard.c: [0x30] = 0x12,0x1f,0x12,0x1F,0x12,0x1F,0x1F,0x00, src/mainboard/pcengines/apu2/mainboard.c: [0x40] = 0x1f,0x13,0x00,0x00,0x00,0x00,0x00,0x00,
Hello build bot (Jenkins), Michał Żygowski, Paul Menzel, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42097
to look at the new patch set (#4).
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table
MP Table and IRQ routing tables are buggy, which results in Linux kernel panic when 'acpi=noirq' flag is added during boot.
On PC Engines apuX platforms there is no audio device present. Also OHCI is outdated, hence only EHCI is configured.
TEST=boot Linux debian 4.19.0-9 on PC Engines apu4 platform and see if IRQs are correctly assigned to devices analyzing 'cat proc/interrupts' and `lspci -v` outputs.
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I9eadba65eb5f8f66d1d28c89c7c29d0745c93119 --- M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/mptable.c M src/northbridge/amd/pi/00730F01/pci_devs.h 3 files changed, 30 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/42097/4
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG@9 PS3, Line 9: were
I’d use present tense.
Done
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG@10 PS3, Line 10: was
is
Done
https://review.coreboot.org/c/coreboot/+/42097/3//COMMIT_MSG@11 PS3, Line 11:
Please roughly mention, that there is no audio device, and that OHCI is outdated. […]
Mentioned about lack of audio device and OHCI. AFAIK table was copy-pasted and that's why it is not suitable for a particular apu platform.
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/3/src/mainboard/pcengines/apu... PS3, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00,
This should be a separate commit. There are more occurrences. […]
Those changes are introduced in CB:42388
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... PS4, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, One more capital letter to move to CB:42388
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mptable.c:
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... PS4, Line 77: PCI_INT(0x0, 0x12, 0x0, intr_data_ptr[PIRQ_OHCI1]); : PCI_INT(0x0, 0x13, 0x0, intr_data_ptr[PIRQ_OHCI2]); : PCI_INT(0x0, 0x16, 0x0, intr_data_ptr[PIRQ_OHCI3]); : PCI_INT(0x0, 0x10, 0x0, intr_data_ptr[PIRQ_OHCI3]); Why we still use PIRQ_OHCIn?
Hello build bot (Jenkins), Michał Żygowski, Paul Menzel, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42097
to look at the new patch set (#5).
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table
MP Table and IRQ routing tables are buggy, which results in Linux kernel panic when 'acpi=noirq' flag is added during boot.
On PC Engines apuX platforms there is no audio device present. Also OHCI is outdated, hence only EHCI is configured.
TEST=boot Linux debian 4.19.0-9 on PC Engines apu4 platform and see if IRQs are correctly assigned to devices analyzing 'cat proc/interrupts' and `lspci -v` outputs.
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I9eadba65eb5f8f66d1d28c89c7c29d0745c93119 --- M src/mainboard/pcengines/apu2/mainboard.c M src/mainboard/pcengines/apu2/mptable.c M src/northbridge/amd/pi/00730F01/pci_devs.h M src/southbridge/amd/pi/hudson/amd_pci_int_defs.h M src/southbridge/amd/pi/hudson/pci_devs.h 5 files changed, 39 insertions(+), 54 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/42097/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 5:
(16 comments)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00, space required after that ',' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 95: {IOMMU_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 96: {NB_PCIE_PORT1_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* NIC: 02.1 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 97: {NB_PCIE_PORT2_DEVFN, {PIRQ_B, PIRQ_C, PIRQ_D, PIRQ_A}}, /* NIC: 02.2 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 99: {NB_PCIE_PORT4_DEVFN, {PIRQ_D, PIRQ_A, PIRQ_B, PIRQ_C}}, /* NIC: 02.4 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 100: {NB_PCIE_PORT5_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* mPCIe slot 1: 02.5 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 103: {EHCI1_DEVFN, {PIRQ_EHCI1, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI0: 12.0 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 104: {EHCI2_DEVFN, {PIRQ_EHCI2, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI1: 13.0 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 106: {EHCI3_DEVFN, {PIRQ_EHCI3, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 16.0 (same device as xHCI 10.0) */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/42097/5/src/mainboard/pcengines/apu... PS5, Line 106: {EHCI3_DEVFN, {PIRQ_EHCI3, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 16.0 (same device as xHCI 10.0) */ space required after that close brace '}'
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... PS4, Line 73: [0x30] = 0x12,0x1F,0x12,0x1F,0x12,0x1F,0x1F,0x00,
One more capital letter to move to CB:42388
fixed in CB:42388
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mptable.c:
https://review.coreboot.org/c/coreboot/+/42097/4/src/mainboard/pcengines/apu... PS4, Line 77: PCI_INT(0x0, 0x12, 0x0, intr_data_ptr[PIRQ_OHCI1]); : PCI_INT(0x0, 0x13, 0x0, intr_data_ptr[PIRQ_OHCI2]); : PCI_INT(0x0, 0x16, 0x0, intr_data_ptr[PIRQ_OHCI3]); : PCI_INT(0x0, 0x10, 0x0, intr_data_ptr[PIRQ_OHCI3]);
Why we still use PIRQ_OHCIn?
I have deleted any references to PIRQ_OHCIn.
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42097/5/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/amd_pci_int_defs.h:
https://review.coreboot.org/c/coreboot/+/42097/5/src/southbridge/amd/pi/huds... PS5, Line 43: #define PIRQ_OHCI1 0x30 /* USB OHCI 12h.0 */ : #define PIRQ_EHCI1 0x31 /* USB EHCI 12h.2 */ : #define PIRQ_OHCI2 0x32 /* USB OHCI 13h.0 */ : #define PIRQ_EHCI2 0x33 /* USB EHCI 13h.2 */ : #define PIRQ_OHCI3 0x34 /* USB OHCI 16h.0 */ : #define PIRQ_EHCI3 0x35 /* USB EHCI 16h.2 */ : #define PIRQ_OHCI4 0x36 /* USB OHCI 14h.5 */ I'm not quite sure about those changes as it is not board specific. However, if we want to do away with OHCI references it should be done this way, shouldn't it?
https://review.coreboot.org/c/coreboot/+/42097/5/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/42097/5/src/southbridge/amd/pi/huds... PS5, Line 27: /* OHCI */ : #define OHCI1_DEV 0x12 : #define OHCI1_FUNC 0 : #define OHCI2_DEV 0x13 : #define OHCI2_FUNC 0 : #define OHCI3_DEV 0x16 : #define OHCI3_FUNC 0 : #define OHCI4_DEV 0x14 : #define OHCI4_FUNC 5 : #define OHCI_DEVID 0x7807 : #define OHCI1_DEVFN PCI_DEVFN(OHCI1_DEV,OHCI1_FUNC) : #define OHCI2_DEVFN PCI_DEVFN(OHCI2_DEV,OHCI2_FUNC) : #define OHCI3_DEVFN PCI_DEVFN(OHCI3_DEV,OHCI3_FUNC) : #define OHCI4_DEVFN PCI_DEVFN(OHCI4_DEV,OHCI4_FUNC) As I mentioned in the other comment, I'm not sure if it is right to get rid of OHCI at all. However, if we want to refer properly, then at least EHCIn_FUNC must be redefine to 0 and it would collide with OHCIn_FUNC. Is there a better way to avoid this?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42097 )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Patch Set 6:
(9 comments)
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 95: {IOMMU_DEVFN, {PIRQ_A, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 96: {NB_PCIE_PORT1_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* NIC: 02.1 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 97: {NB_PCIE_PORT2_DEVFN, {PIRQ_B, PIRQ_C, PIRQ_D, PIRQ_A}}, /* NIC: 02.2 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 99: {NB_PCIE_PORT4_DEVFN, {PIRQ_D, PIRQ_A, PIRQ_B, PIRQ_C}}, /* NIC: 02.4 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 100: {NB_PCIE_PORT5_DEVFN, {PIRQ_A, PIRQ_B, PIRQ_C, PIRQ_D}}, /* mPCIe slot 1: 02.5 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 103: {EHCI1_DEVFN, {PIRQ_EHCI1, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI0: 12.0 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 104: {EHCI2_DEVFN, {PIRQ_EHCI2, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI1: 13.0 */ space required after that close brace '}'
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 106: {EHCI3_DEVFN, {PIRQ_EHCI3, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 16.0 (same device as xHCI 10.0) */ line over 96 characters
https://review.coreboot.org/c/coreboot/+/42097/6/src/mainboard/pcengines/apu... PS6, Line 106: {EHCI3_DEVFN, {PIRQ_EHCI3, PIRQ_NC, PIRQ_NC, PIRQ_NC}}, /* EHCI2: 16.0 (same device as xHCI 10.0) */ space required after that close brace '}'
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42097?usp=email )
Change subject: mb/pcengines/apu2/mptable.c: fix invalid MP table and IRQ table ......................................................................
Abandoned
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Michał Żygowski (Code Review)
-
Paul Menzel (Code Review)
-
Piotr Kleinschmidt (Code Review)
-
Stefan Reinauer (Code Review)