Attention is currently required from: Nico Huber. 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 7:
(12 comments)
File src/soc/intel/common/block/irq/irq.c:
https://review.coreboot.org/c/coreboot/+/49408/comment/04b8d4f5_3163e76c PS6, Line 109: (enum pci_pin)
Should this be part of the macro?
Good call, yes
https://review.coreboot.org/c/coreboot/+/49408/comment/bb94c167_64ba1b0c PS6, Line 117: used
`used_count` might catch it better. […]
share_count works
https://review.coreboot.org/c/coreboot/+/49408/comment/5cb3a873_9d4ec5e7 PS6, Line 126: * assigned a PIRQ eventually.
This makes the function a find_shareable_pin()?
I like it
https://review.coreboot.org/c/coreboot/+/49408/comment/c44dcbff_c0b3bcfa PS6, Line 128: PIN2IDX(pin)
Naa, this smells. It's `fn_pin_map[MAX_FNS]`, i.e. not indexed by pin. […]
Oops, good catch! This is indeed supposed to be pin_irq_map
https://review.coreboot.org/c/coreboot/+/49408/comment/639d2794_46646cff PS6, Line 129: i
`i < MAX_FNS` but we have `used[MAX_PINS]`. I assume `PIN2IDX(pin)`.
That is more correct, will fix.
NB. Did I ever ask you how much you cling to C? There's a nice language hooked up to our build system that supports actual type checking. Especially when it comes to index ranges, it's awesome :D
I love the idea of Ada, I have never taken the time to actually sit down and read some code and learn to write it... Agreed, Ada would make this much cleaner, and those errors couldn't happen then 😊 I'll dig around for some resources to learn it, feel free to send any good ones you have my way
https://review.coreboot.org/c/coreboot/+/49408/comment/e8affc36_b30adc8a PS6, Line 189: constraints[i].pci_pin
Maybe it's worth to check if it's valid. If not, we should use […]
Sure, I can add that, can return failure on that condition as well.
https://review.coreboot.org/c/coreboot/+/49408/comment/1e2fa25c_e1d591f0 PS6, Line 192: pin != PCI_INT_NONE
Why not use is_valid_pin()?
No reason, good catch 😊
https://review.coreboot.org/c/coreboot/+/49408/comment/d2118ec6_d386e9ef PS6, Line 193: fn_pin_map[i] = pin;
So PCI_INTERRUPT_PIN != PCI_INT_NONE is treated as fixed pin? […]
Basically, yes.
According to the EDS, some devices have INTPIN an RO field fixed at 01h... and even reading it out, I find a few devices that do have this set already: 00:0D.2 (pin A) 00:12.1 (pin A) 00:13.0 (pin D)
https://review.coreboot.org/c/coreboot/+/49408/comment/b59e62d9_d2b0f809 PS6, Line 207: fn_pin_map[i] == PCI_INT_NONE
!is_valid_pin()?
Done
https://review.coreboot.org/c/coreboot/+/49408/comment/fe0c287f_ac9defe0 PS6, Line 261: if (!pin_irq_map[PIN2IDX(pin)]) {
Shouldn't we check if the pin is used at all?
This does produce extra, unnecessary assignments, I can just check for that.
https://review.coreboot.org/c/coreboot/+/49408/comment/ef87f401_6d042a44 PS6, Line 262: int irq = first_unused_pirq(pin_irq_map);
This has a very local view on what is "unused". […]
That is accurate, this is a slot-local view of what is used... in practice, no it won't go above PIRQ D... good thought, let me try something...
https://review.coreboot.org/c/coreboot/+/49408/comment/6f3b0ac1_f715997d PS6, Line 352: __weak const struct soc_irq_constraints *soc_irq_constraints(void)
Is this really needed? iow. should this file be compiled without […]
Good point, this could just mask someone mistyping the name of the function, etc. Will remove.
-
Tim Wawrzynczak (Code Review)