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 20:
Patch Set 20:
Patch Set 20:
Patch Set 20:
Patch Set 20: Ok, But this code already creates a irq info data , I am using the same to generate the ACPI code too, I do not need to re-populate the irq matrix.
This argument sounds to me as if you don't want to use the common facilities we already have because you already wrote your own reimplementation of them. I hope I misunderstand you because that's not very convincing.
What is missing in acpi_pirq_gen that is present in your code (and can't be added there to avoid having all the common parts twice in our code base)?
The intent of this code was to populate irq info based on the device tree selection and pass it to FSP for IRQ configuration based on the irq assignment rules, this implementation is added new(does not exist in acpi_pirq_gen(neither should be scoped in ACPI module)), I think Arthur made a point to re-use APCI generation part from acpi_pirq_gen, to which my response was I did not want to re-populate the irq matrix again, since this code already creates that info.
You don't have to repopulate anything, the matrix is a local structure of that code. You just have to provide a callback function called 'intel_common_map_pirq' which returns a route for a given pin on a given PCI device. Given your current code that should be easy or even trivial to implement. Give the FSP what it needs using this code(and please document it first!) and use the common southbridge code create ACPI _PIR tables.
Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects.
Please consider that reusing common code usually means avoiding pitfalls someone else fell into in the past.
Hi Arthur, I wanted to understand more on you below comment:
[Arthur]Your code potentially has multiple entries for the same PCI device and pin in the _PIR table. That needs to be avoided. The best way to avoid that is by populating a matrix[MAX_PCI_DEV][4] '4' being pin[A-D] otherwise you have to keep track of what device and what pin already have an entry. You end up needing a 'matrix' anyway as the data FSP needs does not fully match what ACPI _PIR expects.
[Aamir] I do not see multiple entries with same PCI device and int pin mapping , below is the ACPI package that is currently generated with this implementation:
Name (PIRX, Package (0x11) { Package (0x04) { 0x0012FFFF, 0x00, Zero, 0x10 },
Package (0x04) { 0x0014FFFF, 0x00, Zero, 0x10 },
Package (0x04) { 0x0014FFFF, 0x01, Zero, 0x11 },
Package (0x04) { 0x0014FFFF, 0x02, Zero, 0x12 },
Package (0x04) { 0x0015FFFF, 0x00, Zero, 0x10 },
Package (0x04) { 0x0015FFFF, 0x01, Zero, 0x11 },
Package (0x04) { 0x0015FFFF, 0x02, Zero, 0x12 },
Package (0x04) { 0x0016FFFF, 0x00, Zero, 0x10 },
Package (0x04) { 0x0017FFFF, 0x00, Zero, 0x10 },
Package (0x04) { 0x0019FFFF, 0x03, Zero, 0x13 },
Package (0x04) { 0x001DFFFF, 0x00, Zero, 0x10 },
Package (0x04) { 0x001DFFFF, 0x01, Zero, 0x11 },
Package (0x04) { 0x001EFFFF, 0x00, Zero, 0x14 },
Package (0x04) { 0x001EFFFF, 0x01, Zero, 0x15 },
Package (0x04) { 0x001EFFFF, 0x02, Zero, 0x16 },
Package (0x04) { 0x001FFFFF, 0x00, Zero, 0x10 },
Package (0x04) { 0x001FFFFF, 0x01, Zero, 0x11 } })
And this is the IRQ table passed to FSP for configuration.(LPSS would not share same IRQs) https://review.coreboot.org/c/coreboot/+/34089/13/src/soc/intel/common/block...
Both aligns and do not mismatch, that is the advantage of using the same irq data for creating the APCI package as well.