linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/6] phy: qcom-qmp: Fix phy pipe clock gating
       [not found] <1500634921-25914-1-git-send-email-mgautam@codeaurora.org>
@ 2017-07-21 11:01 ` Manu Gautam
  2017-07-21 16:59   ` Stephen Boyd
  2017-07-21 11:01 ` [PATCH v1 2/6] phy: qcom-qmp: Power-on PHY before initialization Manu Gautam
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Manu Gautam @ 2017-07-21 11:01 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Vivek Gautam, Manu Gautam, Jaehoon Chung,
	Yoshihiro Shimoda, Fengguang Wu, Wei Yongjun,
	open list:GENERIC PHY FRAMEWORK

From: Vivek Gautam <vivek.gautam@codeaurora.org>

Pipe clock comes out of the phy and is available as long as
the phy is turned on. Clock controller fails to gate this
clock after the phy is turned off and generates a warning.

/ # [   33.048561] gcc_usb3_phy_pipe_clk status stuck at 'on'
[   33.048585] ------------[ cut here ]------------
[   33.052621] WARNING: CPU: 1 PID: 18 at ../drivers/clk/qcom/clk-branch.c:97 clk_branch_wait+0xf0/0x108
[   33.057384] Modules linked in:
[   33.066497] CPU: 1 PID: 18 Comm: kworker/1:0 Tainted: G        W       4.12.0-rc7-00024-gfe926e34c36d-dirty #96
[   33.069451] Hardware name: Qualcomm Technologies, Inc. DB820c (DT)
...
[   33.278565] [<ffff00000849b27c>] clk_branch_wait+0xf0/0x108
[   33.286375] [<ffff00000849b2f4>] clk_branch2_disable+0x28/0x34
[   33.291761] [<ffff0000084868dc>] clk_core_disable+0x5c/0x88
[   33.297660] [<ffff000008487d68>] clk_core_disable_lock+0x20/0x34
[   33.303129] [<ffff000008487d98>] clk_disable+0x1c/0x24
[   33.309384] [<ffff0000083ccd78>] qcom_qmp_phy_poweroff+0x20/0x48
[   33.314328] [<ffff0000083c53f4>] phy_power_off+0x80/0xdc
[   33.320492] [<ffff00000875c950>] dwc3_core_exit+0x94/0xa0
[   33.325784] [<ffff00000875c9ac>] dwc3_suspend_common+0x50/0x60
[   33.331080] [<ffff00000875ca04>] dwc3_runtime_suspend+0x48/0x6c
[   33.336810] [<ffff0000085b82f4>] pm_generic_runtime_suspend+0x28/0x38
[   33.342627] [<ffff0000085bace0>] __rpm_callback+0x150/0x254
[   33.349222] [<ffff0000085bae08>] rpm_callback+0x24/0x78
[   33.354604] [<ffff0000085b9fd8>] rpm_suspend+0xe0/0x4e4
[   33.359813] [<ffff0000085bb784>] pm_runtime_work+0xdc/0xf0
[   33.365028] [<ffff0000080d7b30>] process_one_work+0x12c/0x28c
[   33.370576] [<ffff0000080d7ce8>] worker_thread+0x58/0x3b8
[   33.376393] [<ffff0000080dd4a8>] kthread+0x100/0x12c
[   33.381776] [<ffff0000080836c0>] ret_from_fork+0x10/0x50

Fix this by enabling pipe clock at the end of phy_init(), and disabling
it as the first thing in phy_exit().

Fixes: e78f3d15e115 ("phy: qcom-qmp: new qmp phy driver for qcom-chipsets")

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 78ca628..a230c7b 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -610,19 +610,10 @@ static int qcom_qmp_phy_poweron(struct phy *phy)
 
 	/* turn on regulator supplies */
 	ret = regulator_bulk_enable(num, qmp->vregs);
-	if (ret) {
+	if (ret)
 		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
-		return ret;
-	}
-
-	ret = clk_prepare_enable(qphy->pipe_clk);
-	if (ret) {
-		dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret);
-		regulator_bulk_disable(num, qmp->vregs);
-		return ret;
-	}
 
-	return 0;
+	return ret;
 }
 
 static int qcom_qmp_phy_poweroff(struct phy *phy)
@@ -630,8 +621,6 @@ static int qcom_qmp_phy_poweroff(struct phy *phy)
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
 	struct qcom_qmp *qmp = qphy->qmp;
 
-	clk_disable_unprepare(qphy->pipe_clk);
-
 	regulator_bulk_disable(qmp->cfg->num_vregs, qmp->vregs);
 
 	return 0;
@@ -797,7 +786,14 @@ static int qcom_qmp_phy_init(struct phy *phy)
 		goto err_pcs_ready;
 	}
 
-	return ret;
+	/* phy is initialized; we can turn on the pipe clock now */
+	ret = clk_prepare_enable(qphy->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret);
+		goto err_pcs_ready;
+	}
+
+	return 0;
 
 err_pcs_ready:
 	if (cfg->has_lane_rst)
@@ -818,6 +814,8 @@ static int qcom_qmp_phy_exit(struct phy *phy)
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 	int i = cfg->num_clks;
 
+	clk_disable_unprepare(qphy->pipe_clk);
+
 	/* PHY reset */
 	qphy_setbits(qphy->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

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

* [PATCH v1 2/6] phy: qcom-qmp: Power-on PHY before initialization
       [not found] <1500634921-25914-1-git-send-email-mgautam@codeaurora.org>
  2017-07-21 11:01 ` [PATCH v1 1/6] phy: qcom-qmp: Fix phy pipe clock gating Manu Gautam
