| Message ID | 20170227151436.18698-8-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Arnd, all, On Mon, Feb 27, 2017 at 03:14:18PM +0000, Lorenzo Pieralisi wrote: > The introduction of the pci_remap_cfgspace() interface allows > PCI host controller drivers to map PCI config space through a > dedicated kernel interface. Current PCI host controller drivers > use the devm_ioremap_* Devres interfaces to map PCI configuration > space regions so in order to update them to the new > pci_remap_cfgspace() mapping interface a new set of Devres interfaces > should be implemented so that PCI host controller drivers can make > use of them. > > Introduce two new functions in the PCI kernel layer and Devres > documentation: > > - devm_pci_remap_cfgspace() > - devm_pci_remap_cfg_resource() > > so that PCI host controller drivers can make use of them to map > PCI configuration space regions. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Bjorn Helgaas <bhelgaas@google.com> > --- > Documentation/driver-model/devres.txt | 6 ++- > drivers/pci/pci.c | 82 +++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 5 +++ > 3 files changed, 91 insertions(+), 2 deletions(-) > > diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt > index bf34d5b..e72587f 100644 > --- a/Documentation/driver-model/devres.txt > +++ b/Documentation/driver-model/devres.txt > @@ -342,8 +342,10 @@ PER-CPU MEM > devm_free_percpu() > > PCI > - pcim_enable_device() : after success, all PCI ops become managed > - pcim_pin_device() : keep PCI device enabled after release > + devm_pci_remap_cfgspace() : ioremap PCI configuration space > + devm_pci_remap_cfg_resource() : ioremap PCI configuration space resource > + pcim_enable_device() : after success, all PCI ops become managed > + pcim_pin_device() : keep PCI device enabled after release > > PHY > devm_usb_get_phy() > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index bfb3c6e..1e435c2 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3401,6 +3401,88 @@ void pci_unmap_iospace(struct resource *res) > #endif > } > > +/** > + * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace() > + * @dev: Generic device to remap IO address for > + * @offset: BUS offset to map > + * @size: Size of map > + * > + * Managed pci_remap_cfgspace(). Map is automatically unmapped on driver > + * detach. > + */ > +void __iomem *devm_pci_remap_cfgspace(struct device *dev, > + resource_size_t offset, > + resource_size_t size) > +{ > + void __iomem **ptr, *addr; > + > + ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return NULL; > + > + addr = pci_remap_cfgspace(offset, size); Provided this series is acceptable as-is, I noticed that I have to add a #define for pci_remap_cfgspace() on all arches that do not include asm-generic/io.h in their asm/io.h, I missed that, please shout if you see an easier way to implement a pci_remap_cfgspace() fall-back to ioremap_nocache() on all arches that do not override it. Thanks ! Lorenzo > + if (addr) { > + *ptr = addr; > + devres_add(dev, ptr); > + } else > + devres_free(ptr); > + > + return addr; > +} > +EXPORT_SYMBOL(devm_pci_remap_cfgspace); > + > +/** > + * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource > + * @dev: generic device to handle the resource for > + * @res: configuration space resource to be handled > + * > + * Checks that a resource is a valid memory region, requests the memory > + * region and ioremaps with pci_remap_cfgspace() API that ensures the > + * proper PCI configuration space memory attributes are guaranteed. > + * > + * All operations are managed and will be undone on driver detach. > + * > + * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code > + * on failure. Usage example: > + * > + * res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + * base = devm_pci_remap_cfg_resource(&pdev->dev, res); > + * if (IS_ERR(base)) > + * return PTR_ERR(base); > + */ > +void __iomem *devm_pci_remap_cfg_resource(struct device *dev, > + struct resource *res) > +{ > + resource_size_t size; > + const char *name; > + void __iomem *dest_ptr; > + > + BUG_ON(!dev); > + > + if (!res || resource_type(res) != IORESOURCE_MEM) { > + dev_err(dev, "invalid resource\n"); > + return IOMEM_ERR_PTR(-EINVAL); > + } > + > + size = resource_size(res); > + name = res->name ?: dev_name(dev); > + > + if (!devm_request_mem_region(dev, res->start, size, name)) { > + dev_err(dev, "can't request region for resource %pR\n", res); > + return IOMEM_ERR_PTR(-EBUSY); > + } > + > + dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size); > + if (!dest_ptr) { > + dev_err(dev, "ioremap failed for resource %pR\n", res); > + devm_release_mem_region(dev, res->start, size); > + dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > + } > + > + return dest_ptr; > +} > +EXPORT_SYMBOL(devm_pci_remap_cfg_resource); > + > static void __pci_set_master(struct pci_dev *dev, bool enable) > { > u16 old_cmd, cmd; > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 282ed32..5a3588b 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1199,6 +1199,11 @@ unsigned long pci_address_to_pio(phys_addr_t addr); > phys_addr_t pci_pio_to_address(unsigned long pio); > int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr); > void pci_unmap_iospace(struct resource *res); > +void __iomem *devm_pci_remap_cfgspace(struct device *dev, > + resource_size_t offset, > + resource_size_t size); > +void __iomem *devm_pci_remap_cfg_resource(struct device *dev, > + struct resource *res); > > static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar) > { > -- > 2.10.0 >
On Mon, Feb 27, 2017 at 5:14 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > The introduction of the pci_remap_cfgspace() interface allows > PCI host controller drivers to map PCI config space through a > dedicated kernel interface. Current PCI host controller drivers > use the devm_ioremap_* Devres interfaces to map PCI configuration > space regions so in order to update them to the new > pci_remap_cfgspace() mapping interface a new set of Devres interfaces > should be implemented so that PCI host controller drivers can make > use of them. > > Introduce two new functions in the PCI kernel layer and Devres > documentation: > > - devm_pci_remap_cfgspace() > - devm_pci_remap_cfg_resource() > > so that PCI host controller drivers can make use of them to map > PCI configuration space regions. Wouldn't you like to be consistent with current PCI API, i.e.: 1. devm_*() functions called pcim_*() in PCI. 2. If you may notice there is no separate pcim_*map*() stuff, they are dynamically adapting to the case. ?
Hi Andy, On Thu, Mar 02, 2017 at 01:54:42AM +0200, Andy Shevchenko wrote: > On Mon, Feb 27, 2017 at 5:14 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > The introduction of the pci_remap_cfgspace() interface allows > > PCI host controller drivers to map PCI config space through a > > dedicated kernel interface. Current PCI host controller drivers > > use the devm_ioremap_* Devres interfaces to map PCI configuration > > space regions so in order to update them to the new > > pci_remap_cfgspace() mapping interface a new set of Devres interfaces > > should be implemented so that PCI host controller drivers can make > > use of them. > > > > Introduce two new functions in the PCI kernel layer and Devres > > documentation: > > > > - devm_pci_remap_cfgspace() > > - devm_pci_remap_cfg_resource() > > > > so that PCI host controller drivers can make use of them to map > > PCI configuration space regions. > > Wouldn't you like to be consistent with current PCI API, i.e.: > 1. devm_*() functions called pcim_*() in PCI. I thought about that and did not do it because here we are remapping resources that are _not_ PCI bus resources (ie it is not PCI BARs we are remapping), keeping the devm_* prefix would be more consistent to the typical device drivers remapping functions pattern (ie a typical PCI host controller driver would mix devm_ and pcim_ calls which is a bit hard to parse), that was my rationale. I am not too fussed about that either way, I am happy to update it to pcim_* though, it is Bjorn/Arnd's decision. > 2. If you may notice there is no separate pcim_*map*() stuff, they are > dynamically adapting to the case. I do not understand what you mean here I would ask you to elaborate a bit more please so that I can do something about it. Thanks ! Lorenzo
On Thu, Mar 2, 2017 at 2:05 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Thu, Mar 02, 2017 at 01:54:42AM +0200, Andy Shevchenko wrote: >> On Mon, Feb 27, 2017 at 5:14 PM, Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> wrote: +Cc: Tejun, who is initial author of PCI managed resources implementation. > I thought about that and did not do it because here we are remapping > resources that are _not_ PCI bus resources (ie it is not PCI BARs we > are remapping), keeping the devm_* prefix would be more consistent > to the typical device drivers remapping functions pattern (ie a > typical PCI host controller driver would mix devm_ and pcim_ calls > which is a bit hard to parse), that was my rationale. > > I am not too fussed about that either way, I am happy to update it to > pcim_* though, it is Bjorn/Arnd's decision. I would vote for pcim_*() variant. >> 2. If you may notice there is no separate pcim_*map*() stuff, they are >> dynamically adapting to the case. > > I do not understand what you mean here I would ask you to elaborate > a bit more please so that I can do something about it. Oh, sorry, there are two examples currently, i.e. pci_enable_msi()/pci_enable_msix() and pci_request_region*() which has no "m" in the name, but are managed on release by pcim_release(). Some developers consider this as a bad idea, but so far no patch has been sent to introduce pcim_*() variants of those. So, regarding to your stuff, I would stick with "pcim" prefix.
Hello, On Thu, Mar 02, 2017 at 02:50:00PM +0200, Andy Shevchenko wrote: > > I thought about that and did not do it because here we are remapping > > resources that are _not_ PCI bus resources (ie it is not PCI BARs we > > are remapping), keeping the devm_* prefix would be more consistent > > to the typical device drivers remapping functions pattern (ie a > > typical PCI host controller driver would mix devm_ and pcim_ calls > > which is a bit hard to parse), that was my rationale. > > > > I am not too fussed about that either way, I am happy to update it to > > pcim_* though, it is Bjorn/Arnd's decision. > > I would vote for pcim_*() variant. Me too, for brevity. > >> 2. If you may notice there is no separate pcim_*map*() stuff, they are > >> dynamically adapting to the case. > > > > I do not understand what you mean here I would ask you to elaborate > > a bit more please so that I can do something about it. > > Oh, sorry, there are two examples currently, i.e. > pci_enable_msi()/pci_enable_msix() and pci_request_region*() which has > no "m" in the name, but are managed on release by pcim_release(). > Some developers consider this as a bad idea, but so far no patch has > been sent to introduce pcim_*() variants of those. > > So, regarding to your stuff, I would stick with "pcim" prefix. Sounds good to me. Thanks.
On Thu, Mar 02, 2017 at 02:24:06PM -0500, Tejun Heo wrote: > Hello, > > On Thu, Mar 02, 2017 at 02:50:00PM +0200, Andy Shevchenko wrote: > > > I thought about that and did not do it because here we are remapping > > > resources that are _not_ PCI bus resources (ie it is not PCI BARs we > > > are remapping), keeping the devm_* prefix would be more consistent > > > to the typical device drivers remapping functions pattern (ie a > > > typical PCI host controller driver would mix devm_ and pcim_ calls > > > which is a bit hard to parse), that was my rationale. > > > > > > I am not too fussed about that either way, I am happy to update it to > > > pcim_* though, it is Bjorn/Arnd's decision. > > > > I would vote for pcim_*() variant. > > Me too, for brevity. devm_* is equally brief. Also, all existing pcim_*() functions take a struct pci_dev * as their first argument, because they operate on the PCI devices. However in this case the devm_pci_remap_*() functions do not operate on PCI devices. Rather they operate on the struct device that represents the PCI host bridge. Therefore I think devm_ is more appropriate here. Thierry
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt index bf34d5b..e72587f 100644 --- a/Documentation/driver-model/devres.txt +++ b/Documentation/driver-model/devres.txt @@ -342,8 +342,10 @@ PER-CPU MEM devm_free_percpu() PCI - pcim_enable_device() : after success, all PCI ops become managed - pcim_pin_device() : keep PCI device enabled after release + devm_pci_remap_cfgspace() : ioremap PCI configuration space + devm_pci_remap_cfg_resource() : ioremap PCI configuration space resource + pcim_enable_device() : after success, all PCI ops become managed + pcim_pin_device() : keep PCI device enabled after release PHY devm_usb_get_phy() diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index bfb3c6e..1e435c2 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3401,6 +3401,88 @@ void pci_unmap_iospace(struct resource *res) #endif } +/** + * devm_pci_remap_cfgspace - Managed pci_remap_cfgspace() + * @dev: Generic device to remap IO address for + * @offset: BUS offset to map + * @size: Size of map + * + * Managed pci_remap_cfgspace(). Map is automatically unmapped on driver + * detach. + */ +void __iomem *devm_pci_remap_cfgspace(struct device *dev, + resource_size_t offset, + resource_size_t size) +{ + void __iomem **ptr, *addr; + + ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return NULL; + + addr = pci_remap_cfgspace(offset, size); + if (addr) { + *ptr = addr; + devres_add(dev, ptr); + } else + devres_free(ptr); + + return addr; +} +EXPORT_SYMBOL(devm_pci_remap_cfgspace); + +/** + * devm_pci_remap_cfg_resource - check, request region and ioremap cfg resource + * @dev: generic device to handle the resource for + * @res: configuration space resource to be handled + * + * Checks that a resource is a valid memory region, requests the memory + * region and ioremaps with pci_remap_cfgspace() API that ensures the + * proper PCI configuration space memory attributes are guaranteed. + * + * All operations are managed and will be undone on driver detach. + * + * Returns a pointer to the remapped memory or an ERR_PTR() encoded error code + * on failure. Usage example: + * + * res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + * base = devm_pci_remap_cfg_resource(&pdev->dev, res); + * if (IS_ERR(base)) + * return PTR_ERR(base); + */ +void __iomem *devm_pci_remap_cfg_resource(struct device *dev, + struct resource *res) +{ + resource_size_t size; + const char *name; + void __iomem *dest_ptr; + + BUG_ON(!dev); + + if (!res || resource_type(res) != IORESOURCE_MEM) { + dev_err(dev, "invalid resource\n"); + return IOMEM_ERR_PTR(-EINVAL); + } + + size = resource_size(res); + name = res->name ?: dev_name(dev); + + if (!devm_request_mem_region(dev, res->start, size, name)) { + dev_err(dev, "can't request region for resource %pR\n", res); + return IOMEM_ERR_PTR(-EBUSY); + } + + dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size); + if (!dest_ptr) { + dev_err(dev, "ioremap failed for resource %pR\n", res); + devm_release_mem_region(dev, res->start, size); + dest_ptr = IOMEM_ERR_PTR(-ENOMEM); + } + + return dest_ptr; +} +EXPORT_SYMBOL(devm_pci_remap_cfg_resource); + static void __pci_set_master(struct pci_dev *dev, bool enable) { u16 old_cmd, cmd; diff --git a/include/linux/pci.h b/include/linux/pci.h index 282ed32..5a3588b 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1199,6 +1199,11 @@ unsigned long pci_address_to_pio(phys_addr_t addr); phys_addr_t pci_pio_to_address(unsigned long pio); int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr); void pci_unmap_iospace(struct resource *res); +void __iomem *devm_pci_remap_cfgspace(struct device *dev, + resource_size_t offset, + resource_size_t size); +void __iomem *devm_pci_remap_cfg_resource(struct device *dev, + struct resource *res); static inline pci_bus_addr_t pci_bus_address(struct pci_dev *pdev, int bar) {
The introduction of the pci_remap_cfgspace() interface allows PCI host controller drivers to map PCI config space through a dedicated kernel interface. Current PCI host controller drivers use the devm_ioremap_* Devres interfaces to map PCI configuration space regions so in order to update them to the new pci_remap_cfgspace() mapping interface a new set of Devres interfaces should be implemented so that PCI host controller drivers can make use of them. Introduce two new functions in the PCI kernel layer and Devres documentation: - devm_pci_remap_cfgspace() - devm_pci_remap_cfg_resource() so that PCI host controller drivers can make use of them to map PCI configuration space regions. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Jonathan Corbet <corbet@lwn.net> Cc: Bjorn Helgaas <bhelgaas@google.com> --- Documentation/driver-model/devres.txt | 6 ++- drivers/pci/pci.c | 82 +++++++++++++++++++++++++++++++++++ include/linux/pci.h | 5 +++ 3 files changed, 91 insertions(+), 2 deletions(-)