[PATCH v1 0/8] phy: qcom-ufs: Enable regulators to be off in suspend

[PATCH v1 0/8] phy: qcom-ufs: Enable regulators to be off in suspend

[Evan Green]

The goal with this series is to enable shutting off regulators that power
UFS during system suspend.

In "the good life" version of this, we'd just disable the regulators
in phy_poweroff and be done with it. Unfortunately, that's not symmetric,
as regulators are not enabled during phy_poweron. Ok, so you might think
we could just move the regulator enable and anything else that needs to
come along into phy_poweron, so that we can then undo it all in
phy_poweroff. That's where things get tricky.

The qcom-qmp-phy overloaded the phy_init and phy_poweron callbacks,
basically to mean "init phase 1" and "init phase 2". There are two phases
because they have this phy_reset bit outside of the phy (in the UFS
controller registers), and they need to make sure this bit is toggled at
specific points in the phy init sequence. So there's this implicit
sequence in the init dance between ufs-qcom.c and phy-qcom-qmp.c:
1) ufs-qcom asserts the PHY reset bit.
2) phy-qcom-qmp phy_init does most of its initialization, but exits early.
3) ufs-qcom deasserts the PHY reset bit.
4) phy-qcom-qmp phy_poweron finishes its initialization.

This init dance is very difficult to follow in the code (since it's split
between two drivers and not spelled out well), and arguably represents a
deficiency in the hardware description of these devices.

In this series I'm proposing tweaking the bindings for the Qualcomm
UFS controller and PHY. In it we expose a reset controller from the
UFS controller, that is then picked up and used from the PHY code.
With this, the phy code can be reorganized to complete its initialization
in a single function, removing the implicit two-phase overloading.

Then I can move most of the phy initialization, including enabling
the regulators, into phy_poweron. Now, when phy_poweroff is called,
the phy actually powers off. This finally disables the regulators
and allows me to save power in system suspend.

Because the UFS PHY reset bit is now toggled in the PHY, rather
than in ufs-qcom, this also percolated to all other PHYs using
ufs-qcom, which from what I can see is just 8996.

There are a couple of tradeoffs in this series that I'd welcome feedback
on. First, it breaks compatibility with device trees that don't expose
this new reset controller. Making this work with older device trees
would be pretty ugly in the code, and given that the SDM845 UFS DT nodes
aren't accepted upstream yet, the breakage seemed worth it. I'm not as
sure about 8996.

Second, I removed the calls to phy_poweroff during clock gating. This
was originally dialing down a clock or two, while leaving the phy powered.
I've now changed the semantics of phy_poweroff to, well, actually power off.
This works great for userlands that have set UFS's spm_lvl to 5 (off) like
I have, but maybe changes power consumption for devices that have spm_lvl
set to 3. I could try to use phy_init and phy_poweron as the two different
possible transitions (fully off, and clocks off respectively), but I'm not
sure if it actually matters, and I like the idea that phy_poweroff really
does power the thing off.

Also, I don't have an 8996 device to test. If someone is able to test this
out and perhaps point out any (hopefully obvious) bugs in the 8996 portion,
I'd be grateful.

This patch is based atop phy-next, plus the UFS DT nodes, which are now
patch 3, 4, 5 of [1].

[1] https://lore.kernel.org/lkml/20181210192826.241350-1-evgreen@chromium.org/


Evan Green (8):
  dt-bindings: phy-qcom-qmp: Add UFS PHY reset
  dt-bindings: phy: qcom-ufs: Add resets property
  arm64: dts: sdm845: Add UFS PHY reset
  arm64: dts: msm8996: Add UFS PHY reset controller
  scsi: ufs: qcom: Expose the reset controller for PHY
  phy: qcom-qmp: Utilize UFS reset controller
  phy: qcom-qmp: Move UFS phy to phy_poweron/off
  phy: qcom-ufs: Refactor all init steps into phy_poweron

 .../devicetree/bindings/phy/qcom-qmp-phy.txt  |   6 +-
 .../devicetree/bindings/ufs/ufs-qcom.txt      |   1 +
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   4 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   3 +
 drivers/phy/qualcomm/phy-qcom-qmp.c           | 123 ++++++++++--------
 drivers/phy/qualcomm/phy-qcom-ufs-i.h         |   5 +-
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c  |  25 +---
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c  |  25 +---
 drivers/phy/qualcomm/phy-qcom-ufs.c           |  57 ++++++--
 drivers/scsi/ufs/Kconfig                      |   1 +
 drivers/scsi/ufs/ufs-qcom.c                   | 110 +++++++++-------
 drivers/scsi/ufs/ufs-qcom.h                   |   4 +
 12 files changed, 197 insertions(+), 167 deletions(-)

-- 
2.18.1

[PATCH v1 1/8] dt-bindings: phy-qcom-qmp: Add UFS PHY reset

[Evan Green]

Add a required reset to the SDM845 UFS phy to express the PHY reset
bit inside the UFS controller register space. Before this change, this
reset was not expressed in the DT, and the driver utilized two different
callbacks (phy_init and phy_poweron) to implement a two-phase
initialization procedure that involved deasserting this reset between
init and poweron. This abused the two callbacks and diluted their
purpose.

That scheme does not work as regulators cannot be turned off in
phy_poweroff because they were turned on in init, rather than poweron.
The net result is that regulators are left on in suspend that shouldn't
be.

This new scheme gives the UFS reset to the PHY, so that it can fully
initialize itself in a single callback. We can then turn regulators on
during poweron and off during poweroff.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
I realize I'm not supposed to add a required property after the fact,
but given that the UFS DT nodes that would use this binding are not
yet upstream (and this would be the first), I was hoping to squeak by.

 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
index 41a1074228ba7..6b6ca4456dc7c 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -53,7 +53,8 @@ Required properties:
 	   one for each entry in reset-names.
  - reset-names: "phy" for reset of phy block,
 		"common" for phy common block reset,
-		"cfg" for phy's ahb cfg block reset.
+		"cfg" for phy's ahb cfg block reset,
+		"ufsphy" for the PHY reset in the UFS controller.
 
 		For "qcom,ipq8074-qmp-pcie-phy" must contain:
 			"phy", "common".
@@ -65,7 +66,8 @@ Required properties:
 			"phy", "common".
 		For "qcom,sdm845-qmp-usb3-uni-phy" must contain:
 			"phy", "common".
-		For "qcom,sdm845-qmp-ufs-phy": no resets are listed.
+		For "qcom,sdm845-qmp-ufs-phy": must contain:
+			"ufsphy".
 
  - vdda-phy-supply: Phandle to a regulator supply to PHY core block.
  - vdda-pll-supply: Phandle to 1.8V regulator supply to PHY refclk pll block.
-- 
2.18.1

[PATCH v1 2/8] dt-bindings: phy: qcom-ufs: Add resets property

[Evan Green]

Add a resets property to the PHY that represents the PHY reset
register in the UFS controller itself. This better describes the
complete specification of the PHY, and allows the PHY to perform
its initialization in a single function, rather than relying on
back-channel sequencing of initialization through the PHY framework.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

 Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
index 21d9a93db2e97..985f5e99ab332 100644
--- a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
+++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
@@ -29,6 +29,7 @@ Optional properties:
 - vdda-pll-max-microamp : specifies max. load that can be drawn from pll supply
 - vddp-ref-clk-supply   : phandle to UFS device ref_clk pad power supply
 - vddp-ref-clk-max-microamp : specifies max. load that can be drawn from this supply
+- resets : specifies the PHY reset in the UFS controller
 
 Example:
 
-- 
2.18.1

[PATCH v1 3/8] arm64: dts: sdm845: Add UFS PHY reset

[Evan Green]

Wire up the reset controller in the Qcom UFS controller for the PHY.
This will be used to toggle PHY reset during initialization of the PHY.

Signed-off-by: Evan Green <evgreen@chromium.org>
---
This commit is based atop the series at [1]. Patches 1 and 2 of that
series have landed, but 3, 4, and 5 are still outstanding.

[1] https://lore.kernel.org/lkml/20181210192826.241350-1-evgreen@chromium.org/

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index b29332b265d9e..029ab66405cf4 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -990,6 +990,7 @@
 			phy-names = "ufsphy";
 			lanes-per-direction = <2>;
 			power-domains = <&gcc UFS_PHY_GDSC>;
+			#reset-cells = <1>;
 
 			clock-names =
 				"core_clk",
@@ -1033,6 +1034,8 @@
 			clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>,
 				 <&gcc GCC_UFS_PHY_PHY_AUX_CLK>;
 
+			resets = <&ufs_mem_hc 0>;
+			reset-names = "ufsphy";
 			status = "disabled";
 
 			ufs_mem_phy_lanes: lanes@1d87400 {
-- 
2.18.1

[PATCH v1 4/8] arm64: dts: msm8996: Add UFS PHY reset controller

[Evan Green]

Add the reset controller for the UFS controller, and wire it up
so that the UFS PHY can initialize itself without relying on
implicit sequencing between the two drivers.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

 arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 99b7495455a62..179f1988d45c5 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -663,10 +663,11 @@
 			clock-names = "ref_clk_src", "ref_clk";
 			clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
 				 <&gcc GCC_UFS_CLKREF_CLK>;
+			resets = <&ufshc 0>;
 			status = "disabled";
 		};
 
-		ufshc@624000 {
+		ufshc: ufshc@624000 {
 			compatible = "qcom,ufshc";
 			reg = <0x624000 0x2500>;
 			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
@@ -722,6 +723,7 @@
 				<0 0>;
 
 			lanes-per-direction = <1>;
+			#reset-cells = <1>;
 			status = "disabled";
 
 			ufs_variant {
-- 
2.18.1

[PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Evan Green]

Expose a reset controller that the phy can use to perform its
initialization in a single callback.

Also, change the use of the phy functions from ufs-qcom such that
phy_poweron actually fires up the phy, and phy_poweroff actually
powers it down.

Signed-off-by: Evan Green <evgreen@chromium.org>

---
Note: This change depends on the remaining changes in this series,
since UFS PHY reset now needs to be done by the PHY driver.

 drivers/scsi/ufs/Kconfig    |   1 +
 drivers/scsi/ufs/ufs-qcom.c | 110 +++++++++++++++++++++---------------
 drivers/scsi/ufs/ufs-qcom.h |   4 ++
 3 files changed, 71 insertions(+), 44 deletions(-)

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index 2ddbb26d9c265..63c5c4115981f 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -100,6 +100,7 @@ config SCSI_UFS_QCOM
 	tristate "QCOM specific hooks to UFS controller platform driver"
 	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
 	select PHY_QCOM_UFS
+	select RESET_CONTROLLER
 	help
 	  This selects the QCOM specific additions to UFSHCD platform driver.
 	  UFS host on QCOM needs some vendor specific configuration before
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 3aeadb14aae1e..db46f9a64b54c 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/phy/phy.h>
+#include <linux/reset.h>
 
 #include "ufshcd.h"
 #include "ufshcd-pltfrm.h"
@@ -49,6 +50,11 @@ static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
 static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
 						       u32 clk_cycles);
 
+static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
+{
+	return container_of(rcd, struct ufs_qcom_host, rcdev);
+}
+
 static void ufs_qcom_dump_regs_wrapper(struct ufs_hba *hba, int offset, int len,
 				       const char *prefix, void *priv)
 {
@@ -255,11 +261,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 	if (is_rate_B)
 		phy_set_mode(phy, PHY_MODE_UFS_HS_B);
 
-	/* Assert PHY reset and apply PHY calibration values */
-	ufs_qcom_assert_reset(hba);
-	/* provide 1ms delay to let the reset pulse propagate */
-	usleep_range(1000, 1100);
-
 	/* phy initialization - calibrate the phy */
 	ret = phy_init(phy);
 	if (ret) {
@@ -268,15 +269,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 		goto out;
 	}
 
-	/* De-assert PHY reset and start serdes */
-	ufs_qcom_deassert_reset(hba);
-
-	/*
-	 * after reset deassertion, phy will need all ref clocks,
-	 * voltage, current to settle down before starting serdes.
-	 */
-	usleep_range(1000, 1100);
-
 	/* power on phy - start serdes and phy's power and clocks */
 	ret = phy_power_on(phy);
 	if (ret) {
@@ -290,7 +282,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
 	return 0;
 
 out_disable_phy:
-	ufs_qcom_assert_reset(hba);
 	phy_exit(phy);
 out:
 	return ret;
@@ -554,21 +545,10 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		ufs_qcom_disable_lane_clks(host);
 		phy_power_off(phy);
 
-		/* Assert PHY soft reset */
-		ufs_qcom_assert_reset(hba);
-		goto out;
-	}
-
-	/*
-	 * If UniPro link is not active, PHY ref_clk, main PHY analog power
-	 * rail and low noise analog power rail for PLL can be switched off.
-	 */
-	if (!ufs_qcom_is_link_active(hba)) {
+	} else if (!ufs_qcom_is_link_active(hba)) {
 		ufs_qcom_disable_lane_clks(host);
-		phy_power_off(phy);
 	}
 
-out:
 	return ret;
 }
 
@@ -578,21 +558,26 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	struct phy *phy = host->generic_phy;
 	int err;
 
-	err = phy_power_on(phy);
-	if (err) {
-		dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
-			__func__, err);
-		goto out;
-	}
+	if (ufs_qcom_is_link_off(hba)) {
+		err = phy_power_on(phy);
+		if (err) {
+			dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
+				__func__, err);
+			return err;
+		}
 
-	err = ufs_qcom_enable_lane_clks(host);
-	if (err)
-		goto out;
+		err = ufs_qcom_enable_lane_clks(host);
+		if (err)
+			return err;
 
-	hba->is_sys_suspended = false;
+	} else if (!ufs_qcom_is_link_active(hba)) {
+		err = ufs_qcom_enable_lane_clks(host);
+		if (err)
+			return err;
+	}
 
