* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
@ 2015-07-13 10:43 Lorenzo Pieralisi
2015-07-22 9:02 ` Lorenzo Pieralisi
2015-07-22 9:30 ` Zhou Wang
0 siblings, 2 replies; 9+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-13 10:43 UTC (permalink / raw
To: linux-arm-kernel
On ARM PCI systems relying on the pcibios API to initialize PCI host
controllers, the pcibios_msi_controller weak callback is used to look-up
the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
pci_sys_data is an arch specific structure, which prevents using the
same mechanism (so same PCI host controller drivers) on ARM64 systems.
Since the struct pci_bus already contains an msi_controller pointer and
the kernel already uses it to look-up the msi controller,
this patch converts ARM host controller and relate pcibios/host bridges
initialization routines so that the msi_controller pointer look-up can be
carried out by PCI core code through the struct pci_bus msi pointer,
removing the need for arch specific pcibios_msi_controller callback
and the related pci_sys_data msi_ctrl pointer.
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Pratyush Anand <pratyush.anand@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jingoo Han <jingoohan1@gmail.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Simon Horman <horms@verge.net.au>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
---
Bjorn, all,
I am posting this patch to sound out if that's a reasonable approach.
xgene implements MSI look-up this way at present and this would represent
another step forward towards having common drivers for ARM/ARM64, comments
and testing welcome.
Thanks,
Lorenzo
arch/arm/include/asm/mach/pci.h | 3 ---
arch/arm/kernel/bios32.c | 35 +++++++++++++++++------------------
drivers/pci/host/pcie-designware.c | 9 +++++++--
drivers/pci/host/pcie-xilinx.c | 12 ++++++++++--
4 files changed, 34 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 28b9bb3..32abc0c 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -42,9 +42,6 @@ struct hw_pci {
* Per-controller structure
*/
struct pci_sys_data {
-#ifdef CONFIG_PCI_MSI
- struct msi_controller *msi_ctrl;
-#endif
struct list_head node;
int busnr; /* primary bus number */
u64 mem_offset; /* bus->cpu memory mapping offset */
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index fcbbbb1..c841b33 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -18,15 +18,6 @@
static int debug_pci;
-#ifdef CONFIG_PCI_MSI
-struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
-{
- struct pci_sys_data *sysdata = dev->bus->sysdata;
-
- return sysdata->msi_ctrl;
-}
-#endif
-
/*
* We can't use pci_get_device() here since we are
* called from interrupt context.
@@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
if (!sys)
panic("PCI: unable to allocate sys data!");
-#ifdef CONFIG_PCI_MSI
- sys->msi_ctrl = hw->msi_ctrl;
-#endif
sys->busnr = busnr;
sys->swizzle = hw->swizzle;
sys->map_irq = hw->map_irq;
@@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
break;
}
- if (hw->scan)
+ if (hw->scan) {
sys->bus = hw->scan(nr, sys);
- else
- sys->bus = pci_scan_root_bus(parent, sys->busnr,
- hw->ops, sys, &sys->resources);
+ if (!sys->bus)
+ panic("PCI: unable to scan bus!");
+ } else {
+ sys->bus = pci_create_root_bus(parent,
+ sys->busnr,
+ hw->ops, sys,
+ &sys->resources);
+ if (WARN_ON(!sys->bus)) {
+ kfree(sys);
+ break;
+ }
+#ifdef CONFIG_PCI_MSI
+ sys->bus->msi = hw->msi_ctrl;
+#endif
- if (!sys->bus)
- panic("PCI: unable to scan bus!");
+ pci_scan_child_bus(sys->bus);
+ }
busnr = sys->bus->busn_res.end + 1;
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 69486be..e584dfa 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
#ifdef CONFIG_PCI_MSI
dw_pcie_msi_chip.dev = pp->dev;
- dw_pci.msi_ctrl = &dw_pcie_msi_chip;
#endif
dw_pci.nr_controllers = 1;
@@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
struct pcie_port *pp = sys_to_pcie(sys);
pp->root_bus_nr = sys->busnr;
- bus = pci_scan_root_bus(pp->dev, sys->busnr,
+ bus = pci_create_root_bus(pp->dev, sys->busnr,
&dw_pcie_ops, sys, &sys->resources);
if (!bus)
return NULL;
+#ifdef CONFIG_PCI_MSI
+ bus->msi = &dw_pcie_msi_chip;
+#endif
+
+ pci_scan_child_bus(bus);
+
if (bus && pp->ops->scan_bus)
pp->ops->scan_bus(pp);
diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
index f1a06a0..b21eb7d 100644
--- a/drivers/pci/host/pcie-xilinx.c
+++ b/drivers/pci/host/pcie-xilinx.c
@@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
struct pci_bus *bus;
port->root_busno = sys->busnr;
- bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
+ bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
sys, &sys->resources);
+ if (!bus)
+ return NULL;
+
+#ifdef CONFIG_PCI_MSI
+ bus->msi = &xilinx_pcie_msi_chip;
+#endif
+
+ pci_scan_child_bus(bus);
+
return bus;
}
@@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
#ifdef CONFIG_PCI_MSI
xilinx_pcie_msi_chip.dev = port->dev;
- hw.msi_ctrl = &xilinx_pcie_msi_chip;
#endif
pci_common_init_dev(dev, &hw);
--
2.2.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-13 10:43 [RFT PATCH] ARM: pci: kill pcibios_msi_controller Lorenzo Pieralisi
@ 2015-07-22 9:02 ` Lorenzo Pieralisi
2015-07-22 9:24 ` Marc Zyngier
2015-07-22 9:30 ` Zhou Wang
1 sibling, 1 reply; 9+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-22 9:02 UTC (permalink / raw
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 11:43:25AM +0100, Lorenzo Pieralisi wrote:
> On ARM PCI systems relying on the pcibios API to initialize PCI host
> controllers, the pcibios_msi_controller weak callback is used to look-up
> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
>
> pci_sys_data is an arch specific structure, which prevents using the
> same mechanism (so same PCI host controller drivers) on ARM64 systems.
>
> Since the struct pci_bus already contains an msi_controller pointer and
> the kernel already uses it to look-up the msi controller,
> this patch converts ARM host controller and relate pcibios/host bridges
> initialization routines so that the msi_controller pointer look-up can be
> carried out by PCI core code through the struct pci_bus msi pointer,
> removing the need for arch specific pcibios_msi_controller callback
> and the related pci_sys_data msi_ctrl pointer.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Pratyush Anand <pratyush.anand@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
> Bjorn, all,
>
> I am posting this patch to sound out if that's a reasonable approach.
> xgene implements MSI look-up this way at present and this would represent
> another step forward towards having common drivers for ARM/ARM64, comments
> and testing welcome.
Any comments/testing on this patch ? I do not have platforms
with these host bridges handy (apart from an iMX6 Sabrelite)
so I can't test on them (change is quite mechanical though),
help on testing and feedback on the patch much appreciated.
Thanks,
Lorenzo
> arch/arm/include/asm/mach/pci.h | 3 ---
> arch/arm/kernel/bios32.c | 35 +++++++++++++++++------------------
> drivers/pci/host/pcie-designware.c | 9 +++++++--
> drivers/pci/host/pcie-xilinx.c | 12 ++++++++++--
> 4 files changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> index 28b9bb3..32abc0c 100644
> --- a/arch/arm/include/asm/mach/pci.h
> +++ b/arch/arm/include/asm/mach/pci.h
> @@ -42,9 +42,6 @@ struct hw_pci {
> * Per-controller structure
> */
> struct pci_sys_data {
> -#ifdef CONFIG_PCI_MSI
> - struct msi_controller *msi_ctrl;
> -#endif
> struct list_head node;
> int busnr; /* primary bus number */
> u64 mem_offset; /* bus->cpu memory mapping offset */
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1..c841b33 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -18,15 +18,6 @@
>
> static int debug_pci;
>
> -#ifdef CONFIG_PCI_MSI
> -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
> -{
> - struct pci_sys_data *sysdata = dev->bus->sysdata;
> -
> - return sysdata->msi_ctrl;
> -}
> -#endif
> -
> /*
> * We can't use pci_get_device() here since we are
> * called from interrupt context.
> @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> if (!sys)
> panic("PCI: unable to allocate sys data!");
>
> -#ifdef CONFIG_PCI_MSI
> - sys->msi_ctrl = hw->msi_ctrl;
> -#endif
> sys->busnr = busnr;
> sys->swizzle = hw->swizzle;
> sys->map_irq = hw->map_irq;
> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> break;
> }
>
> - if (hw->scan)
> + if (hw->scan) {
> sys->bus = hw->scan(nr, sys);
> - else
> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
> - hw->ops, sys, &sys->resources);
> + if (!sys->bus)
> + panic("PCI: unable to scan bus!");
> + } else {
> + sys->bus = pci_create_root_bus(parent,
> + sys->busnr,
> + hw->ops, sys,
> + &sys->resources);
> + if (WARN_ON(!sys->bus)) {
> + kfree(sys);
> + break;
> + }
> +#ifdef CONFIG_PCI_MSI
> + sys->bus->msi = hw->msi_ctrl;
> +#endif
>
> - if (!sys->bus)
> - panic("PCI: unable to scan bus!");
> + pci_scan_child_bus(sys->bus);
> + }
>
> busnr = sys->bus->busn_res.end + 1;
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 69486be..e584dfa 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>
> #ifdef CONFIG_PCI_MSI
> dw_pcie_msi_chip.dev = pp->dev;
> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
> #endif
>
> dw_pci.nr_controllers = 1;
> @@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> struct pcie_port *pp = sys_to_pcie(sys);
>
> pp->root_bus_nr = sys->busnr;
> - bus = pci_scan_root_bus(pp->dev, sys->busnr,
> + bus = pci_create_root_bus(pp->dev, sys->busnr,
> &dw_pcie_ops, sys, &sys->resources);
> if (!bus)
> return NULL;
>
> +#ifdef CONFIG_PCI_MSI
> + bus->msi = &dw_pcie_msi_chip;
> +#endif
> +
> + pci_scan_child_bus(bus);
> +
> if (bus && pp->ops->scan_bus)
> pp->ops->scan_bus(pp);
>
> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
> index f1a06a0..b21eb7d 100644
> --- a/drivers/pci/host/pcie-xilinx.c
> +++ b/drivers/pci/host/pcie-xilinx.c
> @@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> struct pci_bus *bus;
>
> port->root_busno = sys->busnr;
> - bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> + bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> sys, &sys->resources);
>
> + if (!bus)
> + return NULL;
> +
> +#ifdef CONFIG_PCI_MSI
> + bus->msi = &xilinx_pcie_msi_chip;
> +#endif
> +
> + pci_scan_child_bus(bus);
> +
> return bus;
> }
>
> @@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>
> #ifdef CONFIG_PCI_MSI
> xilinx_pcie_msi_chip.dev = port->dev;
> - hw.msi_ctrl = &xilinx_pcie_msi_chip;
> #endif
> pci_common_init_dev(dev, &hw);
>
> --
> 2.2.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-22 9:02 ` Lorenzo Pieralisi
@ 2015-07-22 9:24 ` Marc Zyngier
2015-07-22 12:49 ` Lorenzo Pieralisi
0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2015-07-22 9:24 UTC (permalink / raw
To: linux-arm-kernel
On 22/07/15 10:02, Lorenzo Pieralisi wrote:
> On Mon, Jul 13, 2015 at 11:43:25AM +0100, Lorenzo Pieralisi wrote:
>> On ARM PCI systems relying on the pcibios API to initialize PCI host
>> controllers, the pcibios_msi_controller weak callback is used to look-up
>> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
>>
>> pci_sys_data is an arch specific structure, which prevents using the
>> same mechanism (so same PCI host controller drivers) on ARM64 systems.
>>
>> Since the struct pci_bus already contains an msi_controller pointer and
>> the kernel already uses it to look-up the msi controller,
>> this patch converts ARM host controller and relate pcibios/host bridges
>> initialization routines so that the msi_controller pointer look-up can be
>> carried out by PCI core code through the struct pci_bus msi pointer,
>> removing the need for arch specific pcibios_msi_controller callback
>> and the related pci_sys_data msi_ctrl pointer.
>>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Pratyush Anand <pratyush.anand@gmail.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Jingoo Han <jingoohan1@gmail.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Simon Horman <horms@verge.net.au>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> Bjorn, all,
>>
>> I am posting this patch to sound out if that's a reasonable approach.
>> xgene implements MSI look-up this way at present and this would represent
>> another step forward towards having common drivers for ARM/ARM64, comments
>> and testing welcome.
>
> Any comments/testing on this patch ? I do not have platforms
> with these host bridges handy (apart from an iMX6 Sabrelite)
> so I can't test on them (change is quite mechanical though),
> help on testing and feedback on the patch much appreciated.
Hi Lorenzo,
I can't test it (no relevant HW), but that seems like a very sensible
approach to me (the less dependency on sysdata, the better).
One remark below:
>> arch/arm/include/asm/mach/pci.h | 3 ---
>> arch/arm/kernel/bios32.c | 35 +++++++++++++++++------------------
>> drivers/pci/host/pcie-designware.c | 9 +++++++--
>> drivers/pci/host/pcie-xilinx.c | 12 ++++++++++--
>> 4 files changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
>> index 28b9bb3..32abc0c 100644
>> --- a/arch/arm/include/asm/mach/pci.h
>> +++ b/arch/arm/include/asm/mach/pci.h
>> @@ -42,9 +42,6 @@ struct hw_pci {
>> * Per-controller structure
>> */
>> struct pci_sys_data {
>> -#ifdef CONFIG_PCI_MSI
>> - struct msi_controller *msi_ctrl;
>> -#endif
>> struct list_head node;
>> int busnr; /* primary bus number */
>> u64 mem_offset; /* bus->cpu memory mapping offset */
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>> index fcbbbb1..c841b33 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -18,15 +18,6 @@
>>
>> static int debug_pci;
>>
>> -#ifdef CONFIG_PCI_MSI
>> -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
>> -{
>> - struct pci_sys_data *sysdata = dev->bus->sysdata;
>> -
>> - return sysdata->msi_ctrl;
>> -}
>> -#endif
>> -
>> /*
>> * We can't use pci_get_device() here since we are
>> * called from interrupt context.
>> @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>> if (!sys)
>> panic("PCI: unable to allocate sys data!");
>>
>> -#ifdef CONFIG_PCI_MSI
>> - sys->msi_ctrl = hw->msi_ctrl;
>> -#endif
>> sys->busnr = busnr;
>> sys->swizzle = hw->swizzle;
>> sys->map_irq = hw->map_irq;
>> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>> break;
>> }
>>
>> - if (hw->scan)
>> + if (hw->scan) {
>> sys->bus = hw->scan(nr, sys);
>> - else
>> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
>> - hw->ops, sys, &sys->resources);
>> + if (!sys->bus)
>> + panic("PCI: unable to scan bus!");
This was in the original code, but I have to ask: Do we really want to
panic the kernel if we couldn't scan the bus? Worse case, the system
won't be able to boot at all and will panic somewhere else anyway, but
we should give the user a chance to understand what's happening...
>> + } else {
>> + sys->bus = pci_create_root_bus(parent,
>> + sys->busnr,
>> + hw->ops, sys,
>> + &sys->resources);
>> + if (WARN_ON(!sys->bus)) {
>> + kfree(sys);
>> + break;
>> + }
>> +#ifdef CONFIG_PCI_MSI
>> + sys->bus->msi = hw->msi_ctrl;
>> +#endif
>>
>> - if (!sys->bus)
>> - panic("PCI: unable to scan bus!");
>> + pci_scan_child_bus(sys->bus);
>> + }
>>
>> busnr = sys->bus->busn_res.end + 1;
>>
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>> index 69486be..e584dfa 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>
>> #ifdef CONFIG_PCI_MSI
>> dw_pcie_msi_chip.dev = pp->dev;
>> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
>> #endif
>>
>> dw_pci.nr_controllers = 1;
>> @@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>> struct pcie_port *pp = sys_to_pcie(sys);
>>
>> pp->root_bus_nr = sys->busnr;
>> - bus = pci_scan_root_bus(pp->dev, sys->busnr,
>> + bus = pci_create_root_bus(pp->dev, sys->busnr,
>> &dw_pcie_ops, sys, &sys->resources);
>> if (!bus)
>> return NULL;
>>
>> +#ifdef CONFIG_PCI_MSI
>> + bus->msi = &dw_pcie_msi_chip;
>> +#endif
>> +
>> + pci_scan_child_bus(bus);
>> +
>> if (bus && pp->ops->scan_bus)
>> pp->ops->scan_bus(pp);
>>
>> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
>> index f1a06a0..b21eb7d 100644
>> --- a/drivers/pci/host/pcie-xilinx.c
>> +++ b/drivers/pci/host/pcie-xilinx.c
>> @@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>> struct pci_bus *bus;
>>
>> port->root_busno = sys->busnr;
>> - bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
>> + bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
>> sys, &sys->resources);
>>
>> + if (!bus)
>> + return NULL;
>> +
>> +#ifdef CONFIG_PCI_MSI
>> + bus->msi = &xilinx_pcie_msi_chip;
>> +#endif
>> +
>> + pci_scan_child_bus(bus);
>> +
>> return bus;
>> }
>>
>> @@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>>
>> #ifdef CONFIG_PCI_MSI
>> xilinx_pcie_msi_chip.dev = port->dev;
>> - hw.msi_ctrl = &xilinx_pcie_msi_chip;
>> #endif
>> pci_common_init_dev(dev, &hw);
>>
>> --
>> 2.2.1
>>
>
Bar the nit above:
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-13 10:43 [RFT PATCH] ARM: pci: kill pcibios_msi_controller Lorenzo Pieralisi
2015-07-22 9:02 ` Lorenzo Pieralisi
@ 2015-07-22 9:30 ` Zhou Wang
2015-07-22 10:15 ` Marc Zyngier
1 sibling, 1 reply; 9+ messages in thread
From: Zhou Wang @ 2015-07-22 9:30 UTC (permalink / raw
To: linux-arm-kernel
On 2015/7/13 18:43, Lorenzo Pieralisi wrote:
> On ARM PCI systems relying on the pcibios API to initialize PCI host
> controllers, the pcibios_msi_controller weak callback is used to look-up
> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
>
> pci_sys_data is an arch specific structure, which prevents using the
> same mechanism (so same PCI host controller drivers) on ARM64 systems.
>
> Since the struct pci_bus already contains an msi_controller pointer and
> the kernel already uses it to look-up the msi controller,
> this patch converts ARM host controller and relate pcibios/host bridges
> initialization routines so that the msi_controller pointer look-up can be
> carried out by PCI core code through the struct pci_bus msi pointer,
> removing the need for arch specific pcibios_msi_controller callback
> and the related pci_sys_data msi_ctrl pointer.
>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Pratyush Anand <pratyush.anand@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Jingoo Han <jingoohan1@gmail.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> ---
> Bjorn, all,
>
> I am posting this patch to sound out if that's a reasonable approach.
> xgene implements MSI look-up this way at present and this would represent
> another step forward towards having common drivers for ARM/ARM64, comments
> and testing welcome.
>
> Thanks,
> Lorenzo
>
> arch/arm/include/asm/mach/pci.h | 3 ---
> arch/arm/kernel/bios32.c | 35 +++++++++++++++++------------------
> drivers/pci/host/pcie-designware.c | 9 +++++++--
> drivers/pci/host/pcie-xilinx.c | 12 ++++++++++--
> 4 files changed, 34 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> index 28b9bb3..32abc0c 100644
> --- a/arch/arm/include/asm/mach/pci.h
> +++ b/arch/arm/include/asm/mach/pci.h
> @@ -42,9 +42,6 @@ struct hw_pci {
> * Per-controller structure
> */
> struct pci_sys_data {
> -#ifdef CONFIG_PCI_MSI
> - struct msi_controller *msi_ctrl;
> -#endif
> struct list_head node;
> int busnr; /* primary bus number */
> u64 mem_offset; /* bus->cpu memory mapping offset */
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index fcbbbb1..c841b33 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -18,15 +18,6 @@
>
> static int debug_pci;
>
> -#ifdef CONFIG_PCI_MSI
> -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
> -{
> - struct pci_sys_data *sysdata = dev->bus->sysdata;
> -
> - return sysdata->msi_ctrl;
> -}
> -#endif
> -
> /*
> * We can't use pci_get_device() here since we are
> * called from interrupt context.
> @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> if (!sys)
> panic("PCI: unable to allocate sys data!");
>
> -#ifdef CONFIG_PCI_MSI
> - sys->msi_ctrl = hw->msi_ctrl;
> -#endif
> sys->busnr = busnr;
> sys->swizzle = hw->swizzle;
> sys->map_irq = hw->map_irq;
> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> break;
> }
>
> - if (hw->scan)
> + if (hw->scan) {
> sys->bus = hw->scan(nr, sys);
> - else
> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
> - hw->ops, sys, &sys->resources);
> + if (!sys->bus)
> + panic("PCI: unable to scan bus!");
> + } else {
> + sys->bus = pci_create_root_bus(parent,
> + sys->busnr,
> + hw->ops, sys,
> + &sys->resources);
> + if (WARN_ON(!sys->bus)) {
> + kfree(sys);
> + break;
> + }
> +#ifdef CONFIG_PCI_MSI
> + sys->bus->msi = hw->msi_ctrl;
> +#endif
>
> - if (!sys->bus)
> - panic("PCI: unable to scan bus!");
> + pci_scan_child_bus(sys->bus);
> + }
>
> busnr = sys->bus->busn_res.end + 1;
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 69486be..e584dfa 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>
> #ifdef CONFIG_PCI_MSI
> dw_pcie_msi_chip.dev = pp->dev;
> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
> #endif
>
> dw_pci.nr_controllers = 1;
> @@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> struct pcie_port *pp = sys_to_pcie(sys);
>
> pp->root_bus_nr = sys->busnr;
> - bus = pci_scan_root_bus(pp->dev, sys->busnr,
> + bus = pci_create_root_bus(pp->dev, sys->busnr,
> &dw_pcie_ops, sys, &sys->resources);
> if (!bus)
> return NULL;
>
> +#ifdef CONFIG_PCI_MSI
> + bus->msi = &dw_pcie_msi_chip;
> +#endif
> +
> + pci_scan_child_bus(bus);
> +
> if (bus && pp->ops->scan_bus)
> pp->ops->scan_bus(pp);
>
> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
> index f1a06a0..b21eb7d 100644
> --- a/drivers/pci/host/pcie-xilinx.c
> +++ b/drivers/pci/host/pcie-xilinx.c
> @@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> struct pci_bus *bus;
>
> port->root_busno = sys->busnr;
> - bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> + bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> sys, &sys->resources);
>
> + if (!bus)
> + return NULL;
> +
> +#ifdef CONFIG_PCI_MSI
> + bus->msi = &xilinx_pcie_msi_chip;
> +#endif
> +
> + pci_scan_child_bus(bus);
> +
> return bus;
> }
>
> @@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>
> #ifdef CONFIG_PCI_MSI
> xilinx_pcie_msi_chip.dev = port->dev;
> - hw.msi_ctrl = &xilinx_pcie_msi_chip;
> #endif
> pci_common_init_dev(dev, &hw);
>
>
Hi Lorenzo,
I wonder if we can delete weak pcibios_msi_controller and the call in
pci_msi_controller in drivers/pci/msi.c
Regards,
Zhou
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-22 9:30 ` Zhou Wang
@ 2015-07-22 10:15 ` Marc Zyngier
2015-07-22 12:52 ` Lorenzo Pieralisi
0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2015-07-22 10:15 UTC (permalink / raw
To: linux-arm-kernel
On 22/07/15 10:30, Zhou Wang wrote:
> On 2015/7/13 18:43, Lorenzo Pieralisi wrote:
>> On ARM PCI systems relying on the pcibios API to initialize PCI host
>> controllers, the pcibios_msi_controller weak callback is used to look-up
>> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
>>
>> pci_sys_data is an arch specific structure, which prevents using the
>> same mechanism (so same PCI host controller drivers) on ARM64 systems.
>>
>> Since the struct pci_bus already contains an msi_controller pointer and
>> the kernel already uses it to look-up the msi controller,
>> this patch converts ARM host controller and relate pcibios/host bridges
>> initialization routines so that the msi_controller pointer look-up can be
>> carried out by PCI core code through the struct pci_bus msi pointer,
>> removing the need for arch specific pcibios_msi_controller callback
>> and the related pci_sys_data msi_ctrl pointer.
>>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Pratyush Anand <pratyush.anand@gmail.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Jingoo Han <jingoohan1@gmail.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Simon Horman <horms@verge.net.au>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: Thierry Reding <thierry.reding@gmail.com>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>> Bjorn, all,
>>
>> I am posting this patch to sound out if that's a reasonable approach.
>> xgene implements MSI look-up this way at present and this would represent
>> another step forward towards having common drivers for ARM/ARM64, comments
>> and testing welcome.
>>
>> Thanks,
>> Lorenzo
>>
>> arch/arm/include/asm/mach/pci.h | 3 ---
>> arch/arm/kernel/bios32.c | 35 +++++++++++++++++------------------
>> drivers/pci/host/pcie-designware.c | 9 +++++++--
>> drivers/pci/host/pcie-xilinx.c | 12 ++++++++++--
>> 4 files changed, 34 insertions(+), 25 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
>> index 28b9bb3..32abc0c 100644
>> --- a/arch/arm/include/asm/mach/pci.h
>> +++ b/arch/arm/include/asm/mach/pci.h
>> @@ -42,9 +42,6 @@ struct hw_pci {
>> * Per-controller structure
>> */
>> struct pci_sys_data {
>> -#ifdef CONFIG_PCI_MSI
>> - struct msi_controller *msi_ctrl;
>> -#endif
>> struct list_head node;
>> int busnr; /* primary bus number */
>> u64 mem_offset; /* bus->cpu memory mapping offset */
>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>> index fcbbbb1..c841b33 100644
>> --- a/arch/arm/kernel/bios32.c
>> +++ b/arch/arm/kernel/bios32.c
>> @@ -18,15 +18,6 @@
>>
>> static int debug_pci;
>>
>> -#ifdef CONFIG_PCI_MSI
>> -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
>> -{
>> - struct pci_sys_data *sysdata = dev->bus->sysdata;
>> -
>> - return sysdata->msi_ctrl;
>> -}
>> -#endif
>> -
>> /*
>> * We can't use pci_get_device() here since we are
>> * called from interrupt context.
>> @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>> if (!sys)
>> panic("PCI: unable to allocate sys data!");
>>
>> -#ifdef CONFIG_PCI_MSI
>> - sys->msi_ctrl = hw->msi_ctrl;
>> -#endif
>> sys->busnr = busnr;
>> sys->swizzle = hw->swizzle;
>> sys->map_irq = hw->map_irq;
>> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>> break;
>> }
>>
>> - if (hw->scan)
>> + if (hw->scan) {
>> sys->bus = hw->scan(nr, sys);
>> - else
>> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
>> - hw->ops, sys, &sys->resources);
>> + if (!sys->bus)
>> + panic("PCI: unable to scan bus!");
>> + } else {
>> + sys->bus = pci_create_root_bus(parent,
>> + sys->busnr,
>> + hw->ops, sys,
>> + &sys->resources);
>> + if (WARN_ON(!sys->bus)) {
>> + kfree(sys);
>> + break;
>> + }
>> +#ifdef CONFIG_PCI_MSI
>> + sys->bus->msi = hw->msi_ctrl;
>> +#endif
>>
>> - if (!sys->bus)
>> - panic("PCI: unable to scan bus!");
>> + pci_scan_child_bus(sys->bus);
>> + }
>>
>> busnr = sys->bus->busn_res.end + 1;
>>
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>> index 69486be..e584dfa 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>
>> #ifdef CONFIG_PCI_MSI
>> dw_pcie_msi_chip.dev = pp->dev;
>> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
>> #endif
>>
>> dw_pci.nr_controllers = 1;
>> @@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>> struct pcie_port *pp = sys_to_pcie(sys);
>>
>> pp->root_bus_nr = sys->busnr;
>> - bus = pci_scan_root_bus(pp->dev, sys->busnr,
>> + bus = pci_create_root_bus(pp->dev, sys->busnr,
>> &dw_pcie_ops, sys, &sys->resources);
>> if (!bus)
>> return NULL;
>>
>> +#ifdef CONFIG_PCI_MSI
>> + bus->msi = &dw_pcie_msi_chip;
>> +#endif
>> +
>> + pci_scan_child_bus(bus);
>> +
>> if (bus && pp->ops->scan_bus)
>> pp->ops->scan_bus(pp);
>>
>> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
>> index f1a06a0..b21eb7d 100644
>> --- a/drivers/pci/host/pcie-xilinx.c
>> +++ b/drivers/pci/host/pcie-xilinx.c
>> @@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>> struct pci_bus *bus;
>>
>> port->root_busno = sys->busnr;
>> - bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
>> + bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
>> sys, &sys->resources);
>>
>> + if (!bus)
>> + return NULL;
>> +
>> +#ifdef CONFIG_PCI_MSI
>> + bus->msi = &xilinx_pcie_msi_chip;
>> +#endif
>> +
>> + pci_scan_child_bus(bus);
>> +
>> return bus;
>> }
>>
>> @@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>>
>> #ifdef CONFIG_PCI_MSI
>> xilinx_pcie_msi_chip.dev = port->dev;
>> - hw.msi_ctrl = &xilinx_pcie_msi_chip;
>> #endif
>> pci_common_init_dev(dev, &hw);
>>
>>
>
> Hi Lorenzo,
>
> I wonder if we can delete weak pcibios_msi_controller and the call in
> pci_msi_controller in drivers/pci/msi.c
Yeah, I think we can replace it with a straight "return NULL;" in
pci_msi_controller, as ARM is the only arch to implement this function
(which Lorenzo now removes).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-22 9:24 ` Marc Zyngier
@ 2015-07-22 12:49 ` Lorenzo Pieralisi
2015-07-23 15:10 ` Bjorn Helgaas
0 siblings, 1 reply; 9+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-22 12:49 UTC (permalink / raw
To: linux-arm-kernel
Hi Marc,
On Wed, Jul 22, 2015 at 10:24:25AM +0100, Marc Zyngier wrote:
> On 22/07/15 10:02, Lorenzo Pieralisi wrote:
> > On Mon, Jul 13, 2015 at 11:43:25AM +0100, Lorenzo Pieralisi wrote:
> >> On ARM PCI systems relying on the pcibios API to initialize PCI host
> >> controllers, the pcibios_msi_controller weak callback is used to look-up
> >> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
> >>
> >> pci_sys_data is an arch specific structure, which prevents using the
> >> same mechanism (so same PCI host controller drivers) on ARM64 systems.
> >>
> >> Since the struct pci_bus already contains an msi_controller pointer and
> >> the kernel already uses it to look-up the msi controller,
> >> this patch converts ARM host controller and relate pcibios/host bridges
> >> initialization routines so that the msi_controller pointer look-up can be
> >> carried out by PCI core code through the struct pci_bus msi pointer,
> >> removing the need for arch specific pcibios_msi_controller callback
> >> and the related pci_sys_data msi_ctrl pointer.
> >>
> >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> Cc: Pratyush Anand <pratyush.anand@gmail.com>
> >> Cc: Arnd Bergmann <arnd@arndb.de>
> >> Cc: Jingoo Han <jingoohan1@gmail.com>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Simon Horman <horms@verge.net.au>
> >> Cc: Russell King <linux@arm.linux.org.uk>
> >> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> >> Cc: Thierry Reding <thierry.reding@gmail.com>
> >> Cc: Michal Simek <michal.simek@xilinx.com>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >> Bjorn, all,
> >>
> >> I am posting this patch to sound out if that's a reasonable approach.
> >> xgene implements MSI look-up this way at present and this would represent
> >> another step forward towards having common drivers for ARM/ARM64, comments
> >> and testing welcome.
> >
> > Any comments/testing on this patch ? I do not have platforms
> > with these host bridges handy (apart from an iMX6 Sabrelite)
> > so I can't test on them (change is quite mechanical though),
> > help on testing and feedback on the patch much appreciated.
>
> Hi Lorenzo,
>
> I can't test it (no relevant HW), but that seems like a very sensible
> approach to me (the less dependency on sysdata, the better).
Thanks !
> One remark below:
>
> >> arch/arm/include/asm/mach/pci.h | 3 ---
> >> arch/arm/kernel/bios32.c | 35 +++++++++++++++++------------------
> >> drivers/pci/host/pcie-designware.c | 9 +++++++--
> >> drivers/pci/host/pcie-xilinx.c | 12 ++++++++++--
> >> 4 files changed, 34 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> >> index 28b9bb3..32abc0c 100644
> >> --- a/arch/arm/include/asm/mach/pci.h
> >> +++ b/arch/arm/include/asm/mach/pci.h
> >> @@ -42,9 +42,6 @@ struct hw_pci {
> >> * Per-controller structure
> >> */
> >> struct pci_sys_data {
> >> -#ifdef CONFIG_PCI_MSI
> >> - struct msi_controller *msi_ctrl;
> >> -#endif
> >> struct list_head node;
> >> int busnr; /* primary bus number */
> >> u64 mem_offset; /* bus->cpu memory mapping offset */
> >> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> >> index fcbbbb1..c841b33 100644
> >> --- a/arch/arm/kernel/bios32.c
> >> +++ b/arch/arm/kernel/bios32.c
> >> @@ -18,15 +18,6 @@
> >>
> >> static int debug_pci;
> >>
> >> -#ifdef CONFIG_PCI_MSI
> >> -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
> >> -{
> >> - struct pci_sys_data *sysdata = dev->bus->sysdata;
> >> -
> >> - return sysdata->msi_ctrl;
> >> -}
> >> -#endif
> >> -
> >> /*
> >> * We can't use pci_get_device() here since we are
> >> * called from interrupt context.
> >> @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> >> if (!sys)
> >> panic("PCI: unable to allocate sys data!");
> >>
> >> -#ifdef CONFIG_PCI_MSI
> >> - sys->msi_ctrl = hw->msi_ctrl;
> >> -#endif
> >> sys->busnr = busnr;
> >> sys->swizzle = hw->swizzle;
> >> sys->map_irq = hw->map_irq;
> >> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> >> break;
> >> }
> >>
> >> - if (hw->scan)
> >> + if (hw->scan) {
> >> sys->bus = hw->scan(nr, sys);
> >> - else
> >> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
> >> - hw->ops, sys, &sys->resources);
> >> + if (!sys->bus)
> >> + panic("PCI: unable to scan bus!");
>
> This was in the original code, but I have to ask: Do we really want to
> panic the kernel if we couldn't scan the bus? Worse case, the system
> won't be able to boot at all and will panic somewhere else anyway, but
> we should give the user a chance to understand what's happening...
No, it was in the original code but I was very tempted to remove it
or merge the error paths and make it a warning, and that's what I am
going to do, unless someone complains (that panic statement has been there
forever).
> >> + } else {
> >> + sys->bus = pci_create_root_bus(parent,
> >> + sys->busnr,
> >> + hw->ops, sys,
> >> + &sys->resources);
> >> + if (WARN_ON(!sys->bus)) {
> >> + kfree(sys);
> >> + break;
> >> + }
> >> +#ifdef CONFIG_PCI_MSI
> >> + sys->bus->msi = hw->msi_ctrl;
> >> +#endif
> >>
> >> - if (!sys->bus)
> >> - panic("PCI: unable to scan bus!");
> >> + pci_scan_child_bus(sys->bus);
> >> + }
> >>
> >> busnr = sys->bus->busn_res.end + 1;
> >>
> >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> >> index 69486be..e584dfa 100644
> >> --- a/drivers/pci/host/pcie-designware.c
> >> +++ b/drivers/pci/host/pcie-designware.c
> >> @@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >>
> >> #ifdef CONFIG_PCI_MSI
> >> dw_pcie_msi_chip.dev = pp->dev;
> >> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
> >> #endif
> >>
> >> dw_pci.nr_controllers = 1;
> >> @@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> >> struct pcie_port *pp = sys_to_pcie(sys);
> >>
> >> pp->root_bus_nr = sys->busnr;
> >> - bus = pci_scan_root_bus(pp->dev, sys->busnr,
> >> + bus = pci_create_root_bus(pp->dev, sys->busnr,
> >> &dw_pcie_ops, sys, &sys->resources);
> >> if (!bus)
> >> return NULL;
> >>
> >> +#ifdef CONFIG_PCI_MSI
> >> + bus->msi = &dw_pcie_msi_chip;
> >> +#endif
> >> +
> >> + pci_scan_child_bus(bus);
> >> +
> >> if (bus && pp->ops->scan_bus)
> >> pp->ops->scan_bus(pp);
> >>
> >> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
> >> index f1a06a0..b21eb7d 100644
> >> --- a/drivers/pci/host/pcie-xilinx.c
> >> +++ b/drivers/pci/host/pcie-xilinx.c
> >> @@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> >> struct pci_bus *bus;
> >>
> >> port->root_busno = sys->busnr;
> >> - bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> >> + bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> >> sys, &sys->resources);
> >>
> >> + if (!bus)
> >> + return NULL;
> >> +
> >> +#ifdef CONFIG_PCI_MSI
> >> + bus->msi = &xilinx_pcie_msi_chip;
> >> +#endif
> >> +
> >> + pci_scan_child_bus(bus);
> >> +
> >> return bus;
> >> }
> >>
> >> @@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
> >>
> >> #ifdef CONFIG_PCI_MSI
> >> xilinx_pcie_msi_chip.dev = port->dev;
> >> - hw.msi_ctrl = &xilinx_pcie_msi_chip;
> >> #endif
> >> pci_common_init_dev(dev, &hw);
> >>
> >> --
> >> 2.2.1
> >>
> >
>
> Bar the nit above:
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Thanks ! I will prepare a v2 with required changes.
Lorenzo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-22 10:15 ` Marc Zyngier
@ 2015-07-22 12:52 ` Lorenzo Pieralisi
0 siblings, 0 replies; 9+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-22 12:52 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 22, 2015 at 11:15:53AM +0100, Marc Zyngier wrote:
> On 22/07/15 10:30, Zhou Wang wrote:
> > On 2015/7/13 18:43, Lorenzo Pieralisi wrote:
> >> On ARM PCI systems relying on the pcibios API to initialize PCI host
> >> controllers, the pcibios_msi_controller weak callback is used to look-up
> >> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
> >>
> >> pci_sys_data is an arch specific structure, which prevents using the
> >> same mechanism (so same PCI host controller drivers) on ARM64 systems.
> >>
> >> Since the struct pci_bus already contains an msi_controller pointer and
> >> the kernel already uses it to look-up the msi controller,
> >> this patch converts ARM host controller and relate pcibios/host bridges
> >> initialization routines so that the msi_controller pointer look-up can be
> >> carried out by PCI core code through the struct pci_bus msi pointer,
> >> removing the need for arch specific pcibios_msi_controller callback
> >> and the related pci_sys_data msi_ctrl pointer.
> >>
> >> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> Cc: Pratyush Anand <pratyush.anand@gmail.com>
> >> Cc: Arnd Bergmann <arnd@arndb.de>
> >> Cc: Jingoo Han <jingoohan1@gmail.com>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Simon Horman <horms@verge.net.au>
> >> Cc: Russell King <linux@arm.linux.org.uk>
> >> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> >> Cc: Thierry Reding <thierry.reding@gmail.com>
> >> Cc: Michal Simek <michal.simek@xilinx.com>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >> Bjorn, all,
> >>
> >> I am posting this patch to sound out if that's a reasonable approach.
> >> xgene implements MSI look-up this way at present and this would represent
> >> another step forward towards having common drivers for ARM/ARM64, comments
> >> and testing welcome.
> >>
> >> Thanks,
> >> Lorenzo
> >>
> >> arch/arm/include/asm/mach/pci.h | 3 ---
> >> arch/arm/kernel/bios32.c | 35 +++++++++++++++++------------------
> >> drivers/pci/host/pcie-designware.c | 9 +++++++--
> >> drivers/pci/host/pcie-xilinx.c | 12 ++++++++++--
> >> 4 files changed, 34 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
> >> index 28b9bb3..32abc0c 100644
> >> --- a/arch/arm/include/asm/mach/pci.h
> >> +++ b/arch/arm/include/asm/mach/pci.h
> >> @@ -42,9 +42,6 @@ struct hw_pci {
> >> * Per-controller structure
> >> */
> >> struct pci_sys_data {
> >> -#ifdef CONFIG_PCI_MSI
> >> - struct msi_controller *msi_ctrl;
> >> -#endif
> >> struct list_head node;
> >> int busnr; /* primary bus number */
> >> u64 mem_offset; /* bus->cpu memory mapping offset */
> >> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> >> index fcbbbb1..c841b33 100644
> >> --- a/arch/arm/kernel/bios32.c
> >> +++ b/arch/arm/kernel/bios32.c
> >> @@ -18,15 +18,6 @@
> >>
> >> static int debug_pci;
> >>
> >> -#ifdef CONFIG_PCI_MSI
> >> -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
> >> -{
> >> - struct pci_sys_data *sysdata = dev->bus->sysdata;
> >> -
> >> - return sysdata->msi_ctrl;
> >> -}
> >> -#endif
> >> -
> >> /*
> >> * We can't use pci_get_device() here since we are
> >> * called from interrupt context.
> >> @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> >> if (!sys)
> >> panic("PCI: unable to allocate sys data!");
> >>
> >> -#ifdef CONFIG_PCI_MSI
> >> - sys->msi_ctrl = hw->msi_ctrl;
> >> -#endif
> >> sys->busnr = busnr;
> >> sys->swizzle = hw->swizzle;
> >> sys->map_irq = hw->map_irq;
> >> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> >> break;
> >> }
> >>
> >> - if (hw->scan)
> >> + if (hw->scan) {
> >> sys->bus = hw->scan(nr, sys);
> >> - else
> >> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
> >> - hw->ops, sys, &sys->resources);
> >> + if (!sys->bus)
> >> + panic("PCI: unable to scan bus!");
> >> + } else {
> >> + sys->bus = pci_create_root_bus(parent,
> >> + sys->busnr,
> >> + hw->ops, sys,
> >> + &sys->resources);
> >> + if (WARN_ON(!sys->bus)) {
> >> + kfree(sys);
> >> + break;
> >> + }
> >> +#ifdef CONFIG_PCI_MSI
> >> + sys->bus->msi = hw->msi_ctrl;
> >> +#endif
> >>
> >> - if (!sys->bus)
> >> - panic("PCI: unable to scan bus!");
> >> + pci_scan_child_bus(sys->bus);
> >> + }
> >>
> >> busnr = sys->bus->busn_res.end + 1;
> >>
> >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> >> index 69486be..e584dfa 100644
> >> --- a/drivers/pci/host/pcie-designware.c
> >> +++ b/drivers/pci/host/pcie-designware.c
> >> @@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >>
> >> #ifdef CONFIG_PCI_MSI
> >> dw_pcie_msi_chip.dev = pp->dev;
> >> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
> >> #endif
> >>
> >> dw_pci.nr_controllers = 1;
> >> @@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> >> struct pcie_port *pp = sys_to_pcie(sys);
> >>
> >> pp->root_bus_nr = sys->busnr;
> >> - bus = pci_scan_root_bus(pp->dev, sys->busnr,
> >> + bus = pci_create_root_bus(pp->dev, sys->busnr,
> >> &dw_pcie_ops, sys, &sys->resources);
> >> if (!bus)
> >> return NULL;
> >>
> >> +#ifdef CONFIG_PCI_MSI
> >> + bus->msi = &dw_pcie_msi_chip;
> >> +#endif
> >> +
> >> + pci_scan_child_bus(bus);
> >> +
> >> if (bus && pp->ops->scan_bus)
> >> pp->ops->scan_bus(pp);
> >>
> >> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
> >> index f1a06a0..b21eb7d 100644
> >> --- a/drivers/pci/host/pcie-xilinx.c
> >> +++ b/drivers/pci/host/pcie-xilinx.c
> >> @@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> >> struct pci_bus *bus;
> >>
> >> port->root_busno = sys->busnr;
> >> - bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> >> + bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
> >> sys, &sys->resources);
> >>
> >> + if (!bus)
> >> + return NULL;
> >> +
> >> +#ifdef CONFIG_PCI_MSI
> >> + bus->msi = &xilinx_pcie_msi_chip;
> >> +#endif
> >> +
> >> + pci_scan_child_bus(bus);
> >> +
> >> return bus;
> >> }
> >>
> >> @@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
> >>
> >> #ifdef CONFIG_PCI_MSI
> >> xilinx_pcie_msi_chip.dev = port->dev;
> >> - hw.msi_ctrl = &xilinx_pcie_msi_chip;
> >> #endif
> >> pci_common_init_dev(dev, &hw);
> >>
> >>
> >
> > Hi Lorenzo,
> >
> > I wonder if we can delete weak pcibios_msi_controller and the call in
> > pci_msi_controller in drivers/pci/msi.c
>
> Yeah, I think we can replace it with a straight "return NULL;" in
> pci_msi_controller, as ARM is the only arch to implement this function
> (which Lorenzo now removes).
Agreed, I did not do it since I wanted to get feedback on the approach
first, if we agree that's the right thing to do we can kill
pcibios_msi_controller() in the core too (since it becomes unused).
Lorenzo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-22 12:49 ` Lorenzo Pieralisi
@ 2015-07-23 15:10 ` Bjorn Helgaas
2015-07-23 17:15 ` Lorenzo Pieralisi
0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2015-07-23 15:10 UTC (permalink / raw
To: linux-arm-kernel
On Wed, Jul 22, 2015 at 01:49:29PM +0100, Lorenzo Pieralisi wrote:
> > >> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> > >> break;
> > >> }
> > >>
> > >> - if (hw->scan)
> > >> + if (hw->scan) {
> > >> sys->bus = hw->scan(nr, sys);
> > >> - else
> > >> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
> > >> - hw->ops, sys, &sys->resources);
> > >> + if (!sys->bus)
> > >> + panic("PCI: unable to scan bus!");
> >
> > This was in the original code, but I have to ask: Do we really want to
> > panic the kernel if we couldn't scan the bus? Worse case, the system
> > won't be able to boot at all and will panic somewhere else anyway, but
> > we should give the user a chance to understand what's happening...
>
> No, it was in the original code but I was very tempted to remove it
> or merge the error paths and make it a warning, and that's what I am
> going to do, unless someone complains (that panic statement has been there
> forever).
I agree the panic should be removed. I would do it in a separate patch
since it's not related to your main objective here.
Bjorn
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFT PATCH] ARM: pci: kill pcibios_msi_controller
2015-07-23 15:10 ` Bjorn Helgaas
@ 2015-07-23 17:15 ` Lorenzo Pieralisi
0 siblings, 0 replies; 9+ messages in thread
From: Lorenzo Pieralisi @ 2015-07-23 17:15 UTC (permalink / raw
To: linux-arm-kernel
On Thu, Jul 23, 2015 at 04:10:18PM +0100, Bjorn Helgaas wrote:
> On Wed, Jul 22, 2015 at 01:49:29PM +0100, Lorenzo Pieralisi wrote:
>
> > > >> @@ -483,14 +471,25 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
> > > >> break;
> > > >> }
> > > >>
> > > >> - if (hw->scan)
> > > >> + if (hw->scan) {
> > > >> sys->bus = hw->scan(nr, sys);
> > > >> - else
> > > >> - sys->bus = pci_scan_root_bus(parent, sys->busnr,
> > > >> - hw->ops, sys, &sys->resources);
> > > >> + if (!sys->bus)
> > > >> + panic("PCI: unable to scan bus!");
> > >
> > > This was in the original code, but I have to ask: Do we really want to
> > > panic the kernel if we couldn't scan the bus? Worse case, the system
> > > won't be able to boot at all and will panic somewhere else anyway, but
> > > we should give the user a chance to understand what's happening...
> >
> > No, it was in the original code but I was very tempted to remove it
> > or merge the error paths and make it a warning, and that's what I am
> > going to do, unless someone complains (that panic statement has been there
> > forever).
>
> I agree the panic should be removed. I would do it in a separate patch
> since it's not related to your main objective here.
Ok, I will post v2 shortly.
Thanks !
Lorenzo
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-07-23 17:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-13 10:43 [RFT PATCH] ARM: pci: kill pcibios_msi_controller Lorenzo Pieralisi
2015-07-22 9:02 ` Lorenzo Pieralisi
2015-07-22 9:24 ` Marc Zyngier
2015-07-22 12:49 ` Lorenzo Pieralisi
2015-07-23 15:10 ` Bjorn Helgaas
2015-07-23 17:15 ` Lorenzo Pieralisi
2015-07-22 9:30 ` Zhou Wang
2015-07-22 10:15 ` Marc Zyngier
2015-07-22 12:52 ` Lorenzo Pieralisi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).