Attention is currently required from: Nico Huber, Furquan Shaikh. Tim Wawrzynczak 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:
(19 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49408/comment/3d693774_48d6ec70 PS26, Line 25: meet the FSPs rules
nit: I think most(all but 1?) rules are what the BWG requires.
Done.
File src/soc/intel/common/block/include/intelblocks/irq.h:
https://review.coreboot.org/c/coreboot/+/49408/comment/9f5dadad_e36d8b9a PS26, Line 14: irq_constraint_type
Can a single device have multiple constraints? e.g. […]
Right now no, I have not come across a need for that (glanced at a few generations)
https://review.coreboot.org/c/coreboot/+/49408/comment/36b83d03_c4b4a3c3 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 d […]
Sure, I'll use my words instead of the BGW 😊
https://review.coreboot.org/c/coreboot/+/49408/comment/38cd9c9b_d2910afe PS26, Line 30: pci_devfn_t
Shouldn't this be `unsigned int`?
Done
https://review.coreboot.org/c/coreboot/+/49408/comment/ba99fe9b_ec485255 PS26, Line 32: pci_pin
Is this set only in case of `FIXED_INT_PIN`?
Yes, I will rename it to make that more clear.
https://review.coreboot.org/c/coreboot/+/49408/comment/d5daab60_ec47e163 PS26, Line 40: pci_devfn_t
`unsigned int`?
Done
File src/soc/intel/common/block/irq/irq.c:
https://review.coreboot.org/c/coreboot/+/49408/comment/8e3ed9a2_53d115ec PS26, Line 13: MIN_IRQ
Should this be `MIN_SHARED_IRQ` so that it is consistent with other _SHARED_IRQ macros below?
Done
https://review.coreboot.org/c/coreboot/+/49408/comment/3014711b_864588b3 PS26, Line 17: #define TOTAL_IRQ (MAX_IRQ - MIN_IRQ + 1) : #define MAX_IRQ_ENTRIES 120
These macros are unused. […]
From an older patchset, removed.
https://review.coreboot.org/c/coreboot/+/49408/comment/a77e37e3_77340e50 PS26, Line 21: MAX_PINS
Is this the same as PCI_INT_MAX?
Yes, with PCI_INT_MAX now defined, will use that 😊
https://review.coreboot.org/c/coreboot/+/49408/comment/06f30125_b37a8f4b PS26, Line 82: >
Shouldn't this be ==?
MAX_IRQ is 119 here... next patchset redefines it to 120 and then I think the logic here is a little more clear
https://review.coreboot.org/c/coreboot/+/49408/comment/b4a97460_5dfe54cf PS26, Line 91: used
Instead of determining the pin state here and in `find_shareable_pin`, do you think if it would make […]
Nice use of a better data structure to make this cleaner! My original choice was obviously the fn_pin_map and the pin_irq_map (linked by the pins) but I have a good feeling about this change 👍
https://review.coreboot.org/c/coreboot/+/49408/comment/02e42020_4ccc2754 PS26, Line 136: unsigned int share_count[TOTAL_SHARED_IRQ] = {0};
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.
That's correct.
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.
SGTM
https://review.coreboot.org/c/coreboot/+/49408/comment/15f75b26_3f49bdec PS26, Line 175: fn_pin_map
I am trying to understand what state `fn_pin_map[]` would be in. […]
Let me see if I can get rid of the EMPTY_FN ...I think it's a relic from a previous patchset (a sentinel value that would stick out) that I didn't see effectively get removed 😊
also yes there was an assumption about the order there... will fix.
https://review.coreboot.org/c/coreboot/+/49408/comment/57916951_007669aa 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.
Interesting thought; IDENTITY is currently only applicable for PCIe RPs
Also, do we need to handle cases like a `FIXED_INT_PIN` also requires a `UNIQUE_IRQ`?
I haven't come across one yet, but who knows what may come in the future... let me see how that looks.
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.
Indeed, there is a "precedence" that must be handled correctly between different constraint types to make sure the logic works out, so it would complicate things a little bit.
https://review.coreboot.org/c/coreboot/+/49408/comment/f0c70c5a_85734eea PS26, Line 200: return false;
printk(BIOS_ERR, ""); to indicate that the SoC constraint is incorrect.
Done
https://review.coreboot.org/c/coreboot/+/49408/comment/feec364d_843cbdb0 PS26, Line 202: i
Same comment as above. This will have to be PCI_FUNC(... […]
Done
https://review.coreboot.org/c/coreboot/+/49408/comment/e4856b1c_d10a3f96 PS26, Line 208: (pin - PCI_INT_A)
PIN2IDX?
Done
https://review.coreboot.org/c/coreboot/+/49408/comment/ff925b4a_366d20e4 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 […]
I guess then there would be a conflict between a FIXED_INT_PIN (or IDENTITY) and a single-function device requirement... I'll add the check for now.
https://review.coreboot.org/c/coreboot/+/49408/comment/cb2b936a_51f9a781 PS26, Line 226: if (constraints[i].type == UNIQUE_IRQ) {
If you invert this check, additional tab on rest of the lines can be avoided. […]
Done
-
Tim Wawrzynczak (Code Review)