* [PATCH v8 0/2] adding PCI support to AXS10x @ 2016-02-04 15:52 Joao Pinto 2016-02-04 15:52 ` [PATCH v8 1/2] PCI support added to ARC Joao Pinto 2016-02-04 15:52 ` [PATCH v8 2/2] add new platform driver for PCI RC Joao Pinto 0 siblings, 2 replies; 14+ messages in thread From: Joao Pinto @ 2016-02-04 15:52 UTC (permalink / raw) To: Vineet.Gupta1, helgaas, arnd Cc: linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v8 1/2] PCI support added to ARC 2016-02-04 15:52 [PATCH v8 0/2] adding PCI support to AXS10x Joao Pinto @ 2016-02-04 15:52 ` Joao Pinto 2016-02-04 15:52 ` [PATCH v8 2/2] add new platform driver for PCI RC Joao Pinto 1 sibling, 0 replies; 14+ messages in thread From: Joao Pinto @ 2016-02-04 15:52 UTC (permalink / raw) To: Vineet.Gupta1, helgaas, arnd Cc: linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-04 15:52 [PATCH v8 0/2] adding PCI support to AXS10x Joao Pinto 2016-02-04 15:52 ` [PATCH v8 1/2] PCI support added to ARC Joao Pinto @ 2016-02-04 15:52 ` Joao Pinto 2016-02-04 18:19 ` Bjorn Helgaas 1 sibling, 1 reply; 14+ messages in thread From: Joao Pinto @ 2016-02-04 15:52 UTC (permalink / raw) To: Vineet.Gupta1, helgaas, arnd Cc: linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-04 15:52 ` [PATCH v8 2/2] add new platform driver for PCI RC Joao Pinto @ 2016-02-04 18:19 ` Bjorn Helgaas 2016-02-04 18:31 ` Joao Pinto 0 siblings, 1 reply; 14+ messages in thread From: Bjorn Helgaas @ 2016-02-04 18:19 UTC (permalink / raw) To: Joao Pinto Cc: Vineet.Gupta1, arnd, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-04 18:19 ` Bjorn Helgaas @ 2016-02-04 18:31 ` Joao Pinto 2016-02-04 23:43 ` Bjorn Helgaas 0 siblings, 1 reply; 14+ messages in thread From: Joao Pinto @ 2016-02-04 18:31 UTC (permalink / raw) To: Bjorn Helgaas, Joao Pinto Cc: Vineet.Gupta1, arnd, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-04 18:31 ` Joao Pinto @ 2016-02-04 23:43 ` Bjorn Helgaas 2016-02-05 10:44 ` Joao Pinto 0 siblings, 1 reply; 14+ messages in thread From: Bjorn Helgaas @ 2016-02-04 23:43 UTC (permalink / raw) To: Joao Pinto Cc: Vineet.Gupta1, arnd, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-04 23:43 ` Bjorn Helgaas @ 2016-02-05 10:44 ` Joao Pinto 2016-02-05 14:39 ` Arnd Bergmann 0 siblings, 1 reply; 14+ messages in thread From: Joao Pinto @ 2016-02-05 10:44 UTC (permalink / raw) To: Bjorn Helgaas, Joao Pinto Cc: Vineet.Gupta1, arnd, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-05 10:44 ` Joao Pinto @ 2016-02-05 14:39 ` Arnd Bergmann 2016-02-05 14:51 ` Joao Pinto 2016-02-05 23:32 ` Bjorn Helgaas 0 siblings, 2 replies; 14+ messages in thread From: Arnd Bergmann @ 2016-02-05 14:39 UTC (permalink / raw) To: Joao Pinto Cc: Bjorn Helgaas, Vineet.Gupta1, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-05 14:39 ` Arnd Bergmann @ 2016-02-05 14:51 ` Joao Pinto 2016-02-05 15:43 ` Arnd Bergmann 2016-02-05 23:32 ` Bjorn Helgaas 1 sibling, 1 reply; 14+ messages in thread From: Joao Pinto @ 2016-02-05 14:51 UTC (permalink / raw) To: Arnd Bergmann, Joao Pinto Cc: Bjorn Helgaas, Vineet.Gupta1, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-05 14:51 ` Joao Pinto @ 2016-02-05 15:43 ` Arnd Bergmann 2016-02-05 15:50 ` Joao Pinto 0 siblings, 1 reply; 14+ messages in thread From: Arnd Bergmann @ 2016-02-05 15:43 UTC (permalink / raw) To: Joao Pinto Cc: Bjorn Helgaas, Vineet.Gupta1, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-05 15:43 ` Arnd Bergmann @ 2016-02-05 15:50 ` Joao Pinto 0 siblings, 0 replies; 14+ messages in thread From: Joao Pinto @ 2016-02-05 15:50 UTC (permalink / raw) To: Arnd Bergmann, Joao Pinto, Bjorn Helgaas Cc: Vineet.Gupta1, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-05 14:39 ` Arnd Bergmann 2016-02-05 14:51 ` Joao Pinto @ 2016-02-05 23:32 ` Bjorn Helgaas 2016-02-08 12:31 ` Arnd Bergmann 1 sibling, 1 reply; 14+ messages in thread From: Bjorn Helgaas @ 2016-02-05 23:32 UTC (permalink / raw) To: Arnd Bergmann Cc: Joao Pinto, Vineet.Gupta1, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-05 23:32 ` Bjorn Helgaas @ 2016-02-08 12:31 ` Arnd Bergmann 2016-02-08 12:52 ` Joao Pinto 0 siblings, 1 reply; 14+ messages in thread From: Arnd Bergmann @ 2016-02-08 12:31 UTC (permalink / raw) To: Bjorn Helgaas Cc: Joao Pinto, Vineet.Gupta1, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v8 2/2] add new platform driver for PCI RC 2016-02-08 12:31 ` Arnd Bergmann @ 2016-02-08 12:52 ` Joao Pinto 0 siblings, 0 replies; 14+ messages in thread From: Joao Pinto @ 2016-02-08 12:52 UTC (permalink / raw) To: Arnd Bergmann, Bjorn Helgaas Cc: Joao Pinto, Vineet.Gupta1, linux-pci, linux-kernel, linux-snps-arc, CARLOS.PALMINHA, Alexey.Brodkin, robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-02-08 12:58 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-04 15:52 [PATCH v8 0/2] adding PCI support to AXS10x Joao Pinto 2016-02-04 15:52 ` [PATCH v8 1/2] PCI support added to ARC Joao Pinto 2016-02-04 15:52 ` [PATCH v8 2/2] add new platform driver for PCI RC Joao Pinto 2016-02-04 18:19 ` Bjorn Helgaas 2016-02-04 18:31 ` Joao Pinto 2016-02-04 23:43 ` Bjorn Helgaas 2016-02-05 10:44 ` Joao Pinto 2016-02-05 14:39 ` Arnd Bergmann 2016-02-05 14:51 ` Joao Pinto 2016-02-05 15:43 ` Arnd Bergmann 2016-02-05 15:50 ` Joao Pinto 2016-02-05 23:32 ` Bjorn Helgaas 2016-02-08 12:31 ` Arnd Bergmann 2016-02-08 12:52 ` Joao Pinto
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 NNTP newsgroup(s).