Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47852 )
Change subject: lenovo/g505s: properly program the IRQ tables ......................................................................
Patch Set 4:
(13 comments)
All this ACPI, hardware config, mptable and pirqtable synchronisation is just messy. With Intel we solved this by generating most at runtime: see southbridge/intel/common/acpi_pirq_data.c . That solution is quite general and could easily be ported to AMD.
https://review.coreboot.org/c/coreboot/+/47852/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47852/4//COMMIT_MSG@19 PS4, Line 19: As a part of this change, I enabled a 14.5 FCH USB OHCI Controller : in a devicetree.cb, hoping to get a webcam working (it still doesn't). : This doesn't bring any downsides and is reflected at the IRQ tables, : so should be merged simultaneously. CL should be one logical change. If it does not improve anything, adapt the IRQ tables to not hold this?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... File src/mainboard/lenovo/g505s/Kconfig:
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 14: HAVE_PIRQ_TABLE can you do this and fixing DSDT _PRT in a separate CL to make it easier to review?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... File src/mainboard/lenovo/g505s/acpi/routing.asl:
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 239: Name(PE0, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 0*/ : Package(){0x0000FFFF, 0, INTA, 0 }, : Package(){0x0000FFFF, 1, INTB, 0 }, : Package(){0x0000FFFF, 2, INTC, 0 }, : Package(){0x0000FFFF, 3, INTD, 0 }, : }) : Name(APE0, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 0*/ : Package(){0x0000FFFF, 0, 0, 16 }, : Package(){0x0000FFFF, 1, 0, 17 }, : Package(){0x0000FFFF, 2, 0, 18 }, : Package(){0x0000FFFF, 3, 0, 19 }, : }) : : Name(PE1, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 1*/ : Package(){0x0000FFFF, 0, INTB, 0 }, : Package(){0x0000FFFF, 1, INTC, 0 }, : Package(){0x0000FFFF, 2, INTD, 0 }, : Package(){0x0000FFFF, 3, INTA, 0 }, : }) : Name(APE1, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 1*/ : Package(){0x0000FFFF, 0, 0, 17 }, : Package(){0x0000FFFF, 1, 0, 18 }, : Package(){0x0000FFFF, 2, 0, 19 }, : Package(){0x0000FFFF, 3, 0, 16 }, : }) : : Name(PE2, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 2*/ : Package(){0x0000FFFF, 0, INTC, 0 }, : Package(){0x0000FFFF, 1, INTD, 0 }, : Package(){0x0000FFFF, 2, INTA, 0 }, : Package(){0x0000FFFF, 3, INTB, 0 }, : }) : Name(APE2, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 2*/ : Package(){0x0000FFFF, 0, 0, 18 }, : Package(){0x0000FFFF, 1, 0, 19 }, : Package(){0x0000FFFF, 2, 0, 16 }, : Package(){0x0000FFFF, 3, 0, 17 }, : }) : : Name(PE3, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 3 */ : Package(){0x0000FFFF, 0, INTD, 0 }, : Package(){0x0000FFFF, 1, INTA, 0 }, : Package(){0x0000FFFF, 2, INTB, 0 }, : Package(){0x0000FFFF, 3, INTC, 0 }, : }) : Name(APE3, Package(){ : /* PCIe slot - Hooked to PCIe Bridge 3*/ : Package(){0x0000FFFF, 0, 0, 19 }, : Package(){0x0000FFFF, 1, 0, 16 }, : Package(){0x0000FFFF, 2, 0, 17 }, : Package(){0x0000FFFF, 3, 0, 18 }, : }) These seem to be used for devices behind the PCH PCIe bridges. It looks like this is just a bad copy from FAM14 hardware as this is not hooked up anywhere. So it would make sense to remove those in a different CL.
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 299: /* SB PCI Bridge J21, J22 */ : Name(PCIB, Package(){ : /* PCI slots: slot 0, slot 1, slot 2 behind Dev14, Fun4. */ : Package(){0x0005FFFF, 0, 0, 0x14 }, : Package(){0x0005FFFF, 1, 0, 0x15 }, : Package(){0x0005FFFF, 2, 0, 0x16 }, : Package(){0x0005FFFF, 3, 0, 0x17 }, : : Package(){0x0006FFFF, 0, 0, 0x15 }, : Package(){0x0006FFFF, 1, 0, 0x16 }, : Package(){0x0006FFFF, 2, 0, 0x17 }, : Package(){0x0006FFFF, 3, 0, 0x14 }, Why is this removed? PCI PIRQ routing is typically hardwired so you have to look that up in schematics or in the vendor DSDT.
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 54: Bus 0, Dev 20 Put this in order?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 123: 1:00.00 This is dynamically allocated so please remove.
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... File src/mainboard/lenovo/g505s/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 12: 0x1022, /* Vendor */ : 0x780b, changed?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 60: copy_pirq_routing_table Going from something dynamically generated to something static seems like a downgrade.
https://review.coreboot.org/c/coreboot/+/47852/2/src/mainboard/lenovo/g505s/... File src/mainboard/lenovo/g505s/mptable.h:
https://review.coreboot.org/c/coreboot/+/47852/2/src/mainboard/lenovo/g505s/... PS2, Line 17: static const u8 mainboard_picr_data[FCH_INT_TABLE_SIZE] = { : /* INTA# - INTH# */ : [0x00] = 0x03, 0x04, 0x05, 0x07, 0x1F, 0x1F, 0x1F, 0x1F, : /* Misc-nil,0,1,2, INTA-INTD from Serial irq */ : [0x08] = 0xAA, 0xF1, 0x00, 0x00, 0x1F, 0x1F, 0x1F, 0x1F, : /* SCI, SMBUS0, ASF, HDA, SD, GEC, PerMon */ : [0x10] = 0x09, 0x1F, 0x1F, 0x03, 0x1F, 0x1F, 0x1F, : /* IMC INT0-INT5 */ : [0x20] = 0x1F, 0x1F, 0x1F, 0x1F, 0x1F, 0x1F, : /* USB Devs: 18 INTA#,B#; 19 INTA#,B#; 22 INTA#,B#; 20 INTC# */ : [0x30] = 0x05, 0x04, 0x05, 0x04, 0x05, 0x04, 0x05, : /* IDE, SATA */ : [0x40] = 0x1F, 0x07, : /* GPP Int0-Int3 */ : [0x50] = 0x1F, 0x1F, 0x1F, 0x1F : }; : : static const u8 mainboard_intr_data[FCH_INT_TABLE_SIZE] = { : /* INTA# - INTH# */ : [0x00] = 0x10, 0x11, 0x12, 0x13, 0x1F, 0x1F, 0x1F, 0x1F, : /* Misc-nil,0,1,2, INTA-INTD from Serial irq */ : [0x08] = 0x00, 0x00, 0x00, 0x00, 0x1F, 0x1F, 0x1F, 0x1F, : /* SCI, SMBUS0, ASF, HDA, SD, GEC, PerMon */ : [0x10] = 0x09, 0x1F, 0x1F, 0x10, 0x1F, 0x1F, 0x1F, : /* IMC INT0-INT5 */ : [0x20] = 0x1F, 0x1F, 0x1F, 0x1F, 0x1F, 0x1F, : /* USB Devs: 18 INTA#,B#; 19 INTA#,B#; 22 INTA#,B#; 20 INTC# */ : [0x30] = 0x12, 0x11, 0x12, 0x11, 0x12, 0x11, 0x12, : /* IDE, SATA */ : [0x40] = 0x1F, 0x13, : /* GPP Int0-Int3 */ : [0x50] = 0x1F, 0x1F, 0x1F, 0x1F : };
Although it could've been transformed into a bunch of defines, I kept this old format for the easine […]
This will result in 2 copies of this in the final binary, which is inefficient and confusing. If you need it more than once, define it in one *.c file and use 'extern' in the next one. You can put the extern definition in a header if you like.
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... File src/mainboard/lenovo/g505s/mptable.c:
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 129: /* PCI slots */ : struct device *dev = pcidev_on_root(0x14, 4); : if (dev && dev->enabled) { : u8 bus_pci = dev->link_list->secondary; : /* PCI_SLOT 0. */ : PCI_INT(bus_pci, 0x5, 0x0, 0x14); : PCI_INT(bus_pci, 0x5, 0x1, 0x15); : PCI_INT(bus_pci, 0x5, 0x2, 0x16); : PCI_INT(bus_pci, 0x5, 0x3, 0x17); : : /* PCI_SLOT 1. */ : PCI_INT(bus_pci, 0x6, 0x0, 0x15); : PCI_INT(bus_pci, 0x6, 0x1, 0x16); : PCI_INT(bus_pci, 0x6, 0x2, 0x17); : PCI_INT(bus_pci, 0x6, 0x3, 0x14); : : /* PCI_SLOT 2. */ : PCI_INT(bus_pci, 0x7, 0x0, 0x16); : PCI_INT(bus_pci, 0x7, 0x1, 0x17); : PCI_INT(bus_pci, 0x7, 0x2, 0x14); : PCI_INT(bus_pci, 0x7, 0x3, 0x15); : } Where are those?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 77: picr_data_ptr The sizeof(u8 *) is 4 (or 8 on 64bit). This likely not what you want?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 101: /* IOMMU: 0:02.00 - IRQ 3 */ : PCI_INT(0x0, 0x00, 0x0, intr_data_ptr[PIRQ_A]); : PCI_INT(0x0, 0x00, 0x1, intr_data_ptr[PIRQ_B]); : PCI_INT(0x0, 0x00, 0x2, intr_data_ptr[PIRQ_C]); : PCI_INT(0x0, 0x00, 0x3, intr_data_ptr[PIRQ_D]); : : /* APU Integrated Graphics: 0:01.00 - IRQ 3 */ : PCI_INT(0x0, 0x01, 0x0, intr_data_ptr[PIRQ_A]); : /* APU HDMI Audio Controller: 0:01.01 - IRQ 4 */ : PCI_INT(0x0, 0x01, 0x1, intr_data_ptr[PIRQ_B]); : : /* PCIe GPP to dGPU 1:00.00: 0:02.00 - IRQ 3 */ : PCI_INT(0x0, 0x02, 0x0, intr_data_ptr[PIRQ_A]); : PCI_INT(0x0, 0x02, 0x1, intr_data_ptr[PIRQ_B]); : PCI_INT(0x0, 0x02, 0x2, intr_data_ptr[PIRQ_C]); : PCI_INT(0x0, 0x02, 0x3, intr_data_ptr[PIRQ_D]); : /* PCIe GPP to Eth 2:00.00: 0:04.00 - IRQ 3 */ : PCI_INT(0x0, 0x04, 0x0, intr_data_ptr[PIRQ_A]); : PCI_INT(0x0, 0x04, 0x1, intr_data_ptr[PIRQ_B]); : PCI_INT(0x0, 0x04, 0x2, intr_data_ptr[PIRQ_C]); : PCI_INT(0x0, 0x04, 0x3, intr_data_ptr[PIRQ_D]); : /* PCIe GPP to WiFi 3:00.00: 0:05.00 - IRQ 4 */ : PCI_INT(0x0, 0x05, 0x0, intr_data_ptr[PIRQ_B]); : PCI_INT(0x0, 0x05, 0x1, intr_data_ptr[PIRQ_C]); : PCI_INT(0x0, 0x05, 0x2, intr_data_ptr[PIRQ_D]); : PCI_INT(0x0, 0x05, 0x3, intr_data_ptr[PIRQ_A]); : : /* USB XHCI: 0:10.00 - IRQ 5 */ : PCI_INT(0x0, 0x10, 0x0, intr_data_ptr[PIRQ_C]); : /* SATA: 0:11.00 - IRQ 7 */ : PCI_INT(0x0, 0x11, 0x0, intr_data_ptr[PIRQ_SATA]); : /* USB OHCI1: 0:12.00 - IRQ 5 */ : PCI_INT(0x0, 0x12, 0x0, intr_data_ptr[PIRQ_OHCI1]); : /* USB EHCI1: 0:12.02 - IRQ 4 */ : PCI_INT(0x0, 0x12, 0x2, intr_data_ptr[PIRQ_EHCI1]); : /* USB OHCI2: 0:13.00 - IRQ 5 */ : PCI_INT(0x0, 0x13, 0x0, intr_data_ptr[PIRQ_OHCI2]); : /* USB EHCI2: 0:13.02 - IRQ 4 */ : PCI_INT(0x0, 0x13, 0x2, intr_data_ptr[PIRQ_EHCI2]); : /* USB OHCI3: 0:16.00 - IRQ 5 */ : PCI_INT(0x0, 0x16, 0x0, intr_data_ptr[PIRQ_OHCI3]); : /* USB EHCI3: 0:16.02 - IRQ 4 */ : PCI_INT(0x0, 0x16, 0x2, intr_data_ptr[PIRQ_EHCI3]); : /* Southbridge HD Audio: 0:14.02 - IRQ 3 */ : PCI_INT(0x0, 0x14, 0x2, intr_data_ptr[PIRQ_HDA]); : /* USB OHCI4: 0:14.05 - IRQ 5 */ : PCI_INT(0x0, 0x14, 0x5, intr_data_ptr[PIRQ_C]); You can just loop over the discovered PCI devices, find out which INT they are using and look that up in mainboard_intr_data[].
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 150: 0x1 This is dynamically allocated so you want to look at ->link_list->secondary of the PCIe devices.