Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1415550 > unrolled thread

[PATCH v1 00/25] PCI: Request host bridge window resources

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 00/25] PCI: Request host bridge window resources Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:10 +0200
    [PATCH v1 01/25] PCI: Add devm_request_pci_bus_resources() Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:10 +0200
    [PATCH v1 12/25] PCI: xilinx-nwl: Use dev_printk() when possible Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:10 +0200
    [PATCH v1 14/25] PCI: altera: Simplify host bridge window iteration Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:10 +0200
    [PATCH v1 22/25] PCI: tegra: Remove top-level resource from  hierarchy Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:10 +0200
    [PATCH v1 02/25] PCI: designware: Free bridge resource list on  failure Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:20 +0200
    [PATCH v1 11/25] PCI: xilinx-nwl: Request host bridge window  resources Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:20 +0200
    [PATCH v1 06/25] PCI: xgene: Free bridge resource list on failure Bjorn Helgaas <bhelgaas@google.com> - 2016-06-07 01:20 +0200
    Re: [PATCH v1 00/25] PCI: Request host bridge window resources Arnd Bergmann <arnd@arndb.de> - 2016-06-07 10:50 +0200
      Re: [PATCH v1 00/25] PCI: Request host bridge window resources Bjorn Helgaas <helgaas@kernel.org> - 2016-06-07 15:20 +0200
        Re: [PATCH v1 00/25] PCI: Request host bridge window resources Arnd Bergmann <arnd@arndb.de> - 2016-06-07 15:30 +0200
          Re: [PATCH v1 00/25] PCI: Request host bridge window resources Bjorn Helgaas <helgaas@kernel.org> - 2016-06-08 01:40 +0200
    Re: [PATCH v1 00/25] PCI: Request host bridge window resources Duc Dang <dhdang@apm.com> - 2016-06-10 21:10 +0200

#1415550 — [PATCH v1 00/25] PCI: Request host bridge window resources

Several host bridge drivers (designware and all derivatives, iproc,
xgene, xilinx, and xilinx-nwl) don't request the MMIO and I/O port
windows they forward downstream to the PCI bus.

That means the PCI core can't request resources for PCI bridge
windows and PCI BARs.

Several other drivers (altera, generic, mvebu, rcar, tegra) do request
the windows, but use some duplicated code to do it.

This adds a new devm_request_pci_bus_resources() interface and changes
these drivers to use it.  It also fixes several error paths where we failed
to free the resource list allocated by of_pci_get_host_bridge_resources().

Tegra guys, please take a look at "PCI: tegra: Remove top-level resource
from hierarchy" in particular.  Removing the top-level resource definitely
makes /proc/iomem look uglier (although it will look more like that of
other drivers).  A short-term fix could be to include device information in
the resource name.  I think a better long-term fix would be to make the DT
or platform device core request all the resources from the DT.

Comments welcome.  I expect we'll trip over something here, so I marked
this "v1" and I don't plan to put it into -next for a while.

This is on my pci/host-request-windows branch, which you can pull or view
at https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-request-windows

---

Bjorn Helgaas (25):
      PCI: Add devm_request_pci_bus_resources()
      PCI: designware: Free bridge resource list on failure
      PCI: designware: Request host bridge window resources
      PCI: designware: Simplify host bridge window iteration
      PCI: iproc: Request host bridge window resources
      PCI: xgene: Free bridge resource list on failure
      PCI: xgene: Request host bridge window resources
      PCI: xilinx: Free bridge resource list on failure
      PCI: xilinx: Request host bridge window resources
      PCI: xilinx-nwl: Free bridge resource list on failure
      PCI: xilinx-nwl: Request host bridge window resources
      PCI: xilinx-nwl: Use dev_printk() when possible
      PCI: altera: Request host bridge window resources with core function
      PCI: altera: Simplify host bridge window iteration
      PCI: generic: Free resource list close to where it's allocated
      PCI: generic: Request host bridge window resources with core function
      PCI: generic: Simplify host bridge window iteration
      PCI: mvebu: Request host bridge window resources with core function
      PCI: rcar Gen2: Request host bridge window resources
      PCI: rcar: Request host bridge window resources with core function
      PCI: rcar: Simplify host bridge window iteration
      PCI: tegra: Remove top-level resource from hierarchy
      PCI: tegra: Request host bridge window resources with core function
      PCI: versatile: Request host bridge window resources with core function
      PCI: versatile: Simplify host bridge window iteration


 drivers/pci/bus.c                  |   29 +++++++++++++++++
 drivers/pci/host/pci-host-common.c |   61 +++++++++++++++---------------------
 drivers/pci/host/pci-mvebu.c       |   17 ++++------
 drivers/pci/host/pci-rcar-gen2.c   |    4 ++
 drivers/pci/host/pci-tegra.c       |   35 +++------------------
 drivers/pci/host/pci-versatile.c   |   29 ++++++-----------
 drivers/pci/host/pci-xgene.c       |   16 ++++++++-
 drivers/pci/host/pcie-altera.c     |   35 ++++++---------------
 drivers/pci/host/pcie-designware.c |   34 +++++++++++++-------
 drivers/pci/host/pcie-iproc.c      |    4 ++
 drivers/pci/host/pcie-rcar.c       |   33 +++++--------------
 drivers/pci/host/pcie-xilinx-nwl.c |   20 +++++++++---
 drivers/pci/host/pcie-xilinx.c     |   16 ++++++++-
 include/linux/pci.h                |    5 ++-
 14 files changed, 170 insertions(+), 168 deletions(-)

