[PATCH v3 0/6] Qualcomm PCIe driver and designware fixes

[Stanimir Varbanov]

This patch set is continuation of previous v2 at [1]. This time I
decided to drop the PHY driver needed by pcie driver for apq8084
and send it separately soon (when sort out the need of separate phy
driver).

First two patches in the set are fixes in pcie designware driver
found during testing the qcom pcie driver. The second patch can
be treated as an RFC.

Next two patches adds DT binding document and the driver itself.

The last two patches just adds relevant dt node for apq8064 and
enable pcie usage on ifc6410 development board. The testing has been
made on ifc6410 development board.

Comments are welcome!

Here is edited description:

This patchset introduces a Qualcomm PCIe root complex driver for
Snapdragon 805 (APQ8084) and Snapdragon 600 (APQ8064). The PCIe
hardware use Designware IP core plus Qualcomm application
specific hw wrappers.

[1] https://lkml.org/lkml/2015/5/4/267

Stanimir Varbanov (6):
PCI: designware: remove wrong io_base assignment
PCI: designware: add memory barrier after enabling region
DT: PCI: qcom: Document PCIe devicetree bindings
PCI: qcom: Add Qualcomm PCIe controller driver
ARM: dts: apq8064: add pcie devicetree node
ARM: dts: ifc6410: enable pcie dt node for this board

.../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++
MAINTAINERS | 7 +
arch/arm/boot/dts/qcom-apq8064-ifc6410.dts | 26 +
arch/arm/boot/dts/qcom-apq8064.dtsi | 36 ++
drivers/pci/host/Kconfig | 10 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pcie-designware.c | 6 +-
drivers/pci/host/pcie-qcom.c | 623 ++++++++++++++++++++
8 files changed, 939 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt
create mode 100644 drivers/pci/host/pcie-qcom.c

--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Stanimir Varbanov]

The io_base is used to keep the cpu physical address parsed
from ranges dt property. After issue pci_remap_iospace the
io_base has been assigned with io->start, which is not correct
cause io->start is a PCI bus address.

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---
drivers/pci/host/pcie-designware.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 540f077c37ea..02a7452bdf23 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
ret, pp->io);
continue;
}
- pp->io_base = pp->io->start;
break;
case IORESOURCE_MEM:
pp->mem = win->res;

[Stanimir Varbanov]

Add 'write memory' barrier after enable region in PCIE_ATU_CR2
register. The barrier is needed to ensure that the region enable
request has been reached it's destination at time when we
read/write to PCI configuration space.

Without this barrier PCI device enumeration during kernel boot
is not reliable, and reading configuration space for particular
PCI device on the bus returns zero aka no device.

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---

drivers/pci/host/pcie-designware.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 02a7452bdf23..e15a2ae1583f 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
+ /*
+ * ensure that the ATU enable has been happaned before accessing
+ * pci configuration/io spaces through dw_pcie_cfg_[read|write].
+ */
+ smp_wmb();
}

