[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Jayachandran C]

The current code in pci-host-generic.c uses pci_common_init_dev()
from the arch/arm/ to do a part of the PCI initialization, and this
prevents it from being used on arm64.

The initialization done by pci_common_init_dev() that is really
needed by pci-host-generic.c can be done in the same file without
using the hw_pci API of ARM.

The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
this is be handled by setting up 'struct gen_pci' to embed a
pci_sys_data variable as the first element on the ARM platform.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---

Notes:
 - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't
   tested it on ARM.
 - tested it on ARM64 fast model
 - Any information on how this can be tested on arm is welcome.
 - There is only one ifdef, and that can be removed when arm64 gets
   a sysdata, or when arm loses its sysdata.

 drivers/pci/host/pci-host-generic.c | 49 ++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index ba46e58..1631d34 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -39,6 +39,9 @@ struct gen_pci_cfg_windows {
 };
 
 struct gen_pci {
+#ifdef CONFIG_ARM
+	struct pci_sys_data			sys;
+#endif
 	struct pci_host_bridge			host;
 	struct gen_pci_cfg_windows		cfg;
 	struct list_head			resources;
@@ -48,8 +51,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
 					     unsigned int devfn,
 					     int where)
 {
-	struct pci_sys_data *sys = bus->sysdata;
-	struct gen_pci *pci = sys->private_data;
+	struct gen_pci *pci = bus->sysdata;
 	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
 
 	return pci->cfg.win[idx] + ((devfn << 8) | where);
@@ -64,8 +66,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
 					      unsigned int devfn,
 					      int where)
 {
-	struct pci_sys_data *sys = bus->sysdata;
-	struct gen_pci *pci = sys->private_data;
+	struct gen_pci *pci = bus->sysdata;
 	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
 
 	return pci->cfg.win[idx] + ((devfn << 12) | where);
@@ -198,11 +199,33 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
 	return 0;
 }
 
-static int gen_pci_setup(int nr, struct pci_sys_data *sys)
+static int gen_pci_init(struct device *dev, struct gen_pci *pci)
 {
-	struct gen_pci *pci = sys->private_data;
-	list_splice_init(&pci->resources, &sys->resources);
-	return 1;
+	struct pci_bus *bus;
+
+	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
+
+	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
+	if (!bus) {
+		dev_err(dev, "Scanning rootbus failed");
+		return -ENODEV;
+	}
+	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+
+	if (!pci_has_flag(PCI_PROBE_ONLY)) {
+		pci_bus_size_bridges(bus);
+		pci_bus_assign_resources(bus);
+	}
+	pci_bus_add_devices(bus);
+
+	/* Configure PCI Express settings */
+	if (pci_has_flag(PCI_PROBE_ONLY)) {
+		struct pci_bus *child;
+
+		list_for_each_entry(child, &bus->children, node)
+			pcie_bus_configure_settings(child);
+	}
+	return 0;
 }
 
 static int gen_pci_probe(struct platform_device *pdev)
@@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
-	struct hw_pci hw = {
-		.nr_controllers	= 1,
-		.private_data	= (void **)&pci,
-		.setup		= gen_pci_setup,
-		.map_irq	= of_irq_parse_and_map_pci,
-		.ops		= &gen_pci_ops,
-	};
 
 	if (!pci)
 		return -ENOMEM;
@@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	pci_common_init_dev(dev, &hw);
-	return 0;
+	return gen_pci_init(dev, pci);
 }
 
 static struct platform_driver gen_pci_driver = {
-- 
1.9.1

[RFC PATCH 2/2] PCI: generic: add arm64 support

[Jayachandran C]

Make pci-host-generic driver available on arm64. This needs
drivers/pci/setup-irq.o to be built for the arm64 as well.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
---
 drivers/pci/Makefile     | 1 +
 drivers/pci/host/Kconfig | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 73e4af4..be3f631 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
 #
 obj-$(CONFIG_ALPHA) += setup-irq.o
 obj-$(CONFIG_ARM) += setup-irq.o
+obj-$(CONFIG_ARM64) += setup-irq.o
 obj-$(CONFIG_UNICORE32) += setup-irq.o
 obj-$(CONFIG_SUPERH) += setup-irq.o
 obj-$(CONFIG_MIPS) += setup-irq.o
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 1dfb567..51741ad 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -53,7 +53,7 @@ config PCI_RCAR_GEN2_PCIE
 
 config PCI_HOST_GENERIC
 	bool "Generic PCI host controller"
-	depends on ARM && OF
+	depends on (ARM || ARM64) && OF
 	help
 	  Say Y here if you want to support a simple generic PCI host
 	  controller, such as the one emulated by kvmtool.
-- 
1.9.1

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Will Deacon]

