On Thu, Jul 25, 2024 at 01:50:16PM +0530, Siddharth Vadapalli wrote: > On Thu, Jul 25, 2024 at 09:50:01AM +0530, Manivannan Sadhasivam wrote: > > On Wed, Jul 24, 2024 at 09:49:21PM +0530, Manivannan Sadhasivam wrote: > > > On Wed, Jul 24, 2024 at 12:20:48PM +0530, Siddharth Vadapalli wrote: > > > > Since the configuration of Legacy Interrupts (INTx) is not supported, set > > > > the .map_irq and .swizzle_irq callbacks to NULL. This fixes the error: > > > > of_irq_parse_pci: failed with rc=-22 > > > > due to the absence of Legacy Interrupts in the device-tree. > > > > > > > > > > Do you really need to set 'swizzle_irq' to NULL? pci_assign_irq() will bail out > > > if 'map_irq' is set to NULL. > > > > > > > Hold on. The errono of of_irq_parse_pci() is not -ENOENT. So the INTx interrupts > > are described in DT? Then why are they not supported? > > No, the INTx interrupts are not described in the DT. It is the pcieport > driver that is attempting to setup INTx via "of_irq_parse_and_map_pci()" > which is the .map_irq callback. The sequence of execution leading to the > error is as follows: > > pcie_port_probe_service() > pci_device_probe() > pci_assign_irq() > hbrg->map_irq > of_pciof_irq_parse_and_map_pci() > of_irq_parse_pci() > of_irq_parse_raw() > rc = -EINVAL > ... > [DEBUG] OF: of_irq_parse_raw: ipar=/bus@100000/interrupt-controller@1800000, size=3 > if (out_irq->args_count != intsize) > goto fail > return rc > > The call to of_irq_parse_raw() results in the Interrupt-Parent for the > PCIe node in the device-tree being found via of_irq_find_parent(). The > Interrupt-Parent for the PCIe node for MSI happens to be GIC_ITS: > msi-map = <0x0 &gic_its 0x0 0x10000>; > and the parent of GIC_ITS is: > gic500: interrupt-controller@1800000 > which has the following: > #interrupt-cells = <3>; > > The "size=3" portion of the DEBUG print above corresponds to the > #interrupt-cells property above. Now, "out_irq->args_count" is set to 1 > as __assumed__ by of_irq_parse_pci() and mentioned as a comment in that > function: > /* > * Ok, we don't, time to have fun. Let's start by building up an > * interrupt spec. we assume #interrupt-cells is 1, which is standard > * for PCI. If you do different, then don't use that routine. > */ > > In of_irq_parse_pci(), since the PCIe-Port driver doesn't have a > device-tree node, the following doesn't apply: > dn = pci_device_to_OF_node(pdev); > and we skip to the __assumption__ above and proceed as explained in the > execution sequence above. > > If the device-tree nodes for the INTx interrupts were present, the > "ipar" sequence to find the interrupt parent would be skipped and we > wouldn't end up with the -22 (-EINVAL) error code. > > I hope this clarifies the relation between the -22 error code and the > missing device-tree nodes for INTx. > Thanks for explaining the logic. Still I think the logic is flawed. Because the parent (host bridge) doesn't have 'interrupt-map', which means INTx is not supported. But parsing one level up to the GIC node and not returning -ENOENT doesn't make sense to me. Rob, what is your opinion on this behavior? - Mani -- மணிவண்ணன் சதாசிவம்