@ 2017-07-21 11:01 ` Manu Gautam
  2017-08-09 10:51   ` Manu Gautam
  2017-07-21 11:01 ` [PATCH v1 3/6] phy: qcom-qusb2: " Manu Gautam
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Manu Gautam @ 2017-07-21 11:01 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Manu Gautam, Vivek Gautam, Jaehoon Chung,
	Wei Yongjun, Fengguang Wu, open list:GENERIC PHY FRAMEWORK

PHY must be powered on before turning ON clocks and
attempting to initialize it. Driver is exposing
separate init and power_on routines for this.
Apparently USB dwc3 core driver performs power-on
after init. Also, poweron and init for QMP PHY
need to be executed together always, hence remove
poweron callback from phy_ops and explicitly perform
this from init, similar changes needed for poweroff.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index a230c7b..aefb853 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -479,6 +479,8 @@ struct qcom_qmp {
 
 	struct mutex phy_mutex;
 	int init_count;
+	bool power_enabled;
+	bool clk_enabled;
 };
 
 static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
@@ -599,29 +601,69 @@ static void qcom_qmp_phy_configure(void __iomem *base,
 	}
 }
 
-static int qcom_qmp_phy_poweron(struct phy *phy)
+static int qcom_qmp_phy_poweron(struct qcom_qmp *qmp)
 {
-	struct qmp_phy *qphy = phy_get_drvdata(phy);
-	struct qcom_qmp *qmp = qphy->qmp;
 	int num = qmp->cfg->num_vregs;
 	int ret;
 
-	dev_vdbg(&phy->dev, "Powering on QMP phy\n");
+	dev_vdbg(qmp->dev, "Powering on QMP phy\n");
+
+	if (qmp->power_enabled)
+		return 0;
 
 	/* turn on regulator supplies */
 	ret = regulator_bulk_enable(num, qmp->vregs);
 	if (ret)
 		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
+	else
+		qmp->power_enabled = true;
 
 	return ret;
 }
 
-static int qcom_qmp_phy_poweroff(struct phy *phy)
+static int qcom_qmp_phy_poweroff(struct qcom_qmp *qmp)
 {
-	struct qmp_phy *qphy = phy_get_drvdata(phy);
-	struct qcom_qmp *qmp = qphy->qmp;
+	if (!qmp->power_enabled)
+		return 0;
 
 	regulator_bulk_disable(qmp->cfg->num_vregs, qmp->vregs);
+	qmp->power_enabled = false;
+
+	return 0;
+}
+
+static int qcom_qmp_phy_enable_clocks(struct qcom_qmp *qmp)
+{
+	int ret = 0, i;
+
+	if (qmp->clk_enabled)
+		return 0;
+
+	for (i = 0; i < qmp->cfg->num_clks; i++) {
+		ret = clk_prepare_enable(qmp->clks[i]);
+		if (ret) {
+			dev_err(qmp->dev, "failed to enable %s clk, err=%d\n",
+				qmp->cfg->clk_list[i], ret);
+			while (--i >= 0)
+				clk_disable_unprepare(qmp->clks[i]);
+		}
+	}
+	qmp->clk_enabled = true;
+
+	return ret;
+}
+
+static int qcom_qmp_phy_disable_clocks(struct qcom_qmp *qmp)
+{
+	int i = qmp->cfg->num_clks;
+
+	if (!qmp->clk_enabled)
+		return 0;
+
+	while (--i >= 0)
+		clk_disable_unprepare(qmp->clks[i]);
+
+	qmp->clk_enabled = false;
 
 	return 0;
 }
@@ -729,19 +771,17 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	void __iomem *pcs = qphy->pcs;
 	void __iomem *status;
 	unsigned int mask, val;
-	int ret, i;
+	int ret;
 
 	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
 
-	for (i = 0; i < qmp->cfg->num_clks; i++) {
-		ret = clk_prepare_enable(qmp->clks[i]);
-		if (ret) {
-			dev_err(qmp->dev, "failed to enable %s clk, err=%d\n",
-				qmp->cfg->clk_list[i], ret);
-			while (--i >= 0)
-				clk_disable_unprepare(qmp->clks[i]);
-		}
-	}
+	ret = qcom_qmp_phy_poweron(qmp);
+	if (ret)
+		return ret;
+
+	ret = qcom_qmp_phy_enable_clocks(qmp);
+	if (ret)
+		goto err_clk_enable;
 
 	ret = qcom_qmp_phy_com_init(qmp);
 	if (ret)
@@ -801,8 +841,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
 err_lane_rst:
 	qcom_qmp_phy_com_exit(qmp);
 err_com_init:
-	while (--i >= 0)
-		clk_disable_unprepare(qmp->clks[i]);
+	qcom_qmp_phy_disable_clocks(qmp);
+err_clk_enable:
+	qcom_qmp_phy_poweroff(qmp);
 
 	return ret;
 }
@@ -812,7 +853,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
 	struct qcom_qmp *qmp = qphy->qmp;
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
-	int i = cfg->num_clks;
 
 	clk_disable_unprepare(qphy->pipe_clk);
 
@@ -830,8 +870,9 @@ static int qcom_qmp_phy_exit(struct phy *phy)
 
 	qcom_qmp_phy_com_exit(qmp);
 
-	while (--i >= 0)
-		clk_disable_unprepare(qmp->clks[i]);
+	qcom_qmp_phy_disable_clocks(qmp);
+
+	qcom_qmp_phy_poweroff(qmp);
 
 	return 0;
 }
@@ -958,8 +999,6 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, int id)
 static const struct phy_ops qcom_qmp_phy_gen_ops = {
 	.init		= qcom_qmp_phy_init,
 	.exit		= qcom_qmp_phy_exit,
-	.power_on	= qcom_qmp_phy_poweron,
-	.power_off	= qcom_qmp_phy_poweroff,
 	.owner		= THIS_MODULE,
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

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

* [PATCH v1 3/6] phy: qcom-qusb2: Power-on PHY before initialization
       [not found] <1500634921-25914-1-git-send-email-mgautam@codeaurora.org>
  2017-07-21 11:01 ` [PATCH v1 1/6] phy: qcom-qmp: Fix phy pipe clock gating Manu Gautam
  2017-07-21 11:01 ` [PATCH v1 2/6] phy: qcom-qmp: Power-on PHY before initialization Manu Gautam
@ 2017-07-21 11:01 ` Manu Gautam
  2017-07-21 11:01 ` [PATCH v1 4/6] phy: qcom-qusb2: Add support for runtime PM Manu Gautam
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Manu Gautam @ 2017-07-21 11:01 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Manu Gautam, Vivek Gautam, Heiko Stuebner,
	Yoshihiro Shimoda, open list:GENERIC PHY FRAMEWORK

PHY must be powered on before turning ON clocks and
attempting to initialize it. Driver is exposing
separate init and power_on routines for this.
Apparently USB dwc3 core driver performs power-on
after init. Also, poweron and init for QUSB2 PHY
need to be executed together always, hence remove
poweron callback from phy_ops and explicitly perform
this from init, similar changes needed for poweroff.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 6c57524..fa60a99 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -195,13 +195,13 @@ static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
 	qusb2_setbits(qphy->base, QUSB2PHY_PORT_TUNE2, val[0] << 0x4);
 }
 
-static int qusb2_phy_poweron(struct phy *phy)
+static int qusb2_phy_poweron(struct qusb2_phy *qphy)
 {
-	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+	struct device *dev = &qphy->phy->dev;
 	int num = ARRAY_SIZE(qphy->vregs);
 	int ret;
 
-	dev_vdbg(&phy->dev, "%s(): Powering-on QUSB2 phy\n", __func__);
+	dev_vdbg(dev, "%s(): Powering-on QUSB2 phy\n", __func__);
 
 	/* turn on regulator supplies */
 	ret = regulator_bulk_enable(num, qphy->vregs);
@@ -210,7 +210,7 @@ static int qusb2_phy_poweron(struct phy *phy)
 
 	ret = clk_prepare_enable(qphy->iface_clk);
 	if (ret) {
-		dev_err(&phy->dev, "failed to enable iface_clk, %d\n", ret);
+		dev_err(dev, "failed to enable iface_clk, %d\n", ret);
 		regulator_bulk_disable(num, qphy->vregs);
 		return ret;
 	}
@@ -218,10 +218,8 @@ static int qusb2_phy_poweron(struct phy *phy)
 	return 0;
 }
 
-static int qusb2_phy_poweroff(struct phy *phy)
+static int qusb2_phy_poweroff(struct qusb2_phy *qphy)
 {
-	struct qusb2_phy *qphy = phy_get_drvdata(phy);
-
 	clk_disable_unprepare(qphy->iface_clk);
 
 	regulator_bulk_disable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
@@ -238,11 +236,15 @@ static int qusb2_phy_init(struct phy *phy)
 
 	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
 
+	ret = qusb2_phy_poweron(qphy);
+	if (ret)
+		return ret;
+
 	/* enable ahb interface clock to program phy */
 	ret = clk_prepare_enable(qphy->cfg_ahb_clk);
 	if (ret) {
 		dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret);
-		return ret;
+		goto poweroff_phy;
 	}
 
 	/* Perform phy reset */
@@ -344,6 +346,9 @@ static int qusb2_phy_init(struct phy *phy)
 	reset_control_assert(qphy->phy_reset);
 disable_ahb_clk:
 	clk_disable_unprepare(qphy->cfg_ahb_clk);
+poweroff_phy:
+	qusb2_phy_poweroff(qphy);
+
 	return ret;
 }
 
@@ -362,14 +367,14 @@ static int qusb2_phy_exit(struct phy *phy)
 
 	clk_disable_unprepare(qphy->cfg_ahb_clk);
 
+	qusb2_phy_poweroff(qphy);
+
 	return 0;
 }
 
 static const struct phy_ops qusb2_phy_gen_ops = {
 	.init		= qusb2_phy_init,
 	.exit		= qusb2_phy_exit,
-	.power_on	= qusb2_phy_poweron,
-	.power_off	= qusb2_phy_poweroff,
 	.owner		= THIS_MODULE,
 };
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

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

* [PATCH v1 4/6] phy: qcom-qusb2: Add support for runtime PM
       [not found] <1500634921-25914-1-git-send-email-mgautam@codeaurora.org>
                   ` (2 preceding siblings ...)
  2017-07-21 11:01 ` [PATCH v1 3/6] phy: qcom-qusb2: " Manu Gautam
@ 2017-07-21 11:01 ` Manu Gautam
  2017-07-21 17:24   ` Stephen Boyd
  2017-07-21 11:02 ` [PATCH v1 5/6] phy: qcom-qmp: " Manu Gautam
  2017-07-21 11:02 ` [PATCH v1 6/6] usb: dwc3: core: Notify USB3 PHY as well for DRD modes Manu Gautam
  5 siblings, 1 reply; 13+ messages in thread
From: Manu Gautam @ 2017-07-21 11:01 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Manu Gautam, Vivek Gautam, Krzysztof Kozlowski,
	Viresh Kumar, open list:GENERIC PHY FRAMEWORK

Driver can turn off clocks during runtime suspend.
Also, runtime suspend is not as a result of host mode
selective suspend then PHY can be powered off as well.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index fa60a99..b505681 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -132,6 +132,9 @@ struct qusb2_phy {
 
 	const struct qusb2_phy_cfg *cfg;
 	bool has_se_clk_scheme;
+	bool phy_initialized;
+	bool powered_on;
+	enum phy_mode mode;
 };
 
 static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val)
@@ -203,30 +206,111 @@ static int qusb2_phy_poweron(struct qusb2_phy *qphy)
 
 	dev_vdbg(dev, "%s(): Powering-on QUSB2 phy\n", __func__);
 
+	if (qphy->powered_on)
+		return 0;
+
 	/* turn on regulator supplies */
 	ret = regulator_bulk_enable(num, qphy->vregs);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(qphy->iface_clk);
-	if (ret) {
-		dev_err(dev, "failed to enable iface_clk, %d\n", ret);
-		regulator_bulk_disable(num, qphy->vregs);
-		return ret;
-	}
+	qphy->powered_on = true;
 
 	return 0;
 }
 
 static int qusb2_phy_poweroff(struct qusb2_phy *qphy)
 {
-	clk_disable_unprepare(qphy->iface_clk);
+	if (!qphy->powered_on)
+		return 0;
 
 	regulator_bulk_disable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
 
+	qphy->powered_on = false;
+
+	return 0;
+}
+
+static int qusb2_phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+	struct qusb2_phy *qphy = phy_get_drvdata(phy);
+
+	qphy->mode = mode;
+
 	return 0;
 }
 
+static int __maybe_unused qusb2_phy_runtime_suspend(struct device *dev)
+{
+	struct qusb2_phy *qphy = dev_get_drvdata(dev);
+
+	dev_vdbg(dev, "Suspending QUSB2 Phy, mode:%d\n", qphy->mode);
+
+	if (!qphy->phy_initialized) {
+		dev_vdbg(dev, "PHY not initialized, bailing out\n");
+		return 0;
+	}
+
+	if (!qphy->has_se_clk_scheme)
+		clk_disable_unprepare(qphy->ref_clk);
+
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+	clk_disable_unprepare(qphy->iface_clk);
+
+	if (qphy->mode == PHY_MODE_INVALID)
+		qusb2_phy_poweroff(qphy);
+
+	return 0;
+}
+
+static int __maybe_unused qusb2_phy_runtime_resume(struct device *dev)
+{
+	struct qusb2_phy *qphy = dev_get_drvdata(dev);
+	int ret;
+
+	dev_vdbg(dev, "Resuming QUSB2 phy, mode:%d\n", qphy->mode);
+
+	if (!qphy->phy_initialized) {
+		dev_vdbg(dev, "PHY not initialized, bailing out\n");
+		return 0;
+	}
+
+	ret = qusb2_phy_poweron(qphy);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(qphy->iface_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable iface_clk, %d\n", ret);
+		goto poweroff_phy;
+	}
+
+	ret = clk_prepare_enable(qphy->cfg_ahb_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable cfg ahb clock, %d\n", ret);
+		goto disable_iface_clk;
+	}
+
+	if (!qphy->has_se_clk_scheme) {
+		clk_prepare_enable(qphy->ref_clk);
+		if (ret) {
+			dev_err(dev, "failed to enable ref clk, %d\n", ret);
+			goto disable_ahb_clk;
+		}
+	}
+
+	return 0;
+
+disable_ahb_clk:
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+disable_iface_clk:
+	clk_disable_unprepare(qphy->iface_clk);
+poweroff_phy:
+	qusb2_phy_poweroff(qphy);
+
+	return ret;
+}
+
 static int qusb2_phy_init(struct phy *phy)
 {
 	struct qusb2_phy *qphy = phy_get_drvdata(phy);
@@ -240,11 +324,17 @@ static int qusb2_phy_init(struct phy *phy)
 	if (ret)
 		return ret;
 
+	ret = clk_prepare_enable(qphy->iface_clk);
+	if (ret) {
+		dev_err(&phy->dev, "failed to enable iface_clk, %d\n", ret);
+		goto poweroff_phy;
+	}
+
 	/* enable ahb interface clock to program phy */
 	ret = clk_prepare_enable(qphy->cfg_ahb_clk);
 	if (ret) {
 		dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret);
-		goto poweroff_phy;
+		goto disable_iface_clk;
 	}
 
 	/* Perform phy reset */
@@ -336,6 +426,7 @@ static int qusb2_phy_init(struct phy *phy)
 		ret = -EBUSY;
 		goto disable_ref_clk;
 	}
+	qphy->phy_initialized = true;
 
 	return 0;
 
@@ -346,6 +437,8 @@ static int qusb2_phy_init(struct phy *phy)
 	reset_control_assert(qphy->phy_reset);
 disable_ahb_clk:
 	clk_disable_unprepare(qphy->cfg_ahb_clk);
+disable_iface_clk:
+	clk_disable_unprepare(qphy->iface_clk);
 poweroff_phy:
 	qusb2_phy_poweroff(qphy);
 
@@ -366,15 +459,19 @@ static int qusb2_phy_exit(struct phy *phy)
 	reset_control_assert(qphy->phy_reset);
 
 	clk_disable_unprepare(qphy->cfg_ahb_clk);
+	clk_disable_unprepare(qphy->iface_clk);
 
 	qusb2_phy_poweroff(qphy);
 
+	qphy->phy_initialized = false;
+
 	return 0;
 }
 
 static const struct phy_ops qusb2_phy_gen_ops = {
 	.init		= qusb2_phy_init,
 	.exit		= qusb2_phy_exit,
+	.set_mode	= qusb2_phy_set_mode,
 	.owner		= THIS_MODULE,
 };
 
@@ -387,6 +484,11 @@ static int qusb2_phy_exit(struct phy *phy)
 };
 MODULE_DEVICE_TABLE(of, qusb2_phy_of_match_table);
 
+static const struct dev_pm_ops qusb2_phy_pm_ops = {
+	SET_RUNTIME_PM_OPS(qusb2_phy_runtime_suspend,
+			   qusb2_phy_runtime_resume, NULL)
+};
+
 static int qusb2_phy_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -464,11 +566,14 @@ static int qusb2_phy_probe(struct platform_device *pdev)
 		qphy->cell = NULL;
 		dev_dbg(dev, "failed to lookup tune2 hstx trim value\n");
 	}
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
 
 	generic_phy = devm_phy_create(dev, NULL, &qusb2_phy_gen_ops);
 	if (IS_ERR(generic_phy)) {
 		ret = PTR_ERR(generic_phy);
 		dev_err(dev, "failed to create phy, %d\n", ret);
+		pm_runtime_disable(dev);
 		return ret;
 	}
 	qphy->phy = generic_phy;
@@ -479,6 +584,8 @@ static int qusb2_phy_probe(struct platform_device *pdev)
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 	if (!IS_ERR(phy_provider))
 		dev_info(dev, "Registered Qcom-QUSB2 phy\n");
+	else
+		pm_runtime_disable(dev);
 
 	return PTR_ERR_OR_ZERO(phy_provider);
 }
@@ -487,6 +594,7 @@ static int qusb2_phy_probe(struct platform_device *pdev)
 	.probe		= qusb2_phy_probe,
 	.driver = {
 		.name	= "qcom-qusb2-phy",
+		.pm	= &qusb2_phy_pm_ops,
 		.of_match_table = qusb2_phy_of_match_table,
 	},
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

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

* [PATCH v1 5/6] phy: qcom-qmp: Add support for runtime PM
       [not found] <1500634921-25914-1-git-send-email-mgautam@codeaurora.org>
                   ` (3 preceding siblings ...)
  2017-07-21 11:01 ` [PATCH v1 4/6] phy: qcom-qusb2: Add support for runtime PM Manu Gautam
@ 2017-07-21 11:02 ` Manu Gautam
  2017-07-21 11:02 ` [PATCH v1 6/6] usb: dwc3: core: Notify USB3 PHY as well for DRD modes Manu Gautam
  5 siblings, 0 replies; 13+ messages in thread
From: Manu Gautam @ 2017-07-21 11:02 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Manu Gautam, Vivek Gautam, Krzysztof Kozlowski,
	Wei Yongjun, Fengguang Wu, open list:GENERIC PHY FRAMEWORK

Driver can turn off clocks during runtime suspend.
Also, runtime suspend is not as a result of host mode
selective suspend then PHY can be powered off as well.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index aefb853..8394e24 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -481,6 +481,8 @@ struct qcom_qmp {
 	int init_count;
 	bool power_enabled;
 	bool clk_enabled;
+	bool phy_initialized;
+	enum phy_mode mode;
 };
 
 static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
@@ -832,6 +834,7 @@ static int qcom_qmp_phy_init(struct phy *phy)
 		dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret);
 		goto err_pcs_ready;
 	}
+	qmp->phy_initialized = true;
 
 	return 0;
 
@@ -874,6 +877,75 @@ static int qcom_qmp_phy_exit(struct phy *phy)
 
 	qcom_qmp_phy_poweroff(qmp);
 
+	qmp->phy_initialized = false;
+
+	return 0;
+}
+
+static int qcom_qmp_phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+	struct qmp_phy *qphy = phy_get_drvdata(phy);
+	struct qcom_qmp *qmp = qphy->qmp;
+
+	qmp->mode = mode;
+
+	return 0;
+}
+
+static int __maybe_unused qcom_qmp_phy_runtime_suspend(struct device *dev)
+{
+	struct qcom_qmp *qmp = dev_get_drvdata(dev);
+	struct qmp_phy *qphy = qmp->phys[0];
+	const struct qmp_phy_cfg *cfg = qmp->cfg;
+	void __iomem *pcs = qphy->pcs;
+
+	dev_vdbg(dev, "Suspending QMP phy, mode:%d\n", qmp->mode);
+
+	if (!qmp->phy_initialized) {
+		dev_vdbg(dev, "PHY not initialized, bailing out\n");
+		return 0;
+	}
+
+	/* Power down PHY if not in session */
+	if (qmp->mode == PHY_MODE_INVALID)
+		qphy_clrbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
+
+	clk_disable_unprepare(qphy->pipe_clk);
+	qcom_qmp_phy_disable_clocks(qmp);
+
+	if (qmp->mode == PHY_MODE_INVALID)
+		qcom_qmp_phy_poweroff(qmp);
+
+	return 0;
+}
+
+static int __maybe_unused qcom_qmp_phy_runtime_resume(struct device *dev)
+{
+	struct qcom_qmp *qmp = dev_get_drvdata(dev);
+	struct qmp_phy *qphy = qmp->phys[0];
+	const struct qmp_phy_cfg *cfg = qmp->cfg;
+	void __iomem *pcs = qphy->pcs;
+	int ret = 0;
+
+	dev_vdbg(dev, "Resuming QMP phy, mode:%d\n", qmp->mode);
+
+	if (!qmp->phy_initialized) {
+		dev_vdbg(dev, "PHY not initialized, bailing out\n");
+		return 0;
+	}
+
+	qcom_qmp_phy_poweron(qmp);
+
+	qcom_qmp_phy_enable_clocks(qmp);
+
+	ret = clk_prepare_enable(qphy->pipe_clk);
+	if (ret) {
+		dev_err(dev, "pipe_clk enable failed, err=%d\n", ret);
+		return ret;
+	}
+
+	qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
+
 	return 0;
 }
 
@@ -999,6 +1071,7 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, int id)
 static const struct phy_ops qcom_qmp_phy_gen_ops = {
 	.init		= qcom_qmp_phy_init,
 	.exit		= qcom_qmp_phy_exit,
+	.set_mode	= qcom_qmp_phy_set_mode,
 	.owner		= THIS_MODULE,
 };
 
@@ -1091,6 +1164,11 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 };
 MODULE_DEVICE_TABLE(of, qcom_qmp_phy_of_match_table);
 
+static const struct dev_pm_ops qcom_qmp_phy_pm_ops = {
+	SET_RUNTIME_PM_OPS(qcom_qmp_phy_runtime_suspend,
+			   qcom_qmp_phy_runtime_resume, NULL)
+};
+
 static int qcom_qmp_phy_probe(struct platform_device *pdev)
 {
 	struct qcom_qmp *qmp;
@@ -1118,6 +1196,7 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
 	qmp->serdes = base;
 
 	mutex_init(&qmp->phy_mutex);
+	qmp->mode = PHY_MODE_INVALID;
 
 	/* Get the specific init parameters of QMP phy */
 	qmp->cfg = of_device_get_match_data(dev);
@@ -1146,12 +1225,16 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	id = 0;
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	for_each_available_child_of_node(dev->of_node, child) {
 		/* Create per-lane phy */
 		ret = qcom_qmp_phy_create(dev, child, id);
 		if (ret) {
 			dev_err(dev, "failed to create lane%d phy, %d\n",
 				id, ret);
+			pm_runtime_disable(dev);
 			return ret;
 		}
 
@@ -1163,6 +1246,7 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
 		if (ret) {
 			dev_err(qmp->dev,
 				"failed to register pipe clock source\n");
+			pm_runtime_disable(dev);
 			return ret;
 		}
 		id++;
@@ -1171,6 +1255,8 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 	if (!IS_ERR(phy_provider))
 		dev_info(dev, "Registered Qcom-QMP phy\n");
+	else
+		pm_runtime_disable(dev);
 
 	return PTR_ERR_OR_ZERO(phy_provider);
 }
@@ -1179,6 +1265,7 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
 	.probe		= qcom_qmp_phy_probe,
 	.driver = {
 		.name	= "qcom-qmp-phy",
+		.pm	= &qcom_qmp_phy_pm_ops,
 		.of_match_table = qcom_qmp_phy_of_match_table,
 	},
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

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

* [PATCH v1 6/6] usb: dwc3: core: Notify USB3 PHY as well for DRD modes
       [not found] <1500634921-25914-1-git-send-email-mgautam@codeaurora.org>
                   ` (4 preceding siblings ...)
  2017-07-21 11:02 ` [PATCH v1 5/6] phy: qcom-qmp: " Manu Gautam
@ 2017-07-21 11:02 ` Manu Gautam
  2017-07-21 17:09   ` Stephen Boyd
  5 siblings, 1 reply; 13+ messages in thread
From: Manu Gautam @ 2017-07-21 11:02 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Manu Gautam, Greg Kroah-Hartman,
	open list:DESIGNWARE USB3 DRD IP DRIVER, open list

Driver currently notifies only USB3 PHY for mode change.
Extend this to USB3 PHY so that driver based on the mode
can release system resources - clocks, regulators etc.
and can even turn off PHY during runtime suspend.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 326b302..1f6c51e 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -156,9 +156,8 @@ static void __dwc3_set_mode(struct work_struct *work)
 		} else {
 			if (dwc->usb2_phy)
 				otg_set_vbus(dwc->usb2_phy->otg, true);
-			if (dwc->usb2_generic_phy)
-				phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
-
+			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
+			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
 		}
 		break;
 	case DWC3_GCTL_PRTCAP_DEVICE:
@@ -166,8 +165,8 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 		if (dwc->usb2_phy)
 			otg_set_vbus(dwc->usb2_phy->otg, false);
-		if (dwc->usb2_generic_phy)
-			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
 
 		ret = dwc3_gadget_init(dwc);
 		if (ret)
@@ -931,8 +930,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 
 		if (dwc->usb2_phy)
 			otg_set_vbus(dwc->usb2_phy->otg, false);
-		if (dwc->usb2_generic_phy)
-			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
 
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
@@ -946,8 +945,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 
 		if (dwc->usb2_phy)
 			otg_set_vbus(dwc->usb2_phy->otg, true);
-		if (dwc->usb2_generic_phy)
-			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
+		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
+		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
 
 		ret = dwc3_host_init(dwc);
 		if (ret) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

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

* Re: [PATCH v1 1/6] phy: qcom-qmp: Fix phy pipe clock gating
  2017-07-21 11:01 ` [PATCH v1 1/6] phy: qcom-qmp: Fix phy pipe clock gating Manu Gautam