-out:
-	return err;
+	hba->is_sys_suspended = false;
+	return 0;
 }
 
 struct ufs_qcom_dev_params {
@@ -1118,8 +1103,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 		return 0;
 
 	if (on && (status == POST_CHANGE)) {
-		phy_power_on(host->generic_phy);
-
 		/* enable the device ref clock for HS mode*/
 		if (ufshcd_is_hs_mode(&hba->pwr_info))
 			ufs_qcom_dev_ref_clk_ctrl(host, true);
@@ -1131,9 +1114,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 		if (!ufs_qcom_is_link_active(hba)) {
 			/* disable device ref_clk */
 			ufs_qcom_dev_ref_clk_ctrl(host, false);
-
-			/* powering off PHY during aggressive clk gating */
-			phy_power_off(host->generic_phy);
 		}
 
 		vote = host->bus_vote.min_bw_vote;
@@ -1147,6 +1127,39 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 	return err;
 }
 
+static int
+ufs_qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
+
+	WARN_ON(id);
+	ufs_qcom_assert_reset(host->hba);
+	/* provide 1ms delay to let the reset pulse propagate */
+	usleep_range(1000, 1100);
+	return 0;
+}
+
+static int
+ufs_qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
+
+	WARN_ON(id);
+	ufs_qcom_deassert_reset(host->hba);
+
+	/*
+	 * after reset deassertion, phy will need all ref clocks,
+	 * voltage, current to settle down before starting serdes.
+	 */
+	usleep_range(1000, 1100);
+	return 0;
+}
+
+const struct reset_control_ops ufs_qcom_reset_ops = {
+	.assert = ufs_qcom_reset_assert,
+	.deassert = ufs_qcom_reset_deassert,
+};
+
 #define	ANDROID_BOOT_DEV_MAX	30
 static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
 
@@ -1191,6 +1204,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	host->hba = hba;
 	ufshcd_set_variant(hba, host);
 
+	/* Fire up the reset controller. Failure here is non-fatal. */
+	host->rcdev.of_node = dev->of_node;
+	host->rcdev.ops = &ufs_qcom_reset_ops;
+	host->rcdev.owner = dev->driver->owner;
+	host->rcdev.nr_resets = 1;
+	err = devm_reset_controller_register(dev, &host->rcdev);
+	if (err)
+		dev_warn(dev, "Failed to register reset controller\n");
+
 	/*
 	 * voting/devoting device ref_clk source is time consuming hence
 	 * skip devoting it during aggressive clock gating. This clock
diff --git a/drivers/scsi/ufs/ufs-qcom.h b/drivers/scsi/ufs/ufs-qcom.h
index c114826316eb0..68a8801857529 100644
--- a/drivers/scsi/ufs/ufs-qcom.h
+++ b/drivers/scsi/ufs/ufs-qcom.h
@@ -14,6 +14,8 @@
 #ifndef UFS_QCOM_H_
 #define UFS_QCOM_H_
 
+#include <linux/reset-controller.h>
+
 #define MAX_UFS_QCOM_HOSTS	1
 #define MAX_U32                 (~(u32)0)
 #define MPHY_TX_FSM_STATE       0x41
@@ -237,6 +239,8 @@ struct ufs_qcom_host {
 	/* Bitmask for enabling debug prints */
 	u32 dbg_print_en;
 	struct ufs_qcom_testbus testbus;
+
+	struct reset_controller_dev rcdev;
 };
 
 static inline u32
-- 
2.18.1

[PATCH v1 6/8] phy: qcom-qmp: Utilize UFS reset controller

[Evan Green]

Request the newly minted reset controller from the Qualcomm UFS
controller, and use it to toggle the PHY reset line from within
the PHY. This will allow us to merge the two phases of UFS PHY
initialization.

Signed-off-by: Evan Green <evgreen@chromium.org>

---
Note: this change is dependent on the previous changes, including
the DT changes, in order to expose the reset controller from UFS.

 drivers/phy/qualcomm/phy-qcom-qmp.c | 45 +++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index b4006818e1b65..eb1cac8f0fd4e 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -739,6 +739,9 @@ struct qmp_phy_cfg {
 
 	/* true, if PCS block has no separate SW_RESET register */
 	bool no_pcs_sw_reset;
+
+	/* true if the PHY has a UFS reset control to toggle */
+	bool has_ufsphy_reset;
 };
 
 /**
@@ -787,6 +790,7 @@ struct qmp_phy {
  * @init_count: phy common block initialization count
  * @phy_initialized: indicate if PHY has been initialized
  * @mode: current PHY mode
+ * @ufs_reset: optional UFS PHY reset handle
  */
 struct qcom_qmp {
 	struct device *dev;
@@ -804,6 +808,8 @@ struct qcom_qmp {
 	int init_count;
 	bool phy_initialized;
 	enum phy_mode mode;
+
+	struct reset_control *ufs_reset;
 };
 
 static inline void qphy_setbits(void __iomem *base, u32 offset, u32 val)
@@ -1034,6 +1040,8 @@ static const struct qmp_phy_cfg sdm845_ufsphy_cfg = {
 
 	.is_dual_lane_phy	= true,
 	.no_pcs_sw_reset	= true,
+
+	.has_ufsphy_reset	= true,
 };
 
 static void qcom_qmp_phy_configure(void __iomem *base,
@@ -1177,6 +1185,9 @@ static int qcom_qmp_phy_com_exit(struct qcom_qmp *qmp)
 		return 0;
 	}
 
+	if (qmp->ufs_reset)
+		reset_control_assert(qmp->ufs_reset);
+
 	if (cfg->has_phy_com_ctrl) {
 		qphy_setbits(serdes, cfg->regs[QPHY_COM_START_CONTROL],
 			     SERDES_START | PCS_START);
@@ -1214,6 +1225,32 @@ static int qcom_qmp_phy_init(struct phy *phy)
 
 	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
 
+	if (cfg->has_ufsphy_reset) {
+		/*
+		 * Get UFS reset, which is delayed until now to avoid a
+		 * circular dependency where UFS needs its PHY, but the PHY
+		 * needs this UFS reset.
+		 */
+		if (!qmp->ufs_reset) {
+			qmp->ufs_reset = of_reset_control_get(qmp->dev->of_node,
+							      "ufsphy");
+
+			if (IS_ERR(qmp->ufs_reset)) {
+				dev_err(qmp->dev,
+					"failed to get UFS reset: %d\n",
+					PTR_ERR(qmp->ufs_reset));
+
+				return PTR_ERR(qmp->ufs_reset);
+			}
+		}
+
+		ret = reset_control_assert(qmp->ufs_reset);
+		if (ret) {
+			dev_err(qmp->dev, "ufsphy reset deassert failed\n");
+			goto err_lane_rst;
+		}
+	}
+
 	ret = qcom_qmp_phy_com_init(qphy);
 	if (ret)
 		return ret;
@@ -1247,6 +1284,14 @@ static int qcom_qmp_phy_init(struct phy *phy)
 
 	qcom_qmp_phy_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
 
+	if (qmp->ufs_reset) {
+		ret = reset_control_deassert(qmp->ufs_reset);
+		if (ret) {
+			dev_err(qmp->dev, "ufsphy reset deassert failed\n");
+			goto err_lane_rst;
+		}
+	}
+
 	/*
 	 * UFS PHY requires the deassert of software reset before serdes start.
 	 * For UFS PHYs that do not have software reset control bits, defer
-- 
2.18.1

[PATCH v1 7/8] phy: qcom-qmp: Move UFS phy to phy_poweron/off

[Evan Green]

For UFS, move the actual firing up of the PHY to phy_poweron and
phy_poweroff callbacks, rather than init/exit. UFS calls
phy_poweroff during suspend, so now all clocks and regulators for
the phy can be powered down during suspend.

Signed-off-by: Evan Green <evgreen@chromium.org>

---

 drivers/phy/qualcomm/phy-qcom-qmp.c | 82 ++++++++---------------------
 1 file changed, 23 insertions(+), 59 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index eb1cac8f0fd4e..7766c6384d0a8 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1209,8 +1209,7 @@ static int qcom_qmp_phy_com_exit(struct qcom_qmp *qmp)
 	return 0;
 }
 
-/* PHY Initialization */
-static int qcom_qmp_phy_init(struct phy *phy)
+static int qcom_qmp_phy_enable(struct phy *phy)
 {
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
 	struct qcom_qmp *qmp = qphy->qmp;
@@ -1224,7 +1223,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	int ret;
 
 	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
-
 	if (cfg->has_ufsphy_reset) {
 		/*
 		 * Get UFS reset, which is delayed until now to avoid a
@@ -1292,14 +1290,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
 		}
 	}
 
-	/*
-	 * UFS PHY requires the deassert of software reset before serdes start.
-	 * For UFS PHYs that do not have software reset control bits, defer
-	 * starting serdes until the power on callback.
-	 */
-	if ((cfg->type == PHY_TYPE_UFS) && cfg->no_pcs_sw_reset)
-		goto out;
-
 	/*
 	 * Pull out PHY from POWER DOWN state.
 	 * This is active low enable signal to power-down PHY.
@@ -1311,7 +1301,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
 		usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
 
 	/* Pull PHY out of reset state */
-	qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
+	if (!cfg->no_pcs_sw_reset)
+		qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
+
 	if (cfg->has_phy_dp_com_ctrl)
 		qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
 
@@ -1328,11 +1320,12 @@ static int qcom_qmp_phy_init(struct phy *phy)
 		goto err_pcs_ready;
 	}
 	qmp->phy_initialized = true;
-
-out:
-	return ret;
+	return 0;
 
 err_pcs_ready:
+	if (qmp->ufs_reset)
+		reset_control_assert(qmp->ufs_reset);
+
 	clk_disable_unprepare(qphy->pipe_clk);
 err_clk_enable:
 	if (cfg->has_lane_rst)
@@ -1343,7 +1336,7 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	return ret;
 }
 
-static int qcom_qmp_phy_exit(struct phy *phy)
+static int qcom_qmp_phy_disable(struct phy *phy)
 {
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
 	struct qcom_qmp *qmp = qphy->qmp;
@@ -1360,7 +1353,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
 
 	/* Put PHY into POWER DOWN state: active low */
 	qphy_clrbits(qphy->pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
-
 	if (cfg->has_lane_rst)
 		reset_control_assert(qphy->lane_rst);
 
@@ -1371,44 +1363,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
 	return 0;
 }
 
-static int qcom_qmp_phy_poweron(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;
-	void __iomem *pcs = qphy->pcs;
-	void __iomem *status;
-	unsigned int mask, val;
-	int ret = 0;
-
-	if (cfg->type != PHY_TYPE_UFS)
-		return 0;
-
-	/*
-	 * For UFS PHY that has not software reset control, serdes start
-	 * should only happen when UFS driver explicitly calls phy_power_on
-	 * after it deasserts software reset.
-	 */
-	if (cfg->no_pcs_sw_reset && !qmp->phy_initialized &&
-	    (qmp->init_count != 0)) {
-		/* start SerDes and Phy-Coding-Sublayer */
-		qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
-
-		status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
-		mask = cfg->mask_pcs_ready;
-
-		ret = readl_poll_timeout(status, val, !(val & mask), 1,
-					 PHY_INIT_COMPLETE_TIMEOUT);
-		if (ret) {
-			dev_err(qmp->dev, "phy initialization timed-out\n");
-			return ret;
-		}
-		qmp->phy_initialized = true;
-	}
-
-	return ret;
-}
-
 static int qcom_qmp_phy_set_mode(struct phy *phy,
 				 enum phy_mode mode, int submode)
 {
@@ -1658,9 +1612,15 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
 }
 
 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,
+	.init		= qcom_qmp_phy_enable,
+	.exit		= qcom_qmp_phy_disable,
+	.set_mode	= qcom_qmp_phy_set_mode,
+	.owner		= THIS_MODULE,
+};
+
+static const struct phy_ops qcom_qmp_ufs_ops = {
+	.power_on	= qcom_qmp_phy_enable,
+	.power_off	= qcom_qmp_phy_disable,
 	.set_mode	= qcom_qmp_phy_set_mode,
 	.owner		= THIS_MODULE,
 };
@@ -1671,6 +1631,7 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 	struct qcom_qmp *qmp = dev_get_drvdata(dev);
 	struct phy *generic_phy;
 	struct qmp_phy *qphy;
+	const struct phy_ops *ops = &qcom_qmp_phy_gen_ops;
 	char prop_name[MAX_PROP_NAME];
 	int ret;
 
@@ -1757,7 +1718,10 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 		}
 	}
 
