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 26:
(21 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49408/comment/f8f778d9_8ce63cfe PS26, Line 25: meet the FSPs rules nit: I think most(all but 1?) rules are what the BWG requires.
File src/soc/intel/common/block/include/intelblocks/irq.h:
https://review.coreboot.org/c/coreboot/+/49408/comment/f959b867_6d9ac248 PS26, Line 14: irq_constraint_type Can a single device have multiple constraints? e.g. FIXED_INT_PIN and IDENTITY? or FIXED_INT_PIN and UNIQUE_IRQ?
https://review.coreboot.org/c/coreboot/+/49408/comment/4bc2cf4b_4180c9a3 PS26, Line 26: IDENTITY Haven't read rest of the CL yet, but it is not very clear from the comment above how `IDENTITY` is different than PIRQ?
https://review.coreboot.org/c/coreboot/+/49408/comment/ed5ba410_de6b958d PS26, Line 30: pci_devfn_t Shouldn't this be `unsigned int`?
https://review.coreboot.org/c/coreboot/+/49408/comment/6c3e5e5f_bd814b55 PS26, Line 32: pci_pin Is this set only in case of `FIXED_INT_PIN`?
https://review.coreboot.org/c/coreboot/+/49408/comment/4567e035_b6c642fc PS26, Line 40: pci_devfn_t `unsigned int`?
File src/soc/intel/common/block/irq/irq.c:
https://review.coreboot.org/c/coreboot/+/49408/comment/d6bf0969_39939fab PS26, Line 13: MIN_IRQ Should this be `MIN_SHARED_IRQ` so that it is consistent with other _SHARED_IRQ macros below?
https://review.coreboot.org/c/coreboot/+/49408/comment/072c0bc3_78dc6f64 PS26, Line 17: #define TOTAL_IRQ (MAX_IRQ - MIN_IRQ + 1) : #define MAX_IRQ_ENTRIES 120 These macros are unused. Are these required?
https://review.coreboot.org/c/coreboot/+/49408/comment/5778773e_c027f3fe PS26, Line 21: MAX_PINS Is this the same as PCI_INT_MAX?
https://review.coreboot.org/c/coreboot/+/49408/comment/70fd5960_23ab58d3 PS26, Line 82: > Shouldn't this be ==?
https://review.coreboot.org/c/coreboot/+/49408/comment/5ea881d6_0bc1f976 PS26, Line 91: used Instead of determining the pin state here and in `find_shareable_pin`, do you think if it would make sense to have another structure tracking the pin state:
``` struct pin_info { enum pin_state { FREE_PIN, SHARED_PIN, UNIQUE_IRQ_PIN, } pin_state; int usage_count; bool identity_map; int irq; } pin_info[MAX_PINS]; ```
It is basically just combining the information you are already tracking/using in different places: * used[] * share_count[] * pin_irq_map[]
It helps with a few things: 1. Don't need to calculate pin state (used/free) or usage count every time the function is called. 2. Don't need to track pin_irq_map and MAX_SHARED_IRQ to determine if a pin is being used for unique irq. Makes couple of checks in the `find_shareable_pin()` cleaner. 3. With #2 addressed, I think we can have a clean split of pin allocation followed by irq allocation (probably makes it easier to read?):
``` assign_slot() { assign_pins(); // Assigns pin to each valid function. Generates pin_info and fn_pin_map assign_irqs(); // Assigns IRQs to each pin using information from pin_info add_slot_entries(); // Adds entry for each slot function using fn_pin_map and pin_info } ```
where ``` assign_pins() { assign_fixed_pins(); assign_unique_irq_pins(); assign_shareable_pins(); } ```
I think these functions can also make use of some helper function to set pin_info[] which does the required sanity check.
https://review.coreboot.org/c/coreboot/+/49408/comment/668460ae_1717938c PS26, Line 136: unsigned int share_count[TOTAL_SHARED_IRQ] = {0}; Should this be a global array to avoid having to calculate the share_count every time? IIUC, one of the reasons why you are recalculating the share_count every time is because we haven't yet added entries for the current slot. But, if we don't rely on `entries[]` to determine this and instead add a step in `assign_irqs()` to `increment_shared_count()` then share_count will always have the up-to-date information.
``` assign_irqs() { // If unique, assign unique irq. // If shared: // 1. get least used // 2. call increment_shared_count() } ```
https://review.coreboot.org/c/coreboot/+/49408/comment/39318f2e_b2b8f36e PS26, Line 175: fn_pin_map I am trying to understand what state `fn_pin_map[]` would be in. By default, you set all functions 0 - 7 as `EMPTY_FN` (0xff) which I think means that the function is not present in the slot.
And then in the loop below, if `constraints[i].type` is `UNKNOWN`, then the function is left as `EMPTY_FN`, else it is set to `PCI_INT_NONE`. So, IIUC, what you want after this function is:
* Non-existent functions in the slot ==> Set as EMPTY_FN * Existent functions in the slot ==> Set as PCI_INT_NONE
But, the loop below assumes that all functions exist in the order 0 - 7(in the constraints list) and there are no holes in between. I think that assumption is not really clear at the API level. It might be better to check for the function # in the loop and set `fn_pin_map[i]` accordingly.
``` for (i = 0; i < MAX_FNS; i++) { func = PCI_FUNC(constraints[i].devfn); if (constraints[i].type != UNKNOWN) fn_pin_map[func] = PCI_INT_NONE; ... } ```
BTW, is there any advantage of using `EMPTY_FN`? I don't really see it getting used anywhere outside of this function. Can we simply initialize `fn_pin_map` to `PCI_INT_NONE`? Ideally, there should not be any slots that are all empty in the constraint table when we are in this function. You can use the slot count(as I mentioned on line 342) to ensure that.
https://review.coreboot.org/c/coreboot/+/49408/comment/d065dcfb_8095b529 PS26, Line 198: constraints[i].type == FIXED_INT_PIN || constraints[i].type == IDENTITY I wonder if the constraints type should be a bit-field so that multiple can be set e.g. `FIXED_INT_PIN` and `IDENTITY`. `FIXED_INT_PIN` indicates that a fixed pin is allocated for the slot-function. `IDENTITY` indicates that the pin is mapped 1:1 to PIRQ #. Then, we don't need to make the assumption that IDENTITY implies fixed pin allocation.
Also, do we need to handle cases like a `FIXED_INT_PIN` also requires a `UNIQUE_IRQ`?
This needs some more thought because I understand that having multiple bits set at the same time also requires more paths to handle in `assign_pins()`. Probably, will require bits in the same order as we will process them and use something like `fls()` to properly handle it.
https://review.coreboot.org/c/coreboot/+/49408/comment/50d4c9bf_fca5111d PS26, Line 200: return false; printk(BIOS_ERR, ""); to indicate that the SoC constraint is incorrect.
https://review.coreboot.org/c/coreboot/+/49408/comment/9acd4b58_d5abfefc PS26, Line 202: i Same comment as above. This will have to be PCI_FUNC(...)
https://review.coreboot.org/c/coreboot/+/49408/comment/ade6bd08_3462db89 PS26, Line 208: (pin - PCI_INT_A) PIN2IDX?
https://review.coreboot.org/c/coreboot/+/49408/comment/de73a357_c191c001 PS26, Line 214: fn_pin_map[0] = PCI_INT_A; Should there be a check to ensure that fn_pin_map[0] is not already assigned to something other than PCI_INT_A?
https://review.coreboot.org/c/coreboot/+/49408/comment/020ee4a0_27754dd2 PS26, Line 226: if (constraints[i].type == UNIQUE_IRQ) { If you invert this check, additional tab on rest of the lines can be avoided.
``` if (constraints[i].type != UNIQUE_IRQ) continue; ```
https://review.coreboot.org/c/coreboot/+/49408/comment/0f2fbe6c_ff24cd66 PS26, Line 337: entry_count Should `entry_count` be initialized to -1 by default? If there is a failure in `assign_slot()`, `entry_count` can be set to 0 and we don't have to go through the assignment functions again since it is anyways going to fail.
https://review.coreboot.org/c/coreboot/+/49408/comment/e3c1b533_af3246a0 PS26, Line 342: soc_irq_constraints Since `assign_pci_irqs()` is called by SoC code, can we accept this list as an input parameter rather than making a callback into SoC for getting the IRQ constraints? Also, I think you will need a `count` parameter to indicate the number of slots that the SoC has provided in the constraints list.