@ 2017-07-21 16:59   ` Stephen Boyd
  2017-07-24  9:50     ` Manu Gautam
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2017-07-21 16:59 UTC (permalink / raw)
  To: Manu Gautam, Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Vivek Gautam, Jaehoon Chung, Yoshihiro Shimoda,
	Fengguang Wu, Wei Yongjun, open list:GENERIC PHY FRAMEWORK

On 07/21/2017 04:01 AM, Manu Gautam wrote:
> From: Vivek Gautam <vivek.gautam@codeaurora.org>
>
> Pipe clock comes out of the phy and is available as long as
> the phy is turned on. Clock controller fails to gate this
> clock after the phy is turned off and generates a warning.
>
> / # [   33.048561] gcc_usb3_phy_pipe_clk status stuck at 'on'
> [   33.048585] ------------[ cut here ]------------
> [   33.052621] WARNING: CPU: 1 PID: 18 at ../drivers/clk/qcom/clk-branch.c:97 clk_branch_wait+0xf0/0x108
> [   33.057384] Modules linked in:
> [   33.066497] CPU: 1 PID: 18 Comm: kworker/1:0 Tainted: G        W       4.12.0-rc7-00024-gfe926e34c36d-dirty #96
> [   33.069451] Hardware name: Qualcomm Technologies, Inc. DB820c (DT)
> ...
> [   33.278565] [<ffff00000849b27c>] clk_branch_wait+0xf0/0x108
> [   33.286375] [<ffff00000849b2f4>] clk_branch2_disable+0x28/0x34
> [   33.291761] [<ffff0000084868dc>] clk_core_disable+0x5c/0x88
> [   33.297660] [<ffff000008487d68>] clk_core_disable_lock+0x20/0x34
> [   33.303129] [<ffff000008487d98>] clk_disable+0x1c/0x24
> [   33.309384] [<ffff0000083ccd78>] qcom_qmp_phy_poweroff+0x20/0x48
> [   33.314328] [<ffff0000083c53f4>] phy_power_off+0x80/0xdc
> [   33.320492] [<ffff00000875c950>] dwc3_core_exit+0x94/0xa0
> [   33.325784] [<ffff00000875c9ac>] dwc3_suspend_common+0x50/0x60
> [   33.331080] [<ffff00000875ca04>] dwc3_runtime_suspend+0x48/0x6c
> [   33.336810] [<ffff0000085b82f4>] pm_generic_runtime_suspend+0x28/0x38
> [   33.342627] [<ffff0000085bace0>] __rpm_callback+0x150/0x254
> [   33.349222] [<ffff0000085bae08>] rpm_callback+0x24/0x78
> [   33.354604] [<ffff0000085b9fd8>] rpm_suspend+0xe0/0x4e4
> [   33.359813] [<ffff0000085bb784>] pm_runtime_work+0xdc/0xf0
> [   33.365028] [<ffff0000080d7b30>] process_one_work+0x12c/0x28c
> [   33.370576] [<ffff0000080d7ce8>] worker_thread+0x58/0x3b8
> [   33.376393] [<ffff0000080dd4a8>] kthread+0x100/0x12c
> [   33.381776] [<ffff0000080836c0>] ret_from_fork+0x10/0x50
>
> Fix this by enabling pipe clock at the end of phy_init(), and disabling
> it as the first thing in phy_exit().
>
> Fixes: e78f3d15e115 ("phy: qcom-qmp: new qmp phy driver for qcom-chipsets")
>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>

