pci: host: new driver for Marvell Armada 7K/8K PCIe controller

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

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

---
 .../devicetree/bindings/pci/pci-armada8k.txt       | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pci-armada8k.txt

new file mode 100644
index 0000000..598533a
@@ -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

I already acked the last version, did something change?

Hello,

"""
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!
"""

Thanks!

Thomas

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

Found it after I replied...

I did, here:

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.

---
 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

index 7a0780d..a3b6f24 100644
@@ -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

index d85b5fa..a6f85e3 100644
@@ -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

2.6.4

Hello,

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

Hi Thomas,

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

Hello,

Understood.

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

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

Hello Bjorn,

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

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

Hello,

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