add support for Xilinx PCIe root ports on RISC-V v3

add support for Xilinx PCIe root ports on RISC-V v3

[Christoph Hellwig]

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

add support for Xilinx PCIe root ports on RISC-V v3

[Christoph Hellwig]

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

[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 struct pci_host_bridge that
allows the host driver to act upon acting adding devices.

Based on an earlier patch from Wesley W. Terpstra <wesley@sifive.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/probe.c | 6 ++++++
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac876e32de4b..452190fb05e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index abd5d5e17aee..1524adbb30ab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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;
 };
 
-- 
2.18.0

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[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 struct pci_host_bridge that
allows the host driver to act upon acting adding devices.

Based on an earlier patch from Wesley W. Terpstra <wesley@sifive.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/probe.c | 6 ++++++
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac876e32de4b..452190fb05e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index abd5d5e17aee..1524adbb30ab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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;
 };
 
-- 
2.18.0

[PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[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 <wesley@sifive.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/controller/pcie-xilinx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index 7b1389d8e2a5..ccfd91e0515f 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -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

[PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[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 <wesley@sifive.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/controller/pcie-xilinx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index 7b1389d8e2a5..ccfd91e0515f 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -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

[PATCH 3/3] PCI/xilinx: Depend on OF instead of the ARCH

[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_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.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: switch to OF instead of OF_PCI now that the latter is gone]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/controller/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index cc9fa02d32a0..0f7ce5eaeac8 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -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.
-- 
2.18.0

[PATCH 3/3] PCI/xilinx: Depend on OF instead of the ARCH

[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_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.

Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
[hch: switch to OF instead of OF_PCI now that the latter is gone]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/controller/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index cc9fa02d32a0..0f7ce5eaeac8 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -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.
-- 
2.18.0

Re: [PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[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.

On Sat, Aug 4, 2018 at 3:14 AM, Christoph Hellwig <hch@lst.de> wrote:
> 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 <wesley@sifive.com>.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/controller/pcie-xilinx.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
> index 7b1389d8e2a5..ccfd91e0515f 100644
> --- a/drivers/pci/controller/pcie-xilinx.c
> +++ b/drivers/pci/controller/pcie-xilinx.c
> @@ -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
>

[PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[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.

On Sat, Aug 4, 2018 at 3:14 AM, Christoph Hellwig <hch@lst.de> wrote:
> 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 <wesley@sifive.com>.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/controller/pcie-xilinx.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
> index 7b1389d8e2a5..ccfd91e0515f 100644
> --- a/drivers/pci/controller/pcie-xilinx.c
> +++ b/drivers/pci/controller/pcie-xilinx.c
> @@ -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 3/3] PCI/xilinx: Depend on OF instead of the ARCH

[Lorenzo Pieralisi]

On Sat, Aug 04, 2018 at 12:14:02PM +0200, Christoph Hellwig wrote:
> 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_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.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [hch: switch to OF instead of OF_PCI now that the latter is gone]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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

> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index cc9fa02d32a0..0f7ce5eaeac8 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -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.
> -- 
> 2.18.0
> 

[PATCH 3/3] PCI/xilinx: Depend on OF instead of the ARCH

[Lorenzo Pieralisi]

On Sat, Aug 04, 2018 at 12:14:02PM +0200, Christoph Hellwig wrote:
> 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_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.
> 
> Signed-off-by: Palmer Dabbelt <palmer@dabbelt.com>
> [hch: switch to OF instead of OF_PCI now that the latter is gone]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  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

> diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
> index cc9fa02d32a0..0f7ce5eaeac8 100644
> --- a/drivers/pci/controller/Kconfig
> +++ b/drivers/pci/controller/Kconfig
> @@ -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.
> -- 
> 2.18.0
> 

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Lorenzo Pieralisi]

On Sat, Aug 04, 2018 at 12:14:00PM +0200, Christoph Hellwig wrote:
> 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 <wesley@sifive.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/probe.c | 6 ++++++
>  include/linux/pci.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e32de4b..452190fb05e7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);
> +	}

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).

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> +
>  	/* Set up MSI IRQ domain */
>  	pci_set_msi_domain(dev);
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index abd5d5e17aee..1524adbb30ab 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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;
>  };
>  
> -- 
> 2.18.0
> 

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Lorenzo Pieralisi]

On Sat, Aug 04, 2018 at 12:14:00PM +0200, Christoph Hellwig wrote:
> 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 <wesley@sifive.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/probe.c | 6 ++++++
>  include/linux/pci.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e32de4b..452190fb05e7 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);
> +	}

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).

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

