[PATCH v3 0/3] PCI: qcom: Add support to msm8996 pcie controller.

[PATCH v3 0/3] PCI: qcom: Add support to msm8996 pcie controller.

[Srinivas Kandagatla]

This patchset adds support to msm8996 pcie controller. I tested this patch on
v4.9-rc2 along with phy driver patch [1] and
"PCI: designware: check for iATU unroll support after initializing host"
fix [2] on DB820c APQ8096 board on port B and port C using sata and
ethernet controller.

Changes since v2:
	- Removed regulators that belong to phy, spotted by Stephen
	- Removed clocks in to simple pm bus driver, spotted by Stephen
	- renamed msm8996 ops to v2 ops as suggested by Stephen.
	- cleanups as suggested by Stephen.
	- Add runtime pm support to driver.
	- Added pm clk support to simple pm bus driver.

Changes since v1:
	- Fixed dt example as suggested by Rob
	- added smmu bus clk dependency as smmu sits in between
	  system NOC and PCIe.
	- Removed smmu configuration from bindings and driver as
	  the smmu Level2 translation on this SOC is controlled by
	  the secure world, and level 1 translation is disabled,
	  so there is one-to-one mapping of the address space.

Thanks,
srini

[1] https://patchwork.kernel.org/patch/9384711/
[2] https://patchwork.kernel.org/patch/9377557/

Srinivas Kandagatla (3):
  bus: simple-pm: add support to pm clocks
  PCI: qcom: add support to msm8996 PCIE controller
  PCI: qcom: add runtime pm support to pcie_port

 .../devicetree/bindings/pci/qcom,pcie.txt          |  68 +++++++-
 drivers/bus/simple-pm-bus.c                        |  13 +-
 drivers/pci/host/pcie-qcom.c                       | 183 ++++++++++++++++++++-
 3 files changed, 257 insertions(+), 7 deletions(-)

-- 
2.10.1

[PATCH v3 1/3] bus: simple-pm: add support to pm clocks

[Srinivas Kandagatla]

This patch adds support to pm clocks via device tree, so that the clocks
can be turned on and off during runtime pm. This patch is required for
Qualcomm msm8996 pcie controller which sits on a bus with its own
power-domain and clocks.

Without this patch the clock associated with the bus are never turned on.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index c5eb46c..63b7e8c 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_clock.h>
 #include <linux/pm_runtime.h>
 
 
@@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(&pdev->dev);
 
-	if (np)
+	if (np) {
+		of_pm_clk_add_clks(&pdev->dev);
 		of_platform_populate(np, NULL, NULL, &pdev->dev);
+	}
 
 	return 0;
 }
 
+static const struct dev_pm_ops simple_pm_bus_pm_ops = {
+	SET_RUNTIME_PM_OPS(pm_clk_suspend,
+			   pm_clk_resume, NULL)
+};
+
 static int simple_pm_bus_remove(struct platform_device *pdev)
 {
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
 	pm_runtime_disable(&pdev->dev);
+	pm_clk_destroy(&pdev->dev);
+
 	return 0;
 }
 
@@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = {
 	.driver = {
 		.name = "simple-pm-bus",
 		.of_match_table = simple_pm_bus_of_match,
+		.pm = &simple_pm_bus_pm_ops,
 	},
 };
 
-- 
2.10.1

[PATCH v3 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Srinivas Kandagatla]

This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
legacy interrupts and it conforms to PCI Express Base 2.1 specification.

This patch adds post_init callback to qcom_pcie_ops, as this is pcie
pipe clocks are only setup after the phy is powered on.
It also adds ltssm_enable callback as it is very much different to other
supported SOCs in the driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../devicetree/bindings/pci/qcom,pcie.txt          |  68 +++++++-
 drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
 2 files changed, 239 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 4059a6f..4a0538d 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -7,6 +7,7 @@
 			- "qcom,pcie-ipq8064" for ipq8064
 			- "qcom,pcie-apq8064" for apq8064
 			- "qcom,pcie-apq8084" for apq8084
+			- "qcom,pcie-msm8996" for msm8996 or apq8096
 
 - reg:
 	Usage: required
@@ -92,6 +93,16 @@
 			- "aux"		Auxiliary (AUX) clock
 			- "bus_master"	Master AXI clock
 			- "bus_slave"	Slave AXI clock
