[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

This set of two patches add support for the PCIe controller found in
the Marvell Armada 7K/8K ARM processors. The first patch adds the
Device Tree binding documentation, the second patch the driver itself.

The driver is really simple, since the PCIe controller is based on the
Designware design, and only a little bit of vendor-specific glue is
needed.

Changes since v1:

 - Change the DT binding to take only one clock, since in fact all
   PCIe lane clocks are child of the PCIe main clock, so there is no
   need for the PCIe controllers to take a reference to both.

 - Use msleep(1) instead of udelay(1), as suggested by Arnd Bergmann.

 - Add a comment to explain why we simply discard the interrupt, as
   requested by Andrew Lunn.

 - Properly handle EPROBE_DEFER for devm_clk_get(), as suggested by
   Andrew Lunn.

Rob: due to the change to the DT binding (even though minimal), I have
not kept your Acked-by. So if it still stands despite the DT binding
change, could you send it again? Thanks!

Thomas

Thomas Petazzoni (2):
  dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe
    controller
  pci: host: new driver for Marvell Armada 7K/8K PCIe controller

 .../devicetree/bindings/pci/pci-armada8k.txt       |  38 +++
 drivers/pci/host/Kconfig                           |  11 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-armada8k.c                   | 261 +++++++++++++++++++++
 4 files changed, 311 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt
 create mode 100644 drivers/pci/host/pcie-armada8k.c

-- 
2.6.4

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

This set of two patches add support for the PCIe controller found in
the Marvell Armada 7K/8K ARM processors. The first patch adds the
Device Tree binding documentation, the second patch the driver itself.

The driver is really simple, since the PCIe controller is based on the
Designware design, and only a little bit of vendor-specific glue is
needed.

Changes since v1:

 - Change the DT binding to take only one clock, since in fact all
   PCIe lane clocks are child of the PCIe main clock, so there is no
   need for the PCIe controllers to take a reference to both.

 - Use msleep(1) instead of udelay(1), as suggested by Arnd Bergmann.

 - Add a comment to explain why we simply discard the interrupt, as
   requested by Andrew Lunn.

 - Properly handle EPROBE_DEFER for devm_clk_get(), as suggested by
   Andrew Lunn.

Rob: due to the change to the DT binding (even though minimal), I have
not kept your Acked-by. So if it still stands despite the DT binding
change, could you send it again? Thanks!

Thomas

Thomas Petazzoni (2):
  dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe
    controller
  pci: host: new driver for Marvell Armada 7K/8K PCIe controller

 .../devicetree/bindings/pci/pci-armada8k.txt       |  38 +++
 drivers/pci/host/Kconfig                           |  11 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-armada8k.c                   | 261 +++++++++++++++++++++
 4 files changed, 311 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt
 create mode 100644 drivers/pci/host/pcie-armada8k.c

-- 
2.6.4

[PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

This commit adds the Device Tree binding documentation that allows to
describe the PCIe controller found in Marvell Armada 7K/8K SoCs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../devicetree/bindings/pci/pci-armada8k.txt       | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
new file mode 100644
index 0000000..598533a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
@@ -0,0 +1,38 @@
+* Marvell Armada 7K/8K PCIe interface
+
+This PCIe host controller is based on the Synopsis Designware PCIe IP
+and thus inherits all the common properties defined in designware-pcie.txt.
+
+Required properties:
+- compatible: "marvell,armada8k-pcie"
+- reg: must contain two register regions
+   - the control register region
+   - the config space region
+- reg-names:
+   - "ctrl" for the control register region
+   - "config" for the config space region
+- interrupts: Interrupt specifier for the PCIe controler
+- clocks: reference to the PCIe controller clock
+
+Example:
+
+	pcie@f2600000 {
+		compatible = "marvell,armada8k-pcie", "snps,dw-pcie";
+		reg = <0 0xf2600000 0 0x10000>, <0 0xf6f00000 0 0x80000>;
+		reg-names = "ctrl", "config";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+		device_type = "pci";
+		dma-coherent;
+
+		bus-range = <0 0xff>;
+		ranges = <0x81000000 0 0xf9000000 0  0xf9000000 0 0x10000	/* downstream I/O */
+			  0x82000000 0 0xf6000000 0  0xf6000000 0 0xf00000>;	/* non-prefetchable memory */
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+		num-lanes = <1>;
+		clocks = <&cpm_syscon0 1 13>;
+		status = "disabled";
+	};
-- 
2.6.4

[PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

This commit adds the Device Tree binding documentation that allows to
describe the PCIe controller found in Marvell Armada 7K/8K SoCs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../devicetree/bindings/pci/pci-armada8k.txt       | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
new file mode 100644
index 0000000..598533a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
@@ -0,0 +1,38 @@
+* Marvell Armada 7K/8K PCIe interface
+
+This PCIe host controller is based on the Synopsis Designware PCIe IP
+and thus inherits all the common properties defined in designware-pcie.txt.
+
+Required properties:
+- compatible: "marvell,armada8k-pcie"
+- reg: must contain two register regions
+   - the control register region
+   - the config space region
+- reg-names:
+   - "ctrl" for the control register region
+   - "config" for the config space region
+- interrupts: Interrupt specifier for the PCIe controler
+- clocks: reference to the PCIe controller clock
+
+Example:
+
+	pcie at f2600000 {
+		compatible = "marvell,armada8k-pcie", "snps,dw-pcie";
+		reg = <0 0xf2600000 0 0x10000>, <0 0xf6f00000 0 0x80000>;
+		reg-names = "ctrl", "config";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		#interrupt-cells = <1>;
+		device_type = "pci";
+		dma-coherent;
+
+		bus-range = <0 0xff>;
+		ranges = <0x81000000 0 0xf9000000 0  0xf9000000 0 0x10000	/* downstream I/O */
+			  0x82000000 0 0xf6000000 0  0xf6000000 0 0xf00000>;	/* non-prefetchable memory */
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic 0 GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
+		num-lanes = <1>;
+		clocks = <&cpm_syscon0 1 13>;
+		status = "disabled";
+	};
-- 
2.6.4

Re: [PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Rob Herring]

On Thu, Apr 14, 2016 at 05:36:16PM +0200, Thomas Petazzoni wrote:
> This commit adds the Device Tree binding documentation that allows to
> describe the PCIe controller found in Marvell Armada 7K/8K SoCs.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/pci/pci-armada8k.txt       | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt

I already acked the last version, did something change?

Acked-by: Rob Herring <rob@kernel.org>

[PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Rob Herring]

On Thu, Apr 14, 2016 at 05:36:16PM +0200, Thomas Petazzoni wrote:
> This commit adds the Device Tree binding documentation that allows to
> describe the PCIe controller found in Marvell Armada 7K/8K SoCs.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/pci/pci-armada8k.txt       | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt

I already acked the last version, did something change?

Acked-by: Rob Herring <rob@kernel.org>

Re: [PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Thu, 14 Apr 2016 12:40:46 -0500, Rob Herring wrote:

> I already acked the last version, did something change?

>From the cover letter:

"""
Changes since v1:

 - Change the DT binding to take only one clock, since in fact all
   PCIe lane clocks are child of the PCIe main clock, so there is no
   need for the PCIe controllers to take a reference to both.

[...]

Rob: due to the change to the DT binding (even though minimal), I have
not kept your Acked-by. So if it still stands despite the DT binding
change, could you send it again? Thanks!
"""

> Acked-by: Rob Herring <rob@kernel.org>

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Thu, 14 Apr 2016 12:40:46 -0500, Rob Herring wrote:

> I already acked the last version, did something change?

>From the cover letter:

"""
Changes since v1:

 - Change the DT binding to take only one clock, since in fact all
   PCIe lane clocks are child of the PCIe main clock, so there is no
   need for the PCIe controllers to take a reference to both.

[...]

Rob: due to the change to the DT binding (even though minimal), I have
not kept your Acked-by. So if it still stands despite the DT binding
change, could you send it again? Thanks!
"""

> Acked-by: Rob Herring <rob@kernel.org>

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Rob Herring]

On Thu, Apr 14, 2016 at 3:18 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Thu, 14 Apr 2016 12:40:46 -0500, Rob Herring wrote:
>
>> I already acked the last version, did something change?
>
> From the cover letter:
>
> """
> Changes since v1:
>
>  - Change the DT binding to take only one clock, since in fact all
>    PCIe lane clocks are child of the PCIe main clock, so there is no
>    need for the PCIe controllers to take a reference to both.

Found it after I replied...

> [...]
>
> Rob: due to the change to the DT binding (even though minimal), I have
> not kept your Acked-by. So if it still stands despite the DT binding
> change, could you send it again? Thanks!
> """

I did, here:

>> Acked-by: Rob Herring <rob@kernel.org>
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH v2 1/2] dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe controller

[Rob Herring]

On Thu, Apr 14, 2016 at 3:18 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Thu, 14 Apr 2016 12:40:46 -0500, Rob Herring wrote:
>
>> I already acked the last version, did something change?
>
> From the cover letter:
>
> """
> Changes since v1:
>
>  - Change the DT binding to take only one clock, since in fact all
>    PCIe lane clocks are child of the PCIe main clock, so there is no
>    need for the PCIe controllers to take a reference to both.

Found it after I replied...

> [...]
>
> Rob: due to the change to the DT binding (even though minimal), I have
> not kept your Acked-by. So if it still stands despite the DT binding
> change, could you send it again? Thanks!
> """

I did, here:

>> Acked-by: Rob Herring <rob@kernel.org>
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

[PATCH v2 2/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

The Marvell Armada 7K/8K SoCs integrate a PCIe controller from
Synopsys. This commit adds a new driver that provides the small glue
needed to use the existing Designware driver to make it work on
Marvell Armada 7K/8K SoCs.

The MSI support will be enabled at a later point.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/Kconfig         |  11 ++
 drivers/pci/host/Makefile        |   1 +
 drivers/pci/host/pcie-armada8k.c | 261 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/pci/host/pcie-armada8k.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 7a0780d..a3b6f24 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -231,4 +231,15 @@ config PCI_HOST_THUNDER_ECAM
 	help
 	  Say Y here if you want ECAM support for CN88XX-Pass-1.x Cavium Thunder SoCs.
 
+config PCIE_ARMADA_8K
+	bool "Marvell Armada-8K PCIe controller"
+	depends on ARCH_MVEBU
+	select PCIE_DW
+	select PCIEPORTBUS
+	help
+	  Say Y here if you want to enable PCIe controller support on
+	  Armada-8K SoCs. The PCIe controller on Armada-8K is based on
+	  Designware hardware and therefore the driver re-uses the
+	  Designware core functions to implement the driver.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index d85b5fa..a6f85e3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
 obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
+obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
diff --git a/drivers/pci/host/pcie-armada8k.c b/drivers/pci/host/pcie-armada8k.c
new file mode 100644
index 0000000..7569302
--- /dev/null
+++ b/drivers/pci/host/pcie-armada8k.c
@@ -0,0 +1,261 @@
+/*
+ * PCIe host controller driver for Marvell Armada-8K SoCs
+ *
+ * Armada-8K PCIe Glue Layer Source Code
+ *
+ * Copyright (C) 2016 Marvell Technology Group Ltd.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#define pr_fmt(fmt) "armada-8k-pcie: " fmt
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/of_pci.h>
+#include <linux/of_irq.h>
+
+#include "pcie-designware.h"
+
+struct armada8k_pcie {
+	void __iomem *base;
+	struct clk *clk;
+	struct pcie_port pp;
+};
+
+#define PCIE_VENDOR_REGS_OFFSET		0x8000
+
+#define PCIE_GLOBAL_CONTROL_REG		0x0
+#define PCIE_APP_LTSSM_EN		BIT(2)
+#define PCIE_DEVICE_TYPE_SHIFT		4
+#define PCIE_DEVICE_TYPE_MASK		0xF
+#define PCIE_DEVICE_TYPE_EP		0x0 /* Endpoint */
+#define PCIE_DEVICE_TYPE_LEP		0x1 /* Legacy endpoint */
+#define PCIE_DEVICE_TYPE_RC		0x4 /* Root complex */
+
+#define PCIE_GLOBAL_STATUS_REG		0x8
+#define PCIE_GLB_STS_RDLH_LINK_UP	BIT(1)
+#define PCIE_GLB_STS_PHY_LINK_UP	BIT(9)
+
+#define PCIE_GLOBAL_INT_CAUSE1_REG	0x1C
+#define PCIE_GLOBAL_INT_MASK1_REG	0x20
+#define PCIE_INT_A_ASSERT_MASK		BIT(9)
+#define PCIE_INT_B_ASSERT_MASK		BIT(10)
+#define PCIE_INT_C_ASSERT_MASK		BIT(11)
+#define PCIE_INT_D_ASSERT_MASK		BIT(12)
+
+#define PCIE_ARCACHE_TRC_REG		0x50
+#define PCIE_AWCACHE_TRC_REG		0x54
+#define PCIE_ARUSER_REG			0x5C
+#define PCIE_AWUSER_REG			0x60
+/*
+ * AR/AW Cache defauls: Normal memory, Write-Back, Read / Write
+ * allocate
+ */
+#define ARCACHE_DEFAULT_VALUE		0x3511
+#define AWCACHE_DEFAULT_VALUE		0x5311
+
+#define DOMAIN_OUTER_SHAREABLE		0x2
+#define AX_USER_DOMAIN_MASK		0x3
+#define AX_USER_DOMAIN_SHIFT		4
+
+
+
+#define to_armada8k_pcie(x)	container_of(x, struct armada8k_pcie, pp)
+
+static int armada8k_pcie_link_up(struct pcie_port *pp)
+{
+	u32 reg;
+	struct armada8k_pcie *pcie = to_armada8k_pcie(pp);
+	u32 mask = PCIE_GLB_STS_RDLH_LINK_UP | PCIE_GLB_STS_PHY_LINK_UP;
+
+	reg = readl(pcie->base + PCIE_GLOBAL_STATUS_REG);
+
+	if ((reg & mask) == mask)
+		return 1;
+
+	pr_debug("No link detected (Global-Status: 0x%08x).\n", reg);
+	return 0;
+}
+
+static void armada8k_pcie_host_init(struct pcie_port *pp)
+{
+	struct armada8k_pcie *pcie = to_armada8k_pcie(pp);
+	void __iomem *base = pcie->base;
+	int timeout = 1000;
+	u32 reg;
+
+	if (!armada8k_pcie_link_up(pp)) {
+		/* Disable LTSSM state machine to enable configuration */
+		reg = readl(base + PCIE_GLOBAL_CONTROL_REG);
+		reg &= ~(PCIE_APP_LTSSM_EN);
+		writel(reg, base + PCIE_GLOBAL_CONTROL_REG);
+	}
+
+	/* Set the device to root complex mode */
+	reg = readl(base + PCIE_GLOBAL_CONTROL_REG);
+	reg &= ~(PCIE_DEVICE_TYPE_MASK << PCIE_DEVICE_TYPE_SHIFT);
+	reg |= PCIE_DEVICE_TYPE_RC << PCIE_DEVICE_TYPE_SHIFT;
+	writel(reg, base + PCIE_GLOBAL_CONTROL_REG);
+
+	/* Set the PCIe master AxCache attributes */
+	writel(ARCACHE_DEFAULT_VALUE, base + PCIE_ARCACHE_TRC_REG);
+	writel(AWCACHE_DEFAULT_VALUE, base + PCIE_AWCACHE_TRC_REG);
+
+	/* Set the PCIe master AxDomain attributes */
+	reg = readl(base + PCIE_ARUSER_REG);
+	reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
+	reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
+	writel(reg, base + PCIE_ARUSER_REG);
+
+	reg = readl(base + PCIE_AWUSER_REG);
+	reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
+	reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
+	writel(reg, base + PCIE_AWUSER_REG);
+
+	dw_pcie_setup_rc(pp);
+
+	/* Enable INT A-D interrupts */
+	reg = readl(base + PCIE_GLOBAL_INT_MASK1_REG);
+	reg |= PCIE_INT_A_ASSERT_MASK | PCIE_INT_B_ASSERT_MASK |
+	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
+	writel(reg, base + PCIE_GLOBAL_INT_MASK1_REG);
+
+	if (!armada8k_pcie_link_up(pp)) {
+		/* Configuration done. Start LTSSM */
+		reg = readl(base + PCIE_GLOBAL_CONTROL_REG);
+		reg |= PCIE_APP_LTSSM_EN;
+		writel(reg, base + PCIE_GLOBAL_CONTROL_REG);
+	}
+
+	/* Wait until the link becomes active again */
+	while (timeout) {
+		if (armada8k_pcie_link_up(pp))
+			break;
+		msleep(1);
+		timeout--;
+	}
+
+	if (timeout == 0)
+		dev_err(pp->dev, "Link not up after reconfiguration\n");
+}
+
+static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+	struct armada8k_pcie *pcie = to_armada8k_pcie(pp);
+	void __iomem *base = pcie->base;
+	u32 val;
+
+	/*
+	 * Interrupts are directly handled by the device driver of the
+	 * PCI device. However, there are also latched into the PCIe
+	 * controller, so we simply discard them.
+	 */
+	val = readl(base + PCIE_GLOBAL_INT_CAUSE1_REG);
+	writel(val, base + PCIE_GLOBAL_INT_CAUSE1_REG);
+
+	return IRQ_HANDLED;
+}
+
+static struct pcie_host_ops armada8k_pcie_host_ops = {
+	.link_up = armada8k_pcie_link_up,
+	.host_init = armada8k_pcie_host_init,
+};
+
+static int armada8k_pcie_probe(struct platform_device *pdev)
+{
+	struct armada8k_pcie *pcie;
+	struct pcie_port *pp;
+	struct device *dev = &pdev->dev;
+	struct resource *base;
+	int ret;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pcie->clk))
+		return PTR_ERR(pcie->clk);
+
+	clk_prepare_enable(pcie->clk);
+
+	pp = &pcie->pp;
+
+	platform_set_drvdata(pdev, pcie);
+
+	/* Get the dw-pcie unit configuration/control registers base. */
+	base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl");
+	pp->dbi_base = devm_ioremap_resource(dev, base);
+	if (IS_ERR(pp->dbi_base)) {
+		dev_err(dev, "couldn't remap regs base %p\n", base);
+		ret = PTR_ERR(pp->dbi_base);
+		goto fail;
+	}
+
+	pcie->base = pp->dbi_base + PCIE_VENDOR_REGS_OFFSET;
+
+	pp->dev = dev;
+	pp->root_bus_nr = -1;
+	pp->ops = &armada8k_pcie_host_ops;
+
+	pp->irq = platform_get_irq(pdev, 0);
+	if (!pp->irq) {
+		dev_err(dev, "failed to get irq for port\n");
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	ret = devm_request_irq(dev, pp->irq, armada8k_pcie_irq_handler,
+				IRQF_SHARED, "armada8k-pcie", pp);
+	if (ret) {
+		dev_err(dev, "failed to request irq %d\n", pp->irq);
+		goto fail;
+	}
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host: %d\n", ret);
+		goto fail;
+	}
+
+	return 0;
+
+fail:
+	if (!IS_ERR(pcie->clk))
+		clk_disable_unprepare(pcie->clk);
+
+	return ret;
+}
+
+static const struct of_device_id armada8k_pcie_of_match[] = {
+	{ .compatible = "marvell,armada8k-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, armada8k_pcie_of_match);
+
+static struct platform_driver armada8k_pcie_driver = {
+	.probe		= armada8k_pcie_probe,
+	.driver = {
+		.name	= "armada8k-pcie",
+		.of_match_table = of_match_ptr(armada8k_pcie_of_match),
+	},
+};
+
+module_platform_driver(armada8k_pcie_driver);
+
+MODULE_DESCRIPTION("Armada 8k PCIe host controller driver");
+MODULE_AUTHOR("Yehuda Yitshak <yehuday@marvell.com>");
+MODULE_AUTHOR("Shadi Ammouri <shadi@marvell.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

[PATCH v2 2/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

The Marvell Armada 7K/8K SoCs integrate a PCIe controller from
Synopsys. This commit adds a new driver that provides the small glue
needed to use the existing Designware driver to make it work on
Marvell Armada 7K/8K SoCs.

The MSI support will be enabled at a later point.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/Kconfig         |  11 ++
 drivers/pci/host/Makefile        |   1 +
 drivers/pci/host/pcie-armada8k.c | 261 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/pci/host/pcie-armada8k.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 7a0780d..a3b6f24 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -231,4 +231,15 @@ config PCI_HOST_THUNDER_ECAM
 	help
 	  Say Y here if you want ECAM support for CN88XX-Pass-1.x Cavium Thunder SoCs.
 
+config PCIE_ARMADA_8K
+	bool "Marvell Armada-8K PCIe controller"
+	depends on ARCH_MVEBU
+	select PCIE_DW
+	select PCIEPORTBUS
+	help
+	  Say Y here if you want to enable PCIe controller support on
+	  Armada-8K SoCs. The PCIe controller on Armada-8K is based on
+	  Designware hardware and therefore the driver re-uses the
+	  Designware core functions to implement the driver.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index d85b5fa..a6f85e3 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
 obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
 obj-$(CONFIG_PCI_HOST_THUNDER_ECAM) += pci-thunder-ecam.o
 obj-$(CONFIG_PCI_HOST_THUNDER_PEM) += pci-thunder-pem.o
+obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
diff --git a/drivers/pci/host/pcie-armada8k.c b/drivers/pci/host/pcie-armada8k.c
new file mode 100644
index 0000000..7569302
--- /dev/null
+++ b/drivers/pci/host/pcie-armada8k.c
@@ -0,0 +1,261 @@
+/*
+ * PCIe host controller driver for Marvell Armada-8K SoCs
+ *
+ * Armada-8K PCIe Glue Layer Source Code
+ *
+ * Copyright (C) 2016 Marvell Technology Group Ltd.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#define pr_fmt(fmt) "armada-8k-pcie: " fmt
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/of_pci.h>
+#include <linux/of_irq.h>
+
+#include "pcie-designware.h"
+
+struct armada8k_pcie {
+	void __iomem *base;
+	struct clk *clk;
+	struct pcie_port pp;
+};
+
+#define PCIE_VENDOR_REGS_OFFSET		0x8000
+
+#define PCIE_GLOBAL_CONTROL_REG		0x0
+#define PCIE_APP_LTSSM_EN		BIT(2)
+#define PCIE_DEVICE_TYPE_SHIFT		4
+#define PCIE_DEVICE_TYPE_MASK		0xF
+#define PCIE_DEVICE_TYPE_EP		0x0 /* Endpoint */
+#define PCIE_DEVICE_TYPE_LEP		0x1 /* Legacy endpoint */
+#define PCIE_DEVICE_TYPE_RC		0x4 /* Root complex */
+
+#define PCIE_GLOBAL_STATUS_REG		0x8
+#define PCIE_GLB_STS_RDLH_LINK_UP	BIT(1)
+#define PCIE_GLB_STS_PHY_LINK_UP	BIT(9)
+
+#define PCIE_GLOBAL_INT_CAUSE1_REG	0x1C
+#define PCIE_GLOBAL_INT_MASK1_REG	0x20
+#define PCIE_INT_A_ASSERT_MASK		BIT(9)
+#define PCIE_INT_B_ASSERT_MASK		BIT(10)
+#define PCIE_INT_C_ASSERT_MASK		BIT(11)
+#define PCIE_INT_D_ASSERT_MASK		BIT(12)
+
+#define PCIE_ARCACHE_TRC_REG		0x50
+#define PCIE_AWCACHE_TRC_REG		0x54
+#define PCIE_ARUSER_REG			0x5C
+#define PCIE_AWUSER_REG			0x60
+/*
+ * AR/AW Cache defauls: Normal memory, Write-Back, Read / Write
+ * allocate
+ */
+#define ARCACHE_DEFAULT_VALUE		0x3511
+#define AWCACHE_DEFAULT_VALUE		0x5311
+
+#define DOMAIN_OUTER_SHAREABLE		0x2
+#define AX_USER_DOMAIN_MASK		0x3
+#define AX_USER_DOMAIN_SHIFT		4
+
+
+
+#define to_armada8k_pcie(x)	container_of(x, struct armada8k_pcie, pp)
+
+static int armada8k_pcie_link_up(struct pcie_port *pp)
+{
+	u32 reg;
+	struct armada8k_pcie *pcie = to_armada8k_pcie(pp);
+	u32 mask = PCIE_GLB_STS_RDLH_LINK_UP | PCIE_GLB_STS_PHY_LINK_UP;
+
+	reg = readl(pcie->base + PCIE_GLOBAL_STATUS_REG);
+
+	if ((reg & mask) == mask)
+		return 1;
+
+	pr_debug("No link detected (Global-Status: 0x%08x).\n", reg);
+	return 0;
+}
+
+static void armada8k_pcie_host_init(struct pcie_port *pp)
+{
+	struct armada8k_pcie *pcie = to_armada8k_pcie(pp);
+	void __iomem *base = pcie->base;
+	int timeout = 1000;
+	u32 reg;
+
+	if (!armada8k_pcie_link_up(pp)) {
+		/* Disable LTSSM state machine to enable configuration */
+		reg = readl(base + PCIE_GLOBAL_CONTROL_REG);
+		reg &= ~(PCIE_APP_LTSSM_EN);
+		writel(reg, base + PCIE_GLOBAL_CONTROL_REG);
+	}
+
+	/* Set the device to root complex mode */
+	reg = readl(base + PCIE_GLOBAL_CONTROL_REG);
+	reg &= ~(PCIE_DEVICE_TYPE_MASK << PCIE_DEVICE_TYPE_SHIFT);
+	reg |= PCIE_DEVICE_TYPE_RC << PCIE_DEVICE_TYPE_SHIFT;
+	writel(reg, base + PCIE_GLOBAL_CONTROL_REG);
+
+	/* Set the PCIe master AxCache attributes */
+	writel(ARCACHE_DEFAULT_VALUE, base + PCIE_ARCACHE_TRC_REG);
+	writel(AWCACHE_DEFAULT_VALUE, base + PCIE_AWCACHE_TRC_REG);
+
+	/* Set the PCIe master AxDomain attributes */
+	reg = readl(base + PCIE_ARUSER_REG);
+	reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
+	reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
+	writel(reg, base + PCIE_ARUSER_REG);
+
+	reg = readl(base + PCIE_AWUSER_REG);
+	reg &= ~(AX_USER_DOMAIN_MASK << AX_USER_DOMAIN_SHIFT);
+	reg |= DOMAIN_OUTER_SHAREABLE << AX_USER_DOMAIN_SHIFT;
+	writel(reg, base + PCIE_AWUSER_REG);
+
+	dw_pcie_setup_rc(pp);
+
+	/* Enable INT A-D interrupts */
+	reg = readl(base + PCIE_GLOBAL_INT_MASK1_REG);
+	reg |= PCIE_INT_A_ASSERT_MASK | PCIE_INT_B_ASSERT_MASK |
+	       PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
+	writel(reg, base + PCIE_GLOBAL_INT_MASK1_REG);
+
+	if (!armada8k_pcie_link_up(pp)) {
+		/* Configuration done. Start LTSSM */
+		reg = readl(base + PCIE_GLOBAL_CONTROL_REG);
+		reg |= PCIE_APP_LTSSM_EN;
+		writel(reg, base + PCIE_GLOBAL_CONTROL_REG);
+	}
+
+	/* Wait until the link becomes active again */
+	while (timeout) {
+		if (armada8k_pcie_link_up(pp))
+			break;
+		msleep(1);
+		timeout--;
+	}
+
+	if (timeout == 0)
+		dev_err(pp->dev, "Link not up after reconfiguration\n");
+}
+
+static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+	struct armada8k_pcie *pcie = to_armada8k_pcie(pp);
+	void __iomem *base = pcie->base;
+	u32 val;
+
+	/*
+	 * Interrupts are directly handled by the device driver of the
+	 * PCI device. However, there are also latched into the PCIe
+	 * controller, so we simply discard them.
+	 */
+	val = readl(base + PCIE_GLOBAL_INT_CAUSE1_REG);
+	writel(val, base + PCIE_GLOBAL_INT_CAUSE1_REG);
+
+	return IRQ_HANDLED;
+}
+
+static struct pcie_host_ops armada8k_pcie_host_ops = {
+	.link_up = armada8k_pcie_link_up,
+	.host_init = armada8k_pcie_host_init,
+};
+
+static int armada8k_pcie_probe(struct platform_device *pdev)
+{
+	struct armada8k_pcie *pcie;
+	struct pcie_port *pp;
+	struct device *dev = &pdev->dev;
+	struct resource *base;
+	int ret;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pcie->clk))
+		return PTR_ERR(pcie->clk);
+
+	clk_prepare_enable(pcie->clk);
+
+	pp = &pcie->pp;
+
+	platform_set_drvdata(pdev, pcie);
+
+	/* Get the dw-pcie unit configuration/control registers base. */
+	base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl");
+	pp->dbi_base = devm_ioremap_resource(dev, base);
+	if (IS_ERR(pp->dbi_base)) {
+		dev_err(dev, "couldn't remap regs base %p\n", base);
+		ret = PTR_ERR(pp->dbi_base);
+		goto fail;
+	}
+
+	pcie->base = pp->dbi_base + PCIE_VENDOR_REGS_OFFSET;
+
+	pp->dev = dev;
+	pp->root_bus_nr = -1;
+	pp->ops = &armada8k_pcie_host_ops;
+
+	pp->irq = platform_get_irq(pdev, 0);
+	if (!pp->irq) {
+		dev_err(dev, "failed to get irq for port\n");
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	ret = devm_request_irq(dev, pp->irq, armada8k_pcie_irq_handler,
+				IRQF_SHARED, "armada8k-pcie", pp);
+	if (ret) {
+		dev_err(dev, "failed to request irq %d\n", pp->irq);
+		goto fail;
+	}
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "failed to initialize host: %d\n", ret);
+		goto fail;
+	}
+
+	return 0;
+
+fail:
+	if (!IS_ERR(pcie->clk))
+		clk_disable_unprepare(pcie->clk);
+
+	return ret;
+}
+
+static const struct of_device_id armada8k_pcie_of_match[] = {
+	{ .compatible = "marvell,armada8k-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, armada8k_pcie_of_match);
+
+static struct platform_driver armada8k_pcie_driver = {
+	.probe		= armada8k_pcie_probe,
+	.driver = {
+		.name	= "armada8k-pcie",
+		.of_match_table = of_match_ptr(armada8k_pcie_of_match),
+	},
+};
+
+module_platform_driver(armada8k_pcie_driver);
+
+MODULE_DESCRIPTION("Armada 8k PCIe host controller driver");
+MODULE_AUTHOR("Yehuda Yitshak <yehuday@marvell.com>");
+MODULE_AUTHOR("Shadi Ammouri <shadi@marvell.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.6.4

Re: [PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Thu, 14 Apr 2016 17:36:15 +0200, Thomas Petazzoni wrote:

> Thomas Petazzoni (2):
>   dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe
>     controller
>   pci: host: new driver for Marvell Armada 7K/8K PCIe controller
> 
>  .../devicetree/bindings/pci/pci-armada8k.txt       |  38 +++
>  drivers/pci/host/Kconfig                           |  11 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pcie-armada8k.c                   | 261 +++++++++++++++++++++
>  4 files changed, 311 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt
>  create mode 100644 drivers/pci/host/pcie-armada8k.c

Bjorn, any comment about this additional PCIe host driver? The
corresponding Device Tree binding has been approved by Rob Herring. The
driver itself is fairly trivial as it relies on the common Designware
logic.

DO not hesitate to let me know if any adaptation/change is needed. I'd
really like to have this merged for the 4.7 merge window.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Thu, 14 Apr 2016 17:36:15 +0200, Thomas Petazzoni wrote:

> Thomas Petazzoni (2):
>   dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe
>     controller
>   pci: host: new driver for Marvell Armada 7K/8K PCIe controller
> 
>  .../devicetree/bindings/pci/pci-armada8k.txt       |  38 +++
>  drivers/pci/host/Kconfig                           |  11 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pcie-armada8k.c                   | 261 +++++++++++++++++++++
>  4 files changed, 311 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt
>  create mode 100644 drivers/pci/host/pcie-armada8k.c

Bjorn, any comment about this additional PCIe host driver? The
corresponding Device Tree binding has been approved by Rob Herring. The
driver itself is fairly trivial as it relies on the common Designware
logic.

DO not hesitate to let me know if any adaptation/change is needed. I'd
really like to have this merged for the 4.7 merge window.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Bjorn Helgaas]

Hi Thomas,

On Mon, Apr 25, 2016 at 03:46:11PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 14 Apr 2016 17:36:15 +0200, Thomas Petazzoni wrote:
> 
> > Thomas Petazzoni (2):
> >   dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe
> >     controller
> >   pci: host: new driver for Marvell Armada 7K/8K PCIe controller
> > 
> >  .../devicetree/bindings/pci/pci-armada8k.txt       |  38 +++
> >  drivers/pci/host/Kconfig                           |  11 +
> >  drivers/pci/host/Makefile                          |   1 +
> >  drivers/pci/host/pcie-armada8k.c                   | 261 +++++++++++++++++++++
> >  4 files changed, 311 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt
> >  create mode 100644 drivers/pci/host/pcie-armada8k.c
> 
> Bjorn, any comment about this additional PCIe host driver? The
> corresponding Device Tree binding has been approved by Rob Herring. The
> driver itself is fairly trivial as it relies on the common Designware
> logic.

It *is* pretty trivial.  The reason I'm hesitating is because we have
all these DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls,
dw_plat, hisi, qcom, spear13xx, and now armada8k), and they're
*mostly* similar, but they differ in minor, annoying ways.  This is
becoming a significant maintenance burden for me, and I'd like to
figure out how to mitigate that.

For now, I don't have any great ideas except that it would be nice to
remove needless variations.  The following is a typical code
structure, but it's not universally followed:

  XXX_pcie_probe
    XXX_add_pcie_port
      dw_pcie_host_init
        XXX_pcie_host_init
	  dw_pcie_setup_rc
	  XXX_pcie_establish_link

There's a hodge-podge of ways to get related resources (clocks and
PHYs) and initialize them.  IRQ setup is not really consistent across
all the drivers.  It's sort of disappointing that most of these
drivers have a "dbi_base" resource, but they use different DT property
names for it.

The armada8k driver doesn't have a DRV_add_pcie_port() function or a
DRV_pcie_establish_link() function, and it has its own "wait for link"
timeout loop instead of using dw_pcie_wait_for_link().

How about if you just shuffle those bits around into
an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
we'll call that good for now?

Bjorn

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Bjorn Helgaas]