> +
>  	/* Set up MSI IRQ domain */
>  	pci_set_msi_domain(dev);
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index abd5d5e17aee..1524adbb30ab 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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;
>  };
>  
> -- 
> 2.18.0
> 

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Christoph Hellwig]

On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> >  	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);
> > +	}
> 
> 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.

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Christoph Hellwig]

On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> >  	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);
> > +	}
> 
> 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 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[Christoph Hellwig]

On Sun, Aug 05, 2018 at 01:02:58PM -0700, Wesley Terpstra wrote:
> 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.

[PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[Christoph Hellwig]

On Sun, Aug 05, 2018 at 01:02:58PM -0700, Wesley Terpstra wrote:
> 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)

[Lorenzo Pieralisi]

On Mon, Aug 06, 2018 at 02:35:27PM +0200, Christoph Hellwig wrote:
> On Sun, Aug 05, 2018 at 01:02:58PM -0700, Wesley Terpstra wrote:
> > 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.

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

[PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[Lorenzo Pieralisi]

On Mon, Aug 06, 2018 at 02:35:27PM +0200, Christoph Hellwig wrote:
> On Sun, Aug 05, 2018 at 01:02:58PM -0700, Wesley Terpstra wrote:
> > 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.

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 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > >     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);
> > > +   }
> >
> > 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.

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

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > >     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);
> > > +   }
> >
> > 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.

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

[Lorenzo Pieralisi]

On Mon, Aug 06, 2018 at 03:54:13PM +0200, Arnd Bergmann wrote:
> On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > > >     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);
> > > > +   }
> > >
> > > 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.
> 
> 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.

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

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Lorenzo Pieralisi]

On Mon, Aug 06, 2018 at 03:54:13PM +0200, Arnd Bergmann wrote:
> On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > > >     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);
> > > > +   }
> > >
> > > 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.
> 
> 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.

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 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[Christoph Hellwig]

On Mon, Aug 06, 2018 at 02:40:43PM +0100, Lorenzo Pieralisi wrote:
> 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.

[PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[Christoph Hellwig]

On Mon, Aug 06, 2018 at 02:40:43PM +0100, Lorenzo Pieralisi wrote:
> 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)

[Wesley Terpstra]

On Mon, Aug 6, 2018 at 5:35 AM, Christoph Hellwig <hch@lst.de> wrote:
> 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?

[PATCH 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[Wesley Terpstra]

On Mon, Aug 6, 2018 at 5:35 AM, Christoph Hellwig <hch@lst.de> wrote:
> 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)

[Christoph Hellwig]

On Mon, Aug 06, 2018 at 09:21:40AM -0700, Wesley Terpstra wrote:
> On Mon, Aug 6, 2018 at 5:35 AM, Christoph Hellwig <hch@lst.de> wrote:
> > 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 2/3] PCI/xilinx: Work-around for hardware DMA limit (32 bits)

[Christoph Hellwig]

