| Message ID | 1411003825-21521-11-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Sep 18, 2014 at 02:30:25AM +0100, Liviu Dudau wrote: > Introduce a default implementation for remapping PCI bus I/O resources > onto the CPU address space. Architectures with special needs may > provide their own version, but most should be able to use this one. I see that this is used by Tanmay's APM X-Gene PCIe host controller driver. Since it's not used in this series, it'd be nice to mention where it *will* be used. > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Rob Herring <robh+dt@kernel.org> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > --- > drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ > include/asm-generic/pgtable.h | 4 ++++ > include/linux/pci.h | 3 +++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 2c9ac70..654b44c 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2704,6 +2704,39 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name) > } > EXPORT_SYMBOL(pci_request_regions_exclusive); > > +/** > + * pci_remap_iospace - Remap the memory mapped I/O space > + * @res: Resource describing the I/O space > + * @phys_addr: physical address where the range will be mapped. > + * > + * Remap the memory mapped I/O space described by the @res > + * into the CPU physical address space. This comment doesn't seem quite right. I think the space is already in the CPU physical address space. ioremap() and friends normally map existing physical space into the *virtual* address space, i.e., they create mappings from a virtual address to a physical address. > Only architectures > + * that have memory mapped IO defined (and hence PCI_IOBASE) > + * should call this function. > + */ > +int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr) > +{ > + int err = -ENODEV; > + > +#ifdef PCI_IOBASE > + if (!(res->flags & IORESOURCE_IO)) > + return -EINVAL; > + > + if (res->end > IO_SPACE_LIMIT) > + return -EINVAL; > + PCI_IOBASE is a virtual address. So PCI_IOBASE + res->start is also a virtual address (only for IORESOURCE_IO). Since res->start is normally a *physical* address, I think it would be less confusing to do something like this: vaddr = PCI_IOBASE + res->start; ioremap_page_range(vaddr, vaddr + resource_size(res), ...); so we have a hint that the first two ioremap_page_range() parameters are virtual addresses. It's also confusing that it uses "unsigned long" for the virtual addresses, when we usually use "void *". But that's out of scope for this patch. > + err = ioremap_page_range(res->start + (unsigned long)PCI_IOBASE, > + res->end + 1 + (unsigned long)PCI_IOBASE, > + phys_addr, pgprot_device(PAGE_KERNEL)); > +#else > + /* this architecture does not have memory mapped I/O space, > + so this function should never be called */ > + WARN_ON(1); > +#endif > + > + return err; > +} > + > static void __pci_set_master(struct pci_dev *dev, bool enable) > { > u16 old_cmd, cmd; > diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h > index 53b2acc..977e545 100644 > --- a/include/asm-generic/pgtable.h > +++ b/include/asm-generic/pgtable.h > @@ -249,6 +249,10 @@ static inline int pmd_same(pmd_t pmd_a, pmd_t pmd_b) > #define pgprot_writecombine pgprot_noncached > #endif > > +#ifndef pgprot_device > +#define pgprot_device pgprot_noncached > +#endif > + > /* > * When walking page tables, get the address of the next boundary, > * or the end address of the range if that comes earlier. Although no > diff --git a/include/linux/pci.h b/include/linux/pci.h > index a494e5d..fc8c529 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1100,6 +1100,9 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus, > resource_size_t), > void *alignf_data); > > + > +int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr); > + > static inline dma_addr_t pci_bus_address(struct pci_dev *pdev, int bar) > { > struct pci_bus_region region; > -- > 2.1.0 > -- 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 09/17/2014 08:30 PM, Liviu Dudau wrote: > Introduce a default implementation for remapping PCI bus I/O resources > onto the CPU address space. Architectures with special needs may > provide their own version, but most should be able to use this one. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Rob Herring <robh+dt@kernel.org> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> One nit below, otherwise: Reviewed-by: Rob Herring <robh@kernel.org> > --- > drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ > include/asm-generic/pgtable.h | 4 ++++ > include/linux/pci.h | 3 +++ > 3 files changed, 40 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 2c9ac70..654b44c 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -2704,6 +2704,39 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name) > } > EXPORT_SYMBOL(pci_request_regions_exclusive); > > +/** > + * pci_remap_iospace - Remap the memory mapped I/O space > + * @res: Resource describing the I/O space > + * @phys_addr: physical address where the range will be mapped. > + * > + * Remap the memory mapped I/O space described by the @res > + * into the CPU physical address space. Only architectures > + * that have memory mapped IO defined (and hence PCI_IOBASE) > + * should call this function. > + */ > +int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr) > +{ > + int err = -ENODEV; > + > +#ifdef PCI_IOBASE > + if (!(res->flags & IORESOURCE_IO)) > + return -EINVAL; > + > + if (res->end > IO_SPACE_LIMIT) > + return -EINVAL; > + > + err = ioremap_page_range(res->start + (unsigned long)PCI_IOBASE, > + res->end + 1 + (unsigned long)PCI_IOBASE, > + phys_addr, pgprot_device(PAGE_KERNEL)); > +#else > + /* this architecture does not have memory mapped I/O space, > + so this function should never be called */ > + WARN_ON(1); Printing what the comment says in the warning would be better than making the user look-up why they got a warning. 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 Friday 19 September 2014, Bjorn Helgaas wrote: > On Thu, Sep 18, 2014 at 02:30:25AM +0100, Liviu Dudau wrote: > PCI_IOBASE is a virtual address. So PCI_IOBASE + res->start is also a > virtual address (only for IORESOURCE_IO). > > Since res->start is normally a *physical* address, I think it would be less > confusing to do something like this: > > vaddr = PCI_IOBASE + res->start; > ioremap_page_range(vaddr, vaddr + resource_size(res), ...); > > so we have a hint that the first two ioremap_page_range() parameters are > virtual addresses. It's also confusing that it uses "unsigned long" for > the virtual addresses, when we usually use "void *". But that's out of > scope for this patch. Good idea. I think it will have to be (unsigned long)PCI_IOBASE above then, since ioremap_page_range takes a 'unsigned long' virtual address and PCI_IOBASE should really remain an __iomem pointer. 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 20.09.14 04:20:31, Arnd Bergmann wrote: > On Friday 19 September 2014, Bjorn Helgaas wrote: > > On Thu, Sep 18, 2014 at 02:30:25AM +0100, Liviu Dudau wrote: > > > PCI_IOBASE is a virtual address. So PCI_IOBASE + res->start is also a > > virtual address (only for IORESOURCE_IO). > > > > Since res->start is normally a *physical* address, I think it would be less > > confusing to do something like this: > > > > vaddr = PCI_IOBASE + res->start; > > ioremap_page_range(vaddr, vaddr + resource_size(res), ...); > > > > so we have a hint that the first two ioremap_page_range() parameters are > > virtual addresses. It's also confusing that it uses "unsigned long" for > > the virtual addresses, when we usually use "void *". But that's out of > > scope for this patch. > > Good idea. I think it will have to be (unsigned long)PCI_IOBASE above then, > since ioremap_page_range takes a 'unsigned long' virtual address and PCI_IOBASE > should really remain an __iomem pointer. Right, I see this warning here: drivers/pci/pci.c: In function ‘pci_remap_iospace’: drivers/pci/pci.c:2728:8: warning: assignment makes integer from pointer without a cast [enabled by default] -Robert -- 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/drivers/pci/pci.c b/drivers/pci/pci.c index 2c9ac70..654b44c 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -2704,6 +2704,39 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name) } EXPORT_SYMBOL(pci_request_regions_exclusive); +/** + * pci_remap_iospace - Remap the memory mapped I/O space + * @res: Resource describing the I/O space + * @phys_addr: physical address where the range will be mapped. + * + * Remap the memory mapped I/O space described by the @res + * into the CPU physical address space. Only architectures + * that have memory mapped IO defined (and hence PCI_IOBASE) + * should call this function. + */ +int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr) +{ + int err = -ENODEV; + +#ifdef PCI_IOBASE + if (!(res->flags & IORESOURCE_IO)) + return -EINVAL; + + if (res->end > IO_SPACE_LIMIT) + return -EINVAL; + + err = ioremap_page_range(res->start + (unsigned long)PCI_IOBASE, + res->end + 1 + (unsigned long)PCI_IOBASE, + phys_addr, pgprot_device(PAGE_KERNEL)); +#else + /* this architecture does not have memory mapped I/O space, + so this function should never be called */ + WARN_ON(1); +#endif + + return err; +} + static void __pci_set_master(struct pci_dev *dev, bool enable) { u16 old_cmd, cmd; diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h index 53b2acc..977e545 100644 --- a/include/asm-generic/pgtable.h +++ b/include/asm-generic/pgtable.h @@ -249,6 +249,10 @@ static inline int pmd_same(pmd_t pmd_a, pmd_t pmd_b) #define pgprot_writecombine pgprot_noncached #endif +#ifndef pgprot_device +#define pgprot_device pgprot_noncached +#endif + /* * When walking page tables, get the address of the next boundary, * or the end address of the range if that comes earlier. Although no diff --git a/include/linux/pci.h b/include/linux/pci.h index a494e5d..fc8c529 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1100,6 +1100,9 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus, resource_size_t), void *alignf_data); + +int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr); + static inline dma_addr_t pci_bus_address(struct pci_dev *pdev, int bar) { struct pci_bus_region region;