[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 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

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

[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

[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];
> >