| Message ID | 16771871.sfJG2itVJN@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Jun 3, 2014 at 9:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: > The pci-rcar driver is enabled for compile tests, and this has > now shown that the driver cannot build without CONFIG_OF, > following the inclusion of f8f2fe7355fb "PCI: rcar: Use new OF > interrupt mapping when possible": > > drivers/built-in.o: In function `rcar_pci_map_irq': > :(.text+0x1cc7c): undefined reference to `of_irq_parse_and_map_pci' > pci/host/pcie-rcar.c: In function 'pci_dma_range_parser_init': > pci/host/pcie-rcar.c:875:2: error: implicit declaration of function 'of_n_addr_cells' [-Werror=implicit-function-declaration] > > As pointed out by Ben Dooks and Geert Uytterhoeven, this is actually > supposed to build fine, which we can achieve if we make the > declaration of of_irq_parse_and_map_pci conditional on CONFIG_OF > and provide an empty inline function otherwise, as we do for > a lot of other of interfaces. > > This lets us build the rcar_pci driver again without CONFIG_OF > for build testing. All platforms using this driver select OF, > so this doesn't change anything for the users. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > Cc: devicetree@vger.kernel.org > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Magnus Damm <damm@opensource.se> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: Ben Dooks <ben.dooks@codethink.co.uk> > Cc: linux-pci@vger.kernel.org > Cc: linux-sh@vger.kernel.org > ---- > > This replaces "[PATCH v2] of/irq: provide int of_irq_parse_and_map_pci > wrapper", since now the same driver requires additional interfaces. > We still want to be able to build the driver with CONFIG_OF disabled, > but now we need three functions instead of just one. > > Rob, Grant, can you apply this as a bug fix, or provide comments? > > diff --git a/include/linux/of.h b/include/linux/of.h > index 196b34c..7c29e6c 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -511,6 +511,9 @@ static inline struct device_node *of_get_cpu_node(int cpu, > return NULL; > } > > +static inline int of_n_addr_cells(struct device_node *np) { return 0; } > +static inline int of_n_size_cells(struct device_node *np) { return 0; } I'm fine with the rest, but I think these should always be used within some higher level function. I can't seem to find where this is used by rcar. BTW, why does rcar pci DT support fail to have any ranges property? Looking at one use in mvebu pci only confirms my position. An of_for_each_ranges helper would be useful in that case. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 03 June 2014 10:40:34 Rob Herring wrote: > > diff --git a/include/linux/of.h b/include/linux/of.h > > index 196b34c..7c29e6c 100644 > > --- a/include/linux/of.h > > +++ b/include/linux/of.h > > @@ -511,6 +511,9 @@ static inline struct device_node *of_get_cpu_node(int cpu, > > return NULL; > > } > > > > +static inline int of_n_addr_cells(struct device_node *np) { return 0; } > > +static inline int of_n_size_cells(struct device_node *np) { return 0; } > > I'm fine with the rest, but I think these should always be used within > some higher level function. Can you apply the rest already? > I can't seem to find where this is used by rcar. BTW, why does rcar > pci DT support fail to have any ranges property? > > Looking at one use in mvebu pci only confirms my position. An > of_for_each_ranges helper would be useful in that case. It's currently in the pci-next, the code using this looks like +static int pci_dma_range_parser_init(struct of_pci_range_parser *parser, + struct device_node *node) +{ + const int na = 3, ns = 2; + int rlen; + + parser->node = node; + parser->pna = of_n_addr_cells(node); + parser->np = parser->pna + na + ns; + + parser->range = of_get_property(node, "dma-ranges", &rlen); + if (!parser->range) + return -ENOENT; + + parser->end = parser->range + rlen / sizeof(__be32); + return 0; +} + +static int rcar_pcie_parse_map_dma_ranges(struct rcar_pcie *pcie, + struct device_node *np) +{ + struct of_pci_range range; + struct of_pci_range_parser parser; + int index = 0; + int err; + + if (pci_dma_range_parser_init(&parser, np)) + return -EINVAL; + + /* Get the dma-ranges from DT */ + for_each_of_pci_range(&parser, &range) { + u64 end = range.cpu_addr + range.size - 1; + dev_dbg(pcie->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n", + range.flags, range.cpu_addr, end, range.pci_addr); + + err = rcar_pcie_inbound_ranges(pcie, &range, &index); + if (err) + return err; + } + + return 0; +} Should the first function just be moved into of-pci.c? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 3, 2014 at 10:59 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 03 June 2014 10:40:34 Rob Herring wrote: >> > diff --git a/include/linux/of.h b/include/linux/of.h >> > index 196b34c..7c29e6c 100644 >> > --- a/include/linux/of.h >> > +++ b/include/linux/of.h >> > @@ -511,6 +511,9 @@ static inline struct device_node *of_get_cpu_node(int cpu, >> > return NULL; >> > } >> > >> > +static inline int of_n_addr_cells(struct device_node *np) { return 0; } >> > +static inline int of_n_size_cells(struct device_node *np) { return 0; } >> >> I'm fine with the rest, but I think these should always be used within >> some higher level function. > > Can you apply the rest already? Yes, sure. > >> I can't seem to find where this is used by rcar. BTW, why does rcar >> pci DT support fail to have any ranges property? >> >> Looking at one use in mvebu pci only confirms my position. An >> of_for_each_ranges helper would be useful in that case. > > It's currently in the pci-next, the code using this looks like > > +static int pci_dma_range_parser_init(struct of_pci_range_parser *parser, > + struct device_node *node) > +{ > + const int na = 3, ns = 2; > + int rlen; > + > + parser->node = node; > + parser->pna = of_n_addr_cells(node); > + parser->np = parser->pna + na + ns; > + > + parser->range = of_get_property(node, "dma-ranges", &rlen); > + if (!parser->range) > + return -ENOENT; > + > + parser->end = parser->range + rlen / sizeof(__be32); > + return 0; > +} > + > +static int rcar_pcie_parse_map_dma_ranges(struct rcar_pcie *pcie, > + struct device_node *np) > +{ > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + int index = 0; > + int err; > + > + if (pci_dma_range_parser_init(&parser, np)) > + return -EINVAL; > + > + /* Get the dma-ranges from DT */ > + for_each_of_pci_range(&parser, &range) { > + u64 end = range.cpu_addr + range.size - 1; > + dev_dbg(pcie->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n", > + range.flags, range.cpu_addr, end, range.pci_addr); > + > + err = rcar_pcie_inbound_ranges(pcie, &range, &index); > + if (err) > + return err; > + } > + > + return 0; > +} > > Should the first function just be moved into of-pci.c? Yes, and probably some refactoring with the "ranges" version of it too. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/06/14 16:40, Rob Herring wrote: > On Tue, Jun 3, 2014 at 9:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> The pci-rcar driver is enabled for compile tests, and this has >> now shown that the driver cannot build without CONFIG_OF, >> following the inclusion of f8f2fe7355fb "PCI: rcar: Use new OF >> interrupt mapping when possible": >> >> drivers/built-in.o: In function `rcar_pci_map_irq': >> :(.text+0x1cc7c): undefined reference to `of_irq_parse_and_map_pci' >> pci/host/pcie-rcar.c: In function 'pci_dma_range_parser_init': >> pci/host/pcie-rcar.c:875:2: error: implicit declaration of function 'of_n_addr_cells' [-Werror=implicit-function-declaration] >> >> As pointed out by Ben Dooks and Geert Uytterhoeven, this is actually >> supposed to build fine, which we can achieve if we make the >> declaration of of_irq_parse_and_map_pci conditional on CONFIG_OF >> and provide an empty inline function otherwise, as we do for >> a lot of other of interfaces. >> >> This lets us build the rcar_pci driver again without CONFIG_OF >> for build testing. All platforms using this driver select OF, >> so this doesn't change anything for the users. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> Cc: devicetree@vger.kernel.org >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Grant Likely <grant.likely@linaro.org> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> Cc: Magnus Damm <damm@opensource.se> >> Cc: Geert Uytterhoeven <geert@linux-m68k.org> >> Cc: Ben Dooks <ben.dooks@codethink.co.uk> >> Cc: linux-pci@vger.kernel.org >> Cc: linux-sh@vger.kernel.org >> ---- >> >> This replaces "[PATCH v2] of/irq: provide int of_irq_parse_and_map_pci >> wrapper", since now the same driver requires additional interfaces. >> We still want to be able to build the driver with CONFIG_OF disabled, >> but now we need three functions instead of just one. >> >> Rob, Grant, can you apply this as a bug fix, or provide comments? >> >> diff --git a/include/linux/of.h b/include/linux/of.h >> index 196b34c..7c29e6c 100644 >> --- a/include/linux/of.h >> +++ b/include/linux/of.h >> @@ -511,6 +511,9 @@ static inline struct device_node *of_get_cpu_node(int cpu, >> return NULL; >> } >> >> +static inline int of_n_addr_cells(struct device_node *np) { return 0; } >> +static inline int of_n_size_cells(struct device_node *np) { return 0; } > > I'm fine with the rest, but I think these should always be used within > some higher level function. > > I can't seem to find where this is used by rcar. BTW, why does rcar > pci DT support fail to have any ranges property? I think the driver provides all the necessary PCI information internally as it started off as a platform-driver.
On Wednesday 04 June 2014 10:58:15 Ben Dooks wrote: > On 03/06/14 16:40, Rob Herring wrote: > > On Tue, Jun 3, 2014 at 9:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >> This replaces "[PATCH v2] of/irq: provide int of_irq_parse_and_map_pci > >> wrapper", since now the same driver requires additional interfaces. > >> We still want to be able to build the driver with CONFIG_OF disabled, > >> but now we need three functions instead of just one. > >> > >> Rob, Grant, can you apply this as a bug fix, or provide comments? > >> > >> diff --git a/include/linux/of.h b/include/linux/of.h > >> index 196b34c..7c29e6c 100644 > >> --- a/include/linux/of.h > >> +++ b/include/linux/of.h > >> @@ -511,6 +511,9 @@ static inline struct device_node *of_get_cpu_node(int cpu, > >> return NULL; > >> } > >> > >> +static inline int of_n_addr_cells(struct device_node *np) { return 0; } > >> +static inline int of_n_size_cells(struct device_node *np) { return 0; } > > > > I'm fine with the rest, but I think these should always be used within > > some higher level function. > > > > I can't seem to find where this is used by rcar. BTW, why does rcar > > pci DT support fail to have any ranges property? > > I think the driver provides all the necessary PCI information > internally as it started off as a platform-driver. Actually it gets the memory resource from the 'reg' property instead of parsing the 'ranges' property, which of course is not compliant with the generic PCI binding. It also sets up an I/O resource that is located at the same location as the memory resource, which is just a bug but might be used to work around problems of the PCI core when no I/O resource is present. I think that should be fixed. I also see that the ranges parser in the pcie-rcar driver still gets I/O space wrong, we should probably move that to a common range parser once the arm64 pci implementation is in place and we can share more of the common helpers. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/of.h b/include/linux/of.h index 196b34c..7c29e6c 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -511,6 +511,9 @@ static inline struct device_node *of_get_cpu_node(int cpu, return NULL; } +static inline int of_n_addr_cells(struct device_node *np) { return 0; } +static inline int of_n_size_cells(struct device_node *np) { return 0; } + static inline int of_property_read_u64(const struct device_node *np, const char *propname, u64 *out_value) { diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 1a1f5ff..dde3a4a 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -6,14 +6,44 @@ struct pci_dev; struct of_phandle_args; -int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq); -int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin); - struct device_node; + +#ifdef CONFIG_OF +int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq); struct device_node *of_pci_find_child_device(struct device_node *parent, unsigned int devfn); int of_pci_get_devfn(struct device_node *np); +int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin); int of_pci_parse_bus_range(struct device_node *node, struct resource *res); +#else +static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq) +{ + return 0; +} + +static inline struct device_node *of_pci_find_child_device(struct device_node *parent, + unsigned int devfn) +{ + return NULL; +} + +static inline int of_pci_get_devfn(struct device_node *np) +{ + return -EINVAL; +} + +static inline int +of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin) +{ + return 0; +} + +static inline int +of_pci_parse_bus_range(struct device_node *node, struct resource *res) +{ + return -EINVAL; +} +#endif #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI) int of_pci_msi_chip_add(struct msi_chip *chip);
The pci-rcar driver is enabled for compile tests, and this has now shown that the driver cannot build without CONFIG_OF, following the inclusion of f8f2fe7355fb "PCI: rcar: Use new OF interrupt mapping when possible": drivers/built-in.o: In function `rcar_pci_map_irq': :(.text+0x1cc7c): undefined reference to `of_irq_parse_and_map_pci' pci/host/pcie-rcar.c: In function 'pci_dma_range_parser_init': pci/host/pcie-rcar.c:875:2: error: implicit declaration of function 'of_n_addr_cells' [-Werror=implicit-function-declaration] As pointed out by Ben Dooks and Geert Uytterhoeven, this is actually supposed to build fine, which we can achieve if we make the declaration of of_irq_parse_and_map_pci conditional on CONFIG_OF and provide an empty inline function otherwise, as we do for a lot of other of interfaces. This lets us build the rcar_pci driver again without CONFIG_OF for build testing. All platforms using this driver select OF, so this doesn't change anything for the users. Signed-off-by: Arnd Bergmann <arnd@arndb.de> Cc: devicetree@vger.kernel.org Cc: Rob Herring <robh+dt@kernel.org> Cc: Grant Likely <grant.likely@linaro.org> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Magnus Damm <damm@opensource.se> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Ben Dooks <ben.dooks@codethink.co.uk> Cc: linux-pci@vger.kernel.org Cc: linux-sh@vger.kernel.org ---- This replaces "[PATCH v2] of/irq: provide int of_irq_parse_and_map_pci wrapper", since now the same driver requires additional interfaces. We still want to be able to build the driver with CONFIG_OF disabled, but now we need three functions instead of just one. Rob, Grant, can you apply this as a bug fix, or provide comments? -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html