Re: [PATCH v2] PCI: j721e: Disable INTx mapping and swizzling

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Fri, Jul 26, 2024 at 07:29:03PM +0530, Siddharth Vadapalli wrote:
> Since the configuration of INTx (Legacy Interrupt) 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
> when the pcieport driver attempts to setup INTx for the Host Bridge via
> "pci_assign_irq()". The device-tree node of the Host Bridge doesn't
> contain Legacy Interrupts. As a result, Legacy Interrupts are searched for
> in the MSI Interrupt Parent of the Host Bridge with the help of
> "of_irq_parse_raw()". Since the MSI Interrupt Parent of the Host Bridge
> uses 3 interrupt cells while Legacy Interrupts only use 1, the search
> for Legacy Interrupts is terminated prematurely by the "of_irq_parse_raw()"
> function with the -EINVAL error code (rc=-22).
> 
> Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
> Cc: stable@vger.kernel.org
> Reported-by: Andrew Halaney <ahalaney@redhat.com>
> Tested-by: Andrew Halaney <ahalaney@redhat.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

TBH, I'm not comfortable with this change. Because, the INTx compatibility
_should_ come from the platform description (in this case DT) and not the
driver. The default map_irq() function is supposed to check the existence of
INTx and correctly report it. In this case the issue is, the platform is not
supporting INTx, but of_irq_parse_pci() reports a dubious error:

'of_irq_parse_pci: failed with rc=-22'

instead of the actual error:

`of_irq_parse_pci: no interrupt-map found, INTx interrupts not available'

So I would say that the fix is in of_irq_parse_pci() implementation.
of_irq_parse_pci() is supposed to find whether the PCIe controller is supporting
INTx or not by parsing the 'interrupt-{map/extended}' properties till host
bridge/controller node. But this API along with of_irq_parse_raw(), just walks
up the tree till the top level interrupt controller and checks for INTx, which
just feels wrong (that's why you are getting -EINVAL because of #interrupt-cells
mismatch).

I looked into the implementation over the weekend, but I'm not quite sure how to
fix it though.

Rob, perhaps you have any idea?

- Mani

> ---
> 
> Hello,
> 
> This patch is based on commit
> 1722389b0d86 Merge tag 'net-6.11-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> of Mainline Linux.
> 
> v1:
> https://lore.kernel.org/r/20240724065048.285838-1-s-vadapalli@ti.com
> Changes since v1:
> - Added "Cc: stable@vger.kernel.org" in the commit message.
> - Based on Bjorn's feedback at:
>   https://lore.kernel.org/r/20240724162304.GA802428@bhelgaas/
>   the $subject and commit message have been updated. Additionally, the
>   comment in the driver has also been updated to specify "INTx" instead of
>   "Legacy Interrupts".
> - Collected Tested-by tag from Andrew Halaney <ahalaney@redhat.com>:
>   https://lore.kernel.org/r/vj6vtjphpmqv6hcblaalr2m4bwjujjwvom6ca4bjdzcmgazyaa@436unrb2lav7/
> 
> Patch has been tested on J721e-EVM and J784S4-EVM.
> 
> Regards,
> Siddharth.
> 
>  drivers/pci/controller/cadence/pci-j721e.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
> index 85718246016b..eaa6cfeb03c7 100644
> --- a/drivers/pci/controller/cadence/pci-j721e.c
> +++ b/drivers/pci/controller/cadence/pci-j721e.c
> @@ -417,6 +417,10 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  		if (!bridge)
>  			return -ENOMEM;
>  
> +		/* INTx is not supported */
> +		bridge->map_irq = NULL;
> +		bridge->swizzle_irq = NULL;
> +
>  		if (!data->byte_access_allowed)
>  			bridge->ops = &cdns_ti_pcie_host_ops;
>  		rc = pci_host_bridge_priv(bridge);
> -- 
> 2.40.1
> 

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