add support for Xilinx PCIe root ports on RISC-V v3
Hi all,
this series with patches originally from Palmer and Wesley adds support for the pcie-xilinx host driver on RISC-V boards. The interesting part about that is that the IP blocks is limited to 32-bit DMA internally, which didn't seem to be an issue with the existing users, but shows up easily with the Sifive RISC-V boards that have physical memory wired up above 4G.
Note that patches 1 and 2 depend on changes in the dma-mapping tree to add the bus_dma_mask field to struct device and would have to merge through the dma-mapping tree or a shared stable branch. Patch 3 could be merged independently.
Changes since v2:
- rename the add_dev callback to add_device and allow it to return an error code (to allow it to replace pcibios_add_device in the future)
Changes since v1:
- move the add_dev method to struct pci_host_bridge
- use the new bus_dma_mask field
PATCH 1/3 PCI: add a callback to struct pci_host_bridge for adding a new device
There is currently no way for a PCIe bridge to impose constraints on devices added to it. For example, the Xilinx PCIe host bridge only supports 32-bit physical addresses (due to a limitation on the AXI port's address width). Thus, even devices that claim to support 64-bit DMA addresses must be restricted to 32-bit addresses when attached to this host controller.
This patch adds a "add_device" method to struct pci_host_bridge that allows the host driver to act upon acting adding devices.
Based on an earlier patch from Wesley W. Terpstra <[email protected]>.
--- drivers/pci/probe.c | 6 ++++++ include/linux/pci.h | 1 + 2 files changed, 7 insertions(+)
drivers/pci/probe.c +6 -0
@@ -2290,6 +2290,7 @@ static void pci_set_msi_domain(struct pci_dev *dev)void pci_device_add (struct pci_dev *dev,struct pci_bus *bus) { +struct pci_host_bridge *host =pci_find_host_bridge (bus);int ret;pci_configure_device (dev);
@@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) ret =pcibios_add_device (dev);WARN_ON (ret <0 ); +if (host->add_device) { + ret = host->add_device (dev); +WARN_ON (ret <0 ); + } +/* Set up MSI IRQ domain */ pci_set_msi_domain (dev);
include/linux/pci.h +1 -0
@@ -485,6 +485,7 @@ struct pci_host_bridge {resource_size_t start, resource_size_t size, resource_size_t align); +int (*add_device)(struct pci_dev *dev);unsigned long private[0 ]____cacheline_aligned ; };
Signature · 1 line
2.18.0
Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device
Quoted 31 lines · Christoph Hellwig
There is currently no way for a PCIe bridge to impose constraints on devices added to it. For example, the Xilinx PCIe host bridge only supports 32-bit physical addresses (due to a limitation on the AXI port's address width). Thus, even devices that claim to support 64-bit DMA addresses must be restricted to 32-bit addresses when attached to this host controller.
This patch adds a "
add_device" method to structpci_host_bridgethat allows the host driver to act upon acting adding devices.Based on an earlier patch from Wesley W. Terpstra <[email protected]>.
Signed-off-by Christoph Hellwig <[email protected]>--- drivers/pci/probe.c | 6 ++++++ include/linux/pci.h | 1 + 2 files changed, 7 insertions(+)drivers/pci/probe.c +1
-0@@ -2290,6 +2290,7 @@ static void pci_set_msi_domain(struct pci_dev *dev)void pci_device_add (struct pci_dev *dev,struct pci_bus *bus) { +struct pci_host_bridge *host =pci_find_host_bridge (bus);int ret;pci_configure_device (dev);
drivers/pci/probe.c void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) · line 2335
Quoted 7 lines
drivers/pci/probe.c +4
-0@@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) ret =pcibios_add_device (dev);WARN_ON (ret <0 ); +if (host->add_device) { + ret = host->add_device (dev); +WARN_ON (ret <0 ); + }
This looks fine; we could go a step further and make the hunk above the default (weak) implementation of pcibios_add_device() that is currently a NOP returning 0, I will remove it for v4.20.
I am fine with the patch as-is (even though I am not a big fan of those WARN_ON but that's not a problem related to your patch, as you said we are *not* handling the ret value in the current mainline).
Quoted 19 lines
drivers/pci/probe.c +1
-0+/* Set up MSI IRQ domain */ pci_set_msi_domain (dev);include/linux/pci.h +1
-0@@ -485,6 +485,7 @@ struct pci_host_bridge {resource_size_t start, resource_size_t size, resource_size_t align); +int (*add_device)(struct pci_dev *dev);unsigned long private[0 ]____cacheline_aligned ; };Signature · 1 line
2.18.0
Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device
Quoted 5 lines · Lorenzo Pieralisi
Quoted 5 lines
drivers/pci/probe.c +0
-0pci_configure_device (dev);@@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) ret =pcibios_add_device (dev);WARN_ON (ret <0 );
Quoted 8 lines
drivers/pci/probe.c void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) · line 2335
Quoted 4 lines
drivers/pci/probe.c +4
-0+if (host->add_device) { + ret = host->add_device (dev); +WARN_ON (ret <0 ); + }This looks fine; we could go a step further and make the hunk above the default (weak) implementation of
pcibios_add_device()that is currently a NOP returning 0, I will remove it for v4.20.
I'd love to see pcibios_add_device go away entirely. But I wonder how to get setup the pci_host_bridge pointer for the remaining architectures that still implement it. I did look for any easy way but couldn't find one. But then I don't really know this area of the PCI code too well.
Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device
Quoted 16 lines · Christoph Hellwig
Quoted 5 lines · Lorenzo Pieralisi
Quoted 5 lines
pci_configure_device(dev);drivers/pci/probe.c +0
-0@@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) ret =pcibios_add_device (dev);WARN_ON (ret <0 );Quoted 8 lines
drivers/pci/probe.c void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) · line 2335
Quoted 4 lines
drivers/pci/probe.c +4
-0+if (host->add_device) { + ret = host->add_device (dev); +WARN_ON (ret <0 ); + }This looks fine; we could go a step further and make the hunk above the default (weak) implementation of
pcibios_add_device()that is currently a NOP returning 0, I will remove it for v4.20.
Quoted 4 lines
I'd love to see
pcibios_add_devicego away entirely. But I wonder how to get setup thepci_host_bridgepointer for the remaining architectures that still implement it. I did look for any easy way but couldn't find one. But then I don't really know this area of the PCI code too well.
If the platform doesn't already have a pointer to its pci_host_bridge structure, it can call pci_find_host_bridge(bus->dev) to get it.
Ideally, platforms (either arch specific code or drivers/pci/controller) should call pci_alloc_host_bridge()/pci_register_host_bridge() instead of the traditional pci_create_root_bus() that is less flexible. That way, the driver specific structure can get allocated together with the host bridge.
Arnd
Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device
Quoted 22 lines · Arnd Bergmann
Quoted 16 lines · Christoph Hellwig
Quoted 5 lines · Lorenzo Pieralisi
Quoted 5 lines
pci_configure_device(dev);drivers/pci/probe.c +0
-0@@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) ret =pcibios_add_device (dev);WARN_ON (ret <0 );Quoted 8 lines
drivers/pci/probe.c void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) · line 2335
Quoted 4 lines
drivers/pci/probe.c +4
-0+if (host->add_device) { + ret = host->add_device (dev); +WARN_ON (ret <0 ); + }This looks fine; we could go a step further and make the hunk above the default (weak) implementation of
pcibios_add_device()that is currently a NOP returning 0, I will remove it for v4.20.Quoted 4 lines
I'd love to see
pcibios_add_devicego away entirely. But I wonder how to get setup thepci_host_bridgepointer for the remaining architectures that still implement it. I did look for any easy way but couldn't find one. But then I don't really know this area of the PCI code too well.
Quoted 8 lines
If the platform doesn't already have a pointer to its
pci_host_bridgestructure, it can callpci_find_host_bridge(bus->dev)to get it.Ideally, platforms (either arch specific code or
drivers/pci/controller) should call pci_alloc_host_bridge()/pci_register_host_bridge() instead of the traditionalpci_create_root_bus()that is less flexible. That way, the driver specific structure can get allocated together with the host bridge.
Yes that's basically the gist, it should be a matter of identifying code that creates the root bus (explicitly or implicitly - ie pci_scan_root_bus()) and patching it up, probably something to be done (and moved into -next) early -rc* since it is something that can easily break the world if we miss some code paths. I will have a look into that and can take this patch for that series if it does not go into v4.19.
Thanks,
Lorenzo
Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device
Quoted 33 lines · Lorenzo Pieralisi
Quoted 22 lines · Arnd Bergmann
Quoted 16 lines · Christoph Hellwig
Quoted 5 lines · Lorenzo Pieralisi
Quoted 5 lines
pci_configure_device(dev);drivers/pci/probe.c +0
-0@@ -2328,6 +2329,11 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) ret =pcibios_add_device (dev);WARN_ON (ret <0 );Quoted 8 lines
drivers/pci/probe.c void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) · line 2335
Quoted 4 lines
drivers/pci/probe.c +4
-0+if (host->add_device) { + ret = host->add_device (dev); +WARN_ON (ret <0 ); + }This looks fine; we could go a step further and make the hunk above the default (weak) implementation of
pcibios_add_device()that is currently a NOP returning 0, I will remove it for v4.20.Quoted 4 lines
I'd love to see
pcibios_add_devicego away entirely. But I wonder how to get setup thepci_host_bridgepointer for the remaining architectures that still implement it. I did look for any easy way but couldn't find one. But then I don't really know this area of the PCI code too well.Quoted 8 lines
If the platform doesn't already have a pointer to its
pci_host_bridgestructure, it can callpci_find_host_bridge(bus->dev)to get it.Ideally, platforms (either arch specific code or
drivers/pci/controller) should call pci_alloc_host_bridge()/pci_register_host_bridge() instead of the traditionalpci_create_root_bus()that is less flexible. That way, the driver specific structure can get allocated together with the host bridge.
Quoted 7 lines
Yes that's basically the gist, it should be a matter of identifying code that creates the root bus (explicitly or implicitly - ie
pci_scan_root_bus()) and patching it up, probably something to be done (and moved into -next) early -rc* since it is something that can easily break the world if we miss some code paths. I will have a look into that and can take this patch for that series if it does not go into v4.19.
I've started prototyping something now, will post something tomorrow. I have an idea for a version that should be minimally invasive, at the cost of duplicating some code in drivers that still use the old interfaces.
Arnd
PATCH 2/3 PCI/xilinx: Work-around for hardware DMA limit (32 bits)
In reply to Christoph HellwigThis PCIe bridge only has a 32 bit bus master interface, thus truncating the DMA capability of all PCIe devices attached beneath it. This caps the child device capability so that these devices work on systems with physical memory beyond the 4GiB threshold.
Based on an earlier patch from Wesley W. Terpstra <[email protected]>.
--- drivers/pci/controller/pcie-xilinx.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/pci/controller/pcie-xilinx.c +11 -0
@@ -197,6 +197,16 @@ static void __iomem *xilinx_pcie_map_bus(struct pci_bus *bus,return port->reg_base + relbus + where; } +/* +* This PCIe bridge only has a 32 bit bus master interface, thus truncating +* the DMA capability of all PCIe devices attached beneath it. +*/ +static int xilinx_pcie_add_device (struct pci_dev *pdev) +{ + pdev->dev.bus_dma_mask =DMA_BIT_MASK (32 ); +return 0 ; +} +/* PCIe operations */ static struct pci_ops xilinx_pcie_ops = { .map_bus = xilinx_pcie_map_bus,
@@ -665,6 +675,7 @@ static int xilinx_pcie_probe(struct platform_device *pdev) bridge->ops = &xilinx_pcie_ops; bridge->map_irq = of_irq_parse_and_map_pci; bridge->swizzle_irq = pci_common_swizzle; + bridge->add_device = xilinx_pcie_add_device;#ifdef CONFIG_PCI_MSI xilinx_pcie_msi_chip.dev = dev;
Signature · 1 line
2.18.0
Re: [PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)
FYI, This Xilinx PCIe IP 32-bit cap only applies to SOME instances of the IP. The Ultrascale+ version of Xilinx PCIe hard IP does support 64-bit or 32-bit. The Virtex7 version only supports 32-bit. The pcie-xilinx driver woks with both of these root complexes. So probably there should be a conditional hook in the DTS that triggers the work-around behaviour.
Quoted 44 lines · Christoph Hellwig
This PCIe bridge only has a 32 bit bus master interface, thus truncating the DMA capability of all PCIe devices attached beneath it. This caps the child device capability so that these devices work on systems with physical memory beyond the 4GiB threshold.
Based on an earlier patch from Wesley W. Terpstra <[email protected]>.
Signed-off-by Christoph Hellwig <[email protected]>--- drivers/pci/controller/pcie-xilinx.c | 11 +++++++++++ 1 file changed, 11 insertions(+)drivers/pci/controller/pcie-xilinx.c +11
-1@@ -197,6 +197,16 @@ static void __iomem *xilinx_pcie_map_bus(struct pci_bus *bus,return port->reg_base + relbus + where; } +/* +* This PCIe bridge only has a 32 bit bus master interface, thus truncating +* the DMA capability of all PCIe devices attached beneath it. +*/ +static int xilinx_pcie_add_device (struct pci_dev *pdev) +{ + pdev->dev.bus_dma_mask =DMA_BIT_MASK (32 ); +return 0 ; +} +/* PCIe operations */ static struct pci_ops xilinx_pcie_ops = { .map_bus = xilinx_pcie_map_bus,@@ -665,6 +675,7 @@ static int xilinx_pcie_probe(struct platform_device *pdev) bridge->ops = &xilinx_pcie_ops; bridge->map_irq = of_irq_parse_and_map_pci; bridge->swizzle_irq = pci_common_swizzle; + bridge->add_device = xilinx_pcie_add_device;#ifdef CONFIG_PCI_MSI xilinx_pcie_msi_chip.dev = dev; --2.18.0
Re: [PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)
Quoted 6 lines · Wesley Terpstra
FYI, This Xilinx PCIe IP 32-bit cap only applies to SOME instances of the IP. The Ultrascale+ version of Xilinx PCIe hard IP does support 64-bit or 32-bit. The Virtex7 version only supports 32-bit. The pcie-xilinx driver woks with both of these root complexes. So probably there should be a conditional hook in the DTS that triggers the work-around behaviour.
Either we'll need to able to detect which version we use based on registrs, or we will indeed need firmware information.
Note that we already have the mechanism for firmware directed dma limits in place, it is called the dma-ranges DT property. If we can get the SiFive firmware to set it up properly the RISC-V swiotlb code will just do the right thing.
Re: [PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)
Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Wesley Terpstra
FYI, This Xilinx PCIe IP 32-bit cap only applies to SOME instances of the IP. The Ultrascale+ version of Xilinx PCIe hard IP does support 64-bit or 32-bit. The Virtex7 version only supports 32-bit. The pcie-xilinx driver woks with both of these root complexes. So probably there should be a conditional hook in the DTS that triggers the work-around behaviour.
Quoted 7 lines
Either we'll need to able to detect which version we use based on registrs, or we will indeed need firmware information.
Note that we already have the mechanism for firmware directed dma limits in place, it is called the dma-ranges DT property. If we can get the SiFive firmware to set it up properly the RISC-V swiotlb code will just do the right thing.
It would do the right thing without patches 1 and 2 (I would consider merging patch 1 anyway - see pcibios_add_device() removal).
It seems to me that the best course of action consists in patching firmware, that would remove the need for patch 2 (I will merge patch 1 anyway - even if that's for a "different" purpose, see pcibios_add_device() removal), please let us know.
Thanks,
Lorenzo
Re: [PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)
Quoted 8 lines · Lorenzo Pieralisi
It would do the right thing without patches 1 and 2 (I would consider merging patch 1 anyway - see
pcibios_add_device()removal).It seems to me that the best course of action consists in patching firmware, that would remove the need for patch 2 (I will merge patch 1 anyway - even if that's for a "different" purpose, see
pcibios_add_device()removal), please let us know.
Sounds good to me. Patch 1 also doesn't even depend on dma-mapping bits so feel free to pull it in when you think is the best time.
Re: [PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)
In reply to Christoph HellwigQuoted 4 lines · Christoph Hellwig
Note that we already have the mechanism for firmware directed dma limits in place, it is called the dma-ranges DT property. If we can get the SiFive firmware to set it up properly the RISC-V swiotlb code will just do the right thing.
Does this mean we only need to set the dma-ranges property inside the pci DTS node? No changes to the driver needed?
Re: [PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)
Quoted 8 lines · Wesley Terpstra
Quoted 4 lines · Christoph Hellwig
Note that we already have the mechanism for firmware directed dma limits in place, it is called the dma-ranges DT property. If we can get the SiFive firmware to set it up properly the RISC-V swiotlb code will just do the right thing.
Does this mean we only need to set the dma-ranges property inside the pci DTS node? No changes to the driver needed?
The code looks at the DT parent of the PCI bridge device. Take a look at drivers/pci/pci-driver.c:pci_dma_configure() and drivers/of/device.c:of_dma_configure() in 4.18-rc (for older kernels the involved functions are slightly different, but the functionality is the same).
PATCH 3/3 PCI/xilinx: Depend on OF instead of the ARCH
In reply to Christoph HellwigThere isn't a hard dependency of the Xilinx AXI-PCIe host bridge on any architecture. For example: at SiFive we map RISC-V cores to Xilinx FPGAs and connect the Xilinx IP via a TileLink adapter, so the RISC-V Linux port will need to be able to enable PCIE_XILINX in order to have PCIe support.
This patch decouples the PCIE_XILINX support from ARCH. Instead it just depends on OF, which I believe is the only true dependency.
--- drivers/pci/controller/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/Kconfig +1 -1
@@ -102,7 +102,7 @@ config PCI_HOST_GENERIC config PCIE_XILINX bool "Xilinx AXI PCIe host bridge support" - depends on ARCH_ZYNQ || MICROBLAZE || (MIPS && PCI_DRIVERS_GENERIC) || COMPILE_TEST + depends on OF || COMPILE_TEST help Say 'Y' here if you want kernel to support the Xilinx AXI PCIe Host Bridge driver.
Signature · 1 line
2.18.0
Re: [PATCH 3/3] PCI/xilinx: Depend on OF instead of the ARCH
Quoted 9 lines · Christoph Hellwig
There isn't a hard dependency of the Xilinx AXI-PCIe host bridge on any architecture. For example: at SiFive we map RISC-V cores to Xilinx FPGAs and connect the Xilinx IP via a TileLink adapter, so the RISC-V Linux port will need to be able to enable
PCIE_XILINXin order to have PCIe support.This patch decouples the
PCIE_XILINXsupport from ARCH. Instead it just depends on OF, which I believe is the only true dependency.
Quoted 6 lines
Signed-off-by Palmer Dabbelt <[email protected]>[hch: switch to OF instead of OF_PCI now that the latter is gone]Signed-off-by Christoph Hellwig <[email protected]>--- drivers/pci/controller/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
I have applied it to pci/controller/misc for v4.19, even though I am not sure Bjorn will have time to put into -next and send it for the upcoming merge window, we shall try.
Thanks,
Lorenzo
Quoted 16 lines
drivers/pci/controller/Kconfig +1
-1@@ -102,7 +102,7 @@ config PCI_HOST_GENERIC config PCIE_XILINX bool "Xilinx AXI PCIe host bridge support" - depends on ARCH_ZYNQ || MICROBLAZE || (MIPS && PCI_DRIVERS_GENERIC) || COMPILE_TEST + depends on OF || COMPILE_TEST help Say 'Y' here if you want kernel to support the Xilinx AXI PCIe Host Bridge driver.Signature · 1 line
2.18.0