Hi Thomas,

On Mon, Apr 25, 2016 at 03:46:11PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 14 Apr 2016 17:36:15 +0200, Thomas Petazzoni wrote:
> 
> > Thomas Petazzoni (2):
> >   dt-bindings: pci: add DT binding for Marvell Armada 7K/8K PCIe
> >     controller
> >   pci: host: new driver for Marvell Armada 7K/8K PCIe controller
> > 
> >  .../devicetree/bindings/pci/pci-armada8k.txt       |  38 +++
> >  drivers/pci/host/Kconfig                           |  11 +
> >  drivers/pci/host/Makefile                          |   1 +
> >  drivers/pci/host/pcie-armada8k.c                   | 261 +++++++++++++++++++++
> >  4 files changed, 311 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt
> >  create mode 100644 drivers/pci/host/pcie-armada8k.c
> 
> Bjorn, any comment about this additional PCIe host driver? The
> corresponding Device Tree binding has been approved by Rob Herring. The
> driver itself is fairly trivial as it relies on the common Designware
> logic.

It *is* pretty trivial.  The reason I'm hesitating is because we have
all these DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls,
dw_plat, hisi, qcom, spear13xx, and now armada8k), and they're
*mostly* similar, but they differ in minor, annoying ways.  This is
becoming a significant maintenance burden for me, and I'd like to
figure out how to mitigate that.