Hi Jayachandran,

On Wed, Apr 29, 2015 at 12:39:58PM +0100, Jayachandran C wrote:
> The current code in pci-host-generic.c uses pci_common_init_dev()
> from the arch/arm/ to do a part of the PCI initialization, and this
> prevents it from being used on arm64.
> 
> The initialization done by pci_common_init_dev() that is really
> needed by pci-host-generic.c can be done in the same file without
> using the hw_pci API of ARM.
> 
> The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> this is be handled by setting up 'struct gen_pci' to embed a
> pci_sys_data variable as the first element on the ARM platform.
> 
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> ---
> 
> Notes:
>  - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't
>    tested it on ARM.
>  - tested it on ARM64 fast model
>  - Any information on how this can be tested on arm is welcome.

You should be able to test using a 32-bit guest under KVM.

>  - There is only one ifdef, and that can be removed when arm64 gets
>    a sysdata, or when arm loses its sysdata.

Anyway, I believe Lorenzo (CC'd) has already been working on this so I'll
let him comment here.

Will

> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index ba46e58..1631d34 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -39,6 +39,9 @@ struct gen_pci_cfg_windows {
>  };
>  
>  struct gen_pci {
> +#ifdef CONFIG_ARM
> +	struct pci_sys_data			sys;
> +#endif
>  	struct pci_host_bridge			host;
>  	struct gen_pci_cfg_windows		cfg;
>  	struct list_head			resources;
> @@ -48,8 +51,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus,
>  					     unsigned int devfn,
>  					     int where)
>  {
> -	struct pci_sys_data *sys = bus->sysdata;
> -	struct gen_pci *pci = sys->private_data;
> +	struct gen_pci *pci = bus->sysdata;
>  	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
>  
>  	return pci->cfg.win[idx] + ((devfn << 8) | where);
> @@ -64,8 +66,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus,
>  					      unsigned int devfn,
>  					      int where)
>  {
> -	struct pci_sys_data *sys = bus->sysdata;
> -	struct gen_pci *pci = sys->private_data;
> +	struct gen_pci *pci = bus->sysdata;
>  	resource_size_t idx = bus->number - pci->cfg.bus_range->start;
>  
>  	return pci->cfg.win[idx] + ((devfn << 12) | where);
> @@ -198,11 +199,33 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>  	return 0;
>  }
>  
> -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> +static int gen_pci_init(struct device *dev, struct gen_pci *pci)
>  {
> -	struct gen_pci *pci = sys->private_data;
> -	list_splice_init(&pci->resources, &sys->resources);
> -	return 1;
> +	struct pci_bus *bus;
> +
> +	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
> +
> +	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
> +	if (!bus) {
> +		dev_err(dev, "Scanning rootbus failed");
> +		return -ENODEV;
> +	}
> +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> +
> +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> +		pci_bus_size_bridges(bus);
> +		pci_bus_assign_resources(bus);
> +	}
> +	pci_bus_add_devices(bus);
> +
> +	/* Configure PCI Express settings */
> +	if (pci_has_flag(PCI_PROBE_ONLY)) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +	return 0;
>  }
>  
>  static int gen_pci_probe(struct platform_device *pdev)
> @@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> -	struct hw_pci hw = {
> -		.nr_controllers	= 1,
> -		.private_data	= (void **)&pci,
> -		.setup		= gen_pci_setup,
> -		.map_irq	= of_irq_parse_and_map_pci,
> -		.ops		= &gen_pci_ops,
> -	};
>  
>  	if (!pci)
>  		return -ENOMEM;
> @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	pci_common_init_dev(dev, &hw);
> -	return 0;
> +	return gen_pci_init(dev, pci);
>  }
>  
>  static struct platform_driver gen_pci_driver = {
> -- 
> 1.9.1
> 

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Arnd Bergmann]

On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> The current code in pci-host-generic.c uses pci_common_init_dev()
> from the arch/arm/ to do a part of the PCI initialization, and this
> prevents it from being used on arm64.
> 
> The initialization done by pci_common_init_dev() that is really
> needed by pci-host-generic.c can be done in the same file without
> using the hw_pci API of ARM.
> 
> The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> this is be handled by setting up 'struct gen_pci' to embed a
> pci_sys_data variable as the first element on the ARM platform.
> 
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>

This seems very useful

>  
> -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> +static int gen_pci_init(struct device *dev, struct gen_pci *pci)
>  {
> -	struct gen_pci *pci = sys->private_data;
> -	list_splice_init(&pci->resources, &sys->resources);
> -	return 1;
> +	struct pci_bus *bus;
> +
> +	pci_add_flags(PCI_REASSIGN_ALL_RSRC);

Do we want to set that flag unconditionally? At least for servers,
the resources should get assigned by firmware, and things might
go wrong if Linux tries to reassign them. This is probably the
case on kvmtool as well.

> +	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
> +	if (!bus) {
> +		dev_err(dev, "Scanning rootbus failed");
> +		return -ENODEV;
> +	}
> +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);

