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
--
மணிவண்ணன் சதாசிவம்
