[PATCH v8 0/2] adding PCI support to AXS10x

[PATCH v8 0/2] adding PCI support to AXS10x

[Joao Pinto]

This patch set has the goal to add suppport for DesignWare PCIe RC in ARC
AXS10x. It includes the necessary tweaks to the ARC architecture, 
necessary tweaks to the PCI subsystem and a new driver (pcie-synopsys).
This new driver will be used extensively in the PCIe RC Prototyping Kit.

The patches were produced against Bjorn Helgaas' repository. It was properly
tested in an IP Prototyping Kit.

Joao Pinto (2):
  PCI support added to ARC
  add new platform driver for PCI RC

 .../devicetree/bindings/pci/pcie-dw-pltfm.txt      |  36 +++
 MAINTAINERS                                        |   7 +
 arch/arc/Kconfig                                   |  23 ++
 arch/arc/include/asm/dma.h                         |   5 +
 arch/arc/include/asm/io.h                          |   9 +
 arch/arc/include/asm/pci.h                         |  31 +++
 arch/arc/kernel/Makefile                           |   1 +
 arch/arc/kernel/pcibios.c                          |  23 ++
 arch/arc/plat-axs10x/Kconfig                       |   1 +
 drivers/pci/Makefile                               |   1 +
 drivers/pci/host/Kconfig                           |   8 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-dw-pltfm.c                   | 244 +++++++++++++++++++++
 13 files changed, 390 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
 create mode 100644 arch/arc/include/asm/pci.h
 create mode 100644 arch/arc/kernel/pcibios.c
 create mode 100644 drivers/pci/host/pcie-dw-pltfm.c

-- 
1.8.1.5

[PATCH v8 1/2] PCI support added to ARC

[Joao Pinto]

This patch adds PCI support to ARC and updates drivers/pci Makefile
enabling the ARC arch to use the generic PCI setup functions.

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
Change v7 -> v8:
- Nothing changed (just to keep up with patch set version).
Change v6 -> v7 (Bjorn Helgaas):
- Parenthesis removed from definitions
Change v5 -> v6 (Vineet Gupta):
- Removed EXPORT_SYMBOL(pcibios_fixup_bus) from pcibios.c
Change v4 -> v5 (Vineet Gupta):
- pcibios.c unused functions, variables and includes were removed
- ioremap.c has no need for changes (no patch is necessary)
- pci.h unused functions and variables were removed
Change v3 -> v4:
- Nothing changed (just to keep up with patch set version).
Change v2 -> v3 (Bjorn Helgaas):
- arch/arc/kernel/pcibios.c unused functions were removed and also the
arch/arc/include/asm/mach/pci.h was removed because was no longer necessary
Change v1 -> v2:
- In arch/arc/Kconfig, the new menu entry (Bus Configuration) was moved to the
slot between sourcing of drivers/Kconfig and fs/Kconfig (Vineet Gupta)
- In arch/arc/plat-axs10x/Kconfig the "select MIGHT_HAVE_PCI" option was placed
in order as suggested (Vineet Gupta)
- ioport_map() and ioport_unmap() were static inlined and included in
the io.h (Vineet Gupta)
- pcibios_min_io and pcibios_min_mem declaration moved to
pcibios.c (Vineet Gupta)
- pr_err() replaced by dev_err() in pcibios_enable_device() (Bjorn Helgaas)
- string simplified in pcibios_enable_device() (Vineet Gupta)
- pci_host_bridge_window structure was replaced by resource_entry structure, and
list_for_each_entry() for resource_list_for_each_entry() in pcibios.c

 arch/arc/Kconfig             | 23 +++++++++++++++++++++++
 arch/arc/include/asm/dma.h   |  5 +++++
 arch/arc/include/asm/io.h    |  9 +++++++++
 arch/arc/include/asm/pci.h   | 31 +++++++++++++++++++++++++++++++
 arch/arc/kernel/Makefile     |  1 +
 arch/arc/kernel/pcibios.c    | 23 +++++++++++++++++++++++
 arch/arc/plat-axs10x/Kconfig |  1 +
 drivers/pci/Makefile         |  1 +
 8 files changed, 94 insertions(+)
 create mode 100644 arch/arc/include/asm/pci.h
 create mode 100644 arch/arc/kernel/pcibios.c

diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index 2c2ac3f..98b32c1 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -19,6 +19,7 @@ config ARC
 	select GENERIC_FIND_FIRST_BIT
 	# for now, we don't need GENERIC_IRQ_PROBE, CONFIG_GENERIC_IRQ_CHIP
 	select GENERIC_IRQ_SHOW
+	select GENERIC_PCI_IOMAP
 	select GENERIC_PENDING_IRQ if SMP
 	select GENERIC_SMP_IDLE_THREAD
 	select HAVE_ARCH_KGDB
@@ -39,6 +40,9 @@ config ARC
 	select PERF_USE_VMALLOC
 	select HAVE_DEBUG_STACKOVERFLOW
 
+config MIGHT_HAVE_PCI
+	bool
+
 config TRACE_IRQFLAGS_SUPPORT
 	def_bool y
 
@@ -570,6 +574,25 @@ endmenu	 # "ARC Architecture Configuration"
 source "mm/Kconfig"
 source "net/Kconfig"
 source "drivers/Kconfig"
+
+menu "Bus Support"
+
+config PCI
+	bool "PCI support" if MIGHT_HAVE_PCI
+	help
+	  PCI is the name of a bus system, i.e. the way the CPU talks to the other stuff inside
+	  your box.Find out if your board/platform have PCI.
+	  Note: PCIE support for Synopsys Device will be available only when
+	  HAPS DX is configured with PCIE RC bitmap. If you have PCI, say Y, otherwise N.
+
+config PCI_SYSCALL
+	def_bool PCI
+
+source "drivers/pci/Kconfig"
+source "drivers/pci/pcie/Kconfig"
+
+endmenu
+
 source "fs/Kconfig"
 source "arch/arc/Kconfig.debug"
 source "security/Kconfig"