static struct irq_chip dw_msi_irq_chip = {

[Stanimir Varbanov]

From: Stanimir Varbanov <svar...@mm-sol.com>

The PCIe driver reuse the Designware common code for host
and MSI initialization, and also program the Qualcomm
application specific registers.

Signed-off-by: Stanimir Varbanov <svar...@mm-sol.com>

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---

MAINTAINERS | 7 +

drivers/pci/host/Kconfig | 10 +
drivers/pci/host/Makefile | 1 +

drivers/pci/host/pcie-qcom.c | 623 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 641 insertions(+)
create mode 100644 drivers/pci/host/pcie-qcom.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b16bffabe70a..878d9068fe75 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8231,6 +8231,13 @@ S: Maintained
F: Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
F: drivers/pci/host/pcie-hisi.c

+PCIE DRIVER FOR QUALCOMM MSM
+M: Stanimir Varbanov <svar...@mm-sol.com>
+L: linu...@vger.kernel.org
+L: linux-...@vger.kernel.org
+S: Maintained
+F: drivers/pci/host/*qcom*
+
PCMCIA SUBSYSTEM
P: Linux PCMCIA Team
L: linux-...@lists.infradead.org
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index f131ba947dc6..64d33ba3e336 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -172,4 +172,14 @@ config PCI_HISI
help
Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC

+config PCIE_QCOM
+ bool "Qualcomm PCIe controller"
+ depends on ARCH_QCOM && OF || COMPILE_TEST
+ select PCIE_DW
+ select PCIEPORTBUS
+ help
+ Say Y here to enable PCIe controller support on Qualcomm SoCs. The
+ PCIe controller use Designware core plus Qualcomm specific hardware
+ wrappers.
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9d4d3c6924a1..e523c171febf 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_QCOM) += pcie-qcom.o
diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
new file mode 100644
index 000000000000..5f1957988503
--- /dev/null
+++ b/drivers/pci/host/pcie-qcom.c
@@ -0,0 +1,623 @@
+/*
+ * Copyright (c) 2014-2015, The Linux Foundation. All rights reserved.
+ * Copyright 2015 Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define PCIE20_PARF_PHY_CTRL 0x40
+#define PCIE20_PARF_PHY_REFCLK 0x4C
+#define PCIE20_PARF_DBI_BASE_ADDR 0x168
+#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE 0x16c
+#define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT 0x178
+
+#define PCIE20_ELBI_SYS_CTRL 0x04
+#define PCIE20_ELBI_SYS_CTRL_LT_ENABLE BIT(0)
+
+#define PCIE20_ELBI_SYS_STTS 0x08
+#define XMLH_LINK_UP BIT(10)
+
+#define PCIE20_CAP 0x70
+#define PCIE20_CAP_LINKCTRLSTATUS (PCIE20_CAP + 0x10)
+#define PCIE20_CAP_LINKCTRLSTATUS_LINK_UP BIT(29)
+
+#define PERST_DELAY_MS 1
+
+#define LINKUP_DELAY_US 5000
+#define LINKUP_TIMEOUT_US 100000
+
+struct qcom_pcie_resources_v0 {
+ struct clk *iface_clk;
+ struct clk *core_clk;
+ struct clk *phy_clk;
+ struct reset_control *pci_reset;
+ struct reset_control *axi_reset;
+ struct reset_control *ahb_reset;
+ struct reset_control *por_reset;
+ struct reset_control *phy_reset;
+ struct regulator *vdda;
+ struct regulator *vdda_phy;
+ struct regulator *vdda_refclk;
+};
+
+struct qcom_pcie_resources_v1 {
+ struct clk *iface;
+ struct clk *aux;
+ struct clk *master_bus;
+ struct clk *slave_bus;
+ struct reset_control *core;
+ struct regulator *vdda;
+};
+
+union qcom_pcie_resources {
+ struct qcom_pcie_resources_v0 v0;
+ struct qcom_pcie_resources_v1 v1;
+};
+
+struct qcom_pcie;
+
+struct qcom_pcie_ops {
+ int (*get_resources)(struct qcom_pcie *pcie);
+ int (*init)(struct qcom_pcie *pcie);
+ void (*deinit)(struct qcom_pcie *pcie);
+};
+
+struct qcom_pcie {
+ struct pcie_port pp;
+ struct device *dev;
+ union qcom_pcie_resources res;
+ void __iomem *parf;
+ void __iomem *dbi;
+ void __iomem *elbi;
+ struct phy *phy;
+ struct gpio_desc *reset;
+ struct qcom_pcie_ops *ops;
+};
+
+#define to_qcom_pcie(x) container_of(x, struct qcom_pcie, pp)
+
+static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
+{
+ gpiod_set_value(pcie->reset, 1);
+ msleep(PERST_DELAY_MS);
+}
+
+static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
+{
+ gpiod_set_value(pcie->reset, 0);
+ msleep(PERST_DELAY_MS);
+}
+
+static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
+{
+ struct pcie_port *pp = arg;
+
+ return dw_handle_msi_irq(pp);
+}
+
+static int qcom_pcie_enable_link_training(struct qcom_pcie *pcie)
+{
+ struct device *dev = pcie->dev;
+ u32 val;
+ int ret;
+
+ /* enable link training */
+ val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+ val |= PCIE20_ELBI_SYS_CTRL_LT_ENABLE;
+ writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+
+ /* wait for up to 100ms for the link to come up */
+ ret = readl_poll_timeout(pcie->elbi + PCIE20_ELBI_SYS_STTS, val,
+ val & XMLH_LINK_UP, LINKUP_DELAY_US,
+ LINKUP_TIMEOUT_US);
+
+ if (ret < 0 || !dw_pcie_link_up(&pcie->pp)) {
+ dev_err(dev, "link initialization failed\n");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
+ struct device *dev = pcie->dev;
+
+ res->vdda = devm_regulator_get(dev, "vdda");
+ if (IS_ERR(res->vdda))
+ return PTR_ERR(res->vdda);
+
+ res->vdda_phy = devm_regulator_get(dev, "vdda_phy");
+ if (IS_ERR(res->vdda_phy))
+ return PTR_ERR(res->vdda_phy);
+
+ res->vdda_refclk = devm_regulator_get(dev, "vdda_refclk");
+ if (IS_ERR(res->vdda_refclk))
+ return PTR_ERR(res->vdda_refclk);
+
+ res->iface_clk = devm_clk_get(dev, "iface");
+ if (IS_ERR(res->iface_clk))
+ return PTR_ERR(res->iface_clk);
+
+ res->core_clk = devm_clk_get(dev, "core");
+ if (IS_ERR(res->core_clk))
+ return PTR_ERR(res->core_clk);
+
+ res->phy_clk = devm_clk_get(dev, "phy");
+ if (IS_ERR(res->phy_clk))
+ return PTR_ERR(res->phy_clk);
+
+ res->pci_reset = devm_reset_control_get(dev, "pci");
+ if (IS_ERR(res->pci_reset))
+ return PTR_ERR(res->pci_reset);
+
+ res->axi_reset = devm_reset_control_get(dev, "axi");
+ if (IS_ERR(res->axi_reset))
+ return PTR_ERR(res->axi_reset);
+
+ res->ahb_reset = devm_reset_control_get(dev, "ahb");
+ if (IS_ERR(res->ahb_reset))
+ return PTR_ERR(res->ahb_reset);
+
+ res->por_reset = devm_reset_control_get(dev, "por");
+ if (IS_ERR(res->por_reset))
+ return PTR_ERR(res->por_reset);
+
+ res->phy_reset = devm_reset_control_get(dev, "phy");
+ if (IS_ERR(res->phy_reset))
+ return PTR_ERR(res->phy_reset);
+
+ return 0;
+}
+
+static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
+ struct device *dev = pcie->dev;
+
+ res->vdda = devm_regulator_get(dev, "vdda");
+ if (IS_ERR(res->vdda))
+ return PTR_ERR(res->vdda);
+
+ res->iface = devm_clk_get(dev, "iface");
+ if (IS_ERR(res->iface))
+ return PTR_ERR(res->iface);
+
+ res->aux = devm_clk_get(dev, "aux");
+ if (IS_ERR(res->aux))
+ return PTR_ERR(res->aux);
+
+ res->master_bus = devm_clk_get(dev, "master_bus");
+ if (IS_ERR(res->master_bus))
+ return PTR_ERR(res->master_bus);
+
+ res->slave_bus = devm_clk_get(dev, "slave_bus");
+ if (IS_ERR(res->slave_bus))
+ return PTR_ERR(res->slave_bus);
+
+ res->core = devm_reset_control_get(dev, "core");
+ if (IS_ERR(res->core))
+ return PTR_ERR(res->core);
+
+ return 0;
+}
+
+static void qcom_pcie_deinit_v0(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
+
+ reset_control_assert(res->pci_reset);
+ reset_control_assert(res->axi_reset);
+ reset_control_assert(res->ahb_reset);
+ reset_control_assert(res->por_reset);
+ reset_control_assert(res->pci_reset);
+ clk_disable_unprepare(res->iface_clk);
+ clk_disable_unprepare(res->core_clk);
+ clk_disable_unprepare(res->phy_clk);
+ regulator_disable(res->vdda);
+ regulator_disable(res->vdda_phy);
+ regulator_disable(res->vdda_refclk);
+}
+
+static int qcom_pcie_init_v0(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
+ struct device *dev = pcie->dev;
+ u32 val;
+ int ret;
+
+ ret = regulator_enable(res->vdda);
+ if (ret) {
+ dev_err(dev, "cannot enable vdda regulator\n");
+ return ret;
+ }
+
+ ret = regulator_enable(res->vdda_refclk);
+ if (ret) {
+ dev_err(dev, "cannot enable vdda_refclk regulator\n");
+ goto err_refclk;
+ }
+
+ ret = regulator_enable(res->vdda_phy);
+ if (ret) {
+ dev_err(dev, "cannot enable vdda_phy regulator\n");
+ goto err_vdda_phy;
+ }
+
+ ret = reset_control_assert(res->ahb_reset);
+ if (ret) {
+ dev_err(dev, "cannot assert ahb reset\n");
+ goto err_assert_ahb;
+ }
+
+ ret = clk_prepare_enable(res->iface_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable iface clock\n");
+ goto err_assert_ahb;
+ }
+
+ ret = clk_prepare_enable(res->phy_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable phy clock\n");
+ goto err_clk_phy;
+ }
+
+ ret = clk_prepare_enable(res->core_clk);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable core clock\n");
+ goto err_clk_core;
+ }
+
+ ret = reset_control_deassert(res->ahb_reset);
+ if (ret) {
+ dev_err(dev, "cannot deassert ahb reset\n");
+ goto err_deassert_ahb;
+ }
+
+ /* enable PCIe clocks and resets */
+ val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
+ val &= ~BIT(0);
+ writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
+ /* enable external reference clock */
+ val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
+ val |= BIT(16);
+ writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
+
+ ret = reset_control_deassert(res->phy_reset);
+ if (ret) {
+ dev_err(dev, "cannot deassert phy reset\n");
+ return ret;
+ }
+
+ ret = reset_control_deassert(res->pci_reset);
+ if (ret) {
+ dev_err(dev, "cannot deassert pci reset\n");
+ return ret;
+ }
+
+ ret = reset_control_deassert(res->por_reset);
+ if (ret) {
+ dev_err(dev, "cannot deassert por reset\n");
+ return ret;
+ }
+
+ ret = reset_control_deassert(res->axi_reset);
+ if (ret) {
+ dev_err(dev, "cannot deassert axi reset\n");
+ return ret;
+ }
+
+ /* wait for clock acquisition */
+ usleep_range(1000, 1500);
+
+ return 0;
+
+err_deassert_ahb:
+ clk_disable_unprepare(res->core_clk);
+err_clk_core:
+ clk_disable_unprepare(res->phy_clk);
+err_clk_phy:
+ clk_disable_unprepare(res->iface_clk);
+err_assert_ahb:
+ regulator_disable(res->vdda_phy);
+err_vdda_phy:
+ regulator_disable(res->vdda_refclk);
+err_refclk:
+ regulator_disable(res->vdda);
+
+ return ret;
+}
+
+static void qcom_pcie_deinit_v1(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
+
+ reset_control_assert(res->core);
+ clk_disable_unprepare(res->slave_bus);
+ clk_disable_unprepare(res->master_bus);
+ clk_disable_unprepare(res->iface);
+ clk_disable_unprepare(res->aux);
+ regulator_disable(res->vdda);
+}
+
+static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
+{
+ struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
+ struct device *dev = pcie->dev;
+ int ret;
+
+ ret = reset_control_deassert(res->core);
+ if (ret) {
+ dev_err(dev, "cannot deassert core reset\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(res->aux);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable aux clock\n");
+ goto err_res;
+ }
+
+ ret = clk_prepare_enable(res->iface);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable iface clock\n");
+ goto err_aux;
+ }
+
+ ret = clk_prepare_enable(res->master_bus);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable master_bus clock\n");
+ goto err_iface;
+ }
+
+ ret = clk_prepare_enable(res->slave_bus);
+ if (ret) {
+ dev_err(dev, "cannot prepare/enable slave_bus clock\n");
+ goto err_master;
+ }
+
+ ret = regulator_enable(res->vdda);
+ if (ret) {
+ dev_err(dev, "cannot enable vdda regulator\n");
+ goto err_slave;
+ }
+
+
+ /* change DBI base address */
+ writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+
+ val |= BIT(31);
+ writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+ }
+
+ return 0;
+err_slave:
+ clk_disable_unprepare(res->slave_bus);
+err_master:
+ clk_disable_unprepare(res->master_bus);
+err_iface:
+ clk_disable_unprepare(res->iface);
+err_aux:
+ clk_disable_unprepare(res->aux);
+err_res:
+ reset_control_assert(res->core);
+
+ return ret;
+}
+
+static int qcom_pcie_link_up(struct pcie_port *pp)
+{
+ struct qcom_pcie *pcie = to_qcom_pcie(pp);
+ u32 val = readl(pcie->dbi + PCIE20_CAP_LINKCTRLSTATUS);
+
+ return !!(val & PCIE20_CAP_LINKCTRLSTATUS_LINK_UP);
+}
+
+static void qcom_pcie_host_init(struct pcie_port *pp)
+{
+ struct qcom_pcie *pcie = to_qcom_pcie(pp);
+ int ret;
+
+ qcom_ep_reset_assert(pcie);
+
+ ret = pcie->ops->init(pcie);
+ if (ret)
+ goto err_deinit;
+
+ ret = phy_power_on(pcie->phy);
+ if (ret)
+ goto err_deinit;
+
+ dw_pcie_setup_rc(pp);
+
+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ dw_pcie_msi_init(pp);
+
+ qcom_ep_reset_deassert(pcie);
+
+ ret = qcom_pcie_enable_link_training(pcie);
+ if (ret)
+ goto err;
+
+ return;
+err:
+ qcom_ep_reset_assert(pcie);
+ phy_power_off(pcie->phy);
+err_deinit:
+ pcie->ops->deinit(pcie);
+}
+
+static int
+qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, u32 *val)
+{
+ /* the device class is not reported correctly from the register */
+ if (where == PCI_CLASS_REVISION && size == 4) {
+ *val = readl(pp->dbi_base + PCI_CLASS_REVISION);
+ *val &= 0xff; /* keep revision id */
+ *val |= PCI_CLASS_BRIDGE_PCI << 16;
+ return PCIBIOS_SUCCESSFUL;
+ }
+
+ return dw_pcie_cfg_read(pp->dbi_base + where, size, val);
+}
+
+static struct pcie_host_ops qcom_pcie_dw_ops = {
+ .link_up = qcom_pcie_link_up,
+ .host_init = qcom_pcie_host_init,
+ .rd_own_conf = qcom_pcie_rd_own_conf,
+};
+
+static const struct qcom_pcie_ops ops_v0 = {
+ .get_resources = qcom_pcie_get_resources_v0,
+ .init = qcom_pcie_init_v0,
+ .deinit = qcom_pcie_deinit_v0,
+};
+
+static const struct qcom_pcie_ops ops_v1 = {
+ .get_resources = qcom_pcie_get_resources_v1,
+ .init = qcom_pcie_init_v1,
+ .deinit = qcom_pcie_deinit_v1,
+};
+
+static int qcom_pcie_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ struct qcom_pcie *pcie;
+ struct pcie_port *pp;
+ int ret;
+
+ pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+ if (!pcie)
+ return -ENOMEM;
+
+ pcie->ops = (struct qcom_pcie_ops *)of_device_get_match_data(dev);
+ pcie->dev = dev;
+
+ pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_LOW);
+ if (IS_ERR(pcie->reset))
+ return PTR_ERR(pcie->reset);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
+ pcie->parf = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pcie->parf))
+ return PTR_ERR(pcie->parf);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+ pcie->dbi = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pcie->dbi))
+ return PTR_ERR(pcie->dbi);
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "elbi");
+ pcie->elbi = devm_ioremap_resource(dev, res);
+ if (IS_ERR(pcie->elbi))
+ return PTR_ERR(pcie->elbi);
+
+ pcie->phy = devm_phy_optional_get(dev, "pciephy");
+ if (IS_ERR(pcie->phy))
+ return PTR_ERR(pcie->phy);
+
+ ret = pcie->ops->get_resources(pcie);
+ if (ret)
+ return ret;
+
+ pp = &pcie->pp;
+ pp->dev = dev;
+ pp->dbi_base = pcie->dbi;
+ pp->root_bus_nr = -1;
+ pp->ops = &qcom_pcie_dw_ops;
+
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ pp->msi_irq = platform_get_irq_byname(pdev, "msi");
+ if (pp->msi_irq < 0)
+ return pp->msi_irq;
+
+ ret = devm_request_irq(dev, pp->msi_irq,
+ qcom_pcie_msi_irq_handler,
+ IRQF_SHARED, "qcom-pcie-msi", pp);
+ if (ret) {
+ dev_err(dev, "cannot request msi irq\n");
+ return ret;
+ }
+ }
+
+ ret = phy_init(pcie->phy);
+ if (ret)
+ return ret;
+
+ ret = dw_pcie_host_init(pp);
+ if (ret) {
+ dev_err(dev, "cannot initialize host\n");
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, pcie);
+
+ return 0;
+}
+
+static int qcom_pcie_remove(struct platform_device *pdev)
+{
+ struct qcom_pcie *pcie = platform_get_drvdata(pdev);
+
+ qcom_ep_reset_assert(pcie);
+ phy_power_off(pcie->phy);
+ phy_exit(pcie->phy);
+ pcie->ops->deinit(pcie);
+
+ return 0;
+}
+
+static const struct of_device_id qcom_pcie_match[] = {
+ { .compatible = "qcom,pcie-v0", .data = &ops_v0 },
+ { .compatible = "qcom,pcie-v1", .data = &ops_v1 },
+ { }
+};
+MODULE_DEVICE_TABLE(of, qcom_pcie_match);
+
+static struct platform_driver qcom_pcie_driver = {
+ .probe = qcom_pcie_probe,
+ .remove = qcom_pcie_remove,
+ .driver = {
+ .name = "qcom-pcie",
+ .of_match_table = qcom_pcie_match,
+ },
+};
+
+module_platform_driver(qcom_pcie_driver);
+
+MODULE_AUTHOR("Stanimir Varbanov <svar...@mm-sol.com>");
+MODULE_DESCRIPTION("Qualcomm PCIe root complex driver");
+MODULE_LICENSE("GPL v2");