[toc] | [next] | [standalone]

#1415551 — [PATCH v1 01/25] PCI: Add devm_request_pci_bus_resources()

Several host bridge drivers iterate through the list of bridge windows to
request resources.  Several others don't request the window resources at
all.

Add a devm_request_pci_bus_resources() interface to make it easier for
drivers to request all the window resources.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/bus.c   |   29 ++++++++++++++++++++++++++++-
 include/linux/pci.h |    5 ++++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index dd7cdbe..78b90c7 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -91,6 +91,34 @@ void pci_bus_remove_resources(struct pci_bus *bus)
 	}
 }
 
+int devm_request_pci_bus_resources(struct device *dev,
+				   struct list_head *resources)
+{
+	struct resource_entry *win;
+	struct resource *parent, *res;
+	int err;
+
+	resource_list_for_each_entry(win, resources) {
+		res = win->res;
+		switch (resource_type(res)) {
+		case IORESOURCE_IO:
+			parent = &ioport_resource;
+			break;
+		case IORESOURCE_MEM:
+			parent = &iomem_resource;
+			break;
+		default:
+			continue;
+		}
+
+		err = devm_request_resource(dev, parent, res);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static struct pci_bus_region pci_32_bit = {0, 0xffffffffULL};
 #ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
 static struct pci_bus_region pci_64_bit = {0,
@@ -397,4 +425,3 @@ void pci_bus_put(struct pci_bus *bus)
 		put_device(&bus->dev);
 }
 EXPORT_SYMBOL(pci_bus_put);
-
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b67e4df..6ac8360 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1143,9 +1143,12 @@ void pci_add_resource(struct list_head *resources, struct resource *res);
 void pci_add_resource_offset(struct list_head *resources, struct resource *res,
 			     resource_size_t offset);
 void pci_free_resource_list(struct list_head *resources);
-void pci_bus_add_resource(struct pci_bus *bus, struct resource *res, unsigned int flags);
+void pci_bus_add_resource(struct pci_bus *bus, struct resource *res,
+			  unsigned int flags);
 struct resource *pci_bus_resource_n(const struct pci_bus *bus, int n);
 void pci_bus_remove_resources(struct pci_bus *bus);
+int devm_request_pci_bus_resources(struct device *dev,
+				   struct list_head *resources);
 
 #define pci_bus_for_each_resource(bus, res, i)				\
 	for (i = 0;							\

[toc] | [prev] | [next] | [standalone]

#1415552 — [PATCH v1 12/25] PCI: xilinx-nwl: Use dev_printk() when possible

Use dev_printk() when possible to make messages more useful.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 3c16bbf..0b597d9 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -825,7 +825,7 @@ static int nwl_pcie_probe(struct platform_device *pdev)
 
 	err = of_pci_get_host_bridge_resources(node, 0, 0xff, &res, &iobase);
 	if (err) {
-		pr_err("Getting bridge resources failed\n");
+		dev_err(pcie->dev, "Getting bridge resources failed\n");
 		return err;
 	}
 

[toc] | [prev] | [next] | [standalone]

#1415553 — [PATCH v1 14/25] PCI: altera: Simplify host bridge window iteration

The switch is the only statement in the resource_list_for_each_entry()
loop, so remove unnecessary "continue" statements in the switch.  Simplify
checking for the required non-prefetchable memory aperture.  Inline
altera_pcie_release_of_pci_ranges(), which is only called once.

No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pcie-altera.c |   24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
index b97abbc..cf20c67 100644
--- a/drivers/pci/host/pcie-altera.c
+++ b/drivers/pci/host/pcie-altera.c
@@ -415,11 +415,6 @@ static void altera_pcie_isr(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static void altera_pcie_release_of_pci_ranges(struct altera_pcie *pcie)
-{
-	pci_free_resource_list(&pcie->resources);
-}
-
 static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
 {
 	int err, res_valid = 0;
@@ -439,25 +434,18 @@ static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
 	resource_list_for_each_entry(win, &pcie->resources) {
 		struct resource *res = win->res;
 
-		switch (resource_type(res)) {
-		case IORESOURCE_MEM:
+		if (resource_type(res) == IORESOURCE_MEM)
 			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
-			break;
-		default:
-			continue;
-		}
 	}
 
-	if (!res_valid) {
-		dev_err(dev, "non-prefetchable memory resource required\n");
-		err = -EINVAL;
-		goto out_release_res;
-	}
+	if (res_valid)
+		return 0;
 
-	return 0;
+	dev_err(dev, "non-prefetchable memory resource required\n");
+	err = -EINVAL;
 
 out_release_res:
-	altera_pcie_release_of_pci_ranges(pcie);
+	pci_free_resource_list(&pcie->resources);
 	return err;
 }
 

[toc] | [prev] | [next] | [standalone]

#1415554 — [PATCH v1 22/25] PCI: tegra: Remove top-level resource from hierarchy

41534e53786d ("PCI: tegra: Implement a proper resource hierarchy") did two
things:

  1) It added a top-level resource that encloses all resources declared in
     the DT description, including registers and bridge apertures, and

  2) It requested the bridge apertures, which means the PCI core can track
     the resources used by PCI devices below the bridge.

The latter is necessary, but the former is questionable because there's no
guarantee that the bridge registers and the apertures are contiguous.  In
this example:

  # cat /proc/iomem
  00000000-3fffffff : /pcie-controller@00003000
    00000000-00000fff : /pcie-controller@00003000/pci@1,0
    00003000-000037ff : pads
    00003800-000039ff : afi
    10000000-1fffffff : cs

the resource tree claims that [mem 0x00003a00-0x0fffffff] is consumed by
/pcie-controller@00003000, but it's not mentioned in the DT, and it might
actually be used by other devices.

Remove the top-level resource so we don't claim more than the device
actually consumes.

This reintroduces the problem that we can't match the resources, e.g.,
"pads", "afi", "cs", etc., to the DT device.  I think this should be solved
by having the DT core request all resources of all devices in the DT (it
does not do that today).  If a driver claims the device, it can request the
resources it uses.  For example:

  # cat /proc/iomem
  00000000-00000fff : /pcie-controller@00003000
    00000000-00000fff : /pcie-controller@00003000/pci@1,0
  00003000-000037ff : /pcie-controller@00003000
    00003000-000037ff : pads
  00003800-000039ff : /pcie-controller@00003000
    00003800-000039ff : afi
  10000000-1fffffff : /pcie-controller@00003000
    10000000-1fffffff : cs
  ...

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pci-tegra.c |   23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index c388468..920a899 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -274,7 +274,6 @@ struct tegra_pcie {
 	struct list_head buses;
 	struct resource *cs;
 
-	struct resource all;
 	struct resource io;
 	struct resource pio;
 	struct resource mem;
@@ -623,7 +622,7 @@ static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
 	sys->mem_offset = pcie->offset.mem;
 	sys->io_offset = pcie->offset.io;
 
-	err = devm_request_resource(pcie->dev, &pcie->all, &pcie->io);
+	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->io);
 	if (err < 0)
 		return err;
 
@@ -631,11 +630,11 @@ static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
 	if (err < 0)
 		return err;
 
-	err = devm_request_resource(pcie->dev, &pcie->all, &pcie->mem);
+	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->mem);
 	if (err < 0)
 		return err;
 
-	err = devm_request_resource(pcie->dev, &pcie->all, &pcie->prefetch);
+	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->prefetch);
 	if (err)
 		return err;
 