diff --git a/arch/arc/include/asm/dma.h b/arch/arc/include/asm/dma.h
index ca7c451..01e47a6 100644
--- a/arch/arc/include/asm/dma.h
+++ b/arch/arc/include/asm/dma.h
@@ -10,5 +10,10 @@
 #define ASM_ARC_DMA_H
 
 #define MAX_DMA_ADDRESS 0xC0000000
+#ifdef CONFIG_PCI
+extern int isa_dma_bridge_buggy;
+#else
+#define isa_dma_bridge_buggy	0
+#endif
 
 #endif
diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
index 694ece8..947bf0c 100644
--- a/arch/arc/include/asm/io.h
+++ b/arch/arc/include/asm/io.h
@@ -16,6 +16,15 @@
 extern void __iomem *ioremap(unsigned long physaddr, unsigned long size);
 extern void __iomem *ioremap_prot(phys_addr_t offset, unsigned long size,
 				  unsigned long flags);
+static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
+{
+	return (void __iomem *)port;
+}
+
+static inline void ioport_unmap(void __iomem *addr)
+{
+}
+
 extern void iounmap(const void __iomem *addr);
 
 #define ioremap_nocache(phy, sz)	ioremap(phy, sz)
diff --git a/arch/arc/include/asm/pci.h b/arch/arc/include/asm/pci.h
new file mode 100644
index 0000000..2f2011c
--- /dev/null
+++ b/arch/arc/include/asm/pci.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _ASM_ARC_PCI_H
+#define _ASM_ARC_PCI_H
+
+#ifdef __KERNEL__
+#include <asm-generic/pci-dma-compat.h>
+#include <asm-generic/pci-bridge.h>
+
+#include <linux/ioport.h>
+
+#define PCIBIOS_MIN_IO 0x100
+#define PCIBIOS_MIN_MEM 0x100000
+
+#define pcibios_assign_all_busses()	1
+/*
+ * The PCI address space does equal the physical memory address space.
+ * The networking and block device layers use this boolean for bounce
+ * buffer decisions.
+ */
+#define PCI_DMA_BUS_IS_PHYS	1
+
+#endif /* __KERNEL__ */
+
+#endif /* _ASM_ARC_PCI_H */
diff --git a/arch/arc/kernel/Makefile b/arch/arc/kernel/Makefile
index e7f3625..1bc2036 100644
--- a/arch/arc/kernel/Makefile
+++ b/arch/arc/kernel/Makefile
@@ -12,6 +12,7 @@ obj-y	:= arcksyms.o setup.o irq.o time.o reset.o ptrace.o process.o devtree.o
 obj-y	+= signal.o traps.o sys.o troubleshoot.o stacktrace.o disasm.o clk.o
 obj-$(CONFIG_ISA_ARCOMPACT)		+= entry-compact.o intc-compact.o
 obj-$(CONFIG_ISA_ARCV2)			+= entry-arcv2.o intc-arcv2.o
+obj-$(CONFIG_PCI)  			+= pcibios.o
 
 obj-$(CONFIG_MODULES)			+= arcksyms.o module.o
 obj-$(CONFIG_SMP) 			+= smp.o
diff --git a/arch/arc/kernel/pcibios.c b/arch/arc/kernel/pcibios.c
new file mode 100644
index 0000000..7bc3a42
--- /dev/null
+++ b/arch/arc/kernel/pcibios.c
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2014-2015 Synopsys, Inc. (www.synopsys.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/pci.h>
+
+/*
+ * We don't have to worry about legacy ISA devices, so nothing to do here
+ */
+resource_size_t pcibios_align_resource(void *data, const struct resource *res,
+				resource_size_t size, resource_size_t align)
+{
+	return res->start;
+}
+
+void pcibios_fixup_bus(struct pci_bus *bus)
+{
+}
diff --git a/arch/arc/plat-axs10x/Kconfig b/arch/arc/plat-axs10x/Kconfig
index d475f9d..426ac4b 100644
--- a/arch/arc/plat-axs10x/Kconfig
+++ b/arch/arc/plat-axs10x/Kconfig
@@ -11,6 +11,7 @@ menuconfig ARC_PLAT_AXS10X
 	select DW_APB_ICTL
 	select GPIO_DWAPB
 	select OF_GPIO
+	select MIGHT_HAVE_PCI
 	select GENERIC_IRQ_CHIP
 	select ARCH_REQUIRE_GPIOLIB
 	help
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index be3f631..2154092 100644
--- a/drivers/pci/Makefile
+++ b/drivers/pci/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_PCI_IOV) += iov.o
 # Some architectures use the generic PCI setup functions
 #
 obj-$(CONFIG_ALPHA) += setup-irq.o
+obj-$(CONFIG_ARC) += setup-irq.o
 obj-$(CONFIG_ARM) += setup-irq.o
 obj-$(CONFIG_ARM64) += setup-irq.o
 obj-$(CONFIG_UNICORE32) += setup-irq.o
-- 
1.8.1.5

[PATCH v8 2/2] add new platform driver for PCI RC

[Joao Pinto]