I thought this was done by default now. If it is not, can
we make the of_irq swizzling the default?

> +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> +		pci_bus_size_bridges(bus);
> +		pci_bus_assign_resources(bus);
> +	}
> +	pci_bus_add_devices(bus);
> +
> +	/* Configure PCI Express settings */
> +	if (pci_has_flag(PCI_PROBE_ONLY)) {
> +		struct pci_bus *child;
> +
> +		list_for_each_entry(child, &bus->children, node)
> +			pcie_bus_configure_settings(child);
> +	}
> +	return 0;
>  }

We should also try to come up wtih a way to make that
pcie_bus_configure_settings() automatic if it's required here.

>  static int gen_pci_probe(struct platform_device *pdev)
> @@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> -	struct hw_pci hw = {
> -		.nr_controllers	= 1,
> -		.private_data	= (void **)&pci,
> -		.setup		= gen_pci_setup,
> -		.map_irq	= of_irq_parse_and_map_pci,
> -		.ops		= &gen_pci_ops,
> -	};
>  
>  	if (!pci)
>  		return -ENOMEM;
> @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	pci_common_init_dev(dev, &hw);
> -	return 0;
> +	return gen_pci_init(dev, pci);

How about moving the new code right into the probe() function?

	Arnd

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Jayachandran C.]

Hi Arnd,

On Wed, Apr 29, 2015 at 02:34:20PM +0200, Arnd Bergmann wrote:
> On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> > The current code in pci-host-generic.c uses pci_common_init_dev()
> > from the arch/arm/ to do a part of the PCI initialization, and this
> > prevents it from being used on arm64.
> > 
> > The initialization done by pci_common_init_dev() that is really
> > needed by pci-host-generic.c can be done in the same file without
> > using the hw_pci API of ARM.
> > 
> > The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> > this is be handled by setting up 'struct gen_pci' to embed a
> > pci_sys_data variable as the first element on the ARM platform.
> > 
> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> 
> This seems very useful

I have tried to ensure that the new code is as much as possible
functionally equivalent to the call to pci_common_init_dev on ARM.

I should have added this as a note to the patch.

Ideally, I would like this patch to be functionally equivalent
to the existing code, and make further improvements with follow up
patches, but I can make the changes you suggest if it is needed.

> 
> >  
> > -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> > +static int gen_pci_init(struct device *dev, struct gen_pci *pci)
> >  {
> > -	struct gen_pci *pci = sys->private_data;
> > -	list_splice_init(&pci->resources, &sys->resources);
> > -	return 1;
> > +	struct pci_bus *bus;
> > +
> > +	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
> 
> Do we want to set that flag unconditionally? At least for servers,
> the resources should get assigned by firmware, and things might
> go wrong if Linux tries to reassign them. This is probably the
> case on kvmtool as well.
> 
> > +	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
> > +	if (!bus) {
> > +		dev_err(dev, "Scanning rootbus failed");
> > +		return -ENODEV;
> > +	}
> > +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> 
> I thought this was done by default now. If it is not, can
> we make the of_irq swizzling the default?
> 
> > +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> > +		pci_bus_size_bridges(bus);
> > +		pci_bus_assign_resources(bus);
> > +	}
> > +	pci_bus_add_devices(bus);
> > +
> > +	/* Configure PCI Express settings */
> > +	if (pci_has_flag(PCI_PROBE_ONLY)) {
> > +		struct pci_bus *child;
> > +
> > +		list_for_each_entry(child, &bus->children, node)
> > +			pcie_bus_configure_settings(child);
> > +	}
> > +	return 0;
> >  }
> 
> We should also try to come up wtih a way to make that
> pcie_bus_configure_settings() automatic if it's required here.
> 
> >  static int gen_pci_probe(struct platform_device *pdev)
> > @@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> >  	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > -	struct hw_pci hw = {
> > -		.nr_controllers	= 1,
> > -		.private_data	= (void **)&pci,
> > -		.setup		= gen_pci_setup,
> > -		.map_irq	= of_irq_parse_and_map_pci,
> > -		.ops		= &gen_pci_ops,
> > -	};
> >  
> >  	if (!pci)
> >  		return -ENOMEM;
> > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> >  		return err;
> >  	}
> >  
> > -	pci_common_init_dev(dev, &hw);
> > -	return 0;
> > +	return gen_pci_init(dev, pci);
> 
> How about moving the new code right into the probe() function?
> 
> 	Arnd