Missing your signoff? Also, the fixes tag should be right before signoff
without a newline between.

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

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

* Re: [PATCH v1 6/6] usb: dwc3: core: Notify USB3 PHY as well for DRD modes
  2017-07-21 11:02 ` [PATCH v1 6/6] usb: dwc3: core: Notify USB3 PHY as well for DRD modes Manu Gautam
@ 2017-07-21 17:09   ` Stephen Boyd
  2017-07-24  9:59     ` Manu Gautam
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2017-07-21 17:09 UTC (permalink / raw)
  To: Manu Gautam, Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Greg Kroah-Hartman,
	open list:DESIGNWARE USB3 DRD IP DRIVER, open list

On 07/21/2017 04:02 AM, Manu Gautam wrote:
> Driver currently notifies only USB3 PHY for mode change.
> Extend this to USB3 PHY so that driver based on the mode
> can release system resources - clocks, regulators etc.
> and can even turn off PHY during runtime suspend.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>

This patch is confusing because you say "notify for usb3 phy", but then
combine a change to remove the NULL pointer check on phy_set_mode() for
the usb2 phy. Please describe everything that's happening in a patch in
the commit text.

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

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

* Re: [PATCH v1 4/6] phy: qcom-qusb2: Add support for runtime PM
  2017-07-21 11:01 ` [PATCH v1 4/6] phy: qcom-qusb2: Add support for runtime PM Manu Gautam
