On Mon, Aug 5, 2024 at 10:45 AM Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> wrote: > > On Mon, Aug 05, 2024 at 10:01:37AM -0600, Rob Herring wrote: > > On Fri, Jul 26, 2024 at 5:56 AM Manivannan Sadhasivam > > <manivannan.sadhasivam@xxxxxxxxxx> wrote: > > > > > > 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? > > > > Not sure I get the question. How should we handle/determine no INTx? I > > suppose that's either based on the platform (as this patch did) or by > > Platform != driver. Here the driver is making the call, but the platform > capability should come from DT, no? I don't like the idea of disabling INTx in > the driver because, the driver may support multiple SoCs and these capability > may differ between them. So the driver will end up just hardcoding the info > which is already present in DT :/ Let me rephrase it to "a decision made within the driver" (vs. globally decided). That could be hardcoded (for now) or as match data based on compatible. > Moreover, the issue I'm seeing is, even if the platform doesn't support INTx (as > described by DT in this case), of_irq_parse_pci() doesn't report correct > error/log. So of_irq_parse_pci() definitely needs a fixup. Possibly. What's correct here? There was some rework in 6.11 of the interrupt parsing. So it is possible something changed here. There's also this issue still pending: https://lore.kernel.org/all/2046da39e53a8bbca5166e04dfe56bd5.squirrel@_/ > > or by > > failing to parse the interrupts. The interrupt parsing code is pretty > > tricky as it has to deal with some ancient DTs, so I'm a little > > hesitant to rely on that failing. Certainly I wouldn't rely on a > > specific errno value. The downside to doing that is also if someone > > wants interrupts, but has an error in their DT, then all we can do is > > print 'INTx not supported' or something. So we couldn't fail probe as > > the common code wouldn't be able to distinguish. I suppose we could > > just check for 'interrupt-map' present in the host bridge node or not. > > Yeah, as simple as that. But I don't know if that is globally applicable to > all platforms. There's a lot of history and the interrupt parsing is fragile due to all the "interesting" DT interrupt hierarchies. So while I think it would work, that's just a guess. I'm open to trying it and seeing. Rob