For now, I don't have any great ideas except that it would be nice to
remove needless variations.  The following is a typical code
structure, but it's not universally followed:

  XXX_pcie_probe
    XXX_add_pcie_port
      dw_pcie_host_init
        XXX_pcie_host_init
	  dw_pcie_setup_rc
	  XXX_pcie_establish_link

There's a hodge-podge of ways to get related resources (clocks and
PHYs) and initialize them.  IRQ setup is not really consistent across
all the drivers.  It's sort of disappointing that most of these
drivers have a "dbi_base" resource, but they use different DT property
names for it.

The armada8k driver doesn't have a DRV_add_pcie_port() function or a
DRV_pcie_establish_link() function, and it has its own "wait for link"
timeout loop instead of using dw_pcie_wait_for_link().

How about if you just shuffle those bits around into
an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
we'll call that good for now?

Bjorn

Re: [PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Mon, 25 Apr 2016 12:28:46 -0500, Bjorn Helgaas wrote:

> It *is* pretty trivial.  The reason I'm hesitating is because we have
> all these DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls,
> dw_plat, hisi, qcom, spear13xx, and now armada8k), and they're
> *mostly* similar, but they differ in minor, annoying ways.  This is
> becoming a significant maintenance burden for me, and I'd like to
> figure out how to mitigate that.