+
+- clock-names:
+	Usage: required for msm8996/apq8096
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "aux"		Auxiliary (AUX) clock.
+			- "bus_master"	Master AXI clock.
+			- "bus_slave"	Slave AXI clock.
+			- "pipe"	Pipe Clock driving internal logic.
+			- "cfg"		Configuration clk.
 - resets:
 	Usage: required
 	Value type: <prop-encoded-array>
@@ -115,7 +126,7 @@
 			- "core" Core reset
 
 - power-domains:
-	Usage: required for apq8084
+	Usage: required for apq8084 and msm8996/apq8096
 	Value type: <prop-encoded-array>
 	Definition: A phandle and power domain specifier pair to the
 		    power domain which is responsible for collapsing
@@ -231,3 +242,58 @@
 		pinctrl-0 = <&pcie0_pins_default>;
 		pinctrl-names = "default";
 	};
+
+* Example for apq8096:
+
+	pcie@00608000{
+		compatible = "qcom,pcie-msm8996", "snps,dw-pcie";
+		power-domains = <&gcc PCIE1_GDSC>;
+		bus-range = <0x00 0xff>;
+		num-lanes = <1>;
+
+		status  = "disabled";
+
+		reg = <0x00608000 0x2000>,
+		      <0x0d000000 0xf1d>,
+		      <0x0d000f20 0xa8>,
+		      <0x0d100000 0x100000>;
+
+		reg-names = "parf", "dbi", "elbi", "config";
+
+		phys = <&pcie_phy 1>;
+		phy-names = "pciephy";
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		ranges = <0x01000000 0x0 0x0d200000 0x0d200000 0x0 0x100000>,
+			<0x02000000 0x0 0x0d300000 0x0d300000 0x0 0xd00000>;
+
+		interrupts = <GIC_SPI 413 IRQ_TYPE_NONE>;
+		interrupt-names = "msi";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0x7>;
+		interrupt-map = <0 0 0 1 &intc 0 272 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+				<0 0 0 2 &intc 0 273 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+				<0 0 0 3 &intc 0 274 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+				<0 0 0 4 &intc 0 275 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&pcie1_clkreq_default &pcie1_perst_default &pcie1_wake_default>;
+		pinctrl-1 = <&pcie1_clkreq_sleep &pcie1_perst_default &pcie1_wake_sleep>;
+
+		vdda-1p8-supply = <&pm8994_l12>;
+		vdda-supply = <&pm8994_l28>;
+		linux,pci-domain = <1>;
+
+		clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
+			<&gcc GCC_PCIE_1_AUX_CLK>,
+			<&gcc GCC_PCIE_1_CFG_AHB_CLK>,
+			<&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
+			<&gcc GCC_PCIE_1_SLV_AXI_CLK>;
+
+		clock-names =  "pipe",
+				"aux",
+				"cfg",
+				"bus_master",
+				"bus_slave";
+	};
diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index 3593640..f37c690 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -36,11 +36,19 @@
 
 #include "pcie-designware.h"
 
+#define PCIE20_PARF_DBI_BASE_ADDR	0x168
+
+#define PCIE20_PARF_SYS_CTRL			0x00
 #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_MHI_CLOCK_RESET_CTRL	0x174
 #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
+#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
+#define PCIE20_PARF_LTSSM			0x1B0
+#define PCIE20_PARF_SID_OFFSET			0x234
+#define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
 
 #define PCIE20_ELBI_SYS_CTRL			0x04
 #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
@@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
 	struct regulator *vdda;
 };
 
+struct qcom_pcie_resources_v2 {
+	struct clk *aux_clk;
+	struct clk *master_clk;
+	struct clk *slave_clk;
+	struct clk *cfg_clk;
+	struct clk *pipe_clk;
+};
+
 union qcom_pcie_resources {
 	struct qcom_pcie_resources_v0 v0;
 	struct qcom_pcie_resources_v1 v1;
+	struct qcom_pcie_resources_v2 v2;
 };
 
 struct qcom_pcie;
@@ -82,7 +99,9 @@ struct qcom_pcie;
 struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
+	int (*post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
+	void (*ltssm_enable)(struct qcom_pcie *pcie);
 };
 
 struct qcom_pcie {
@@ -116,17 +135,33 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
 	return dw_handle_msi_irq(pp);
 }
 