@ 2017-07-21 17:24   ` Stephen Boyd
  2017-07-24 10:03     ` Manu Gautam
  0 siblings, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2017-07-21 17:24 UTC (permalink / raw)
  To: Manu Gautam, Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Vivek Gautam, Krzysztof Kozlowski, Viresh Kumar,
	open list:GENERIC PHY FRAMEWORK

On 07/21/2017 04:01 AM, Manu Gautam wrote:
> Driver can turn off clocks during runtime suspend.
> Also, runtime suspend is not as a result of host mode
> selective suspend then PHY can be powered off as well.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index fa60a99..b505681 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> @@ -132,6 +132,9 @@ struct qusb2_phy {
>  
>  	const struct qusb2_phy_cfg *cfg;
>  	bool has_se_clk_scheme;
> +	bool phy_initialized;
> +	bool powered_on;

Is the powered_on flag here because the controller driver has unbalanced
power on calls to the phy? Same comment applies for the phy_initialized
flag. Both of these look like workarounds for some odd behavior in the
controller driver.

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

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

* Re: [PATCH v1 1/6] phy: qcom-qmp: Fix phy pipe clock gating
  2017-07-21 16:59   ` Stephen Boyd
@ 2017-07-24  9:50     ` Manu Gautam
  0 siblings, 0 replies; 13+ messages in thread
From: Manu Gautam @ 2017-07-24  9:50 UTC (permalink / raw)
  To: Stephen Boyd, Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Vivek Gautam, Jaehoon Chung, Yoshihiro Shimoda,
	Fengguang Wu, Wei Yongjun, open list:GENERIC PHY FRAMEWORK



On 7/21/2017 10:29 PM, Stephen Boyd wrote:
> On 07/21/2017 04:01 AM, Manu Gautam wrote:
>> From: Vivek Gautam <vivek.gautam@codeaurora.org>
>>
>> Pipe clock comes out of the phy and is available as long as
>> the phy is turned on. Clock controller fails to gate this
>> clock after the phy is turned off and generates a warning.
>>
>> / # [   33.048561] gcc_usb3_phy_pipe_clk status stuck at 'on'
>> [   33.048585] ------------[ cut here ]------------
>> [   33.052621] WARNING: CPU: 1 PID: 18 at ../drivers/clk/qcom/clk-branch.c:97 clk_branch_wait+0xf0/0x108
>> [   33.057384] Modules linked in:
>> [   33.066497] CPU: 1 PID: 18 Comm: kworker/1:0 Tainted: G        W       4.12.0-rc7-00024-gfe926e34c36d-dirty #96
>> [   33.069451] Hardware name: Qualcomm Technologies, Inc. DB820c (DT)
>> ...
>> [   33.278565] [<ffff00000849b27c>] clk_branch_wait+0xf0/0x108
>> [   33.286375] [<ffff00000849b2f4>] clk_branch2_disable+0x28/0x34
>> [   33.291761] [<ffff0000084868dc>] clk_core_disable+0x5c/0x88
>> [   33.297660] [<ffff000008487d68>] clk_core_disable_lock+0x20/0x34
>> [   33.303129] [<ffff000008487d98>] clk_disable+0x1c/0x24
>> [   33.309384] [<ffff0000083ccd78>] qcom_qmp_phy_poweroff+0x20/0x48
>> [   33.314328] [<ffff0000083c53f4>] phy_power_off+0x80/0xdc
>> [   33.320492] [<ffff00000875c950>] dwc3_core_exit+0x94/0xa0
>> [   33.325784] [<ffff00000875c9ac>] dwc3_suspend_common+0x50/0x60
>> [   33.331080] [<ffff00000875ca04>] dwc3_runtime_suspend+0x48/0x6c
>> [   33.336810] [<ffff0000085b82f4>] pm_generic_runtime_suspend+0x28/0x38
>> [   33.342627] [<ffff0000085bace0>] __rpm_callback+0x150/0x254
>> [   33.349222] [<ffff0000085bae08>] rpm_callback+0x24/0x78
>> [   33.354604] [<ffff0000085b9fd8>] rpm_suspend+0xe0/0x4e4
>> [   33.359813] [<ffff0000085bb784>] pm_runtime_work+0xdc/0xf0
>> [   33.365028] [<ffff0000080d7b30>] process_one_work+0x12c/0x28c
>> [   33.370576] [<ffff0000080d7ce8>] worker_thread+0x58/0x3b8
>> [   33.376393] [<ffff0000080dd4a8>] kthread+0x100/0x12c
>> [   33.381776] [<ffff0000080836c0>] ret_from_fork+0x10/0x50
>>
>> Fix this by enabling pipe clock at the end of phy_init(), and disabling
>> it as the first thing in phy_exit().
>>
>> Fixes: e78f3d15e115 ("phy: qcom-qmp: new qmp phy driver for qcom-chipsets")
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Missing your signoff? Also, the fixes tag should be right before signoff
> without a newline between.
>

Will fix it in next version.

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

* Re: [PATCH v1 6/6] usb: dwc3: core: Notify USB3 PHY as well for DRD modes
  2017-07-21 17:09   ` Stephen Boyd
@ 2017-07-24  9:59     ` Manu Gautam
  0 siblings, 0 replies; 13+ messages in thread
From: Manu Gautam @ 2017-07-24  9:59 UTC (permalink / raw)
  To: Stephen Boyd, Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Greg Kroah-Hartman,
	open list:DESIGNWARE USB3 DRD IP DRIVER, open list



On 7/21/2017 10:39 PM, Stephen Boyd wrote:
> On 07/21/2017 04:02 AM, Manu Gautam wrote:
>> Driver currently notifies only USB3 PHY for mode change.
>> Extend this to USB3 PHY so that driver based on the mode
>> can release system resources - clocks, regulators etc.
>> and can even turn off PHY during runtime suspend.
>>
>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> This patch is confusing because you say "notify for usb3 phy", but then
> combine a change to remove the NULL pointer check on phy_set_mode() for
> the usb2 phy. Please describe everything that's happening in a patch in
> the commit text.

Sure, will add a comment for that.
It was done to align with other phy_ops usage in the driver. Also, these
wrapper functions already have a NULL pointer check.

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

* Re: [PATCH v1 4/6] phy: qcom-qusb2: Add support for runtime PM
  2017-07-21 17:24   ` Stephen Boyd
@ 2017-07-24 10:03     ` Manu Gautam
  0 siblings, 0 replies; 13+ messages in thread
From: Manu Gautam @ 2017-07-24 10:03 UTC (permalink / raw)
  To: Stephen Boyd, Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Vivek Gautam, Krzysztof Kozlowski, Viresh Kumar,
	open list:GENERIC PHY FRAMEWORK

Hi,


On 7/21/2017 10:54 PM, Stephen Boyd wrote:
> On 07/21/2017 04:01 AM, Manu Gautam wrote:
>> Driver can turn off clocks during runtime suspend.
>> Also, runtime suspend is not as a result of host mode
>> selective suspend then PHY can be powered off as well.
>>
>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> index fa60a99..b505681 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
>> @@ -132,6 +132,9 @@ struct qusb2_phy {
>>  
>>  	const struct qusb2_phy_cfg *cfg;
>>  	bool has_se_clk_scheme;
>> +	bool phy_initialized;
>> +	bool powered_on;
> Is the powered_on flag here because the controller driver has unbalanced
> power on calls to the phy? Same comment applies for the phy_initialized
> flag. Both of these look like workarounds for some odd behavior in the
> controller driver.
phy_initialized flag is to make runtime_suspend/resume no-ops
until PHY gets initialized.
powered_on flag is not related to any issue issue in core (as of now).
I just added that to bail out early from phy power_on/off which is now
called from phy_init/exit and runtime_suspend/resume as well.

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

* Re: [PATCH v1 2/6] phy: qcom-qmp: Power-on PHY before initialization
  2017-07-21 11:01 ` [PATCH v1 2/6] phy: qcom-qmp: Power-on PHY before initialization Manu Gautam
@ 2017-08-09 10:51   ` Manu Gautam
  0 siblings, 0 replies; 13+ messages in thread
From: Manu Gautam @ 2017-08-09 10:51 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Felipe Balbi
  Cc: linux-arm-msm, Vivek Gautam, Jaehoon Chung, Wei Yongjun,
	Fengguang Wu, open list:GENERIC PHY FRAMEWORK



On 7/21/2017 4:31 PM, Manu Gautam wrote:
>  }
> @@ -729,19 +771,17 @@ static int qcom_qmp_phy_init(struct phy *phy)
>  	void __iomem *pcs = qphy->pcs;
>  	void __iomem *status;
>  	unsigned int mask, val;
> -	int ret, i;
> +	int ret;
>  
>  	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>  
> -	for (i = 0; i < qmp->cfg->num_clks; i++) {
> -		ret = clk_prepare_enable(qmp->clks[i]);
> -		if (ret) {
> -			dev_err(qmp->dev, "failed to enable %s clk, err=%d\n",
> -				qmp->cfg->clk_list[i], ret);
> -			while (--i >= 0)
> -				clk_disable_unprepare(qmp->clks[i]);
> -		}
> -	}
> +	ret = qcom_qmp_phy_poweron(qmp);
> +	if (ret)
> +		return ret;
> +
> +	ret = qcom_qmp_phy_enable_clocks(qmp);
> +	if (ret)
> +		goto err_clk_enable;
>  
>  	ret = qcom_qmp_phy_com_init(qmp);

In next version I will move poweron and clock_enable to com_init
as PCI QMP PHY might use multiple PHY instances (for each lane) each of
them will call phy_init/exit.
qcom_qmp_phy_com_init executed only once, hence better
suited for turning ON clocks/regulators.

>  	if (ret)
> @@ -801,8 +841,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
>  err_lane_rst:
>  	qcom_qmp_phy_com_exit(qmp);
>  err_com_init:
> -	while (--i >= 0)
> -		clk_disable_unprepare(qmp->clks[i]);
> +	qcom_qmp_phy_disable_clocks(qmp);
> +err_clk_enable:
> +	qcom_qmp_phy_poweroff(qmp);
>  
>  	return ret;
>  }
> @@ -812,7 +853,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
>  	struct qmp_phy *qphy = phy_get_drvdata(phy);
>  	struct qcom_qmp *qmp = qphy->qmp;
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> -	int i = cfg->num_clks;
>  
>  	clk_disable_unprepare(qphy->pipe_clk);
>  
> @@ -830,8 +870,9 @@ static int qcom_qmp_phy_exit(struct phy *phy)
>  
>  	qcom_qmp_phy_com_exit(qmp);
>  
> -	while (--i >= 0)
> -		clk_disable_unprepare(qmp->clks[i]);
> +	qcom_qmp_phy_disable_clocks(qmp);
> +
> +	qcom_qmp_phy_poweroff(qmp);