Thanks,
JC.

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Arnd Bergmann]

On Wednesday 29 April 2015 19:55:55 Jayachandran C. wrote:
> On Wed, Apr 29, 2015 at 02:34:20PM +0200, Arnd Bergmann wrote:
> > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> > > The current code in pci-host-generic.c uses pci_common_init_dev()
> > > from the arch/arm/ to do a part of the PCI initialization, and this
> > > prevents it from being used on arm64.
> > > 
> > > The initialization done by pci_common_init_dev() that is really
> > > needed by pci-host-generic.c can be done in the same file without
> > > using the hw_pci API of ARM.
> > > 
> > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> > > this is be handled by setting up 'struct gen_pci' to embed a
> > > pci_sys_data variable as the first element on the ARM platform.
> > > 
> > > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > 
> > This seems very useful
> 
> I have tried to ensure that the new code is as much as possible
> functionally equivalent to the call to pci_common_init_dev on ARM.
> 
> I should have added this as a note to the patch.
> 
> Ideally, I would like this patch to be functionally equivalent
> to the existing code, and make further improvements with follow up
> patches, but I can make the changes you suggest if it is needed.

I believe the driver is only used in virtual machines at the moment,
so it should be easy enough to test the relevant cases on arm32
and make it as simple as possible.

For bisection purposes, it could however be better to start with your
current patch and then follow it up with a second patch that removes
the unnecessary parts again immediately.

	Arnd

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Lorenzo Pieralisi]

On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote:
> On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> > The current code in pci-host-generic.c uses pci_common_init_dev()
> > from the arch/arm/ to do a part of the PCI initialization, and this
> > prevents it from being used on arm64.
> > 
> > The initialization done by pci_common_init_dev() that is really
> > needed by pci-host-generic.c can be done in the same file without
> > using the hw_pci API of ARM.
> > 
> > The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> > this is be handled by setting up 'struct gen_pci' to embed a
> > pci_sys_data variable as the first element on the ARM platform.
> > 
> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> 
> This seems very useful

Yes, it is getting less awful, waiting for pci_sys_data to disappear.

> > -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> > +static int gen_pci_init(struct device *dev, struct gen_pci *pci)
> >  {
> > -	struct gen_pci *pci = sys->private_data;
> > -	list_splice_init(&pci->resources, &sys->resources);
> > -	return 1;
> > +	struct pci_bus *bus;
> > +
> > +	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
> 
> Do we want to set that flag unconditionally? At least for servers,
> the resources should get assigned by firmware, and things might
> go wrong if Linux tries to reassign them. This is probably the
> case on kvmtool as well.

I will give it a go on kvmtool, but at first glance concern is
shared.

> > +	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
> > +	if (!bus) {
> > +		dev_err(dev, "Scanning rootbus failed");
> > +		return -ENODEV;
> > +	}
> > +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> 
> I thought this was done by default now. If it is not, can
> we make the of_irq swizzling the default?

Possibly yes.

> > +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> > +		pci_bus_size_bridges(bus);
> > +		pci_bus_assign_resources(bus);
> > +	}
> > +	pci_bus_add_devices(bus);
> > +
> > +	/* Configure PCI Express settings */
> > +	if (pci_has_flag(PCI_PROBE_ONLY)) {
> > +		struct pci_bus *child;
> > +
> > +		list_for_each_entry(child, &bus->children, node)
> > +			pcie_bus_configure_settings(child);
> > +	}
> > +	return 0;
> >  }
> 
> We should also try to come up wtih a way to make that
> pcie_bus_configure_settings() automatic if it's required here.
> 
> >  static int gen_pci_probe(struct platform_device *pdev)
> > @@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> >  	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > -	struct hw_pci hw = {
> > -		.nr_controllers	= 1,
> > -		.private_data	= (void **)&pci,
> > -		.setup		= gen_pci_setup,
> > -		.map_irq	= of_irq_parse_and_map_pci,
> > -		.ops		= &gen_pci_ops,
> > -	};
> >  
> >  	if (!pci)
> >  		return -ENOMEM;
> > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> >  		return err;
> >  	}
> >  
> > -	pci_common_init_dev(dev, &hw);
> > -	return 0;
> > +	return gen_pci_init(dev, pci);
> 
> How about moving the new code right into the probe() function?

+1.

I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we
still need a patch to prevent resources enablement there (in the
pcibios_enable_device call - actually the check for PCI_PROBE_ONLY
should be moved to the default pcibios_enable_device function in
drivers/pci/pci.c).

The other bit missing is MSI handling, that should still be implemented.

Lorenzo

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Jayachandran C.]