Understood.

> For now, I don't have any great ideas except that it would be nice to
> remove needless variations.  The following is a typical code
> structure, but it's not universally followed:
> 
>   XXX_pcie_probe
>     XXX_add_pcie_port
>       dw_pcie_host_init
>         XXX_pcie_host_init
> 	  dw_pcie_setup_rc
> 	  XXX_pcie_establish_link

Indeed, the first step towards having more common code is to make the
code to be factorized look as similar as possible.

> There's a hodge-podge of ways to get related resources (clocks and
> PHYs) and initialize them.  IRQ setup is not really consistent across
> all the drivers.  It's sort of disappointing that most of these
> drivers have a "dbi_base" resource, but they use different DT property
> names for it.
> 
> The armada8k driver doesn't have a DRV_add_pcie_port() function or a
> DRV_pcie_establish_link() function, and it has its own "wait for link"
> timeout loop instead of using dw_pcie_wait_for_link().
> 
> How about if you just shuffle those bits around into
> an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
> we'll call that good for now?

I'll take a stab at that tomorrow and send an updated version.

Thanks for the feedback!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Mon, 25 Apr 2016 12:28:46 -0500, Bjorn Helgaas wrote:

> It *is* pretty trivial.  The reason I'm hesitating is because we have
> all these DesignWare-based drivers (dra7xx, exynos, imx6, ks, ls,
> dw_plat, hisi, qcom, spear13xx, and now armada8k), and they're
> *mostly* similar, but they differ in minor, annoying ways.  This is
> becoming a significant maintenance burden for me, and I'd like to
> figure out how to mitigate that.

