[PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[John Crispin]

Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
1/2, one lane, one PCIe root complex with support for MSI and legacy
interrupts, and it conforms to PCI Express Base 2.1 specification.

The core init is the sames as for the MSM8996, however the clocks and
reset lines differ.

Signed-off-by: John Crispin <john@phrozen.org>
---
 .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-
 drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++
 2 files changed, 323 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index e15f9b19901f..9d418b71774f 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -8,6 +8,7 @@
 			- "qcom,pcie-apq8064" for apq8064
 			- "qcom,pcie-apq8084" for apq8084
 			- "qcom,pcie-msm8996" for msm8996 or apq8096
+			- "qcom,pcie-ipq4019" for ipq4019
 
 - reg:
 	Usage: required
@@ -87,7 +88,7 @@
 			- "core"	Clocks the pcie hw block
 			- "phy"		Clocks the pcie PHY block
 - clock-names:
-	Usage: required for apq8084
+	Usage: required for apq8084/ipq4019
 	Value type: <stringlist>
 	Definition: Should contain the following entries
 			- "aux"		Auxiliary (AUX) clock
@@ -126,6 +127,23 @@
 	Definition: Should contain the following entries
 			- "core" Core reset
 
+- reset-names:
+	Usage: required for ipq/apq8064
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "axi_m"		AXI master reset
+			- "axi_s"		AXI slave reset
+			- "pipe"		PIPE reset
+			- "axi_m_vmid"		VMID reset
+			- "axi_s_xpu"		XPU reset
+			- "parf"		PARF reset
+			- "phy"			PHY reset
+			- "axi_m_sticky"	AXI sticky reset
+			- "pipe_sticky"		PIPE sticky reset
+			- "pwr"			PWR reset
+			- "ahb"			AHB reset
+			- "phy_ahb"		PHY AHB reset
+
 - power-domains:
 	Usage: required for apq8084 and msm8996/apq8096
 	Value type: <prop-encoded-array>
diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index 734ba0d4a5c8..af2bd552aadf 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -86,10 +86,29 @@ struct qcom_pcie_resources_v2 {
 	struct clk *pipe_clk;
 };
 
+struct qcom_pcie_resources_v3 {
+	struct clk *aux_clk;
+	struct clk *master_clk;
+	struct clk *slave_clk;
+	struct reset_control *axi_m_reset;
+	struct reset_control *axi_s_reset;
+	struct reset_control *pipe_reset;
+	struct reset_control *axi_m_vmid_reset;
+	struct reset_control *axi_s_xpu_reset;
+	struct reset_control *parf_reset;
+	struct reset_control *phy_reset;
+	struct reset_control *axi_m_sticky_reset;
+	struct reset_control *pipe_sticky_reset;
+	struct reset_control *pwr_reset;
+	struct reset_control *ahb_reset;
+	struct reset_control *phy_ahb_reset;
+};
+
 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_resources_v3 v3;
 };
 
 struct qcom_pcie;
@@ -563,6 +582,283 @@ static int qcom_pcie_post_init_v2(struct qcom_pcie *pcie)
 	return 0;
 }
 