On Wed, Apr 29, 2015 at 06:43:57PM +0100, Lorenzo Pieralisi wrote:
> On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote:
> > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> > > The current code in pci-host-generic.c uses pci_common_init_dev()
> > > from the arch/arm/ to do a part of the PCI initialization, and this
> > > prevents it from being used on arm64.
> > > 
> > > The initialization done by pci_common_init_dev() that is really
> > > needed by pci-host-generic.c can be done in the same file without
> > > using the hw_pci API of ARM.
> > > 
> > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> > > this is be handled by setting up 'struct gen_pci' to embed a
> > > pci_sys_data variable as the first element on the ARM platform.
> > > 
> > > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > 
> > This seems very useful
> 
> Yes, it is getting less awful, waiting for pci_sys_data to disappear.
> 
> > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci)
> > >  {
> > > -	struct gen_pci *pci = sys->private_data;
> > > -	list_splice_init(&pci->resources, &sys->resources);
> > > -	return 1;
> > > +	struct pci_bus *bus;
> > > +
> > > +	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
> > 
> > Do we want to set that flag unconditionally? At least for servers,
> > the resources should get assigned by firmware, and things might
> > go wrong if Linux tries to reassign them. This is probably the
> > case on kvmtool as well.

I can skip setting this flag if PCI_PROBE_ONLY is set. Adding another
device tree parameter for this may not be needed.

> I will give it a go on kvmtool, but at first glance concern is
> shared.
> 
> > > +	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
> > > +	if (!bus) {
> > > +		dev_err(dev, "Scanning rootbus failed");
> > > +		return -ENODEV;
> > > +	}
> > > +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > 
> > I thought this was done by default now. If it is not, can
> > we make the of_irq swizzling the default?
> 
> Possibly yes.

I am not sure I understand this, I thought pci_fixup_irqs needs to be
called at this point.

> > > +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> > > +		pci_bus_size_bridges(bus);
> > > +		pci_bus_assign_resources(bus);
> > > +	}
> > > +	pci_bus_add_devices(bus);
> > > +
> > > +	/* Configure PCI Express settings */
> > > +	if (pci_has_flag(PCI_PROBE_ONLY)) {
> > > +		struct pci_bus *child;
> > > +
> > > +		list_for_each_entry(child, &bus->children, node)
> > > +			pcie_bus_configure_settings(child);
> > > +	}
> > > +	return 0;
> > >  }
> > 
> > We should also try to come up wtih a way to make that
> > pcie_bus_configure_settings() automatic if it's required here.
> > 
> > >  static int gen_pci_probe(struct platform_device *pdev)
> > > @@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
> > >  	struct device *dev = &pdev->dev;
> > >  	struct device_node *np = dev->of_node;
> > >  	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > -	struct hw_pci hw = {
> > > -		.nr_controllers	= 1,
> > > -		.private_data	= (void **)&pci,
> > > -		.setup		= gen_pci_setup,
> > > -		.map_irq	= of_irq_parse_and_map_pci,
> > > -		.ops		= &gen_pci_ops,
> > > -	};
> > >  
> > >  	if (!pci)
> > >  		return -ENOMEM;
> > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > >  		return err;
> > >  	}
> > >  
> > > -	pci_common_init_dev(dev, &hw);
> > > -	return 0;
> > > +	return gen_pci_init(dev, pci);
> > 
> > How about moving the new code right into the probe() function?
> 
> +1.

I will add this to patch v2.
 
> I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we
> still need a patch to prevent resources enablement there (in the
> pcibios_enable_device call - actually the check for PCI_PROBE_ONLY
> should be moved to the default pcibios_enable_device function in
> drivers/pci/pci.c).
> 
> The other bit missing is MSI handling, that should still be implemented.

I think we should parse msi-parent and set ->msi on the root bus in
pci-host-generic.c. The implementation of pcibios_msi_controller() for
arm/arm64 can go up the bus hierarchy and return the first msi
controller it finds. This would be trivial to add to arm/arm64.
I can post a follow up patch for this if you are interested.

If there are no additional concerns, I can post v2 of the patch after
testing on arm.

Thanks,
JC.

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Lorenzo Pieralisi]

