linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend
@ 2019-03-21 17:17 Evan Green
  2019-03-21 17:17 ` [PATCH v5 1/8] dt-bindings: ufs: Add #reset-cells for Qualcomm controllers Evan Green
                   ` (8 more replies)
  0 siblings, 9 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, devicetree,
	Pedro Sousa, liwei, linux-scsi, linux-arm-msm, Bart Van Assche,
	linux-kernel, Andy Gross, Subhash Jadavani, Martin K. Petersen,
	Alim Akhtar, Rob Herring, Avri Altman, Mark Rutland,
	James E.J. Bottomley, Janek Kotas, David Brown

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.

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.

Changes in v5:
- Updated tags

Changes in v4:
- Do reset_control_* unconditionally since null is handled (Stephen).
- Keep doing reset_control* unconditionally through refactor (Stephen).

Changes in v3:
- Refactor to only expose the reset controller in one change (Stephen).
- Add period to comment (Stephen).
- Reset err to 0 in ignored error case (Stephen).
- Add include of reset-controller.h (Stephen)
- Refactored to move reset control in a single commit (Stephen)
- Use no_pcs_sw_reset as an indicator of UFS reset in qmp-phy (Stephen).
- Assign ret = PTR_ERR() earlier, for better reuse (Stephen).
- Refactor init => poweron for all PHYs and UFS in one step (Stephen)

Changes in v2:
- Added resets to example (Stephen).
- Remove include of reset.h (Stephen)
- Fix error print of phy_power_on (Stephen)
- Comment for reset controller warnings on id != 0 (Stephen)
- Add static to ufs_qcom_reset_ops (Stephen).
- Use devm_* to get the reset (Stephen)
- Clear ufs_reset on error getting it
- Remove needless error print (Stephen)
- Use devm_ to get the reset (Stephen)
- Removed whitespace changes (Stephen)

Evan Green (8):
  dt-bindings: ufs: Add #reset-cells for Qualcomm controllers
  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: Utilize UFS reset controller
  phy: ufs-qcom: Refactor all init steps into phy_poweron

 .../devicetree/bindings/phy/qcom-qmp-phy.txt  |   6 +-
 .../devicetree/bindings/ufs/ufs-qcom.txt      |   5 +-
 .../devicetree/bindings/ufs/ufshcd-pltfrm.txt |   3 +
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |   4 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   3 +
 drivers/phy/qualcomm/phy-qcom-qmp.c           | 112 +++++++++--------
 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                   | 114 +++++++++++-------
 drivers/scsi/ufs/ufs-qcom.h                   |   4 +
 13 files changed, 193 insertions(+), 171 deletions(-)

-- 
2.20.1


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

* [PATCH v5 1/8] dt-bindings: ufs: Add #reset-cells for Qualcomm controllers
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
@ 2019-03-21 17:17 ` Evan Green
  2019-03-21 17:17 ` [PATCH v5 2/8] dt-bindings: phy-qcom-qmp: Add UFS PHY reset Evan Green
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, Rob Herring,
	devicetree, liwei, linux-kernel, Subhash Jadavani,
	Martin K. Petersen, Rob Herring, Mark Rutland

Enable Qualcomm UFS controllers to expose the PHY reset via a reset
controller.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>

---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
index 5111e9130bc3..f647c09bc62a 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -50,6 +50,8 @@ Optional properties:
 -lanes-per-direction	: number of lanes available per direction - either 1 or 2.
 			  Note that it is assume same number of lanes is used both
 			  directions at once. If not specified, default is 2 lanes per direction.
+- #reset-cells		: Must be <1> for Qualcomm UFS controllers that expose
+			  PHY reset from the UFS controller.
 - resets            : reset node register
 - reset-names       : describe reset node register, the "rst" corresponds to reset the whole UFS IP.
 
@@ -79,4 +81,5 @@ Example:
 		reset-names = "rst";
 		phys = <&ufsphy1>;
 		phy-names = "ufsphy";
+		#reset-cells = <1>;
 	};
-- 
2.20.1


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