-	generic_phy = devm_phy_create(dev, np, &qcom_qmp_phy_gen_ops);
+	if (qmp->cfg->type == PHY_TYPE_UFS)
+		ops = &qcom_qmp_ufs_ops;
+
+	generic_phy = devm_phy_create(dev, np, ops);
 	if (IS_ERR(generic_phy)) {
 		ret = PTR_ERR(generic_phy);
 		dev_err(dev, "failed to create qphy %d\n", ret);
-- 
2.18.1

[PATCH v1 8/8] phy: qcom-ufs: Refactor all init steps into phy_poweron

[Evan Green]

The phy code was using implicit sequencing between the PHY driver
and the UFS driver to implement certain hardware requirements.
Specifically, the PHY reset register in the UFS controller needs
to be deasserted before serdes start occurs in the PHY.

Before this change, the code was doing this by utilizing the two
phy callbacks, phy_init and phy_poweron, as "init step 1" and
"init step 2", where the UFS driver would deassert reset between
these two steps.

This makes it challenging to power off the regulators in suspend,
as regulators are initialized in init, not in poweron, but only
poweroff is called during suspend, not exit.

Consolidate the initialization code into phy_poweron, and utilize
the reset controller exported from the UFS driver to explicitly
perform all the steps needed to initialize the PHY.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

 drivers/phy/qualcomm/phy-qcom-ufs-i.h        |  5 +-
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c | 25 +--------
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c | 25 +--------
 drivers/phy/qualcomm/phy-qcom-ufs.c          | 57 +++++++++++++++-----
 4 files changed, 49 insertions(+), 63 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-i.h b/drivers/phy/qualcomm/phy-qcom-ufs-i.h
index 681644e43248c..404b365c114df 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-i.h
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-i.h
@@ -19,6 +19,7 @@
 #include <linux/clk.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
@@ -113,11 +114,10 @@ struct ufs_qcom_phy {
 	char name[UFS_QCOM_PHY_NAME_LEN];
 	struct ufs_qcom_phy_calibration *cached_regs;
 	int cached_regs_table_size;
-	bool is_powered_on;
-	bool is_started;
 	struct ufs_qcom_phy_specific_ops *phy_spec_ops;
 
 	enum phy_mode mode;
+	struct reset_control *ufs_reset;
 };
 
 /**
@@ -132,6 +132,7 @@ struct ufs_qcom_phy {
  * and writes to QSERDES_RX_SIGDET_CNTRL attribute
  */
 struct ufs_qcom_phy_specific_ops {
+	int (*calibrate)(struct ufs_qcom_phy *ufs_qcom_phy, bool is_rate_B);
 	void (*start_serdes)(struct ufs_qcom_phy *phy);
 	int (*is_physical_coding_sublayer_ready)(struct ufs_qcom_phy *phy);
 	void (*set_tx_lane_enable)(struct ufs_qcom_phy *phy, u32 val);
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
index 1e0d4f2046a45..4815546f228cd 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
@@ -42,28 +42,6 @@ void ufs_qcom_phy_qmp_14nm_advertise_quirks(struct ufs_qcom_phy *phy_common)
 		UFS_QCOM_PHY_QUIRK_HIBERN8_EXIT_AFTER_PHY_PWR_COLLAPSE;
 }
 
-static int ufs_qcom_phy_qmp_14nm_init(struct phy *generic_phy)
-{
-	struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
-	bool is_rate_B = false;
-	int ret;
-
-	if (phy_common->mode == PHY_MODE_UFS_HS_B)
-		is_rate_B = true;
-
-	ret = ufs_qcom_phy_qmp_14nm_phy_calibrate(phy_common, is_rate_B);
-	if (!ret)
-		/* phy calibrated, but yet to be started */
-		phy_common->is_started = false;
-
-	return ret;
-}
-
-static int ufs_qcom_phy_qmp_14nm_exit(struct phy *generic_phy)
-{
-	return 0;
-}
-
 static
 int ufs_qcom_phy_qmp_14nm_set_mode(struct phy *generic_phy,
 				   enum phy_mode mode, int submode)
@@ -124,8 +102,6 @@ static int ufs_qcom_phy_qmp_14nm_is_pcs_ready(struct ufs_qcom_phy *phy_common)
 }
 
 static const struct phy_ops ufs_qcom_phy_qmp_14nm_phy_ops = {
-	.init		= ufs_qcom_phy_qmp_14nm_init,
-	.exit		= ufs_qcom_phy_qmp_14nm_exit,
 	.power_on	= ufs_qcom_phy_power_on,
 	.power_off	= ufs_qcom_phy_power_off,
 	.set_mode	= ufs_qcom_phy_qmp_14nm_set_mode,
@@ -133,6 +109,7 @@ static const struct phy_ops ufs_qcom_phy_qmp_14nm_phy_ops = {
 };
 
 static struct ufs_qcom_phy_specific_ops phy_14nm_ops = {
+	.calibrate		= ufs_qcom_phy_qmp_14nm_phy_calibrate,
 	.start_serdes		= ufs_qcom_phy_qmp_14nm_start_serdes,
 	.is_physical_coding_sublayer_ready = ufs_qcom_phy_qmp_14nm_is_pcs_ready,
 	.set_tx_lane_enable	= ufs_qcom_phy_qmp_14nm_set_tx_lane_enable,
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
index aef40f7a41d49..f1cf819e12eae 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
@@ -61,28 +61,6 @@ void ufs_qcom_phy_qmp_20nm_advertise_quirks(struct ufs_qcom_phy *phy_common)
 		UFS_QCOM_PHY_QUIRK_HIBERN8_EXIT_AFTER_PHY_PWR_COLLAPSE;
 }
 
-static int ufs_qcom_phy_qmp_20nm_init(struct phy *generic_phy)
-{
-	struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
-	bool is_rate_B = false;
-	int ret;
-
-	if (phy_common->mode == PHY_MODE_UFS_HS_B)
-		is_rate_B = true;
-
-	ret = ufs_qcom_phy_qmp_20nm_phy_calibrate(phy_common, is_rate_B);
-	if (!ret)
-		/* phy calibrated, but yet to be started */
-		phy_common->is_started = false;
-
-	return ret;
-}
-
-static int ufs_qcom_phy_qmp_20nm_exit(struct phy *generic_phy)
-{
-	return 0;
-}
-
 static
 int ufs_qcom_phy_qmp_20nm_set_mode(struct phy *generic_phy,
 				   enum phy_mode mode, int submode)
@@ -182,8 +160,6 @@ static int ufs_qcom_phy_qmp_20nm_is_pcs_ready(struct ufs_qcom_phy *phy_common)
 }
 
 static const struct phy_ops ufs_qcom_phy_qmp_20nm_phy_ops = {
-	.init		= ufs_qcom_phy_qmp_20nm_init,
-	.exit		= ufs_qcom_phy_qmp_20nm_exit,
 	.power_on	= ufs_qcom_phy_power_on,
 	.power_off	= ufs_qcom_phy_power_off,
 	.set_mode	= ufs_qcom_phy_qmp_20nm_set_mode,
@@ -191,6 +167,7 @@ static const struct phy_ops ufs_qcom_phy_qmp_20nm_phy_ops = {
 };
 
 static struct ufs_qcom_phy_specific_ops phy_20nm_ops = {
+	.calibrate		= ufs_qcom_phy_qmp_20nm_phy_calibrate,
 	.start_serdes		= ufs_qcom_phy_qmp_20nm_start_serdes,
 	.is_physical_coding_sublayer_ready = ufs_qcom_phy_qmp_20nm_is_pcs_ready,
 	.set_tx_lane_enable	= ufs_qcom_phy_qmp_20nm_set_tx_lane_enable,
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs.c b/drivers/phy/qualcomm/phy-qcom-ufs.c
index f2979ccad00a3..a4cff17fef925 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs.c
@@ -147,6 +147,21 @@ struct phy *ufs_qcom_phy_generic_probe(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(ufs_qcom_phy_generic_probe);
 
+static int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common)
+{
+	struct reset_control *reset;
+
+	if (phy_common->ufs_reset)
+		return 0;
+
+	reset = of_reset_control_get_by_index(phy_common->dev->of_node, 0);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	phy_common->ufs_reset = reset;
+	return 0;
+}
+
 static int __ufs_qcom_phy_clk_get(struct device *dev,
 			 const char *name, struct clk **clk_out, bool err_print)
 {
@@ -528,23 +543,42 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
 {
 	struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
 	struct device *dev = phy_common->dev;
+	bool is_rate_B = false;
 	int err;
 
-	if (phy_common->is_powered_on)
-		return 0;
+	err = ufs_qcom_phy_get_reset(phy_common);
+	if (err)
+		return err;
 
-	if (!phy_common->is_started) {
-		err = ufs_qcom_phy_start_serdes(phy_common);
+	if (phy_common->ufs_reset) {
+		err = reset_control_assert(phy_common->ufs_reset);
 		if (err)
 			return err;
+	}
 
-		err = ufs_qcom_phy_is_pcs_ready(phy_common);
-		if (err)
-			return err;
+	if (phy_common->mode == PHY_MODE_UFS_HS_B)
+		is_rate_B = true;
 
-		phy_common->is_started = true;
+	err = phy_common->phy_spec_ops->calibrate(phy_common, is_rate_B);
+	if (err)
+		return err;
+
+	if (phy_common->ufs_reset) {
+		err = reset_control_deassert(phy_common->ufs_reset);
+		if (err) {
+			dev_err(dev, "Failed to assert UFS PHY reset");
+			return err;
+		}
 	}
 
+	err = ufs_qcom_phy_start_serdes(phy_common);
+	if (err)
+		return err;
+
+	err = ufs_qcom_phy_is_pcs_ready(phy_common);
+	if (err)
+		return err;
+
 	err = ufs_qcom_phy_enable_vreg(dev, &phy_common->vdda_phy);
 	if (err) {
 		dev_err(dev, "%s enable vdda_phy failed, err=%d\n",
@@ -587,7 +621,6 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
 		}
 	}
 
-	phy_common->is_powered_on = true;
 	goto out;
 
 out_disable_ref_clk:
@@ -607,9 +640,6 @@ int ufs_qcom_phy_power_off(struct phy *generic_phy)
 {
 	struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
 
-	if (!phy_common->is_powered_on)
-		return 0;
-
 	phy_common->phy_spec_ops->power_control(phy_common, false);
 
 	if (phy_common->vddp_ref_clk.reg)
@@ -620,7 +650,8 @@ int ufs_qcom_phy_power_off(struct phy *generic_phy)
 
 	ufs_qcom_phy_disable_vreg(phy_common->dev, &phy_common->vdda_pll);
 	ufs_qcom_phy_disable_vreg(phy_common->dev, &phy_common->vdda_phy);
-	phy_common->is_powered_on = false;
+	if (phy_common->ufs_reset)
+		reset_control_assert(phy_common->ufs_reset);
 
 	return 0;
 }
-- 
2.18.1

Re: [PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Kishon Vijay Abraham I]

On 12/01/19 4:31 AM, Evan Green wrote:
> Expose a reset controller that the phy can use to perform its
> initialization in a single callback.
> 
> Also, change the use of the phy functions from ufs-qcom such that
> phy_poweron actually fires up the phy, and phy_poweroff actually
> powers it down.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>

Can I get Ack for this patch from SCSI MAINTAINERS?

Thanks
Kishon

> 
> ---
> Note: This change depends on the remaining changes in this series,
> since UFS PHY reset now needs to be done by the PHY driver.
> 
>  drivers/scsi/ufs/Kconfig    |   1 +
>  drivers/scsi/ufs/ufs-qcom.c | 110 +++++++++++++++++++++---------------
>  drivers/scsi/ufs/ufs-qcom.h |   4 ++
>  3 files changed, 71 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index 2ddbb26d9c265..63c5c4115981f 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -100,6 +100,7 @@ config SCSI_UFS_QCOM
>  	tristate "QCOM specific hooks to UFS controller platform driver"
>  	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
>  	select PHY_QCOM_UFS
> +	select RESET_CONTROLLER
>  	help
>  	  This selects the QCOM specific additions to UFSHCD platform driver.
>  	  UFS host on QCOM needs some vendor specific configuration before
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 3aeadb14aae1e..db46f9a64b54c 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
> +#include <linux/reset.h>
>  
>  #include "ufshcd.h"
>  #include "ufshcd-pltfrm.h"
> @@ -49,6 +50,11 @@ static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>  static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>  						       u32 clk_cycles);
>  
> +static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
> +{
> +	return container_of(rcd, struct ufs_qcom_host, rcdev);
> +}
> +
>  static void ufs_qcom_dump_regs_wrapper(struct ufs_hba *hba, int offset, int len,
>  				       const char *prefix, void *priv)
>  {
> @@ -255,11 +261,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  	if (is_rate_B)
>  		phy_set_mode(phy, PHY_MODE_UFS_HS_B);
>  
> -	/* Assert PHY reset and apply PHY calibration values */
> -	ufs_qcom_assert_reset(hba);
> -	/* provide 1ms delay to let the reset pulse propagate */
> -	usleep_range(1000, 1100);
> -
>  	/* phy initialization - calibrate the phy */
>  	ret = phy_init(phy);
>  	if (ret) {
> @@ -268,15 +269,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  		goto out;
>  	}
>  
> -	/* De-assert PHY reset and start serdes */
> -	ufs_qcom_deassert_reset(hba);
> -
> -	/*
> -	 * after reset deassertion, phy will need all ref clocks,
> -	 * voltage, current to settle down before starting serdes.
> -	 */
> -	usleep_range(1000, 1100);
> -
>  	/* power on phy - start serdes and phy's power and clocks */
>  	ret = phy_power_on(phy);
>  	if (ret) {
> @@ -290,7 +282,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  	return 0;
>  
>  out_disable_phy:
> -	ufs_qcom_assert_reset(hba);
>  	phy_exit(phy);
>  out:
>  	return ret;
> @@ -554,21 +545,10 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		ufs_qcom_disable_lane_clks(host);
>  		phy_power_off(phy);
>  
> -		/* Assert PHY soft reset */
> -		ufs_qcom_assert_reset(hba);
> -		goto out;
> -	}
> -
> -	/*
> -	 * If UniPro link is not active, PHY ref_clk, main PHY analog power
> -	 * rail and low noise analog power rail for PLL can be switched off.
> -	 */
> -	if (!ufs_qcom_is_link_active(hba)) {
> +	} else if (!ufs_qcom_is_link_active(hba)) {
>  		ufs_qcom_disable_lane_clks(host);
> -		phy_power_off(phy);
>  	}
>  
> -out:
>  	return ret;
>  }
>  
> @@ -578,21 +558,26 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	struct phy *phy = host->generic_phy;
>  	int err;
>  
> -	err = phy_power_on(phy);
> -	if (err) {
> -		dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
> -			__func__, err);
> -		goto out;
> -	}
> +	if (ufs_qcom_is_link_off(hba)) {
> +		err = phy_power_on(phy);
> +		if (err) {
> +			dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
> +				__func__, err);
> +			return err;
> +		}
>  
> -	err = ufs_qcom_enable_lane_clks(host);
> -	if (err)
> -		goto out;
> +		err = ufs_qcom_enable_lane_clks(host);
> +		if (err)
> +			return err;
>  
> -	hba->is_sys_suspended = false;
> +	} else if (!ufs_qcom_is_link_active(hba)) {
> +		err = ufs_qcom_enable_lane_clks(host);
> +		if (err)
> +			return err;
> +	}
>  
> -out:
> -	return err;
> +	hba->is_sys_suspended = false;
> +	return 0;
>  }
>  
>  struct ufs_qcom_dev_params {
> @@ -1118,8 +1103,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  		return 0;
>  
>  	if (on && (status == POST_CHANGE)) {
> -		phy_power_on(host->generic_phy);
> -
>  		/* enable the device ref clock for HS mode*/
>  		if (ufshcd_is_hs_mode(&hba->pwr_info))
>  			ufs_qcom_dev_ref_clk_ctrl(host, true);
> @@ -1131,9 +1114,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  		if (!ufs_qcom_is_link_active(hba)) {
>  			/* disable device ref_clk */
>  			ufs_qcom_dev_ref_clk_ctrl(host, false);
> -
> -			/* powering off PHY during aggressive clk gating */
> -			phy_power_off(host->generic_phy);
>  		}
>  
>  		vote = host->bus_vote.min_bw_vote;
> @@ -1147,6 +1127,39 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  	return err;
>  }
>  
> +static int
> +ufs_qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +	WARN_ON(id);
> +	ufs_qcom_assert_reset(host->hba);
> +	/* provide 1ms delay to let the reset pulse propagate */
> +	usleep_range(1000, 1100);
> +	return 0;
> +}
> +
> +static int
> +ufs_qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +	WARN_ON(id);
> +	ufs_qcom_deassert_reset(host->hba);
> +
> +	/*
> +	 * after reset deassertion, phy will need all ref clocks,
> +	 * voltage, current to settle down before starting serdes.
> +	 */
> +	usleep_range(1000, 1100);
> +	return 0;
> +}
> +
> +const struct reset_control_ops ufs_qcom_reset_ops = {
> +	.assert = ufs_qcom_reset_assert,
> +	.deassert = ufs_qcom_reset_deassert,
> +};
> +
>  #define	ANDROID_BOOT_DEV_MAX	30
>  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
>  
> @@ -1191,6 +1204,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  	host->hba = hba;
>  	ufshcd_set_variant(hba, host);
>  
> +	/* Fire up the reset controller. Failure here is non-fatal. */
> +	host->rcdev.of_node = dev->of_node;
> +	host->rcdev.ops = &ufs_qcom_reset_ops;
> +	host->rcdev.owner = dev->driver->owner;
> +	host->rcdev.nr_resets = 1;
> +	err = devm_reset_controller_register(dev, &host->rcdev);
> +	if (err)
> +		dev_warn(dev, "Failed to register reset controller\n");
> +
>  	/*
>  	 * voting/devoting device ref_clk source is time consuming hence
>  	 * skip devoting it during aggressive clock gating. This clock
> diff --git a/drivers/scsi/ufs/ufs-qcom.h b/drivers/scsi/ufs/ufs-qcom.h
> index c114826316eb0..68a8801857529 100644
> --- a/drivers/scsi/ufs/ufs-qcom.h
> +++ b/drivers/scsi/ufs/ufs-qcom.h
> @@ -14,6 +14,8 @@
>  #ifndef UFS_QCOM_H_
>  #define UFS_QCOM_H_
>  
> +#include <linux/reset-controller.h>
> +
>  #define MAX_UFS_QCOM_HOSTS	1
>  #define MAX_U32                 (~(u32)0)
>  #define MPHY_TX_FSM_STATE       0x41
> @@ -237,6 +239,8 @@ struct ufs_qcom_host {
>  	/* Bitmask for enabling debug prints */
>  	u32 dbg_print_en;
>  	struct ufs_qcom_testbus testbus;
> +
> +	struct reset_controller_dev rcdev;
>  };
>  
>  static inline u32
> 