On Thu, Apr 30, 2015 at 10:59:50AM +0100, Jayachandran C. wrote:
> On Wed, Apr 29, 2015 at 06:43:57PM +0100, Lorenzo Pieralisi wrote:
> > On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote:
> > > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> > > > The current code in pci-host-generic.c uses pci_common_init_dev()
> > > > from the arch/arm/ to do a part of the PCI initialization, and this
> > > > prevents it from being used on arm64.
> > > > 
> > > > The initialization done by pci_common_init_dev() that is really
> > > > needed by pci-host-generic.c can be done in the same file without
> > > > using the hw_pci API of ARM.
> > > > 
> > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> > > > this is be handled by setting up 'struct gen_pci' to embed a
> > > > pci_sys_data variable as the first element on the ARM platform.
> > > > 
> > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > > 
> > > This seems very useful
> > 
> > Yes, it is getting less awful, waiting for pci_sys_data to disappear.
> > 
> > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> > > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci)
> > > >  {
> > > > -	struct gen_pci *pci = sys->private_data;
> > > > -	list_splice_init(&pci->resources, &sys->resources);
> > > > -	return 1;
> > > > +	struct pci_bus *bus;
> > > > +
> > > > +	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
> > > 
> > > Do we want to set that flag unconditionally? At least for servers,
> > > the resources should get assigned by firmware, and things might
> > > go wrong if Linux tries to reassign them. This is probably the
> > > case on kvmtool as well.
> 
> I can skip setting this flag if PCI_PROBE_ONLY is set. Adding another
> device tree parameter for this may not be needed.

I think that's reasonable for the time being.

> > I will give it a go on kvmtool, but at first glance concern is
> > shared.
> > 
> > > > +	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
> > > > +	if (!bus) {
> > > > +		dev_err(dev, "Scanning rootbus failed");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > > 
> > > I thought this was done by default now. If it is not, can
> > > we make the of_irq swizzling the default?
> > 
> > Possibly yes.
> 
> I am not sure I understand this, I thought pci_fixup_irqs needs to be
> called at this point.

What we meant is that passing of_irq_parse_and_map_pci as a mapping
function is the most common option, but I do not think there is much
you can do at the moment apart from adding a pci_of_fixup_irqs function
to the generic PCI layer and calling that instead, I do not see where
the improvement is, so I think you can leave it as it is.

> > > > +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> > > > +		pci_bus_size_bridges(bus);
> > > > +		pci_bus_assign_resources(bus);
> > > > +	}
> > > > +	pci_bus_add_devices(bus);
> > > > +
> > > > +	/* Configure PCI Express settings */
> > > > +	if (pci_has_flag(PCI_PROBE_ONLY)) {
> > > > +		struct pci_bus *child;
> > > > +
> > > > +		list_for_each_entry(child, &bus->children, node)
> > > > +			pcie_bus_configure_settings(child);
> > > > +	}
> > > > +	return 0;
> > > >  }
> > > 
> > > We should also try to come up wtih a way to make that
> > > pcie_bus_configure_settings() automatic if it's required here.
> > > 
> > > >  static int gen_pci_probe(struct platform_device *pdev)
> > > > @@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > >  	struct device *dev = &pdev->dev;
> > > >  	struct device_node *np = dev->of_node;
> > > >  	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > > -	struct hw_pci hw = {
> > > > -		.nr_controllers	= 1,
> > > > -		.private_data	= (void **)&pci,
> > > > -		.setup		= gen_pci_setup,
> > > > -		.map_irq	= of_irq_parse_and_map_pci,
> > > > -		.ops		= &gen_pci_ops,
> > > > -	};
> > > >  
> > > >  	if (!pci)
> > > >  		return -ENOMEM;
> > > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > >  		return err;
> > > >  	}
> > > >  
> > > > -	pci_common_init_dev(dev, &hw);
> > > > -	return 0;
> > > > +	return gen_pci_init(dev, pci);
> > > 
> > > How about moving the new code right into the probe() function?
> > 
> > +1.
> 
> I will add this to patch v2.
>  
> > I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we
> > still need a patch to prevent resources enablement there (in the
> > pcibios_enable_device call - actually the check for PCI_PROBE_ONLY
> > should be moved to the default pcibios_enable_device function in
> > drivers/pci/pci.c).
> > 
> > The other bit missing is MSI handling, that should still be implemented.
> 
> I think we should parse msi-parent and set ->msi on the root bus in
> pci-host-generic.c. The implementation of pcibios_msi_controller() for
> arm/arm64 can go up the bus hierarchy and return the first msi
> controller it finds. This would be trivial to add to arm/arm64.
> I can post a follow up patch for this if you are interested.

I do not want to see a pcibios_msi_controller implementation on arm64,
or put it differently what you are proposing has nothing in it arm64
specific, we need to find a solution that goes into generic code, there
is already, pending further discussion:

http://lwn.net/Articles/628872/

Thanks,
Lorenzo

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Jayachandran C.]