* [PATCH v5 2/8] dt-bindings: phy-qcom-qmp: Add UFS PHY reset
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
  2019-03-21 17:17 ` [PATCH v5 1/8] dt-bindings: ufs: Add #reset-cells for Qualcomm controllers Evan Green
@ 2019-03-21 17:17 ` Evan Green
  2019-03-21 17:17 ` [PATCH v5 3/8] dt-bindings: phy: qcom-ufs: Add resets property Evan Green
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, Rob Herring,
	devicetree, linux-arm-msm, linux-kernel, Andy Gross, David Brown,
	Mark Rutland, Rob Herring

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>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 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 5d181fc3cc18..4a78ba8b85bc 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -59,7 +59,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".
@@ -74,7 +75,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.20.1


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

* [PATCH v5 3/8] dt-bindings: phy: qcom-ufs: Add resets property
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
  2019-03-21 17:17 ` [PATCH v5 1/8] dt-bindings: ufs: Add #reset-cells for Qualcomm controllers Evan Green
  2019-03-21 17:17 ` [PATCH v5 2/8] dt-bindings: phy-qcom-qmp: Add UFS PHY reset Evan Green
@ 2019-03-21 17:17 ` Evan Green
  2019-03-21 17:17 ` [PATCH v5 4/8] arm64: dts: sdm845: Add UFS PHY reset Evan Green
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, Rob Herring,
	devicetree, linux-kernel, Rob Herring, Mark Rutland

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>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Added resets to example (Stephen).

 Documentation/devicetree/bindings/ufs/ufs-qcom.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/ufs/ufs-qcom.txt b/Documentation/devicetree/bindings/ufs/ufs-qcom.txt
index 21d9a93db2e9..fd59f93e9556 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:
 
@@ -51,9 +52,11 @@ Example:
 			<&clock_gcc clk_ufs_phy_ldo>,
 			<&clock_gcc clk_gcc_ufs_tx_cfg_clk>,
 			<&clock_gcc clk_gcc_ufs_rx_cfg_clk>;
+		resets = <&ufshc 0>;
 	};
 
-	ufshc@fc598000 {
+	ufshc: ufshc@fc598000 {
+		#reset-cells = <1>;
 		...
 		phys = <&ufsphy1>;
 		phy-names = "ufsphy";
-- 
2.20.1


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

* [PATCH v5 4/8] arm64: dts: sdm845: Add UFS PHY reset
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
                   ` (2 preceding siblings ...)
  2019-03-21 17:17 ` [PATCH v5 3/8] dt-bindings: phy: qcom-ufs: Add resets property Evan Green
@ 2019-03-21 17:17 ` Evan Green
  2019-03-21 17:17 ` [PATCH v5 5/8] arm64: dts: msm8996: Add UFS PHY reset controller Evan Green
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, devicetree,
	linux-arm-msm, linux-kernel, Andy Gross, Rob Herring,
	David Brown, Mark Rutland

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

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 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 5308f1671824..66c77146585b 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1033,6 +1033,7 @@
 			phy-names = "ufsphy";
 			lanes-per-direction = <2>;
 			power-domains = <&gcc UFS_PHY_GDSC>;
+			#reset-cells = <1>;
 
 			iommus = <&apps_smmu 0x100 0xf>;
 
@@ -1078,6 +1079,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.20.1


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

* [PATCH v5 5/8] arm64: dts: msm8996: Add UFS PHY reset controller
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
                   ` (3 preceding siblings ...)
  2019-03-21 17:17 ` [PATCH v5 4/8] arm64: dts: sdm845: Add UFS PHY reset Evan Green
@ 2019-03-21 17:17 ` Evan Green
  2019-03-21 17:17 ` [PATCH v5 6/8] scsi: ufs: qcom: Expose the reset controller for PHY Evan Green
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, devicetree,
	linux-arm-msm, linux-kernel, Andy Gross, Rob Herring,
	David Brown, Mark Rutland

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

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 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 c761269caf80..5683b727283e 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -697,10 +697,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>;
@@ -756,6 +757,7 @@
 				<0 0>;
 
 			lanes-per-direction = <1>;
+			#reset-cells = <1>;
 			status = "disabled";
 
 			ufs_variant {
-- 
2.20.1


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

* [PATCH v5 6/8] scsi: ufs: qcom: Expose the reset controller for PHY
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
                   ` (4 preceding siblings ...)
  2019-03-21 17:17 ` [PATCH v5 5/8] arm64: dts: msm8996: Add UFS PHY reset controller Evan Green
@ 2019-03-21 17:17 ` Evan Green
  2019-03-21 17:17 ` [PATCH v5 7/8] phy: qcom: Utilize UFS reset controller Evan Green
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, Pedro Sousa, liwei,
	linux-scsi, linux-arm-msm, Bart Van Assche, linux-kernel,
	Alim Akhtar, Andy Gross, Martin K. Petersen, Avri Altman,
	James E.J. Bottomley, Janek Kotas, David Brown

Expose a reset controller that the phy will later use to control its
own PHY reset in the UFS controller. This will enable the combining
of PHY init functionality into a single function.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>

---
Note: The remaining changes in this series need this change, since
the PHYs now depend on getting the reset controller.

Changes in v5: None
Changes in v4: None
Changes in v3:
- Refactor to only expose the reset controller in one change (Stephen).
- Add period to comment (Stephen).
- Reset err to 0 in ignored error case (Stephen).
- Add include of reset-controller.h (Stephen)

Changes in v2:
- Remove include of reset.h (Stephen)
- Fix error print of phy_power_on (Stephen)
- Comment for reset controller warnings on id != 0 (Stephen)
- Add static to ufs_qcom_reset_ops (Stephen).

 drivers/scsi/ufs/Kconfig    |  1 +
 drivers/scsi/ufs/ufs-qcom.c | 52 +++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufs-qcom.h |  4 +++
 3 files changed, 57 insertions(+)

diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
index 6db37cf306b0..179bda374544 100644
--- a/drivers/scsi/ufs/Kconfig
+++ b/drivers/scsi/ufs/Kconfig
@@ -99,6 +99,7 @@ config SCSI_UFS_DWC_TC_PLATFORM
 config SCSI_UFS_QCOM
 	tristate "QCOM specific hooks to UFS controller platform driver"
 	depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
+	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 3aeadb14aae1..ab05ef5cfdcd 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-controller.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)
 {
@@ -1147,6 +1153,41 @@ 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);
+
+	/* Currently this code only knows about a single reset. */
+	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);
+
+	/* Currently this code only knows about a single reset. */
+	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;
+}
+
+static 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 +1232,17 @@ 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");
+		err = 0;
+	}
+
 	/*
 	 * 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 c114826316eb..68a880185752 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.20.1


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

* [PATCH v5 7/8] phy: qcom: Utilize UFS reset controller
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
                   ` (5 preceding siblings ...)
  2019-03-21 17:17 ` [PATCH v5 6/8] scsi: ufs: qcom: Expose the reset controller for PHY Evan Green
@ 2019-03-21 17:17 ` Evan Green
  2019-03-21 17:18 ` [PATCH v5 8/8] phy: ufs-qcom: Refactor all init steps into phy_poweron Evan Green
  2019-03-26  7:48 ` [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Kishon Vijay Abraham I
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, James E.J. Bottomley,
	linux-scsi, linux-arm-msm, linux-kernel, Alim Akhtar,
	Avri Altman, Andy Gross, David Brown, Pedro Sousa,
	Martin K. Petersen

Move the PHY reset from ufs-qcom into the respective PHYs. This will
allow us to merge the two phases of UFS PHY initialization.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>

---

Changes in v5: None
Changes in v4:
- Do reset_control_* unconditionally since null is handled (Stephen).

Changes in v3:
- Refactored to move reset control in a single commit (Stephen)
- Use no_pcs_sw_reset as an indicator of UFS reset in qmp-phy (Stephen).
- Assign ret = PTR_ERR() earlier, for better reuse (Stephen).

Changes in v2:
- Use devm_* to get the reset (Stephen)
- Clear ufs_reset on error getting it
- Remove needless error print (Stephen)

 drivers/phy/qualcomm/phy-qcom-qmp.c          | 34 ++++++++++++++++++++
 drivers/phy/qualcomm/phy-qcom-ufs-i.h        |  3 ++
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c |  8 +++++
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c |  8 +++++
 drivers/phy/qualcomm/phy-qcom-ufs.c          | 23 +++++++++++++
 drivers/scsi/ufs/ufs-qcom.c                  | 18 -----------
 6 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 08d6f6f7f039..a808887ab4e2 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -897,6 +897,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;
@@ -914,6 +915,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)
@@ -1314,6 +1317,7 @@ static int qcom_qmp_phy_com_exit(struct qcom_qmp *qmp)
 		return 0;
 	}
 
