Re: [PATCH] PCI: j721e: Set .map_irq and .swizzle_irq to NULL

[Manivannan Sadhasivam]

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

-- 
மணிவண்ணன் சதாசிவம்