Re: [PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Kishon Vijay Abraham I]

On 12/01/19 4:31 AM, Evan Green wrote:
> Expose a reset controller that the phy can use to perform its
> initialization in a single callback.
> 
> Also, change the use of the phy functions from ufs-qcom such that
> phy_poweron actually fires up the phy, and phy_poweroff actually
> powers it down.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>

Can I get Ack for this patch from SCSI MAINTAINERS?

Thanks
Kishon

> 
> ---
> Note: This change depends on the remaining changes in this series,
> since UFS PHY reset now needs to be done by the PHY driver.
> 
>  drivers/scsi/ufs/Kconfig    |   1 +
>  drivers/scsi/ufs/ufs-qcom.c | 110 +++++++++++++++++++++---------------
>  drivers/scsi/ufs/ufs-qcom.h |   4 ++
>  3 files changed, 71 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index 2ddbb26d9c265..63c5c4115981f 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -100,6 +100,7 @@ config SCSI_UFS_QCOM
>  	tristate "QCOM specific hooks to UFS controller platform driver"
>  	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
>  	select PHY_QCOM_UFS
> +	select RESET_CONTROLLER
>  	help
>  	  This selects the QCOM specific additions to UFSHCD platform driver.
>  	  UFS host on QCOM needs some vendor specific configuration before
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 3aeadb14aae1e..db46f9a64b54c 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
> +#include <linux/reset.h>
>  
>  #include "ufshcd.h"
>  #include "ufshcd-pltfrm.h"
> @@ -49,6 +50,11 @@ static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>  static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
>  						       u32 clk_cycles);
>  
> +static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
> +{
> +	return container_of(rcd, struct ufs_qcom_host, rcdev);
> +}
> +
>  static void ufs_qcom_dump_regs_wrapper(struct ufs_hba *hba, int offset, int len,
>  				       const char *prefix, void *priv)
>  {
> @@ -255,11 +261,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  	if (is_rate_B)
>  		phy_set_mode(phy, PHY_MODE_UFS_HS_B);
>  
> -	/* Assert PHY reset and apply PHY calibration values */
> -	ufs_qcom_assert_reset(hba);
> -	/* provide 1ms delay to let the reset pulse propagate */
> -	usleep_range(1000, 1100);
> -
>  	/* phy initialization - calibrate the phy */
>  	ret = phy_init(phy);
>  	if (ret) {
> @@ -268,15 +269,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  		goto out;
>  	}
>  
> -	/* De-assert PHY reset and start serdes */
> -	ufs_qcom_deassert_reset(hba);
> -
> -	/*
> -	 * after reset deassertion, phy will need all ref clocks,
> -	 * voltage, current to settle down before starting serdes.
> -	 */
> -	usleep_range(1000, 1100);
> -
>  	/* power on phy - start serdes and phy's power and clocks */
>  	ret = phy_power_on(phy);
>  	if (ret) {
> @@ -290,7 +282,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>  	return 0;
>  
>  out_disable_phy:
> -	ufs_qcom_assert_reset(hba);
>  	phy_exit(phy);
>  out:
>  	return ret;
> @@ -554,21 +545,10 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  		ufs_qcom_disable_lane_clks(host);
>  		phy_power_off(phy);
>  
> -		/* Assert PHY soft reset */
> -		ufs_qcom_assert_reset(hba);
> -		goto out;
> -	}
> -
> -	/*
> -	 * If UniPro link is not active, PHY ref_clk, main PHY analog power
> -	 * rail and low noise analog power rail for PLL can be switched off.
> -	 */
> -	if (!ufs_qcom_is_link_active(hba)) {
> +	} else if (!ufs_qcom_is_link_active(hba)) {
>  		ufs_qcom_disable_lane_clks(host);
> -		phy_power_off(phy);
>  	}
>  
> -out:
>  	return ret;
>  }
>  
> @@ -578,21 +558,26 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	struct phy *phy = host->generic_phy;
>  	int err;
>  
> -	err = phy_power_on(phy);
> -	if (err) {
> -		dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
> -			__func__, err);
> -		goto out;
> -	}
> +	if (ufs_qcom_is_link_off(hba)) {
> +		err = phy_power_on(phy);
> +		if (err) {
> +			dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
> +				__func__, err);
> +			return err;
> +		}
>  
> -	err = ufs_qcom_enable_lane_clks(host);
> -	if (err)
> -		goto out;
> +		err = ufs_qcom_enable_lane_clks(host);
> +		if (err)
> +			return err;
>  
> -	hba->is_sys_suspended = false;
> +	} else if (!ufs_qcom_is_link_active(hba)) {
> +		err = ufs_qcom_enable_lane_clks(host);
> +		if (err)
> +			return err;
> +	}
>  
> -out:
> -	return err;
> +	hba->is_sys_suspended = false;
> +	return 0;
>  }
>  
>  struct ufs_qcom_dev_params {
> @@ -1118,8 +1103,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  		return 0;
>  
>  	if (on && (status == POST_CHANGE)) {
> -		phy_power_on(host->generic_phy);
> -
>  		/* enable the device ref clock for HS mode*/
>  		if (ufshcd_is_hs_mode(&hba->pwr_info))
>  			ufs_qcom_dev_ref_clk_ctrl(host, true);
> @@ -1131,9 +1114,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  		if (!ufs_qcom_is_link_active(hba)) {
>  			/* disable device ref_clk */
>  			ufs_qcom_dev_ref_clk_ctrl(host, false);
> -
> -			/* powering off PHY during aggressive clk gating */
> -			phy_power_off(host->generic_phy);
>  		}
>  
>  		vote = host->bus_vote.min_bw_vote;
> @@ -1147,6 +1127,39 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  	return err;
>  }
>  
> +static int
> +ufs_qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +	WARN_ON(id);
> +	ufs_qcom_assert_reset(host->hba);
> +	/* provide 1ms delay to let the reset pulse propagate */
> +	usleep_range(1000, 1100);
> +	return 0;
> +}
> +
> +static int
> +ufs_qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +	WARN_ON(id);
> +	ufs_qcom_deassert_reset(host->hba);
> +
> +	/*
> +	 * after reset deassertion, phy will need all ref clocks,
> +	 * voltage, current to settle down before starting serdes.
> +	 */
> +	usleep_range(1000, 1100);
> +	return 0;
> +}
> +
> +const struct reset_control_ops ufs_qcom_reset_ops = {
> +	.assert = ufs_qcom_reset_assert,
> +	.deassert = ufs_qcom_reset_deassert,
> +};
> +
>  #define	ANDROID_BOOT_DEV_MAX	30
>  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
>  
> @@ -1191,6 +1204,15 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>  	host->hba = hba;
>  	ufshcd_set_variant(hba, host);
>  
> +	/* Fire up the reset controller. Failure here is non-fatal. */
> +	host->rcdev.of_node = dev->of_node;
> +	host->rcdev.ops = &ufs_qcom_reset_ops;
> +	host->rcdev.owner = dev->driver->owner;
> +	host->rcdev.nr_resets = 1;
> +	err = devm_reset_controller_register(dev, &host->rcdev);
> +	if (err)
> +		dev_warn(dev, "Failed to register reset controller\n");
> +
>  	/*
>  	 * voting/devoting device ref_clk source is time consuming hence
>  	 * skip devoting it during aggressive clock gating. This clock
> diff --git a/drivers/scsi/ufs/ufs-qcom.h b/drivers/scsi/ufs/ufs-qcom.h
> index c114826316eb0..68a8801857529 100644
> --- a/drivers/scsi/ufs/ufs-qcom.h
> +++ b/drivers/scsi/ufs/ufs-qcom.h
> @@ -14,6 +14,8 @@
>  #ifndef UFS_QCOM_H_
>  #define UFS_QCOM_H_
>  
> +#include <linux/reset-controller.h>
> +
>  #define MAX_UFS_QCOM_HOSTS	1
>  #define MAX_U32                 (~(u32)0)
>  #define MPHY_TX_FSM_STATE       0x41
> @@ -237,6 +239,8 @@ struct ufs_qcom_host {
>  	/* Bitmask for enabling debug prints */
>  	u32 dbg_print_en;
>  	struct ufs_qcom_testbus testbus;
> +
> +	struct reset_controller_dev rcdev;
>  };
>  
>  static inline u32
> 