[Stanimir Varbanov]

Add the pcie dt node so that it can probe and used.

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---

arch/arm/boot/dts/qcom-apq8064.dtsi | 36 +++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index a4c1762b53ea..bb7181e8ef2f 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -659,5 +659,41 @@
compatible = "qcom,tcsr-apq8064", "syscon";
reg = <0x1a400000 0x100>;
};
+
+ pcie: pci@1b500000 {
+ compatible = "qcom,pcie-v0", "snps,dw-pcie";
+ reg = <0x1b500000 0x1000
+ 0x1b502000 0x80
+ 0x1b600000 0x100
+ 0x0ff00000 0x100000>;
+ reg-names = "dbi", "elbi", "parf", "config";
+ device_type = "pci";
+ linux,pci-domain = <0>;
+ bus-range = <0x00 0xff>;
+ num-lanes = <1>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges = <0x81000000 0 0 0x0fe00000 0 0x00100000 /* I/O */
+ 0x82000000 0 0 0x08000000 0 0x07e00000>; /* memory */
+ interrupts = <GIC_SPI 238 IRQ_TYPE_NONE>;
+ interrupt-names = "msi";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 &intc 0 36 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+ <0 0 0 2 &intc 0 37 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+ <0 0 0 3 &intc 0 38 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+ <0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+ clocks = <&gcc PCIE_A_CLK>,
+ <&gcc PCIE_H_CLK>,
+ <&gcc PCIE_PHY_REF_CLK>;
+ clock-names = "core", "iface", "phy";
+ resets = <&gcc PCIE_ACLK_RESET>,
+ <&gcc PCIE_HCLK_RESET>,
+ <&gcc PCIE_POR_RESET>,
+ <&gcc PCIE_PCI_RESET>,
+ <&gcc PCIE_PHY_RESET>;
+ reset-names = "axi", "ahb", "por", "pci", "phy";
+ status = "disabled";
+ };
};
};