This patch adds a new driver that will be the reference platform driver
for all PCI RC IP Protoyping Kits based on ARC SDP.

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
- driver name was changed from pcie-synopsys to pcie-dw-pltfm
- mdelay() replaced for msleep() in the new driver
- Devicetree bindings for the new driver was updated (config space removed
from ranges)
- Unnecessary synopsys_pcie_irq_handler() was removed
- Driver compatibility strings updated
Change v6 -> v7 (Bjorn Helgaas):
- driver name was changed from pcie-snpsdev to pcie-synopsys
- driver internals (functions and certain variables) also changed name
accordingly
- devicetree bindings documentation also changed accordingly
- quirk function dw_pcie_link_retrain() was removed (tests were made
successfully without it)
- driver was changed to meet pci standards (link up confirmation routine,
init rc functions, etc.)
- EPROBE_DEFER were removed
Change v5 -> v6:
- Nothing changed (just to keep up with patch set version).
Change v4 -> v5:
- Nothing changed (just to keep up with patch set version).
Changes v3 -> v4 (Bjorn Helgaas):
- ARCH dependencies were added to the drivers/pci/host/kconfig for the
PCIE_SNPSDEV.
Changes v2 -> v3 (Bjorn Helgaas):
- link init stuff was moved to a snpsdev_pcie_establish_link() function in
pcie-snpsdev
- pcie-snpsdev driver declaration was changed to be more 
standard (Bjorn Helgaas)
- pcie-designware' dw_pcie_link_retrain() now use standard registers from
pci-regs.h (Bjorn Helgaas)
- pcie-snpsdev.txt was complemented with more info (Mark Rutland)
Changes v1 -> v2 (Bjorn Helgaas):
- Fixups snpsdev_pcie_fixup_bridge() and snpsdev_pcie_fixup_res() were removed
from the driver (these functions were for specific tests only and not usefull
in mainline)
- Driver' comments were reviewed (fix Typos and excessive comments removal)
- Removed unnecessary definitions in the driver source (PCIE_PHY_CTRL and
PCIE_PHY_STAT)

 .../devicetree/bindings/pci/pcie-dw-pltfm.txt      |  36 +++
 MAINTAINERS                                        |   7 +
 drivers/pci/host/Kconfig                           |   8 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-dw-pltfm.c                   | 244 +++++++++++++++++++++
 5 files changed, 296 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
 create mode 100644 drivers/pci/host/pcie-dw-pltfm.c

