diff mbox

[v2,1/3] arm/pci: Add support architecture-independent PCIe driver

Message ID 1429091315-31891-2-git-send-email-Minghuan.Lian@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Minghuan Lian April 15, 2015, 9:48 a.m. UTC
PCIe common driver of arm architecture uses private structure
pci_sys_data and hw_pci to associate with specific PCIe controller
ops which results in the PCIe controller driver not compatible
with other architectures. This patch provides another approach
to support architecture-independent PCIe driver which does not
need to use pci_sys_data and hw_pci and call pci_common_init_dev().

Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
---
change log:
v1-v2: no change

 arch/arm/kernel/bios32.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Arnd Bergmann April 15, 2015, 10:16 a.m. UTC | #1
On Wednesday 15 April 2015 17:48:33 Minghuan Lian wrote:
> PCIe common driver of arm architecture uses private structure
> pci_sys_data and hw_pci to associate with specific PCIe controller
> ops which results in the PCIe controller driver not compatible
> with other architectures. This patch provides another approach
> to support architecture-independent PCIe driver which does not
> need to use pci_sys_data and hw_pci and call pci_common_init_dev().
> 
> Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>

This looks ok in principle, but I think we already have plans to
fix this up in a better way.

Your code is broken in the theoretical case where you'd have
two PCI host bridges, and only one of them uses pci_common_init_dev.

> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index ab19b7c0..8820ed5 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -7,6 +7,7 @@
>   */
>  #include <linux/export.h>
>  #include <linux/kernel.h>
> +#include <linux/of_pci.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/init.h>
> @@ -17,12 +18,16 @@
>  #include <asm/mach/pci.h>
>  
>  static int debug_pci;
> +static int pci_commont_init_enable;
>  
>  #ifdef CONFIG_PCI_MSI
>  struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
>  {
>  	struct pci_sys_data *sysdata = dev->bus->sysdata;
>  
> +	if (!pci_commont_init_enable)
> +		return NULL;
> +
>  	return sysdata->msi_ctrl;
>  }
>  #endif
> @@ -508,6 +513,8 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
>  	struct pci_sys_data *sys;
>  	LIST_HEAD(head);
>  
> +	pci_commont_init_enable = 1;
> +
>  	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
>  	if (hw->preinit)
>  		hw->preinit();

A simpler hack to achieve the same thing is to add an empty pci_sys_data
member to the designware pcie_port:

+++ b/drivers/pci/host/pcie-designware.h
@@ -23,6 +23,15 @@
 #define MAX_MSI_CTRLS                  (MAX_MSI_IRQS / 32)
 
 struct pcie_port {
+#ifdef CONFIG_ARM
+       /*
+        * this is a temporary hack to let the driver work on
+        * both arm32 and arm64. it can be removed after the
+        * arm32 cleanup is complete and bios32.c has stopped
+        * referencing host->pci_sys_data.
+        */
+       struct pci_sys_data     dummy;
+#endif
        struct device           *dev;
        u8                      root_bus_nr;
        void __iomem            *dbi_base;

I've just drafted a patch in reply to the hisilicon patch that tries to
achieve the same thing as yours, see "Re: [RFC PATCH 1/3] PCI: host:
designware: support ARM64".


> @@ -651,3 +658,13 @@ void __init pci_map_io_early(unsigned long pfn)
>  	pci_io_desc.pfn = pfn;
>  	iotable_init(&pci_io_desc, 1);
>  }
> +
> +/*
> + * Try to assign the IRQ number from DT when adding a new device
> + */
> +int pcibios_add_device(struct pci_dev *dev)
> +{
> +	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> +
> +	return 0;
> +}

I don't see why this is required here. Doesn't this break platforms
that get their IRQ in some other way?

	Arnd
Minghuan Lian April 15, 2015, 11:54 a.m. UTC | #2
Hi Arnd,

Please see my comments inline.
 
> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Wednesday, April 15, 2015 6:17 PM
> To: linux-arm-kernel@lists.infradead.org
> Cc: Lian Minghuan-B31939; linux-pci@vger.kernel.org; Jingoo Han; Hu
> Mingkai-B21284; Zang Roy-R61911; Yoder Stuart-B08248; Bjorn Helgaas;
> Wood Scott-B07421
> Subject: Re: [PATCH v2 1/3] arm/pci: Add support architecture-independent
> PCIe driver
> 
> On Wednesday 15 April 2015 17:48:33 Minghuan Lian wrote:
> > PCIe common driver of arm architecture uses private structure
> > pci_sys_data and hw_pci to associate with specific PCIe controller ops
> > which results in the PCIe controller driver not compatible with other
> > architectures. This patch provides another approach to support
> > architecture-independent PCIe driver which does not need to use
> > pci_sys_data and hw_pci and call pci_common_init_dev().
> >
> > Signed-off-by: Minghuan Lian <Minghuan.Lian@freescale.com>
> 
> This looks ok in principle, but I think we already have plans to fix this up in a
> better way.
> 
> Your code is broken in the theoretical case where you'd have two PCI host
> bridges, and only one of them uses pci_common_init_dev.
> 
[Minghuan] Yes, my method is not really good, it is just a workaround before the better fixup is merged.

> > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index
> > ab19b7c0..8820ed5 100644
> > --- a/arch/arm/kernel/bios32.c
> > +++ b/arch/arm/kernel/bios32.c
> > @@ -7,6 +7,7 @@
> >   */
> >  #include <linux/export.h>
> >  #include <linux/kernel.h>
> > +#include <linux/of_pci.h>
> >  #include <linux/pci.h>
> >  #include <linux/slab.h>
> >  #include <linux/init.h>
> > @@ -17,12 +18,16 @@
> >  #include <asm/mach/pci.h>
> >
> >  static int debug_pci;
> > +static int pci_commont_init_enable;
> >
> >  #ifdef CONFIG_PCI_MSI
> >  struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)  {
> >  	struct pci_sys_data *sysdata = dev->bus->sysdata;
> >
> > +	if (!pci_commont_init_enable)
> > +		return NULL;
> > +
> >  	return sysdata->msi_ctrl;
> >  }
> >  #endif
> > @@ -508,6 +513,8 @@ void pci_common_init_dev(struct device *parent,
> struct hw_pci *hw)
> >  	struct pci_sys_data *sys;
> >  	LIST_HEAD(head);
> >
> > +	pci_commont_init_enable = 1;
> > +
> >  	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
> >  	if (hw->preinit)
> >  		hw->preinit();
> 
> A simpler hack to achieve the same thing is to add an empty pci_sys_data
> member to the designware pcie_port:
> 
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -23,6 +23,15 @@
>  #define MAX_MSI_CTRLS                  (MAX_MSI_IRQS / 32)
> 
>  struct pcie_port {
> +#ifdef CONFIG_ARM
> +       /*
> +        * this is a temporary hack to let the driver work on
> +        * both arm32 and arm64. it can be removed after the
> +        * arm32 cleanup is complete and bios32.c has stopped
> +        * referencing host->pci_sys_data.
> +        */
> +       struct pci_sys_data     dummy;
> +#endif
>         struct device           *dev;
>         u8                      root_bus_nr;
>         void __iomem            *dbi_base;
> 
> I've just drafted a patch in reply to the hisilicon patch that tries to achieve the
> same thing as yours, see "Re: [RFC PATCH 1/3] PCI: host:
> designware: support ARM64".
> 
> 
> > @@ -651,3 +658,13 @@ void __init pci_map_io_early(unsigned long pfn)
> >  	pci_io_desc.pfn = pfn;
> >  	iotable_init(&pci_io_desc, 1);
> >  }
> > +
> > +/*
> > + * Try to assign the IRQ number from DT when adding a new device  */
> > +int pcibios_add_device(struct pci_dev *dev) {
> > +	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> > +
> > +	return 0;
> > +}
> 
> I don't see why this is required here. Doesn't this break platforms that get
> their IRQ in some other way?
> 
[Minghuan] It is for IRQ map to replace sys->map_irq.

> 	Arnd
diff mbox

Patch

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index ab19b7c0..8820ed5 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -7,6 +7,7 @@ 
  */
 #include <linux/export.h>
 #include <linux/kernel.h>
+#include <linux/of_pci.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/init.h>
@@ -17,12 +18,16 @@ 
 #include <asm/mach/pci.h>
 
 static int debug_pci;
+static int pci_commont_init_enable;
 
 #ifdef CONFIG_PCI_MSI
 struct msi_controller *pcibios_msi_controller(struct pci_dev *dev)
 {
 	struct pci_sys_data *sysdata = dev->bus->sysdata;
 
+	if (!pci_commont_init_enable)
+		return NULL;
+
 	return sysdata->msi_ctrl;
 }
 #endif
@@ -508,6 +513,8 @@  void pci_common_init_dev(struct device *parent, struct hw_pci *hw)
 	struct pci_sys_data *sys;
 	LIST_HEAD(head);
 
+	pci_commont_init_enable = 1;
+
 	pci_add_flags(PCI_REASSIGN_ALL_RSRC);
 	if (hw->preinit)
 		hw->preinit();
@@ -597,7 +604,7 @@  resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 
 	start = (start + align - 1) & ~(align - 1);
 
-	if (sys->align_resource)
+	if (pci_commont_init_enable && sys->align_resource)
 		return sys->align_resource(dev, res, start, size, align);
 
 	return start;
@@ -651,3 +658,13 @@  void __init pci_map_io_early(unsigned long pfn)
 	pci_io_desc.pfn = pfn;
 	iotable_init(&pci_io_desc, 1);
 }
+
+/*
+ * Try to assign the IRQ number from DT when adding a new device
+ */
+int pcibios_add_device(struct pci_dev *dev)
+{
+	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+
+	return 0;
+}