@@ -1822,12 +1821,6 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 	struct resource res;
 	int err;
 
-	memset(&pcie->all, 0, sizeof(pcie->all));
-	pcie->all.flags = IORESOURCE_MEM;
-	pcie->all.name = np->full_name;
-	pcie->all.start = ~0;
-	pcie->all.end = 0;
-
 	if (of_pci_range_parser_init(&parser, np)) {
 		dev_err(pcie->dev, "missing \"ranges\" property\n");
 		return -EINVAL;
@@ -1880,18 +1873,8 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
 			}
 			break;
 		}
-
-		if (res.start <= pcie->all.start)
-			pcie->all.start = res.start;
-
-		if (res.end >= pcie->all.end)
-			pcie->all.end = res.end;
 	}
 
-	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->all);
-	if (err < 0)
-		return err;
-
 	err = of_pci_parse_bus_range(np, &pcie->busn);
 	if (err < 0) {
 		dev_err(pcie->dev, "failed to parse ranges property: %d\n",

[toc] | [prev] | [next] | [standalone]

#1415555 — [PATCH v1 02/25] PCI: designware: Free bridge resource list on failure

of_pci_get_host_bridge_resources() allocates a list of resources for host
bridge windows.  If we fail after allocating that list, free it before we
return error.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pcie-designware.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index aafd766..9ade767 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -493,7 +493,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 					resource_size(pp->cfg));
 		if (!pp->dbi_base) {
 			dev_err(pp->dev, "error with ioremap\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error;
 		}
 	}
 
@@ -504,7 +505,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 						pp->cfg0_size);
 		if (!pp->va_cfg0_base) {
 			dev_err(pp->dev, "error with ioremap in function\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error;
 		}
 	}
 
