LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 0/3] Qualcomm QCS404 PCIe support
@ 2019-05-02  0:19 Bjorn Andersson
  2019-05-02  0:19 ` [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers Bjorn Andersson
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Bjorn Andersson @ 2019-05-02  0:19 UTC (permalink / raw)
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Stanimir Varbanov,
	Lorenzo Pieralisi, linux-arm-msm, linux-pci, devicetree,
	linux-kernel

This series adds support for the PCIe controller in the Qualcomm QCS404
platform.

Bjorn Andersson (3):
  PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  dt-bindings: PCI: qcom: Add QCS404 to the binding
  PCI: qcom: Add QCS404 PCIe controller support

 .../devicetree/bindings/pci/qcom,pcie.txt     |  25 +++-
 drivers/pci/controller/dwc/pcie-qcom.c        | 109 ++++++++----------
 2 files changed, 73 insertions(+), 61 deletions(-)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-02  0:19 [PATCH v3 0/3] Qualcomm QCS404 PCIe support Bjorn Andersson
@ 2019-05-02  0:19 ` Bjorn Andersson
  2019-05-02 11:53   ` Vinod Koul
  2019-05-16  9:14   ` Stanimir Varbanov
  2019-05-02  0:19 ` [PATCH v3 2/3] dt-bindings: PCI: qcom: Add QCS404 to the binding Bjorn Andersson
  2019-05-02  0:19 ` [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support Bjorn Andersson
  2 siblings, 2 replies; 14+ messages in thread
From: Bjorn Andersson @ 2019-05-02  0:19 UTC (permalink / raw)
  To: Bjorn Helgaas, Stanimir Varbanov, Lorenzo Pieralisi
  Cc: Rob Herring, Mark Rutland, linux-arm-msm, linux-pci, devicetree,
	linux-kernel

Before introducing the QCS404 platform, which uses the same PCIe
controller as IPQ4019, migrate this to use the bulk clock API, in order
to make the error paths slighly cleaner.

Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- Defined QCOM_PCIE_2_4_0_MAX_CLOCKS

 drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
 1 file changed, 14 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 0ed235d560e3..d740cbe0e56d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
 };
 
+#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
 struct qcom_pcie_resources_2_4_0 {
-	struct clk *aux_clk;
-	struct clk *master_clk;
-	struct clk *slave_clk;
+	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
+	int num_clks;
 	struct reset_control *axi_m_reset;
 	struct reset_control *axi_s_reset;
 	struct reset_control *pipe_reset;
@@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
+	int ret;
 
-	res->aux_clk = devm_clk_get(dev, "aux");
-	if (IS_ERR(res->aux_clk))
-		return PTR_ERR(res->aux_clk);
+	res->clks[0].id = "aux";
+	res->clks[1].id = "master_bus";
+	res->clks[2].id = "slave_bus";
 
-	res->master_clk = devm_clk_get(dev, "master_bus");
-	if (IS_ERR(res->master_clk))
-		return PTR_ERR(res->master_clk);
+	res->num_clks = 3;
 
-	res->slave_clk = devm_clk_get(dev, "slave_bus");
-	if (IS_ERR(res->slave_clk))
-		return PTR_ERR(res->slave_clk);
+	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
+	if (ret < 0)
+		return ret;
 
 	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
 	if (IS_ERR(res->axi_m_reset))
@@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
 	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);
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
 }
 
 static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
@@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
 
 	usleep_range(10000, 12000);
 
-	ret = clk_prepare_enable(res->aux_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable iface clock\n");
+	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
+	if (ret)
 		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);
@@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
 
 	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:
-- 
2.18.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 2/3] dt-bindings: PCI: qcom: Add QCS404 to the binding
  2019-05-02  0:19 [PATCH v3 0/3] Qualcomm QCS404 PCIe support Bjorn Andersson
  2019-05-02  0:19 ` [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers Bjorn Andersson
@ 2019-05-02  0:19 ` Bjorn Andersson
  2019-05-02 11:54   ` Vinod Koul
  2019-05-02  0:19 ` [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support Bjorn Andersson
  2 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2019-05-02  0:19 UTC (permalink / raw)
  To: Bjorn Helgaas, Rob Herring, Mark Rutland, Stanimir Varbanov,
	Lorenzo Pieralisi
  Cc: linux-arm-msm, linux-pci, devicetree, linux-kernel

The Qualcomm QCS404 platform contains a PCIe controller, add this to the
Qualcomm PCI binding document. The controller is the same version as the
one used in IPQ4019, but the PHY part is described separately, hence the
difference in clocks and resets.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- None

 .../devicetree/bindings/pci/qcom,pcie.txt     | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 1fd703bd73e0..ada80b01bf0c 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -10,6 +10,7 @@
 			- "qcom,pcie-msm8996" for msm8996 or apq8096
 			- "qcom,pcie-ipq4019" for ipq4019
 			- "qcom,pcie-ipq8074" for ipq8074
+			- "qcom,pcie-qcs404" for qcs404
 
 - reg:
 	Usage: required
@@ -116,6 +117,15 @@
 			- "ahb"		AHB clock
 			- "aux"		Auxiliary clock
 
+- clock-names:
+	Usage: required for qcs404
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "iface"	AHB clock
+			- "aux"		Auxiliary clock
+			- "master_bus"	AXI Master clock
+			- "slave_bus"	AXI Slave clock
+
 - resets:
 	Usage: required
 	Value type: <prop-encoded-array>
@@ -167,6 +177,17 @@
 			- "ahb"			AHB Reset
 			- "axi_m_sticky"	AXI Master Sticky reset
 
+- reset-names:
+	Usage: required for qcs404
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "axi_m"		AXI Master reset
+			- "axi_s"		AXI Slave reset
+			- "axi_m_sticky"	AXI Master Sticky reset
+			- "pipe_sticky"		PIPE sticky reset
+			- "pwr"			PWR reset
+			- "ahb"			AHB reset
+
 - power-domains:
 	Usage: required for apq8084 and msm8996/apq8096
 	Value type: <prop-encoded-array>
@@ -195,12 +216,12 @@
 	Definition: A phandle to the PCIe endpoint power supply
 
 - phys:
-	Usage: required for apq8084
+	Usage: required for apq8084 and qcs404
 	Value type: <phandle>
 	Definition: List of phandle(s) as listed in phy-names property
 
 - phy-names:
-	Usage: required for apq8084
+	Usage: required for apq8084 and qcs404
 	Value type: <stringlist>
 	Definition: Should contain "pciephy"
 
-- 
2.18.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support
  2019-05-02  0:19 [PATCH v3 0/3] Qualcomm QCS404 PCIe support Bjorn Andersson
  2019-05-02  0:19 ` [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers Bjorn Andersson
  2019-05-02  0:19 ` [PATCH v3 2/3] dt-bindings: PCI: qcom: Add QCS404 to the binding Bjorn Andersson
@ 2019-05-02  0:19 ` Bjorn Andersson
  2019-05-02 11:59   ` Vinod Koul
  2019-05-16  9:39   ` Stanimir Varbanov
  2 siblings, 2 replies; 14+ messages in thread
From: Bjorn Andersson @ 2019-05-02  0:19 UTC (permalink / raw)
  To: Bjorn Helgaas, Stanimir Varbanov, Lorenzo Pieralisi
  Cc: Rob Herring, Mark Rutland, linux-arm-msm, linux-pci, devicetree,
	linux-kernel

The QCS404 platform contains a PCIe controller of version 2.4.0 and a
Qualcomm PCIe2 PHY. The driver already supports version 2.4.0, for the
IPQ4019, but this support touches clocks and resets related to the PHY
as well, and there's no upstream driver for the PHY.

On QCS404 we must initialize the PHY, so a separate PHY driver is
implemented to take care of this and the controller driver is updated to
not require the PHY related resources. This is done by relying on the
fact that operations in both the clock and reset framework are nops when
passed NULL, so we can isolate this change to only the get_resource
function.

For QCS404 we also need to enable the AHB (iface) clock, in order to
access the register space of the controller, but as this is not part of
the IPQ4019 DT binding this is only added for new users of the 2.4.0
controller.

Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- None

 drivers/pci/controller/dwc/pcie-qcom.c | 64 +++++++++++++++-----------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index d740cbe0e56d..d101bc5c0def 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -112,7 +112,7 @@ struct qcom_pcie_resources_2_3_2 {
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
 };
 
-#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
+#define QCOM_PCIE_2_4_0_MAX_CLOCKS	4
 struct qcom_pcie_resources_2_4_0 {
 	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
 	int num_clks;
@@ -638,13 +638,16 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
+	bool is_ipq = of_device_is_compatible(dev->of_node, "qcom,pcie-ipq4019");
 	int ret;
 
 	res->clks[0].id = "aux";
 	res->clks[1].id = "master_bus";
 	res->clks[2].id = "slave_bus";
+	res->clks[3].id = "iface";
 
-	res->num_clks = 3;
+	/* qcom,pcie-ipq4019 is defined without "iface" */
+	res->num_clks = is_ipq ? 3 : 4;
 
 	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
 	if (ret < 0)
@@ -658,27 +661,33 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
 	if (IS_ERR(res->axi_s_reset))
 		return PTR_ERR(res->axi_s_reset);
 
-	res->pipe_reset = devm_reset_control_get_exclusive(dev, "pipe");
-	if (IS_ERR(res->pipe_reset))
-		return PTR_ERR(res->pipe_reset);
-
-	res->axi_m_vmid_reset = devm_reset_control_get_exclusive(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_exclusive(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_exclusive(dev, "parf");
-	if (IS_ERR(res->parf_reset))
-		return PTR_ERR(res->parf_reset);
-
-	res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
-	if (IS_ERR(res->phy_reset))
-		return PTR_ERR(res->phy_reset);
+	if (is_ipq) {
+		/*
+		 * These resources relates to the PHY or are secure clocks, but
+		 * are controlled here for IPQ4019
+		 */
+		res->pipe_reset = devm_reset_control_get_exclusive(dev, "pipe");
+		if (IS_ERR(res->pipe_reset))
+			return PTR_ERR(res->pipe_reset);
+
+		res->axi_m_vmid_reset = devm_reset_control_get_exclusive(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_exclusive(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_exclusive(dev, "parf");
+		if (IS_ERR(res->parf_reset))
+			return PTR_ERR(res->parf_reset);
+
+		res->phy_reset = devm_reset_control_get_exclusive(dev, "phy");
+		if (IS_ERR(res->phy_reset))
+			return PTR_ERR(res->phy_reset);
+	}
 
 	res->axi_m_sticky_reset = devm_reset_control_get_exclusive(dev,
 								   "axi_m_sticky");
@@ -698,9 +707,11 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
 	if (IS_ERR(res->ahb_reset))
 		return PTR_ERR(res->ahb_reset);
 
-	res->phy_ahb_reset = devm_reset_control_get_exclusive(dev, "phy_ahb");
-	if (IS_ERR(res->phy_ahb_reset))
-		return PTR_ERR(res->phy_ahb_reset);
+	if (is_ipq) {
+		res->phy_ahb_reset = devm_reset_control_get_exclusive(dev, "phy_ahb");
+		if (IS_ERR(res->phy_ahb_reset))
+			return PTR_ERR(res->phy_ahb_reset);
+	}
 
 	return 0;
 }
@@ -1268,6 +1279,7 @@ static const struct of_device_id qcom_pcie_match[] = {
 	{ .compatible = "qcom,pcie-msm8996", .data = &ops_2_3_2 },
 	{ .compatible = "qcom,pcie-ipq8074", .data = &ops_2_3_3 },
 	{ .compatible = "qcom,pcie-ipq4019", .data = &ops_2_4_0 },
+	{ .compatible = "qcom,pcie-qcs404", .data = &ops_2_4_0 },
 	{ }
 };
 
-- 
2.18.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-02  0:19 ` [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers Bjorn Andersson
@ 2019-05-02 11:53   ` Vinod Koul
  2019-05-02 15:00     ` Bjorn Andersson
  2019-05-16  9:14   ` Stanimir Varbanov
  1 sibling, 1 reply; 14+ messages in thread
From: Vinod Koul @ 2019-05-02 11:53 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Bjorn Helgaas, Stanimir Varbanov, Lorenzo Pieralisi, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-pci, devicetree, linux-kernel

On 01-05-19, 17:19, Bjorn Andersson wrote:
> Before introducing the QCS404 platform, which uses the same PCIe
> controller as IPQ4019, migrate this to use the bulk clock API, in order
> to make the error paths slighly cleaner.
> 
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v2:
> - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
>  1 file changed, 14 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..d740cbe0e56d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
>  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>  };
>  
> +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3

empty line after the define please

>  struct qcom_pcie_resources_2_4_0 {
> -	struct clk *aux_clk;
> -	struct clk *master_clk;
> -	struct clk *slave_clk;
> +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> +	int num_clks;
>  	struct reset_control *axi_m_reset;
>  	struct reset_control *axi_s_reset;
>  	struct reset_control *pipe_reset;
> @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	int ret;
>  
> -	res->aux_clk = devm_clk_get(dev, "aux");
> -	if (IS_ERR(res->aux_clk))
> -		return PTR_ERR(res->aux_clk);
> +	res->clks[0].id = "aux";
> +	res->clks[1].id = "master_bus";
> +	res->clks[2].id = "slave_bus";
>  
> -	res->master_clk = devm_clk_get(dev, "master_bus");
> -	if (IS_ERR(res->master_clk))
> -		return PTR_ERR(res->master_clk);
> +	res->num_clks = 3;
>  
> -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> -	if (IS_ERR(res->slave_clk))
> -		return PTR_ERR(res->slave_clk);
> +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> +	if (ret < 0)
> +		return ret;
>  
>  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
>  	if (IS_ERR(res->axi_m_reset))
> @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
>  	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);
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>  
>  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	usleep_range(10000, 12000);
>  
> -	ret = clk_prepare_enable(res->aux_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable iface clock\n");
> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> +	if (ret)
>  		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);
> @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	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:
> -- 
> 2.18.0


rest lgtm:

Reviewed-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 2/3] dt-bindings: PCI: qcom: Add QCS404 to the binding
  2019-05-02  0:19 ` [PATCH v3 2/3] dt-bindings: PCI: qcom: Add QCS404 to the binding Bjorn Andersson
@ 2019-05-02 11:54   ` Vinod Koul
  0 siblings, 0 replies; 14+ messages in thread
From: Vinod Koul @ 2019-05-02 11:54 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Bjorn Helgaas, Rob Herring, Mark Rutland, Stanimir Varbanov,
	Lorenzo Pieralisi, linux-arm-msm, linux-pci, devicetree,
	linux-kernel

On 01-05-19, 17:19, Bjorn Andersson wrote:
> The Qualcomm QCS404 platform contains a PCIe controller, add this to the
> Qualcomm PCI binding document. The controller is the same version as the
> one used in IPQ4019, but the PHY part is described separately, hence the
> difference in clocks and resets.

Reviewed-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support
  2019-05-02  0:19 ` [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support Bjorn Andersson
@ 2019-05-02 11:59   ` Vinod Koul
  2019-05-16  9:39   ` Stanimir Varbanov
  1 sibling, 0 replies; 14+ messages in thread
From: Vinod Koul @ 2019-05-02 11:59 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Bjorn Helgaas, Stanimir Varbanov, Lorenzo Pieralisi, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-pci, devicetree, linux-kernel

On 01-05-19, 17:19, Bjorn Andersson wrote:
> The QCS404 platform contains a PCIe controller of version 2.4.0 and a
> Qualcomm PCIe2 PHY. The driver already supports version 2.4.0, for the
> IPQ4019, but this support touches clocks and resets related to the PHY
> as well, and there's no upstream driver for the PHY.
> 
> On QCS404 we must initialize the PHY, so a separate PHY driver is
> implemented to take care of this and the controller driver is updated to
> not require the PHY related resources. This is done by relying on the
> fact that operations in both the clock and reset framework are nops when
> passed NULL, so we can isolate this change to only the get_resource
> function.
> 
> For QCS404 we also need to enable the AHB (iface) clock, in order to
> access the register space of the controller, but as this is not part of
> the IPQ4019 DT binding this is only added for new users of the 2.4.0
> controller.

Reviewed-by: Vinod Koul <vkoul@kernel.org>

-- 
~Vinod

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-02 11:53   ` Vinod Koul
@ 2019-05-02 15:00     ` Bjorn Andersson
  2019-05-03  3:10       ` Vinod Koul
  0 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2019-05-02 15:00 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Bjorn Helgaas, Stanimir Varbanov, Lorenzo Pieralisi, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-pci, devicetree, linux-kernel

On Thu 02 May 04:53 PDT 2019, Vinod Koul wrote:
> On 01-05-19, 17:19, Bjorn Andersson wrote:
[..]
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0ed235d560e3..d740cbe0e56d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> >  };
> >  
> > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> 
> empty line after the define please
> 

This follows the style of QCOM_PCIE_2_3_2_MAX_SUPPLY one block up, so
I think this is the way we want it.

> >  struct qcom_pcie_resources_2_4_0 {
[..]
> 
> 
> rest lgtm:
> 
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> 

Thanks!

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-02 15:00     ` Bjorn Andersson
@ 2019-05-03  3:10       ` Vinod Koul
  0 siblings, 0 replies; 14+ messages in thread
From: Vinod Koul @ 2019-05-03  3:10 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Bjorn Helgaas, Stanimir Varbanov, Lorenzo Pieralisi, Rob Herring,
	Mark Rutland, linux-arm-msm, linux-pci, devicetree, linux-kernel

On 02-05-19, 08:00, Bjorn Andersson wrote:
> On Thu 02 May 04:53 PDT 2019, Vinod Koul wrote:
> > On 01-05-19, 17:19, Bjorn Andersson wrote:
> [..]
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 0ed235d560e3..d740cbe0e56d 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> > >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> > >  };
> > >  
> > > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> > 
> > empty line after the define please
> > 
> 
> This follows the style of QCOM_PCIE_2_3_2_MAX_SUPPLY one block up, so
> I think this is the way we want it.

Okay sounds fine to me

> 
> > >  struct qcom_pcie_resources_2_4_0 {
> [..]
> > 
> > 
> > rest lgtm:
> > 
> > Reviewed-by: Vinod Koul <vkoul@kernel.org>
> > 
> 
> Thanks!
> 
> Regards,
> Bjorn

-- 
~Vinod

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-02  0:19 ` [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers Bjorn Andersson
  2019-05-02 11:53   ` Vinod Koul
@ 2019-05-16  9:14   ` Stanimir Varbanov
  2019-05-28 15:13     ` Lorenzo Pieralisi
  2019-05-29  0:58     ` Bjorn Andersson
  1 sibling, 2 replies; 14+ messages in thread
From: Stanimir Varbanov @ 2019-05-16  9:14 UTC (permalink / raw)
  To: Bjorn Andersson, Bjorn Helgaas, Lorenzo Pieralisi
  Cc: Rob Herring, Mark Rutland, linux-arm-msm, linux-pci, devicetree,
	linux-kernel

Hi Bjorn,

On 5/2/19 3:19 AM, Bjorn Andersson wrote:
> Before introducing the QCS404 platform, which uses the same PCIe
> controller as IPQ4019, migrate this to use the bulk clock API, in order
> to make the error paths slighly cleaner.
> 
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v2:
> - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
>  1 file changed, 14 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..d740cbe0e56d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
>  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>  };
>  
> +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
>  struct qcom_pcie_resources_2_4_0 {
> -	struct clk *aux_clk;
> -	struct clk *master_clk;
> -	struct clk *slave_clk;
> +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> +	int num_clks;
>  	struct reset_control *axi_m_reset;
>  	struct reset_control *axi_s_reset;
>  	struct reset_control *pipe_reset;
> @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	int ret;
>  
> -	res->aux_clk = devm_clk_get(dev, "aux");
> -	if (IS_ERR(res->aux_clk))
> -		return PTR_ERR(res->aux_clk);
> +	res->clks[0].id = "aux";
> +	res->clks[1].id = "master_bus";
> +	res->clks[2].id = "slave_bus";
>  
> -	res->master_clk = devm_clk_get(dev, "master_bus");
> -	if (IS_ERR(res->master_clk))
> -		return PTR_ERR(res->master_clk);
> +	res->num_clks = 3;

Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?

>  
> -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> -	if (IS_ERR(res->slave_clk))
> -		return PTR_ERR(res->slave_clk);
> +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> +	if (ret < 0)
> +		return ret;
>  
>  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
>  	if (IS_ERR(res->axi_m_reset))
> @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
>  	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);
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>  
>  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	usleep_range(10000, 12000);
>  
> -	ret = clk_prepare_enable(res->aux_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable iface clock\n");
> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> +	if (ret)
>  		goto err_clk_aux;

Maybe you have to change the name of the label too?

> -	}
> -
> -	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);
> @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	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:
> 

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support
  2019-05-02  0:19 ` [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support Bjorn Andersson
  2019-05-02 11:59   ` Vinod Koul
@ 2019-05-16  9:39   ` Stanimir Varbanov
  1 sibling, 0 replies; 14+ messages in thread
From: Stanimir Varbanov @ 2019-05-16  9:39 UTC (permalink / raw)
  To: Bjorn Andersson, Bjorn Helgaas, Lorenzo Pieralisi
  Cc: Rob Herring, Mark Rutland, linux-arm-msm, linux-pci, devicetree,
	linux-kernel

Hi Bjorn,

On 5/2/19 3:19 AM, Bjorn Andersson wrote:
> The QCS404 platform contains a PCIe controller of version 2.4.0 and a
> Qualcomm PCIe2 PHY. The driver already supports version 2.4.0, for the
> IPQ4019, but this support touches clocks and resets related to the PHY
> as well, and there's no upstream driver for the PHY.
> 
> On QCS404 we must initialize the PHY, so a separate PHY driver is
> implemented to take care of this and the controller driver is updated to
> not require the PHY related resources. This is done by relying on the
> fact that operations in both the clock and reset framework are nops when
> passed NULL, so we can isolate this change to only the get_resource
> function.
> 
> For QCS404 we also need to enable the AHB (iface) clock, in order to
> access the register space of the controller, but as this is not part of
> the IPQ4019 DT binding this is only added for new users of the 2.4.0
> controller.
> 
> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v2:
> - None
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 64 +++++++++++++++-----------
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index d740cbe0e56d..d101bc5c0def 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -112,7 +112,7 @@ struct qcom_pcie_resources_2_3_2 {
>  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>  };
>  
> -#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	4
>  struct qcom_pcie_resources_2_4_0 {
>  	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
>  	int num_clks;
> @@ -638,13 +638,16 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	bool is_ipq = of_device_is_compatible(dev->of_node, "qcom,pcie-ipq4019");
>  	int ret;
>  
>  	res->clks[0].id = "aux";
>  	res->clks[1].id = "master_bus";
>  	res->clks[2].id = "slave_bus";
> +	res->clks[3].id = "iface";
>  
> -	res->num_clks = 3;
> +	/* qcom,pcie-ipq4019 is defined without "iface" */
> +	res->num_clks = is_ipq ? 3 : 4;

This is ugly but I don't have better idea except having static const
resource structures where we can describe num_clks and select the right
resource from compatible string, but lets leave that for the future.

Otherwise:

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

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-16  9:14   ` Stanimir Varbanov
@ 2019-05-28 15:13     ` Lorenzo Pieralisi
  2019-05-28 20:42       ` Stanimir Varbanov
  2019-05-29  0:58     ` Bjorn Andersson
  1 sibling, 1 reply; 14+ messages in thread
From: Lorenzo Pieralisi @ 2019-05-28 15:13 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Bjorn Andersson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	linux-arm-msm, linux-pci, devicetree, linux-kernel

On Thu, May 16, 2019 at 12:14:04PM +0300, Stanimir Varbanov wrote:
> Hi Bjorn,
> 
> On 5/2/19 3:19 AM, Bjorn Andersson wrote:
> > Before introducing the QCS404 platform, which uses the same PCIe
> > controller as IPQ4019, migrate this to use the bulk clock API, in order
> > to make the error paths slighly cleaner.
> > 
> > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v2:
> > - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> > 
> >  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
> >  1 file changed, 14 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0ed235d560e3..d740cbe0e56d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> >  };
> >  
> > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> >  struct qcom_pcie_resources_2_4_0 {
> > -	struct clk *aux_clk;
> > -	struct clk *master_clk;
> > -	struct clk *slave_clk;
> > +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> > +	int num_clks;
> >  	struct reset_control *axi_m_reset;
> >  	struct reset_control *axi_s_reset;
> >  	struct reset_control *pipe_reset;
> > @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
> >  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> >  	struct dw_pcie *pci = pcie->pci;
> >  	struct device *dev = pci->dev;
> > +	int ret;
> >  
> > -	res->aux_clk = devm_clk_get(dev, "aux");
> > -	if (IS_ERR(res->aux_clk))
> > -		return PTR_ERR(res->aux_clk);
> > +	res->clks[0].id = "aux";
> > +	res->clks[1].id = "master_bus";
> > +	res->clks[2].id = "slave_bus";
> >  
> > -	res->master_clk = devm_clk_get(dev, "master_bus");
> > -	if (IS_ERR(res->master_clk))
> > -		return PTR_ERR(res->master_clk);
> > +	res->num_clks = 3;
> 
> Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?
> 
> >  
> > -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> > -	if (IS_ERR(res->slave_clk))
> > -		return PTR_ERR(res->slave_clk);
> > +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
> >  	if (IS_ERR(res->axi_m_reset))
> > @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
> >  	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);
> > +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> >  }
> >  
> >  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> >  
> >  	usleep_range(10000, 12000);
> >  
> > -	ret = clk_prepare_enable(res->aux_clk);
> > -	if (ret) {
> > -		dev_err(dev, "cannot prepare/enable iface clock\n");
> > +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > +	if (ret)
> >  		goto err_clk_aux;
> 
> Maybe you have to change the name of the label too?
> 
> > -	}
> > -
> > -	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);
> > @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> >  
> >  	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:

Hi Bjorn, Stanimir,

can I merge the series as-is or we need a v4 for the requested
updates ? Please let me know.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-28 15:13     ` Lorenzo Pieralisi
@ 2019-05-28 20:42       ` Stanimir Varbanov
  0 siblings, 0 replies; 14+ messages in thread
From: Stanimir Varbanov @ 2019-05-28 20:42 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Bjorn Andersson, Bjorn Helgaas, Rob Herring, Mark Rutland,
	linux-arm-msm, linux-pci, devicetree, linux-kernel

Hi Lorenzo,

On 28.05.19 г. 18:13 ч., Lorenzo Pieralisi wrote:
> On Thu, May 16, 2019 at 12:14:04PM +0300, Stanimir Varbanov wrote:
>> Hi Bjorn,
>>
>> On 5/2/19 3:19 AM, Bjorn Andersson wrote:
>>> Before introducing the QCS404 platform, which uses the same PCIe
>>> controller as IPQ4019, migrate this to use the bulk clock API, in order
>>> to make the error paths slighly cleaner.
>>>
>>> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>>> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> ---
>>>
>>> Changes since v2:
>>> - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
>>>
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
>>>   1 file changed, 14 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index 0ed235d560e3..d740cbe0e56d 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
>>>   	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>>>   };
>>>   
>>> +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
>>>   struct qcom_pcie_resources_2_4_0 {
>>> -	struct clk *aux_clk;
>>> -	struct clk *master_clk;
>>> -	struct clk *slave_clk;
>>> +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
>>> +	int num_clks;
>>>   	struct reset_control *axi_m_reset;
>>>   	struct reset_control *axi_s_reset;
>>>   	struct reset_control *pipe_reset;
>>> @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
>>>   	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>>>   	struct dw_pcie *pci = pcie->pci;
>>>   	struct device *dev = pci->dev;
>>> +	int ret;
>>>   
>>> -	res->aux_clk = devm_clk_get(dev, "aux");
>>> -	if (IS_ERR(res->aux_clk))
>>> -		return PTR_ERR(res->aux_clk);
>>> +	res->clks[0].id = "aux";
>>> +	res->clks[1].id = "master_bus";
>>> +	res->clks[2].id = "slave_bus";
>>>   
>>> -	res->master_clk = devm_clk_get(dev, "master_bus");
>>> -	if (IS_ERR(res->master_clk))
>>> -		return PTR_ERR(res->master_clk);
>>> +	res->num_clks = 3;
>>
>> Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?
>>
>>>   
>>> -	res->slave_clk = devm_clk_get(dev, "slave_bus");
>>> -	if (IS_ERR(res->slave_clk))
>>> -		return PTR_ERR(res->slave_clk);
>>> +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
>>> +	if (ret < 0)
>>> +		return ret;
>>>   
>>>   	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
>>>   	if (IS_ERR(res->axi_m_reset))
>>> @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
>>>   	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);
>>> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>>>   }
>>>   
>>>   static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>>> @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>>>   
>>>   	usleep_range(10000, 12000);
>>>   
>>> -	ret = clk_prepare_enable(res->aux_clk);
>>> -	if (ret) {
>>> -		dev_err(dev, "cannot prepare/enable iface clock\n");
>>> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
>>> +	if (ret)
>>>   		goto err_clk_aux;
>>
>> Maybe you have to change the name of the label too?
>>
>>> -	}
>>> -
>>> -	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);
>>> @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>>>   
>>>   	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:
> 
> Hi Bjorn, Stanimir,
> 
> can I merge the series as-is or we need a v4 for the requested
> updates ? Please let me know.

I'm fine with either way:

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

regards,
Stan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers
  2019-05-16  9:14   ` Stanimir Varbanov
  2019-05-28 15:13     ` Lorenzo Pieralisi
@ 2019-05-29  0:58     ` Bjorn Andersson
  1 sibling, 0 replies; 14+ messages in thread
From: Bjorn Andersson @ 2019-05-29  0:58 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rob Herring, Mark Rutland,
	linux-arm-msm, linux-pci, devicetree, linux-kernel

On Thu 16 May 02:14 PDT 2019, Stanimir Varbanov wrote:

> Hi Bjorn,
> 
> On 5/2/19 3:19 AM, Bjorn Andersson wrote:
> > Before introducing the QCS404 platform, which uses the same PCIe
> > controller as IPQ4019, migrate this to use the bulk clock API, in order
> > to make the error paths slighly cleaner.
> > 
> > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v2:
> > - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> > 
> >  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
> >  1 file changed, 14 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0ed235d560e3..d740cbe0e56d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> >  };
> >  
> > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> >  struct qcom_pcie_resources_2_4_0 {
> > -	struct clk *aux_clk;
> > -	struct clk *master_clk;
> > -	struct clk *slave_clk;
> > +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> > +	int num_clks;
> >  	struct reset_control *axi_m_reset;
> >  	struct reset_control *axi_s_reset;
> >  	struct reset_control *pipe_reset;
> > @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
> >  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> >  	struct dw_pcie *pci = pcie->pci;
> >  	struct device *dev = pci->dev;
> > +	int ret;
> >  
> > -	res->aux_clk = devm_clk_get(dev, "aux");
> > -	if (IS_ERR(res->aux_clk))
> > -		return PTR_ERR(res->aux_clk);
> > +	res->clks[0].id = "aux";
> > +	res->clks[1].id = "master_bus";
> > +	res->clks[2].id = "slave_bus";
> >  
> > -	res->master_clk = devm_clk_get(dev, "master_bus");
> > -	if (IS_ERR(res->master_clk))
> > -		return PTR_ERR(res->master_clk);
> > +	res->num_clks = 3;
> 
> Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?
> 

As I replace it in patch 3/3 with a value different from "max clocks", I
don't think it makes sense to use the define here. So I'm leaving this
as is.

> >  
> > -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> > -	if (IS_ERR(res->slave_clk))
> > -		return PTR_ERR(res->slave_clk);
> > +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
> >  	if (IS_ERR(res->axi_m_reset))
> > @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
> >  	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);
> > +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> >  }
> >  
> >  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> >  
> >  	usleep_range(10000, 12000);
> >  
> > -	ret = clk_prepare_enable(res->aux_clk);
> > -	if (ret) {
> > -		dev_err(dev, "cannot prepare/enable iface clock\n");
> > +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > +	if (ret)
> >  		goto err_clk_aux;
> 
> Maybe you have to change the name of the label too?
> 

Updated this and posted v5. Should be good to be merged now.

Thanks for your reviews!

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-05-29  0:58 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-02  0:19 [PATCH v3 0/3] Qualcomm QCS404 PCIe support Bjorn Andersson
2019-05-02  0:19 ` [PATCH v3 1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers Bjorn Andersson
2019-05-02 11:53   ` Vinod Koul
2019-05-02 15:00     ` Bjorn Andersson
2019-05-03  3:10       ` Vinod Koul
2019-05-16  9:14   ` Stanimir Varbanov
2019-05-28 15:13     ` Lorenzo Pieralisi
2019-05-28 20:42       ` Stanimir Varbanov
2019-05-29  0:58     ` Bjorn Andersson
2019-05-02  0:19 ` [PATCH v3 2/3] dt-bindings: PCI: qcom: Add QCS404 to the binding Bjorn Andersson
2019-05-02 11:54   ` Vinod Koul
2019-05-02  0:19 ` [PATCH v3 3/3] PCI: qcom: Add QCS404 PCIe controller support Bjorn Andersson
2019-05-02 11:59   ` Vinod Koul
2019-05-16  9:39   ` Stanimir Varbanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).