[Stanimir Varbanov]

Enable pcie dt node and fill pcie dt node with regulator, pinctrl
and reset gpio, to use the pcie on the ifc6410 board.

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---

arch/arm/boot/dts/qcom-apq8064-ifc6410.dts | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
index 11ac608b6d50..bdee36a9b418 100644
--- a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
+++ b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
@@ -47,6 +47,18 @@
bias-disable;
};
};
+
+ pcie0_pins: pcie0_pinmux {
+ mux {
+ pins = "gpio27";
+ function = "gpio";
+ };
+ conf {
+ pins = "gpio27";
+ drive-strength = <12>;
+ bias-disable;
+ };
+ };
};

rpm@108000 {
@@ -123,6 +135,10 @@
pm8921_lvs1: lvs1 {
bias-pull-down;
};
+
+ pm8921_lvs6: lvs6 {
+ bias-pull-down;
+ };
};
};

@@ -231,6 +247,16 @@
status = "okay";
};

+ pci@1b500000 {
+ status = "ok";
+ vdda-supply = <&pm8921_s3>;
+ vdda_phy-supply = <&pm8921_lvs6>;
+ vdda_refclk-supply = <&ext_3p3v>;
+ pinctrl-0 = <&pcie0_pins>;
+ pinctrl-names = "default";
+ perst-gpio = <&tlmm_pinmux 27 GPIO_ACTIVE_LOW>;
+ };
+
qcom,ssbi@500000 {
pmic@0 {
gpio@150 {

[Stanimir Varbanov]

From: Stanimir Varbanov <svar...@mm-sol.com>

Document Qualcomm PCIe driver devicetree bindings.

Signed-off-by: Stanimir Varbanov <svar...@mm-sol.com>

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---

.../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++++++++++++++
1 file changed, 231 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
new file mode 100644
index 000000000000..d7640d45fa31
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -0,0 +1,231 @@
+* Qualcomm PCI express root complex
+
+- compatible:
+ Usage: required
+ Value type: <stringlist>
+ Definition: Value shall include
+ - "qcom,pcie-v0" for apq/ipq8064
+ - "qcom,pcie-v1" for apq8084
+
+- reg:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: Register ranges as listed in the reg-names property
+
+- reg-names:
+ Usage: required
+ Value type: <stringlist>
+ Definition: Must include the following entries
+ - "parf" Qualcomm specific registers
+ - "dbi" Designware PCIe registers
+ - "elbi" External local bus interface registers
+ - "config" PCIe configuration space
+
+- device_type:
+ Usage: required
+ Value type: <string>
+ Definition: Should be "pci". As specified in designware-pcie.txt
+
+- #address-cells:
+ Usage: required
+ Value type: <u32>
+ Definition: Should be set to 3. As specified in designware-pcie.txt
+
+- #size-cells:
+ Usage: required
+ Value type: <u32>
+ Definition: Should be set 2. As specified in designware-pcie.txt
+
+- ranges:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: As specified in designware-pcie.txt
+
+- interrupts:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: MSI interrupt
+
+- interrupt-names:
+ Usage: required
+ Value type: <stringlist>
+ Definition: Should contain "msi"
+
+- #interrupt-cells:
+ Usage: required
+ Value type: <u32>
+ Definition: Should be 1. As specified in designware-pcie.txt
+
+- interrupt-map-mask:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: As specified in designware-pcie.txt
+
+- interrupt-map:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: As specified in designware-pcie.txt
+
+- clocks:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: List of phandle and clock specifier pairs as listed
+ in clock-names property
+
+- clock-names:
+ Usage: required
+ Value type: <stringlist>
+ Definition: Should contain the following entries
+ * should be populated for v0 and v1
+ - "iface" Configuration AHB clock
+
+ * should be populated for v0
+ - "core" Clocks the pcie hw block
+ - "phy" Clocks the pcie PHY block
+
+ * should be populated for v1
+ - "aux" Auxiliary (AUX) clock
+ - "bus_master" Master AXI clock
+ - "bus_slave" Slave AXI clock
+
+- resets:
+ Usage: required
+ Value type: <prop-encoded-array>
+ Definition: List of phandle and reset specifier pairs as listed
+ in reset-names property
+
+- reset-names:
+ Usage: required
+ Value type: <stringlist>
+ Definition: Should contain the following entries
+ * should be populated for v0
+ - "axi" AXI reset
+ - "ahb" AHB reset
+ - "por" POR reset
+ - "pci" PCI reset
+ - "phy" PHY reset
+
+ * should be populated for v1
+ - "core" Core reset
+
+- power-domains:
+ Usage: required (for v1 only)
+ Value type: <prop-encoded-array>
+ Definition: A phandle and power domain specifier pair to the
+ power domain which is responsible for collapsing
+ and restoring power to the peripheral
+
+- <name>-supply:
+ Usage: required
+ Value type: <phandle>
+ Definition: List of phandles to the power supply regulator(s)
+ * should be populated for v0 and v1
+ - "vdda" core analog power supply
+
+ * should be populated for v0
+ - "vdda_phy" analog power supply for PHY
+ - "vdda_refclk" analog power supply for IC which generate
+ reference clock
+
+- phys:
+ Usage: required (for v1 only)
+ Value type: <phandle>
+ Definition: List of phandle(s) as listed in phy-names property
+
+- phy-names:
+ Usage: required (for v1 only)
+ Value type: <stringlist>
+ Definition: Should contain "pciephy"
+
+- <name>-gpio:
+ Usage: optional
+ Value type: <prop-encoded-array>
+ Definition: List of phandle and gpio specifier pairs. Should contain
+ - "perst" PCIe endpoint reset signal line
+ - "pewake" PCIe endpoint wake signal line
+
+- pinctrl-0:
+ Usage: required
+ Value type: <phandle>
+ Definition: List of phandles pointing at a pin(s) configuration
+
+- pinctrl-names
+ Usage: required
+ Value type: <stringlist>
+ Definition: List of names of pinctrl-0 state
+
+* Example for v0
+ pcie0: pci@1b500000 {

+ compatible = "qcom,pcie-v0", "snps,dw-pcie";
+ reg = <0x1b500000 0x1000
+ 0x1b502000 0x80
+ 0x1b600000 0x100
+ 0x0ff00000 0x100000>;
+ reg-names = "dbi", "elbi", "parf", "config";
+ device_type = "pci";
+ linux,pci-domain = <0>;
+ bus-range = <0x00 0xff>;
+ num-lanes = <1>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges = <0x81000000 0 0 0x0fe00000 0 0x00100000 /* I/O */

+ 0x82000000 0 0x00000000 0x08000000 0 0x07e00000>; /* memory */

+ interrupts = <GIC_SPI 238 IRQ_TYPE_NONE>;
+ interrupt-names = "msi";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;
+ interrupt-map = <0 0 0 1 &intc 0 36 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+ <0 0 0 2 &intc 0 37 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+ <0 0 0 3 &intc 0 38 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+ <0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+ clocks = <&gcc PCIE_A_CLK>,
+ <&gcc PCIE_H_CLK>,

+ <&gcc PCIE_PHY_CLK>;

+ clock-names = "core", "iface", "phy";
+ resets = <&gcc PCIE_ACLK_RESET>,
+ <&gcc PCIE_HCLK_RESET>,
+ <&gcc PCIE_POR_RESET>,
+ <&gcc PCIE_PCI_RESET>,
+ <&gcc PCIE_PHY_RESET>;
+ reset-names = "axi", "ahb", "por", "pci", "phy";

+ };
+
+* Example for v1
+ pcie0@fc520000 {
+ compatible = "qcom,pcie-v1", "snps,dw-pcie";
+ reg = <0xfc520000 0x2000>,
+ <0xff000000 0x1000>,
+ <0xff001000 0x1000>,
+ <0xff002000 0x2000>;
+ reg-names = "parf", "dbi", "elbi", "config";

+ device_type = "pci";
+ linux,pci-domain = <0>;
+ bus-range = <0x00 0xff>;
+ num-lanes = <1>;
+ #address-cells = <3>;
+ #size-cells = <2>;

+ ranges = <0x81000000 0 0 0xff200000 0 0x00100000 /* I/O */
+ 0x82000000 0 0x00300000 0xff300000 0 0x00d00000>; /* memory */
+ interrupts = <GIC_SPI 243 IRQ_TYPE_NONE>;

+ interrupt-names = "msi";
+ #interrupt-cells = <1>;
+ interrupt-map-mask = <0 0 0 0x7>;

+ interrupt-map = <0 0 0 1 &intc 0 244 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+ <0 0 0 2 &intc 0 245 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+ <0 0 0 3 &intc 0 247 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+ <0 0 0 4 &intc 0 248 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+ clocks = <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
+ <&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
+ <&gcc GCC_PCIE_0_SLV_AXI_CLK>,
+ <&gcc GCC_PCIE_0_AUX_CLK>;
+ clock-names = "iface", "master_bus", "slave_bus", "aux";
+ resets = <&gcc GCC_PCIE_0_BCR>;
+ reset-names = "core";
+ power-domains = <&gcc PCIE0_GDSC>;
+ vdda-supply = <&pma8084_l3>;
+ phys = <&pciephy0>;
+ phy-names = "pciephy";
+ perst-gpio = <&tlmm 70 GPIO_ACTIVE_LOW>;
+ pinctrl-0 = <&pcie0_pins_default>;
+ pinctrl-names = "default";
+ };

[Arnd Bergmann]

On Monday 23 November 2015 11:28:58 Stanimir Varbanov wrote:
> The io_base is used to keep the cpu physical address parsed
> from ranges dt property. After issue pci_remap_iospace the
> io_base has been assigned with io->start, which is not correct
> cause io->start is a PCI bus address.
>
> Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
> ---
> drivers/pci/host/pcie-designware.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 540f077c37ea..02a7452bdf23 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> ret, pp->io);
> continue;
> }
> - pp->io_base = pp->io->start;
> break;
> case IORESOURCE_MEM:
> pp->mem = win->res;

I was surprised to see such an obvious bug here, as we had spent a lot of
time trying to get it right. However, it broke only recently and it's
worth mentioning what commit did it, so

Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() to parse DT")
Reviewed-by: Arnd Bergmann <ar...@arndb.de>

The bug is present in 4.4-rc1 and we should get your fix merged into 4.4
as well, while all the other patches in your series are presumably for 4.5.

Arnd

[Gabriele Paoloni]

Hi Stanimir, Many Thanks for this fix

> -----Original Message-----
> From: linux-ker...@vger.kernel.org [mailto:linux-kernel-
> ow...@vger.kernel.org] On Behalf Of Arnd Bergmann
> Sent: 23 November 2015 10:00
> To: Stanimir Varbanov
> Cc: linux-...@vger.kernel.org; linux-...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; devic...@vger.kernel.org; linux-
> p...@vger.kernel.org; Bjorn Helgaas; Srinivas Kandagatla; Rob Herring; Rob
> Herring; Mark Rutland; Pawel Moll; Ian Campbell; Jingoo Han; Pratyush Anand;
> Bjorn Andersson
> Subject: Re: [PATCH v3 1/6] PCI: designware: remove wrong io_base assignment

>
> On Monday 23 November 2015 11:28:58 Stanimir Varbanov wrote:
> > The io_base is used to keep the cpu physical address parsed
> > from ranges dt property. After issue pci_remap_iospace the
> > io_base has been assigned with io->start, which is not correct
> > cause io->start is a PCI bus address.
> >
> > Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
> > ---
> > drivers/pci/host/pcie-designware.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-
> designware.c
> > index 540f077c37ea..02a7452bdf23 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > ret, pp->io);
> > continue;
> > }
> > - pp->io_base = pp->io->start;
> > break;
> > case IORESOURCE_MEM:
> > pp->mem = win->res;
>
> I was surprised to see such an obvious bug here, as we had spent a lot of
> time trying to get it right. However, it broke only recently and it's
> worth mentioning what commit did it, so