@@ -513,7 +515,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 						pp->cfg1_size);
 		if (!pp->va_cfg1_base) {
 			dev_err(pp->dev, "error with ioremap\n");
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto error;
 		}
 	}
 
@@ -528,7 +531,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 						&dw_pcie_msi_chip);
 			if (!pp->irq_domain) {
 				dev_err(pp->dev, "irq domain init failed\n");
-				return -ENXIO;
+				ret = -ENXIO;
+				goto error;
 			}
 
 			for (i = 0; i < MAX_MSI_IRQS; i++)
@@ -536,7 +540,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		} else {
 			ret = pp->ops->msi_host_init(pp, &dw_pcie_msi_chip);
 			if (ret < 0)
-				return ret;
+				goto error;
 		}
 	}
 
@@ -552,8 +556,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
 	} else
 		bus = pci_scan_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops,
 					pp, &res);
-	if (!bus)
-		return -ENOMEM;
+	if (!bus) {
+		ret = -ENOMEM;
+		goto error;
+	}
 
 	if (pp->ops->scan_bus)
 		pp->ops->scan_bus(pp);
@@ -571,6 +577,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
 
 	pci_bus_add_devices(bus);
 	return 0;
+
+error:
+	pci_free_resource_list(&res);
+	return ret;
 }
 
 static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,

[toc] | [prev] | [next] | [standalone]

#1415563 — [PATCH v1 11/25] PCI: xilinx-nwl: Request host bridge window resources