On Mon, Aug 06, 2018 at 09:21:40AM -0700, Wesley Terpstra wrote:
> On Mon, Aug 6, 2018 at 5:35 AM, Christoph Hellwig <hch@lst.de> wrote:
> > 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).

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Mon, Aug 6, 2018 at 4:56 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Aug 06, 2018 at 03:54:13PM +0200, Arnd Bergmann wrote:
> > On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > > > >     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);
> > > > > +   }
> > > >
> > > > 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.
> >
> > 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.
>
> 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 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Mon, Aug 6, 2018 at 4:56 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, Aug 06, 2018 at 03:54:13PM +0200, Arnd Bergmann wrote:
> > On Mon, Aug 6, 2018 at 2:25 PM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > On Mon, Aug 06, 2018 at 12:23:10PM +0100, Lorenzo Pieralisi wrote:
> > > > >     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);
> > > > > +   }
> > > >
> > > > 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.
> >
> > 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.
>
> 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

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Fri, Aug 17, 2018 at 5:25 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Aug 16, 2018 at 11:04:53PM +0200, Arnd Bergmann wrote:
> > On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> >
> > > > This patch seems OK to me.
> > > >
> > > > I don't really care about the prototype.  There's only one
> > > > pcibios_add_device() implementation (x86) that returns anything other
> > > > than 0, and that's a pretty obscure error case related to f9a37be0f02a
> > > > ("x86: Use PCI setup data"), which lets us use ROM data from boot
> > > > services.  Even then the only thing that happens is a WARN_ON().  A
> > > > more descriptive printk would be a lot more useful.
> > >
> > > Thinking about this some more, I'm not so sure about the connection
> > > with removing pcibios_add_device().  This host_bridge->add_dev() hook
> > > would be for host bridge-specific things, while pcibios_add_device()
> > > is for arch-specific things.
> > >
> > > I'd still love to get rid of pcibios_add_device() (especially the
> > > non-arch-specific things like the pci_claim_resource() in s390); I'm
> > > just not sure yet whether this particular patch is the vehicle.
> >
> > I think most of the arch-specific pcibios_* calls are actually
> > host bridge specific after all, it just so happens that they are
> > implemented on architectures that only have one specific
> > host bridge implementation, or that they are used on an
> > architecture that does something odd in one place and needs
> > to do something else in another place.
> >
> > For pci_claim_resource() we seem to be doing this in a number
> > of different places, but there isn't strictly a reason for that.
>
> pci_claim_resource() is needed if either arch code or the host
> controller driver does not trigger a resources assignment (which claims
> them while at it); in theory that's arch agnostic but it turned out to
> be very arch/platform specific - aka if we move s390 code to core code
> we will notice :) so pci_claim_resource() in a pcibios call is
> unfortunately legitimate - whether it can be moved out of it to
> generic code that's a very complicated problem.

The generic pci_host_probe() already does either pci_bus_claim_resources() or
pci_bus_size_bridges()/pci_bus_assign_resources()/pcie_bus_configure_settings()

I would hope that we can move a lot of implementations over to just
call this function, with the appropriate PCI_PROBE_ONLY setting.

       Arnd

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Fri, Aug 17, 2018 at 5:25 PM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Thu, Aug 16, 2018 at 11:04:53PM +0200, Arnd Bergmann wrote:
> > On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> >
> > > > This patch seems OK to me.
> > > >
> > > > I don't really care about the prototype.  There's only one
> > > > pcibios_add_device() implementation (x86) that returns anything other
> > > > than 0, and that's a pretty obscure error case related to f9a37be0f02a
> > > > ("x86: Use PCI setup data"), which lets us use ROM data from boot
> > > > services.  Even then the only thing that happens is a WARN_ON().  A
> > > > more descriptive printk would be a lot more useful.
> > >
> > > Thinking about this some more, I'm not so sure about the connection
> > > with removing pcibios_add_device().  This host_bridge->add_dev() hook
> > > would be for host bridge-specific things, while pcibios_add_device()
> > > is for arch-specific things.
> > >
> > > I'd still love to get rid of pcibios_add_device() (especially the
> > > non-arch-specific things like the pci_claim_resource() in s390); I'm
> > > just not sure yet whether this particular patch is the vehicle.
> >
> > I think most of the arch-specific pcibios_* calls are actually
> > host bridge specific after all, it just so happens that they are
> > implemented on architectures that only have one specific
> > host bridge implementation, or that they are used on an
> > architecture that does something odd in one place and needs
> > to do something else in another place.
> >
> > For pci_claim_resource() we seem to be doing this in a number
> > of different places, but there isn't strictly a reason for that.
>
> pci_claim_resource() is needed if either arch code or the host
> controller driver does not trigger a resources assignment (which claims
> them while at it); in theory that's arch agnostic but it turned out to
> be very arch/platform specific - aka if we move s390 code to core code
> we will notice :) so pci_claim_resource() in a pcibios call is
> unfortunately legitimate - whether it can be moved out of it to
> generic code that's a very complicated problem.