Re: [PATCH v1 2/8] dt-bindings: phy: qcom-ufs: Add resets property

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:23)
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> index 21d9a93db2e97..985f5e99ab332 100644
> --- a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> @@ -29,6 +29,7 @@ Optional properties:
>  - vdda-pll-max-microamp : specifies max. load that can be drawn from pll supply
>  - vddp-ref-clk-supply   : phandle to UFS device ref_clk pad power supply
>  - vddp-ref-clk-max-microamp : specifies max. load that can be drawn from this supply
> +- resets : specifies the PHY reset in the UFS controller

Nitpick: Can you also add it to the example?

Re: [PATCH v1 2/8] dt-bindings: phy: qcom-ufs: Add resets property

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:23)
> diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> index 21d9a93db2e97..985f5e99ab332 100644
> --- a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> +++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> @@ -29,6 +29,7 @@ Optional properties:
>  - vdda-pll-max-microamp : specifies max. load that can be drawn from pll supply
>  - vddp-ref-clk-supply   : phandle to UFS device ref_clk pad power supply
>  - vddp-ref-clk-max-microamp : specifies max. load that can be drawn from this supply
> +- resets : specifies the PHY reset in the UFS controller

Nitpick: Can you also add it to the example?

Re: [PATCH v1 0/8] phy: qcom-ufs: Enable regulators to be off in suspend

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:21)
> 
> Because the UFS PHY reset bit is now toggled in the PHY, rather
> than in ufs-qcom, this also percolated to all other PHYs using
> ufs-qcom, which from what I can see is just 8996.
> 
> There are a couple of tradeoffs in this series that I'd welcome feedback
> on. First, it breaks compatibility with device trees that don't expose
> this new reset controller. Making this work with older device trees
> would be pretty ugly in the code, and given that the SDM845 UFS DT nodes
> aren't accepted upstream yet, the breakage seemed worth it. I'm not as
> sure about 8996.

It may take some minor surgery to the reset framework, but it should be
possible to register a reset lookup that can be used if nothing matches
in DT. Right now the reset code looks to only want to use DT if it's
there for the device requesting the reset. If there isn't a DT node for
the device it will look in the global lookup list. That could be changed
to fallback to the global lookup that uses device names (similar to clk
framework) when the DT lookup fails. Then it's just a matter of
registering the lookup for this reset with the handful of device names
that need the non-DT way of finding the reset.

Of course, that's another change and if breaking DT is simpler and
acceptable then I would say go for the path of least resistance and
don't try to modify reset framework for this.

Re: [PATCH v1 0/8] phy: qcom-ufs: Enable regulators to be off in suspend

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:21)
> 
> Because the UFS PHY reset bit is now toggled in the PHY, rather
> than in ufs-qcom, this also percolated to all other PHYs using
> ufs-qcom, which from what I can see is just 8996.
> 
> There are a couple of tradeoffs in this series that I'd welcome feedback
> on. First, it breaks compatibility with device trees that don't expose
> this new reset controller. Making this work with older device trees
> would be pretty ugly in the code, and given that the SDM845 UFS DT nodes
> aren't accepted upstream yet, the breakage seemed worth it. I'm not as
> sure about 8996.

It may take some minor surgery to the reset framework, but it should be
possible to register a reset lookup that can be used if nothing matches
in DT. Right now the reset code looks to only want to use DT if it's
there for the device requesting the reset. If there isn't a DT node for
the device it will look in the global lookup list. That could be changed
to fallback to the global lookup that uses device names (similar to clk
framework) when the DT lookup fails. Then it's just a matter of
registering the lookup for this reset with the handful of device names
that need the non-DT way of finding the reset.

Of course, that's another change and if breaking DT is simpler and
acceptable then I would say go for the path of least resistance and
don't try to modify reset framework for this.

Re: [PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Martin K. Petersen]

Kishon,

> On 12/01/19 4:31 AM, Evan Green wrote:
>> Expose a reset controller that the phy can use to perform its
>> initialization in a single callback.
>> 
>> Also, change the use of the phy functions from ufs-qcom such that
>> phy_poweron actually fires up the phy, and phy_poweroff actually
>> powers it down.
>> 
>> Signed-off-by: Evan Green <evgreen@chromium.org>
>
> Can I get Ack for this patch from SCSI MAINTAINERS?

No objection from me if there is general consensus that moving reset to
the phy is the right thing to do.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Martin K. Petersen]

Kishon,

> On 12/01/19 4:31 AM, Evan Green wrote:
>> Expose a reset controller that the phy can use to perform its
>> initialization in a single callback.
>> 
>> Also, change the use of the phy functions from ufs-qcom such that
>> phy_poweron actually fires up the phy, and phy_poweroff actually
>> powers it down.
>> 
>> Signed-off-by: Evan Green <evgreen@chromium.org>
>
> Can I get Ack for this patch from SCSI MAINTAINERS?

No objection from me if there is general consensus that moving reset to
the phy is the right thing to do.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v1 3/8] arm64: dts: sdm845: Add UFS PHY reset

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:24)
> Wire up the reset controller in the Qcom UFS controller for the PHY.
> This will be used to toggle PHY reset during initialization of the PHY.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v1 3/8] arm64: dts: sdm845: Add UFS PHY reset

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:24)
> Wire up the reset controller in the Qcom UFS controller for the PHY.
> This will be used to toggle PHY reset during initialization of the PHY.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v1 4/8] arm64: dts: msm8996: Add UFS PHY reset controller

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:25)
> Add the reset controller for the UFS controller, and wire it up
> so that the UFS PHY can initialize itself without relying on
> implicit sequencing between the two drivers.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v1 4/8] arm64: dts: msm8996: Add UFS PHY reset controller

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:25)
> Add the reset controller for the UFS controller, and wire it up
> so that the UFS PHY can initialize itself without relying on
> implicit sequencing between the two drivers.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:26)
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 3aeadb14aae1e..db46f9a64b54c 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
> +#include <linux/reset.h>

Shouldn't this be <linux/reset-controller.h>?

>  
>  #include "ufshcd.h"
>  #include "ufshcd-pltfrm.h"
> @@ -255,11 +261,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>         if (is_rate_B)
>                 phy_set_mode(phy, PHY_MODE_UFS_HS_B);
>  
> -       /* Assert PHY reset and apply PHY calibration values */
> -       ufs_qcom_assert_reset(hba);
> -       /* provide 1ms delay to let the reset pulse propagate */
> -       usleep_range(1000, 1100);
> -
>         /* phy initialization - calibrate the phy */
>         ret = phy_init(phy);
>         if (ret) {
> @@ -268,15 +269,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>                 goto out;
>         }
>  
> -       /* De-assert PHY reset and start serdes */
> -       ufs_qcom_deassert_reset(hba);
> -
> -       /*
> -        * after reset deassertion, phy will need all ref clocks,
> -        * voltage, current to settle down before starting serdes.
> -        */
> -       usleep_range(1000, 1100);
> -
>         /* power on phy - start serdes and phy's power and clocks */
>         ret = phy_power_on(phy);
>         if (ret) {
> @@ -290,7 +282,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>         return 0;
>  
>  out_disable_phy:
> -       ufs_qcom_assert_reset(hba);
>         phy_exit(phy);
>  out:
>         return ret;
> @@ -554,21 +545,10 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>                 ufs_qcom_disable_lane_clks(host);
>                 phy_power_off(phy);
>  
> -               /* Assert PHY soft reset */
> -               ufs_qcom_assert_reset(hba);
> -               goto out;
> -       }
> -
> -       /*
> -        * If UniPro link is not active, PHY ref_clk, main PHY analog power
> -        * rail and low noise analog power rail for PLL can be switched off.

We lost this comment?

> -        */
> -       if (!ufs_qcom_is_link_active(hba)) {
> +       } else if (!ufs_qcom_is_link_active(hba)) {
>                 ufs_qcom_disable_lane_clks(host);
> -               phy_power_off(phy);

And now this looks similar to the above if statement, so can they be
combined?

>  
> -out:
>         return ret;
>  }
>  
> @@ -578,21 +558,26 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>         struct phy *phy = host->generic_phy;
>         int err;
>  
> -       err = phy_power_on(phy);
> -       if (err) {
> -               dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
> -                       __func__, err);
> -               goto out;
> -       }
> +       if (ufs_qcom_is_link_off(hba)) {
> +               err = phy_power_on(phy);
> +               if (err) {
> +                       dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",

Not a problem with this translation, but I would expect this error to
say something more like 'failed to power on phy' instead of 'enabling
regs'.

> +                               __func__, err);
> +                       return err;
> +               }
>  
> -       err = ufs_qcom_enable_lane_clks(host);
> -       if (err)
> -               goto out;
> +               err = ufs_qcom_enable_lane_clks(host);
> +               if (err)
> +                       return err;
>  
> -       hba->is_sys_suspended = false;
> +       } else if (!ufs_qcom_is_link_active(hba)) {
> +               err = ufs_qcom_enable_lane_clks(host);
> +               if (err)
> +                       return err;
> +       }
>  
> -out:
> -       return err;
> +       hba->is_sys_suspended = false;
> +       return 0;
>  }
>  
>  struct ufs_qcom_dev_params {
> @@ -1118,8 +1103,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>                 return 0;
>  
>         if (on && (status == POST_CHANGE)) {
> -               phy_power_on(host->generic_phy);
> -

How is it ok to remove this call here?

>                 /* enable the device ref clock for HS mode*/
>                 if (ufshcd_is_hs_mode(&hba->pwr_info))
>                         ufs_qcom_dev_ref_clk_ctrl(host, true);
> @@ -1131,9 +1114,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>                 if (!ufs_qcom_is_link_active(hba)) {
>                         /* disable device ref_clk */
>                         ufs_qcom_dev_ref_clk_ctrl(host, false);
> -
> -                       /* powering off PHY during aggressive clk gating */
> -                       phy_power_off(host->generic_phy);

And here?

>                 }
>  
>                 vote = host->bus_vote.min_bw_vote;
> @@ -1147,6 +1127,39 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>         return err;
>  }
>  
> +static int
> +ufs_qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +       struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +       WARN_ON(id);

Nitpick: Add a comment explaining that there's only one reset expected?

> +       ufs_qcom_assert_reset(host->hba);
> +       /* provide 1ms delay to let the reset pulse propagate */
> +       usleep_range(1000, 1100);
> +       return 0;
> +}
> +
> +static int
> +ufs_qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +       struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +       WARN_ON(id);