Request host bridge window resources so they appear in ioport_resource and
iomem_resource and are reflected in /proc/ioports and /proc/iomem.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pcie-xilinx-nwl.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c
index 506da7b..3c16bbf 100644
--- a/drivers/pci/host/pcie-xilinx-nwl.c
+++ b/drivers/pci/host/pcie-xilinx-nwl.c
@@ -829,6 +829,10 @@ static int nwl_pcie_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	err = devm_request_pci_bus_resources(pcie->dev, &res);
+	if (err)
+		goto error;
+
 	err = nwl_pcie_init_irq_domain(pcie);
 	if (err) {
 		dev_err(pcie->dev, "Failed creating IRQ Domain\n");

[toc] | [prev] | [next] | [standalone]

#1415565 — [PATCH v1 06/25] PCI: xgene: Free bridge resource list on failure

of_pci_get_host_bridge_resources() allocates a list of resources for host
bridge windows.  If we fail after allocating that list, free it before we
return error.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pci-xgene.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index ae00ce2..bc4e1c6 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -542,12 +542,14 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 
 	ret = xgene_pcie_setup(port, &res, iobase);
 	if (ret)
-		return ret;
+		goto error;
 
 	bus = pci_create_root_bus(&pdev->dev, 0,
 					&xgene_pcie_ops, port, &res);
-	if (!bus)
-		return -ENOMEM;
+	if (!bus) {
+		ret = -ENOMEM;
+		goto error;
+	}
 
 	pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
@@ -555,6 +557,10 @@ static int xgene_pcie_probe_bridge(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, port);
 	return 0;
+
+error:
+	pci_free_resource_list(&res);
+	return ret;
 }
 
 static const struct of_device_id xgene_pcie_match_table[] = {

[toc] | [prev] | [next] | [standalone]

#1415877

On Monday, June 6, 2016 6:04:44 PM CEST Bjorn Helgaas wrote:
> Several host bridge drivers (designware and all derivatives, iproc,
> xgene, xilinx, and xilinx-nwl) don't request the MMIO and I/O port
> windows they forward downstream to the PCI bus.
> 
> That means the PCI core can't request resources for PCI bridge
> windows and PCI BARs.
> 
> Several other drivers (altera, generic, mvebu, rcar, tegra) do request
> the windows, but use some duplicated code to do it.
> 
> This adds a new devm_request_pci_bus_resources() interface and changes
> these drivers to use it.  It also fixes several error paths where we failed
> to free the resource list allocated by of_pci_get_host_bridge_resources().
> 
> Tegra guys, please take a look at "PCI: tegra: Remove top-level resource
> from hierarchy" in particular.  Removing the top-level resource definitely
> makes /proc/iomem look uglier (although it will look more like that of
> other drivers).  A short-term fix could be to include device information in
> the resource name.  I think a better long-term fix would be to make the DT
> or platform device core request all the resources from the DT.
> 
> Comments welcome.  I expect we'll trip over something here, so I marked
> this "v1" and I don't plan to put it into -next for a while.
> 
> This is on my pci/host-request-windows branch, which you can pull or view
> at https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-request-windows

This looks very nice. There is one related aspect that I have been
grumbling about for a while, but I don't know what the driver is
actually supposed to do there:

For the IORESOURCE_IO resources, some drivers request the MMIO address
that the window is mapped into, some drivers request the PIO range, and
some of them request both. I also believe the resource that gets put
into the bridge resources list is not always the same one (or maybe
that got fixed by now).

What do you think is the correct behavior here, should the driver only
request the PIO range with parent=ioport_resource, or should it also
request the MMIO window for the I/O ports with parent=iomem_resource?
In the latter case, any idea how that can be generalized?

Another aspect is that we already have the
gen_pci_parse_request_of_pci_ranges() function that does the same as your
new devm_request_pci_bus_resources() and then a few other things. I
have been wondering whether we could move that function into common
code convert drivers to use that wherever possible, but I guess we can
always do that as a follow-up after this series.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1416151

On Tue, Jun 07, 2016 at 10:21:36AM +0200, Arnd Bergmann wrote:
> On Monday, June 6, 2016 6:04:44 PM CEST Bjorn Helgaas wrote:
> > Several host bridge drivers (designware and all derivatives, iproc,
> > xgene, xilinx, and xilinx-nwl) don't request the MMIO and I/O port
> > windows they forward downstream to the PCI bus.
> > 
> > That means the PCI core can't request resources for PCI bridge
> > windows and PCI BARs.
> > 
> > Several other drivers (altera, generic, mvebu, rcar, tegra) do request
> > the windows, but use some duplicated code to do it.
> > 
> > This adds a new devm_request_pci_bus_resources() interface and changes
> > these drivers to use it.  It also fixes several error paths where we failed
> > to free the resource list allocated by of_pci_get_host_bridge_resources().
> > 
> > Tegra guys, please take a look at "PCI: tegra: Remove top-level resource
> > from hierarchy" in particular.  Removing the top-level resource definitely
> > makes /proc/iomem look uglier (although it will look more like that of
> > other drivers).  A short-term fix could be to include device information in
> > the resource name.  I think a better long-term fix would be to make the DT
> > or platform device core request all the resources from the DT.
> > 
> > Comments welcome.  I expect we'll trip over something here, so I marked
> > this "v1" and I don't plan to put it into -next for a while.
> > 
> > This is on my pci/host-request-windows branch, which you can pull or view
> > at https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-request-windows
> 
> This looks very nice. There is one related aspect that I have been
> grumbling about for a while, but I don't know what the driver is
> actually supposed to do there:
> 
> For the IORESOURCE_IO resources, some drivers request the MMIO address
> that the window is mapped into, some drivers request the PIO range, and
> some of them request both. I also believe the resource that gets put
> into the bridge resources list is not always the same one (or maybe
> that got fixed by now).
> 
> What do you think is the correct behavior here, should the driver only
> request the PIO range with parent=ioport_resource, or should it also
> request the MMIO window for the I/O ports with parent=iomem_resource?
> In the latter case, any idea how that can be generalized?

I think it should request both because I think iomem_resource should
contain everything in the memory map.  This would be required if we ever
did any significant reassignment of top-level devices, e.g., ACPI devices.
For example, on ia64, we do this:

  /proc/ioports:
  00000000-00003fff : PCI Bus 0000:00
  00004000-00009fff : PCI Bus 0000:80
  0000a000-0000bfff : PCI Bus 0000:a0
  0000c000-0000ffff : PCI Bus 0000:c0

  /proc/iomem:
  80000000-9fffffff : PCI Bus 0000:00
  a0000000-cfffffff : PCI Bus 0000:80
  d0000000-dfffffff : PCI Bus 0000:a0
  e0000000-fdffffff : PCI Bus 0000:c0
  80004000000-80103fffffe : PCI Bus 0000:00
  c0004000000-c0103fffffe : PCI Bus 0000:80
  d0004000000-d0103fffffe : PCI Bus 0000:a0
  e0004000000-e0103fffffe : PCI Bus 0000:c0
  3fffffc000000-3fffffcffffff : PCI Bus 0000:00 I/O Ports 00000000-00003fff
  3fffffd000000-3fffffe7fffff : PCI Bus 0000:80 I/O Ports 00004000-00009fff
  3fffffe800000-3fffffeffffff : PCI Bus 0000:a0 I/O Ports 0000a000-0000bfff
  3ffffff000000-3ffffffffffff : PCI Bus 0000:c0 I/O Ports 0000c000-0000ffff

> Another aspect is that we already have the
> gen_pci_parse_request_of_pci_ranges() function that does the same as your
> new devm_request_pci_bus_resources() and then a few other things. I
> have been wondering whether we could move that function into common
> code convert drivers to use that wherever possible, but I guess we can
> always do that as a follow-up after this series.

Oh, I didn't notice that; thanks for pointing it out.  That should be
consolidated somehow.  It also checks to be sure there is a
non-prefetchable memory resource.  A few other drivers also do that, but
most don't.  I suppose that will mostly catch DT errors.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1416158

On Tuesday, June 7, 2016 8:11:05 AM CEST Bjorn Helgaas wrote:
> > 
> > What do you think is the correct behavior here, should the driver only
> > request the PIO range with parent=ioport_resource, or should it also
> > request the MMIO window for the I/O ports with parent=iomem_resource?
> > In the latter case, any idea how that can be generalized?
> 
> I think it should request both because I think iomem_resource should
> contain everything in the memory map.  This would be required if we ever
> did any significant reassignment of top-level devices, e.g., ACPI devices.

Ok. Should we try to pass the mmio resource for the I/O window to
the devm_request_pci_bus_resources() function along with the other
arguments then?

As far as I can tell, it should not go into the resource list
because it is not something the PCI core code should access the
way it handles the other resources.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1416740

On Tue, Jun 07, 2016 at 03:25:46PM +0200, Arnd Bergmann wrote:
> On Tuesday, June 7, 2016 8:11:05 AM CEST Bjorn Helgaas wrote:
> > > 
> > > What do you think is the correct behavior here, should the driver only
> > > request the PIO range with parent=ioport_resource, or should it also
> > > request the MMIO window for the I/O ports with parent=iomem_resource?
> > > In the latter case, any idea how that can be generalized?
> > 
> > I think it should request both because I think iomem_resource should
> > contain everything in the memory map.  This would be required if we ever
> > did any significant reassignment of top-level devices, e.g., ACPI devices.
> 
> Ok. Should we try to pass the mmio resource for the I/O window to
> the devm_request_pci_bus_resources() function along with the other
> arguments then?

I think memory-mapped I/O port windows are different enough that maybe
we ought to handle them separately.  It seems like there are several
things related to setting up those windows (requesting the resource,
ioremapping it, allocating the CPU port number space, etc.), and maybe
if we keep this out, a pattern will emerge.

Maybe I should rename this to "devm_request_pci_host_windows()" or
something?

> As far as I can tell, it should not go into the resource list
> because it is not something the PCI core code should access the
> way it handles the other resources.

Right.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1419668

On Mon, Jun 6, 2016 at 4:04 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> Several host bridge drivers (designware and all derivatives, iproc,
> xgene, xilinx, and xilinx-nwl) don't request the MMIO and I/O port
> windows they forward downstream to the PCI bus.
>
> That means the PCI core can't request resources for PCI bridge
> windows and PCI BARs.
>
> Several other drivers (altera, generic, mvebu, rcar, tegra) do request
> the windows, but use some duplicated code to do it.
>
> This adds a new devm_request_pci_bus_resources() interface and changes
> these drivers to use it.  It also fixes several error paths where we failed
> to free the resource list allocated by of_pci_get_host_bridge_resources().
>
> Tegra guys, please take a look at "PCI: tegra: Remove top-level resource
> from hierarchy" in particular.  Removing the top-level resource definitely
> makes /proc/iomem look uglier (although it will look more like that of
> other drivers).  A short-term fix could be to include device information in
> the resource name.  I think a better long-term fix would be to make the DT
> or platform device core request all the resources from the DT.
>
> Comments welcome.  I expect we'll trip over something here, so I marked
> this "v1" and I don't plan to put it into -next for a while.
>
> This is on my pci/host-request-windows branch, which you can pull or view
> at https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-request-windows
>
> ---
>
> Bjorn Helgaas (25):
>       PCI: Add devm_request_pci_bus_resources()
>       PCI: designware: Free bridge resource list on failure
>       PCI: designware: Request host bridge window resources
>       PCI: designware: Simplify host bridge window iteration
>       PCI: iproc: Request host bridge window resources
>       PCI: xgene: Free bridge resource list on failure
>       PCI: xgene: Request host bridge window resources
>       PCI: xilinx: Free bridge resource list on failure
>       PCI: xilinx: Request host bridge window resources
>       PCI: xilinx-nwl: Free bridge resource list on failure
>       PCI: xilinx-nwl: Request host bridge window resources
>       PCI: xilinx-nwl: Use dev_printk() when possible
>       PCI: altera: Request host bridge window resources with core function
>       PCI: altera: Simplify host bridge window iteration
>       PCI: generic: Free resource list close to where it's allocated
>       PCI: generic: Request host bridge window resources with core function
>       PCI: generic: Simplify host bridge window iteration
>       PCI: mvebu: Request host bridge window resources with core function
>       PCI: rcar Gen2: Request host bridge window resources
>       PCI: rcar: Request host bridge window resources with core function
>       PCI: rcar: Simplify host bridge window iteration
>       PCI: tegra: Remove top-level resource from hierarchy
>       PCI: tegra: Request host bridge window resources with core function
>       PCI: versatile: Request host bridge window resources with core function
>       PCI: versatile: Simplify host bridge window iteration

Thanks, Bjorn.

For the 2 X-Gene patches:
     PCI: xgene: Free bridge resource list on failure
     PCI: xgene: Request host bridge window resources

Tested-by: Duc Dang <dhdang@apm.com>

Regards,
Duc Dang.
>
>
>  drivers/pci/bus.c                  |   29 +++++++++++++++++
>  drivers/pci/host/pci-host-common.c |   61 +++++++++++++++---------------------
>  drivers/pci/host/pci-mvebu.c       |   17 ++++------
>  drivers/pci/host/pci-rcar-gen2.c   |    4 ++
>  drivers/pci/host/pci-tegra.c       |   35 +++------------------
>  drivers/pci/host/pci-versatile.c   |   29 ++++++-----------
>  drivers/pci/host/pci-xgene.c       |   16 ++++++++-
>  drivers/pci/host/pcie-altera.c     |   35 ++++++---------------
>  drivers/pci/host/pcie-designware.c |   34 +++++++++++++-------
>  drivers/pci/host/pcie-iproc.c      |    4 ++
>  drivers/pci/host/pcie-rcar.c       |   33 +++++--------------
>  drivers/pci/host/pcie-xilinx-nwl.c |   20 +++++++++---
>  drivers/pci/host/pcie-xilinx.c     |   16 ++++++++-
>  include/linux/pci.h                |    5 ++-
>  14 files changed, 170 insertions(+), 168 deletions(-)
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web