Change in coreboot[master]: soc/intel/common: Add new IRQ block

[Tim Wawrzynczak (Code Review)]

Attention is currently required from: Nico Huber, Furquan Shaikh, Martin Roth, Duncan Laurie, Aamir Bohra, Srinidhi N Kaushik, Patrick Rudolph.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49408 )

Change subject: soc/intel/common: Add new IRQ block
......................................................................

Patch Set 3:

(9 comments)

File src/soc/intel/common/block/irq/irq.c:

https://review.coreboot.org/c/coreboot/+/49408/comment/0cff4cae_2f32c867 
PS3, Line 69: 			return entries[i].irq;
> Put the for body in braces for consistency and balance?

Sure

https://review.coreboot.org/c/coreboot/+/49408/comment/a8975ed0_bd6a31e0 
PS3, Line 82: 1
> PCI_INT_A

Done

https://review.coreboot.org/c/coreboot/+/49408/comment/abe9f04c_1a2f5791 
PS3, Line 87: 			return (enum pci_pin)(i + 1);
> Put the for body in braces for consistency and balance?

Done

https://review.coreboot.org/c/coreboot/+/49408/comment/dcf47520_7ff655e2 
PS3, Line 87: i + 1
> PCI_INT_A + i […]

Done

https://review.coreboot.org/c/coreboot/+/49408/comment/db5836ed_6c1d25cd 
PS3, Line 102: 	while (next != curr_idx) {
> Assumes `current <= MAX_IRQ` to terminate.

Yes, I will add to the condition to make the assumption clear

https://review.coreboot.org/c/coreboot/+/49408/comment/0125b026_192092ab 
PS3, Line 119: 	dev = pcidev_path_on_root(devfn);
             : 	if (!dev) {
             : 		dummy.bus = &bus;
             : 		dummy.path.type = DEVICE_PATH_PCI;
             : 		dummy.path.pci.devfn = devfn;
             : 		dev = &dummy;
             : 	}
             : 
             : 	const uint8_t pin = pci_read_config8(dev, PCI_INTERRUPT_PIN);
> Looks like calling pci_s_read_config8() directly would be easier here.

Oops yes it would be 😊

https://review.coreboot.org/c/coreboot/+/49408/comment/199e30d5_52db496c 
PS3, Line 155: 					line = MIN_IRQ;
> Why?

I think that escaped from a previous draft, should not be necessary.

https://review.coreboot.org/c/coreboot/+/49408/comment/420e342b_6fe86a2a 
PS3, Line 186: 			int pin = get_pci_irq_pin(devfn);
> This seems a bit fragile, `UNIQUE_IRQ` would have the requirement that no other fn
> has the same pin set. Which is harder to verify. next_pin_for_slot() also does
> not take potential future get_pci_irq_pin() results into account and relies on
> the already set `entries` alone. It seems everything would be easier if we say
> `UNIQUE_IRQ` requires all pins of a `slot` configurable and all fn's should be
> set to `UNIQUE_IRQ`. That seems true for the TGL use-case at least.

In practice this works  because the LPSS controllers are mostly off by themselves. You're correct, this is a greedy algorithm, it does not "search" the possible solution space with backtracking, etc. if it comes across a conflict.

I have a hypothesis Intel may have taken that into account when laying out the slot/fn combos...

https://review.coreboot.org/c/coreboot/+/49408/comment/a7bc33c2_2102b425 
PS3, Line 209: PIRQ
> PIRQA

Done