Understood.

> For now, I don't have any great ideas except that it would be nice to
> remove needless variations.  The following is a typical code
> structure, but it's not universally followed:
> 
>   XXX_pcie_probe
>     XXX_add_pcie_port
>       dw_pcie_host_init
>         XXX_pcie_host_init
> 	  dw_pcie_setup_rc
> 	  XXX_pcie_establish_link

Indeed, the first step towards having more common code is to make the
code to be factorized look as similar as possible.

> There's a hodge-podge of ways to get related resources (clocks and
> PHYs) and initialize them.  IRQ setup is not really consistent across
> all the drivers.  It's sort of disappointing that most of these
> drivers have a "dbi_base" resource, but they use different DT property
> names for it.
> 
> The armada8k driver doesn't have a DRV_add_pcie_port() function or a
> DRV_pcie_establish_link() function, and it has its own "wait for link"
> timeout loop instead of using dw_pcie_wait_for_link().
> 
> How about if you just shuffle those bits around into
> an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
> we'll call that good for now?

I'll take a stab at that tomorrow and send an updated version.

Thanks for the feedback!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello Bjorn,

On Mon, 25 Apr 2016 12:28:46 -0500, Bjorn Helgaas wrote:

> How about if you just shuffle those bits around into
> an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
> we'll call that good for now?