-static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
+static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
 {
 	u32 val;
-
-	if (dw_pcie_link_up(&pcie->pp))
-		return 0;
-
 	/* 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);
+}
+
+static void qcom_pcie_v2_ltssm_enable(struct qcom_pcie *pcie)
+{
+	u32 val;
+	/* enable link training */
+	val = readl(pcie->parf + PCIE20_PARF_LTSSM);
+	val |= BIT(8);
+	writel(val, pcie->parf + PCIE20_PARF_LTSSM);
+}
+
+static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
+{
+
+	if (dw_pcie_link_up(&pcie->pp))
+		return 0;
+
+	/* Enable Link Training state machine */
+	if (pcie->ops->ltssm_enable)
+		pcie->ops->ltssm_enable(pcie);
 
 	return dw_pcie_wait_for_link(&pcie->pp);
 }
@@ -421,6 +456,113 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
 	return ret;
 }
 
+static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+	struct device *dev = pcie->pp.dev;
+
+	res->aux_clk = devm_clk_get(dev, "aux");
+	if (IS_ERR(res->aux_clk))
+		return PTR_ERR(res->aux_clk);
+
+	res->cfg_clk = devm_clk_get(dev, "cfg");
+	if (IS_ERR(res->cfg_clk))
+		return PTR_ERR(res->cfg_clk);
+
+	res->master_clk = devm_clk_get(dev, "bus_master");
+	if (IS_ERR(res->master_clk))
+		return PTR_ERR(res->master_clk);
+
+	res->slave_clk = devm_clk_get(dev, "bus_slave");
+	if (IS_ERR(res->slave_clk))
+		return PTR_ERR(res->slave_clk);
+
+	res->pipe_clk = devm_clk_get(dev, "pipe");
+	if (IS_ERR(res->pipe_clk))
+		return PTR_ERR(res->pipe_clk);
+
+	return 0;
+}
+
+static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+	struct device *dev = pcie->pp.dev;
+	u32 val;
+	int ret = 0;
+
+	ret = clk_prepare_enable(res->aux_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable aux clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(res->cfg_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable cfg clock\n");
+		goto err_cfg_clk;
+	}
+
+	ret = clk_prepare_enable(res->master_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable master clock\n");
+		goto err_master_clk;
+	}
+
+	ret = clk_prepare_enable(res->slave_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable slave clock\n");
+		goto err_slave_clk;
+	}
+
+	/* enable PCIe clocks and resets */
+	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
+	val &= ~BIT(0);
+	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
+	/* change DBI base address */
+	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+	/* MAC PHY_POWERDOWN MUX DISABLE  */
+	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
+	val &= ~BIT(29);
+	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
+
+	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+	val |= BIT(4);
+	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+
+	val = readl(pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+	val |= BIT(31);
+	writel(val, pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+
+	return 0;
+
+err_slave_clk:
+	clk_disable_unprepare(res->master_clk);
+err_master_clk:
+	clk_disable_unprepare(res->cfg_clk);
+err_cfg_clk:
+	clk_disable_unprepare(res->aux_clk);
+
+	return ret;
+}
+
+static int qcom_pcie_post_init_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+	struct device *dev = pcie->pp.dev;
+	int ret;
+
+	ret = clk_prepare_enable(res->pipe_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable pipe clock\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int qcom_pcie_link_up(struct pcie_port *pp)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
@@ -429,6 +571,17 @@ static int qcom_pcie_link_up(struct pcie_port *pp)
 	return !!(val & PCI_EXP_LNKSTA_DLLLA);
 }
 
+static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+
+	clk_disable_unprepare(res->slave_clk);
+	clk_disable_unprepare(res->master_clk);
+	clk_disable_unprepare(res->cfg_clk);
+	clk_disable_unprepare(res->aux_clk);
+	clk_disable_unprepare(res->pipe_clk);
+}
+
 static void qcom_pcie_host_init(struct pcie_port *pp)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
@@ -444,6 +597,9 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	if (ret)
 		goto err_deinit;
 
+	if (pcie->ops->post_init)
+		pcie->ops->post_init(pcie);
+
 	dw_pcie_setup_rc(pp);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI))
@@ -487,12 +643,22 @@ 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,
+	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
 };
 
 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,