Looking at
"[PATCH v12 0/8] PCI: hisi: Add PCIe host support for HiSilicon SoC Hip05":

actually the bug came in as patch 3/6 of v11 patchset split

[...]

Change from v11:
- Split 3/6 in v11 to 3/8, 4/8, 5/8 in v12.

[...]

This was not present in v11...sorry about this.

Gab

[Russell King - ARM Linux]

So, why is this a SMP barrier?

--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[Stanimir Varbanov]

I wrongly assumed that smp_wmb will come down to wmb(), I have no excuse.

But despite my ignorant, as I noted in cover letter I want this patch to
be treated as an RFC (maybe I had to add it in the subject).

Do we really need a memory barrier after enabling ATU in PCI_ATU_CR2
register and before first access to pci configuration space using
read[lwb] (see dw_pcie_rd_other_conf)? Or I have made totally wrong
assumption.

--
regards,
Stan

[Stanimir Varbanov]

I think the bug is introduced in:

cbce7900598c ("PCI: designware: Make driver arch-agnostic")

cause the io_base is correctly calculated in 0021d22b73d6, do you agree?

--
regards,
Stan

[Arnd Bergmann]

On Monday 23 November 2015 18:23:47 Stanimir Varbanov wrote:
> >>
> >> Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources()
> >> to parse DT")
> >> Reviewed-by: Arnd Bergmann <ar...@arndb.de>
>
> I think the bug is introduced in:
>
> cbce7900598c ("PCI: designware: Make driver arch-agnostic")
>
> cause the io_base is correctly calculated in 0021d22b73d6, do you agree?