diff --git a/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt b/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
new file mode 100644
index 0000000..ee2a877
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
@@ -0,0 +1,36 @@
+Synopsys PCI RC IP Prototyping Kit
+----------------------------------
+
+This is the reference platform driver to be used in the Synopsys PCI Root
+Complex IP Prototyping Kit.
+
+Required properties:
+- compatible: set to "snps,dw-pcie" or "snps,ipk-pcie";
+- reg: base address and length of the pcie controller registers and
+configuration address space
+- reg-names: Must be "config" for the PCIe configuration space.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions.
+- interrupts: one interrupt source for MSI interrupts, followed by interrupt
+	source for hardware related interrupts.
+- #interrupt-cells: set to <1>
+- num-lanes: set to <1>;
+
+Example configuration:
+
+	pcie: pcie@0xdffff000 {
+		compatible = "snps,dw-pcie";
+		reg = <0xdffff000 0x1000
+		       0xd0000000 0x2000>;
+		reg-names = "csr", "config";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000
+			  0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
+		interrupts = <25>, <24>;
+		#interrupt-cells = <1>;
+		num-lanes = <1>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index e9caa4b..8362189 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8230,6 +8230,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
 F:	drivers/pci/host/pcie-hisi.c
 
+PCI DRIVER FOR SYNOPSYS PROTOTYPING DEVICE
+M:	Joao Pinto <jpinto@synopsys.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
+F:	drivers/pci/host/pcie-dw-pltfm.c
+
 PCMCIA SUBSYSTEM
 P:	Linux PCMCIA Team
 L:	linux-pcmcia@lists.infradead.org
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index f131ba9..61cdc72 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -172,4 +172,12 @@ config PCI_HISI
 	help
 	  Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
 
+config PCIE_DW_PLAT
+	bool "Platform Driver for Synopsys PCIe controller"
+	depends on ARC && OF
+	select PCIEPORTBUS
+	select PCIE_DW
+	help
+	  Say Y here if you want to enable Synopsys PCIe controller platform
+	  driver.
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9d4d3c6..8c84ba4 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
 obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
 obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
 obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
+obj-$(CONFIG_PCIE_DW_PLAT) += pcie-dw-pltfm.o
diff --git a/drivers/pci/host/pcie-dw-pltfm.c b/drivers/pci/host/pcie-dw-pltfm.c
new file mode 100644
index 0000000..0924ec4
--- /dev/null
+++ b/drivers/pci/host/pcie-dw-pltfm.c
@@ -0,0 +1,244 @@
+/*
+ * PCIe RC driver for Synopsys Designware Core
+ *
+ * Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com)
+ *
+ * Authors: Manjunath Bettegowda <manjumb@synopsys.com>,
+ *	    Jie Deng <jiedeng@synopsys.com>
+ *	    Joao Pinto <jpinto@synopsys.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/signal.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define to_dw_pltfm_pcie(x)	container_of(x, struct dw_pltfm_pcie, pp)
+
+struct dw_pltfm_pcie {
+	void __iomem		*mem_base; /* Memory Base to access Core's [RC]
+					    * Config Space Layout
+					    */
+	struct pcie_port	pp;        /* RC Root Port specific structure -
+					    * DWC_PCIE_RC stuff
+					    */
+};
+
+#define PCI_EQUAL_CONTROL_PHY 0x00000707
+#define PCIE_PHY_DEBUG_R1_LINK_UP 0x00000010
+
+/* PCIe Port Logic registers (memory-mapped) */
+#define PLR_OFFSET 0x700
+#define PCIE_PHY_DEBUG_R0 (PLR_OFFSET + 0x28) /* 0x728 */
+#define PCIE_PHY_DEBUG_R1 (PLR_OFFSET + 0x2c) /* 0x72c */
+
+static irqreturn_t dw_pltfm_pcie_msi_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+
+	dw_handle_msi_irq(pp);
+
+	return dw_handle_msi_irq(pp);
+}
+
+static void dw_pltfm_pcie_init_phy(struct pcie_port *pp)
+{
+	/* write Lane 0 Equalization Control fields register */
+	writel(PCI_EQUAL_CONTROL_PHY, pp->dbi_base + 0x154);
+}
+
+static int dw_pltfm_pcie_deassert_core_reset(struct pcie_port *pp)
+{
+	return 0;
+}
+
+static void dw_pltfm_pcie_establish_link(struct pcie_port *pp)
+{
+	int retries = 0;
+
+	/* check if the link is up or not */
+	for (retries = 0; retries < 10; retries++) {
+		if (dw_pcie_link_up(pp)) {
+			dev_info(pp->dev, "Link up\n");
+			return;
+		}
+		msleep(100);
+	}
+}
+
+/*
+ * dw_pltfm_pcie_host_init()
+ * Platform specific host/RC initialization
+ *	a. Assert the core reset
+ *	b. Assert and deassert phy reset and initialize the phy
+ *	c. De-Assert the core reset
+ *	d. Initializet the Root Port (BARs/Memory Or IO/ Interrupt/ Commnad Reg)
+ *	e. Initiate Link startup procedure
+ *
+ */
+static void dw_pltfm_pcie_host_init(struct pcie_port *pp)
+{
+	/* Initialize Phy (Reset/poweron/control-inputs ) */
+	dw_pltfm_pcie_init_phy(pp);
+
+	dw_pltfm_pcie_deassert_core_reset(pp);
+
+	dw_pcie_setup_rc(pp);
+
+	dw_pltfm_pcie_establish_link(pp);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dw_pcie_msi_init(pp);
+}
+
+static int dw_pltfm_pcie_link_up(struct pcie_port *pp)
+{
+	u32 val;
+
+	val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
+	return val & PCIE_PHY_DEBUG_R1_LINK_UP;
+}
+
+/**
+ * This is RC operation structure
+ * dw_pltfm_pcie_link_up: the function which initiates the phy link up procedure
+ * dw_pltfm_pcie_host_init: the function which does the host/RC Root port
+ * initialization.
+ */
+static struct pcie_host_ops dw_pltfm_pcie_host_ops = {
+	.link_up = dw_pltfm_pcie_link_up,
+	.host_init = dw_pltfm_pcie_host_init,
+};
+
+/**
+ * dw_pltfm_add_pcie_port
+ * This function
+ * a. installs the interrupt handler
+ * b. registers host operations in the pcie_port structure
+ */
+static int dw_pltfm_add_pcie_port(struct pcie_port *pp,
+				 struct platform_device *pdev)
+{
+	int ret;
+
+	pp->irq = platform_get_irq(pdev, 1);
+
+	if (pp->irq < 0)
+		return pp->irq;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		pp->msi_irq = platform_get_irq(pdev, 0);
+
+		if (pp->msi_irq < 0)
+			return pp->msi_irq;
+
+		ret = devm_request_irq(&pdev->dev, pp->msi_irq,
+					dw_pltfm_pcie_msi_irq_handler,
+					IRQF_SHARED, "dw-pltfm-pcie-msi", pp);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request msi irq\n");
+			return ret;
+		}
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &dw_pltfm_pcie_host_ops;
+
+	/* Below function:
+	 * Checks for range property from DT
+	 * Gets the IO and MEMORY and CONFIG-Space ranges from DT
+	 * Does IOREMAPS on the physical addresses
+	 * Gets the num-lanes from DT
+	 * Gets MSI capability from DT
+	 * Calls the platform specific host initialization
+	 * Program the correct class, BAR0, Link width, in Config space
+	 * Then it calls pci common init routine
+	 * Then it calls function to assign "unassigned resources"
+	 */
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * dw_pltfm_pcie_probe()
+ * This function gets called as part of pcie registration. if the id matches
+ * the platform driver framework will call this function.
+ *
+ * @pdev: Pointer to the platform_device structure
+ *
+ * Returns zero on success; Negative errorno on failure
+ */
+static int dw_pltfm_pcie_probe(struct platform_device *pdev)
+{
+	struct dw_pltfm_pcie *dw_pltfm_pcie;
+	struct pcie_port *pp;
+	struct resource *res;  /* Resource from DT */
+	int ret;
+
+	dw_pltfm_pcie = devm_kzalloc(&pdev->dev, sizeof(*dw_pltfm_pcie),
+					GFP_KERNEL);
+	if (!dw_pltfm_pcie)
+		return -ENOMEM;
+
+	pp = &dw_pltfm_pcie->pp;
+	pp->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!res)
+		return -ENODEV;
+
+	dw_pltfm_pcie->mem_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dw_pltfm_pcie->mem_base))
+		return PTR_ERR(dw_pltfm_pcie->mem_base);
+
+	pp->dbi_base = dw_pltfm_pcie->mem_base;
+
+	ret = dw_pltfm_add_pcie_port(pp, pdev);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, dw_pltfm_pcie);
+
+	return 0;
+}
+
+static const struct of_device_id dw_pltfm_pcie_of_match[] = {
+	{ .compatible = "snps,dw-pcie", },
+	{ .compatible = "snps,ipk-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_pltfm_pcie_of_match);
+
+static struct platform_driver dw_pltfm_pcie_driver = {
+	.driver = {
+		.name	= "dw-pcie",
+		.of_match_table = dw_pltfm_pcie_of_match,
+	},
+	.probe = dw_pltfm_pcie_probe,
+};
+
+module_platform_driver(dw_pltfm_pcie_driver);
+
+MODULE_AUTHOR("Manjunath Bettegowda <manjumb@synopsys.com>");
+MODULE_DESCRIPTION("Synopsys PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.5

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Bjorn Helgaas]

On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote:
> This patch adds a new driver that will be the reference platform driver
> for all PCI RC IP Protoyping Kits based on ARC SDP.
> 
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> ---
> Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
> - driver name was changed from pcie-synopsys to pcie-dw-pltfm

"pcie-dw-pltfm" seems worse to me.  We have eight existing drivers
that call dw_pcie_host_init(), and they're all platform_drivers.
"pcie-dw-pltfm" could apply equally well to any of them.

I think I see what happened: I wrote "It doesn't seem necessary to me
to include both 'synopsys' and 'ipk' in the filename and the driver
name."  I meant that using one of them should be sufficient, not that
*both* should be removed.

I don't know the SoC landscape, but from Arnd's comment, it sounds
like "synopsys" might be too generic because many of the other drivers
are connected with Synopsys.  I don't know what "ipk" means, but maybe
that could work.  It's convenient if the name *means* something, and
if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic.
Is "haps" or "haps_dx" a name people would associate with this
hardware?  I guess it'd be nice if the driver name were related to the
DT compat strings, so "ipk" is better from that perspective.

My pci/host-synopsys branch (which is still published even though I
removed it from for-linus) contains your v7 series plus some fixes.
This v8 series lost the fixes.  For example:

> +static irqreturn_t dw_pltfm_pcie_msi_irq_handler(int irq, void *arg)
> +{
> +	struct pcie_port *pp = arg;
> +
> +	dw_handle_msi_irq(pp);
> +
> +	return dw_handle_msi_irq(pp);
> +}

This is a bug.  You should call dw_handle_msi_irq() *once* and return
the value it returns.  I already fixed this in my pci/host-synopsys
branch, so you should start with that, and then apply your v7-v8
changes.

Bjorn

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Joao Pinto]

Hi Bjorn,

On 2/4/2016 6:19 PM, Bjorn Helgaas wrote:
> On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote:
>> This patch adds a new driver that will be the reference platform driver
>> for all PCI RC IP Protoyping Kits based on ARC SDP.
>>
>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>> ---
>> Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
>> - driver name was changed from pcie-synopsys to pcie-dw-pltfm
> 
> "pcie-dw-pltfm" seems worse to me.  We have eight existing drivers
> that call dw_pcie_host_init(), and they're all platform_drivers.
> "pcie-dw-pltfm" could apply equally well to any of them.
> 
> I think I see what happened: I wrote "It doesn't seem necessary to me
> to include both 'synopsys' and 'ipk' in the filename and the driver
> name."  I meant that using one of them should be sufficient, not that
> *both* should be removed.
> 
> I don't know the SoC landscape, but from Arnd's comment, it sounds
> like "synopsys" might be too generic because many of the other drivers
> are connected with Synopsys.  I don't know what "ipk" means, but maybe
> that could work.  It's convenient if the name *means* something, and
> if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic.
> Is "haps" or "haps_dx" a name people would associate with this
> hardware?  I guess it'd be nice if the driver name were related to the
> DT compat strings, so "ipk" is better from that perspective.

Synopsys has a product called IP Prototyping Kit which is a bundle of HAPSDX +
PCIE RC IP + drivers + Development Board. This driver was implemented originally
to serve this IPK but it can be used by SoC that use the Synopsys PCIe RC IP.
"ipk" would say that the driver is usable only in the IP Prototyping Kits which
is not 100% true, it is usable in any SoC with Synopsys IP or in limit serve as
a base for specific SoC drivers.
Suggestions: "pcie-dw-soc-agnostic", "pcie-dw-ipk", "pcie-dw-haps-prototyping"

What do you think?

> 
> My pci/host-synopsys branch (which is still published even though I
> removed it from for-linus) contains your v7 series plus some fixes.
> This v8 series lost the fixes.  For example:
> 
>> +static irqreturn_t dw_pltfm_pcie_msi_irq_handler(int irq, void *arg)
>> +{
>> +	struct pcie_port *pp = arg;
>> +
>> +	dw_handle_msi_irq(pp);
>> +
>> +	return dw_handle_msi_irq(pp);
>> +}
> 
> This is a bug.  You should call dw_handle_msi_irq() *once* and return
> the value it returns.  I already fixed this in my pci/host-synopsys
> branch, so you should start with that, and then apply your v7-v8
> changes.

I am going to fetch your repo and get the host/pcie-synopsys to generate the
next patch version.

> 
> Bjorn
> 

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Bjorn Helgaas]

