[v2,2/6] PCI: tegra: use new OF interrupt mapping when possible

Commit Message

Lucas Stach March 5, 2014, 1:25 p.m. UTC

This is the recommended method of doing the IRQ
mapping. For old devicetrees we fall back to the
previous practice.

Signed-off-by: Lucas Stach <[e-mail]>
Acked-by: Arnd Bergmann <[e-mail]>
---
 drivers/pci/host/pci-tegra.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Stephen Warren March 6, 2014, 5:36 p.m. UTC | #1

On 03/05/2014 06:25 AM, Lucas Stach wrote:
> This is the recommended method of doing the IRQ
> mapping. For old devicetrees we fall back to the
> previous practice.

Tested-by: Stephen Warren <[e-mail]>

I tested both with and without patch 1/6, and the PCIe-based NIC on
Beaver worked fine either way. Without patch 1/6, I do see:

pci 0000:00:01.0: of_irq_parse_pci() failed with rc=-22

... but that seems reasonable given that the DT that of_irq_parse_pci()
parses is missing, and did correctly trigger the fallback path, so
everything still worked.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lucas Stach March 6, 2014, 5:39 p.m. UTC | #2

Am Donnerstag, den 06.03.2014, 10:36 -0700 schrieb Stephen Warren:
> On 03/05/2014 06:25 AM, Lucas Stach wrote:
> > This is the recommended method of doing the IRQ
> > mapping. For old devicetrees we fall back to the
> > previous practice.
> 
> Tested-by: Stephen Warren <[e-mail]>
> 
> I tested both with and without patch 1/6, and the PCIe-based NIC on
> Beaver worked fine either way. Without patch 1/6, I do see:
> 
> pci 0000:00:01.0: of_irq_parse_pci() failed with rc=-22
> 
> ... but that seems reasonable given that the DT that of_irq_parse_pci()
> parses is missing, and did correctly trigger the fallback path, so
> everything still worked.

Yes, this should be normal. It spits this error for old DTs, but keeps
doing the right thing. I'm not sure if we should downgrade this to info
or dbg.

Regards,
Lucas

Arnd Bergmann March 7, 2014, 12:25 a.m. UTC | #3

On Thursday 06 March 2014, Lucas Stach wrote:
> Am Donnerstag, den 06.03.2014, 10:36 -0700 schrieb Stephen Warren:
> > On 03/05/2014 06:25 AM, Lucas Stach wrote:
> > > This is the recommended method of doing the IRQ
> > > mapping. For old devicetrees we fall back to the
> > > previous practice.
> > 
> > Tested-by: Stephen Warren <[e-mail]>
> > 
> > I tested both with and without patch 1/6, and the PCIe-based NIC on
> > Beaver worked fine either way. Without patch 1/6, I do see:
> > 
> > pci 0000:00:01.0: of_irq_parse_pci() failed with rc=-22
> > 
> > ... but that seems reasonable given that the DT that of_irq_parse_pci()
> > parses is missing, and did correctly trigger the fallback path, so
> > everything still worked.
> 
> Yes, this should be normal. It spits this error for old DTs, but keeps
> doing the right thing. I'm not sure if we should downgrade this to info
> or dbg.

No, I think printing an error like this is appropriate: it is an incentive
to update the dts files, but doesn't look too urgent as long as everything
still works.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jingoo Han March 7, 2014, 3:31 a.m. UTC | #4

On Friday, March 07, 2014 9:25 AM, Arnd Bergmann wrote:
> On Thursday 06 March 2014, Lucas Stach wrote:
> > Am Donnerstag, den 06.03.2014, 10:36 -0700 schrieb Stephen Warren:
> > > On 03/05/2014 06:25 AM, Lucas Stach wrote:
> > > > This is the recommended method of doing the IRQ
> > > > mapping. For old devicetrees we fall back to the
> > > > previous practice.
> > >
> > > Tested-by: Stephen Warren <[e-mail]>
> > >
> > > I tested both with and without patch 1/6, and the PCIe-based NIC on
> > > Beaver worked fine either way. Without patch 1/6, I do see:
> > >
> > > pci 0000:00:01.0: of_irq_parse_pci() failed with rc=-22
> > >
> > > ... but that seems reasonable given that the DT that of_irq_parse_pci()
> > > parses is missing, and did correctly trigger the fallback path, so
> > > everything still worked.
> >
> > Yes, this should be normal. It spits this error for old DTs, but keeps
> > doing the right thing. I'm not sure if we should downgrade this to info
> > or dbg.
> 
> No, I think printing an error like this is appropriate: it is an incentive
> to update the dts files, but doesn't look too urgent as long as everything
> still works.