I think cbce7900598c just slightly changes the io_base value, but
it's still referring to a bus address, not a cpu address, both before
and after the patch.

0021d22b73d6 hower seems to remove the correct 'pp->io_base = range.cpu_addr;'
and replaces it with 'pp->io_base = pp->io->start;', so it's now in
the wrong address space.

Arnd

[Bjorn Andersson]

On Mon 23 Nov 01:29 PST 2015, Stanimir Varbanov wrote:

> From: Stanimir Varbanov <svar...@mm-sol.com>
>
> Document Qualcomm PCIe driver devicetree bindings.
>
> Signed-off-by: Stanimir Varbanov <svar...@mm-sol.com>
> Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
> ---
> .../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++++++++++++++
> 1 file changed, 231 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> new file mode 100644
> index 000000000000..d7640d45fa31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -0,0 +1,231 @@
> +* Qualcomm PCI express root complex
> +
> +- compatible:
> + Usage: required
> + Value type: <stringlist>
> + Definition: Value shall include
> + - "qcom,pcie-v0" for apq/ipq8064
> + - "qcom,pcie-v1" for apq8084

Do you know if we have the same v1 of this block in 8994?

[..]

> +- clock-names:
> + Usage: required
> + Value type: <stringlist>
> + Definition: Should contain the following entries
> + * should be populated for v0 and v1
> + - "iface" Configuration AHB clock
> +
> + * should be populated for v0
> + - "core" Clocks the pcie hw block
> + - "phy" Clocks the pcie PHY block
> +
> + * should be populated for v1
> + - "aux" Auxiliary (AUX) clock
> + - "bus_master" Master AXI clock
> + - "bus_slave" Slave AXI clock

You have white spaces among your tabs here.

[..]

> +- <name>-supply:
> + Usage: required
> + Value type: <phandle>
> + Definition: List of phandles to the power supply regulator(s)
> + * should be populated for v0 and v1
> + - "vdda" core analog power supply
> +
> + * should be populated for v0
> + - "vdda_phy" analog power supply for PHY
> + - "vdda_refclk" analog power supply for IC which generate
> + reference clock

Exploding these into 3 different property descriptions would make it
easier to read, and you can say "required for v0" for the latter
two and simply "required" on the vdda.

[..]

> +- <name>-gpio:
> + Usage: optional
> + Value type: <prop-encoded-array>
> + Definition: List of phandle and gpio specifier pairs. Should contain
> + - "perst" PCIe endpoint reset signal line
> + - "pewake" PCIe endpoint wake signal line

This property should be pluralized, i.e. it's <name>-gpios.

Are these identifiers coming from some data sheet? Or could we simply
name them "reset" and "wakeup"?

> +
> +- pinctrl-0:
> + Usage: required
> + Value type: <phandle>
> + Definition: List of phandles pointing at a pin(s) configuration

This is not required and as it's a property applicable to all devices we
normally don't mention them.

> +
> +- pinctrl-names
> + Usage: required
> + Value type: <stringlist>
> + Definition: List of names of pinctrl-0 state
> +

dito

Regards,
Bjorn

[Rob Herring]

On Mon, Nov 23, 2015 at 11:29:00AM +0200, Stanimir Varbanov wrote:
> From: Stanimir Varbanov <svar...@mm-sol.com>
>
> Document Qualcomm PCIe driver devicetree bindings.
>
> Signed-off-by: Stanimir Varbanov <svar...@mm-sol.com>
> Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
> ---
> .../devicetree/bindings/pci/qcom,pcie.txt | 231 ++++++++++++++++++++
> 1 file changed, 231 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> new file mode 100644
> index 000000000000..d7640d45fa31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -0,0 +1,231 @@
> +* Qualcomm PCI express root complex
> +
> +- compatible:
> + Usage: required
> + Value type: <stringlist>
> + Definition: Value shall include
> + - "qcom,pcie-v0" for apq/ipq8064
> + - "qcom,pcie-v1" for apq8084

