Attention is currently required from: Nico Huber, Tim Wawrzynczak. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49408 )
Change subject: soc/intel/common: Add new IRQ module ......................................................................
Patch Set 27:
(18 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49408/comment/bec10b0d_487aecb3 PS27, Line 7: Add new IRQ module As I was reading irq.c file again, I realized that majority of the work that is being done there doesn't really need to happen at runtime for every boot.
Ideally, what can be done is: 1. Create a constraint table for SoC 2. Run it through a tool that emits a .c file with `pci_irq_entry` table. Only the slots that use direct irq route will have to be left empty since the GPIO table is unknown at that time. 3. Integrate this generated .c file into coreboot build. 4. At runtime, only calculate direct irq i.e. only find_free_unique_irq is required.
The logic that you have implemented in irq.c is the important piece. Once that is finalized, it can be reused as is in the tool later on. The current direction in this change is okay. The suggestion for the tool is mostly for a follow up change.
File src/soc/intel/common/block/include/intelblocks/irq.h:
https://review.coreboot.org/c/coreboot/+/49408/comment/a622f7e1_dbcff318 PS27, Line 6: #include <device/device.h> : #include <device/pci_def.h> : #include <device/pci.h> : #include <southbridge/intel/common/acpi_pirq_gen.h> nit: These are most likely not required in this file.
https://review.coreboot.org/c/coreboot/+/49408/comment/5f6b979f_03a6fe0f PS27, Line 16: BIT(0) Empty fields will have a value of 0, right? I am assuming that empty fields basically means that an empty constraint that the SoC did not fill in the constraints table probably because there is no slot/function present.
https://review.coreboot.org/c/coreboot/+/49408/comment/79e5c35a_0be88375 PS27, Line 54: const struct soc_irq_constraints *soc_irq_constraints(void); This is unused and can be dropped.
File src/soc/intel/common/block/irq/Kconfig:
https://review.coreboot.org/c/coreboot/+/49408/comment/b6d58cc3_89f74878 PS27, Line 6: . and exposed to OS in ACPI tables?
File src/soc/intel/common/block/irq/irq.c:
https://review.coreboot.org/c/coreboot/+/49408/comment/a6488584_44e7e714 PS26, Line 198: constraints[i].type == FIXED_INT_PIN || constraints[i].type == IDENTITY
Interesting thought; IDENTITY is currently only applicable for PCIe RPs
True. But, there is nothing that restricts the use of identity for any other slot function. In fact, identity is just a special case of saying that the slot function uses a fixed PIRQ. Also, currently, multiple of the constraint types are already required to be set, but are being implicitly assumed.
Example: In assign_fixed_pins, you have: ``` if (constraints[i].type != FIXED_INT_PIN && constraints[i].type != IDENTITY) continue; ```
Basically, identity assumes a fixed pin as well.
What we really need to know for each slot function is: 1. Does it need a fixed int pin? If yes, which pin? 2. Does it need a fixed pirq? If yes, which pirq? 3. What route does the irq use - none (empty function), pirq, direct irq.
If we encode these at the SoC level, then there is no need to define "unique", "identity", etc. Building up a little more on these 3 questions, I am adding a comment here: https://review.coreboot.org/c/coreboot/+/49408/27/src/soc/intel/common/block...
File src/soc/intel/common/block/irq/irq.c:
https://review.coreboot.org/c/coreboot/+/49408/comment/9460b6f9_35b23697 PS27, Line 4: #include <device/device.h> Is this required?
https://review.coreboot.org/c/coreboot/+/49408/comment/3cac5d05_14624b97 PS27, Line 5: pci pci_type.h?
https://review.coreboot.org/c/coreboot/+/49408/comment/a0fc83cf_a12933d5 PS27, Line 8: #include <soc/irq.h> Is this required?
https://review.coreboot.org/c/coreboot/+/49408/comment/616b751c_b0c7fb87 PS27, Line 22: MAX_ENTRIES Just curious: How was the max value of 64 determined?
https://review.coreboot.org/c/coreboot/+/49408/comment/d223bda4_85f6e916 PS27, Line 27: SHARED_PIN nit: `SHARED_IRQ_PIN` to keep the name consistent with `UNIQUE_IRQ_PIN`?
https://review.coreboot.org/c/coreboot/+/49408/comment/82cce2cf_420b0542 PS27, Line 31: bool identity_map; Based on comment https://review.coreboot.org/c/coreboot/+/49408/27/src/soc/intel/common/block..., this will not be required. Instead, we should maintain a pirq mapping here.
``` enum pirq pirq; ```
It will have to be set to PIRQ_INVALID by default and initialized to appropriate pirq based on: - fixed_pirq, or - pirq assigned as part of find_global_least_used_pirq
https://review.coreboot.org/c/coreboot/+/49408/comment/96ddf3d4_b79a512e PS27, Line 128: const struct pin_info pin_info[PCI_INT_MAX] This is unused now.
https://review.coreboot.org/c/coreboot/+/49408/comment/1e3f538e_c51c8078 PS27, Line 161: const unsigned int pin_idx = PIN2IDX(pin); : const unsigned int fn = PCI_FUNC(constraints[i].devfn); : fn_pin_map[fn] = pin; : pin_info[pin_idx].pin_state = SHARED_PIN; : pin_info[pin_idx].usage_count++; : : if (constraints[i].type == IDENTITY) : pin_info[pin_idx].identity_map = true; This part is actually repeated for all three pin assignment functions. We should create a helper function `assign_pin()` that performs all required checks and assigns pin(passed in from caller).
Something like: 1. Sanity check pin (PCI_INT_A <= pin <= PCI_INT_D) 2. If irq route is PIRQ, reqd pin state is SHARED_IRQ_PIN, else UNIQUE_IRQ_PIN. 3. Check current pin state is okay to be updated to reqd pin state. 4. Update usage count and current pin state.
Then each of the pin assignment functions can call `assign_pin()` to perform the same checks and work.
Similarly, we should add `assign_pirq()` that can be used to perform required checks and assign pirq passed in by the caller (either from `fixed_pirq` in constraints or using `find_global_least_used_pirq()`).
https://review.coreboot.org/c/coreboot/+/49408/comment/faab08a6_b53b9775 PS27, Line 228: assign_fixed_pins Can you please add a comment here that the order in which the pins are assigned matters so that stricter constraints are resolved first? FIXED -> UNIQUE -> SHARED
https://review.coreboot.org/c/coreboot/+/49408/comment/3198a020_0c5fb594 PS27, Line 313: (entry_count < 0) I don't see entry_count being set to < 0 ever. Did you mean to do that on line 324?
https://review.coreboot.org/c/coreboot/+/49408/comment/bf113f26_7c4fdf5a PS27, Line 314: return entries; nit: In case of error (entry_count < 0), entries is being returned here but NULL on line 325. Should this function be consistent about the return value for entries in case of error?
https://review.coreboot.org/c/coreboot/+/49408/comment/207edb77_96ce08bd PS27, Line 319: UNKNOWN UNKNOWN is defined as (1 << 0). However, in the follow up CLs, empty slots are left uninitialized and thus would be set to 0. This check never really satisfies.
Also, do we really need to allocate a table for every slot(0-31)? Would it work if we just encode available slots and encode the constraints for each function (empty or used) for that slot? (This is mostly a nit. If you think having a table for all slots is simpler to understand and manage, I think that's fine too).
Combining this with the comment I made on patchset#26(https://review.coreboot.org/c/coreboot/+/49408/comment/d065dcfb_8095b529/) I am thinking something like this:
``` struct soc_irq_constraints { unsigned int slot; struct { enum pci_pin fixed_int_pin; enum pirq fixed_pirq; enum { IRQ_NONE = 0, // No IRQ assigned -- empty function IRQ_PIRQ = 1, // Use PIRQ routing - will be assigned shared IRQ 16-23 IRQ_DIRECT = 2, // No PIRQ routing - will be assigned unique IRQ > 23 } irq_route; } fns[MAX_FNS]; }; ```
and the table can be something like(copying a few entries from follow up TGL CL):
``` static const struct soc_irq_constraints irq_constraints[] = { { .slot = SA_DEV_SLOT_IGD, .fns = { [PCI_FUNC(SA_DEVFN_IGD)] = { .irq_route = IRQ_PIRQ, }, }, }, { .slot = SA_DEV_SLOT_TBT, .fns = { [PCI_FUNC(SA_DEVFN_TBT0)] = { .fixed_int_pin = PCI_INT_A, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(SA_DEVFN_TBT1)] = { .fixed_int_pin = PCI_INT_B, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(SA_DEVFN_TBT2)] = { .fixed_int_pin = PCI_INT_C, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(SA_DEVFN_TBT3)] = { .fixed_int_pin = PCI_INT_D, .irq_route = IRQ_PIRQ, }, }, }, { .slot = PCH_DEV_SLOT_ISH, .fns = { [PCI_FUNC(PCH_DEVFN_ISH)] = { .irq_route = IRQ_DIRECT, }, [PCI_FUNC(PCH_DEVFN_GSPI2)] = { .irq_route = IRQ_DIRECT, }, }, }, { .slot = PCH_DEV_SLOT_PCIE, .fns = { [PCI_FUNC(PCH_DEVFN_PCIE1)] = { .fixed_int_pin = PCI_INT_A, .fixed_pirq = PIRQ_A, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(PCH_DEVFN_PCIE2)] = { .fixed_int_pin = PCI_INT_B, .fixed_pirq = PIRQ_B, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(PCH_DEVFN_PCIE3)] = { .fixed_int_pin = PCI_INT_C, .fixed_pirq = PIRQ_C, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(PCH_DEVFN_PCIE4)] = { .fixed_int_pin = PCI_INT_D, .fixed_pirq = PIRQ_D, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(PCH_DEVFN_PCIE5)] = { .fixed_int_pin = PCI_INT_A, .fixed_pirq = PIRQ_A, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(PCH_DEVFN_PCIE6)] = { .fixed_int_pin = PCI_INT_B, .fixed_pirq = PIRQ_B, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(PCH_DEVFN_PCIE7)] = { .fixed_int_pin = PCI_INT_C, .fixed_pirq = PIRQ_C, .irq_route = IRQ_PIRQ, }, [PCI_FUNC(PCH_DEVFN_PCIE8)] = { .fixed_int_pin = PCI_INT_D, .fixed_pirq = PIRQ_D, .irq_route = IRQ_PIRQ, }, }, } ```
We can also provide some helper macros to fill the constraint entries, but I am not sure if it makes things easier to read:
``` #define ANY_PIRQ(x) [PCI_FUNC(x)] = { .fixed_int_pin = PCI_INT_NONE, \ .fixed_pirq = PIRQ_INVALID, \ .irq_route = IRQ_PIRQ, } #define FIXED_INT_ANY_PIRQ(x, pn) [PCI_FUNC(x)] = { .fixed_int_pin = pn, \ .fixed_pirq = PIRQ_INVALID, \ .irq_route = IRQ_PIRQ, } #define DIRECT_IRQ(x) [PCI_FUNC(x)] = { .fixed_int_pin = PCI_INT_NONE, \ .fixed_pirq = PIRQ_INVALID, \ .irq_route = IRQ_DIRECT, } #define FIXED_INT_PIRQ(x, pn, pq) [PCI_FUNC(x)] = { .fixed_int_pin = pn, \ .fixed_pirq = pq, \ .irq_route = IRQ_PIRQ, } ```
Above table would look something like:
``` static const struct soc_irq_constraints irq_constraints[] = { { .slot = SA_DEV_SLOT_IGD, .fns = { ANY_PIRQ(SA_DEVFN_IGD), }, }, { .slot = SA_DEV_SLOT_TBT, .fns = { FIXED_INT_ANY_PIRQ(SA_DEVFN_TBT0, PCI_INT_A), FIXED_INT_ANY_PIRQ(SA_DEVFN_TBT1, PCI_INT_B), FIXED_INT_ANY_PIRQ(SA_DEVFN_TBT2, PCI_INT_C), FIXED_INT_ANY_PIRQ(SA_DEVFN_TBT3, PCI_INT_D), }, { .slot = PCH_DEV_SLOT_ISH, .fns = { DIRECT_IRQ(PCH_DEVFN_ISH), DIRECT_IRQ(PCH_DEVFN_GSPI2), }, }, { .slot = PCH_DEV_SLOT_PCIE, .fns = { FIXED_INT_PIRQ(PCH_DEVFN_PCIE1, PCI_INT_A, PIRQ_A), FIXED_INT_PIRQ(PCH_DEVFN_PCIE2, PCI_INT_B, PIRQ_B), FIXED_INT_PIRQ(PCH_DEVFN_PCIE3, PCI_INT_C, PIRQ_C), FIXED_INT_PIRQ(PCH_DEVFN_PCIE4, PCI_INT_D, PIRQ_D), FIXED_INT_PIRQ(PCH_DEVFN_PCIE5, PCI_INT_A, PIRQ_A), FIXED_INT_PIRQ(PCH_DEVFN_PCIE6, PCI_INT_B, PIRQ_B), FIXED_INT_PIRQ(PCH_DEVFN_PCIE7, PCI_INT_C, PIRQ_C), FIXED_INT_PIRQ(PCH_DEVFN_PCIE8, PCI_INT_D, PIRQ_D), }, } ```
The benefit of having macros for irq constraints is if in the future there is a need to support more fields in constraints or make any changes to current fields, it can be done within the macros without impacting SoC code.
e.g. ``` #define FIXED_INT_DIRECT_IRQ(x, p) { .fixed_int_pin = p, \ .fixed_pirq = PIRQ_INVALID, \ .irq_route = IRQ_DIRECT, } ```
It also reduces chances of mistakes in SoC code since the individual constraint fields are not initialized directly.