diff mbox

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

Message ID 1394025951-32438-7-git-send-email-l.stach@pengutronix.de (mailing list archive)
State Accepted
Commit f86b3e392780050e5907f1c0f3cb6c4cc05fd6bb
Headers show

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 <l.stach@pengutronix.de>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2: pass in parent dev to relevant functions, to make DT parsing
    work (spotted by Tim Harvey <tharvey@gateworks.com>)
---
 drivers/pci/host/pcie-designware.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Jason Gunthorpe March 5, 2014, 6:42 p.m. UTC | #1
On Wed, Mar 05, 2014 at 02:25:51PM +0100, Lucas Stach wrote:
> -	return pp->irq;
> +	irq = of_irq_parse_and_map_pci(dev, slot, pin);
> +	if (!irq)
> +		irq = pp->irq;

In light of the two bugs that Tim found, it might be wise to throw a
'dev_warn(FW_BUG "Missing DT interrupt mapping")' in the fall back
path, so it doesn't continue to silently cover up errors on the OF/DT
side..

Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jingoo Han March 6, 2014, 2:47 a.m. UTC | #2
On Wednesday, March 05, 2014 10:26 PM, 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 <l.stach@pengutronix.de>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

(+cc Mohit KUMAR, Richard Zhu, Pratyush Anand, Marek Vasut,
       Kishon Vijay Abraham I)

Acked-by: Jingoo Han <jg1.han@samsung.com>

It works properly on Exynos platform.
Thank you.

Best regards,
Jingoo Han