On Thu, Feb 04, 2016 at 06:31:05PM +0000, Joao Pinto wrote:
> On 2/4/2016 6:19 PM, Bjorn Helgaas wrote:
> > On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote:
> >> This patch adds a new driver that will be the reference platform driver
> >> for all PCI RC IP Protoyping Kits based on ARC SDP.
> >>
> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> >> ---
> >> Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
> >> - driver name was changed from pcie-synopsys to pcie-dw-pltfm
> > 
> > "pcie-dw-pltfm" seems worse to me.  We have eight existing drivers
> > that call dw_pcie_host_init(), and they're all platform_drivers.
> > "pcie-dw-pltfm" could apply equally well to any of them.
> > 
> > I think I see what happened: I wrote "It doesn't seem necessary to me
> > to include both 'synopsys' and 'ipk' in the filename and the driver
> > name."  I meant that using one of them should be sufficient, not that
> > *both* should be removed.
> > 
> > I don't know the SoC landscape, but from Arnd's comment, it sounds
> > like "synopsys" might be too generic because many of the other drivers
> > are connected with Synopsys.  I don't know what "ipk" means, but maybe
> > that could work.  It's convenient if the name *means* something, and
> > if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic.
> > Is "haps" or "haps_dx" a name people would associate with this
> > hardware?  I guess it'd be nice if the driver name were related to the
> > DT compat strings, so "ipk" is better from that perspective.
> 
> Synopsys has a product called IP Prototyping Kit which is a bundle of
> HAPSDX + PCIE RC IP + drivers + Development Board. This driver was
> implemented originally to serve this IPK but it can be used by SoC that
> use the Synopsys PCIe RC IP.  "ipk" would say that the driver is usable
> only in the IP Prototyping Kits which is not 100% true, it is usable in
> any SoC with Synopsys IP or in limit serve as a base for specific SoC
> drivers.  Suggestions: "pcie-dw-soc-agnostic", "pcie-dw-ipk",
> "pcie-dw-haps-prototyping"
> 
> What do you think?

