| Message ID | 1485241525-201782-3-git-send-email-yuanzhichang@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Jan 24, 2017 at 03:05:22PM +0800, zhichang.yuan wrote: > After indirect-IO is introduced, system must can assigned indirect-IO devices > with logical I/O ranges which are different from those for PCI I/O devices. > Otherwise, I/O accessors can't identify whether the I/O port is for memory > mapped I/O or indirect-IO. Maybe: We must assign logical I/O port space for indirect I/O such that the I/O accessors can tell whether a logical I/O port refers to memory- mapped I/O space or indirect I/O space. > As current helper, pci_register_io_range(), is used for PCI I/O ranges > registration and translation, indirect-IO devices should also apply these > helpers to manage the I/O ranges. It will be easy to ensure the assigned > logical I/O ranges unique. > But for indirect-IO devices, there is no cpu address. The current > pci_register_io_range() can not work for this case. > > This patch makes some changes on the pci_register_io_range() to support the > I/O range registration with device's fwnode also. After this, the indirect-IO > devices can register the device-local I/O range to system logical I/O and > easily perform the translation between device-local I/O range and sytem > logical I/O range. > -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > +int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, > + resource_size_t size, unsigned long *port) Why is this __weak? It looks like it's been __weak since its introduction by 41f8bba7f555 ("of/pci: Add pci_register_io_range() and pci_pio_to_address()"), but I don't see any other implementations of it. Can you add a patch that does nothing but make this non-weak? > +#else > + /* > + * powerpc and microblaze have their own registration, > + * just look up the value here Can you include a pointer to the powerpc and microblaze registration code here? It's conceivable that somebody could generalize this enough to support powerpc and microblaze as well. > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -34,6 +34,9 @@ > > #include <linux/pci_ids.h> > > +/* the macro below flags an invalid cpu address > + * and is used by IO special hosts */ s/cpu/CPU/ Use conventional multi-line comment style: /* * IO_RANGE_IOEXT flags an invalid CPU address ... */ > +#define IO_RANGE_IOEXT (resource_size_t)(-1ull) And put this close to related things, e.g., pci_register_io_range(), instead of just dropping it in at the top of the file. > /* > * The PCI interface treats multi-function devices as independent > * devices. The slot/function address of each device is encoded > @@ -1197,8 +1200,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus, > resource_size_t), > void *alignf_data); > > - > -int pci_register_io_range(phys_addr_t addr, resource_size_t size); > +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, > + resource_size_t size, unsigned long *port); > 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); > -- > 1.9.1 > -- 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, Jan 24, 2017 at 03:05:22PM +0800, zhichang.yuan wrote: > After indirect-IO is introduced, system must can assigned indirect-IO devices > with logical I/O ranges which are different from those for PCI I/O devices. > Otherwise, I/O accessors can't identify whether the I/O port is for memory > mapped I/O or indirect-IO. > As current helper, pci_register_io_range(), is used for PCI I/O ranges > registration and translation, indirect-IO devices should also apply these > helpers to manage the I/O ranges. It will be easy to ensure the assigned > logical I/O ranges unique. > But for indirect-IO devices, there is no cpu address. The current > pci_register_io_range() can not work for this case. > > This patch makes some changes on the pci_register_io_range() to support the > I/O range registration with device's fwnode also. After this, the indirect-IO > devices can register the device-local I/O range to system logical I/O and > easily perform the translation between device-local I/O range and sytem > logical I/O range. > > Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> I had a couple trivial comments, but you can include my: Acked-by: Bjorn Helgaas <bhelgaas@google.com> # drivers/pci parts in your next revision. I don't know who you have in mind to merge this; it doesn't really touch much of PCI. But let me know if you need anything else from me. > --- > drivers/acpi/pci_root.c | 12 +++++------- > drivers/of/address.c | 8 ++------ > drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > include/linux/pci.h | 7 +++++-- > 4 files changed, 52 insertions(+), 19 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index bf601d4..6cadf05 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct device *dev, > } > } > > -static void acpi_pci_root_remap_iospace(struct resource_entry *entry) > +static void acpi_pci_root_remap_iospace(struct fwnode_handle *node, > + struct resource_entry *entry) > { > #ifdef PCI_IOBASE > struct resource *res = entry->res; > @@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct resource_entry *entry) > resource_size_t length = resource_size(res); > unsigned long port; > > - if (pci_register_io_range(cpu_addr, length)) > - goto err; > - > - port = pci_address_to_pio(cpu_addr); > - if (port == (unsigned long)-1) > + if (pci_register_io_range(node, cpu_addr, length, &port)) > goto err; > > res->start = port; > @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) > else { > resource_list_for_each_entry_safe(entry, tmp, list) { > if (entry->res->flags & IORESOURCE_IO) > - acpi_pci_root_remap_iospace(entry); > + acpi_pci_root_remap_iospace(&device->fwnode, > + entry); > > if (entry->res->flags & IORESOURCE_DISABLED) > resource_list_destroy_entry(entry); > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 02b2903..d85d228 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -2,6 +2,7 @@ > #define pr_fmt(fmt) "OF: " fmt > > #include <linux/device.h> > +#include <linux/fwnode.h> > #include <linux/io.h> > #include <linux/ioport.h> > #include <linux/module.h> > @@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range *range, > > if (res->flags & IORESOURCE_IO) { > unsigned long port; > - err = pci_register_io_range(range->cpu_addr, range->size); > + err = pci_register_io_range(&np->fwnode, range->cpu_addr, range->size, &port); > if (err) > goto invalid_range; > - port = pci_address_to_pio(range->cpu_addr); > - if (port == (unsigned long)-1) { > - err = -EINVAL; > - goto invalid_range; > - } > res->start = port; > } else { > if ((sizeof(resource_size_t) < 8) && > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index a881c0d..5289221 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3241,6 +3241,7 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name) > #ifdef PCI_IOBASE > struct io_range { > struct list_head list; > + struct fwnode_handle *node; > phys_addr_t start; > resource_size_t size; > }; > @@ -3253,7 +3254,8 @@ struct io_range { > * Record the PCI IO range (expressed as CPU physical address + size). > * Return a negative value if an error has occured, zero otherwise > */ > -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > +int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, > + resource_size_t size, unsigned long *port) > { > int err = 0; > > @@ -3261,10 +3263,31 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > struct io_range *range; > resource_size_t allocated_size = 0; > > + /* > + * As indirect-IO which support multiple bus instances is introduced, > + * the input 'addr' is probably not page-aligned. If the PCI I/O > + * ranges are registered after indirect-IO, there is risk that the > + * start logical PIO assigned to PCI I/O is not page-aligned. > + * This will cause some I/O subranges are not remapped or overlapped > + * in pci_remap_iospace() handling. > + */ > + WARN_ON(addr != IO_RANGE_IOEXT && !(addr & PAGE_MASK)); > + /* > + * MMIO will call ioremap, it is better to align size with PAGE_SIZE, > + * then the return linux virtual PIO is page-aligned. > + */ > + if (size & PAGE_MASK) > + size = PAGE_ALIGN(size); > + > /* check if the range hasn't been previously recorded */ > spin_lock(&io_range_lock); > list_for_each_entry(range, &io_range_list, list) { > - if (addr >= range->start && addr + size <= range->start + size) { > + if (node == range->node) > + goto end_register; > + > + if (addr != IO_RANGE_IOEXT && > + addr >= range->start && > + addr + size <= range->start + size) { > /* range already registered, bail out */ > goto end_register; > } > @@ -3290,6 +3313,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > goto end_register; > } > > + range->node = node; > range->start = addr; > range->size = size; > > @@ -3297,6 +3321,14 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > > end_register: > spin_unlock(&io_range_lock); > + > + *port = allocated_size; > +#else > + /* > + * powerpc and microblaze have their own registration, > + * just look up the value here > + */ > + *port = pci_address_to_pio(addr); > #endif > > return err; > @@ -3315,7 +3347,9 @@ phys_addr_t pci_pio_to_address(unsigned long pio) > > spin_lock(&io_range_lock); > list_for_each_entry(range, &io_range_list, list) { > - if (pio >= allocated_size && pio < allocated_size + range->size) { > + if (range->start != IO_RANGE_IOEXT && > + pio >= allocated_size && > + pio < allocated_size + range->size) { > address = range->start + pio - allocated_size; > break; > } > @@ -3336,7 +3370,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address) > > spin_lock(&io_range_lock); > list_for_each_entry(res, &io_range_list, list) { > - if (address >= res->start && address < res->start + res->size) { > + if (res->start != IO_RANGE_IOEXT && > + address >= res->start && > + address < res->start + res->size) { > addr = address - res->start + offset; > break; > } > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e2d1a12..8d91af8 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -34,6 +34,9 @@ > > #include <linux/pci_ids.h> > > +/* the macro below flags an invalid cpu address > + * and is used by IO special hosts */ > +#define IO_RANGE_IOEXT (resource_size_t)(-1ull) > /* > * The PCI interface treats multi-function devices as independent > * devices. The slot/function address of each device is encoded > @@ -1197,8 +1200,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus, > resource_size_t), > void *alignf_data); > > - > -int pci_register_io_range(phys_addr_t addr, resource_size_t size); > +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, > + resource_size_t size, unsigned long *port); > 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); > -- > 1.9.1 > -- 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 31/01/2017 00:10, Bjorn Helgaas wrote: > On Tue, Jan 24, 2017 at 03:05:22PM +0800, zhichang.yuan wrote: >> After indirect-IO is introduced, system must can assigned indirect-IO devices >> with logical I/O ranges which are different from those for PCI I/O devices. >> Otherwise, I/O accessors can't identify whether the I/O port is for memory >> mapped I/O or indirect-IO. > > Maybe: > > We must assign logical I/O port space for indirect I/O such that the > I/O accessors can tell whether a logical I/O port refers to memory- > mapped I/O space or indirect I/O space. > It's better >> As current helper, pci_register_io_range(), is used for PCI I/O ranges >> registration and translation, indirect-IO devices should also apply these >> helpers to manage the I/O ranges. It will be easy to ensure the assigned >> logical I/O ranges unique. >> But for indirect-IO devices, there is no cpu address. The current >> pci_register_io_range() can not work for this case. >> >> This patch makes some changes on the pci_register_io_range() to support the >> I/O range registration with device's fwnode also. After this, the indirect-IO >> devices can register the device-local I/O range to system logical I/O and >> easily perform the translation between device-local I/O range and sytem >> logical I/O range. > >> -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) >> +int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, >> + resource_size_t size, unsigned long *port) > > Why is this __weak? It looks like it's been __weak since its > introduction by 41f8bba7f555 ("of/pci: Add pci_register_io_range() and > pci_pio_to_address()"), but I don't see any other implementations of > it. > > Can you add a patch that does nothing but make this non-weak? > OK >> +#else >> + /* >> + * powerpc and microblaze have their own registration, >> + * just look up the value here > > Can you include a pointer to the powerpc and microblaze registration > code here? It's conceivable that somebody could generalize this > enough to support powerpc and microblaze as well. > It should be no problem >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -34,6 +34,9 @@ >> >> #include <linux/pci_ids.h> >> >> +/* the macro below flags an invalid cpu address >> + * and is used by IO special hosts */ > > s/cpu/CPU/ > OK > Use conventional multi-line comment style: > > /* > * IO_RANGE_IOEXT flags an invalid CPU address ... > */ > >> +#define IO_RANGE_IOEXT (resource_size_t)(-1ull) > > And put this close to related things, e.g., pci_register_io_range(), > instead of just dropping it in at the top of the file. OK > >> /* >> * The PCI interface treats multi-function devices as independent >> * devices. The slot/function address of each device is encoded >> @@ -1197,8 +1200,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus, >> resource_size_t), >> void *alignf_data); >> >> - >> -int pci_register_io_range(phys_addr_t addr, resource_size_t size); >> +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, >> + resource_size_t size, unsigned long *port); >> 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); >> -- >> 1.9.1 >> Thanks, John > > . > -- 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
Hi Rafael, Could you kindly check the minor changes to drivers/acpi/pci_root.c? It would also be appreciated if you could review the ACPI-relevant parts in lib/extio.c, in [PATCH V5 5/5] LPC: Add the ACPI LPC support Thanks, John On 24/01/2017 07:05, zhichang.yuan wrote: > After indirect-IO is introduced, system must can assigned indirect-IO devices > with logical I/O ranges which are different from those for PCI I/O devices. > Otherwise, I/O accessors can't identify whether the I/O port is for memory > mapped I/O or indirect-IO. > As current helper, pci_register_io_range(), is used for PCI I/O ranges > registration and translation, indirect-IO devices should also apply these > helpers to manage the I/O ranges. It will be easy to ensure the assigned > logical I/O ranges unique. > But for indirect-IO devices, there is no cpu address. The current > pci_register_io_range() can not work for this case. > > This patch makes some changes on the pci_register_io_range() to support the > I/O range registration with device's fwnode also. After this, the indirect-IO > devices can register the device-local I/O range to system logical I/O and > easily perform the translation between device-local I/O range and sytem > logical I/O range. > > Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/acpi/pci_root.c | 12 +++++------- > drivers/of/address.c | 8 ++------ > drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++---- > include/linux/pci.h | 7 +++++-- > 4 files changed, 52 insertions(+), 19 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index bf601d4..6cadf05 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct device *dev, > } > } > > -static void acpi_pci_root_remap_iospace(struct resource_entry *entry) > +static void acpi_pci_root_remap_iospace(struct fwnode_handle *node, > + struct resource_entry *entry) > { > #ifdef PCI_IOBASE > struct resource *res = entry->res; > @@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct resource_entry *entry) > resource_size_t length = resource_size(res); > unsigned long port; > > - if (pci_register_io_range(cpu_addr, length)) > - goto err; > - > - port = pci_address_to_pio(cpu_addr); > - if (port == (unsigned long)-1) > + if (pci_register_io_range(node, cpu_addr, length, &port)) > goto err; > > res->start = port; > @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) > else { > resource_list_for_each_entry_safe(entry, tmp, list) { > if (entry->res->flags & IORESOURCE_IO) > - acpi_pci_root_remap_iospace(entry); > + acpi_pci_root_remap_iospace(&device->fwnode, > + entry); > > if (entry->res->flags & IORESOURCE_DISABLED) > resource_list_destroy_entry(entry); > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 02b2903..d85d228 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -2,6 +2,7 @@ > #define pr_fmt(fmt) "OF: " fmt > > #include <linux/device.h> > +#include <linux/fwnode.h> > #include <linux/io.h> > #include <linux/ioport.h> > #include <linux/module.h> > @@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range *range, > > if (res->flags & IORESOURCE_IO) { > unsigned long port; > - err = pci_register_io_range(range->cpu_addr, range->size); > + err = pci_register_io_range(&np->fwnode, range->cpu_addr, range->size, &port); > if (err) > goto invalid_range; > - port = pci_address_to_pio(range->cpu_addr); > - if (port == (unsigned long)-1) { > - err = -EINVAL; > - goto invalid_range; > - } > res->start = port; > } else { > if ((sizeof(resource_size_t) < 8) && > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index a881c0d..5289221 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3241,6 +3241,7 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name) > #ifdef PCI_IOBASE > struct io_range { > struct list_head list; > + struct fwnode_handle *node; > phys_addr_t start; > resource_size_t size; > }; > @@ -3253,7 +3254,8 @@ struct io_range { > * Record the PCI IO range (expressed as CPU physical address + size). > * Return a negative value if an error has occured, zero otherwise > */ > -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > +int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, > + resource_size_t size, unsigned long *port) > { > int err = 0; > > @@ -3261,10 +3263,31 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > struct io_range *range; > resource_size_t allocated_size = 0; > > + /* > + * As indirect-IO which support multiple bus instances is introduced, > + * the input 'addr' is probably not page-aligned. If the PCI I/O > + * ranges are registered after indirect-IO, there is risk that the > + * start logical PIO assigned to PCI I/O is not page-aligned. > + * This will cause some I/O subranges are not remapped or overlapped > + * in pci_remap_iospace() handling. > + */ > + WARN_ON(addr != IO_RANGE_IOEXT && !(addr & PAGE_MASK)); > + /* > + * MMIO will call ioremap, it is better to align size with PAGE_SIZE, > + * then the return linux virtual PIO is page-aligned. > + */ > + if (size & PAGE_MASK) > + size = PAGE_ALIGN(size); > + > /* check if the range hasn't been previously recorded */ > spin_lock(&io_range_lock); > list_for_each_entry(range, &io_range_list, list) { > - if (addr >= range->start && addr + size <= range->start + size) { > + if (node == range->node) > + goto end_register; > + > + if (addr != IO_RANGE_IOEXT && > + addr >= range->start && > + addr + size <= range->start + size) { > /* range already registered, bail out */ > goto end_register; > } > @@ -3290,6 +3313,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > goto end_register; > } > > + range->node = node; > range->start = addr; > range->size = size; > > @@ -3297,6 +3321,14 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) > > end_register: > spin_unlock(&io_range_lock); > + > + *port = allocated_size; > +#else > + /* > + * powerpc and microblaze have their own registration, > + * just look up the value here > + */ > + *port = pci_address_to_pio(addr); > #endif > > return err; > @@ -3315,7 +3347,9 @@ phys_addr_t pci_pio_to_address(unsigned long pio) > > spin_lock(&io_range_lock); > list_for_each_entry(range, &io_range_list, list) { > - if (pio >= allocated_size && pio < allocated_size + range->size) { > + if (range->start != IO_RANGE_IOEXT && > + pio >= allocated_size && > + pio < allocated_size + range->size) { > address = range->start + pio - allocated_size; > break; > } > @@ -3336,7 +3370,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address) > > spin_lock(&io_range_lock); > list_for_each_entry(res, &io_range_list, list) { > - if (address >= res->start && address < res->start + res->size) { > + if (res->start != IO_RANGE_IOEXT && > + address >= res->start && > + address < res->start + res->size) { > addr = address - res->start + offset; > break; > } > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e2d1a12..8d91af8 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -34,6 +34,9 @@ > > #include <linux/pci_ids.h> > > +/* the macro below flags an invalid cpu address > + * and is used by IO special hosts */ > +#define IO_RANGE_IOEXT (resource_size_t)(-1ull) > /* > * The PCI interface treats multi-function devices as independent > * devices. The slot/function address of each device is encoded > @@ -1197,8 +1200,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus, > resource_size_t), > void *alignf_data); > > - > -int pci_register_io_range(phys_addr_t addr, resource_size_t size); > +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, > + resource_size_t size, unsigned long *port); > 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); >
On Thu, Feb 2, 2017 at 6:36 PM, John Garry <john.garry@huawei.com> wrote: > Hi Rafael, > > Could you kindly check the minor changes to drivers/acpi/pci_root.c? It > would also be appreciated if you could review the ACPI-relevant parts in > lib/extio.c, in [PATCH V5 5/5] LPC: Add the ACPI LPC support If you want patches to be reviewed from the ACPI perspective, please CC them to linux-acpi. I'm not the only person who may be interested in them. The change is pci_root.c looks OK to me, but it's Bjorn's call anyway. Thanks, Rafael > On 24/01/2017 07:05, zhichang.yuan wrote: >> >> After indirect-IO is introduced, system must can assigned indirect-IO >> devices >> with logical I/O ranges which are different from those for PCI I/O >> devices. >> Otherwise, I/O accessors can't identify whether the I/O port is for memory >> mapped I/O or indirect-IO. >> As current helper, pci_register_io_range(), is used for PCI I/O ranges >> registration and translation, indirect-IO devices should also apply these >> helpers to manage the I/O ranges. It will be easy to ensure the assigned >> logical I/O ranges unique. >> But for indirect-IO devices, there is no cpu address. The current >> pci_register_io_range() can not work for this case. >> >> This patch makes some changes on the pci_register_io_range() to support >> the >> I/O range registration with device's fwnode also. After this, the >> indirect-IO >> devices can register the device-local I/O range to system logical I/O and >> easily perform the translation between device-local I/O range and sytem >> logical I/O range. >> >> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> drivers/acpi/pci_root.c | 12 +++++------- >> drivers/of/address.c | 8 ++------ >> drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++---- >> include/linux/pci.h | 7 +++++-- >> 4 files changed, 52 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index bf601d4..6cadf05 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct >> device *dev, >> } >> } >> >> -static void acpi_pci_root_remap_iospace(struct resource_entry *entry) >> +static void acpi_pci_root_remap_iospace(struct fwnode_handle *node, >> + struct resource_entry *entry) >> { >> #ifdef PCI_IOBASE >> struct resource *res = entry->res; >> @@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct >> resource_entry *entry) >> resource_size_t length = resource_size(res); >> unsigned long port; >> >> - if (pci_register_io_range(cpu_addr, length)) >> - goto err; >> - >> - port = pci_address_to_pio(cpu_addr); >> - if (port == (unsigned long)-1) >> + if (pci_register_io_range(node, cpu_addr, length, &port)) >> goto err; >> >> res->start = port; >> @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct >> acpi_pci_root_info *info) >> else { >> resource_list_for_each_entry_safe(entry, tmp, list) { >> if (entry->res->flags & IORESOURCE_IO) >> - acpi_pci_root_remap_iospace(entry); >> + >> acpi_pci_root_remap_iospace(&device->fwnode, >> + entry); >> >> if (entry->res->flags & IORESOURCE_DISABLED) >> resource_list_destroy_entry(entry); >> diff --git a/drivers/of/address.c b/drivers/of/address.c >> index 02b2903..d85d228 100644 >> --- a/drivers/of/address.c >> +++ b/drivers/of/address.c >> @@ -2,6 +2,7 @@ >> #define pr_fmt(fmt) "OF: " fmt >> >> #include <linux/device.h> >> +#include <linux/fwnode.h> >> #include <linux/io.h> >> #include <linux/ioport.h> >> #include <linux/module.h> >> @@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range >> *range, >> >> if (res->flags & IORESOURCE_IO) { >> unsigned long port; >> - err = pci_register_io_range(range->cpu_addr, range->size); >> + err = pci_register_io_range(&np->fwnode, range->cpu_addr, >> range->size, &port); >> if (err) >> goto invalid_range; >> - port = pci_address_to_pio(range->cpu_addr); >> - if (port == (unsigned long)-1) { >> - err = -EINVAL; >> - goto invalid_range; >> - } >> res->start = port; >> } else { >> if ((sizeof(resource_size_t) < 8) && >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index a881c0d..5289221 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3241,6 +3241,7 @@ int pci_request_regions_exclusive(struct pci_dev >> *pdev, const char *res_name) >> #ifdef PCI_IOBASE >> struct io_range { >> struct list_head list; >> + struct fwnode_handle *node; >> phys_addr_t start; >> resource_size_t size; >> }; >> @@ -3253,7 +3254,8 @@ struct io_range { >> * Record the PCI IO range (expressed as CPU physical address + size). >> * Return a negative value if an error has occured, zero otherwise >> */ >> -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) >> +int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t >> addr, >> + resource_size_t size, unsigned long >> *port) >> { >> int err = 0; >> >> @@ -3261,10 +3263,31 @@ int __weak pci_register_io_range(phys_addr_t addr, >> resource_size_t size) >> struct io_range *range; >> resource_size_t allocated_size = 0; >> >> + /* >> + * As indirect-IO which support multiple bus instances is >> introduced, >> + * the input 'addr' is probably not page-aligned. If the PCI I/O >> + * ranges are registered after indirect-IO, there is risk that the >> + * start logical PIO assigned to PCI I/O is not page-aligned. >> + * This will cause some I/O subranges are not remapped or >> overlapped >> + * in pci_remap_iospace() handling. >> + */ >> + WARN_ON(addr != IO_RANGE_IOEXT && !(addr & PAGE_MASK)); >> + /* >> + * MMIO will call ioremap, it is better to align size with >> PAGE_SIZE, >> + * then the return linux virtual PIO is page-aligned. >> + */ >> + if (size & PAGE_MASK) >> + size = PAGE_ALIGN(size); >> + >> /* check if the range hasn't been previously recorded */ >> spin_lock(&io_range_lock); >> list_for_each_entry(range, &io_range_list, list) { >> - if (addr >= range->start && addr + size <= range->start + >> size) { >> + if (node == range->node) >> + goto end_register; >> + >> + if (addr != IO_RANGE_IOEXT && >> + addr >= range->start && >> + addr + size <= range->start + size) { >> /* range already registered, bail out */ >> goto end_register; >> } >> @@ -3290,6 +3313,7 @@ int __weak pci_register_io_range(phys_addr_t addr, >> resource_size_t size) >> goto end_register; >> } >> >> + range->node = node; >> range->start = addr; >> range->size = size; >> >> @@ -3297,6 +3321,14 @@ int __weak pci_register_io_range(phys_addr_t addr, >> resource_size_t size) >> >> end_register: >> spin_unlock(&io_range_lock); >> + >> + *port = allocated_size; >> +#else >> + /* >> + * powerpc and microblaze have their own registration, >> + * just look up the value here >> + */ >> + *port = pci_address_to_pio(addr); >> #endif >> >> return err; >> @@ -3315,7 +3347,9 @@ phys_addr_t pci_pio_to_address(unsigned long pio) >> >> spin_lock(&io_range_lock); >> list_for_each_entry(range, &io_range_list, list) { >> - if (pio >= allocated_size && pio < allocated_size + >> range->size) { >> + if (range->start != IO_RANGE_IOEXT && >> + pio >= allocated_size && >> + pio < allocated_size + range->size) { >> address = range->start + pio - allocated_size; >> break; >> } >> @@ -3336,7 +3370,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t >> address) >> >> spin_lock(&io_range_lock); >> list_for_each_entry(res, &io_range_list, list) { >> - if (address >= res->start && address < res->start + >> res->size) { >> + if (res->start != IO_RANGE_IOEXT && >> + address >= res->start && >> + address < res->start + res->size) { >> addr = address - res->start + offset; >> break; >> } >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index e2d1a12..8d91af8 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -34,6 +34,9 @@ >> >> #include <linux/pci_ids.h> >> >> +/* the macro below flags an invalid cpu address >> + * and is used by IO special hosts */ >> +#define IO_RANGE_IOEXT (resource_size_t)(-1ull) >> /* >> * The PCI interface treats multi-function devices as independent >> * devices. The slot/function address of each device is encoded >> @@ -1197,8 +1200,8 @@ int __must_check pci_bus_alloc_resource(struct >> pci_bus *bus, >> resource_size_t), >> void *alignf_data); >> >> - >> -int pci_register_io_range(phys_addr_t addr, resource_size_t size); >> +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, >> + resource_size_t size, unsigned long *port); >> 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); >> > >
On 31/01/2017 00:15, Bjorn Helgaas wrote: > On Tue, Jan 24, 2017 at 03:05:22PM +0800, zhichang.yuan wrote: >> After indirect-IO is introduced, system must can assigned indirect-IO devices >> with logical I/O ranges which are different from those for PCI I/O devices. >> Otherwise, I/O accessors can't identify whether the I/O port is for memory >> mapped I/O or indirect-IO. >> As current helper, pci_register_io_range(), is used for PCI I/O ranges >> registration and translation, indirect-IO devices should also apply these >> helpers to manage the I/O ranges. It will be easy to ensure the assigned >> logical I/O ranges unique. >> But for indirect-IO devices, there is no cpu address. The current >> pci_register_io_range() can not work for this case. >> >> This patch makes some changes on the pci_register_io_range() to support the >> I/O range registration with device's fwnode also. After this, the indirect-IO >> devices can register the device-local I/O range to system logical I/O and >> easily perform the translation between device-local I/O range and sytem >> logical I/O range. >> >> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > I had a couple trivial comments, but you can include my: > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> # drivers/pci parts > > in your next revision. I don't know who you have in mind to merge this; > it doesn't really touch much of PCI. But let me know if you need anything > else from me. Thanks Bjorn. The current status is that we plan to send a new patchset early next week with the suggested change to combine PCI+extio IO framework. The IO space range stuff (pci_register_io_range() et al) will move from drivers/pci/pci.c to new lib/libio.c (renamed from extio.c). We still need someone to merge (we are praying to make 4.11), and also Arnd to kindly check the update from his sketch and also more reviews of the ACPI part of what was extio.c (http://www.spinics.net/lists/devicetree/msg160611.html, 5/5) Much appreciated, John > >> --- >> drivers/acpi/pci_root.c | 12 +++++------- >> drivers/of/address.c | 8 ++------ >> drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++---- >> include/linux/pci.h | 7 +++++-- >> 4 files changed, 52 insertions(+), 19 deletions(-) >>
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index bf601d4..6cadf05 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -730,7 +730,8 @@ static void acpi_pci_root_validate_resources(struct device *dev, } } -static void acpi_pci_root_remap_iospace(struct resource_entry *entry) +static void acpi_pci_root_remap_iospace(struct fwnode_handle *node, + struct resource_entry *entry) { #ifdef PCI_IOBASE struct resource *res = entry->res; @@ -739,11 +740,7 @@ static void acpi_pci_root_remap_iospace(struct resource_entry *entry) resource_size_t length = resource_size(res); unsigned long port; - if (pci_register_io_range(cpu_addr, length)) - goto err; - - port = pci_address_to_pio(cpu_addr); - if (port == (unsigned long)-1) + if (pci_register_io_range(node, cpu_addr, length, &port)) goto err; res->start = port; @@ -781,7 +778,8 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) else { resource_list_for_each_entry_safe(entry, tmp, list) { if (entry->res->flags & IORESOURCE_IO) - acpi_pci_root_remap_iospace(entry); + acpi_pci_root_remap_iospace(&device->fwnode, + entry); if (entry->res->flags & IORESOURCE_DISABLED) resource_list_destroy_entry(entry); diff --git a/drivers/of/address.c b/drivers/of/address.c index 02b2903..d85d228 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -2,6 +2,7 @@ #define pr_fmt(fmt) "OF: " fmt #include <linux/device.h> +#include <linux/fwnode.h> #include <linux/io.h> #include <linux/ioport.h> #include <linux/module.h> @@ -323,14 +324,9 @@ int of_pci_range_to_resource(struct of_pci_range *range, if (res->flags & IORESOURCE_IO) { unsigned long port; - err = pci_register_io_range(range->cpu_addr, range->size); + err = pci_register_io_range(&np->fwnode, range->cpu_addr, range->size, &port); if (err) goto invalid_range; - port = pci_address_to_pio(range->cpu_addr); - if (port == (unsigned long)-1) { - err = -EINVAL; - goto invalid_range; - } res->start = port; } else { if ((sizeof(resource_size_t) < 8) && diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index a881c0d..5289221 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3241,6 +3241,7 @@ int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name) #ifdef PCI_IOBASE struct io_range { struct list_head list; + struct fwnode_handle *node; phys_addr_t start; resource_size_t size; }; @@ -3253,7 +3254,8 @@ struct io_range { * Record the PCI IO range (expressed as CPU physical address + size). * Return a negative value if an error has occured, zero otherwise */ -int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) +int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, + resource_size_t size, unsigned long *port) { int err = 0; @@ -3261,10 +3263,31 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) struct io_range *range; resource_size_t allocated_size = 0; + /* + * As indirect-IO which support multiple bus instances is introduced, + * the input 'addr' is probably not page-aligned. If the PCI I/O + * ranges are registered after indirect-IO, there is risk that the + * start logical PIO assigned to PCI I/O is not page-aligned. + * This will cause some I/O subranges are not remapped or overlapped + * in pci_remap_iospace() handling. + */ + WARN_ON(addr != IO_RANGE_IOEXT && !(addr & PAGE_MASK)); + /* + * MMIO will call ioremap, it is better to align size with PAGE_SIZE, + * then the return linux virtual PIO is page-aligned. + */ + if (size & PAGE_MASK) + size = PAGE_ALIGN(size); + /* check if the range hasn't been previously recorded */ spin_lock(&io_range_lock); list_for_each_entry(range, &io_range_list, list) { - if (addr >= range->start && addr + size <= range->start + size) { + if (node == range->node) + goto end_register; + + if (addr != IO_RANGE_IOEXT && + addr >= range->start && + addr + size <= range->start + size) { /* range already registered, bail out */ goto end_register; } @@ -3290,6 +3313,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) goto end_register; } + range->node = node; range->start = addr; range->size = size; @@ -3297,6 +3321,14 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) end_register: spin_unlock(&io_range_lock); + + *port = allocated_size; +#else + /* + * powerpc and microblaze have their own registration, + * just look up the value here + */ + *port = pci_address_to_pio(addr); #endif return err; @@ -3315,7 +3347,9 @@ phys_addr_t pci_pio_to_address(unsigned long pio) spin_lock(&io_range_lock); list_for_each_entry(range, &io_range_list, list) { - if (pio >= allocated_size && pio < allocated_size + range->size) { + if (range->start != IO_RANGE_IOEXT && + pio >= allocated_size && + pio < allocated_size + range->size) { address = range->start + pio - allocated_size; break; } @@ -3336,7 +3370,9 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address) spin_lock(&io_range_lock); list_for_each_entry(res, &io_range_list, list) { - if (address >= res->start && address < res->start + res->size) { + if (res->start != IO_RANGE_IOEXT && + address >= res->start && + address < res->start + res->size) { addr = address - res->start + offset; break; } diff --git a/include/linux/pci.h b/include/linux/pci.h index e2d1a12..8d91af8 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -34,6 +34,9 @@ #include <linux/pci_ids.h> +/* the macro below flags an invalid cpu address + * and is used by IO special hosts */ +#define IO_RANGE_IOEXT (resource_size_t)(-1ull) /* * The PCI interface treats multi-function devices as independent * devices. The slot/function address of each device is encoded @@ -1197,8 +1200,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus, resource_size_t), void *alignf_data); - -int pci_register_io_range(phys_addr_t addr, resource_size_t size); +int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr, + resource_size_t size, unsigned long *port); 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);