Use chip part numbers, not versions.

Also, shouldn't you have the Designware compatible in addition.

[...]

> +
> +- <name>-gpio:

Use -gpios

> + Usage: optional
> + Value type: <prop-encoded-array>
> + Definition: List of phandle and gpio specifier pairs. Should contain
> + - "perst" PCIe endpoint reset signal line
> + - "pewake" PCIe endpoint wake signal line

[Stanimir Varbanov]

Bjorn, thanks for the comments!

I have no idea, but looking in caf msm-3.18 it should be possible to
reuse the code for v1.

>
> [..]
>> +- clock-names:
>> + Usage: required
>> + Value type: <stringlist>
>> + Definition: Should contain the following entries
>> + * should be populated for v0 and v1
>> + - "iface" Configuration AHB clock
>> +
>> + * should be populated for v0
>> + - "core" Clocks the pcie hw block
>> + - "phy" Clocks the pcie PHY block
>> +
>> + * should be populated for v1
>> + - "aux" Auxiliary (AUX) clock
>> + - "bus_master" Master AXI clock
>> + - "bus_slave" Slave AXI clock
>
> You have white spaces among your tabs here.

Ops, I forgot to remove the white spaces.

>
> [..]
>> +- <name>-supply:
>> + Usage: required
>> + Value type: <phandle>
>> + Definition: List of phandles to the power supply regulator(s)
>> + * should be populated for v0 and v1
>> + - "vdda" core analog power supply
>> +
>> + * should be populated for v0
>> + - "vdda_phy" analog power supply for PHY
>> + - "vdda_refclk" analog power supply for IC which generate
>> + reference clock
>
> Exploding these into 3 different property descriptions would make it
> easier to read, and you can say "required for v0" for the latter
> two and simply "required" on the vdda.

yes, that is a good idea.

>
> [..]
>> +- <name>-gpio:
>> + Usage: optional
>> + Value type: <prop-encoded-array>
>> + Definition: List of phandle and gpio specifier pairs. Should contain
>> + - "perst" PCIe endpoint reset signal line
>> + - "pewake" PCIe endpoint wake signal line
>
> This property should be pluralized, i.e. it's <name>-gpios.

I hope you mean :

- "perst-gpios" PCIe endpoint reset signal line
- "pewake-gpios" PCIe endpoint wake signal line

>
> Are these identifiers coming from some data sheet? Or could we simply
> name them "reset" and "wakeup"?

In the pcie express card electromechanical specification we have signal
names PERST# and WAKE#, so I'd like to keep as they are in the
specification, thus the wake# is wrong and I will rename it to wake-gpios.

>
>> +
>> +- pinctrl-0:
>> + Usage: required
>> + Value type: <phandle>
>> + Definition: List of phandles pointing at a pin(s) configuration
>
> This is not required and as it's a property applicable to all devices we
> normally don't mention them.

agreed.

>
>> +
>> +- pinctrl-names
>> + Usage: required
>> + Value type: <stringlist>
>> + Definition: List of names of pinctrl-0 state
>> +
>
> dito
>
> Regards,
> Bjorn
> --

> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in

> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

--
regards,
Stan

[Stanimir Varbanov]

Thanks for the comments!

Sure.

>
> Also, shouldn't you have the Designware compatible in addition.
>
> [...]
>
>> +
>> +- <name>-gpio:
>
> Use -gpios

Ok I will.

--
regards,
Stan

[Stanimir Varbanov]

On 11/23/2015 06:40 PM, Arnd Bergmann wrote:
> On Monday 23 November 2015 18:23:47 Stanimir Varbanov wrote:
>>>>
>>>> Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources()
>>>> to parse DT")
>>>> Reviewed-by: Arnd Bergmann <ar...@arndb.de>
>>
>> I think the bug is introduced in:
>>
>> cbce7900598c ("PCI: designware: Make driver arch-agnostic")
>>
>> cause the io_base is correctly calculated in 0021d22b73d6, do you agree?
>
>
> I think cbce7900598c just slightly changes the io_base value, but
> it's still referring to a bus address, not a cpu address, both before
> and after the patch.
>
> 0021d22b73d6 hower seems to remove the correct 'pp->io_base = range.cpu_addr;'
> and replaces it with 'pp->io_base = pp->io->start;', so it's now in
> the wrong address space.
>
> Arnd
>

OK, I will resend this as separate patch, and will drop it from this series.

--
regards,
Stan

[Stanimir Varbanov]

The io_base is used to keep the cpu physical address parsed
from ranges dt property. After issue pci_remap_iospace the
io_base has been assigned with io->start, which is not correct
cause io->start is a PCI bus address.

Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() to parse DT")
Reviewed-by: Arnd Bergmann <ar...@arndb.de>

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---

drivers/pci/host/pcie-designware.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 540f077c37ea..02a7452bdf23 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
ret, pp->io);
continue;
}
- pp->io_base = pp->io->start;
break;
case IORESOURCE_MEM:
pp->mem = win->res;

--
1.7.9.5

[Bjorn Helgaas]

On Tue, Nov 24, 2015 at 11:36:52AM +0200, Stanimir Varbanov wrote:
> The io_base is used to keep the cpu physical address parsed
> from ranges dt property. After issue pci_remap_iospace the
> io_base has been assigned with io->start, which is not correct
> cause io->start is a PCI bus address.
>
> Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() to parse DT")
> Reviewed-by: Arnd Bergmann <ar...@arndb.de>
> Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
> ---
> drivers/pci/host/pcie-designware.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 540f077c37ea..02a7452bdf23 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
> ret, pp->io);
> continue;
> }
> - pp->io_base = pp->io->start;
> break;
> case IORESOURCE_MEM:
> pp->mem = win->res;

Applied to for-linus for v4.4 with changelog as follows, thanks!


commit 5228e39e3f709c82e6c4643402fc25de54391e32
Author: Stanimir Varbanov <stanimir...@linaro.org>
Date: Tue Nov 24 11:36:52 2015 +0200

PCI: designware: Remove incorrect io_base assignment

"pp->io" is an I/O resource, e.g., "[io 0x0000-0xffff]"; "pp->io_base" is
the CPU physical address of a region where the host bridge converts CPU
memory accesses into PCI I/O transactions.

Corrupting pp->io_base by assigning pp->io->start to it breaks access to
the PCI I/O space, as reported by Kishon.

Remove the invalid assignment.

[bhelgaas: changelog]

Fixes: 0021d22b73d6 ("PCI: designware: Use of_pci_get_host_bridge_resources() to parse DT")

