Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50857 )
Change subject: sb/intel/common: Refactor _PRT generation to support GSI-based tables ......................................................................
Patch Set 7: Code-Review+1
(5 comments)
File src/southbridge/intel/common/acpi_pirq_gen.h:
https://review.coreboot.org/c/coreboot/+/50857/comment/d2235053_80a15281 PS7, Line 55: [32][4] Do we have names for the `32` and `4` magic numbers?
File src/southbridge/intel/common/acpi_pirq_gen.c:
https://review.coreboot.org/c/coreboot/+/50857/comment/d782558c_4afcd9fb PS7, Line 45: pirq_map->gsi[pirq - PIRQ_A]); nit: Even though braces aren't strictly necessary for either branch of this conditional block, I'd still add them since the statements are broken into multiple lines.
https://review.coreboot.org/c/coreboot/+/50857/comment/e47ac628_1cf7ff4c PS7, Line 75: /* pop scope */ Huh?
https://review.coreboot.org/c/coreboot/+/50857/comment/7a0c20e8_9ced3893 PS7, Line 85: char matrix[32][4]; Since `intel_create_pirq_matrix()` may not always initialise all elements, I'd add an explicit zero initialiser to be safe.
File src/southbridge/intel/common/rcba_pirq.c:
https://review.coreboot.org/c/coreboot/+/50857/comment/1d35bb5e_cc480cce PS7, Line 57: int_pin - PCI_INT_A This appears four times in the loop body. Maybe:
const u8 pci_dev = PCI_SLOT(dev->path.pci.devfn); const u8 int_pin = pci_read_config8(dev, PCI_INTERRUPT_PIN);
if (int_pin < PCI_INT_A || int_pin > PCI_INT_D) continue;
const u8 int_idx = int_pin - PCI_INT_A;
if (matrix[pci_dev][int_idx] != PIRQ_NONE) continue;
matrix[pci_dev][int_idx] = map_pirq(dev, int_pin); printk(BIOS_SPEW, "ACPI_PIRQ_GEN: %s: pin=%d pirq=%d\n", dev_path(dev), int_idx, matrix[pci_dev][int_idx] - PIRQ_A); num_devs++;
Thoughts?