I just sent a v3 which implements this.

However, to be honest, after looking at the other drivers, I found the
XYZ_add_pcie_port() function to not be very useful: it simply
continues the work done by the probe function, and there is really no
reason to split the remaining work of the probe() in a separate
function. The XYZ_add_pcie_port() is called only one time for each
probe() call, so having a separate function is somewhat useless. In
addition, the separation of work between probe() and
XYZ_add_pcie_port() is not very consistent accross drivers. In
pci-imx6, XYZ_add_pcie_port() only sets up the interrupt handler and
calls dw_pcie_host_init(). In pci-dra7xx, XYZ_add_pcie_port() sets up
the interrupt, maps some registers and calls dw_pcie_host_init().

So my v3 implements what you suggested, and creates a
armada8k_add_pcie_port() to make it consistent with the other drivers,
but long term, I'm not sure this particular function is really useful.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello Bjorn,

On Mon, 25 Apr 2016 12:28:46 -0500, Bjorn Helgaas wrote:

> How about if you just shuffle those bits around into
> an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
> we'll call that good for now?

I just sent a v3 which implements this.

However, to be honest, after looking at the other drivers, I found the
XYZ_add_pcie_port() function to not be very useful: it simply
continues the work done by the probe function, and there is really no
reason to split the remaining work of the probe() in a separate
function. The XYZ_add_pcie_port() is called only one time for each
probe() call, so having a separate function is somewhat useless. In
addition, the separation of work between probe() and
XYZ_add_pcie_port() is not very consistent accross drivers. In
pci-imx6, XYZ_add_pcie_port() only sets up the interrupt handler and
calls dw_pcie_host_init(). In pci-dra7xx, XYZ_add_pcie_port() sets up
the interrupt, maps some registers and calls dw_pcie_host_init().