Reported-and-tested-by: Kishon Vijay Abraham I <kis...@ti.com>
Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
Signed-off-by: Bjorn Helgaas <bhel...@google.com>
Reviewed-by: Arnd Bergmann <ar...@arndb.de>

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 540f077..02a7452 100644

--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -440,7 +440,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
ret, pp->io);
continue;
}
- pp->io_base = pp->io->start;
break;
case IORESOURCE_MEM:
pp->mem = win->res;
--

[Stanimir Varbanov]

Add 'write memory' barrier after enable region in PCIE_ATU_CR2
register. The barrier is needed to ensure that the region enable
request has been reached it's destination at time when we
read/write to PCI configuration space.

Without this barrier PCI device enumeration during kernel boot
is not reliable, and reading configuration space for particular
PCI device on the bus returns zero aka no device.

Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
---

drivers/pci/host/pcie-designware.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 02a7452bdf23..ed4dc2e2553b 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c

@@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
+ /*
+ * ensure that the ATU enable has been happaned before accessing
+ * pci configuration/io spaces through dw_pcie_cfg_[read|write].
+ */

+ wmb();

}

static struct irq_chip dw_msi_irq_chip = {
--
1.7.9.5

[Stanimir Varbanov]

On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> Add 'write memory' barrier after enable region in PCIE_ATU_CR2
> register. The barrier is needed to ensure that the region enable
> request has been reached it's destination at time when we
> read/write to PCI configuration space.
>
> Without this barrier PCI device enumeration during kernel boot
> is not reliable, and reading configuration space for particular
> PCI device on the bus returns zero aka no device.

Anand, Jingoo, what is your opinion?

>
> Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
> ---
> drivers/pci/host/pcie-designware.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 02a7452bdf23..ed4dc2e2553b 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
> dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
> dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
> dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> + /*
> + * ensure that the ATU enable has been happaned before accessing
> + * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> + */
> + wmb();
> }
>
> static struct irq_chip dw_msi_irq_chip = {
>


--

regards,
Stan

[Pratyush Anand]

My understnading is that since writel() of dw_pcie_writel_rc() in
above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
will follow) goes through same device (ie PCIe host here). So, it is
guaranteed that 1st writel() will be executed before later
readl()/writel(). If that is true then we do not need any explicit
barrier here.

Arnd, Russel: whats your opinion here.

~Pratyush

[Arnd Bergmann]

On Wednesday 09 December 2015 10:10:05 Pratyush Anand wrote:
> On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov

> > > Signed-off-by: Stanimir Varbanov <stanimir...@linaro.org>
> > > ---
> > > drivers/pci/host/pcie-designware.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index 02a7452bdf23..ed4dc2e2553b 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
> > > dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
> > > dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
> > > dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > > + /*
> > > + * ensure that the ATU enable has been happaned before accessing
> > > + * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> > > + */
> > > + wmb();
> > > }
> > >
>
>
> My understnading is that since writel() of dw_pcie_writel_rc() in
> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> will follow) goes through same device (ie PCIe host here). So, it is
> guaranteed that 1st writel() will be executed before later
> readl()/writel(). If that is true then we do not need any explicit
> barrier here.
>
> Arnd, Russel: whats your opinion here.

I think the ordering is only enforced if the two register accesses are
on the same device as seen from the bus, and it's possible that the
RC registers and the config space registers are not considered the
same thing here.

For config write, this is not a problem, because the config space write
has a wmb() that enforces ordering, but it's possible that the config
space read may hit the device in parallel with the PCIE_ATU_ENABLE
write.

Arnd

[Russell King - ARM Linux]

^l

writel() has a barrier _before_ the access but not after.

The fact is that there's nothing which guarantees that the write will hit
the hardware in a timely manner (forget any rules about PCI config space,
the PCI ordering rules apply to the PCI bus, not to the ARM buses.)

If you need this write to have hit the hardware before continuing, you
need to read back from the same register.

I'm just looking at this driver, trying to decipher what it's doing. It
_looks_ to me like it's reprogramming one of the outbound windows (IO?)
so that configuration space can be accessed. Doesn't this have the
effect of disabling access to the IO segment of the PCI bus from the
host CPU?

What protections are there against other CPUs in the system issuing a
PCI I/O read/write while this outbound window is programmed as
configuration space?

--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/

FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[Stanimir Varbanov]

Hmm, just a matter of fact - as I described in the patch description
this wmb() fixed an issue with pcie device enumeration (I came down to
pci_bus_read_dev_vendor_id() returns zero) i.e. exactly a pci
configuration space read.

--
regards,
Stan

[Pratyush Anand]

On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux
<li...@arm.linux.org.uk> wrote:

[...]

>> > > dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>> > > + /*
>> > > + * ensure that the ATU enable has been happaned before accessing
>> > > + * pci configuration/io spaces through dw_pcie_cfg_[read|write].
>> > > + */
>> > > + wmb();
>> > > }
>> > >
>>
>>
>> My understnading is that since writel() of dw_pcie_writel_rc() in
>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
>> will follow) goes through same device (ie PCIe host here). So, it is
>> guaranteed that 1st writel() will be executed before later
>> readl()/writel(). If that is true then we do not need any explicit
>> barrier here.
>>
>> Arnd, Russel: whats your opinion here.
> ^l

Sorry :(

>
> writel() has a barrier _before_ the access but not after.
>
> The fact is that there's nothing which guarantees that the write will hit
> the hardware in a timely manner (forget any rules about PCI config space,
> the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
>
> If you need this write to have hit the hardware before continuing, you
> need to read back from the same register.

OK, so better to replace wmb() with read back of control register.

>
> I'm just looking at this driver, trying to decipher what it's doing. It
> _looks_ to me like it's reprogramming one of the outbound windows (IO?)
> so that configuration space can be accessed. Doesn't this have the
> effect of disabling access to the IO segment of the PCI bus from the
> host CPU?
>
> What protections are there against other CPUs in the system issuing a
> PCI I/O read/write while this outbound window is programmed as
> configuration space?

Yes, that is an issue with this driver. Most of the host controller
has 4 or more viewpoints, and it is very easy to handle for them. But
there are few which has only two viewpoints. Do not know how to solve
it, so that it works for all.

~Pratyush

[Jisheng Zhang]

On Fri, 11 Dec 2015 09:35:10 +0530 Pratyush Anand wrote:

The default outbound iATU number is two, this may be the reason why the driver
is written in current style. And two outbound iATUs may be common for pcie dw
users because ASIC people just follow the default configuration ;).

In our case, Marvell Berlin SoCs have two outbound iATUs.

Thanks,
Jisheng

[Stanimir Varbanov]

Would the patch be acceptable if I replace wmb with read?

--
regards,
Stan

[Pratyush Anand]

For me it would be fine.

~Pratyush

[Jingoo Han]

Hmm, we need to add new DT property to handle the number of outbound iATUs.
Then, 'pcie-designware.c' should configure registers according to the number.
Anyway, we should add this agenda to ToDo list.

Best regards,
Jingoo Han