+	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);
@@ -1351,6 +1355,33 @@ static int qcom_qmp_phy_init(struct phy *phy)
 
 	dev_vdbg(qmp->dev, "Initializing QMP phy\n");
 
+	if (cfg->no_pcs_sw_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 =
+				devm_reset_control_get_exclusive(qmp->dev,
+								 "ufsphy");
+
+			if (IS_ERR(qmp->ufs_reset)) {
+				ret = PTR_ERR(qmp->ufs_reset);
+				dev_err(qmp->dev,
+					"failed to get UFS reset: %d\n",
+					ret);
+
+				qmp->ufs_reset = NULL;
+				return ret;
+			}
+		}
+
+		ret = reset_control_assert(qmp->ufs_reset);
+		if (ret)
+			goto err_lane_rst;
+	}
+
 	ret = qcom_qmp_phy_com_init(qphy);
 	if (ret)
 		return ret;
@@ -1383,6 +1414,9 @@ static int qcom_qmp_phy_init(struct phy *phy)
 				       cfg->rx_tbl, cfg->rx_tbl_num);
 
 	qcom_qmp_phy_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
+	ret = reset_control_deassert(qmp->ufs_reset);
+	if (ret)
+		goto err_lane_rst;
 
 	/*
 	 * UFS PHY requires the deassert of software reset before serdes start.
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-i.h b/drivers/phy/qualcomm/phy-qcom-ufs-i.h
index f798fb64de94..ba77348d807c 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>
@@ -101,6 +102,7 @@ struct ufs_qcom_phy {
 	struct ufs_qcom_phy_specific_ops *phy_spec_ops;
 
 	enum phy_mode mode;
+	struct reset_control *ufs_reset;
 };
 
 /**
@@ -132,6 +134,7 @@ struct phy *ufs_qcom_phy_generic_probe(struct platform_device *pdev,
 			struct ufs_qcom_phy *common_cfg,
 			const struct phy_ops *ufs_qcom_phy_gen_ops,
 			struct ufs_qcom_phy_specific_ops *phy_spec_ops);
+int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common);
 int ufs_qcom_phy_calibrate(struct ufs_qcom_phy *ufs_qcom_phy,
 			struct ufs_qcom_phy_calibration *tbl_A, int tbl_size_A,
 			struct ufs_qcom_phy_calibration *tbl_B, int tbl_size_B,
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
index 1e0d4f2046a4..cc343517a2ca 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
@@ -48,6 +48,14 @@ static int ufs_qcom_phy_qmp_14nm_init(struct phy *generic_phy)
 	bool is_rate_B = false;
 	int ret;
 
+	ret = ufs_qcom_phy_get_reset(phy_common);
+	if (ret)
+		return ret;
+
+	ret = reset_control_assert(phy_common->ufs_reset);
+	if (ret)
+		return ret;
+
 	if (phy_common->mode == PHY_MODE_UFS_HS_B)
 		is_rate_B = true;
 
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
index aef40f7a41d4..54b2af9d8702 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
@@ -67,6 +67,14 @@ static int ufs_qcom_phy_qmp_20nm_init(struct phy *generic_phy)
 	bool is_rate_B = false;
 	int ret;
 
+	ret = ufs_qcom_phy_get_reset(phy_common);
+	if (ret)
+		return ret;
+
+	ret = reset_control_assert(phy_common->ufs_reset);
+	if (ret)
+		return ret;
+
 	if (phy_common->mode == PHY_MODE_UFS_HS_B)
 		is_rate_B = true;
 
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs.c b/drivers/phy/qualcomm/phy-qcom-ufs.c
index f2979ccad00a..fe59785a55f2 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs.c
@@ -147,6 +147,22 @@ struct phy *ufs_qcom_phy_generic_probe(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(ufs_qcom_phy_generic_probe);
 
+int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common)
+{
+	struct reset_control *reset;
+
+	if (phy_common->ufs_reset)
+		return 0;
+
+	reset = devm_reset_control_get_exclusive_by_index(phy_common->dev, 0);
+	if (IS_ERR(reset))
+		return PTR_ERR(reset);
+
+	phy_common->ufs_reset = reset;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ufs_qcom_phy_get_reset);
+
 static int __ufs_qcom_phy_clk_get(struct device *dev,
 			 const char *name, struct clk **clk_out, bool err_print)
 {
@@ -533,6 +549,12 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
 	if (phy_common->is_powered_on)
 		return 0;
 
+	err = reset_control_deassert(phy_common->ufs_reset);
+	if (err) {
+		dev_err(dev, "Failed to assert UFS PHY reset");
+		return err;
+	}
+
 	if (!phy_common->is_started) {
 		err = ufs_qcom_phy_start_serdes(phy_common);
 		if (err)
@@ -620,6 +642,7 @@ 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);
+	reset_control_assert(phy_common->ufs_reset);
 	phy_common->is_powered_on = false;
 
 	return 0;
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index ab05ef5cfdcd..1c25b1c82314 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -261,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) {
@@ -274,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) {
@@ -296,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;
@@ -559,9 +544,6 @@ 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;
 	}
 
-- 
2.20.1


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

* [PATCH v5 8/8] phy: ufs-qcom: Refactor all init steps into phy_poweron
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
                   ` (6 preceding siblings ...)
  2019-03-21 17:17 ` [PATCH v5 7/8] phy: qcom: Utilize UFS reset controller Evan Green
@ 2019-03-21 17:18 ` Evan Green
  2019-03-26  7:48 ` [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Kishon Vijay Abraham I
  8 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-21 17:18 UTC (permalink / raw)
  To: Kishon Vijay Abraham I
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, Evan Green, James E.J. Bottomley,
	linux-scsi, linux-arm-msm, linux-kernel, Alim Akhtar,
	Avri Altman, Andy Gross, David Brown, Pedro Sousa,
	Martin K. Petersen

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

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.

QMP is a little tricky because the PHY is also shared with PCIe and
USB3, which have their own definitions for init() and poweron(). Rename
the meaty functions to _enable() and _disable() to disentangle from the
PHY core names, and then create two different ops structures: one for
UFS and one for the other PHY types.

In phy-qcom-ufs, remove the 'is_powered_on' and 'is_started' guards,
as the generic PHY code does the reference counting. The
14/20nm-specific init functions get collapsed into the generic power_on()
function, with the addition of a calibrate() callback specific to 14/20nm.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v5:
- Updated tags

Changes in v4:
- Keep doing reset_control* unconditionally through refactor (Stephen).

Changes in v3:
- Refactor init => poweron for all PHYs and UFS in one step (Stephen)

Changes in v2:
- Use devm_ to get the reset (Stephen)
- Removed whitespace changes (Stephen)

 drivers/phy/qualcomm/phy-qcom-qmp.c          | 78 ++++++--------------
 drivers/phy/qualcomm/phy-qcom-ufs-i.h        |  4 +-
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c | 33 +--------
 drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c | 33 +--------
 drivers/phy/qualcomm/phy-qcom-ufs.c          | 42 ++++++-----
 drivers/scsi/ufs/ufs-qcom.c                  | 44 +++++------
 6 files changed, 64 insertions(+), 170 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index a808887ab4e2..3b01ebf76f66 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1339,8 +1339,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;
@@ -1418,14 +1417,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	if (ret)
 		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
-	 * 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.
@@ -1437,7 +1428,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);
 
@@ -1454,11 +1447,10 @@ 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:
+	reset_control_assert(qmp->ufs_reset);
 	clk_disable_unprepare(qphy->pipe_clk);
 err_clk_enable:
 	if (cfg->has_lane_rst)
@@ -1469,7 +1461,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;
@@ -1497,44 +1489,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)
 {
@@ -1784,9 +1738,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,
 };
@@ -1797,6 +1757,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;
 
@@ -1883,7 +1844,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);
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-i.h b/drivers/phy/qualcomm/phy-qcom-ufs-i.h
index ba77348d807c..109ddd67be82 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-i.h
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-i.h
@@ -97,8 +97,6 @@ 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;
@@ -117,6 +115,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);
@@ -134,7 +133,6 @@ struct phy *ufs_qcom_phy_generic_probe(struct platform_device *pdev,
 			struct ufs_qcom_phy *common_cfg,
 			const struct phy_ops *ufs_qcom_phy_gen_ops,
 			struct ufs_qcom_phy_specific_ops *phy_spec_ops);
-int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common);
 int ufs_qcom_phy_calibrate(struct ufs_qcom_phy *ufs_qcom_phy,
 			struct ufs_qcom_phy_calibration *tbl_A, int tbl_size_A,
 			struct ufs_qcom_phy_calibration *tbl_B, int tbl_size_B,
diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
index cc343517a2ca..4815546f228c 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
@@ -42,36 +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;
-
-	ret = ufs_qcom_phy_get_reset(phy_common);
-	if (ret)
-		return ret;
-
-	ret = reset_control_assert(phy_common->ufs_reset);
-	if (ret)
-		return 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)
@@ -132,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,
@@ -141,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 54b2af9d8702..f1cf819e12ea 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
@@ -61,36 +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;
-
-	ret = ufs_qcom_phy_get_reset(phy_common);
-	if (ret)
-		return ret;
-
-	ret = reset_control_assert(phy_common->ufs_reset);
-	if (ret)
-		return 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)
@@ -190,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,
@@ -199,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 fe59785a55f2..0a9f50f086b6 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs.c
@@ -147,7 +147,7 @@ struct phy *ufs_qcom_phy_generic_probe(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(ufs_qcom_phy_generic_probe);
 
-int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common)
+static int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common)
 {
 	struct reset_control *reset;
 
@@ -161,7 +161,6 @@ int ufs_qcom_phy_get_reset(struct ufs_qcom_phy *phy_common)
 	phy_common->ufs_reset = reset;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(ufs_qcom_phy_get_reset);
 
 static int __ufs_qcom_phy_clk_get(struct device *dev,
 			 const char *name, struct clk **clk_out, bool err_print)
@@ -544,10 +543,23 @@ 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;
+
+	err = reset_control_assert(phy_common->ufs_reset);
+	if (err)
+		return err;
+
+	if (phy_common->mode == PHY_MODE_UFS_HS_B)
+		is_rate_B = true;
+
+	err = phy_common->phy_spec_ops->calibrate(phy_common, is_rate_B);
+	if (err)
+		return err;
 
 	err = reset_control_deassert(phy_common->ufs_reset);
 	if (err) {
@@ -555,17 +567,13 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
 		return err;
 	}
 
-	if (!phy_common->is_started) {
-		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_start_serdes(phy_common);
+	if (err)
+		return err;
 
-		phy_common->is_started = true;
-	}
+	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) {
@@ -609,7 +617,6 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
 		}
 	}
 
-	phy_common->is_powered_on = true;
 	goto out;
 
 out_disable_ref_clk:
@@ -629,9 +636,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)
@@ -643,8 +647,6 @@ 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);
 	reset_control_assert(phy_common->ufs_reset);
-	phy_common->is_powered_on = false;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ufs_qcom_phy_power_off);
diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 1c25b1c82314..de9d3f56b58c 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -544,19 +544,11 @@ 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);
-		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;
 }
 
@@ -566,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 PHY power on: %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 {
@@ -1106,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);
@@ -1119,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;
-- 
2.20.1


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

* Re: [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend
  2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
                   ` (7 preceding siblings ...)
  2019-03-21 17:18 ` [PATCH v5 8/8] phy: ufs-qcom: Refactor all init steps into phy_poweron Evan Green
@ 2019-03-26  7:48 ` Kishon Vijay Abraham I
  2019-03-26 16:17   ` Evan Green
  8 siblings, 1 reply; 11+ messages in thread
From: Kishon Vijay Abraham I @ 2019-03-26  7:48 UTC (permalink / raw)
  To: Evan Green
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das, devicetree, Pedro Sousa, liwei,
	linux-scsi, linux-arm-msm, Bart Van Assche, linux-kernel,
	Andy Gross, Subhash Jadavani, Martin K. Petersen, Alim Akhtar,
	Rob Herring, Avri Altman, Mark Rutland, James E.J. Bottomley,
	Janek Kotas, David Brown

Hi,

On 21/03/19 10:47 PM, Evan Green wrote:
> 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.
> 
> 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.

Merged the series except the dts patches.

Thanks
Kishon
> 
> Changes in v5:
> - Updated tags
> 
> Changes in v4:
> - Do reset_control_* unconditionally since null is handled (Stephen).
> - Keep doing reset_control* unconditionally through refactor (Stephen).
> 
> Changes in v3:
> - Refactor to only expose the reset controller in one change (Stephen).
> - Add period to comment (Stephen).
> - Reset err to 0 in ignored error case (Stephen).
> - Add include of reset-controller.h (Stephen)
> - Refactored to move reset control in a single commit (Stephen)
> - Use no_pcs_sw_reset as an indicator of UFS reset in qmp-phy (Stephen).
> - Assign ret = PTR_ERR() earlier, for better reuse (Stephen).
> - Refactor init => poweron for all PHYs and UFS in one step (Stephen)
> 
> Changes in v2:
> - Added resets to example (Stephen).
> - Remove include of reset.h (Stephen)
> - Fix error print of phy_power_on (Stephen)
> - Comment for reset controller warnings on id != 0 (Stephen)
> - Add static to ufs_qcom_reset_ops (Stephen).
> - Use devm_* to get the reset (Stephen)
> - Clear ufs_reset on error getting it
> - Remove needless error print (Stephen)
> - Use devm_ to get the reset (Stephen)
> - Removed whitespace changes (Stephen)
> 
> Evan Green (8):
>   dt-bindings: ufs: Add #reset-cells for Qualcomm controllers
>   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: Utilize UFS reset controller
>   phy: ufs-qcom: Refactor all init steps into phy_poweron
> 
>  .../devicetree/bindings/phy/qcom-qmp-phy.txt  |   6 +-
>  .../devicetree/bindings/ufs/ufs-qcom.txt      |   5 +-
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt |   3 +
>  arch/arm64/boot/dts/qcom/msm8996.dtsi         |   4 +-
>  arch/arm64/boot/dts/qcom/sdm845.dtsi          |   3 +
>  drivers/phy/qualcomm/phy-qcom-qmp.c           | 112 +++++++++--------
>  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                   | 114 +++++++++++-------
>  drivers/scsi/ufs/ufs-qcom.h                   |   4 +
>  13 files changed, 193 insertions(+), 171 deletions(-)
> 

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

* Re: [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend
  2019-03-26  7:48 ` [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Kishon Vijay Abraham I
@ 2019-03-26 16:17   ` Evan Green
  0 siblings, 0 replies; 11+ messages in thread
From: Evan Green @ 2019-03-26 16:17 UTC (permalink / raw)
  To: Kishon Vijay Abraham I, Andy Gross, Bjorn Andersson
  Cc: Stephen Boyd, Marc Gonzalez, Can Guo, Vivek Gautam,
	Douglas Anderson, Asutosh Das,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Pedro Sousa, liwei, SCSI, linux-arm-msm, Bart Van Assche, LKML,
	Subhash Jadavani, Martin K. Petersen, Alim Akhtar, Rob Herring,
	Avri Altman, Mark Rutland, James E.J. Bottomley, Janek Kotas,
	David Brown

On Tue, Mar 26, 2019 at 12:49 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Hi,
>
> On 21/03/19 10:47 PM, Evan Green wrote:
> > 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.
> >
> > 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.
>
> Merged the series except the dts patches.

Yay, thanks Kishon.

Andy, Bjorn, are you able to take the DT patches?
-Evan

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

end of thread, other threads:[~2019-03-26 16:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-21 17:17 [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Evan Green
2019-03-21 17:17 ` [PATCH v5 1/8] dt-bindings: ufs: Add #reset-cells for Qualcomm controllers Evan Green
2019-03-21 17:17 ` [PATCH v5 2/8] dt-bindings: phy-qcom-qmp: Add UFS PHY reset Evan Green
2019-03-21 17:17 ` [PATCH v5 3/8] dt-bindings: phy: qcom-ufs: Add resets property Evan Green
2019-03-21 17:17 ` [PATCH v5 4/8] arm64: dts: sdm845: Add UFS PHY reset Evan Green
2019-03-21 17:17 ` [PATCH v5 5/8] arm64: dts: msm8996: Add UFS PHY reset controller Evan Green
2019-03-21 17:17 ` [PATCH v5 6/8] scsi: ufs: qcom: Expose the reset controller for PHY Evan Green
2019-03-21 17:17 ` [PATCH v5 7/8] phy: qcom: Utilize UFS reset controller Evan Green
2019-03-21 17:18 ` [PATCH v5 8/8] phy: ufs-qcom: Refactor all init steps into phy_poweron Evan Green
2019-03-26  7:48 ` [PATCH v5 0/8] phy: qcom-ufs: Enable regulators to be off in suspend Kishon Vijay Abraham I
2019-03-26 16:17   ` Evan Green

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