+	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
+};
+
+static const struct qcom_pcie_ops ops_v2 = {
+	.get_resources = qcom_pcie_get_resources_v2,
+	.init = qcom_pcie_init_v2,
+	.post_init = qcom_pcie_post_init_v2,
+	.deinit = qcom_pcie_deinit_v2,
+	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
 };
 
 static int qcom_pcie_probe(struct platform_device *pdev)
@@ -572,6 +738,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_v0 },
 	{ .compatible = "qcom,pcie-apq8064", .data = &ops_v0 },
 	{ .compatible = "qcom,pcie-apq8084", .data = &ops_v1 },
+	{ .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },
 	{ }
 };
 
-- 
2.10.1

[PATCH v3 3/3] PCI: qcom: add runtime pm support to pcie_port

[Srinivas Kandagatla]

This patch is required when the pcie controller sits on a bus with
its own power domain and clocks which are controlled via a bus driver
like simple pm bus. As these bus driver have runtime pm enabled, it makes
sense to update the usage counter so that the runtime pm does not suspend
the clks or power domain associated with the bus driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/pci/host/pcie-qcom.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index f37c690..3751635 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -587,6 +587,9 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
 	int ret;
 
+
+	pm_runtime_get_sync(pp->dev);
+
 	qcom_ep_reset_assert(pcie);
 
 	ret = pcie->ops->init(pcie);
@@ -617,6 +620,7 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	phy_power_off(pcie->phy);
 err_deinit:
 	pcie->ops->deinit(pcie);
+	pm_runtime_put_sync(pp->dev);
 }
 
 static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
@@ -669,10 +673,12 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	struct pcie_port *pp;
 	int ret;
 
+
 	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
 		return -ENOMEM;
 
+	pm_runtime_enable(dev);
 	pp = &pcie->pp;
 	pcie->ops = (struct qcom_pcie_ops *)of_device_get_match_data(dev);
 
-- 
2.10.1

Re: [PATCH v3 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Vivek Gautam]

Hi,

On Fri, Nov 4, 2016 at 6:29 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks are only setup after the phy is powered on.
> It also adds ltssm_enable callback as it is very much different to other
> supported SOCs in the driver.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---

Few minor nits.

>  .../devicetree/bindings/pci/qcom,pcie.txt          |  68 +++++++-
>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>  2 files changed, 239 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 4059a6f..4a0538d 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -7,6 +7,7 @@
>                         - "qcom,pcie-ipq8064" for ipq8064
>                         - "qcom,pcie-apq8064" for apq8064
>                         - "qcom,pcie-apq8084" for apq8084
> +                       - "qcom,pcie-msm8996" for msm8996 or apq8096

Since this works for both apq8096 and msm8996, compatible -
"qcom,pcie-apq8096" for uniformity ?
[snip]