Same nitpick.

> +       ufs_qcom_deassert_reset(host->hba);
> +
> +       /*
> +        * after reset deassertion, phy will need all ref clocks,
> +        * voltage, current to settle down before starting serdes.
> +        */
> +       usleep_range(1000, 1100);
> +       return 0;
> +}
> +
> +const struct reset_control_ops ufs_qcom_reset_ops = {

Can it be static?

> +       .assert = ufs_qcom_reset_assert,
> +       .deassert = ufs_qcom_reset_deassert,
> +};
> +
>  #define        ANDROID_BOOT_DEV_MAX    30
>  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
>  

Re: [PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:26)
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 3aeadb14aae1e..db46f9a64b54c 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
> +#include <linux/reset.h>

Shouldn't this be <linux/reset-controller.h>?

>  
>  #include "ufshcd.h"
>  #include "ufshcd-pltfrm.h"
> @@ -255,11 +261,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>         if (is_rate_B)
>                 phy_set_mode(phy, PHY_MODE_UFS_HS_B);
>  
> -       /* Assert PHY reset and apply PHY calibration values */
> -       ufs_qcom_assert_reset(hba);
> -       /* provide 1ms delay to let the reset pulse propagate */
> -       usleep_range(1000, 1100);
> -
>         /* phy initialization - calibrate the phy */
>         ret = phy_init(phy);
>         if (ret) {
> @@ -268,15 +269,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>                 goto out;
>         }
>  
> -       /* De-assert PHY reset and start serdes */
> -       ufs_qcom_deassert_reset(hba);
> -
> -       /*
> -        * after reset deassertion, phy will need all ref clocks,
> -        * voltage, current to settle down before starting serdes.
> -        */
> -       usleep_range(1000, 1100);
> -
>         /* power on phy - start serdes and phy's power and clocks */
>         ret = phy_power_on(phy);
>         if (ret) {
> @@ -290,7 +282,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
>         return 0;
>  
>  out_disable_phy:
> -       ufs_qcom_assert_reset(hba);
>         phy_exit(phy);
>  out:
>         return ret;
> @@ -554,21 +545,10 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>                 ufs_qcom_disable_lane_clks(host);
>                 phy_power_off(phy);
>  
> -               /* Assert PHY soft reset */
> -               ufs_qcom_assert_reset(hba);
> -               goto out;
> -       }
> -
> -       /*
> -        * If UniPro link is not active, PHY ref_clk, main PHY analog power
> -        * rail and low noise analog power rail for PLL can be switched off.

We lost this comment?

> -        */
> -       if (!ufs_qcom_is_link_active(hba)) {
> +       } else if (!ufs_qcom_is_link_active(hba)) {
>                 ufs_qcom_disable_lane_clks(host);
> -               phy_power_off(phy);

And now this looks similar to the above if statement, so can they be
combined?

>  
> -out:
>         return ret;
>  }
>  
> @@ -578,21 +558,26 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>         struct phy *phy = host->generic_phy;
>         int err;
>  
> -       err = phy_power_on(phy);
> -       if (err) {
> -               dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
> -                       __func__, err);
> -               goto out;
> -       }
> +       if (ufs_qcom_is_link_off(hba)) {
> +               err = phy_power_on(phy);
> +               if (err) {
> +                       dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",

Not a problem with this translation, but I would expect this error to
say something more like 'failed to power on phy' instead of 'enabling
regs'.

> +                               __func__, err);
> +                       return err;
> +               }
>  
> -       err = ufs_qcom_enable_lane_clks(host);
> -       if (err)
> -               goto out;
> +               err = ufs_qcom_enable_lane_clks(host);
> +               if (err)
> +                       return err;
>  
> -       hba->is_sys_suspended = false;
> +       } else if (!ufs_qcom_is_link_active(hba)) {
> +               err = ufs_qcom_enable_lane_clks(host);
> +               if (err)
> +                       return err;
> +       }
>  
> -out:
> -       return err;
> +       hba->is_sys_suspended = false;
> +       return 0;
>  }
>  
>  struct ufs_qcom_dev_params {
> @@ -1118,8 +1103,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>                 return 0;
>  
>         if (on && (status == POST_CHANGE)) {
> -               phy_power_on(host->generic_phy);
> -

How is it ok to remove this call here?

>                 /* enable the device ref clock for HS mode*/
>                 if (ufshcd_is_hs_mode(&hba->pwr_info))
>                         ufs_qcom_dev_ref_clk_ctrl(host, true);
> @@ -1131,9 +1114,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>                 if (!ufs_qcom_is_link_active(hba)) {
>                         /* disable device ref_clk */
>                         ufs_qcom_dev_ref_clk_ctrl(host, false);
> -
> -                       /* powering off PHY during aggressive clk gating */
> -                       phy_power_off(host->generic_phy);

And here?

>                 }
>  
>                 vote = host->bus_vote.min_bw_vote;
> @@ -1147,6 +1127,39 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>         return err;
>  }
>  
> +static int
> +ufs_qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +       struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +       WARN_ON(id);

Nitpick: Add a comment explaining that there's only one reset expected?

> +       ufs_qcom_assert_reset(host->hba);
> +       /* provide 1ms delay to let the reset pulse propagate */
> +       usleep_range(1000, 1100);
> +       return 0;
> +}
> +
> +static int
> +ufs_qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +       struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> +
> +       WARN_ON(id);

Same nitpick.

> +       ufs_qcom_deassert_reset(host->hba);
> +
> +       /*
> +        * after reset deassertion, phy will need all ref clocks,
> +        * voltage, current to settle down before starting serdes.
> +        */
> +       usleep_range(1000, 1100);
> +       return 0;
> +}
> +
> +const struct reset_control_ops ufs_qcom_reset_ops = {

Can it be static?

> +       .assert = ufs_qcom_reset_assert,
> +       .deassert = ufs_qcom_reset_deassert,
> +};
> +
>  #define        ANDROID_BOOT_DEV_MAX    30
>  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
>  