The generic pci_host_probe() already does either pci_bus_claim_resources() or
pci_bus_size_bridges()/pci_bus_assign_resources()/pcie_bus_configure_settings()

I would hope that we can move a lot of implementations over to just
call this function, with the appropriate PCI_PROBE_ONLY setting.

       Arnd

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Lorenzo Pieralisi]

On Thu, Aug 16, 2018 at 11:04:53PM +0200, Arnd Bergmann wrote:
> On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> 
> > > This patch seems OK to me.
> > >
> > > I don't really care about the prototype.  There's only one
> > > pcibios_add_device() implementation (x86) that returns anything other
> > > than 0, and that's a pretty obscure error case related to f9a37be0f02a
> > > ("x86: Use PCI setup data"), which lets us use ROM data from boot
> > > services.  Even then the only thing that happens is a WARN_ON().  A
> > > more descriptive printk would be a lot more useful.
> >
> > Thinking about this some more, I'm not so sure about the connection
> > with removing pcibios_add_device().  This host_bridge->add_dev() hook
> > would be for host bridge-specific things, while pcibios_add_device()
> > is for arch-specific things.
> >
> > I'd still love to get rid of pcibios_add_device() (especially the
> > non-arch-specific things like the pci_claim_resource() in s390); I'm
> > just not sure yet whether this particular patch is the vehicle.
> 
> I think most of the arch-specific pcibios_* calls are actually
> host bridge specific after all, it just so happens that they are
> implemented on architectures that only have one specific
> host bridge implementation, or that they are used on an
> architecture that does something odd in one place and needs
> to do something else in another place.
> 
> For pci_claim_resource() we seem to be doing this in a number
> of different places, but there isn't strictly a reason for that.

pci_claim_resource() is needed if either arch code or the host
controller driver does not trigger a resources assignment (which claims
them while at it); in theory that's arch agnostic but it turned out to
be very arch/platform specific - aka if we move s390 code to core code
we will notice :) so pci_claim_resource() in a pcibios call is
unfortunately legitimate - whether it can be moved out of it to
generic code that's a very complicated problem.

Lorenzo

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Lorenzo Pieralisi]

On Thu, Aug 16, 2018 at 11:04:53PM +0200, Arnd Bergmann wrote:
> On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> > > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> 
> > > This patch seems OK to me.
> > >
> > > I don't really care about the prototype.  There's only one
> > > pcibios_add_device() implementation (x86) that returns anything other
> > > than 0, and that's a pretty obscure error case related to f9a37be0f02a
> > > ("x86: Use PCI setup data"), which lets us use ROM data from boot
> > > services.  Even then the only thing that happens is a WARN_ON().  A
> > > more descriptive printk would be a lot more useful.
> >
> > Thinking about this some more, I'm not so sure about the connection
> > with removing pcibios_add_device().  This host_bridge->add_dev() hook
> > would be for host bridge-specific things, while pcibios_add_device()
> > is for arch-specific things.
> >
> > I'd still love to get rid of pcibios_add_device() (especially the
> > non-arch-specific things like the pci_claim_resource() in s390); I'm
> > just not sure yet whether this particular patch is the vehicle.
> 
> I think most of the arch-specific pcibios_* calls are actually
> host bridge specific after all, it just so happens that they are
> implemented on architectures that only have one specific
> host bridge implementation, or that they are used on an
> architecture that does something odd in one place and needs
> to do something else in another place.
> 
> For pci_claim_resource() we seem to be doing this in a number
> of different places, but there isn't strictly a reason for that.

pci_claim_resource() is needed if either arch code or the host
controller driver does not trigger a resources assignment (which claims
them while at it); in theory that's arch agnostic but it turned out to
be very arch/platform specific - aka if we move s390 code to core code
we will notice :) so pci_claim_resource() in a pcibios call is
unfortunately legitimate - whether it can be moved out of it to
generic code that's a very complicated problem.

Lorenzo

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:

> > This patch seems OK to me.
> >
> > I don't really care about the prototype.  There's only one
> > pcibios_add_device() implementation (x86) that returns anything other
> > than 0, and that's a pretty obscure error case related to f9a37be0f02a
> > ("x86: Use PCI setup data"), which lets us use ROM data from boot
> > services.  Even then the only thing that happens is a WARN_ON().  A
> > more descriptive printk would be a lot more useful.
>
> Thinking about this some more, I'm not so sure about the connection
> with removing pcibios_add_device().  This host_bridge->add_dev() hook
> would be for host bridge-specific things, while pcibios_add_device()
> is for arch-specific things.
>
> I'd still love to get rid of pcibios_add_device() (especially the
> non-arch-specific things like the pci_claim_resource() in s390); I'm
> just not sure yet whether this particular patch is the vehicle.

I think most of the arch-specific pcibios_* calls are actually
host bridge specific after all, it just so happens that they are
implemented on architectures that only have one specific
host bridge implementation, or that they are used on an
architecture that does something odd in one place and needs
to do something else in another place.

For pci_claim_resource() we seem to be doing this in a number
of different places, but there isn't strictly a reason for that.

I've started a patch series to turn all the __weak pcibios_*
functions into callbacks, which seems to be the right solution
for almost all of them.

      Arnd

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Arnd Bergmann]

On Thu, Aug 16, 2018 at 10:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:

> > This patch seems OK to me.
> >
> > I don't really care about the prototype.  There's only one
> > pcibios_add_device() implementation (x86) that returns anything other
> > than 0, and that's a pretty obscure error case related to f9a37be0f02a
> > ("x86: Use PCI setup data"), which lets us use ROM data from boot
> > services.  Even then the only thing that happens is a WARN_ON().  A
> > more descriptive printk would be a lot more useful.
>
> Thinking about this some more, I'm not so sure about the connection
> with removing pcibios_add_device().  This host_bridge->add_dev() hook
> would be for host bridge-specific things, while pcibios_add_device()
> is for arch-specific things.
>
> I'd still love to get rid of pcibios_add_device() (especially the
> non-arch-specific things like the pci_claim_resource() in s390); I'm
> just not sure yet whether this particular patch is the vehicle.

I think most of the arch-specific pcibios_* calls are actually
host bridge specific after all, it just so happens that they are
implemented on architectures that only have one specific
host bridge implementation, or that they are used on an
architecture that does something odd in one place and needs
to do something else in another place.

For pci_claim_resource() we seem to be doing this in a number
of different places, but there isn't strictly a reason for that.

I've started a patch series to turn all the __weak pcibios_*
functions into callbacks, which seems to be the right solution
for almost all of them.

      Arnd

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Bjorn Helgaas]

On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Aug 01, 2018 at 05:14:01PM +0200, Christoph Hellwig wrote:
> > > 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_dev" 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 <wesley@sifive.com>.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  drivers/pci/probe.c | 4 ++++
> > >  include/linux/pci.h | 1 +
> > >  2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index ac876e32de4b..8736d78ffc66 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -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);
> > > @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > >  	/* Set up MSI IRQ domain */
> > >  	pci_set_msi_domain(dev);
> > >  
> > > +	if (host->add_dev)
> > > +		host->add_dev(dev);
> > 
> > Hi Christoph,
> > 
> > while at it, IMHO it would be good to match the current
> > pcibios_add_device() prototype (ie returning an int error value)
> > so that this change can pave the way to removing yet another
> > pcibios call.
> > 
> > Actually we could make pcibios_add_device() removal as part of this
> > series but you would have to patch all related host bridge drivers
> > initializations so I am happy for this patch to go standalone (I can
> > patch the rest of the code) - I leave it to Bjorn's decision.
> 
> This patch seems OK to me.
> 
> I don't really care about the prototype.  There's only one
> pcibios_add_device() implementation (x86) that returns anything other
> than 0, and that's a pretty obscure error case related to f9a37be0f02a
> ("x86: Use PCI setup data"), which lets us use ROM data from boot
> services.  Even then the only thing that happens is a WARN_ON().  A
> more descriptive printk would be a lot more useful.

Thinking about this some more, I'm not so sure about the connection
with removing pcibios_add_device().  This host_bridge->add_dev() hook
would be for host bridge-specific things, while pcibios_add_device()
is for arch-specific things.

I'd still love to get rid of pcibios_add_device() (especially the
non-arch-specific things like the pci_claim_resource() in s390); I'm
just not sure yet whether this particular patch is the vehicle.

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Bjorn Helgaas]

On Wed, Aug 15, 2018 at 02:52:28PM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Aug 01, 2018 at 05:14:01PM +0200, Christoph Hellwig wrote:
> > > 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_dev" 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 <wesley@sifive.com>.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  drivers/pci/probe.c | 4 ++++
> > >  include/linux/pci.h | 1 +
> > >  2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index ac876e32de4b..8736d78ffc66 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -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);
> > > @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> > >  	/* Set up MSI IRQ domain */
> > >  	pci_set_msi_domain(dev);
> > >  
> > > +	if (host->add_dev)
> > > +		host->add_dev(dev);
> > 
> > Hi Christoph,
> > 
> > while at it, IMHO it would be good to match the current
> > pcibios_add_device() prototype (ie returning an int error value)
> > so that this change can pave the way to removing yet another
> > pcibios call.
> > 
> > Actually we could make pcibios_add_device() removal as part of this
> > series but you would have to patch all related host bridge drivers
> > initializations so I am happy for this patch to go standalone (I can
> > patch the rest of the code) - I leave it to Bjorn's decision.
> 
> This patch seems OK to me.
> 
> I don't really care about the prototype.  There's only one
> pcibios_add_device() implementation (x86) that returns anything other
> than 0, and that's a pretty obscure error case related to f9a37be0f02a
> ("x86: Use PCI setup data"), which lets us use ROM data from boot
> services.  Even then the only thing that happens is a WARN_ON().  A
> more descriptive printk would be a lot more useful.

Thinking about this some more, I'm not so sure about the connection
with removing pcibios_add_device().  This host_bridge->add_dev() hook
would be for host bridge-specific things, while pcibios_add_device()
is for arch-specific things.

I'd still love to get rid of pcibios_add_device() (especially the
non-arch-specific things like the pci_claim_resource() in s390); I'm
just not sure yet whether this particular patch is the vehicle.

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Bjorn Helgaas]

On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Aug 01, 2018 at 05:14:01PM +0200, Christoph Hellwig wrote:
> > 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_dev" 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 <wesley@sifive.com>.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  drivers/pci/probe.c | 4 ++++
> >  include/linux/pci.h | 1 +
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index ac876e32de4b..8736d78ffc66 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -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);
> > @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> >  	/* Set up MSI IRQ domain */
> >  	pci_set_msi_domain(dev);
> >  
> > +	if (host->add_dev)
> > +		host->add_dev(dev);
> 
> Hi Christoph,
> 
> while at it, IMHO it would be good to match the current
> pcibios_add_device() prototype (ie returning an int error value)
> so that this change can pave the way to removing yet another
> pcibios call.
> 
> Actually we could make pcibios_add_device() removal as part of this
> series but you would have to patch all related host bridge drivers
> initializations so I am happy for this patch to go standalone (I can
> patch the rest of the code) - I leave it to Bjorn's decision.

This patch seems OK to me.

I don't really care about the prototype.  There's only one
pcibios_add_device() implementation (x86) that returns anything other
than 0, and that's a pretty obscure error case related to f9a37be0f02a
("x86: Use PCI setup data"), which lets us use ROM data from boot
services.  Even then the only thing that happens is a WARN_ON().  A
more descriptive printk would be a lot more useful.