So my v3 implements what you suggested, and creates a
armada8k_add_pcie_port() to make it consistent with the other drivers,
but long term, I'm not sure this particular function is really useful.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello Bjorn,

On Mon, 25 Apr 2016 12:28:46 -0500, Bjorn Helgaas wrote:

> How about if you just shuffle those bits around into
> an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
> we'll call that good for now?

I just sent a v3 which implements this.

However, to be honest, after looking at the other drivers, I found the
XYZ_add_pcie_port() function to not be very useful: it simply
continues the work done by the probe function, and there is really no
reason to split the remaining work of the probe() in a separate
function. The XYZ_add_pcie_port() is called only one time for each
probe() call, so having a separate function is somewhat useless. In
addition, the separation of work between probe() and
XYZ_add_pcie_port() is not very consistent accross drivers. In
pci-imx6, XYZ_add_pcie_port() only sets up the interrupt handler and
calls dw_pcie_host_init(). In pci-dra7xx, XYZ_add_pcie_port() sets up
the interrupt, maps some registers and calls dw_pcie_host_init().

So my v3 implements what you suggested, and creates a
armada8k_add_pcie_port() to make it consistent with the other drivers,
but long term, I'm not sure this particular function is really useful.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Bjorn Helgaas]

On Tue, Apr 26, 2016 at 10:42:08AM +0200, Thomas Petazzoni wrote:
> Hello Bjorn,
> 
> On Mon, 25 Apr 2016 12:28:46 -0500, Bjorn Helgaas wrote:
> 
> > How about if you just shuffle those bits around into
> > an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
> > we'll call that good for now?
> 
> I just sent a v3 which implements this.
> 
> However, to be honest, after looking at the other drivers, I found the
> XYZ_add_pcie_port() function to not be very useful: it simply
> continues the work done by the probe function, and there is really no
> reason to split the remaining work of the probe() in a separate
> function. The XYZ_add_pcie_port() is called only one time for each
> probe() call, so having a separate function is somewhat useless. In
> addition, the separation of work between probe() and
> XYZ_add_pcie_port() is not very consistent accross drivers. In
> pci-imx6, XYZ_add_pcie_port() only sets up the interrupt handler and
> calls dw_pcie_host_init(). In pci-dra7xx, XYZ_add_pcie_port() sets up
> the interrupt, maps some registers and calls dw_pcie_host_init().
> 
> So my v3 implements what you suggested, and creates a
> armada8k_add_pcie_port() to make it consistent with the other drivers,
> but long term, I'm not sure this particular function is really useful.