On Fri, May 01, 2015 at 09:40:14AM +0100, Lorenzo Pieralisi wrote:
> On Thu, Apr 30, 2015 at 10:59:50AM +0100, Jayachandran C. wrote:
> > On Wed, Apr 29, 2015 at 06:43:57PM +0100, Lorenzo Pieralisi wrote:
> > > On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote:
> > > > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> > > > > The current code in pci-host-generic.c uses pci_common_init_dev()
> > > > > from the arch/arm/ to do a part of the PCI initialization, and this
> > > > > prevents it from being used on arm64.
> > > > > 
> > > > > The initialization done by pci_common_init_dev() that is really
> > > > > needed by pci-host-generic.c can be done in the same file without
> > > > > using the hw_pci API of ARM.
> > > > > 
> > > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> > > > > this is be handled by setting up 'struct gen_pci' to embed a
> > > > > pci_sys_data variable as the first element on the ARM platform.
> > > > > 
> > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > > > 
> > > > This seems very useful
> > > 
> > > Yes, it is getting less awful, waiting for pci_sys_data to disappear.
> > > 
> > > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> > > > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci)
> > > > >  {
> > > > > -	struct gen_pci *pci = sys->private_data;
> > > > > -	list_splice_init(&pci->resources, &sys->resources);
> > > > > -	return 1;
> > > > > +	struct pci_bus *bus;
> > > > > +
> > > > > +	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
> > > > 
> > > > Do we want to set that flag unconditionally? At least for servers,
> > > > the resources should get assigned by firmware, and things might
> > > > go wrong if Linux tries to reassign them. This is probably the
> > > > case on kvmtool as well.
> > 
> > I can skip setting this flag if PCI_PROBE_ONLY is set. Adding another
> > device tree parameter for this may not be needed.
> 
> I think that's reasonable for the time being.
> 
> > > I will give it a go on kvmtool, but at first glance concern is
> > > shared.
> > > 
> > > > > +	bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources);
> > > > > +	if (!bus) {
> > > > > +		dev_err(dev, "Scanning rootbus failed");
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > > > 
> > > > I thought this was done by default now. If it is not, can
> > > > we make the of_irq swizzling the default?
> > > 
> > > Possibly yes.
> > 
> > I am not sure I understand this, I thought pci_fixup_irqs needs to be
> > called at this point.
> 
> What we meant is that passing of_irq_parse_and_map_pci as a mapping
> function is the most common option, but I do not think there is much
> you can do at the moment apart from adding a pci_of_fixup_irqs function
> to the generic PCI layer and calling that instead, I do not see where
> the improvement is, so I think you can leave it as it is.
> 
> > > > > +	if (!pci_has_flag(PCI_PROBE_ONLY)) {
> > > > > +		pci_bus_size_bridges(bus);
> > > > > +		pci_bus_assign_resources(bus);
> > > > > +	}
> > > > > +	pci_bus_add_devices(bus);
> > > > > +
> > > > > +	/* Configure PCI Express settings */
> > > > > +	if (pci_has_flag(PCI_PROBE_ONLY)) {
> > > > > +		struct pci_bus *child;
> > > > > +
> > > > > +		list_for_each_entry(child, &bus->children, node)
> > > > > +			pcie_bus_configure_settings(child);
> > > > > +	}
> > > > > +	return 0;
> > > > >  }
> > > > 
> > > > We should also try to come up wtih a way to make that
> > > > pcie_bus_configure_settings() automatic if it's required here.
> > > > 
> > > > >  static int gen_pci_probe(struct platform_device *pdev)
> > > > > @@ -214,13 +237,6 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > > >  	struct device *dev = &pdev->dev;
> > > > >  	struct device_node *np = dev->of_node;
> > > > >  	struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > > > -	struct hw_pci hw = {
> > > > > -		.nr_controllers	= 1,
> > > > > -		.private_data	= (void **)&pci,
> > > > > -		.setup		= gen_pci_setup,
> > > > > -		.map_irq	= of_irq_parse_and_map_pci,
> > > > > -		.ops		= &gen_pci_ops,
> > > > > -	};
> > > > >  
> > > > >  	if (!pci)
> > > > >  		return -ENOMEM;
> > > > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > > >  		return err;
> > > > >  	}
> > > > >  
> > > > > -	pci_common_init_dev(dev, &hw);
> > > > > -	return 0;
> > > > > +	return gen_pci_init(dev, pci);
> > > > 
> > > > How about moving the new code right into the probe() function?
> > > 
> > > +1.
> > 
> > I will add this to patch v2.
> >  
> > > I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we
> > > still need a patch to prevent resources enablement there (in the
> > > pcibios_enable_device call - actually the check for PCI_PROBE_ONLY
> > > should be moved to the default pcibios_enable_device function in
> > > drivers/pci/pci.c).
> > > 
> > > The other bit missing is MSI handling, that should still be implemented.
> > 
> > I think we should parse msi-parent and set ->msi on the root bus in
> > pci-host-generic.c. The implementation of pcibios_msi_controller() for
> > arm/arm64 can go up the bus hierarchy and return the first msi
> > controller it finds. This would be trivial to add to arm/arm64.
> > I can post a follow up patch for this if you are interested.
> 
> I do not want to see a pcibios_msi_controller implementation on arm64,
> or put it differently what you are proposing has nothing in it arm64
> specific, we need to find a solution that goes into generic code, there
> is already, pending further discussion:
> 
> http://lwn.net/Articles/628872/