I don't think the "dw" part is relevant (none of the other
DesignWare-based drivers includes it in the driver or file name).

How do people typically refer to this board?

I really like "synopsys" because it fits the pattern of being
recognizable and pronounceable like "altera", "designware", "qcom",
"keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
too generic.

"ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
cover 100% of the possible systems.

Bjorn

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Joao Pinto]

Hi,

On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
>> What do you think?
> 
> I don't think the "dw" part is relevant (none of the other
> DesignWare-based drivers includes it in the driver or file name).
> 
> How do people typically refer to this board?
> 
> I really like "synopsys" because it fits the pattern of being
> recognizable and pronounceable like "altera", "designware", "qcom",
> "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
> too generic.
> 
> "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
> cover 100% of the possible systems.

I think we should follow the iproc example: pcie-iproc-platform.c
In this case we would have pcie-designware-platform.c
I think this would be the best name because the driver is a non soc specific
designware platform driver.

Arnd and Bjorn agree on this name?

> 
> Bjorn
> 

Joao

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Arnd Bergmann]

On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
> Hi,
> 
> On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
> >> What do you think?
> > 
> > I don't think the "dw" part is relevant (none of the other
> > DesignWare-based drivers includes it in the driver or file name).
> > 
> > How do people typically refer to this board?
> > 
> > I really like "synopsys" because it fits the pattern of being
> > recognizable and pronounceable like "altera", "designware", "qcom",
> > "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
> > too generic.
> > 
> > "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
> > cover 100% of the possible systems.
> 
> I think we should follow the iproc example: pcie-iproc-platform.c
> In this case we would have pcie-designware-platform.c
> I think this would be the best name because the driver is a non soc specific
> designware platform driver.
> 
> Arnd and Bjorn agree on this name?

Sorry, I did not realize that your submission was for the generic dw-pcie
implementation rather than a particular product integrating it.

I think in this case, we should do this completely differently:

How about putting all the new code into drivers/pci/host/pcie-designware.c
as functions that can be used by the other drivers in absence of a chip
specific handler?

Instead of providing a new instance of struct pcie_host_ops, maybe add
it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
for drivers that don't provide their own. "hisi_pcie_host_ops" currently
provides no host_init() callback function, so you will have to change
the hisi frontend to a provide nop-function.

For all other drivers, check if they can be changed to use your generic
implementation and remove their private callbacks if possible.

I think the MSI implementation should be split out into a separate file
though, as not everyone uses this.

	Arnd

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Joao Pinto]

On 2/5/2016 2:39 PM, Arnd Bergmann wrote:
> On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
>> Hi,
>>
>> On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
>>>> What do you think?
>>>
>>> I don't think the "dw" part is relevant (none of the other
>>> DesignWare-based drivers includes it in the driver or file name).
>>>
>>> How do people typically refer to this board?
>>>
>>> I really like "synopsys" because it fits the pattern of being
>>> recognizable and pronounceable like "altera", "designware", "qcom",
>>> "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
>>> too generic.
>>>
>>> "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
>>> cover 100% of the possible systems.
>>
>> I think we should follow the iproc example: pcie-iproc-platform.c
>> In this case we would have pcie-designware-platform.c
>> I think this would be the best name because the driver is a non soc specific
>> designware platform driver.
>>
>> Arnd and Bjorn agree on this name?
> 
> Sorry, I did not realize that your submission was for the generic dw-pcie
> implementation rather than a particular product integrating it.
> 

It is a driver that is useful for PCIe RC prototyping and to be a reference
platform driver for DesignWare PCIe RC, I don't know if merging some of the
driver's code into pcie-designware is really necessary depends on the usefulness
of it. I would suggest that bigger step to be done in a 2nd stage since I will
be around to maintain what's necessary. Agree?

I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to
match your suggestions and Bjorn's. Could you please comment check it?

> I think in this case, we should do this completely differently:
> 
> How about putting all the new code into drivers/pci/host/pcie-designware.c
> as functions that can be used by the other drivers in absence of a chip
> specific handler?
> 
> Instead of providing a new instance of struct pcie_host_ops, maybe add
> it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
> for drivers that don't provide their own. "hisi_pcie_host_ops" currently
> provides no host_init() callback function, so you will have to change
> the hisi frontend to a provide nop-function.
> 
> For all other drivers, check if they can be changed to use your generic
> implementation and remove their private callbacks if possible.
> 
> I think the MSI implementation should be split out into a separate file
> though, as not everyone uses this.
> 
> 	Arnd
> 

Joao

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Arnd Bergmann]

