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(-)
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(-)
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-designware.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -452,6 +452,10 @@ int dw_pcie_host_init(struct pcie_port *pp)if(ret)returnret;+ret=devm_request_pci_bus_resources(&pdev->dev,&res);+if(ret)+gotoerror;+/* Get the I/O and memory ranges from DT */resource_list_for_each_entry(win,&res){switch(resource_type(win->res)){
The switch is the only statement in the resource_list_for_each_entry()
loop, so remove unnecessary "continue" statements in the switch.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pcie-designware.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
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-iproc.c | 4 ++++
1 file changed, 4 insertions(+)
Request host bridge window resources so they appear in ioport_resource and
iomem_resource and are reflected in /proc/ioports and /proc/iomem.
For example, the following entries did not previously appear in /proc/iomem:
e180000000-e1ffffffff : /soc/pcie at 1f2b0000
e180000000-e182ffffff : PCI Bus 0000:01
e180000000-e181ffffff : 0000:01:00.0
e182000000-e1820fffff : 0000:01:00.0
e182100000-e1821fffff : 0000:01:00.0
f000000000-ffffffffff : /soc/pcie at 1f2b0000
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-xgene.c | 4 ++++
1 file changed, 4 insertions(+)
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-xilinx.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
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.c | 5 +++++
1 file changed, 5 insertions(+)
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-xilinx-nwl.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
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(-)
Use devm_request_pci_bus_resources() to request host bridge window
resources instead of doing it by hand in the driver.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pcie-altera.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
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(-)
Previously we allocated the PCI resource list in
gen_pci_parse_request_of_pci_ranges(), but if we had an error, we freed it
on error in gen_pci_init().
Reorder gen_pci_init() so we can take care of error path cleanup in
gen_pci_parse_request_of_pci_ranges() instead.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-host-common.c | 32 +++++++++++++++-----------------
1 file changed, 15 insertions(+), 17 deletions(-)
Use devm_request_pci_bus_resources() to request host bridge window
resources instead of doing it by hand in the driver.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-host-common.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
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.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-host-common.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
Use devm_request_pci_bus_resources() to request host bridge window
resources instead of doing it by hand in the driver.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-mvebu.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
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/pci-rcar-gen2.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -275,6 +276,9 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)/* Add PCI resources */pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);+ret=devm_request_pci_bus_resources(priv->dev,&sys->resources);+if(ret<0)+returnret;/* Setup bus number based on platform device id / of bus-range */sys->busnr=priv->busnr;
Use devm_request_pci_bus_resources() to request host bridge window
resources instead of doing it by hand in the driver.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pcie-rcar.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
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 at 00003000
00000000-00000fff : /pcie-controller at 00003000/pci at 1,0
00003000-000037ff : pads
00003800-000039ff : afi
10000000-1fffffff : cs
the resource tree claims that [mem 0x00003a00-0x0fffffff] is consumed by
/pcie-controller at 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 at 00003000
00000000-00000fff : /pcie-controller at 00003000/pci at 1,0
00003000-000037ff : /pcie-controller at 00003000
00003000-000037ff : pads
00003800-000039ff : /pcie-controller at 00003000
00003800-000039ff : afi
10000000-1fffffff : /pcie-controller at 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(-)
Use devm_request_pci_bus_resources() to request host bridge window
resources instead of doing it by hand in the driver.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-tegra.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
Use devm_request_pci_bus_resources() to request host bridge window
resources instead of doing it by hand in the driver.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-versatile.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
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.
No functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/host/pci-versatile.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
The switch is the only statement in the resource_list_for_each_entry()
loop, so remove unnecessary cases and "continue" statements in the switch.
Inline rcar_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-rcar.c | 21 +++------------------
1 file changed, 3 insertions(+), 18 deletions(-)
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(+)
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(-)
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(-)
@@ -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;+gotoerror;}}
@@ -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;+gotoerror;}}
@@ -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;+gotoerror;}}
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
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:
quoted
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
On Tuesday, June 7, 2016 8:11:05 AM CEST Bjorn Helgaas wrote:
quoted
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
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:
quoted
quoted
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.
On Mon, Jun 6, 2016 at 4:04 PM, Bjorn Helgaas [off-list ref] 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 <redacted>
Regards,
Duc Dang.
On Tue, Jun 07, 2016 at 08:11:05AM -0500, Bjorn Helgaas wrote:
On Tue, Jun 07, 2016 at 10:21:36AM +0200, Arnd Bergmann wrote:
quoted
On Monday, June 6, 2016 6:04:44 PM CEST Bjorn Helgaas wrote:
quoted
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
quoted
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.
Coming back to this, I did actually change
gen_pci_parse_request_of_pci_ranges() to call my new function in
[16/25] "PCI: generic: Request host bridge window resources with core
function".
The gen_pci_parse_request_of_pci_ranges() is still there and it still
contains the loop to deal with the I/O port space and to validate that
a non-prefetchable memory window exists. Both of those could probably
be made more generic later.
Bjorn
On Mon, Jun 06, 2016 at 06:04:44PM -0500, 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
I merged this to my -next branch, so it should show up in linux-next
in a couple days. Let me know if you see any problems.
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(-)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Bjorn,
On 6 June 2016 at 16:06, Bjorn Helgaas [off-list ref] wrote:
Previously we allocated the PCI resource list in
gen_pci_parse_request_of_pci_ranges(), but if we had an error, we freed it
on error in gen_pci_init().
Reorder gen_pci_init() so we can take care of error path cleanup in
gen_pci_parse_request_of_pci_ranges() instead.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
From: Lorenzo Pieralisi <hidden> Date: 2016-06-20 17:23:23
On Mon, Jun 20, 2016 at 09:56:45AM -0700, Tyler Baker wrote:
Hi Bjorn,
On 6 June 2016 at 16:06, Bjorn Helgaas [off-list ref] wrote:
quoted
Previously we allocated the PCI resource list in
gen_pci_parse_request_of_pci_ranges(), but if we had an error, we freed it
on error in gen_pci_init().
Reorder gen_pci_init() so we can take care of error path cleanup in
gen_pci_parse_request_of_pci_ranges() instead.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
The kernelci.org bot has reported[0] new qemu-aarch64
(arm64-defconfig) boot failures[1][2] in next-20160620. I've
bisected[3] this boot failure down to this patch, and confirmed
reverting it on top of next-20160620 resolves the boot issue.
I have not investigated further, but you can easily reproduce[4] the
boot failure on an x86 host running qemu-system-aarch64 (I'm running
qemu-system 2.6).
That's most likely because pci_ecam_create() requires the bus_range
resource (its busr parameter) to be initialized when it is called
and that's not the case after this patch is applied if I read it
correctly.
It is probably a NULL pointer dereference in pci_ecam_create().
Thanks,
Lorenzo
Hi Bjorn,
On Tue, Jun 7, 2016 at 1:07 AM, Bjorn Helgaas [off-list ref] wrote:
quoted hunk
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/pci-rcar-gen2.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -275,6 +276,9 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)/* Add PCI resources */pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);+ret=devm_request_pci_bus_resources(priv->dev,&sys->resources);+if(ret<0)+returnret;/* Setup bus number based on platform device id / of bus-range */sys->busnr=priv->busnr;
This patch (commit 1bd019707b7c9249d34c5d348f1ef75eb4d83e89 in pci/next)
broke PCI on r8a7791/koelsch. Dmesg differences are:
pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11
-pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
-pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
-pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
-pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]
-pci 0000:00:00.0: [1033:0000] type 00 class 0x060000
-pci 0000:00:00.0: reg 0x10: [mem 0xee090800-0xee090bff]
-pci 0000:00:00.0: reg 0x14: [mem 0x40000000-0x7fffffff pref]
-pci 0000:00:01.0: [1033:0035] type 00 class 0x0c0310
-pci 0000:00:01.0: reg 0x10: [mem 0x00000000-0x00000fff]
-pci 0000:00:01.0: supports D1 D2
-pci 0000:00:01.0: PME# supported from D0 D1 D2 D3hot
-pci 0000:00:02.0: [1033:00e0] type 00 class 0x0c0320
-pci 0000:00:02.0: reg 0x10: [mem 0x00000000-0x000000ff]
-pci 0000:00:02.0: supports D1 D2
-pci 0000:00:02.0: PME# supported from D0 D1 D2 D3hot
-PCI: bus0: Fast back to back transfers disabled
-pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to 00
-pci 0000:00:01.0: BAR 0: assigned [mem 0xee080000-0xee080fff]
-pci 0000:00:02.0: BAR 0: assigned [mem 0xee081000-0xee0810ff]
+pci-rcar-gen2 ee090000.pci: resource collision: [io
0xee080000-0xee0810ff] conflicts with PCI IO [io 0x0000-0xfffff]
and:
pci-rcar-gen2 ee0d0000.pci: PCI: bus0 revision 11
-pci-rcar-gen2 ee0d0000.pci: PCI host bridge to bus 0001:01
-pci_bus 0001:01: root bus resource [io 0xee0c0000-0xee0c10ff]
-pci_bus 0001:01: root bus resource [mem 0xee0c0000-0xee0c10ff]
-pci_bus 0001:01: No busn resource found for root bus, will use [bus 01-ff]
-pci 0001:01:00.0: [1033:0000] type 00 class 0x060000
-pci 0001:01:00.0: reg 0x10: [mem 0xee0d0800-0xee0d0bff]
-pci 0001:01:00.0: reg 0x14: [mem 0x40000000-0x7fffffff pref]
-pci 0001:01:01.0: [1033:0035] type 00 class 0x0c0310
-pci 0001:01:01.0: reg 0x10: [mem 0x00000000-0x00000fff]
-pci 0001:01:01.0: supports D1 D2
-pci 0001:01:01.0: PME# supported from D0 D1 D2 D3hot
-pci 0001:01:02.0: [1033:00e0] type 00 class 0x0c0320
-pci 0001:01:02.0: reg 0x10: [mem 0x00000000-0x000000ff]
-pci 0001:01:02.0: supports D1 D2
-pci 0001:01:02.0: PME# supported from D0 D1 D2 D3hot
-PCI: bus1: Fast back to back transfers disabled
-pci_bus 0001:01: busn_res: [bus 01-ff] end is updated to 01
-pci 0001:01:01.0: BAR 0: assigned [mem 0xee0c0000-0xee0c0fff]
-pci 0001:01:02.0: BAR 0: assigned [mem 0xee0c1000-0xee0c10ff]
+pci-rcar-gen2 ee0d0000.pci: resource collision: [io
0xee0c0000-0xee0c10ff] conflicts with PCI IO [io 0x0000-0xfffff]
# cat /proc/iomem
30000000-37ffffff : /pcie at fe000000
38000000-3fffffff : /pcie at fe000000
40000000-6fffffff : System RAM
40008000-40a1c15b : Kernel code
40e00000-40e9ea97 : Kernel data
e6060000-e606024f : /pfc at e6060000
e60b0000-e60b0424 : /i2c at e60b0000
e6150000-e6150fff : /clock-controller at e6150000
e61f0000-e61f0013 : /thermal at e61f0000
e61f0100-e61f0137 : /thermal at e61f0000
e6530000-e653003f : /i2c at e6530000
e6590000-e65900ff : /usb at e6590000
e6590100-e65901ff : /usb-phy at e6590100
e65a0000-e65a00ff : /dma-controller at e65a0000
e65b0000-e65b00ff : /dma-controller at e65b0000
e6700000-e671ffff : /dma-controller at e6700000
e6720000-e673ffff : /dma-controller at e6720000
e6b10000-e6b1002b : /spi at e6b10000
e6e20000-e6e20063 : /spi at e6e20000
e6e60000-e6e6003f : e6e60000.serial
e6e68000-e6e6803f : e6e68000.serial
e6ef1000-e6ef1fff : /video at e6ef1000
ec500000-ec500fff : scu
ec540000-ec540fff : ssiu
ec541000-ec54127f : ssi
ec5a0000-ec5a00ff : adg
ec700000-ec70ffff : /dma-controller at ec700000
ec720000-ec72ffff : /dma-controller at ec720000
ec740000-ec7401ff : audmapp
ee090000-ee090bff : /pci at ee090000
ee0d0000-ee0d0bff : /pci at ee0d0000
ee300000-ee301fff : /sata at ee300000
ee700000-ee7003ff : /ethernet at ee700000
fe000000-fe07ffff : /pcie at fe000000
fe200000-fe3fffff : /pcie at fe000000
fe928000-fe92ffff : /vsp1 at fe928000
fe930000-fe937fff : /vsp1 at fe930000
fe938000-fe93ffff : /vsp1 at fe938000
fe980000-fe9902ff : /jpeg-codec at fe980000
feb00000-feb3ffff : du
feb90000-feb9001b : lvds.0
# cat /proc/ioports
00000000-000fffff : /pcie at fe000000
#
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Bjorn, use devm_request_resource() for host bridge resource is cool,
what about do the similar change for x86, now we request host bridge resource
in pci_acpi_root_add_resources() in x86, and we would release the host bridge
resource when host bridge device refcount reach 0. This logic may introduce issue,
E.g.
If we try to remove a pci host bridge, but there is a child pci device which refcount
cannot decrease to 0 after remove the device, in this case, its parent pci_bus and
parent device, all their refcount cannot reach to 0, the result is pci host bridge
refcount can not reach 0, so its .release_fn() won't be called, and host bridge
resouces can not release. If we want to add the pci host bridge again, all pci devices
can not work because the resource is conflict with the old.
devm resource would be released when the driver detach, this is better than what we do now, I think.
Thanks!
Yijing.
? 2016/6/19 2:07, Bjorn Helgaas ??:
On Mon, Jun 06, 2016 at 06:04:44PM -0500, Bjorn Helgaas wrote:
quoted
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
I merged this to my -next branch, so it should show up in linux-next
in a couple days. Let me know if you see any problems.
quoted
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(-)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[+cc Valentine]
Hi Geert,
Thanks a lot for testing this, and sorry for the breakage.
On Tue, Jun 21, 2016 at 12:41:31PM +0200, Geert Uytterhoeven wrote:
On Tue, Jun 7, 2016 at 1:07 AM, Bjorn Helgaas [off-list ref] wrote:
quoted
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/pci-rcar-gen2.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -275,6 +276,9 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)/* Add PCI resources */pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);+ret=devm_request_pci_bus_resources(priv->dev,&sys->resources);+if(ret<0)+returnret;/* Setup bus number based on platform device id / of bus-range */sys->busnr=priv->busnr;
This patch (commit 1bd019707b7c9249d34c5d348f1ef75eb4d83e89 in pci/next)
broke PCI on r8a7791/koelsch. Dmesg differences are:
pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11
-pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
-pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
-pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
This is probably a result of this code in drivers/pci/host/pci-rcar-gen2.c:
/*
* The controller does not support/use port I/O,
* so setup a dummy port I/O region here.
*/
priv->io_res.start = priv->mem_res.start;
priv->io_res.end = priv->mem_res.end;
priv->io_res.flags = IORESOURCE_IO;
We try to avoid adding dummy regions like this, but maybe we missed
this one. I haven't found any email discussion about it yet, so I
don't know what the reason for this one is. Valentine, do you
remember?
Can you try the patch below (apply it before the 1bd019707b7c patch
that broke things)?
commit b64dc28f5f2b3afe47ee4a42fb79db84ec4227f8
Author: Bjorn Helgaas [off-list ref]
Date: Tue Jun 21 09:19:34 2016 -0500
PCI: rcar: Drop gen2 dummy I/O port region
Previously we added a dummy I/O port region even though the R-Car
controller doesn't support PCI port I/O. This resulted in bogus root bus
resources like this:
pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
Drop the unused dummy I/O port region.
Signed-off-by: Bjorn Helgaas [off-list ref]
@@ -273,7 +272,6 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)rcar_pci_setup_errirq(priv);/* Add PCI resources */-pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);/* Setup bus number based on platform device id / of bus-range */
@@ -371,14 +369,6 @@ static int rcar_pci_probe(struct platform_device *pdev)return-ENOMEM;priv->mem_res=*mem_res;-/*-*Thecontrollerdoesnotsupport/useportI/O,-*sosetupadummyportI/Oregionhere.-*/-priv->io_res.start=priv->mem_res.start;-priv->io_res.end=priv->mem_res.end;-priv->io_res.flags=IORESOURCE_IO;-priv->cfg_res=cfg_res;priv->irq=platform_get_irq(pdev,0);
On Tue, Jun 21, 2016 at 07:58:08PM +0800, wangyijing wrote:
Hi Bjorn, use devm_request_resource() for host bridge resource is cool,
what about do the similar change for x86, now we request host bridge resource
in pci_acpi_root_add_resources() in x86, and we would release the host bridge
resource when host bridge device refcount reach 0. This logic may introduce issue,
E.g.
If we try to remove a pci host bridge, but there is a child pci device which refcount
cannot decrease to 0 after remove the device, in this case, its parent pci_bus and
parent device, all their refcount cannot reach to 0, the result is pci host bridge
refcount can not reach 0, so its .release_fn() won't be called, and host bridge
resouces can not release. If we want to add the pci host bridge again, all pci devices
can not work because the resource is conflict with the old.
devm resource would be released when the driver detach, this is better than what we do now, I think.
I'm not going to convert pci_root.c to use devm right now. That might
be a good thing, but this current series is mostly trivial. I think
changing pci_root.c would not be trivial, so that looks like a project
all by itself.
I don't quite follow the example of removing a host bridge while a
child PCI device refcount is non-zero. That sounds like an invalid
scenario regardless of whether the resources are released by a
.release_fn() or by devm.
Bjorn
On Mon, Jun 20, 2016 at 06:22:40PM +0100, Lorenzo Pieralisi wrote:
On Mon, Jun 20, 2016 at 09:56:45AM -0700, Tyler Baker wrote:
quoted
Hi Bjorn,
On 6 June 2016 at 16:06, Bjorn Helgaas [off-list ref] wrote:
quoted
Previously we allocated the PCI resource list in
gen_pci_parse_request_of_pci_ranges(), but if we had an error, we freed it
on error in gen_pci_init().
Reorder gen_pci_init() so we can take care of error path cleanup in
gen_pci_parse_request_of_pci_ranges() instead.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
The kernelci.org bot has reported[0] new qemu-aarch64
(arm64-defconfig) boot failures[1][2] in next-20160620. I've
bisected[3] this boot failure down to this patch, and confirmed
reverting it on top of next-20160620 resolves the boot issue.
I have not investigated further, but you can easily reproduce[4] the
boot failure on an x86 host running qemu-system-aarch64 (I'm running
qemu-system 2.6).
That's most likely because pci_ecam_create() requires the bus_range
resource (its busr parameter) to be initialized when it is called
and that's not the case after this patch is applied if I read it
correctly.
It is probably a NULL pointer dereference in pci_ecam_create().
Yep, thanks everybody, I dropped that ill-considered patch altogether.
On Tue, Jun 21, 2016 at 09:26:23AM -0500, Bjorn Helgaas wrote:
[+cc Valentine]
Hi Bjorn,
Hi Geert,
Thanks a lot for testing this, and sorry for the breakage.
On Tue, Jun 21, 2016 at 12:41:31PM +0200, Geert Uytterhoeven wrote:
quoted
On Tue, Jun 7, 2016 at 1:07 AM, Bjorn Helgaas [off-list ref] wrote:
quoted
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/pci-rcar-gen2.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -275,6 +276,9 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)/* Add PCI resources */pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);+ret=devm_request_pci_bus_resources(priv->dev,&sys->resources);+if(ret<0)+returnret;/* Setup bus number based on platform device id / of bus-range */sys->busnr=priv->busnr;
This patch (commit 1bd019707b7c9249d34c5d348f1ef75eb4d83e89 in pci/next)
broke PCI on r8a7791/koelsch. Dmesg differences are:
pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11
-pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
-pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
-pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
This is probably a result of this code in drivers/pci/host/pci-rcar-gen2.c:
/*
* The controller does not support/use port I/O,
* so setup a dummy port I/O region here.
*/
priv->io_res.start = priv->mem_res.start;
priv->io_res.end = priv->mem_res.end;
priv->io_res.flags = IORESOURCE_IO;
We try to avoid adding dummy regions like this, but maybe we missed
this one. I haven't found any email discussion about it yet, so I
don't know what the reason for this one is. Valentine, do you
remember?
I do not, but I think I've found something in my mailbox.
Below is the quote from the original conversation with:
(Re: [PATCH] pci: Add R-Car Gen2 internal PCI support)
[quote starts]
>>+ priv->mem_res = *mem_res;
>>+ /*
>>+ * The controller does not support/use port I/O,
>>+ * so setup a dummy port I/O region here.
>>+ */
>>+ priv->io_res.start = priv->mem_res.start;
>>+ priv->io_res.end = priv->mem_res.end;
>>+ priv->io_res.flags = IORESOURCE_IO;
>>
> I don't understand this. There's no requirement (at least as far as the
> PCI core is concerned) to supply an I/O aperture at all, and I think it
> would be better if you didn't.
>
> Oh, I see ... maybe pcibios_init_resources() forces you to have an
> I/O resource to avoid having it give you a default one? And I
> suppose that since you have several host bridges, these dummy I/O
> regions have to be distinct. Ugh. Well, I guess this is something
> you'd have to fix here or in the ARM code, it's up to you what to do.
Exactly. This is to avoid assigning default I/O resources.
[quote ends]
quoted hunk
Can you try the patch below (apply it before the 1bd019707b7c patch
that broke things)?
commit b64dc28f5f2b3afe47ee4a42fb79db84ec4227f8
Author: Bjorn Helgaas [off-list ref]
Date: Tue Jun 21 09:19:34 2016 -0500
PCI: rcar: Drop gen2 dummy I/O port region
Previously we added a dummy I/O port region even though the R-Car
controller doesn't support PCI port I/O. This resulted in bogus root bus
resources like this:
pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
Drop the unused dummy I/O port region.
Signed-off-by: Bjorn Helgaas [off-list ref]
@@ -273,7 +272,6 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)rcar_pci_setup_errirq(priv);/* Add PCI resources */-pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);/* Setup bus number based on platform device id / of bus-range */
@@ -371,14 +369,6 @@ static int rcar_pci_probe(struct platform_device *pdev)return-ENOMEM;priv->mem_res=*mem_res;-/*-*Thecontrollerdoesnotsupport/useportI/O,-*sosetupadummyportI/Oregionhere.-*/-priv->io_res.start=priv->mem_res.start;-priv->io_res.end=priv->mem_res.end;-priv->io_res.flags=IORESOURCE_IO;-priv->cfg_res=cfg_res;priv->irq=platform_get_irq(pdev,0);
On Tue, Jun 21, 2016 at 06:41:00PM +0300, Valentine Barshak wrote:
On Tue, Jun 21, 2016 at 09:26:23AM -0500, Bjorn Helgaas wrote:
quoted
[+cc Valentine]
Hi Bjorn,
quoted
Hi Geert,
Thanks a lot for testing this, and sorry for the breakage.
On Tue, Jun 21, 2016 at 12:41:31PM +0200, Geert Uytterhoeven wrote:
quoted
On Tue, Jun 7, 2016 at 1:07 AM, Bjorn Helgaas [off-list ref] wrote:
quoted
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/pci-rcar-gen2.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -275,6 +276,9 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)/* Add PCI resources */pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);+ret=devm_request_pci_bus_resources(priv->dev,&sys->resources);+if(ret<0)+returnret;/* Setup bus number based on platform device id / of bus-range */sys->busnr=priv->busnr;
This patch (commit 1bd019707b7c9249d34c5d348f1ef75eb4d83e89 in pci/next)
broke PCI on r8a7791/koelsch. Dmesg differences are:
pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11
-pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
-pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
-pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
This is probably a result of this code in drivers/pci/host/pci-rcar-gen2.c:
/*
* The controller does not support/use port I/O,
* so setup a dummy port I/O region here.
*/
priv->io_res.start = priv->mem_res.start;
priv->io_res.end = priv->mem_res.end;
priv->io_res.flags = IORESOURCE_IO;
We try to avoid adding dummy regions like this, but maybe we missed
this one. I haven't found any email discussion about it yet, so I
don't know what the reason for this one is. Valentine, do you
remember?
I do not, but I think I've found something in my mailbox.
Below is the quote from the original conversation with:
(Re: [PATCH] pci: Add R-Car Gen2 internal PCI support)
[quote starts]
>>+ priv->mem_res = *mem_res;
>>+ /*
>>+ * The controller does not support/use port I/O,
>>+ * so setup a dummy port I/O region here.
>>+ */
>>+ priv->io_res.start = priv->mem_res.start;
>>+ priv->io_res.end = priv->mem_res.end;
>>+ priv->io_res.flags = IORESOURCE_IO;
>>
> I don't understand this. There's no requirement (at least as far as the
> PCI core is concerned) to supply an I/O aperture at all, and I think it
> would be better if you didn't.
>
> Oh, I see ... maybe pcibios_init_resources() forces you to have an
> I/O resource to avoid having it give you a default one? And I
> suppose that since you have several host bridges, these dummy I/O
> regions have to be distinct. Ugh. Well, I guess this is something
> you'd have to fix here or in the ARM code, it's up to you what to do.
Exactly. This is to avoid assigning default I/O resources.
[quote ends]
Oh, right, now I remember. Thanks for digging that out.
I propose the patches below to remove the requirement for having an
I/O space. Any chance one of you could test them?
commit 8387e687f72747b994c54e29e31861f0db07eb0c
Author: Bjorn Helgaas [off-list ref]
Date: Tue Jun 21 10:54:29 2016 -0500
ARM: Make PCI I/O space optional
For callers of pci_common_init_dev(), we previously always required a PCI
I/O port resource. If the caller's ->setup() function had added an I/O
resource, we used that; otherwise, we added a default 64K I/O port space
for it.
There are PCI host bridges that do not support I/O port space, and we
should not add fictitious spaces for them.
If a caller sets struct hw_pci.io_optional, assume it is responsible for
adding any I/O port resource it desires, and do not add any default I/O
port space.
Signed-off-by: Bjorn Helgaas [off-list ref]
commit eeb4d6cdf8960c484c5a6eb9b310145b75a59ec1
Author: Bjorn Helgaas [off-list ref]
Date: Tue Jun 21 09:19:34 2016 -0500
PCI: rcar: Drop gen2 dummy I/O port region
Previously we added a dummy I/O port region even though the R-Car
controller doesn't support PCI port I/O. This resulted in bogus root bus
resources like this:
pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
Drop the unused dummy I/O port region and set struct hw_pci.io_optional so
the ARM PCI code doesn't add a default one for us.
Signed-off-by: Bjorn Helgaas [off-list ref]
@@ -273,7 +272,6 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)rcar_pci_setup_errirq(priv);/* Add PCI resources */-pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);/* Setup bus number based on platform device id / of bus-range */
@@ -371,14 +369,6 @@ static int rcar_pci_probe(struct platform_device *pdev)return-ENOMEM;priv->mem_res=*mem_res;-/*-*Thecontrollerdoesnotsupport/useportI/O,-*sosetupadummyportI/Oregionhere.-*/-priv->io_res.start=priv->mem_res.start;-priv->io_res.end=priv->mem_res.end;-priv->io_res.flags=IORESOURCE_IO;-priv->cfg_res=cfg_res;priv->irq=platform_get_irq(pdev,0);
@@ -421,6 +411,7 @@ static int rcar_pci_probe(struct platform_device *pdev)hw_private[0]=priv;memset(&hw,0,sizeof(hw));hw.nr_controllers=ARRAY_SIZE(hw_private);+hw.io_optional=1;hw.private_data=hw_private;hw.map_irq=rcar_pci_map_irq;hw.ops=&rcar_pci_ops;
On Tue, Jun 21, 2016 at 07:58:08PM +0800, wangyijing wrote:
quoted
Hi Bjorn, use devm_request_resource() for host bridge resource is cool,
what about do the similar change for x86, now we request host bridge resource
in pci_acpi_root_add_resources() in x86, and we would release the host bridge
resource when host bridge device refcount reach 0. This logic may introduce issue,
E.g.
If we try to remove a pci host bridge, but there is a child pci device which refcount
cannot decrease to 0 after remove the device, in this case, its parent pci_bus and
parent device, all their refcount cannot reach to 0, the result is pci host bridge
refcount can not reach 0, so its .release_fn() won't be called, and host bridge
resouces can not release. If we want to add the pci host bridge again, all pci devices
can not work because the resource is conflict with the old.
devm resource would be released when the driver detach, this is better than what we do now, I think.
I'm not going to convert pci_root.c to use devm right now. That might
be a good thing, but this current series is mostly trivial. I think
changing pci_root.c would not be trivial, so that looks like a project
all by itself.
OK.
I don't quite follow the example of removing a host bridge while a
child PCI device refcount is non-zero. That sounds like an invalid
scenario regardless of whether the resources are released by a
.release_fn() or by devm.
I would send a draft patch to describe and fix the issue, because it's not related to
this series, so let's discuess it in another thread. :)
Thanks!
Yijing.
Hi Bjorn,
On Tue, Jun 21, 2016 at 6:49 PM, Bjorn Helgaas [off-list ref] wrote:
On Tue, Jun 21, 2016 at 06:41:00PM +0300, Valentine Barshak wrote:
quoted
On Tue, Jun 21, 2016 at 09:26:23AM -0500, Bjorn Helgaas wrote:
quoted
On Tue, Jun 21, 2016 at 12:41:31PM +0200, Geert Uytterhoeven wrote:
quoted
On Tue, Jun 7, 2016 at 1:07 AM, Bjorn Helgaas [off-list ref] wrote:
quoted
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/pci-rcar-gen2.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -275,6 +276,9 @@ static int rcar_pci_setup(int nr, struct pci_sys_data *sys)/* Add PCI resources */pci_add_resource(&sys->resources,&priv->io_res);pci_add_resource(&sys->resources,&priv->mem_res);+ret=devm_request_pci_bus_resources(priv->dev,&sys->resources);+if(ret<0)+returnret;/* Setup bus number based on platform device id / of bus-range */sys->busnr=priv->busnr;
This patch (commit 1bd019707b7c9249d34c5d348f1ef75eb4d83e89 in pci/next)
broke PCI on r8a7791/koelsch. Dmesg differences are:
pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11
-pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
-pci_bus 0000:00: root bus resource [io 0xee080000-0xee0810ff]
-pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
This is probably a result of this code in drivers/pci/host/pci-rcar-gen2.c:
/*
* The controller does not support/use port I/O,
* so setup a dummy port I/O region here.
*/
priv->io_res.start = priv->mem_res.start;
priv->io_res.end = priv->mem_res.end;
priv->io_res.flags = IORESOURCE_IO;
We try to avoid adding dummy regions like this, but maybe we missed
this one. I haven't found any email discussion about it yet, so I
don't know what the reason for this one is. Valentine, do you
remember?
I do not, but I think I've found something in my mailbox.
Below is the quote from the original conversation with:
(Re: [PATCH] pci: Add R-Car Gen2 internal PCI support)
[quote starts]
>>+ priv->mem_res = *mem_res;
>>+ /*
>>+ * The controller does not support/use port I/O,
>>+ * so setup a dummy port I/O region here.
>>+ */
>>+ priv->io_res.start = priv->mem_res.start;
>>+ priv->io_res.end = priv->mem_res.end;
>>+ priv->io_res.flags = IORESOURCE_IO;
>>
> I don't understand this. There's no requirement (at least as far as the
> PCI core is concerned) to supply an I/O aperture at all, and I think it
> would be better if you didn't.
>
> Oh, I see ... maybe pcibios_init_resources() forces you to have an
> I/O resource to avoid having it give you a default one? And I
> suppose that since you have several host bridges, these dummy I/O
> regions have to be distinct. Ugh. Well, I guess this is something
> you'd have to fix here or in the ARM code, it's up to you what to do.
Exactly. This is to avoid assigning default I/O resources.
[quote ends]
Oh, right, now I remember. Thanks for digging that out.
I propose the patches below to remove the requirement for having an
I/O space. Any chance one of you could test them?
Thanks!
PCI seems to be working again on r8a7791/koelsch using today's pci/next,
which includes these patches. Lspci shows the USB controllers again.
/proc/iomem gained a few entries:
ec700000-ec70ffff : /dma-controller at ec700000
ec720000-ec72ffff : /dma-controller at ec720000
ec740000-ec7401ff : audmapp
+ee080000-ee0810ff : /pci at ee090000
+ ee080000-ee080fff : 0000:00:01.0
+ ee081000-ee0810ff : 0000:00:02.0
ee090000-ee090bff : /pci at ee090000
+ee0c0000-ee0c10ff : /pci at ee0d0000
+ ee0c0000-ee0c0fff : 0001:01:01.0
+ ee0c1000-ee0c10ff : 0001:01:02.0
ee0d0000-ee0d0bff : /pci at ee0d0000
ee300000-ee301fff : /sata at ee300000
ee700000-ee7003ff : /ethernet at ee700000
Hence
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds