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

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

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

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

[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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index 7b1389d8e2a5..da65b18aa45e 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -197,6 +197,15 @@ 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 void xilinx_pcie_add_dev(struct pci_dev *pdev)
+{
+	pdev->dev.bus_dma_mask = DMA_BIT_MASK(32);
+}
+
 /* PCIe operations */
 static struct pci_ops xilinx_pcie_ops = {
 	.map_bus = xilinx_pcie_map_bus,
@@ -665,6 +674,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_dev = xilinx_pcie_add_dev;
 
 #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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index 7b1389d8e2a5..da65b18aa45e 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -197,6 +197,15 @@ 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 void xilinx_pcie_add_dev(struct pci_dev *pdev)
+{
+	pdev->dev.bus_dma_mask = DMA_BIT_MASK(32);
+}
+
 /* PCIe operations */
 static struct pci_ops xilinx_pcie_ops = {
 	.map_bus = xilinx_pcie_map_bus,
@@ -665,6 +674,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_dev = xilinx_pcie_add_dev;
 
 #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: add support for Xilinx PCIe root ports on RISC-V v2

[Wesley Terpstra]

FYI, I don't believe this solution is the one we intended to merge
upstream. There is a different approach used in the riscv-linux tree
from Palmer.

On Wed, Aug 1, 2018 at 8:14 AM, Christoph Hellwig <hch@lst.de> wrote:
> 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 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 v2

[Wesley Terpstra]

FYI, I don't believe this solution is the one we intended to merge
upstream. There is a different approach used in the riscv-linux tree
from Palmer.

On Wed, Aug 1, 2018 at 8:14 AM, Christoph Hellwig <hch@lst.de> wrote:
> 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 v1:
>  - move the add_dev method to struct pci_host_bridge
>  - use the new bus_dma_mask field

Re: add support for Xilinx PCIe root ports on RISC-V v2

[Christoph Hellwig]

On Wed, Aug 01, 2018 at 04:02:34PM -0700, Wesley Terpstra wrote:
> FYI, I don't believe this solution is the one we intended to merge
> upstream. There is a different approach used in the riscv-linux tree
> from Palmer.

This is the solution I as the dma-mapping maintainer want for upstream,
and I'm trying to convince the PCI maintainers to agree with me.

What is in the RISC-V trees is a horribly bad idea that should have
never seen the light of day (copying dma_map_ops and patching them up).

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

[Christoph Hellwig]

On Wed, Aug 01, 2018 at 04:02:34PM -0700, Wesley Terpstra wrote:
> FYI, I don't believe this solution is the one we intended to merge
> upstream. There is a different approach used in the riscv-linux tree
> from Palmer.

This is the solution I as the dma-mapping maintainer want for upstream,
and I'm trying to convince the PCI maintainers to agree with me.

What is in the RISC-V trees is a horribly bad idea that should have
never seen the light of day (copying dma_map_ops and patching them up).

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
> 

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

[Lorenzo Pieralisi]

On Wed, Aug 01, 2018 at 05:14:03PM +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 could have taken it for this merge window but unfortunately it is a
bit late (ie PCI patches queue is close), I can certainly queue it up
for v4.20 though.

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 Wed, Aug 01, 2018 at 05:14:03PM +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 could have taken it for this merge window but unfortunately it is a
bit late (ie PCI patches queue is close), I can certainly queue it up
for v4.20 though.

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

[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 3/3] PCI/xilinx: Depend on OF instead of the ARCH

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 05:56:23PM +0100, Lorenzo Pieralisi wrote:
> I could have taken it for this merge window but unfortunately it is a
> bit late (ie PCI patches queue is close), I can certainly queue it up
> for v4.20 though.

It is a little sad that we can't take a trivial kconfig patch more than a
week before the end of the merge window.  But then against given how long
a version of this patch has been around and not submitted who am I to
complain..

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

[Christoph Hellwig]

On Thu, Aug 02, 2018 at 05:56:23PM +0100, Lorenzo Pieralisi wrote:
> I could have taken it for this merge window but unfortunately it is a
> bit late (ie PCI patches queue is close), I can certainly queue it up
> for v4.20 though.

It is a little sad that we can't take a trivial kconfig patch more than a
week before the end of the merge window.  But then against given how long
a version of this patch has been around and not submitted who am I to
complain..

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

[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

[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

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