Message-ID: <CAL_JsqKxF6yYTWbmU8SRhxemNMwErNViHuk05sLyFjFzssh=Eg@mail.gmail.com>
Date: Mon, 5 Aug 2024 13:05:14 -0600
From: Rob Herring <robh@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Siddharth Vadapalli <s-vadapalli@...com>, bhelgaas@...gle.com, lpieralisi@...nel.org, 
	kw@...ux.com, vigneshr@...com, kishon@...nel.org, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	stable@...r.kernel.org, ahalaney@...hat.com, srk@...com
Subject: Re: [PATCH] PCI: j721e: Set .map_irq and .swizzle_irq to NULL

On Mon, Aug 5, 2024 at 10:45 AM Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org> 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@...aro.org> 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@...000/interrupt-controller@...0000, 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@...0000
> > > > 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

Powered by blists - more mailing lists