I agree completely with everything you said.  I would be thrilled if
somebody were to make this more sensible, as long as we did it to
*all* the drivers, not just one.

I think things started out fairly simple, then diverged over time as
we added clocks, PHYs, MSI, etc.  It looks like we're going to have
more divergence for power management, suspend, hotplug, modules, etc.

Bjorn

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Bjorn Helgaas]

On Tue, Apr 26, 2016 at 10:42:08AM +0200, Thomas Petazzoni wrote:
> Hello Bjorn,
> 
> On Mon, 25 Apr 2016 12:28:46 -0500, Bjorn Helgaas wrote:
> 
> > How about if you just shuffle those bits around into
> > an armada8k_add_pcie_port() and an armada8k_pcie_establish_link(), and
> > we'll call that good for now?
> 
> I just sent a v3 which implements this.
> 
> However, to be honest, after looking at the other drivers, I found the
> XYZ_add_pcie_port() function to not be very useful: it simply
> continues the work done by the probe function, and there is really no
> reason to split the remaining work of the probe() in a separate
> function. The XYZ_add_pcie_port() is called only one time for each
> probe() call, so having a separate function is somewhat useless. In
> addition, the separation of work between probe() and
> XYZ_add_pcie_port() is not very consistent accross drivers. In
> pci-imx6, XYZ_add_pcie_port() only sets up the interrupt handler and
> calls dw_pcie_host_init(). In pci-dra7xx, XYZ_add_pcie_port() sets up
> the interrupt, maps some registers and calls dw_pcie_host_init().
> 
> So my v3 implements what you suggested, and creates a
> armada8k_add_pcie_port() to make it consistent with the other drivers,
> but long term, I'm not sure this particular function is really useful.

I agree completely with everything you said.  I would be thrilled if
somebody were to make this more sensible, as long as we did it to
*all* the drivers, not just one.

I think things started out fairly simple, then diverged over time as
we added clocks, PHYs, MSI, etc.  It looks like we're going to have
more divergence for power management, suspend, hotplug, modules, etc.

Bjorn

Re: [PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Tue, 26 Apr 2016 12:00:04 -0500, Bjorn Helgaas wrote:

> > So my v3 implements what you suggested, and creates a
> > armada8k_add_pcie_port() to make it consistent with the other drivers,
> > but long term, I'm not sure this particular function is really useful.
> 
> I agree completely with everything you said.  I would be thrilled if
> somebody were to make this more sensible, as long as we did it to
> *all* the drivers, not just one.

OK. I'll see if I can come up with a few patches. Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 0/2] pci: host: new driver for Marvell Armada 7K/8K PCIe controller

[Thomas Petazzoni]

Hello,

On Tue, 26 Apr 2016 12:00:04 -0500, Bjorn Helgaas wrote:

> > So my v3 implements what you suggested, and creates a
> > armada8k_add_pcie_port() to make it consistent with the other drivers,
> > but long term, I'm not sure this particular function is really useful.
> 
> I agree completely with everything you said.  I would be thrilled if
> somebody were to make this more sensible, as long as we did it to
> *all* the drivers, not just one.

OK. I'll see if I can come up with a few patches. Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com