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

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

On Thu, Aug 08, 2024 at 03:56:10PM -0500, Andrew Halaney wrote:

[...]

> > 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.
> 
> Would something like this be what you're imagining? If so I can post a
> patch if this patch is a dead end:
> 
>     diff --git a/drivers/pci/of.c b/drivers/pci/of.c
>     index dacea3fc5128..4e4ecaa95599 100644
>     --- a/drivers/pci/of.c
>     +++ b/drivers/pci/of.c
>     @@ -512,6 +512,10 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
>                             if (ppnode == NULL) {
>                                     rc = -EINVAL;
>                                     goto err;
>     +                       } else if (!of_get_property(ppnode, "interrupt-map", NULL)) {
>     +                               /* No interrupt-map on a host bridge means we're done here */
>     +                               rc = -ENOENT;
>     +                               goto err;
>                             }
>                     } else {
>                             /* We found a P2P bridge, check if it has a node */
> 

This is a reasonable change if the parent is the host bridge. But if parent is a
PCI bridge node (note the else condition), then of_irq_parse_raw() will get
called and we will hit the same issue.

IMO, either we need to fix of_irq_parse_raw() or come up with another
implementation that does the right thing i.e., travese upto the host bridge and
check for the 'interrupt-map'. Currently it goes till the top level interrupt
controller.

> I must admit that you being nervous has me being nervous since I'm not all
> that familiar with PCI... but if y'all think this is ok then I'm for it.
> I'm sure I'm not picturing all the cases here so would appreciate
> some scrutiny.
> 
> You still end up with warnings, which kind of sucks, since as I
> understand it the lack of INTx interrupts on this platform is
> *intentional*:
> 
>     [    3.342548] pci_bus 0000:00: 2-byte config write to 0000:00:00.0 offset 0x4 may corrupt adjacent RW1C bits
>     [    3.346716] pcieport 0000:00:00.0: of_irq_parse_pci: no interrupt-map found, INTx interrupts not available
>     [    3.346721] PCI: OF: of_irq_parse_pci: possibly some PCI slots don't have level triggered interrupts capability
> 

I propose to demote these prints to debug as these are not warnings by any
means.

> You could have a combo of both this patch (to indicate that a specific driver (even further
> limited to a match data based on compatible) doesn't support these) as well as
> the above diff (to improve the message printed in the situation where a driver
> *does* claim to support these interrupts but fails to describe them properly).
> 

Again, I don't think we need to have the change in the driver. DT already
indicated that there is no INTx support, so why should driver duplicate the same
info?

- Mani

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