Message-ID: <CAL_JsqLq6cwjKY8jiRmB6TYiKBEs8F3PDedwAyRW3dqTArJhUg@mail.gmail.com>
Date: Mon, 5 Aug 2024 14:40:09 -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 1:05 PM Rob Herring <robh@...nel.org> wrote:
>
> 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@_/

I meant this rework was in 6.10.

Rob

Powered by blists - more mailing lists