Attention is currently required from: Tim Wawrzynczak, Angel Pons, Patrick Rudolph. Furquan Shaikh 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 9:
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/50857/comment/01731f58_7ffd37f0 PS9, Line 10: insteade instead
File src/southbridge/intel/common/acpi_pirq_gen.h:
https://review.coreboot.org/c/coreboot/+/50857/comment/289d641a_c51d486f PS9, Line 17: PCI_INT_D, nit: PCI_INT_MAX = PCI_INT_D instead of a separate MAX_PINS above? That way it is also clear that this is related to `enum pci_pin`.
https://review.coreboot.org/c/coreboot/+/50857/comment/ed49bd47_8f062f8f PS9, Line 29: PIRQ_H, nit: PIRQ_MAX = PIRQ_H + 1 instead of a separate MAX_PIRQS above?
the + 1 is to make sure that the caller can create a mapping table using:
``` { [PIRQ_A] = ..., [PIRQ_B] = ..., ... [PIRQ_H] = ..., }; ```
Honestly, I don't really like the +1. It is primarily because of PIRQ_NONE. It looks like PIRQ_NONE is used only within src/southbridge/intel/common/. We can use this opportunity to get rid of PIRQ_NONE and avoid the +1. See my comment on acpi_pirq_gen.c about using a list of only the devs that need to emit IRQ information in APIC and PIC mode instead of passing in the entire sparse matrix. That will get rid of the use of PIRQ_NONE in acpi_pirq_gen.c and hence its callers as well.
https://review.coreboot.org/c/coreboot/+/50857/comment/df2c6888_4575dd68 PS9, Line 32: slot_pin_irq_map I think it would be helpful to have a comment block here that explains what information is expected to be provided. Also see my comment in acpi_pirq_gen.c. We should really accept information for APIC in the form of GSI # and PIC in the form of PIRQ #.
https://review.coreboot.org/c/coreboot/+/50857/comment/2e9aac50_cac70f12 PS9, Line 42: pirq_map I think this should be named `pic_pirq_map` since it should really be used only for PIC mode. See my comment in acpi_pirq_gen.c file.
https://review.coreboot.org/c/coreboot/+/50857/comment/90edd7c3_a47e340f PS9, Line 42: struct pirq_map { : enum pirq_map_type type; : unsigned int gsi[MAX_PIRQS]; : char source_path[MAX_PIRQS][DEVICE_PATH_MAX]; : }; ... and a comment block here explaining how the PIRQ can be routed statically to a GSI or dynamically using a source path.
File src/southbridge/intel/common/acpi_pirq_gen.c:
https://review.coreboot.org/c/coreboot/+/50857/comment/6f125524_52e63d47 PS9, Line 17: gen_irq_route Should we split this function into two - one for APIC and other for PICM? I know it will duplicate the loop. But I think it will make the code more readable.
https://review.coreboot.org/c/coreboot/+/50857/comment/76188e8f_27126ffc PS9, Line 29: EMIT_APIC Sorry about the back and forth on this again. When I look at the code here, we are encoding some assumptions within this function which doesn't seem like the right place to do it:
We are accepting GSI # and PIRQ # from the caller. If the caller says that it uses dynamic mapping for PIRQ in PIC mode (this is for older generation Intel platforms), then we calculate GSI # using 16 + pirq - PIRQ_A for it in APIC mode. It is really just applicable to the older generation Intel platforms.
I think we need to basically accept GSI # to be used in APIC mode and PIRQ # to be used in PIC mode from the caller.
``` struct slot_pin_irq_map { int apic_gsi; // GSI # used in APIC mode int pic_pirq; // PIRQ # used in PIC mode } ```
The caller will be expected to fill in GSI # for APIC mode and PIRQ # for PIC mode. This will allow the functions to be something like:
``` static void gen_irq_apic_routes(const struct slot_pin_irq_map pin_irq_map[MAX_SLOTS][MAX_PINS]) { unsigned int slot, pin, gsi;
for (slot = 0; slot < MAX_SLOTS; slot++) { for (pin = 0; pin < MAX_PINS; pin++) { gsi = pin_irq_map[slot][pin].apic_gsi; if (gsi == 0) continue;
acpigen_write_PRT_GSI_entry(slot, pin, gsi); } } }
static void gen_irq_pic_routes(const struct slot_pin_irq_map pin_irq_map[MAX_SLOTS][MAX_PINS], const struct pirq_map *pirq_map) { unsigned int slot, pin; enum pirq pirq;
for (slot = 0; slot < MAX_SLOTS; slot++) { for (pin = 0; pin < MAX_PINS; pin++) { pirq = pin_irq_map[slot][pin].pic_pirq; if (pirq == PIRQ_NONE) continue; if (pirq_map->type == PIRQ_GSI) { acpigen_write_PRT_GSI_entry(slot, pin, pirq_map[pirq].gsi); } else { acpigen_write_PRT_source_entry(slot, pin, pirq_map[pirq].source_path); } } } } ```
This will require `intel_acpi_gen_def_acpi_pirq()` to do little more work, but I think that will keep these helpers free of any platform-generation specific information.
https://review.coreboot.org/c/coreboot/+/50857/comment/35ae825c_241cd9ef PS9, Line 76: intel_acpi_gen_def_acpi_pirq I think this function can be moved to rcba_pirq.c and probably renamed to `intel_rcba_gen_acpi_pirq()`. This doesn't really need to live in this common file.
https://review.coreboot.org/c/coreboot/+/50857/comment/7eee75b8_9b9be12e PS9, Line 78: pin_irq_map Just a thought: This is a sparse matrix. I don't really think space is a problem, but we don't have to scan it entirely multiple times i.e. for filling and for generating PIC and APIC entries. It shouldn't really add a lot of boot time, but seems unnecessary. We can instead maintain a map which holds slot and pin in addition to apic_gsi and pic_pirq:
``` struct slot_pin_irq_map { int slot; int pin; int apic_gsi; int pic_pirq; }; ```
It can still be allocated space statically using: ``` static slot_pin_irq_map pin_irq_map[MAX_SLOTS * MAX_PINS]; ```
or dynamically using malloc. Anyways, I don't think it is super critical, but since we are refactoring the entire code, I added this comment as well. It is also helpful in getting rid of the `PIRQ_NONE`.
https://review.coreboot.org/c/coreboot/+/50857/comment/cce1b0fa_8a90451f PS9, Line 78: struct static
https://review.coreboot.org/c/coreboot/+/50857/comment/be0c0b2d_77795859 PS9, Line 98: intel_create_pirq_matrix I think we can pull the content of `intel_create_pirq_matrix()` into this function. There is no need to loop through the slots two times.
``` for (dev = pcidev_on_root(0, 0); dev; dev = dev->sibling) { ...
pirq = map_pirq(dev, int_pin); if (pirq == PIRQ_NONE) continue;
pin_irq_map[pci_dev][int_idx].apic_gsi = 16 + (pirq - PIRQ_A); pin_irq_map[pci_dev][int_idx].pic_pirq = pirq; } ```