+static int qcom_pcie_get_resources_v3(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v3 *res = &pcie->res.v3;
+	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->master_clk = devm_clk_get(dev, "master_bus");
+	if (IS_ERR(res->master_clk))
+		return PTR_ERR(res->master_clk);
+
+	res->slave_clk = devm_clk_get(dev, "slave_bus");
+	if (IS_ERR(res->slave_clk))
+		return PTR_ERR(res->slave_clk);
+
+	res->axi_m_reset = devm_reset_control_get(dev, "axi_m");
+	if (IS_ERR(res->axi_m_reset))
+		return PTR_ERR(res->axi_m_reset);
+
+	res->axi_s_reset = devm_reset_control_get(dev, "axi_s");
+	if (IS_ERR(res->axi_s_reset))
+		return PTR_ERR(res->axi_s_reset);
+
+	res->pipe_reset = devm_reset_control_get(dev, "pipe");
+	if (IS_ERR(res->pipe_reset))
+		return PTR_ERR(res->pipe_reset);
+
+	res->axi_m_vmid_reset = devm_reset_control_get(dev, "axi_m_vmid");
+	if (IS_ERR(res->axi_m_vmid_reset))
+		return PTR_ERR(res->axi_m_vmid_reset);
+
+	res->axi_s_xpu_reset = devm_reset_control_get(dev, "axi_s_xpu");
+	if (IS_ERR(res->axi_s_xpu_reset))
+		return PTR_ERR(res->axi_s_xpu_reset);
+
+	res->parf_reset = devm_reset_control_get(dev, "parf");
+	if (IS_ERR(res->parf_reset))
+		return PTR_ERR(res->parf_reset);
+
+	res->phy_reset = devm_reset_control_get(dev, "phy");
+	if (IS_ERR(res->phy_reset))
+		return PTR_ERR(res->phy_reset);
+
+	res->axi_m_sticky_reset = devm_reset_control_get(dev, "axi_m_sticky");
+	if (IS_ERR(res->axi_m_sticky_reset))
+		return PTR_ERR(res->axi_m_sticky_reset);
+
+	res->pipe_sticky_reset = devm_reset_control_get(dev, "pipe_sticky");
+	if (IS_ERR(res->pipe_sticky_reset))
+		return PTR_ERR(res->pipe_sticky_reset);
+
+	res->pwr_reset = devm_reset_control_get(dev, "pwr");
+	if (IS_ERR(res->pwr_reset))
+		return PTR_ERR(res->pwr_reset);
+
+	res->ahb_reset = devm_reset_control_get(dev, "ahb");
+	if (IS_ERR(res->ahb_reset))
+		return PTR_ERR(res->ahb_reset);
+
+	res->phy_ahb_reset = devm_reset_control_get(dev, "phy_ahb");
+	if (IS_ERR(res->phy_ahb_reset))
+		return PTR_ERR(res->phy_ahb_reset);
+
+	return 0;
+}
+
+static void qcom_pcie_deinit_v3(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v3 *res = &pcie->res.v3;
+
+	reset_control_assert(res->axi_m_reset);
+	reset_control_assert(res->axi_s_reset);
+	reset_control_assert(res->pipe_reset);
+	reset_control_assert(res->pipe_sticky_reset);
+	reset_control_assert(res->phy_reset);
+	reset_control_assert(res->phy_ahb_reset);
+	reset_control_assert(res->axi_m_sticky_reset);
+	reset_control_assert(res->pwr_reset);
+	reset_control_assert(res->ahb_reset);
+	clk_disable_unprepare(res->aux_clk);
+	clk_disable_unprepare(res->master_clk);
+	clk_disable_unprepare(res->slave_clk);
+}
+
+static int qcom_pcie_init_v3(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v3 *res = &pcie->res.v3;
+	struct device *dev = pcie->pp.dev;
+	u32 val;
+	int ret;
+
+	ret = reset_control_assert(res->axi_m_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert axi master reset\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(res->axi_s_reset);
+	if (ret) {
+		dev_err(dev, "cannot asser axi slave reset\n");
+		return ret;
+	}
+
+	usleep_range(10000, 12000);
+
+	ret = reset_control_assert(res->pipe_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert pipe reset\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(res->pipe_sticky_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert pipe sticky reset\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(res->phy_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert phy reset\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(res->phy_ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert phy ahb reset\n");
+		return ret;
+	}
+
+	usleep_range(10000, 12000);
+
+	ret = reset_control_assert(res->axi_m_sticky_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert axi master sticky reset\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(res->pwr_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert power reset\n");
+		return ret;
+	}
+
+	ret = reset_control_assert(res->ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert ahb reset\n");
+		return ret;
+	}
+
+	usleep_range(10000, 12000);
+
+	ret = reset_control_deassert(res->phy_ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert phy ahb reset\n");
+		return ret;
+	}
+
+	ret = reset_control_deassert(res->phy_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert phy reset\n");
+		goto err_rst_phy;
+	}
+
+	ret = reset_control_deassert(res->pipe_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert pipe reset\n");
+		goto err_rst_pipe;
+	}
+
+	ret = reset_control_deassert(res->pipe_sticky_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert pipe sticky reset\n");
+		goto err_rst_pipe_sticky;
+	}
+
+	usleep_range(10000, 12000);
+
+	ret = reset_control_deassert(res->axi_m_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert axi master reset\n");
+		goto err_rst_axi_m;
+	}
+
+	ret = reset_control_deassert(res->axi_m_sticky_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert axi master sticky reset\n");
+		goto err_rst_axi_m_sticky;
+	}
+
+	ret = reset_control_deassert(res->axi_s_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert axi slave reset\n");
+		goto err_rst_axi_s;
+	}
+
+	ret = reset_control_deassert(res->pwr_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert power reset\n");
+		goto err_rst_pwr;
+	}
+
+	ret = reset_control_deassert(res->ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert ahb reset\n");
+		goto err_rst_ahb;
+	}
+
+	usleep_range(10000, 12000);
+
+	ret = clk_prepare_enable(res->aux_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable iface clock\n");
+		goto err_clk_aux;
+	}
+
+	ret = clk_prepare_enable(res->master_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable core clock\n");
+		goto err_clk_axi_m;
+	}
+
+	ret = clk_prepare_enable(res->slave_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable phy clock\n");
+		goto err_clk_axi_s;
+	}
+
+	/* 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 + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
+	val |= BIT(31);
+	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
+
+	return 0;
+
+err_clk_axi_s:
+	clk_disable_unprepare(res->master_clk);
+err_clk_axi_m:
+	clk_disable_unprepare(res->aux_clk);
+err_clk_aux:
+	reset_control_assert(res->ahb_reset);
+err_rst_ahb:
+	reset_control_assert(res->pwr_reset);
+err_rst_pwr:
+	reset_control_assert(res->axi_s_reset);
+err_rst_axi_s:
+	reset_control_assert(res->axi_m_sticky_reset);
+err_rst_axi_m_sticky:
+	reset_control_assert(res->axi_m_reset);
+err_rst_axi_m:
+	reset_control_assert(res->pipe_sticky_reset);
+err_rst_pipe_sticky:
+	reset_control_assert(res->pipe_reset);
+err_rst_pipe:
+	reset_control_assert(res->phy_reset);
+err_rst_phy:
+	reset_control_assert(res->phy_ahb_reset);
+	return ret;
+}
+
 static int qcom_pcie_link_up(struct pcie_port *pp)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
@@ -661,6 +957,13 @@ static const struct qcom_pcie_ops ops_v2 = {
 	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
 };
 
+static const struct qcom_pcie_ops ops_v3 = {
+	.get_resources = qcom_pcie_get_resources_v3,
+	.init = qcom_pcie_init_v3,
+	.deinit = qcom_pcie_deinit_v3,
+	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
+};
+
 static int qcom_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -739,6 +1042,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-apq8064", .data = &ops_v0 },
 	{ .compatible = "qcom,pcie-apq8084", .data = &ops_v1 },
 	{ .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },
+	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_v3 },
 	{ }
 };
 
-- 
2.11.0

Re: [PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[Rob Herring]

On Fri, May 05, 2017 at 05:25:24PM +0200, John Crispin wrote:
> Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
> 1/2, one lane, one PCIe root complex with support for MSI and legacy
> interrupts, and it conforms to PCI Express Base 2.1 specification.
> 
> The core init is the sames as for the MSM8996, however the clocks and
> reset lines differ.
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-

For the binding:

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

>  drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++
>  2 files changed, 323 insertions(+), 1 deletion(-)


> +static int qcom_pcie_init_v3(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v3 *res = &pcie->res.v3;
> +	struct device *dev = pcie->pp.dev;
> +	u32 val;
> +	int ret;
> +
> +	ret = reset_control_assert(res->axi_m_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert axi master reset\n");

These all seem kind of verbose. Perhaps the error message should be in 
reset_control_assert function.

> +		return ret;
> +	}
> +
> +	ret = reset_control_assert(res->axi_s_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot asser axi slave reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	ret = reset_control_assert(res->pipe_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert pipe reset\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_assert(res->pipe_sticky_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert pipe sticky reset\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_assert(res->phy_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert phy reset\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_assert(res->phy_ahb_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert phy ahb reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	ret = reset_control_assert(res->axi_m_sticky_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert axi master sticky reset\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_assert(res->pwr_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert power reset\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_assert(res->ahb_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot assert ahb reset\n");
> +		return ret;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	ret = reset_control_deassert(res->phy_ahb_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert phy ahb reset\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_deassert(res->phy_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert phy reset\n");
> +		goto err_rst_phy;
> +	}
> +
> +	ret = reset_control_deassert(res->pipe_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert pipe reset\n");
> +		goto err_rst_pipe;
> +	}
> +
> +	ret = reset_control_deassert(res->pipe_sticky_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert pipe sticky reset\n");
> +		goto err_rst_pipe_sticky;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	ret = reset_control_deassert(res->axi_m_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert axi master reset\n");
> +		goto err_rst_axi_m;
> +	}
> +
> +	ret = reset_control_deassert(res->axi_m_sticky_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert axi master sticky reset\n");
> +		goto err_rst_axi_m_sticky;
> +	}
> +
> +	ret = reset_control_deassert(res->axi_s_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert axi slave reset\n");
> +		goto err_rst_axi_s;
> +	}
> +
> +	ret = reset_control_deassert(res->pwr_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert power reset\n");
> +		goto err_rst_pwr;
> +	}
> +
> +	ret = reset_control_deassert(res->ahb_reset);
> +	if (ret) {
> +		dev_err(dev, "cannot deassert ahb reset\n");
> +		goto err_rst_ahb;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	ret = clk_prepare_enable(res->aux_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable iface clock\n");
> +		goto err_clk_aux;
> +	}
> +
> +	ret = clk_prepare_enable(res->master_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable core clock\n");
> +		goto err_clk_axi_m;
> +	}
> +
> +	ret = clk_prepare_enable(res->slave_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable phy clock\n");
> +		goto err_clk_axi_s;
> +	}
> +
> +	/* 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 + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> +	val |= BIT(31);
> +	writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2);
> +
> +	return 0;
> +
> +err_clk_axi_s:
> +	clk_disable_unprepare(res->master_clk);
> +err_clk_axi_m:
> +	clk_disable_unprepare(res->aux_clk);


In deinit you turn off the clocks last.


> +err_clk_aux:
> +	reset_control_assert(res->ahb_reset);
> +err_rst_ahb:
> +	reset_control_assert(res->pwr_reset);
> +err_rst_pwr:
> +	reset_control_assert(res->axi_s_reset);
> +err_rst_axi_s:
> +	reset_control_assert(res->axi_m_sticky_reset);
> +err_rst_axi_m_sticky:
> +	reset_control_assert(res->axi_m_reset);
> +err_rst_axi_m:
> +	reset_control_assert(res->pipe_sticky_reset);
> +err_rst_pipe_sticky:
> +	reset_control_assert(res->pipe_reset);
> +err_rst_pipe:
> +	reset_control_assert(res->phy_reset);
> +err_rst_phy:
> +	reset_control_assert(res->phy_ahb_reset);
> +	return ret;
> +}
> +
>  static int qcom_pcie_link_up(struct pcie_port *pp)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pp);
> @@ -661,6 +957,13 @@ static const struct qcom_pcie_ops ops_v2 = {
>  	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
>  };
>  
> +static const struct qcom_pcie_ops ops_v3 = {
> +	.get_resources = qcom_pcie_get_resources_v3,
> +	.init = qcom_pcie_init_v3,
> +	.deinit = qcom_pcie_deinit_v3,
> +	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
> +};
> +
>  static int qcom_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -739,6 +1042,7 @@ static const struct of_device_id qcom_pcie_match[] = {
>  	{ .compatible = "qcom,pcie-apq8064", .data = &ops_v0 },
>  	{ .compatible = "qcom,pcie-apq8084", .data = &ops_v1 },
>  	{ .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },
> +	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_v3 },
>  	{ }
>  };
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[Bjorn Helgaas]

Stanimir?

On Fri, May 05, 2017 at 05:25:24PM +0200, John Crispin wrote:
> Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
> 1/2, one lane, one PCIe root complex with support for MSI and legacy
> interrupts, and it conforms to PCI Express Base 2.1 specification.
> 
> The core init is the sames as for the MSM8996, however the clocks and
> reset lines differ.
> 
> Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-
>  drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++

pcie-qcom.c was recently moved to drivers/pci/dwc, but I was able to
apply this by hand pretty easily.  But I would like Stanimir's ack
first.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[Bjorn Helgaas]

Stanimir?

On Fri, May 05, 2017 at 05:25:24PM +0200, John Crispin wrote:
> Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
> 1/2, one lane, one PCIe root complex with support for MSI and legacy
> interrupts, and it conforms to PCI Express Base 2.1 specification.
> 
> The core init is the sames as for the MSM8996, however the clocks and
> reset lines differ.
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-
>  drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++

pcie-qcom.c was recently moved to drivers/pci/dwc, but I was able to
apply this by hand pretty easily.  But I would like Stanimir's ack
first.

Bjorn

Re: [PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[Stanimir Varbanov]

Hi,

On 23.05.2017 23:07, Bjorn Helgaas wrote:
> Stanimir?
>
> On Fri, May 05, 2017 at 05:25:24PM +0200, John Crispin wrote:
>> Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
>> 1/2, one lane, one PCIe root complex with support for MSI and legacy
>> interrupts, and it conforms to PCI Express Base 2.1 specification.
>>
>> The core init is the sames as for the MSM8996, however the clocks and
>> reset lines differ.
>>
>> Signed-off-by: John Crispin <john@phrozen.org>
>> ---
>>  .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-
>>  drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++
>
> pcie-qcom.c was recently moved to drivers/pci/dwc, but I was able to
> apply this by hand pretty easily.  But I would like Stanimir's ack
> first.

Bjorn, thanks for the reminder.

Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>

regards,
Stan

Re: [PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[Bjorn Helgaas]

On Wed, May 24, 2017 at 02:31:27AM +0300, Stanimir Varbanov wrote:
> Hi,
> 
> On 23.05.2017 23:07, Bjorn Helgaas wrote:
> >Stanimir?
> >
> >On Fri, May 05, 2017 at 05:25:24PM +0200, John Crispin wrote:
> >>Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
> >>1/2, one lane, one PCIe root complex with support for MSI and legacy
> >>interrupts, and it conforms to PCI Express Base 2.1 specification.
> >>
> >>The core init is the sames as for the MSM8996, however the clocks and
> >>reset lines differ.
> >>
> >>Signed-off-by: John Crispin <john@phrozen.org>
> >>---
> >> .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-
> >> drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++
> >
> >pcie-qcom.c was recently moved to drivers/pci/dwc, but I was able to
> >apply this by hand pretty easily.  But I would like Stanimir's ack
> >first.
> 
> Bjorn, thanks for the reminder.
> 
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>

Thanks.

Rob took the trouble to provide useful comments, so I'm a bit
disappointed that there hasn't been a response them.

Re the verbose error messages, I agree they make the code ugly.  I
think John copied the existing style, which is reasonable.  I don't
see a good way to move the message into the callee, e.g.,
reset_control_assert(), because that's a generic function with
hundreds of callers.  But most of those callers don't check the return
value.  I'm not sure whether there's really any value in checking
here.

Re the ordering in the deinit() functions vs the init() error paths,
the new v3 code uses the same ordering as the existing v1 code, i.e.,

  qcom_pcie_deinit_v3
    reset_control_assert
    clk_disable_unprepare

  qcom_pcie_init_v3
      reset_control_deassert
      clk_prepare_enable
    err:
      clk_disable_unprepare
      reset_control_assert

In general, there's just a worrying lack of symmetry across the init,
deinit, and error paths of all the versions.

I would also propose the patch below, which doesn't change any code,
but moves a few functions so all the v0 code is together, all the v1
code is together and in the same order, etc.

Bjorn



commit ebacba9579462bdc009f4cfc1b605b3ca7a7a1d2
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Wed May 24 15:19:36 2017 -0500

    PCI: qcom: Reorder to put v0 functions together, v1 functions together, etc
    
    Previously the v0, v1, and v2 functions were not grouped together in a
    consistent order.  Reorder them to make them consistent.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c
index 9b186518cb72..5872a6771ea5 100644
--- a/drivers/pci/dwc/pcie-qcom.c
+++ b/drivers/pci/dwc/pcie-qcom.c
@@ -152,26 +152,6 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
 	return dw_handle_msi_irq(pp);
 }
 
-static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
-{
-	u32 val;
-
-	/* 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)
 {
 	struct dw_pcie *pci = pcie->pci;
@@ -186,6 +166,16 @@ static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
 	return dw_pcie_wait_for_link(pci);
 }
 
+static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
+{
+	u32 val;
+
+	/* 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 int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
@@ -236,36 +226,6 @@ static int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
 	return PTR_ERR_OR_ZERO(res->phy_reset);
 }
 
-static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
-	struct dw_pcie *pci = pcie->pci;
-	struct device *dev = pci->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");
-	return PTR_ERR_OR_ZERO(res->core);
-}
-
 static void qcom_pcie_deinit_v0(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
@@ -394,6 +354,36 @@ static int qcom_pcie_init_v0(struct qcom_pcie *pcie)
 	return ret;
 }
 
+static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
+	struct dw_pcie *pci = pcie->pci;
+	struct device *dev = pci->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");
+	return PTR_ERR_OR_ZERO(res->core);
+}
+
 static void qcom_pcie_deinit_v1(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
@@ -474,6 +464,16 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
 	return ret;
 }
 
+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_get_resources_v2(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
@@ -500,6 +500,17 @@ static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
 	return PTR_ERR_OR_ZERO(res->pipe_clk);
 }
 
+static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+
+	clk_disable_unprepare(res->pipe_clk);
+	clk_disable_unprepare(res->slave_clk);
+	clk_disable_unprepare(res->master_clk);
+	clk_disable_unprepare(res->cfg_clk);
+	clk_disable_unprepare(res->aux_clk);
+}
+
 static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
@@ -865,17 +876,6 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
 	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->pipe_clk);
-	clk_disable_unprepare(res->slave_clk);
-	clk_disable_unprepare(res->master_clk);
-	clk_disable_unprepare(res->cfg_clk);
-	clk_disable_unprepare(res->aux_clk);
-}
-
 static void qcom_pcie_host_init(struct pcie_port *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

Re: [PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[Stanimir Varbanov]

Hi Bjorn,

On 25.05.2017 00:04, Bjorn Helgaas wrote:
> On Wed, May 24, 2017 at 02:31:27AM +0300, Stanimir Varbanov wrote:
>> Hi,
>>
>> On 23.05.2017 23:07, Bjorn Helgaas wrote:
>>> Stanimir?
>>>
>>> On Fri, May 05, 2017 at 05:25:24PM +0200, John Crispin wrote:
>>>> Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
>>>> 1/2, one lane, one PCIe root complex with support for MSI and legacy
>>>> interrupts, and it conforms to PCI Express Base 2.1 specification.
>>>>
>>>> The core init is the sames as for the MSM8996, however the clocks and
>>>> reset lines differ.
>>>>
>>>> Signed-off-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
>>>> ---
>>>> .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-
>>>> drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++
>>>
>>> pcie-qcom.c was recently moved to drivers/pci/dwc, but I was able to
>>> apply this by hand pretty easily.  But I would like Stanimir's ack
>>> first.
>>
>> Bjorn, thanks for the reminder.
>>
>> Acked-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
>
> Thanks.
>
> Rob took the trouble to provide useful comments, so I'm a bit
> disappointed that there hasn't been a response them.
>
> Re the verbose error messages, I agree they make the code ugly.  I
> think John copied the existing style, which is reasonable.  I don't
> see a good way to move the message into the callee, e.g.,
> reset_control_assert(), because that's a generic function with
> hundreds of callers.  But most of those callers don't check the return
> value.  I'm not sure whether there's really any value in checking
> here.

I have an idea how all those reset controls will become more beautiful. 
I'm just waiting this [1] to me merged. When this happened I will post a 
cleanup patch for resets and error messages.

>
> Re the ordering in the deinit() functions vs the init() error paths,
> the new v3 code uses the same ordering as the existing v1 code, i.e.,
>
>   qcom_pcie_deinit_v3
>     reset_control_assert
>     clk_disable_unprepare
>
>   qcom_pcie_init_v3
>       reset_control_deassert
>       clk_prepare_enable
>     err:
>       clk_disable_unprepare
>       reset_control_assert
>
> In general, there's just a worrying lack of symmetry across the init,
> deinit, and error paths of all the versions.

Probably the order doesn't matter in deinit case so let's keep it as is 
in John's patch.

>
> I would also propose the patch below, which doesn't change any code,
> but moves a few functions so all the v0 code is together, all the v1
> code is together and in the same order, etc.

Thanks, looks good to me.

regards,
Stan

[1] https://lkml.org/lkml/2017/5/22/273

>
> Bjorn
>
>
>
> commit ebacba9579462bdc009f4cfc1b605b3ca7a7a1d2
> Author: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Date:   Wed May 24 15:19:36 2017 -0500
>
>     PCI: qcom: Reorder to put v0 functions together, v1 functions together, etc
>
>     Previously the v0, v1, and v2 functions were not grouped together in a
>     consistent order.  Reorder them to make them consistent.
>
>     Signed-off-by: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c
> index 9b186518cb72..5872a6771ea5 100644
> --- a/drivers/pci/dwc/pcie-qcom.c
> +++ b/drivers/pci/dwc/pcie-qcom.c
> @@ -152,26 +152,6 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
>  	return dw_handle_msi_irq(pp);
>  }
>
> -static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
> -{
> -	u32 val;
> -
> -	/* 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)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> @@ -186,6 +166,16 @@ static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
>  	return dw_pcie_wait_for_link(pci);
>  }
>
> +static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
> +{
> +	u32 val;
> +
> +	/* 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 int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
> @@ -236,36 +226,6 @@ static int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
>  	return PTR_ERR_OR_ZERO(res->phy_reset);
>  }
>
> -static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
> -{
> -	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
> -	struct dw_pcie *pci = pcie->pci;
> -	struct device *dev = pci->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");
> -	return PTR_ERR_OR_ZERO(res->core);
> -}
> -
>  static void qcom_pcie_deinit_v0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
> @@ -394,6 +354,36 @@ static int qcom_pcie_init_v0(struct qcom_pcie *pcie)
>  	return ret;
>  }
>
> +static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->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");
> +	return PTR_ERR_OR_ZERO(res->core);
> +}
> +
>  static void qcom_pcie_deinit_v1(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
> @@ -474,6 +464,16 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
>  	return ret;
>  }
>
> +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_get_resources_v2(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> @@ -500,6 +500,17 @@ static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
>  	return PTR_ERR_OR_ZERO(res->pipe_clk);
>  }
>
> +static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +
> +	clk_disable_unprepare(res->pipe_clk);
> +	clk_disable_unprepare(res->slave_clk);
> +	clk_disable_unprepare(res->master_clk);
> +	clk_disable_unprepare(res->cfg_clk);
> +	clk_disable_unprepare(res->aux_clk);
> +}
> +
>  static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> @@ -865,17 +876,6 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
>  	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->pipe_clk);
> -	clk_disable_unprepare(res->slave_clk);
> -	clk_disable_unprepare(res->master_clk);
> -	clk_disable_unprepare(res->cfg_clk);
> -	clk_disable_unprepare(res->aux_clk);
> -}
> -
>  static void qcom_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: qcom: Add support for IPQ4019 PCIe controller

[Stanimir Varbanov]

Hi Bjorn,

On 25.05.2017 00:04, Bjorn Helgaas wrote:
> On Wed, May 24, 2017 at 02:31:27AM +0300, Stanimir Varbanov wrote:
>> Hi,
>>
>> On 23.05.2017 23:07, Bjorn Helgaas wrote:
>>> Stanimir?
>>>
>>> On Fri, May 05, 2017 at 05:25:24PM +0200, John Crispin wrote:
>>>> Add support for the IPQ4019 PCIe controller. IPQ4019 supports Gen
>>>> 1/2, one lane, one PCIe root complex with support for MSI and legacy
>>>> interrupts, and it conforms to PCI Express Base 2.1 specification.
>>>>
>>>> The core init is the sames as for the MSM8996, however the clocks and
>>>> reset lines differ.
>>>>
>>>> Signed-off-by: John Crispin <john@phrozen.org>
>>>> ---
>>>> .../devicetree/bindings/pci/qcom,pcie.txt          |  20 +-
>>>> drivers/pci/host/pcie-qcom.c                       | 304 +++++++++++++++++++++
>>>
>>> pcie-qcom.c was recently moved to drivers/pci/dwc, but I was able to
>>> apply this by hand pretty easily.  But I would like Stanimir's ack
>>> first.
>>
>> Bjorn, thanks for the reminder.
>>
>> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>
> Thanks.
>
> Rob took the trouble to provide useful comments, so I'm a bit
> disappointed that there hasn't been a response them.
>
> Re the verbose error messages, I agree they make the code ugly.  I
> think John copied the existing style, which is reasonable.  I don't
> see a good way to move the message into the callee, e.g.,
> reset_control_assert(), because that's a generic function with
> hundreds of callers.  But most of those callers don't check the return
> value.  I'm not sure whether there's really any value in checking
> here.

I have an idea how all those reset controls will become more beautiful. 
I'm just waiting this [1] to me merged. When this happened I will post a 
cleanup patch for resets and error messages.

>
> Re the ordering in the deinit() functions vs the init() error paths,
> the new v3 code uses the same ordering as the existing v1 code, i.e.,
>
>   qcom_pcie_deinit_v3
>     reset_control_assert
>     clk_disable_unprepare
>
>   qcom_pcie_init_v3
>       reset_control_deassert
>       clk_prepare_enable
>     err:
>       clk_disable_unprepare
>       reset_control_assert
>
> In general, there's just a worrying lack of symmetry across the init,
> deinit, and error paths of all the versions.

Probably the order doesn't matter in deinit case so let's keep it as is 
in John's patch.

>
> I would also propose the patch below, which doesn't change any code,
> but moves a few functions so all the v0 code is together, all the v1
> code is together and in the same order, etc.

Thanks, looks good to me.

regards,
Stan

[1] https://lkml.org/lkml/2017/5/22/273

>
> Bjorn
>
>
>
> commit ebacba9579462bdc009f4cfc1b605b3ca7a7a1d2
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Wed May 24 15:19:36 2017 -0500
>
>     PCI: qcom: Reorder to put v0 functions together, v1 functions together, etc
>
>     Previously the v0, v1, and v2 functions were not grouped together in a
>     consistent order.  Reorder them to make them consistent.
>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c
> index 9b186518cb72..5872a6771ea5 100644
> --- a/drivers/pci/dwc/pcie-qcom.c
> +++ b/drivers/pci/dwc/pcie-qcom.c
> @@ -152,26 +152,6 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
>  	return dw_handle_msi_irq(pp);
>  }
>
> -static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
> -{
> -	u32 val;
> -
> -	/* 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)
>  {
>  	struct dw_pcie *pci = pcie->pci;
> @@ -186,6 +166,16 @@ static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
>  	return dw_pcie_wait_for_link(pci);
>  }
>
> +static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
> +{
> +	u32 val;
> +
> +	/* 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 int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
> @@ -236,36 +226,6 @@ static int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
>  	return PTR_ERR_OR_ZERO(res->phy_reset);
>  }
>
> -static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
> -{
> -	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
> -	struct dw_pcie *pci = pcie->pci;
> -	struct device *dev = pci->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");
> -	return PTR_ERR_OR_ZERO(res->core);
> -}
> -
>  static void qcom_pcie_deinit_v0(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
> @@ -394,6 +354,36 @@ static int qcom_pcie_init_v0(struct qcom_pcie *pcie)
>  	return ret;
>  }
>
> +static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
> +	struct dw_pcie *pci = pcie->pci;
> +	struct device *dev = pci->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");
> +	return PTR_ERR_OR_ZERO(res->core);
> +}
> +
>  static void qcom_pcie_deinit_v1(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
> @@ -474,6 +464,16 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
>  	return ret;
>  }
>
> +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_get_resources_v2(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> @@ -500,6 +500,17 @@ static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
>  	return PTR_ERR_OR_ZERO(res->pipe_clk);
>  }
>
> +static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +
> +	clk_disable_unprepare(res->pipe_clk);
> +	clk_disable_unprepare(res->slave_clk);
> +	clk_disable_unprepare(res->master_clk);
> +	clk_disable_unprepare(res->cfg_clk);
> +	clk_disable_unprepare(res->aux_clk);
> +}
> +
>  static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
>  {
>  	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> @@ -865,17 +876,6 @@ static int qcom_pcie_link_up(struct dw_pcie *pci)
>  	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->pipe_clk);
> -	clk_disable_unprepare(res->slave_clk);
> -	clk_disable_unprepare(res->master_clk);
> -	clk_disable_unprepare(res->cfg_clk);
> -	clk_disable_unprepare(res->aux_clk);
> -}
> -
>  static void qcom_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>