| Message ID | 1394025951-32438-7-git-send-email-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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?
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)?
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
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
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)