I assume you'll probably merge this along with the rest of the series,
Lorenzo, so:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> > +
> >  	/* Notifier could use PCI capabilities */
> >  	dev->match_driver = false;
> >  	ret = device_add(&dev->dev);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index abd5d5e17aee..5eedae8e8f2b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -485,6 +485,7 @@ struct pci_host_bridge {
> >  			resource_size_t start,
> >  			resource_size_t size,
> >  			resource_size_t align);
> > +	void (*add_dev)(struct pci_dev *dev);
> >  	unsigned long	private[0] ____cacheline_aligned;
> >  };
> >  
> > -- 
> > 2.18.0
> > 

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Bjorn Helgaas]

On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Aug 01, 2018 at 05:14:01PM +0200, Christoph Hellwig wrote:
> > 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_dev" 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 <wesley@sifive.com>.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  drivers/pci/probe.c | 4 ++++
> >  include/linux/pci.h | 1 +
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index ac876e32de4b..8736d78ffc66 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -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);
> > @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> >  	/* Set up MSI IRQ domain */
> >  	pci_set_msi_domain(dev);
> >  
> > +	if (host->add_dev)
> > +		host->add_dev(dev);
> 
> Hi Christoph,
> 
> while at it, IMHO it would be good to match the current
> pcibios_add_device() prototype (ie returning an int error value)
> so that this change can pave the way to removing yet another
> pcibios call.
> 
> Actually we could make pcibios_add_device() removal as part of this
> series but you would have to patch all related host bridge drivers
> initializations so I am happy for this patch to go standalone (I can
> patch the rest of the code) - I leave it to Bjorn's decision.

This patch seems OK to me.

I don't really care about the prototype.  There's only one
pcibios_add_device() implementation (x86) that returns anything other
than 0, and that's a pretty obscure error case related to f9a37be0f02a
("x86: Use PCI setup data"), which lets us use ROM data from boot
services.  Even then the only thing that happens is a WARN_ON().  A
more descriptive printk would be a lot more useful.

I assume you'll probably merge this along with the rest of the series,
Lorenzo, so:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> > +
> >  	/* Notifier could use PCI capabilities */
> >  	dev->match_driver = false;
> >  	ret = device_add(&dev->dev);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index abd5d5e17aee..5eedae8e8f2b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -485,6 +485,7 @@ struct pci_host_bridge {
> >  			resource_size_t start,
> >  			resource_size_t size,
> >  			resource_size_t align);
> > +	void (*add_dev)(struct pci_dev *dev);
> >  	unsigned long	private[0] ____cacheline_aligned;
> >  };
> >  
> > -- 
> > 2.18.0
> > 

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> while at it, IMHO it would be good to match the current
> pcibios_add_device() prototype (ie returning an int error value)
> so that this change can pave the way to removing yet another
> pcibios call.

Replacing pcibios_add_device sounds like a good idea.  But then
again we can't actually handle the error of that one either
right now..

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 05:54:30PM +0100, Lorenzo Pieralisi wrote:
> while at it, IMHO it would be good to match the current
> pcibios_add_device() prototype (ie returning an int error value)
> so that this change can pave the way to removing yet another
> pcibios call.

Replacing pcibios_add_device sounds like a good idea.  But then
again we can't actually handle the error of that one either
right now..

Re: [PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Lorenzo Pieralisi]

On Wed, Aug 01, 2018 at 05:14:01PM +0200, Christoph Hellwig wrote:
> 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_dev" 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 <wesley@sifive.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/probe.c | 4 ++++
>  include/linux/pci.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e32de4b..8736d78ffc66 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);
> @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  	/* Set up MSI IRQ domain */
>  	pci_set_msi_domain(dev);
>  
> +	if (host->add_dev)
> +		host->add_dev(dev);

Hi Christoph,

