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

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

[Siddharth Vadapalli]

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.

Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
Reported-by: Andrew Halaney <ahalaney@redhat.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---

Hello,

This patch is based on commit
786c8248dbd3 Merge tag 'perf-tools-fixes-for-v6.11-2024-07-23' of git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools
of Mainline Linux.

Patch has been tested on J784S4-EVM and J721e-EVM, both of which have
the PCIe Controller configured by the pci-j721e.c driver.

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..5372218849a8 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;
 
+		/* Legacy interrupts are 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

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

[Manivannan Sadhasivam]

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.

- Mani

> Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
> Reported-by: Andrew Halaney <ahalaney@redhat.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on commit
> 786c8248dbd3 Merge tag 'perf-tools-fixes-for-v6.11-2024-07-23' of git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools
> of Mainline Linux.
> 
> Patch has been tested on J784S4-EVM and J721e-EVM, both of which have
> the PCIe Controller configured by the pci-j721e.c driver.
> 
> 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..5372218849a8 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;
>  
> +		/* Legacy interrupts are 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
> 
> 

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

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

[Manivannan Sadhasivam]

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?

- Mani

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

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

[Siddharth Vadapalli]

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@100000/interrupt-controller@1800000, 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@1800000
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.

Regards,
Siddharth.

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

[Manivannan Sadhasivam]

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@100000/interrupt-controller@1800000, 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@1800000
> 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?

- Mani

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

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

[Rob Herring]

On Fri, Jul 26, 2024 at 5:56 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.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@100000/interrupt-controller@1800000, 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@1800000
> > 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
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.
Need to check the Marvell binding which is a bit weird with child
nodes.

Rob

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

[Manivannan Sadhasivam]

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@linaro.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@100000/interrupt-controller@1800000, 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@1800000
> > > 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 :/

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.

> 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.

- Mani

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

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

[Rob Herring]

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

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

[Rob Herring]

On Mon, Aug 5, 2024 at 1:05 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Aug 5, 2024 at 10:45 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.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@linaro.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@100000/interrupt-controller@1800000, 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@1800000
> > > > > 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

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

[Andrew Halaney]

On Mon, Aug 05, 2024 at 01:05:14PM GMT, Rob Herring wrote:
> On Mon, Aug 5, 2024 at 10:45 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.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@linaro.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@100000/interrupt-controller@1800000, 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@1800000
> > > > > 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.

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 */

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

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).

Am I barking up the right tree? If so I'll submit a proper patch
independent of this (and depending on your views we can continue with v2
of this patch too, or not).

Thanks,
Andrew

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

[Manivannan Sadhasivam]

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

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

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

[Siddharth Vadapalli]

On Wed, Jul 24, 2024 at 09:49:16PM +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.

While 'swizzle_irq' won't be invoked if 'map_irq' is NULL, having a
non-NULL 'swizzle_irq' (pci_common_swizzle in this case) with a NULL
'map_irq' seems inconsistent to me though the code-path may never invoke
it. Wouldn't a non-NULL 'swizzle_irq' imply that Legacy Interrupts are
supported, while a NULL 'map_irq' indicates that they aren't? Since they
are always described in pairs, whether it is in the initial commit that
added support for the Cadence PCIe Host controller (used by pci-j721e.c):
https://github.com/torvalds/linux/commit/1b79c5284439
OR the commit which moved the shared 'map_irq' and 'swizzle_irq' defaults
from all the host drivers into the common 'devm_of_pci_bridge_init()'
function:
https://github.com/torvalds/linux/commit/b64aa11eb2dd
I have set both of them to NULL for the sake of consistency.

Regards,
Siddharth.

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

[Manivannan Sadhasivam]

On Thu, Jul 25, 2024 at 10:50:13AM +0530, Siddharth Vadapalli wrote:
> On Wed, Jul 24, 2024 at 09:49:16PM +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.
> 
> While 'swizzle_irq' won't be invoked if 'map_irq' is NULL, having a
> non-NULL 'swizzle_irq' (pci_common_swizzle in this case) with a NULL
> 'map_irq' seems inconsistent to me though the code-path may never invoke
> it. Wouldn't a non-NULL 'swizzle_irq' imply that Legacy Interrupts are
> supported, while a NULL 'map_irq' indicates that they aren't? Since they
> are always described in pairs, whether it is in the initial commit that
> added support for the Cadence PCIe Host controller (used by pci-j721e.c):
> https://github.com/torvalds/linux/commit/1b79c5284439
> OR the commit which moved the shared 'map_irq' and 'swizzle_irq' defaults
> from all the host drivers into the common 'devm_of_pci_bridge_init()'
> function:
> https://github.com/torvalds/linux/commit/b64aa11eb2dd
> I have set both of them to NULL for the sake of consistency.
> 

Since both callbacks are populated in the pci/of driver, this consistency won't
be visible in the controller drivers. From the functionality pov, setting both
callbacks to NULL is *not* required to disable INTx, right?

- Mani

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

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

[Siddharth Vadapalli]

On Thu, Jul 25, 2024 at 01:17:08PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jul 25, 2024 at 10:50:13AM +0530, Siddharth Vadapalli wrote:
> > On Wed, Jul 24, 2024 at 09:49:16PM +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.
> > 
> > While 'swizzle_irq' won't be invoked if 'map_irq' is NULL, having a
> > non-NULL 'swizzle_irq' (pci_common_swizzle in this case) with a NULL
> > 'map_irq' seems inconsistent to me though the code-path may never invoke
> > it. Wouldn't a non-NULL 'swizzle_irq' imply that Legacy Interrupts are
> > supported, while a NULL 'map_irq' indicates that they aren't? Since they
> > are always described in pairs, whether it is in the initial commit that
> > added support for the Cadence PCIe Host controller (used by pci-j721e.c):
> > https://github.com/torvalds/linux/commit/1b79c5284439
> > OR the commit which moved the shared 'map_irq' and 'swizzle_irq' defaults
> > from all the host drivers into the common 'devm_of_pci_bridge_init()'
> > function:
> > https://github.com/torvalds/linux/commit/b64aa11eb2dd
> > I have set both of them to NULL for the sake of consistency.
> > 
> 
> Since both callbacks are populated in the pci/of driver, this consistency won't
> be visible in the controller drivers. From the functionality pov, setting both
> callbacks to NULL is *not* required to disable INTx, right?

Yes, setting 'swizzle_irq' to NULL isn't required. The execution sequence
with 'swizzle_irq' set to 'pci_common_swizzle()' is as follows:

pci_assign_irq()
  if (pin) {
    if (hbrg->swizzle_irq)
      slot = (*(hbrg->swizzle_irq))(dev, &pin);
        pci_common_swizzle()
	  while (!pci_is_root_bus(dev->bus)) <= NOT entered
	..continue execution similar to 'swizzle_irq' being NULL.

Having 'swizzle_irq' set to 'pci_common_swizzle()' will only result
in a no-op which could have been avoided by setting it to NULL. So there
is no difference w.r.t. functionality.

Regards,
Siddharth.

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

[Siddharth Vadapalli]

On Thu, Jul 25, 2024 at 02:01:48PM +0530, Siddharth Vadapalli wrote:
> On Thu, Jul 25, 2024 at 01:17:08PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Jul 25, 2024 at 10:50:13AM +0530, Siddharth Vadapalli wrote:
> > > On Wed, Jul 24, 2024 at 09:49:16PM +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.
> > > 
> > > While 'swizzle_irq' won't be invoked if 'map_irq' is NULL, having a
> > > non-NULL 'swizzle_irq' (pci_common_swizzle in this case) with a NULL
> > > 'map_irq' seems inconsistent to me though the code-path may never invoke
> > > it. Wouldn't a non-NULL 'swizzle_irq' imply that Legacy Interrupts are
> > > supported, while a NULL 'map_irq' indicates that they aren't? Since they
> > > are always described in pairs, whether it is in the initial commit that
> > > added support for the Cadence PCIe Host controller (used by pci-j721e.c):
> > > https://github.com/torvalds/linux/commit/1b79c5284439
> > > OR the commit which moved the shared 'map_irq' and 'swizzle_irq' defaults
> > > from all the host drivers into the common 'devm_of_pci_bridge_init()'
> > > function:
> > > https://github.com/torvalds/linux/commit/b64aa11eb2dd
> > > I have set both of them to NULL for the sake of consistency.
> > > 
> > 
> > Since both callbacks are populated in the pci/of driver, this consistency won't
> > be visible in the controller drivers. From the functionality pov, setting both
> > callbacks to NULL is *not* required to disable INTx, right?
> 
> Yes, setting 'swizzle_irq' to NULL isn't required. The execution sequence
> with 'swizzle_irq' set to 'pci_common_swizzle()' is as follows:
> 
> pci_assign_irq()
>   if (pin) {
>     if (hbrg->swizzle_irq)
>       slot = (*(hbrg->swizzle_irq))(dev, &pin);
>         pci_common_swizzle()
> 	  while (!pci_is_root_bus(dev->bus)) <= NOT entered
> 	..continue execution similar to 'swizzle_irq' being NULL.
> 
> Having 'swizzle_irq' set to 'pci_common_swizzle()' will only result
> in a no-op which could have been avoided by setting it to NULL. So there
> is no difference w.r.t. functionality.

Mani,

I prefer setting 'swizzle_irq' to NULL as well unless you have an objection
to it. Kindly let me know. I plan to post the v2 for this patch addressing
Bjorn's feedback and collecting Andrew's "Tested-by" tag as well.

Regards,
Siddharth.

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

[Manivannan Sadhasivam]

On Fri, Jul 26, 2024 at 03:54:17PM +0530, Siddharth Vadapalli wrote:
> On Thu, Jul 25, 2024 at 02:01:48PM +0530, Siddharth Vadapalli wrote:
> > On Thu, Jul 25, 2024 at 01:17:08PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Jul 25, 2024 at 10:50:13AM +0530, Siddharth Vadapalli wrote:
> > > > On Wed, Jul 24, 2024 at 09:49:16PM +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.
> > > > 
> > > > While 'swizzle_irq' won't be invoked if 'map_irq' is NULL, having a
> > > > non-NULL 'swizzle_irq' (pci_common_swizzle in this case) with a NULL
> > > > 'map_irq' seems inconsistent to me though the code-path may never invoke
> > > > it. Wouldn't a non-NULL 'swizzle_irq' imply that Legacy Interrupts are
> > > > supported, while a NULL 'map_irq' indicates that they aren't? Since they
> > > > are always described in pairs, whether it is in the initial commit that
> > > > added support for the Cadence PCIe Host controller (used by pci-j721e.c):
> > > > https://github.com/torvalds/linux/commit/1b79c5284439
> > > > OR the commit which moved the shared 'map_irq' and 'swizzle_irq' defaults
> > > > from all the host drivers into the common 'devm_of_pci_bridge_init()'
> > > > function:
> > > > https://github.com/torvalds/linux/commit/b64aa11eb2dd
> > > > I have set both of them to NULL for the sake of consistency.
> > > > 
> > > 
> > > Since both callbacks are populated in the pci/of driver, this consistency won't
> > > be visible in the controller drivers. From the functionality pov, setting both
> > > callbacks to NULL is *not* required to disable INTx, right?
> > 
> > Yes, setting 'swizzle_irq' to NULL isn't required. The execution sequence
> > with 'swizzle_irq' set to 'pci_common_swizzle()' is as follows:
> > 
> > pci_assign_irq()
> >   if (pin) {
> >     if (hbrg->swizzle_irq)
> >       slot = (*(hbrg->swizzle_irq))(dev, &pin);
> >         pci_common_swizzle()
> > 	  while (!pci_is_root_bus(dev->bus)) <= NOT entered
> > 	..continue execution similar to 'swizzle_irq' being NULL.
> > 
> > Having 'swizzle_irq' set to 'pci_common_swizzle()' will only result
> > in a no-op which could have been avoided by setting it to NULL. So there
> > is no difference w.r.t. functionality.
> 
> Mani,
> 
> I prefer setting 'swizzle_irq' to NULL as well unless you have an objection
> to it. Kindly let me know. I plan to post the v2 for this patch addressing
> Bjorn's feedback and collecting Andrew's "Tested-by" tag as well.
> 

Ok, fine with me.

- Mani

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

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

[Siddharth Vadapalli]

On Fri, Jul 26, 2024 at 05:00:08PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jul 26, 2024 at 03:54:17PM +0530, Siddharth Vadapalli wrote:

[...]

> > Mani,
> > 
> > I prefer setting 'swizzle_irq' to NULL as well unless you have an objection
> > to it. Kindly let me know. I plan to post the v2 for this patch addressing
> > Bjorn's feedback and collecting Andrew's "Tested-by" tag as well.
> > 
> 
> Ok, fine with me.

Thank you for the confirmation.

Regards,
Siddharth.

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

[Bjorn Helgaas]

Subject should say something about why this change is needed, not just
translate the C code to English.

On Wed, Jul 24, 2024 at 12:20:48PM +0530, Siddharth Vadapalli wrote:
> Since the configuration of Legacy Interrupts (INTx) is not supported, set

I assume you mean J721E doesn't support INTx?

> the .map_irq and .swizzle_irq callbacks to NULL. This fixes the error:
>   of_irq_parse_pci: failed with rc=-22

I guess this happens because devm_of_pci_bridge_init() initializes
.map_irq and .swizzle_irq unconditionally?

I'm not sure the default assumption should be that a host bridge
supports INTx.

Maybe .map_irq and .swizzle_irq should only be set when we discover
some information about INTx routing, e.g., an ACPI _PRT or the
corresponding DT properties.

> due to the absence of Legacy Interrupts in the device-tree.
> 
> Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
> Reported-by: Andrew Halaney <ahalaney@redhat.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
> 
> Hello,
> 
> This patch is based on commit
> 786c8248dbd3 Merge tag 'perf-tools-fixes-for-v6.11-2024-07-23' of git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools
> of Mainline Linux.
> 
> Patch has been tested on J784S4-EVM and J721e-EVM, both of which have
> the PCIe Controller configured by the pci-j721e.c driver.
> 
> 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..5372218849a8 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;
>  
> +		/* Legacy interrupts are not supported */

Say "INTx" explicitly here instead of assuming "legacy" == "INTx".

> +		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
> 

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

[Siddharth Vadapalli]

On Wed, Jul 24, 2024 at 11:23:04AM -0500, Bjorn Helgaas wrote:
> Subject should say something about why this change is needed, not just
> translate the C code to English.

My intent was to summarize the change to make it easy for anyone to find
out what's being done. The commit message below explains in detail as to
why they are set to NULL. As an alternative, I could change the $subject
to:
PCI: j721e: Disable INTx mapping and swizzling
where the "Disable" is equivalent to NULL. Kindly let me know if this is
acceptable or needs to be improved further.

> 
> On Wed, Jul 24, 2024 at 12:20:48PM +0530, Siddharth Vadapalli wrote:
> > Since the configuration of Legacy Interrupts (INTx) is not supported, set
> 
> I assume you mean J721E doesn't support INTx?

Yes, the driver support doesn't exist and the device-tree also doesn't
have the required nodes. Kishon had posted a series to enable it for
J721E, J7200 and AM64 on the 4th of August 2021:
https://lore.kernel.org/r/20210804132912.30685-1-kishon@ti.com/
and based on the discussion on the series, an Errata was discovered for
J721E:
https://www.ti.com/lit/er/sprz455d/sprz455d.pdf
i2094: PCIe: End of Interrupt (EOI) Not Enabled for PCIe Legacy Interrupts

Thus, there is no support for Legacy Interrupts (INTx) in the pci-j721e.c
driver and as a consequence, no device-tree nodes either.

> 
> > the .map_irq and .swizzle_irq callbacks to NULL. This fixes the error:
> >   of_irq_parse_pci: failed with rc=-22
> 
> I guess this happens because devm_of_pci_bridge_init() initializes
> .map_irq and .swizzle_irq unconditionally?

Yes. The PCIe Controller on J721E and other SoCs is the Cadence PCIe
Controller. In the commit which added driver support for the Cadence
PCIe Controller's Host functionality:
https://github.com/torvalds/linux/commit/1b79c5284439
'map_irq' and 'swizzle_irq' were set to the same functions:
'of_irq_parse_and_map_pci()' and 'pci_common_swizzle()'
respectively. Commit:
https://github.com/torvalds/linux/commit/b64aa11eb2dd
extracted out the shared defaults from multiple Host Controller drivers
and moved them into the common 'devm_of_pci_bridge_init()' function.

While 'map_irq' and 'swizzle_irq' are unconditionally set to the
defaults in 'devm_of_pci_bridge_init()', those Host Controller driver
which haven't been using the shared defaults do have the choice of
overriding them in the driver (which is already being done).

Similarly, for pci-j721e.c, since the shared defaults inherited from the
Cadence PCIe Controller are not applicable (due to the Errata), the
pci-j721e.c driver should override them to NULL as done in this patch.

> 
> I'm not sure the default assumption should be that a host bridge
> supports INTx.
> 
> Maybe .map_irq and .swizzle_irq should only be set when we discover
> some information about INTx routing, e.g., an ACPI _PRT or the
> corresponding DT properties.

I believe that the defaults were set since most of the Host drivers were
using them in their respective drivers as indicated in the commit:
https://github.com/torvalds/linux/commit/b64aa11eb2dd

[...]

> > 
> >  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..5372218849a8 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;
> >  
> > +		/* Legacy interrupts are not supported */
> 
> Say "INTx" explicitly here instead of assuming "legacy" == "INTx".

Sure. I will update it in the v2 patch.

Thank you for reviewing this patch and sharing your feedback.

Regards,
Siddharth.

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

[Bjorn Helgaas]

On Thu, Jul 25, 2024 at 11:23:43AM +0530, Siddharth Vadapalli wrote:
> On Wed, Jul 24, 2024 at 11:23:04AM -0500, Bjorn Helgaas wrote:
> > Subject should say something about why this change is needed, not just
> > translate the C code to English.
> 
> My intent was to summarize the change to make it easy for anyone to find
> out what's being done. The commit message below explains in detail as to
> why they are set to NULL. As an alternative, I could change the $subject
> to:
> PCI: j721e: Disable INTx mapping and swizzling
> where the "Disable" is equivalent to NULL. Kindly let me know if this is
> acceptable or needs to be improved further.

Sounds fine to me.

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

[Andrew Halaney]

On Wed, Jul 24, 2024 at 12:20:48PM GMT, 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.
> 
> Fixes: f3e25911a430 ("PCI: j721e: Add TI J721E PCIe driver")
> Reported-by: Andrew Halaney <ahalaney@redhat.com>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Tested-by: Andrew Halaney <ahalaney@redhat.com>

Thanks for the quick work and follow through on the patch, I appreciate
it! I would not have come to this solution myself, I was definitely off
in the weeds when debugging :P