On similar lines will be moving these to qcom_qmp_phy_com_exit().
>  
>  	return 0;
>  }
> @@ -958,8 +999,6 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, int id)
>  static const struct phy_ops qcom_qmp_phy_gen_ops = {
>  	.init		= qcom_qmp_phy_init,
>  	.exit		= qcom_qmp_phy_exit,
> -	.power_on	= qcom_qmp_phy_poweron,
> -	.power_off	= qcom_qmp_phy_poweroff,
>  	.owner		= THIS_MODULE,
>  };
>  

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

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

end of thread, other threads:[~2017-08-09 10:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1500634921-25914-1-git-send-email-mgautam@codeaurora.org>
2017-07-21 11:01 ` [PATCH v1 1/6] phy: qcom-qmp: Fix phy pipe clock gating Manu Gautam
2017-07-21 16:59   ` Stephen Boyd
2017-07-24  9:50     ` Manu Gautam
2017-07-21 11:01 ` [PATCH v1 2/6] phy: qcom-qmp: Power-on PHY before initialization Manu Gautam
2017-08-09 10:51   ` Manu Gautam
2017-07-21 11:01 ` [PATCH v1 3/6] phy: qcom-qusb2: " Manu Gautam
2017-07-21 11:01 ` [PATCH v1 4/6] phy: qcom-qusb2: Add support for runtime PM Manu Gautam
2017-07-21 17:24   ` Stephen Boyd
2017-07-24 10:03     ` Manu Gautam
2017-07-21 11:02 ` [PATCH v1 5/6] phy: qcom-qmp: " Manu Gautam
2017-07-21 11:02 ` [PATCH v1 6/6] usb: dwc3: core: Notify USB3 PHY as well for DRD modes Manu Gautam
2017-07-21 17:09   ` Stephen Boyd
2017-07-24  9:59     ` Manu Gautam

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