> @@ -231,3 +242,58 @@
>                 pinctrl-0 = <&pcie0_pins_default>;
>                 pinctrl-names = "default";
>         };
> +
> +* Example for apq8096:
> +
> +       pcie@00608000{
> +               compatible = "qcom,pcie-msm8996", "snps,dw-pcie";

this will change accordingly.

> +               power-domains = <&gcc PCIE1_GDSC>;
> +               bus-range = <0x00 0xff>;
> +               num-lanes = <1>;
> +

[snip]

> +
> +static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
> +{
> +       struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +       struct device *dev = pcie->pp.dev;
> +       u32 val;
> +       int ret = 0;

you don't need to initialize ret here.

> +
> +       ret = clk_prepare_enable(res->aux_clk);
> +       if (ret) {
> +               dev_err(dev, "cannot prepare/enable aux clock\n");
> +               return ret;
> +       }

[snip]

> @@ -429,6 +571,17 @@ static int qcom_pcie_link_up(struct pcie_port *pp)
>         return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>
> +static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
> +{
> +       struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +
> +       clk_disable_unprepare(res->slave_clk);
> +       clk_disable_unprepare(res->master_clk);
> +       clk_disable_unprepare(res->cfg_clk);
> +       clk_disable_unprepare(res->aux_clk);
> +       clk_disable_unprepare(res->pipe_clk);

i am sure, this is not affecting the functionality, but the pipe clock
is enabled after all the clocks.
so it makes sense to disable it in the first place. you can just move
this above slave_clk.

[snip]

> @@ -572,6 +738,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>         { .compatible = "qcom,pcie-ipq8064", .data = &ops_v0 },
>         { .compatible = "qcom,pcie-apq8064", .data = &ops_v0 },
>         { .compatible = "qcom,pcie-apq8084", .data = &ops_v1 },
> +       { .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },

this will change according to earlier comment in bindings.


Thanks
Vivek


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Rob Herring]

On Fri, Nov 04, 2016 at 12:59:46PM +0000, Srinivas Kandagatla wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
> 
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks are only setup after the phy is powered on.
> It also adds ltssm_enable callback as it is very much different to other
> supported SOCs in the driver.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  68 +++++++-
>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>  2 files changed, 239 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 4059a6f..4a0538d 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -7,6 +7,7 @@
>  			- "qcom,pcie-ipq8064" for ipq8064
>  			- "qcom,pcie-apq8064" for apq8064
>  			- "qcom,pcie-apq8084" for apq8084
> +			- "qcom,pcie-msm8996" for msm8996 or apq8096
>  
>  - reg:
>  	Usage: required
> @@ -92,6 +93,16 @@
>  			- "aux"		Auxiliary (AUX) clock
>  			- "bus_master"	Master AXI clock
>  			- "bus_slave"	Slave AXI clock
> +
> +- clock-names:
> +	Usage: required for msm8996/apq8096
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "aux"		Auxiliary (AUX) clock.
> +			- "bus_master"	Master AXI clock.
> +			- "bus_slave"	Slave AXI clock.
> +			- "pipe"	Pipe Clock driving internal logic.
> +			- "cfg"		Configuration clk.

The order here and the order in the example don't match. The order 
should be defined.

>  - resets:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> @@ -115,7 +126,7 @@
>  			- "core" Core reset
>  
>  - power-domains:
> -	Usage: required for apq8084
> +	Usage: required for apq8084 and msm8996/apq8096
>  	Value type: <prop-encoded-array>
>  	Definition: A phandle and power domain specifier pair to the
>  		    power domain which is responsible for collapsing
> @@ -231,3 +242,58 @@
>  		pinctrl-0 = <&pcie0_pins_default>;
>  		pinctrl-names = "default";
>  	};
> +
> +* Example for apq8096:
> +
> +	pcie@00608000{

Drop leading 0s.

> +		compatible = "qcom,pcie-msm8996", "snps,dw-pcie";
> +		power-domains = <&gcc PCIE1_GDSC>;
> +		bus-range = <0x00 0xff>;
> +		num-lanes = <1>;
> +
> +		status  = "disabled";

No point to have status in the example.

> +
> +		reg = <0x00608000 0x2000>,
> +		      <0x0d000000 0xf1d>,
> +		      <0x0d000f20 0xa8>,
> +		      <0x0d100000 0x100000>;
> +
> +		reg-names = "parf", "dbi", "elbi", "config";
> +
> +		phys = <&pcie_phy 1>;
> +		phy-names = "pciephy";
> +
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges = <0x01000000 0x0 0x0d200000 0x0d200000 0x0 0x100000>,
> +			<0x02000000 0x0 0x0d300000 0x0d300000 0x0 0xd00000>;
> +
> +		interrupts = <GIC_SPI 413 IRQ_TYPE_NONE>;
> +		interrupt-names = "msi";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 0x7>;
> +		interrupt-map = <0 0 0 1 &intc 0 272 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +				<0 0 0 2 &intc 0 273 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 274 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 275 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&pcie1_clkreq_default &pcie1_perst_default &pcie1_wake_default>;
> +		pinctrl-1 = <&pcie1_clkreq_sleep &pcie1_perst_default &pcie1_wake_sleep>;
> +
> +		vdda-1p8-supply = <&pm8994_l12>;
> +		vdda-supply = <&pm8994_l28>;
> +		linux,pci-domain = <1>;
> +
> +		clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> +			<&gcc GCC_PCIE_1_AUX_CLK>,
> +			<&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> +			<&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> +			<&gcc GCC_PCIE_1_SLV_AXI_CLK>;
> +
> +		clock-names =  "pipe",
> +				"aux",
> +				"cfg",
> +				"bus_master",
> +				"bus_slave";
> +	};

Re: [PATCH v3 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Srinivas Kandagatla]

On 09/11/16 10:37, Vivek Gautam wrote:
> Hi,
>
> On Fri, Nov 4, 2016 at 6:29 PM, Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
>> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
>> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>>
>> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
>> pipe clocks are only setup after the phy is powered on.
>> It also adds ltssm_enable callback as it is very much different to other
>> supported SOCs in the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>
> Few minor nits.
>
>>  .../devicetree/bindings/pci/qcom,pcie.txt          |  68 +++++++-
>>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>>  2 files changed, 239 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> index 4059a6f..4a0538d 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>> @@ -7,6 +7,7 @@
>>                         - "qcom,pcie-ipq8064" for ipq8064
>>                         - "qcom,pcie-apq8064" for apq8064
>>                         - "qcom,pcie-apq8084" for apq8084
>> +                       - "qcom,pcie-msm8996" for msm8996 or apq8096
>
> Since this works for both apq8096 and msm8996, compatible -
> "qcom,pcie-apq8096" for uniformity ?

AFAIK, compatible is selected based on SOC on which this IP is 
integrated first, So msm8996 seems to be correct, in that way.

Also if we look at clk controller compatible strings, you would see them 
as *msm8996* rather than *8096*.
>> +
>> +static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
>> +{
>> +       struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
>> +       struct device *dev = pcie->pp.dev;
>> +       u32 val;
>> +       int ret = 0;
>
> you don't need to initialize ret here.
Yep, I will fix it.
>
>> +
>> +       ret = clk_prepare_enable(res->aux_clk);
>> +       if (ret) {
>> +               dev_err(dev, "cannot prepare/enable aux clock\n");
>> +               return ret;
>> +       }
>
> [snip]
>
>> @@ -429,6 +571,17 @@ static int qcom_pcie_link_up(struct pcie_port *pp)
>>         return !!(val & PCI_EXP_LNKSTA_DLLLA);
>>  }
>>
>> +static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
>> +{
>> +       struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
>> +
>> +       clk_disable_unprepare(res->slave_clk);
>> +       clk_disable_unprepare(res->master_clk);
>> +       clk_disable_unprepare(res->cfg_clk);
>> +       clk_disable_unprepare(res->aux_clk);
>> +       clk_disable_unprepare(res->pipe_clk);
>
> i am sure, this is not affecting the functionality, but the pipe clock
> is enabled after all the clocks.
> so it makes sense to disable it in the first place. you can just move
> this above slave_clk.

Sure.. will fix it.
>
> [snip]
>

Re: [PATCH v3 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Vivek Gautam]

Hi,


On Mon, Nov 14, 2016 at 4:02 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
> On 09/11/16 10:37, Vivek Gautam wrote:
>>
>> Hi,
>>
>> On Fri, Nov 4, 2016 at 6:29 PM, Srinivas Kandagatla
>> <srinivas.kandagatla@linaro.org> wrote:
>>>
>>> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
>>> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
>>> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>>>
>>> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
>>> pipe clocks are only setup after the phy is powered on.
>>> It also adds ltssm_enable callback as it is very much different to other
>>> supported SOCs in the driver.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> ---
>>
>>
>> Few minor nits.
>>
>>>  .../devicetree/bindings/pci/qcom,pcie.txt          |  68 +++++++-
>>>  drivers/pci/host/pcie-qcom.c                       | 177
>>> ++++++++++++++++++++-
>>>  2 files changed, 239 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>> b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>> index 4059a6f..4a0538d 100644
>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
>>> @@ -7,6 +7,7 @@
>>>                         - "qcom,pcie-ipq8064" for ipq8064
>>>                         - "qcom,pcie-apq8064" for apq8064
>>>                         - "qcom,pcie-apq8084" for apq8084
>>> +                       - "qcom,pcie-msm8996" for msm8996 or apq8096
>>
>>
>> Since this works for both apq8096 and msm8996, compatible -
>> "qcom,pcie-apq8096" for uniformity ?
>
>
> AFAIK, compatible is selected based on SOC on which this IP is integrated
> first, So msm8996 seems to be correct, in that way.
>
> Also if we look at clk controller compatible strings, you would see them as
> *msm8996* rather than *8096*.

ok, cool. I didn't notice that. This looks good then.


Thanks
Vivek

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project