> ---
> v2: pass in parent dev to relevant functions, to make DT parsing
>     work (spotted by Tim Harvey <tharvey@gateworks.com>)
> ---
>  drivers/pci/host/pcie-designware.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 17ce88f79d2b..98c118e04dba 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/msi.h>
>  #include <linux/of_address.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/pci_regs.h>
>  #include <linux/types.h>
> @@ -492,7 +493,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
>  	dw_pci.nr_controllers = 1;
>  	dw_pci.private_data = (void **)&pp;
> 
> -	pci_common_init(&dw_pci);
> +	pci_common_init_dev(pp->dev, &dw_pci);
>  	pci_assign_unassigned_resources();
>  #ifdef CONFIG_PCI_DOMAINS
>  	dw_pci.domain++;
> @@ -725,7 +726,7 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> 
>  	if (pp) {
>  		pp->root_bus_nr = sys->busnr;
> -		bus = pci_scan_root_bus(NULL, sys->busnr, &dw_pcie_ops,
> +		bus = pci_scan_root_bus(pp->dev, sys->busnr, &dw_pcie_ops,
>  					sys, &sys->resources);
>  	} else {
>  		bus = NULL;
> @@ -738,8 +739,13 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>  static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>  {
>  	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
> +	int irq;
> 
> -	return pp->irq;
> +	irq = of_irq_parse_and_map_pci(dev, slot, pin);
> +	if (!irq)
> +		irq = pp->irq;
> +
> +	return irq;
>  }
> 
>  static void dw_pcie_add_bus(struct pci_bus *bus)
> --
> 1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marek Vasut March 6, 2014, 3:33 p.m. UTC | #3
On Thursday, March 06, 2014 at 03:47:03 AM, Jingoo Han wrote:
> On Wednesday, March 05, 2014 10:26 PM, 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 <l.stach@pengutronix.de>
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> (+cc Mohit KUMAR, Richard Zhu, Pratyush Anand, Marek Vasut,
>        Kishon Vijay Abraham I)
> 
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> 
> It works properly on Exynos platform.
> Thank you.

Looks reasonable,

Reviewed-by: Marek Vasut <marex@denx.de>

+CC Troy if he wants to test it on SL/N6X .

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tim Harvey March 13, 2014, 5:41 p.m. UTC | #4
On Thu, Mar 6, 2014 at 7:33 AM, Marek Vasut <marex@denx.de> wrote:
> On Thursday, March 06, 2014 at 03:47:03 AM, Jingoo Han wrote:
>> On Wednesday, March 05, 2014 10:26 PM, 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 <l.stach@pengutronix.de>
>> > Acked-by: Arnd Bergmann <arnd@arndb.de>
>>
>> (+cc Mohit KUMAR, Richard Zhu, Pratyush Anand, Marek Vasut,
>>        Kishon Vijay Abraham I)
>>
>> Acked-by: Jingoo Han <jg1.han@samsung.com>
>>
>> It works properly on Exynos platform.
>> Thank you.
>
> Looks reasonable,
>
> Reviewed-by: Marek Vasut <marex@denx.de>
>

I've tested this on the Gateworks Ventana products with P2P bridges
and with the patch I just submitted here:
http://thread.gmane.org/gmane.linux.kernel.pci/30017

Tested-by: Tim Harvey <tharvey@gateworks.com>

Tim
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 4, 2014, 5:03 p.m. UTC | #5
On Wed, Mar 05, 2014 at 11:42:19AM -0700, Jason Gunthorpe wrote:
> On Wed, Mar 05, 2014 at 02:25:51PM +0100, Lucas Stach wrote:
> > -	return pp->irq;
> > +	irq = of_irq_parse_and_map_pci(dev, slot, pin);
> > +	if (!irq)
> > +		irq = pp->irq;
> 
> In light of the two bugs that Tim found, it might be wise to throw a
> 'dev_warn(FW_BUG "Missing DT interrupt mapping")' in the fall back
> path, so it doesn't continue to silently cover up errors on the OF/DT
> side..

This sounds like a reasonable thing to do, but I didn't see a response to
this comment.  Should I merge it as-is, or do you want to add the message?
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 4, 2014, 5:05 p.m. UTC | #6
On Fri, Apr 04, 2014 at 11:03:41AM -0600, Bjorn Helgaas wrote:
> On Wed, Mar 05, 2014 at 11:42:19AM -0700, Jason Gunthorpe wrote:
> > On Wed, Mar 05, 2014 at 02:25:51PM +0100, Lucas Stach wrote:
> > > -	return pp->irq;
> > > +	irq = of_irq_parse_and_map_pci(dev, slot, pin);
> > > +	if (!irq)
> > > +		irq = pp->irq;
> > 
> > In light of the two bugs that Tim found, it might be wise to throw a
> > 'dev_warn(FW_BUG "Missing DT interrupt mapping")' in the fall back
> > path, so it doesn't continue to silently cover up errors on the OF/DT
> > side..
> 
> This sounds like a reasonable thing to do, but I didn't see a response to
> this comment.  Should I merge it as-is, or do you want to add the message?

Oh, and I suppose the same question applies to the other host drivers in
this series (tegra, rcar)?
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas Stach April 7, 2014, 8:38 a.m. UTC | #7
Hi Bjorn,

Am Freitag, den 04.04.2014, 11:05 -0600 schrieb Bjorn Helgaas:
> On Fri, Apr 04, 2014 at 11:03:41AM -0600, Bjorn Helgaas wrote:
> > On Wed, Mar 05, 2014 at 11:42:19AM -0700, Jason Gunthorpe wrote:
> > > On Wed, Mar 05, 2014 at 02:25:51PM +0100, Lucas Stach wrote:
> > > > -	return pp->irq;
> > > > +	irq = of_irq_parse_and_map_pci(dev, slot, pin);
> > > > +	if (!irq)
> > > > +		irq = pp->irq;
> > > 
> > > In light of the two bugs that Tim found, it might be wise to throw a
> > > 'dev_warn(FW_BUG "Missing DT interrupt mapping")' in the fall back
> > > path, so it doesn't continue to silently cover up errors on the OF/DT
> > > side..
> > 
> > This sounds like a reasonable thing to do, but I didn't see a response to
> > this comment.  Should I merge it as-is, or do you want to add the message?
> 
> Oh, and I suppose the same question applies to the other host drivers in
> this series (tegra, rcar)?

Please apply as-is. of_irq_parse_and_map_pci() already prints an error
message if it isn't able to establish the mapping, thus right before we
fall into the fallback path. So any the warning would be redundant.

Regards,
Lucas
Jingoo Han April 7, 2014, 9:13 a.m. UTC | #8
On Monday, April 07, 2014 5:39 PM, Lucas Stach wrote:
> Am Freitag, den 04.04.2014, 11:05 -0600 schrieb Bjorn Helgaas:
> > On Fri, Apr 04, 2014 at 11:03:41AM -0600, Bjorn Helgaas wrote:
> > > On Wed, Mar 05, 2014 at 11:42:19AM -0700, Jason Gunthorpe wrote:
> > > > On Wed, Mar 05, 2014 at 02:25:51PM +0100, Lucas Stach wrote:
> > > > > -	return pp->irq;
> > > > > +	irq = of_irq_parse_and_map_pci(dev, slot, pin);
> > > > > +	if (!irq)
> > > > > +		irq = pp->irq;
> > > >
> > > > In light of the two bugs that Tim found, it might be wise to throw a
> > > > 'dev_warn(FW_BUG "Missing DT interrupt mapping")' in the fall back
> > > > path, so it doesn't continue to silently cover up errors on the OF/DT
> > > > side..
> > >
> > > This sounds like a reasonable thing to do, but I didn't see a response to
> > > this comment.  Should I merge it as-is, or do you want to add the message?
> >
> > Oh, and I suppose the same question applies to the other host drivers in
> > this series (tegra, rcar)?
> 
> Please apply as-is. of_irq_parse_and_map_pci() already prints an error
> message if it isn't able to establish the mapping, thus right before we
> fall into the fallback path. So any the warning would be redundant.

+1

I agree with Lucas Stach's opinion. of_irq_parse_and_map_pci() already
prints an error as below. So, additional warning message of host controller
looks superfluous.

	ret = of_irq_parse_pci(dev, &oirq);
	if (ret) {
		dev_err(&dev->dev, "of_irq_parse_pci() failed with rc=%d\n", ret);
		return 0; /* Proper return code 0 == NO_IRQ */


Best regards,
Jingoo Han

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

Patch

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 17ce88f79d2b..98c118e04dba 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -17,6 +17,7 @@ 
 #include <linux/module.h>
 #include <linux/msi.h>
 #include <linux/of_address.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/pci_regs.h>
 #include <linux/types.h>
@@ -492,7 +493,7 @@  int __init dw_pcie_host_init(struct pcie_port *pp)
 	dw_pci.nr_controllers = 1;
 	dw_pci.private_data = (void **)&pp;
 
-	pci_common_init(&dw_pci);
+	pci_common_init_dev(pp->dev, &dw_pci);
 	pci_assign_unassigned_resources();
 #ifdef CONFIG_PCI_DOMAINS
 	dw_pci.domain++;
@@ -725,7 +726,7 @@  static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 
 	if (pp) {
 		pp->root_bus_nr = sys->busnr;
-		bus = pci_scan_root_bus(NULL, sys->busnr, &dw_pcie_ops,
+		bus = pci_scan_root_bus(pp->dev, sys->busnr, &dw_pcie_ops,
 					sys, &sys->resources);
 	} else {
 		bus = NULL;
@@ -738,8 +739,13 @@  static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 static int dw_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 {
 	struct pcie_port *pp = sys_to_pcie(dev->bus->sysdata);
+	int irq;
 
-	return pp->irq;
+	irq = of_irq_parse_and_map_pci(dev, slot, pin);
+	if (!irq)
+		irq = pp->irq;
+
+	return irq;
 }
 
 static void dw_pcie_add_bus(struct pci_bus *bus)