On Friday 05 February 2016 14:51:39 Joao Pinto wrote:
> 
> It is a driver that is useful for PCIe RC prototyping and to be a reference
> platform driver for DesignWare PCIe RC, I don't know if merging some of the
> driver's code into pcie-designware is really necessary depends on the usefulness
> of it. I would suggest that bigger step to be done in a 2nd stage since I will
> be around to maintain what's necessary. Agree?
> 
> I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to
> match your suggestions and Bjorn's. Could you please comment check it?

I think it would be useful to do this as generic as possible, and you seem
to be the right person to integrate it into the generic driver as you have
access to the hardware documents. Normally the folks writing the drivers
are just guessing based on what little information they get out of manuals
or vendor-provided drivers, but I'm sure that not all of that makes sense.

	Arnd

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Joao Pinto]

On 2/5/2016 3:43 PM, Arnd Bergmann wrote:
> On Friday 05 February 2016 14:51:39 Joao Pinto wrote:
>>
>> It is a driver that is useful for PCIe RC prototyping and to be a reference
>> platform driver for DesignWare PCIe RC, I don't know if merging some of the
>> driver's code into pcie-designware is really necessary depends on the usefulness
>> of it. I would suggest that bigger step to be done in a 2nd stage since I will
>> be around to maintain what's necessary. Agree?
>>
>> I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to
>> match your suggestions and Bjorn's. Could you please comment check it?
> 
> I think it would be useful to do this as generic as possible, and you seem
> to be the right person to integrate it into the generic driver as you have
> access to the hardware documents. Normally the folks writing the drivers
> are just guessing based on what little information they get out of manuals
> or vendor-provided drivers, but I'm sure that not all of that makes sense.

Arnd, sure and I would love to be useful and improve the dw pci ecosystem, but
maybe now we should submit the driver as suggested in "[PATCH] synopsys pcie rc
generic platform driver update" and then make in a future intervention.

Bjorn, could you please comment? Should we go now with this driver version and
in the future we make the changes suggested by Arnd?

> 
> 	Arnd
> 

Joao

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Bjorn Helgaas]

On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote:
> On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
> > Hi,
> > 
> > On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
> > >> What do you think?
> > > 
> > > I don't think the "dw" part is relevant (none of the other
> > > DesignWare-based drivers includes it in the driver or file name).
> > > 
> > > How do people typically refer to this board?
> > > 
> > > I really like "synopsys" because it fits the pattern of being
> > > recognizable and pronounceable like "altera", "designware", "qcom",
> > > "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
> > > too generic.
> > > 
> > > "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
> > > cover 100% of the possible systems.
> > 
> > I think we should follow the iproc example: pcie-iproc-platform.c
> > In this case we would have pcie-designware-platform.c
> > I think this would be the best name because the driver is a non soc specific
> > designware platform driver.
> > 
> > Arnd and Bjorn agree on this name?
> 
> Sorry, I did not realize that your submission was for the generic dw-pcie
> implementation rather than a particular product integrating it.
> 
> I think in this case, we should do this completely differently:
> 
> How about putting all the new code into drivers/pci/host/pcie-designware.c
> as functions that can be used by the other drivers in absence of a chip
> specific handler?
> 
> Instead of providing a new instance of struct pcie_host_ops, maybe add
> it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
> for drivers that don't provide their own. "hisi_pcie_host_ops" currently
> provides no host_init() callback function, so you will have to change
> the hisi frontend to a provide nop-function.
> 
> For all other drivers, check if they can be changed to use your generic
> implementation and remove their private callbacks if possible.
> 
> I think the MSI implementation should be split out into a separate file
> though, as not everyone uses this.

I'm not sure I understand what you're proposing, Arnd, so let me
ramble and you can direct me back on course.

Currently drivers/pci/host/pcie-designware.c is not usable by itself;
it doesn't register a platform_driver.

There's hardly any code in Joao's patches; it looks like they add a
minimal wrapper around the functionality in pcie-designware.c and
register it as a platform_driver.

Are you suggesting that we should just add that functionality directly
in pcie-designware.c so that file could both be a minimal driver with
the functionality of Joao's patches, *and* continue to provide the
shared code used by all the existing DesignWare-based drivers?  Maybe
the platform_driver registration part could be controlled by its own
separate Kconfig option.

For example, he could make dw_pcie_link_up() look like:

  int dw_pcie_link_up(struct pcie_port *pp)
  {
    u32 val;

    if (pp->ops->link_up)
      return pp->ops->link_up(pp);

    val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
    return val & PCIE_PHY_DEBUG_R1_LINK_UP;
  }

That seems like it would make sense to me.  It would resolve the
filename question, since there wouldn't be a new file.  And if this is
merely a driver for the generic DesignWare core without any
extensions, I'm happy with some sort of "dw"-based driver name and
compatibility string.

Bjorn

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Arnd Bergmann]

On Friday 05 February 2016 17:32:48 Bjorn Helgaas wrote:
> On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote:
> > On Friday 05 February 2016 10:44:29 Joao Pinto wrote:

> > I think in this case, we should do this completely differently:
> > 
> > How about putting all the new code into drivers/pci/host/pcie-designware.c
> > as functions that can be used by the other drivers in absence of a chip
> > specific handler?
> > 
> > Instead of providing a new instance of struct pcie_host_ops, maybe add
> > it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
> > for drivers that don't provide their own. "hisi_pcie_host_ops" currently
> > provides no host_init() callback function, so you will have to change
> > the hisi frontend to a provide nop-function.
> > 
> > For all other drivers, check if they can be changed to use your generic
> > implementation and remove their private callbacks if possible.
> > 
> > I think the MSI implementation should be split out into a separate file
> > though, as not everyone uses this.
> 
> I'm not sure I understand what you're proposing, Arnd, so let me
> ramble and you can direct me back on course.
> 
> Currently drivers/pci/host/pcie-designware.c is not usable by itself;
> it doesn't register a platform_driver.
> 
> There's hardly any code in Joao's patches; it looks like they add a
> minimal wrapper around the functionality in pcie-designware.c and
> register it as a platform_driver.
> 
> Are you suggesting that we should just add that functionality directly
> in pcie-designware.c so that file could both be a minimal driver with
> the functionality of Joao's patches, *and* continue to provide the
> shared code used by all the existing DesignWare-based drivers?  Maybe
> the platform_driver registration part could be controlled by its own
> separate Kconfig option.

