Arthur Heymans 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 20:
Patch Set 20:
Patch Set 20:
Patch Set 20:
Patch Set 20:
Patch Set 20:
Patch Set 20: Code-Review-2
(1 comment)
southbridge/intel/common has code to do exactly this, besides that you may want to avoid generating the the legacy PIC entries. You just need implement the function to map int pin to int line.
We had this discussion earlier here : https://review.coreboot.org/c/coreboot/+/34089/9//COMMIT_MSG#14
The idea is to create a interrupt table for PCI IOAPIC mapping , which can we passed to FSP to do the IRQ programming + help generate a APCI package with same info to be passed in _PRT. the implementation also takes care of following IRQ assignment rules:
- For single function PCI device capable of generating
- interrupt, use INTA(IRQ16)
- For multi function PCI device capable of generating
- interrupt, at least one should use INTA(IRQ16)
- LPSS controllers need to be assigned unique IRQs i.e
- no LPSS controllers can have same IRQ# mapped to them.
I get that the FSP needs a specific format to set up those PIN/IRQ_routes (and the FSP integration documentation is severely lacking in that regard!!!) and that you need the code below to provide that format. But the ACPI generation does not need to be reinvented. You could just implement a intel_common_map_pirq() based on the code you have here and reuse existing code for ACPI generation, which this patchseries does not do.
Arthur, this CL does that https://review.coreboot.org/c/coreboot/+/34658 uses the IRQ info created here and create a ACPI package.
And I'm saying that's reinventing the wheel. Please hook up sb/intel/common/acpi_pirq_gen.c for that instead
I already have the irq data populated through this module and stored, ACPI uses the same to create a static package. IMO, would save effort on creating the pirq matrix again through the southbridge/intel/common module, This seems to be a simpler approach, thoughts?
Populating the matrix is trivial. With very little modification you could use it so it would indeed save effort to reuse existing code.
Plus the newer SOC code(code structured in CPU + PCH ) do not have reference to southbridge common code, if I have to use(ignoring the irq info that is already populated) it, I'll have to move to common location , won't be able to validate it for older platforms(currently using it).
You should use as much common code as possible and it does not matter if it is in southbridge/intel or soc/intel. There is no need to move that code as it is already 'common' code. With your intends you should not even have to modify that common code. southbridge/intel/common/acpi_pirq_gen.c is very generic, I don't see how you could break it.