+1

I agree with Arnd's opinion. I think that all dts files should be
updated properly. This error message can be the good way to do it.
Thank you.

Best regards,
Jingoo Han

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas April 4, 2014, 4:55 p.m. UTC | #5

On Wed, Mar 05, 2014 at 02:25:47PM +0100, Lucas Stach wrote:
> This is the recommended method of doing the IRQ
> mapping. For old devicetrees we fall back to the
> previous practice.
> 
> Signed-off-by: Lucas Stach <[e-mail]>
> Acked-by: Arnd Bergmann <[e-mail]>

Applied with Stephen's Tested-by to my pending/host-tegra branch.  I'll
rebase and rename it after v3.15-rc1, and I think we can squeeze it into
v3.15 shortly after that.  Thanks.

> ---
>  drivers/pci/host/pci-tegra.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 330f7e3a32dd..083cf37ca047 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -639,10 +639,15 @@ static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
>  static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
>  {
>  	struct tegra_pcie *pcie = sys_to_pcie(pdev->bus->sysdata);
> +	int irq;
>  
>  	tegra_cpuidle_pcie_irqs_in_use();
>  
> -	return pcie->irq;
> +	irq = of_irq_parse_and_map_pci(pdev, slot, pin);
> +	if (!irq)
> +		irq = pcie->irq;
> +
> +	return irq;
>  }
>  
>  static void tegra_pcie_add_bus(struct pci_bus *bus)
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Srikanth Thokala April 11, 2014, 5:40 p.m. UTC | #6

Hi Lucas/Tanmay,

On Thu, Mar 6, 2014 at 11:09 PM, Lucas Stach <[e-mail]> wrote:
> Am Donnerstag, den 06.03.2014, 10:36 -0700 schrieb Stephen Warren:
>> On 03/05/2014 06:25 AM, Lucas Stach wrote:
>> > This is the recommended method of doing the IRQ
>> > mapping. For old devicetrees we fall back to the
>> > previous practice.
>>
>> Tested-by: Stephen Warren <[e-mail]>
>>
>> I tested both with and without patch 1/6, and the PCIe-based NIC on
>> Beaver worked fine either way. Without patch 1/6, I do see:
>>
>> pci 0000:00:01.0: of_irq_parse_pci() failed with rc=-22
>>
>> ... but that seems reasonable given that the DT that of_irq_parse_pci()
>> parses is missing, and did correctly trigger the fallback path, so
>> everything still worked.
>
> Yes, this should be normal. It spits this error for old DTs, but keeps
> doing the right thing. I'm not sure if we should downgrade this to info
> or dbg.

I see this error too on my setup (Xilinx PCIe Root Complex Driver),
https://lkml.org/lkml/2014/3/3/183

After digging into it, I see I need to override the API
pcibios_get_phb_of_node()
to pass my device node.  Is this the only possible way?

From the discussion in this thread, I feel am missing something in the dt node.
Below is my dt node, please let me know if you see any issues.

+++++++++++++++++++++++
pcie{
   reg = <XXXX>;
   ranges = <YYYY>;
   interrupts = <0 52 4>;

   pcie_intc: interrupt-controller {
       interrupt-controller;
       #address-cells = <0>;
       #interrupt-cells = <1>;
   }

   interrupt-map-mask = <0 0 0 7>;
   interrupt-map = <0 0 0 1 &pcie_intc 1>,
                          <0 0 0 2 &pcie_intc 2>,
                          <0 0 0 3 &pcie_intc 3>,
                          <0 0 0 4 &pcie_intc 4>;
};

Thanks in advance,
Srikanth.

>
> Regards,
> Lucas
> --
> Pengutronix e.K.                           | Lucas Stach                 |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: [phone]-5076 |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe April 11, 2014, 8:41 p.m. UTC | #7

On Fri, Apr 11, 2014 at 11:10:59PM +0530, Srikanth Thokala wrote:

> I see this error too on my setup (Xilinx PCIe Root Complex Driver),
> https://lkml.org/lkml/2014/3/3/183

> After digging into it, I see I need to override the API
> pcibios_get_phb_of_node()

No, as I pointed out before, the DT node comes in through
pci_scan_root_bus:

+static struct pci_bus __init *xilinx_pcie_scan_bus(int nr,
+				struct pci_sys_data *sys)
+{
+	struct xilinx_pcie_port *port = sys_to_pcie(sys);
+	struct pci_bus *bus;
+
+	if (port) {
+		port->root_busno = sys->busnr;
+		bus = pci_scan_root_bus(NULL, sys->busnr, &xilinx_pcie_ops,
                                       ^^^^^^

You can't pass NULL here and have DT work properly. 

See http://www.spinics.net/lists/arm-kernel/msg312392.html

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Srikanth Thokala April 14, 2014, 10:56 a.m. UTC | #8

On Sat, Apr 12, 2014 at 2:11 AM, Jason Gunthorpe
<[e-mail]> wrote:
> On Fri, Apr 11, 2014 at 11:10:59PM +0530, Srikanth Thokala wrote:
>
>> I see this error too on my setup (Xilinx PCIe Root Complex Driver),
>> https://lkml.org/lkml/2014/3/3/183
>
>> After digging into it, I see I need to override the API
>> pcibios_get_phb_of_node()
>
> No, as I pointed out before, the DT node comes in through
> pci_scan_root_bus:

Thanks Jason for the advice, it is working.

Srikanth

>
> +static struct pci_bus __init *xilinx_pcie_scan_bus(int nr,
> +                               struct pci_sys_data *sys)
> +{
> +       struct xilinx_pcie_port *port = sys_to_pcie(sys);
> +       struct pci_bus *bus;
> +
> +       if (port) {
> +               port->root_busno = sys->busnr;
> +               bus = pci_scan_root_bus(NULL, sys->busnr, &xilinx_pcie_ops,
>                                        ^^^^^^
>
> You can't pass NULL here and have DT work properly.
>
> See http://www.spinics.net/lists/arm-kernel/msg312392.html
>
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lucas Stach April 15, 2014, 10:07 a.m. UTC | #9

Hi Bjorn,

Am Freitag, den 04.04.2014, 10:55 -0600 schrieb Bjorn Helgaas:
> On Wed, Mar 05, 2014 at 02:25:47PM +0100, Lucas Stach wrote:
> > This is the recommended method of doing the IRQ
> > mapping. For old devicetrees we fall back to the
> > previous practice.
> > 
> > Signed-off-by: Lucas Stach <[e-mail]>
> > Acked-by: Arnd Bergmann <[e-mail]>
> 
> Applied with Stephen's Tested-by to my pending/host-tegra branch.  I'll
> rebase and rename it after v3.15-rc1, and I think we can squeeze it into
> v3.15 shortly after that.  Thanks.
> 

Are you still planning to push this into 3.15, or has this slipped to
3.16?

Regards,
Lucas

Bjorn Helgaas April 15, 2014, 6:30 p.m. UTC | #10

On Tue, Apr 15, 2014 at 12:07:34PM +0200, Lucas Stach wrote:
> Hi Bjorn,
> 
> Am Freitag, den 04.04.2014, 10:55 -0600 schrieb Bjorn Helgaas:
> > On Wed, Mar 05, 2014 at 02:25:47PM +0100, Lucas Stach wrote:
> > > This is the recommended method of doing the IRQ
> > > mapping. For old devicetrees we fall back to the
> > > previous practice.
> > > 
> > > Signed-off-by: Lucas Stach <[e-mail]>
> > > Acked-by: Arnd Bergmann <[e-mail]>
> > 
> > Applied with Stephen's Tested-by to my pending/host-tegra branch.  I'll
> > rebase and rename it after v3.15-rc1, and I think we can squeeze it into
> > v3.15 shortly after that.  Thanks.
> > 
> 
> Are you still planning to push this into 3.15, or has this slipped to
> 3.16?

Yes, I'm hoping to put them in v3.15.  I assume these actually
fix something, e.g., we need these changes to boot with new devicetrees, or
something?

The changelogs don't make it clear that these are fixes, and I want to heed
Linus' guidance: "Anyway, because -rc1 is already pretty darn big, I do
*not* want to hear about 'sorry this missed the window, can I still sneak
in'.  Fixes only."

I should have applied these sooner to make the merge window; I apologize
for that.  Anyway, if you outline what these fix, I'll update the
changelogs in my tree.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lucas Stach April 16, 2014, 8:20 a.m. UTC | #11

Am Dienstag, den 15.04.2014, 12:30 -0600 schrieb Bjorn Helgaas:
> On Tue, Apr 15, 2014 at 12:07:34PM +0200, Lucas Stach wrote:
> > Hi Bjorn,
> > 
> > Am Freitag, den 04.04.2014, 10:55 -0600 schrieb Bjorn Helgaas:
> > > On Wed, Mar 05, 2014 at 02:25:47PM +0100, Lucas Stach wrote:
> > > > This is the recommended method of doing the IRQ
> > > > mapping. For old devicetrees we fall back to the
> > > > previous practice.
> > > > 
> > > > Signed-off-by: Lucas Stach <[e-mail]>
> > > > Acked-by: Arnd Bergmann <[e-mail]>
> > > 
> > > Applied with Stephen's Tested-by to my pending/host-tegra branch.  I'll
> > > rebase and rename it after v3.15-rc1, and I think we can squeeze it into
> > > v3.15 shortly after that.  Thanks.
> > > 
> > 
> > Are you still planning to push this into 3.15, or has this slipped to
> > 3.16?
> 
> Yes, I'm hoping to put them in v3.15.  I assume these actually
> fix something, e.g., we need these changes to boot with new devicetrees, or
> something?
> 
> The changelogs don't make it clear that these are fixes, and I want to heed
> Linus' guidance: "Anyway, because -rc1 is already pretty darn big, I do
> *not* want to hear about 'sorry this missed the window, can I still sneak
> in'.  Fixes only."
> 
> I should have applied these sooner to make the merge window; I apologize
> for that.  Anyway, if you outline what these fix, I'll update the
> changelogs in my tree.
> 
Actually they are a bit on the fence.

The i.MX and thus the designware patch actually fixes wrong behavior,
where all PCI legacy interrupts would be mapped to a single GIC
interrupt, which would leave INT B,C,D nonfunctional on i.MX.

The others only make DT interrupt mapping functional for all drivers, so
they would be useful if you need to remap interrupts across bridges or
something. But apparently nobody had the need to to this on platforms
other than i.MX until now, so those patches only fix a theoretical
issue.

Regards,
Lucas

Bjorn Helgaas April 16, 2014, 4:29 p.m. UTC | #12

On Wed, Apr 16, 2014 at 10:20:45AM +0200, Lucas Stach wrote:
> Am Dienstag, den 15.04.2014, 12:30 -0600 schrieb Bjorn Helgaas:
> > On Tue, Apr 15, 2014 at 12:07:34PM +0200, Lucas Stach wrote:
> > > Hi Bjorn,
> > > 
> > > Am Freitag, den 04.04.2014, 10:55 -0600 schrieb Bjorn Helgaas:
> > > > On Wed, Mar 05, 2014 at 02:25:47PM +0100, Lucas Stach wrote:
> > > > > This is the recommended method of doing the IRQ
> > > > > mapping. For old devicetrees we fall back to the
> > > > > previous practice.
> > > > > 
> > > > > Signed-off-by: Lucas Stach <[e-mail]>
> > > > > Acked-by: Arnd Bergmann <[e-mail]>
> > > > 
> > > > Applied with Stephen's Tested-by to my pending/host-tegra branch.  I'll
> > > > rebase and rename it after v3.15-rc1, and I think we can squeeze it into
> > > > v3.15 shortly after that.  Thanks.
> > > > 
> > > 
> > > Are you still planning to push this into 3.15, or has this slipped to
> > > 3.16?
> > 
> > Yes, I'm hoping to put them in v3.15.  I assume these actually
> > fix something, e.g., we need these changes to boot with new devicetrees, or
> > something?
> > 
> > The changelogs don't make it clear that these are fixes, and I want to heed
> > Linus' guidance: "Anyway, because -rc1 is already pretty darn big, I do
> > *not* want to hear about 'sorry this missed the window, can I still sneak
> > in'.  Fixes only."
> > 
> > I should have applied these sooner to make the merge window; I apologize
> > for that.  Anyway, if you outline what these fix, I'll update the
> > changelogs in my tree.
> > 
> Actually they are a bit on the fence.
> 
> The i.MX and thus the designware patch actually fixes wrong behavior,
> where all PCI legacy interrupts would be mapped to a single GIC
> interrupt, which would leave INT B,C,D nonfunctional on i.MX.
> 
> The others only make DT interrupt mapping functional for all drivers, so
> they would be useful if you need to remap interrupts across bridges or
> something. But apparently nobody had the need to to this on platforms
> other than i.MX until now, so those patches only fix a theoretical
> issue.

It sounds like the others should fix real problems; it's just that nobody
has actually tested relevant configurations yet.  I think that's fair game,
so I updated the changelogs and put them in my for-linus branch for v3.15.

This includes the designware, rcar, and tegra patches.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 330f7e3a32dd..083cf37ca047 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -639,10 +639,15 @@  static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
 static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
 {
 	struct tegra_pcie *pcie = sys_to_pcie(pdev->bus->sysdata);
+	int irq;
 
 	tegra_cpuidle_pcie_irqs_in_use();
 
-	return pcie->irq;
+	irq = of_irq_parse_and_map_pci(pdev, slot, pin);
+	if (!irq)
+		irq = pcie->irq;
+
+	return irq;
 }
 
 static void tegra_pcie_add_bus(struct pci_bus *bus)