Either way is fine, we just have to be a little careful about the
initialization ordering.

> For example, he could make dw_pcie_link_up() look like:
> 
>   int dw_pcie_link_up(struct pcie_port *pp)
>   {
>     u32 val;
> 
>     if (pp->ops->link_up)
>       return pp->ops->link_up(pp);
> 
>     val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
>     return val & PCIE_PHY_DEBUG_R1_LINK_UP;
>   }

This is definitely good (after checking that all existing drivers
either work with the generic version, or provide their own callbacks
already).

> That seems like it would make sense to me.  It would resolve the
> filename question, since there wouldn't be a new file.  And if this is
> merely a driver for the generic DesignWare core without any
> extensions, I'm happy with some sort of "dw"-based driver name and
> compatibility string.

The important part I think is that the new driver should not require
and code that is seen as soc-specific: If it works with any implementation
of pci-dw rather than a specific system, the driver should know how
to do the right thing.

It may be helpful to move the actual matching on the compatible string
and calling of the generic probe function into another module, if we
are going forward with loadable PCI host drivers as posted by 
Paul Gortmaker today. Otherwise we end up with a device being bound
to the generic driver when a more specific one exists and both
are loadable modules, because the generic driver is always loaded
first.  As long as both drivers are built-in, it works fine because
we first look for a driver matching the most specific compatible string.

	Arnd

Re: [PATCH v8 2/2] add new platform driver for PCI RC

[Joao Pinto]

Hi Bjorn and Arnd,

I agree with you in adding the platform register mechanism to the
pcie-designware core driver. This feature is excellent for who wants to quickly
have a taste of a pcie-designware platform drive for their Synopsys PCIe RC
controller.

Do you have some example for me to check in order to follow the same logic.
Adding something like this in pcie-designware:

#ifdef CONFIG_PCIE_DW_PLAT
<add compatibility and platform register code>
#endif

would be acceptable? maybe the platform tweeks should be external makeing it
cleaner like Arnd suggested. An example to follow would be nice.

Thanks,
Joao

On 2/8/2016 12:31 PM, Arnd Bergmann wrote:
> On Friday 05 February 2016 17:32:48 Bjorn Helgaas wrote:
>> On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote:
>>> On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
> 
>>> I think in this case, we should do this completely differently:
>>>
>>> How about putting all the new code into drivers/pci/host/pcie-designware.c
>>> as functions that can be used by the other drivers in absence of a chip
>>> specific handler?
>>>
>>> Instead of providing a new instance of struct pcie_host_ops, maybe add
>>> it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
>>> for drivers that don't provide their own. "hisi_pcie_host_ops" currently
>>> provides no host_init() callback function, so you will have to change
>>> the hisi frontend to a provide nop-function.
>>>
>>> For all other drivers, check if they can be changed to use your generic
>>> implementation and remove their private callbacks if possible.
>>>
>>> I think the MSI implementation should be split out into a separate file
>>> though, as not everyone uses this.
>>
>> I'm not sure I understand what you're proposing, Arnd, so let me
>> ramble and you can direct me back on course.
>>
>> Currently drivers/pci/host/pcie-designware.c is not usable by itself;
>> it doesn't register a platform_driver.
>>
>> There's hardly any code in Joao's patches; it looks like they add a
>> minimal wrapper around the functionality in pcie-designware.c and
>> register it as a platform_driver.
>>
>> Are you suggesting that we should just add that functionality directly
>> in pcie-designware.c so that file could both be a minimal driver with
>> the functionality of Joao's patches, *and* continue to provide the
>> shared code used by all the existing DesignWare-based drivers?  Maybe
>> the platform_driver registration part could be controlled by its own
>> separate Kconfig option.
> 
> Either way is fine, we just have to be a little careful about the
> initialization ordering.
> 
>> For example, he could make dw_pcie_link_up() look like:
>>
>>   int dw_pcie_link_up(struct pcie_port *pp)
>>   {
>>     u32 val;
>>
>>     if (pp->ops->link_up)
>>       return pp->ops->link_up(pp);
>>
>>     val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
>>     return val & PCIE_PHY_DEBUG_R1_LINK_UP;
>>   }
> 
> This is definitely good (after checking that all existing drivers
> either work with the generic version, or provide their own callbacks
> already).
> 
>> That seems like it would make sense to me.  It would resolve the
>> filename question, since there wouldn't be a new file.  And if this is
>> merely a driver for the generic DesignWare core without any
>> extensions, I'm happy with some sort of "dw"-based driver name and
>> compatibility string.
> 
> The important part I think is that the new driver should not require
> and code that is seen as soc-specific: If it works with any implementation
> of pci-dw rather than a specific system, the driver should know how
> to do the right thing.
> 
> It may be helpful to move the actual matching on the compatible string
> and calling of the generic probe function into another module, if we
> are going forward with loadable PCI host drivers as posted by 
> Paul Gortmaker today. Otherwise we end up with a device being bound
> to the generic driver when a more specific one exists and both
> are loadable modules, because the generic driver is always loaded
> first.  As long as both drivers are built-in, it works fine because
> we first look for a driver matching the most specific compatible string.
> 
> 	Arnd
>