Re: [PATCH v1 6/8] phy: qcom-qmp: Utilize UFS reset controller

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:27)
> @@ -1214,6 +1225,32 @@ static int qcom_qmp_phy_init(struct phy *phy)
>  
>         dev_vdbg(qmp->dev, "Initializing QMP phy\n");
>  
> +       if (cfg->has_ufsphy_reset) {
> +               /*
> +                * Get UFS reset, which is delayed until now to avoid a
> +                * circular dependency where UFS needs its PHY, but the PHY
> +                * needs this UFS reset.
> +                */
> +               if (!qmp->ufs_reset) {
> +                       qmp->ufs_reset = of_reset_control_get(qmp->dev->of_node,

Can you use devm_reset_control_get()? Put another way, why is this DT
specific instead of using a firmware/platform agnostic API?

> +                                                             "ufsphy");
> +
> +                       if (IS_ERR(qmp->ufs_reset)) {
> +                               dev_err(qmp->dev,
> +                                       "failed to get UFS reset: %d\n",
> +                                       PTR_ERR(qmp->ufs_reset));
> +
> +                               return PTR_ERR(qmp->ufs_reset);
> +                       }
> +               }
> +
> +               ret = reset_control_assert(qmp->ufs_reset);
> +               if (ret) {
> +                       dev_err(qmp->dev, "ufsphy reset deassert failed\n");

It's an assert though. Maybe just ignore the error message because the
user won't be able to do anything anyway?

Re: [PATCH v1 7/8] phy: qcom-qmp: Move UFS phy to phy_poweron/off

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:28)
> For UFS, move the actual firing up of the PHY to phy_poweron and
> phy_poweroff callbacks, rather than init/exit. UFS calls
> phy_poweroff during suspend, so now all clocks and regulators for
> the phy can be powered down during suspend.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> 
> ---
> 
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 82 ++++++++---------------------
>  1 file changed, 23 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index eb1cac8f0fd4e..7766c6384d0a8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -1224,7 +1223,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
>         int ret;
>  
>         dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> -
>         if (cfg->has_ufsphy_reset) {
>                 /*
>                  * Get UFS reset, which is delayed until now to avoid a

Nitpick: Drop this hunk.

> @@ -1360,7 +1353,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
>  
>         /* Put PHY into POWER DOWN state: active low */
>         qphy_clrbits(qphy->pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
> -
>         if (cfg->has_lane_rst)
>                 reset_control_assert(qphy->lane_rst);
>  

And this hunk.

> @@ -1371,44 +1363,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
>         return 0;
>  }
>  
> -static int qcom_qmp_phy_poweron(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;
> -       void __iomem *pcs = qphy->pcs;
> -       void __iomem *status;
> -       unsigned int mask, val;
> -       int ret = 0;
> -
> -       if (cfg->type != PHY_TYPE_UFS)
> -               return 0;
> -
> -       /*
> -        * For UFS PHY that has not software reset control, serdes start
> -        * should only happen when UFS driver explicitly calls phy_power_on
> -        * after it deasserts software reset.
> -        */
> -       if (cfg->no_pcs_sw_reset && !qmp->phy_initialized &&
> -           (qmp->init_count != 0)) {
> -               /* start SerDes and Phy-Coding-Sublayer */
> -               qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
> -
> -               status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
> -               mask = cfg->mask_pcs_ready;
> -
> -               ret = readl_poll_timeout(status, val, !(val & mask), 1,
> -                                        PHY_INIT_COMPLETE_TIMEOUT);

So we never need to poll this bit anymore?

> -               if (ret) {
> -                       dev_err(qmp->dev, "phy initialization timed-out\n");
> -                       return ret;
> -               }
> -               qmp->phy_initialized = true;
> -       }
> -
> -       return ret;
> -}
> -
>  static int qcom_qmp_phy_set_mode(struct phy *phy,
>                                  enum phy_mode mode, int submode)
>  {
> @@ -1658,9 +1612,15 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>  }
>  
>  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,
> +       .init           = qcom_qmp_phy_enable,
> +       .exit           = qcom_qmp_phy_disable,
> +       .set_mode       = qcom_qmp_phy_set_mode,
> +       .owner          = THIS_MODULE,
> +};
> +
> +static const struct phy_ops qcom_qmp_ufs_ops = {
> +       .power_on       = qcom_qmp_phy_enable,
> +       .power_off      = qcom_qmp_phy_disable,
>         .set_mode       = qcom_qmp_phy_set_mode,
>         .owner          = THIS_MODULE,
>  };

So the UFS and the non-UFS phys will use the same single function, but
the callers of the phys will see that phy_power_on() powers on the phy
for UFS but does nothing for non-UFS devices? Do the users of this
common phy call the API differently between drivers? Kishon, is there
guidance on how phys are supposed to be used by drivers?

Re: [PATCH v1 8/8] phy: qcom-ufs: Refactor all init steps into phy_poweron

[Stephen Boyd]

Quoting Evan Green (2019-01-11 15:01:29)
> diff --git a/drivers/phy/qualcomm/phy-qcom-ufs.c b/drivers/phy/qualcomm/phy-qcom-ufs.c
> index f2979ccad00a3..a4cff17fef925 100644
> --- a/drivers/phy/qualcomm/phy-qcom-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ufs.c
> @@ -147,6 +147,21 @@ struct phy *ufs_qcom_phy_generic_probe(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(ufs_qcom_phy_generic_probe);
>  
> +static int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common)
> +{
> +       struct reset_control *reset;
> +
> +       if (phy_common->ufs_reset)
> +               return 0;
> +
> +       reset = of_reset_control_get_by_index(phy_common->dev->of_node, 0);

Same question about using non-DT specific APIs to get the reset control
here.

> +       if (IS_ERR(reset))
> +               return PTR_ERR(reset);
> +
> +       phy_common->ufs_reset = reset;
> +       return 0;
> +}
> +
>  static int __ufs_qcom_phy_clk_get(struct device *dev,
>                          const char *name, struct clk **clk_out, bool err_print)
>  {
> @@ -528,23 +543,42 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
>  {
>         struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
>         struct device *dev = phy_common->dev;
> +       bool is_rate_B = false;
>         int err;
>  
> -       if (phy_common->is_powered_on)
> -               return 0;
> +       err = ufs_qcom_phy_get_reset(phy_common);
> +       if (err)
> +               return err;
>  
> -       if (!phy_common->is_started) {
> -               err = ufs_qcom_phy_start_serdes(phy_common);
> +       if (phy_common->ufs_reset) {
> +               err = reset_control_assert(phy_common->ufs_reset);
>                 if (err)
>                         return err;
> +       }
>  
> -               err = ufs_qcom_phy_is_pcs_ready(phy_common);
> -               if (err)
> -                       return err;
> +       if (phy_common->mode == PHY_MODE_UFS_HS_B)
> +               is_rate_B = true;
>  
> -               phy_common->is_started = true;
> +       err = phy_common->phy_spec_ops->calibrate(phy_common, is_rate_B);

Is there always a calibrate phys_spec_ops function assigned?

> +       if (err)
> +               return err;
> +
> +       if (phy_common->ufs_reset) {
> +               err = reset_control_deassert(phy_common->ufs_reset);
> +               if (err) {
> +                       dev_err(dev, "Failed to assert UFS PHY reset");
> +                       return err;
> +               }
>         }

Re: [PATCH v1 1/8] dt-bindings: phy-qcom-qmp: Add UFS PHY reset

[Rob Herring]

On Fri, 11 Jan 2019 15:01:22 -0800, Evan Green wrote:
> Add a required reset to the SDM845 UFS phy to express the PHY reset
> bit inside the UFS controller register space. Before this change, this
> reset was not expressed in the DT, and the driver utilized two different
> callbacks (phy_init and phy_poweron) to implement a two-phase
> initialization procedure that involved deasserting this reset between
> init and poweron. This abused the two callbacks and diluted their
> purpose.
> 
> That scheme does not work as regulators cannot be turned off in
> phy_poweroff because they were turned on in init, rather than poweron.
> The net result is that regulators are left on in suspend that shouldn't
> be.
> 
> This new scheme gives the UFS reset to the PHY, so that it can fully
> initialize itself in a single callback. We can then turn regulators on
> during poweron and off during poweroff.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> I realize I'm not supposed to add a required property after the fact,
> but given that the UFS DT nodes that would use this binding are not
> yet upstream (and this would be the first), I was hoping to squeak by.
> 
>  Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

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

Re: [PATCH v1 1/8] dt-bindings: phy-qcom-qmp: Add UFS PHY reset

[Rob Herring]

On Fri, 11 Jan 2019 15:01:22 -0800, Evan Green wrote:
> Add a required reset to the SDM845 UFS phy to express the PHY reset
> bit inside the UFS controller register space. Before this change, this
> reset was not expressed in the DT, and the driver utilized two different
> callbacks (phy_init and phy_poweron) to implement a two-phase
> initialization procedure that involved deasserting this reset between
> init and poweron. This abused the two callbacks and diluted their
> purpose.
> 
> That scheme does not work as regulators cannot be turned off in
> phy_poweroff because they were turned on in init, rather than poweron.
> The net result is that regulators are left on in suspend that shouldn't
> be.
> 
> This new scheme gives the UFS reset to the PHY, so that it can fully
> initialize itself in a single callback. We can then turn regulators on
> during poweron and off during poweroff.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> I realize I'm not supposed to add a required property after the fact,
> but given that the UFS DT nodes that would use this binding are not
> yet upstream (and this would be the first), I was hoping to squeak by.
> 
>  Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

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

Re: [PATCH v1 3/8] arm64: dts: sdm845: Add UFS PHY reset

[Rob Herring]

On Fri, Jan 11, 2019 at 03:01:24PM -0800, Evan Green wrote:
> Wire up the reset controller in the Qcom UFS controller for the PHY.
> This will be used to toggle PHY reset during initialization of the PHY.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> This commit is based atop the series at [1]. Patches 1 and 2 of that
> series have landed, but 3, 4, and 5 are still outstanding.
> 
> [1] https://lore.kernel.org/lkml/20181210192826.241350-1-evgreen@chromium.org/
> 
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index b29332b265d9e..029ab66405cf4 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -990,6 +990,7 @@
>  			phy-names = "ufsphy";
>  			lanes-per-direction = <2>;
>  			power-domains = <&gcc UFS_PHY_GDSC>;
> +			#reset-cells = <1>;

Is this documented?

>  
>  			clock-names =
>  				"core_clk",
> @@ -1033,6 +1034,8 @@
>  			clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>,
>  				 <&gcc GCC_UFS_PHY_PHY_AUX_CLK>;
>  
> +			resets = <&ufs_mem_hc 0>;
> +			reset-names = "ufsphy";
>  			status = "disabled";
>  
>  			ufs_mem_phy_lanes: lanes@1d87400 {
> -- 
> 2.18.1
> 

Re: [PATCH v1 2/8] dt-bindings: phy: qcom-ufs: Add resets property

[Rob Herring]

On Fri, 11 Jan 2019 15:01:23 -0800, Evan Green wrote:
> Add a resets property to the PHY that represents the PHY reset
> register in the UFS controller itself. This better describes the
> complete specification of the PHY, and allows the PHY to perform
> its initialization in a single function, rather than relying on
> back-channel sequencing of initialization through the PHY framework.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> 
>  Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

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

Re: [PATCH v1 2/8] dt-bindings: phy: qcom-ufs: Add resets property

[Rob Herring]

On Fri, 11 Jan 2019 15:01:23 -0800, Evan Green wrote:
> Add a resets property to the PHY that represents the PHY reset
> register in the UFS controller itself. This better describes the
> complete specification of the PHY, and allows the PHY to perform
> its initialization in a single function, rather than relying on
> back-channel sequencing of initialization through the PHY framework.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> 
>  Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

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

Re: [PATCH v1 3/8] arm64: dts: sdm845: Add UFS PHY reset

[Evan Green]

On Mon, Jan 21, 2019 at 4:25 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Jan 11, 2019 at 03:01:24PM -0800, Evan Green wrote:
> > Wire up the reset controller in the Qcom UFS controller for the PHY.
> > This will be used to toggle PHY reset during initialization of the PHY.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> > This commit is based atop the series at [1]. Patches 1 and 2 of that
> > series have landed, but 3, 4, and 5 are still outstanding.
> >
> > [1] https://lore.kernel.org/lkml/20181210192826.241350-1-evgreen@chromium.org/
> >
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index b29332b265d9e..029ab66405cf4 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -990,6 +990,7 @@
> >                       phy-names = "ufsphy";
> >                       lanes-per-direction = <2>;
> >                       power-domains = <&gcc UFS_PHY_GDSC>;
> > +                     #reset-cells = <1>;
>
> Is this documented?
>

No, I'll add this in my next spin. Thanks.
-Evan

Re: [PATCH v1 2/8] dt-bindings: phy: qcom-ufs: Add resets property

[Evan Green]

On Wed, Jan 16, 2019 at 1:29 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Evan Green (2019-01-11 15:01:23)
> > diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> > index 21d9a93db2e97..985f5e99ab332 100644
> > --- a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> > +++ b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
> > @@ -29,6 +29,7 @@ Optional properties:
> >  - vdda-pll-max-microamp : specifies max. load that can be drawn from pll supply
> >  - vddp-ref-clk-supply   : phandle to UFS device ref_clk pad power supply
> >  - vddp-ref-clk-max-microamp : specifies max. load that can be drawn from this supply
> > +- resets : specifies the PHY reset in the UFS controller
>
> Nitpick: Can you also add it to the example?
>

Sure, will do.

Re: [PATCH v1 5/8] scsi: ufs: qcom: Expose the reset controller for PHY

[Evan Green]

On Fri, Jan 18, 2019 at 2:31 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Evan Green (2019-01-11 15:01:26)
> > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > index 3aeadb14aae1e..db46f9a64b54c 100644
> > --- a/drivers/scsi/ufs/ufs-qcom.c
> > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/phy/phy.h>
> > +#include <linux/reset.h>
>
> Shouldn't this be <linux/reset-controller.h>?

Oh, actually I don't need this at all since ufs-qcom.h includes
reset-controller.h. Will remove.

>
> >
> >  #include "ufshcd.h"
> >  #include "ufshcd-pltfrm.h"
> > @@ -255,11 +261,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> >         if (is_rate_B)
> >                 phy_set_mode(phy, PHY_MODE_UFS_HS_B);
> >
> > -       /* Assert PHY reset and apply PHY calibration values */
> > -       ufs_qcom_assert_reset(hba);
> > -       /* provide 1ms delay to let the reset pulse propagate */
> > -       usleep_range(1000, 1100);
> > -
> >         /* phy initialization - calibrate the phy */
> >         ret = phy_init(phy);
> >         if (ret) {
> > @@ -268,15 +269,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> >                 goto out;
> >         }
> >
> > -       /* De-assert PHY reset and start serdes */
> > -       ufs_qcom_deassert_reset(hba);
> > -
> > -       /*
> > -        * after reset deassertion, phy will need all ref clocks,
> > -        * voltage, current to settle down before starting serdes.
> > -        */
> > -       usleep_range(1000, 1100);
> > -
> >         /* power on phy - start serdes and phy's power and clocks */
> >         ret = phy_power_on(phy);
> >         if (ret) {
> > @@ -290,7 +282,6 @@ static int ufs_qcom_power_up_sequence(struct ufs_hba *hba)
> >         return 0;
> >
> >  out_disable_phy:
> > -       ufs_qcom_assert_reset(hba);
> >         phy_exit(phy);
> >  out:
> >         return ret;
> > @@ -554,21 +545,10 @@ static int ufs_qcom_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >                 ufs_qcom_disable_lane_clks(host);
> >                 phy_power_off(phy);
> >
> > -               /* Assert PHY soft reset */
> > -               ufs_qcom_assert_reset(hba);
> > -               goto out;
> > -       }
> > -
> > -       /*
> > -        * If UniPro link is not active, PHY ref_clk, main PHY analog power
> > -        * rail and low noise analog power rail for PLL can be switched off.
>
> We lost this comment?

Yeah. These are all phy implementation choices, and phy-qcom-qmp
wasn't even doing any of this, so it didn't seem like an appropriate
comment for the UFS controller code.

>
> > -        */
> > -       if (!ufs_qcom_is_link_active(hba)) {
> > +       } else if (!ufs_qcom_is_link_active(hba)) {
> >                 ufs_qcom_disable_lane_clks(host);
> > -               phy_power_off(phy);
>
> And now this looks similar to the above if statement, so can they be
> combined?

well, the if statement above has an extra phy_power_off in it... the
only possible combining I see is this, which looks worse, doesn't it?

if (ufs_qcom_is_link_off(hba) || !ufs_qcom_is_link_active(hba)) {
    ufs_qcom_disable_lane_clocks(host);
    if (ufs_qcom_is_link_off(hba)) {
        phy_power_off(phy);
    }
}

>
> >
> > -out:
> >         return ret;
> >  }
> >
> > @@ -578,21 +558,26 @@ static int ufs_qcom_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
> >         struct phy *phy = host->generic_phy;
> >         int err;
> >
> > -       err = phy_power_on(phy);
> > -       if (err) {
> > -               dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
> > -                       __func__, err);
> > -               goto out;
> > -       }
> > +       if (ufs_qcom_is_link_off(hba)) {
> > +               err = phy_power_on(phy);
> > +               if (err) {
> > +                       dev_err(hba->dev, "%s: failed enabling regs, err = %d\n",
>
> Not a problem with this translation, but I would expect this error to
> say something more like 'failed to power on phy' instead of 'enabling
> regs'.

Oh yeah. Will fix.

>
> > +                               __func__, err);
> > +                       return err;
> > +               }
> >
> > -       err = ufs_qcom_enable_lane_clks(host);
> > -       if (err)
> > -               goto out;
> > +               err = ufs_qcom_enable_lane_clks(host);
> > +               if (err)
> > +                       return err;
> >
> > -       hba->is_sys_suspended = false;
> > +       } else if (!ufs_qcom_is_link_active(hba)) {
> > +               err = ufs_qcom_enable_lane_clks(host);
> > +               if (err)
> > +                       return err;
> > +       }
> >
> > -out:
> > -       return err;
> > +       hba->is_sys_suspended = false;
> > +       return 0;
> >  }
> >
> >  struct ufs_qcom_dev_params {
> > @@ -1118,8 +1103,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> >                 return 0;
> >
> >         if (on && (status == POST_CHANGE)) {
> > -               phy_power_on(host->generic_phy);
> > -
>
> How is it ok to remove this call here?
>
> >                 /* enable the device ref clock for HS mode*/
> >                 if (ufshcd_is_hs_mode(&hba->pwr_info))
> >                         ufs_qcom_dev_ref_clk_ctrl(host, true);
> > @@ -1131,9 +1114,6 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> >                 if (!ufs_qcom_is_link_active(hba)) {
> >                         /* disable device ref_clk */
> >                         ufs_qcom_dev_ref_clk_ctrl(host, false);
> > -
> > -                       /* powering off PHY during aggressive clk gating */
> > -                       phy_power_off(host->generic_phy);
>
> And here?

This pair was calling phy_power_on and phy_power_off during clock
gating (and init). For SDM845/phy-qcom-qmp, this did nothing, since
there was no phy_power_off. In fact they needed an extra patch to not
call phy_power_on too early during init because of this function [1].
So for sdm845 this is a noop, since the phy will already be powered on
in ufs_qcom_power_up_sequence.

For msm8996/phy-qcom-ufs, it may change behavior a bit. Where we used
to end up in ufs_qcom_phy_power_off during clock gating, this change
is now not doing that. So regulators and a couple clocks are being
left on during clock gating. The phy power off now happens in suspend
if usermode selects that level. It seemed weird to be doing a bunch of
regulator and power down stuff in something called "clock gating".

Although looking at it now, I'm not even sure if these calls really
did do anything, since phy_power_on is reference counted, and
ufs_qcom_power_up_sequence called it... so it's possible ever since
commit 052553af6a31 ("ufs/phy: qcom: Refactor to use phy_init call")
in late 2017 this hasn't been doing anything at all.

[1] https://lkml.org/lkml/2018/9/21/204

>
> >                 }
> >
> >                 vote = host->bus_vote.min_bw_vote;
> > @@ -1147,6 +1127,39 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
> >         return err;
> >  }
> >
> > +static int
> > +ufs_qcom_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +       struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> > +
> > +       WARN_ON(id);
>
> Nitpick: Add a comment explaining that there's only one reset expected?

Will do.

>
> > +       ufs_qcom_assert_reset(host->hba);
> > +       /* provide 1ms delay to let the reset pulse propagate */
> > +       usleep_range(1000, 1100);
> > +       return 0;
> > +}
> > +
> > +static int
> > +ufs_qcom_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +       struct ufs_qcom_host *host = rcdev_to_ufs_host(rcdev);
> > +
> > +       WARN_ON(id);
>
> Same nitpick.

Yep.

>
> > +       ufs_qcom_deassert_reset(host->hba);
> > +
> > +       /*
> > +        * after reset deassertion, phy will need all ref clocks,
> > +        * voltage, current to settle down before starting serdes.
> > +        */
> > +       usleep_range(1000, 1100);
> > +       return 0;
> > +}
> > +
> > +const struct reset_control_ops ufs_qcom_reset_ops = {
>
> Can it be static?

Yes!

>
> > +       .assert = ufs_qcom_reset_assert,
> > +       .deassert = ufs_qcom_reset_deassert,
> > +};
> > +
> >  #define        ANDROID_BOOT_DEV_MAX    30
> >  static char android_boot_dev[ANDROID_BOOT_DEV_MAX];
> >

Re: [PATCH v1 6/8] phy: qcom-qmp: Utilize UFS reset controller

[Evan Green]

On Fri, Jan 18, 2019 at 2:33 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Evan Green (2019-01-11 15:01:27)
> > @@ -1214,6 +1225,32 @@ static int qcom_qmp_phy_init(struct phy *phy)
> >
> >         dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> >
> > +       if (cfg->has_ufsphy_reset) {
> > +               /*
> > +                * Get UFS reset, which is delayed until now to avoid a
> > +                * circular dependency where UFS needs its PHY, but the PHY
> > +                * needs this UFS reset.
> > +                */
> > +               if (!qmp->ufs_reset) {
> > +                       qmp->ufs_reset = of_reset_control_get(qmp->dev->of_node,
>
> Can you use devm_reset_control_get()? Put another way, why is this DT
> specific instead of using a firmware/platform agnostic API?

Sure, will fix. (Though sadly of_* is peppered all over the place in
this file for lane resets, pipe clocks, and more).

>
> > +                                                             "ufsphy");
> > +
> > +                       if (IS_ERR(qmp->ufs_reset)) {
> > +                               dev_err(qmp->dev,
> > +                                       "failed to get UFS reset: %d\n",
> > +                                       PTR_ERR(qmp->ufs_reset));
> > +
> > +                               return PTR_ERR(qmp->ufs_reset);
> > +                       }
> > +               }
> > +
> > +               ret = reset_control_assert(qmp->ufs_reset);
> > +               if (ret) {
> > +                       dev_err(qmp->dev, "ufsphy reset deassert failed\n");
>
> It's an assert though. Maybe just ignore the error message because the
> user won't be able to do anything anyway?
>

Fair enough. I'll remove the print from here and the deassert later.
-Evan

Re: [PATCH v1 7/8] phy: qcom-qmp: Move UFS phy to phy_poweron/off

[Evan Green]

On Fri, Jan 18, 2019 at 2:39 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Evan Green (2019-01-11 15:01:28)
> > For UFS, move the actual firing up of the PHY to phy_poweron and
> > phy_poweroff callbacks, rather than init/exit. UFS calls
> > phy_poweroff during suspend, so now all clocks and regulators for
> > the phy can be powered down during suspend.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> >
> > ---
> >
> >  drivers/phy/qualcomm/phy-qcom-qmp.c | 82 ++++++++---------------------
> >  1 file changed, 23 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index eb1cac8f0fd4e..7766c6384d0a8 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -1224,7 +1223,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
> >         int ret;
> >
> >         dev_vdbg(qmp->dev, "Initializing QMP phy\n");
> > -
> >         if (cfg->has_ufsphy_reset) {
> >                 /*
> >                  * Get UFS reset, which is delayed until now to avoid a
>
> Nitpick: Drop this hunk.

Ok.

>
> > @@ -1360,7 +1353,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
> >
> >         /* Put PHY into POWER DOWN state: active low */
> >         qphy_clrbits(qphy->pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
> > -
> >         if (cfg->has_lane_rst)
> >                 reset_control_assert(qphy->lane_rst);
> >
>
> And this hunk.

Ok.

>
> > @@ -1371,44 +1363,6 @@ static int qcom_qmp_phy_exit(struct phy *phy)
> >         return 0;
> >  }
> >
> > -static int qcom_qmp_phy_poweron(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;
> > -       void __iomem *pcs = qphy->pcs;
> > -       void __iomem *status;
> > -       unsigned int mask, val;
> > -       int ret = 0;
> > -
> > -       if (cfg->type != PHY_TYPE_UFS)
> > -               return 0;
> > -
> > -       /*
> > -        * For UFS PHY that has not software reset control, serdes start
> > -        * should only happen when UFS driver explicitly calls phy_power_on
> > -        * after it deasserts software reset.
> > -        */
> > -       if (cfg->no_pcs_sw_reset && !qmp->phy_initialized &&
> > -           (qmp->init_count != 0)) {
> > -               /* start SerDes and Phy-Coding-Sublayer */
> > -               qphy_setbits(pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
> > -
> > -               status = pcs + cfg->regs[QPHY_PCS_READY_STATUS];
> > -               mask = cfg->mask_pcs_ready;
> > -
> > -               ret = readl_poll_timeout(status, val, !(val & mask), 1,
> > -                                        PHY_INIT_COMPLETE_TIMEOUT);
>
> So we never need to poll this bit anymore?

We do poll that bit, we just do it in qcom_qmp_phy_enable now, since I
removed the conditional that exited early for UFS PHYs. It's almost
like someone copied and pasted the lower half of the function into
qcom_qmp_phy_poweron :)

>
> > -               if (ret) {
> > -                       dev_err(qmp->dev, "phy initialization timed-out\n");
> > -                       return ret;
> > -               }
> > -               qmp->phy_initialized = true;
> > -       }
> > -
> > -       return ret;
> > -}
> > -
> >  static int qcom_qmp_phy_set_mode(struct phy *phy,
> >                                  enum phy_mode mode, int submode)
> >  {
> > @@ -1658,9 +1612,15 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
> >  }
> >
> >  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,
> > +       .init           = qcom_qmp_phy_enable,
> > +       .exit           = qcom_qmp_phy_disable,
> > +       .set_mode       = qcom_qmp_phy_set_mode,
> > +       .owner          = THIS_MODULE,
> > +};
> > +
> > +static const struct phy_ops qcom_qmp_ufs_ops = {
> > +       .power_on       = qcom_qmp_phy_enable,
> > +       .power_off      = qcom_qmp_phy_disable,
> >         .set_mode       = qcom_qmp_phy_set_mode,
> >         .owner          = THIS_MODULE,
> >  };
>
> So the UFS and the non-UFS phys will use the same single function, but
> the callers of the phys will see that phy_power_on() powers on the phy
> for UFS but does nothing for non-UFS devices? Do the users of this
> common phy call the API differently between drivers? Kishon, is there
> guidance on how phys are supposed to be used by drivers?
>

I'll leave that one for Kishon.
-Evan

Re: [PATCH v1 8/8] phy: qcom-ufs: Refactor all init steps into phy_poweron

[Evan Green]

On Fri, Jan 18, 2019 at 3:50 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Evan Green (2019-01-11 15:01:29)
> > diff --git a/drivers/phy/qualcomm/phy-qcom-ufs.c b/drivers/phy/qualcomm/phy-qcom-ufs.c
> > index f2979ccad00a3..a4cff17fef925 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-ufs.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-ufs.c
> > @@ -147,6 +147,21 @@ struct phy *ufs_qcom_phy_generic_probe(struct platform_device *pdev,
> >  }
> >  EXPORT_SYMBOL_GPL(ufs_qcom_phy_generic_probe);
> >
> > +static int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common)
> > +{
> > +       struct reset_control *reset;
> > +
> > +       if (phy_common->ufs_reset)
> > +               return 0;
> > +
> > +       reset = of_reset_control_get_by_index(phy_common->dev->of_node, 0);
>
> Same question about using non-DT specific APIs to get the reset control
> here.

Will fix.

>
> > +       if (IS_ERR(reset))
> > +               return PTR_ERR(reset);
> > +
> > +       phy_common->ufs_reset = reset;
> > +       return 0;
> > +}
> > +
> >  static int __ufs_qcom_phy_clk_get(struct device *dev,
> >                          const char *name, struct clk **clk_out, bool err_print)
> >  {
> > @@ -528,23 +543,42 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
> >  {
> >         struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
> >         struct device *dev = phy_common->dev;
> > +       bool is_rate_B = false;
> >         int err;
> >
> > -       if (phy_common->is_powered_on)
> > -               return 0;
> > +       err = ufs_qcom_phy_get_reset(phy_common);
> > +       if (err)
> > +               return err;
> >
> > -       if (!phy_common->is_started) {
> > -               err = ufs_qcom_phy_start_serdes(phy_common);
> > +       if (phy_common->ufs_reset) {
> > +               err = reset_control_assert(phy_common->ufs_reset);
> >                 if (err)
> >                         return err;
> > +       }
> >
> > -               err = ufs_qcom_phy_is_pcs_ready(phy_common);
> > -               if (err)
> > -                       return err;
> > +       if (phy_common->mode == PHY_MODE_UFS_HS_B)
> > +               is_rate_B = true;
> >
> > -               phy_common->is_started = true;
> > +       err = phy_common->phy_spec_ops->calibrate(phy_common, is_rate_B);
>
> Is there always a calibrate phys_spec_ops function assigned?

Yeah. This whole table is only used to support phy-qcom-ufs-qmp-14nm.c
and phy-qcom-ufs-qmp-20nm.c. I don't even see any device trees using
phy-qcom-ufs-qmp-20nm.c. So it seemed okay to assume it was populated.