The default (weak) implementation of pcibios_msi_controller() could go
up the parent buses to the root bus and return the first MSI controller,
instead of retuning NULL always.

This would also eliminate the need to keep the msi_ctrl in sysdata for ARM.

Anyway, I will do v2 of this patchset and leave MSI for another patchset/RFC.

Thanks,
JC.

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Suravee Suthikulpanit]

On 4/29/15 12:43, Lorenzo Pieralisi wrote:
> On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote:
>> >On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
>>> > >The current code in pci-host-generic.c uses pci_common_init_dev()
>>> > >from the arch/arm/ to do a part of the PCI initialization, and this
>>> > >prevents it from being used on arm64.
>>> > >
>>> > >The initialization done by pci_common_init_dev() that is really
>>> > >needed by pci-host-generic.c can be done in the same file without
>>> > >using the hw_pci API of ARM.
>>> > >
>>> > >The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
>>> > >this is be handled by setting up 'struct gen_pci' to embed a
>>> > >pci_sys_data variable as the first element on the ARM platform.
>>> > >
>>> > >Signed-off-by: Jayachandran C<jchandra@broadcom.com>
>> >
>> >This seems very useful
> Yes, it is getting less awful, waiting for pci_sys_data to disappear.
>

Lorenzo,

A while back, you mentioned here (https://lkml.org/lkml/2015/2/16/364) 
that the ARM32 pcibios_align_resource() implementation requires
pci_sys_data, so we _still_ rely on pci_common_init_dev to create one
for us. Is this still the case?

I am looking at the arch/arm32/kernel/bios32.c: pcibios_init_hw() and 
see that it setup the pci_sys_data.align_resource to 
hw_pci.align_resource (see here 
http://lxr.free-electrons.com/source/arch/arm/kernel/bios32.c#L471).

However it seems that the hw_pci.align_resource is never setup in the 
pci-host-generic.c.  Am I missing something here?

Thanks,

Suravee

[RFC PATCH 1/2] PCI: generic: remove dependency on hw_pci

[Jayachandran C.]

On Sun, May 03, 2015 at 04:06:15PM -0500, Suravee Suthikulpanit wrote:
> 
> 
> On 4/29/15 12:43, Lorenzo Pieralisi wrote:
> >On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote:
> >>>On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote:
> >>>> >The current code in pci-host-generic.c uses pci_common_init_dev()
> >>>> >from the arch/arm/ to do a part of the PCI initialization, and this
> >>>> >prevents it from being used on arm64.
> >>>> >
> >>>> >The initialization done by pci_common_init_dev() that is really
> >>>> >needed by pci-host-generic.c can be done in the same file without
> >>>> >using the hw_pci API of ARM.
> >>>> >
> >>>> >The ARM platform requires a pci_sys_data as sysdata for the PCI bus,
> >>>> >this is be handled by setting up 'struct gen_pci' to embed a
> >>>> >pci_sys_data variable as the first element on the ARM platform.
> >>>> >
> >>>> >Signed-off-by: Jayachandran C<jchandra@broadcom.com>
> >>>
> >>>This seems very useful
> >Yes, it is getting less awful, waiting for pci_sys_data to disappear.
> >
> 
> Lorenzo,
> 
> A while back, you mentioned here
> (https://lkml.org/lkml/2015/2/16/364) that the ARM32
> pcibios_align_resource() implementation requires
> pci_sys_data, so we _still_ rely on pci_common_init_dev to create one
> for us. Is this still the case?
> 
> I am looking at the arch/arm32/kernel/bios32.c: pcibios_init_hw()
> and see that it setup the pci_sys_data.align_resource to
> hw_pci.align_resource (see here
> http://lxr.free-electrons.com/source/arch/arm/kernel/bios32.c#L471).
> 
> However it seems that the hw_pci.align_resource is never setup in
> the pci-host-generic.c.  Am I missing something here?

ARM32 needs a sysdata because functions like pcibios_align_resource()
will dereference and access members of this structure. However having
an allocated but zeroed sysdata is fine, since the function pointers
in sysdata (as well as things like busnr, msi_ctrl) will be 0/NULL
and pcibios_align_resource will do the default handling.

If this patch needs changes to work on your platform please let me know.

Thanks,
JC.