while at it, IMHO it would be good to match the current
pcibios_add_device() prototype (ie returning an int error value)
so that this change can pave the way to removing yet another
pcibios call.

Actually we could make pcibios_add_device() removal as part of this
series but you would have to patch all related host bridge drivers
initializations so I am happy for this patch to go standalone (I can
patch the rest of the code) - I leave it to Bjorn's decision.

Thanks,
Lorenzo

> +
>  	/* Notifier could use PCI capabilities */
>  	dev->match_driver = false;
>  	ret = device_add(&dev->dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index abd5d5e17aee..5eedae8e8f2b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -485,6 +485,7 @@ struct pci_host_bridge {
>  			resource_size_t start,
>  			resource_size_t size,
>  			resource_size_t align);
> +	void (*add_dev)(struct pci_dev *dev);
>  	unsigned long	private[0] ____cacheline_aligned;
>  };
>  
> -- 
> 2.18.0
> 

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[Lorenzo Pieralisi]

On Wed, Aug 01, 2018 at 05:14:01PM +0200, Christoph Hellwig wrote:
> 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_dev" 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 <wesley@sifive.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/probe.c | 4 ++++
>  include/linux/pci.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e32de4b..8736d78ffc66 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -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);
> @@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>  	/* Set up MSI IRQ domain */
>  	pci_set_msi_domain(dev);
>  
> +	if (host->add_dev)
> +		host->add_dev(dev);

Hi Christoph,

while at it, IMHO it would be good to match the current
pcibios_add_device() prototype (ie returning an int error value)
so that this change can pave the way to removing yet another
pcibios call.

Actually we could make pcibios_add_device() removal as part of this
series but you would have to patch all related host bridge drivers
initializations so I am happy for this patch to go standalone (I can
patch the rest of the code) - I leave it to Bjorn's decision.

Thanks,
Lorenzo

> +
>  	/* Notifier could use PCI capabilities */
>  	dev->match_driver = false;
>  	ret = device_add(&dev->dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index abd5d5e17aee..5eedae8e8f2b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -485,6 +485,7 @@ struct pci_host_bridge {
>  			resource_size_t start,
>  			resource_size_t size,
>  			resource_size_t align);
> +	void (*add_dev)(struct pci_dev *dev);
>  	unsigned long	private[0] ____cacheline_aligned;
>  };
>  
> -- 
> 2.18.0
> 

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[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_dev" 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 <wesley@sifive.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/probe.c | 4 ++++
 include/linux/pci.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac876e32de4b..8736d78ffc66 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
@@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	/* Set up MSI IRQ domain */
 	pci_set_msi_domain(dev);
 
+	if (host->add_dev)
+		host->add_dev(dev);
+
 	/* Notifier could use PCI capabilities */
 	dev->match_driver = false;
 	ret = device_add(&dev->dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index abd5d5e17aee..5eedae8e8f2b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -485,6 +485,7 @@ struct pci_host_bridge {
 			resource_size_t start,
 			resource_size_t size,
 			resource_size_t align);
+	void (*add_dev)(struct pci_dev *dev);
 	unsigned long	private[0] ____cacheline_aligned;
 };
 
-- 
2.18.0

[PATCH 1/3] PCI: add a callback to struct pci_host_bridge for adding a new device

[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_dev" 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 <wesley@sifive.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/probe.c | 4 ++++
 include/linux/pci.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac876e32de4b..8736d78ffc66 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -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);
@@ -2331,6 +2332,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	/* Set up MSI IRQ domain */
 	pci_set_msi_domain(dev);
 
+	if (host->add_dev)
+		host->add_dev(dev);
+
 	/* Notifier could use PCI capabilities */
 	dev->match_driver = false;
 	ret = device_add(&dev->dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index abd5d5e17aee..5eedae8e8f2b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -485,6 +485,7 @@ struct pci_host_bridge {
 			resource_size_t start,
 			resource_size_t size,
 			resource_size_t align);
+	void (*add_dev)(struct pci_dev *dev);
 	unsigned long	private[0] ____cacheline_aligned;
 };
 
-- 
2.18.0