Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34089 )
Change subject: src/soc/intel/common/itss: Add support to get IRQ configuration for PCI devices ......................................................................
Patch Set 14:
(10 comments)
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/irq.h:
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 25: P
nit: space after *
Done
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 31: PCI devices
Is this list of PCI devfns?
Yes. Updated the comments. Is it Ok now?
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 32: int
pci_devfn_t?
Done
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 44:
Ack
Done
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... File src/soc/intel/common/block/itss/irq.c:
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 32: LPSS controllers need to be assigned unique IRQs
Does this mean that IRQ# for LPSS device cannot be shared with another LPSS device or any other devi […]
The LPSS devices cannot share the same IRQ# and needs to be assigned different IRQs. Documented in EDS. Serial IO(UART) PCR register section.
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 33: *
What about PCI swizzling for PCIe root ports?
Add as rule? or did you mean if this implementation would take care of it. Assuming latter, the current implementation would ensure the PCIe RP enabled on same slot are mapped to different INTx
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 34:
Ack
Done
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 47: slot != PCI_SLOT(dev->path.pci.devfn)
It looks like this is setting int_line to PIRQA_APIC_IRQ if slot for current device does not match w […]
Yes. Below implementation would take take for the sorting I believe. if (!dev->enabled || dev->path.type != DEVICE_PATH_PCI ||!is_irq_dev(dev))
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 71: soc_irq_devices_list
Instead of having another call go back to SoC, can it provide the device list as a parameter to fill […]
I can add an additional parameter and eliminate the soc call from common. fill_pci_irq_config with just pointer to config data and size seemed to better fit. Let me know if you do not agree, I'll make that change.
Actually, can fill_pci_irq_config() loop only over the device list provided by SoC instead of all devices?
I think we will loose the sorting then, unless SOC explicitly ensures that.
https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block... PS13, Line 99: index++;
Curious why SoC has to pass in the address of table that needs to be filled in? Instead can this fil […]
Did not think of that. SOC max buffer can change based on the SOCs, But that does not seem to case for upcoming SOCs and 32 as MAX buffer should suffice. Actually SOC function is written such that it would only call fill_pci_irq_config once(if it returns a valid size) and later functions(ACPI) can just refer to static irq config buffer populated from 1st call. Does it sound right?