Mike Banon 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:
(16 comments)
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. […]
Ack, will split into a separate change and address the other Acks as well (meanwhile please answer a few questions)
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?
Ack, will do.
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. […]
Removed at CB:47913
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 schemati […]
G505S, being a laptop, does not have these PCI slots. So there couldn't be any devices behind this PCI Bridge. Let's remove this routing at CB:47913 ?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 7: IOMMU: 0:02.00
This is wrong.
Do you think there are no reasons for giving the IRQ to IOMMU device?
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 13: IRQ 3
These things are whatever is programmed in INTx# so it's not very useful information.
Ack
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 54: Bus 0, Dev 20
Put this in order?
Ack
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 66: IRQ 3
We are in APIC mode now ;-)
Should I remove this "IOMMU routing"?
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.
Ack
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?
That's what getpir utility told me (0x1022 0x780b is 00:14.0 SMBus [0c05]: Advanced Micro Devices, Inc. [AMD] FCH SMBus Controller [1022:780b] (rev 16)) . Meanwhile, a previously used " 0x1002 0x4384 device doesn't exist on this system at all 😋
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.
This dynamic stuff never worked properly and seems to have a lot of dead code.
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?
This looks like a code borrowed from desktops, because this laptop doesn't have any PCI slots (except one for WiFi card, but it's hooked differently - to a 3:00.00 behind a 0:05.00)
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). […]
Ack, will be removed.
https://review.coreboot.org/c/coreboot/+/47852/4/src/mainboard/lenovo/g505s/... PS4, Line 76: /* PIC IRQ routine */ : for (byte = 0x0; byte < sizeof(picr_data_ptr); byte++) { : outb(byte, 0xC00); : outb(mainboard_picr_data[byte], 0xC01); : } : : /* APIC IRQ routine */ : for (byte = 0x0; byte < sizeof(intr_data_ptr); byte++) { : outb(byte | 0x80, 0xC00); : outb(mainboard_intr_data[byte], 0xC01); : }
It looks like this is also done in the southbridge code, so it could be fully removed from the mainb […]
Ack, will be removed.
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 u […]
Ack, will try using this code from ./src/southbridge/intel/common/acpi_pirq_gen.c: ... pci_dev = PCI_SLOT(dev->path.pci.devfn); int_pin = pci_read_config8(dev, PCI_INTERRUPT_PIN); ...
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.
Ack