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, plus the UFS DT nodes, which are now
patch 3, 4, 5 of [1].
[1] https://lore.kernel.org/lkml/[email protected]/
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 | 117 +++++++++---------
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, 202 insertions(+), 167 deletions(-)
--
2.20.1
Enable Qualcomm UFS controllers to expose the PHY reset via a reset
controller.
Signed-off-by: Evan Green <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
Fixing up this aspect of it made me notice that this patch [1]
hasn't landed yet. It really ought to.
[1] https://lore.kernel.org/lkml/[email protected]/T/#u
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 8cf59452c675..e2460b666ae4 100644
--- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
+++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
@@ -47,6 +47,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.
@@ -76,4 +78,5 @@ Example:
reset-names = "rst";
phys = <&ufsphy1>;
phy-names = "ufsphy";
+ #reset-cells = <1>;
};
--
2.20.1
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 <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
I realize I'm not supposed to add a required property after the fact,
but given that the UFS DT nodes that would use this binding are not
yet upstream (and this would be the first), I was hoping to squeak by.
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 4ff26dbf4310..49b8a5eed3cd 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -56,7 +56,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".
@@ -70,7 +71,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
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 <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
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
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 <[email protected]>
---
Note: The remaining changes in this series need this change, since
the PHYs now depend on getting the reset controller.
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 2ddbb26d9c26..63c5c4115981 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 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
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 <[email protected]>
---
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 | 39 ++++++++++++++++++++
drivers/phy/qualcomm/phy-qcom-ufs-i.h | 3 ++
drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c | 10 +++++
drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c | 10 +++++
drivers/phy/qualcomm/phy-qcom-ufs.c | 27 ++++++++++++++
drivers/scsi/ufs/ufs-qcom.c | 18 ---------
6 files changed, 89 insertions(+), 18 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index daf751500232..2861c64b9bcc 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,9 @@ static int qcom_qmp_phy_com_exit(struct qcom_qmp *qmp)
return 0;
}
+ if (qmp->ufs_reset)
+ reset_control_assert(qmp->ufs_reset);
+
if (cfg->has_phy_com_ctrl) {
qphy_setbits(serdes, cfg->regs[QPHY_COM_START_CONTROL],
SERDES_START | PCS_START);
@@ -1351,6 +1357,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;
@@ -1384,6 +1417,12 @@ static int qcom_qmp_phy_init(struct phy *phy)
qcom_qmp_phy_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
+ if (qmp->ufs_reset) {
+ ret = reset_control_deassert(qmp->ufs_reset);
+ if (ret)
+ 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
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..ef1383123883 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
@@ -48,6 +48,16 @@ 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;
+
+ if (phy_common->ufs_reset) {
+ 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..b05f89d734f0 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
@@ -67,6 +67,16 @@ 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;
+
+ if (phy_common->ufs_reset) {
+ 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..9cc8aa0b7a4f 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,14 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
if (phy_common->is_powered_on)
return 0;
+ if (phy_common->ufs_reset) {
+ err = reset_control_deassert(phy_common->ufs_reset);
+ if (err) {
+ dev_err(dev, "Failed to assert UFS PHY reset");
+ return err;
+ }
+ }
+
if (!phy_common->is_started) {
err = ufs_qcom_phy_start_serdes(phy_common);
if (err)
@@ -620,6 +644,9 @@ 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);
+ if (phy_common->ufs_reset)
+ 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
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 <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
This commit is based atop the series at [1]. Patches 1 and 2 of that
series have landed, but 3, 4, and 5 are still outstanding.
[1] https://lore.kernel.org/lkml/[email protected]/
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 b29332b265d9..029ab66405cf 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -990,6 +990,7 @@
phy-names = "ufsphy";
lanes-per-direction = <2>;
power-domains = <&gcc UFS_PHY_GDSC>;
+ #reset-cells = <1>;
clock-names =
"core_clk",
@@ -1033,6 +1034,8 @@
clocks = <&gcc GCC_UFS_MEM_CLKREF_CLK>,
<&gcc GCC_UFS_PHY_PHY_AUX_CLK>;
+ resets = <&ufs_mem_hc 0>;
+ reset-names = "ufsphy";
status = "disabled";
ufs_mem_phy_lanes: lanes@1d87400 {
--
2.20.1
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 <[email protected]>
---
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 | 80 ++++++--------------
drivers/phy/qualcomm/phy-qcom-ufs-i.h | 4 +-
drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c | 35 +--------
drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c | 35 +--------
drivers/phy/qualcomm/phy-qcom-ufs.c | 44 ++++++-----
drivers/scsi/ufs/ufs-qcom.c | 44 +++++------
6 files changed, 68 insertions(+), 174 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 2861c64b9bcc..3597c4c0e50f 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -1341,8 +1341,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;
@@ -1423,14 +1422,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
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.
@@ -1442,7 +1433,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);
@@ -1459,11 +1452,12 @@ static int qcom_qmp_phy_init(struct phy *phy)
goto err_pcs_ready;
}
qmp->phy_initialized = true;
-
-out:
- return ret;
+ return 0;
err_pcs_ready:
+ if (qmp->ufs_reset)
+ reset_control_assert(qmp->ufs_reset);
+
clk_disable_unprepare(qphy->pipe_clk);
err_clk_enable:
if (cfg->has_lane_rst)
@@ -1474,7 +1468,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;
@@ -1502,44 +1496,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)
{
@@ -1789,9 +1745,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,
};
@@ -1802,6 +1764,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;
@@ -1888,7 +1851,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 ef1383123883..4815546f228c 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-14nm.c
@@ -42,38 +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;
-
- if (phy_common->ufs_reset) {
- 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)
@@ -134,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,
@@ -143,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 b05f89d734f0..f1cf819e12ea 100644
--- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
+++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
@@ -61,38 +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;
-
- if (phy_common->ufs_reset) {
- 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)
@@ -192,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,
@@ -201,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 9cc8aa0b7a4f..8860ced486f3 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,25 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
{
struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
struct device *dev = phy_common->dev;
+ bool is_rate_B = false;
int err;
- if (phy_common->is_powered_on)
- return 0;
+ err = ufs_qcom_phy_get_reset(phy_common);
+ if (err)
+ return err;
+
+ if (phy_common->ufs_reset) {
+ 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;
if (phy_common->ufs_reset) {
err = reset_control_deassert(phy_common->ufs_reset);
@@ -557,17 +571,13 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
}
}
- 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) {
@@ -611,7 +621,6 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
}
}
- phy_common->is_powered_on = true;
goto out;
out_disable_ref_clk:
@@ -631,9 +640,6 @@ int ufs_qcom_phy_power_off(struct phy *generic_phy)
{
struct ufs_qcom_phy *phy_common = get_ufs_qcom_phy(generic_phy);
- if (!phy_common->is_powered_on)
- return 0;
-
phy_common->phy_spec_ops->power_control(phy_common, false);
if (phy_common->vddp_ref_clk.reg)
@@ -647,8 +653,6 @@ int ufs_qcom_phy_power_off(struct phy *generic_phy)
if (phy_common->ufs_reset)
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
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 <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
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 99b7495455a6..179f1988d45c 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -663,10 +663,11 @@
clock-names = "ref_clk_src", "ref_clk";
clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
<&gcc GCC_UFS_CLKREF_CLK>;
+ resets = <&ufshc 0>;
status = "disabled";
};
- ufshc@624000 {
+ ufshc: ufshc@624000 {
compatible = "qcom,ufshc";
reg = <0x624000 0x2500>;
interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
@@ -722,6 +723,7 @@
<0 0>;
lanes-per-direction = <1>;
+ #reset-cells = <1>;
status = "disabled";
ufs_variant {
--
2.20.1
On 06/02/19 12:29 AM, Evan Green wrote:
> 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 <[email protected]>
I'd like to get ACK from scsi/ufs/ MAINTAINER Vinayak for me merge it in PHY tree.
Thanks
Kishon
>
> ---
> Note: The remaining changes in this series need this change, since
> the PHYs now depend on getting the reset controller.
>
> 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 2ddbb26d9c26..63c5c4115981 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 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
>
On 06/02/2019 12:42, Kishon Vijay Abraham I wrote:
> On 06/02/19 12:29 AM, Evan Green wrote:
>
>> 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 <[email protected]>
>
> I'd like to get ACK from scsi/ufs/ MAINTAINER Vinayak for me merge it in PHY tree.
Kishon, I haven't seen any messages from Vinayak in a while.
https://patchwork.kernel.org/patch/10795501/
New reviewers are Alim, Avri, and Pedro (haven't heard from Pedro yet).
Regards.
Hi Marc,
On 06/02/19 5:24 PM, Marc Gonzalez wrote:
> On 06/02/2019 12:42, Kishon Vijay Abraham I wrote:
>
>> On 06/02/19 12:29 AM, Evan Green wrote:
>>
>>> 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 <[email protected]>
>>
>> I'd like to get ACK from scsi/ufs/ MAINTAINER Vinayak for me merge it in PHY tree.
>
> Kishon, I haven't seen any messages from Vinayak in a while.
>
> https://patchwork.kernel.org/patch/10795501/
>
> New reviewers are Alim, Avri, and Pedro (haven't heard from Pedro yet).
Thanks for letting me know.
Alim, Avri,
Can you give your ACK for this patch?
Thanks
Kishon
>
> Regards.
>
Hi,
> On 06/02/19 12:29 AM, Evan Green wrote:
> > 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 <[email protected]>
>
> I'd like to get ACK from scsi/ufs/ MAINTAINER Vinayak for me merge it in PHY
> tree.
Looks like this series is qcom specific, and has less impact of the ufs core driver.
> > + err = devm_reset_controller_register(dev, &host->rcdev);
Just my 2 cents:
Isn't this should be done somewhere in drivers/clk/qcom,
Like its done for any other qcom board?
Quoting Avri Altman (2019-02-06 05:57:17)
> Hi,
>
> > On 06/02/19 12:29 AM, Evan Green wrote:
> > > 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 <[email protected]>
> >
> > I'd like to get ACK from scsi/ufs/ MAINTAINER Vinayak for me merge it in PHY
> > tree.
> Looks like this series is qcom specific, and has less impact of the ufs core driver.
>
> > > + err = devm_reset_controller_register(dev, &host->rcdev);
> Just my 2 cents:
> Isn't this should be done somewhere in drivers/clk/qcom,
> Like its done for any other qcom board?
It's not a clk controller, so I don't see how that makes sense. There
are clk/reset controllers on qcom platforms, and so we've combined them
into the same overall driver there because those resets affect clk
operations and vice-versa. But this looks like some sort of reset in the
phy or the controller itself and isn't the "clk type" resets that we
implement in drivers/clk/qcom/
Quoting Evan Green (2019-02-05 10:59:00)
> 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 <[email protected]>
>
> ---
Reviewed-by: Stephen Boyd <[email protected]>
Quoting Evan Green (2019-02-05 10:59:01)
> diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
> index aef40f7a41d4..b05f89d734f0 100644
> --- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
> +++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
> @@ -67,6 +67,16 @@ 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;
> +
> + if (phy_common->ufs_reset) {
> + ret = reset_control_assert(phy_common->ufs_reset);
It looks like you can always call this and set phy_common->ufs_reset to
NULL when it should be a no-op. But it would never be NULL because we
would always fail earlier in ufs_qcom_phy_get_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..9cc8aa0b7a4f 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;
I find this API weird that it doesn't return the pointer to the reset
controller. It would be more kernel idiomatic if it returned the pointer
and used error pointers.
Are there now two places where we're getting the reset controller? I'm
confused now.
> +}
> +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,14 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
> if (phy_common->is_powered_on)
> return 0;
>
> + if (phy_common->ufs_reset) {
> + err = reset_control_deassert(phy_common->ufs_reset);
> + if (err) {
> + dev_err(dev, "Failed to assert UFS PHY reset");
> + return err;
> + }
> + }
> +
> if (!phy_common->is_started) {
> err = ufs_qcom_phy_start_serdes(phy_common);
> if (err)
> @@ -620,6 +644,9 @@ 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);
> + if (phy_common->ufs_reset)
> + reset_control_assert(phy_common->ufs_reset);
All these ifs can go away basically.
> +
> phy_common->is_powered_on = false;
>
> return 0;
Quoting Evan Green (2019-02-05 10:59:02)
> 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 <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
although it may change if the patch before this is changed
significantly.
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 2861c64b9bcc..3597c4c0e50f 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -1341,8 +1341,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;
> @@ -1423,14 +1422,6 @@ static int qcom_qmp_phy_init(struct phy *phy)
> 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.
> @@ -1442,7 +1433,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);
>
This is non-obvious, but OK I can see now that it's fine that everything
is combined into one function. I didn't realize that phy_init() was
polling the ready bit in addition to power on doing that under certain
circumstances.
Quite honestly, I find the whole implementation to be obtuse; combining
all the phy code together into one function and then changing the
execution paths via booleans and the 'type' member. It makes following
and reviewing this code really painful. We should make phy ops for each
type of phy: USB, USB+DP, PCIE, UFS, etc. and then have those ops call
simpler building block functions that do the common things. Then
reviewers don't have to untangle the execution paths in their minds to
make sure that nothing goes wrong when a stray 'if (my_phy_flag)' is
thrown into the init function and have to wonder if that causes problems
for something else.
On Wed, Feb 6, 2019 at 1:33 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Evan Green (2019-02-05 10:59:01)
> > diff --git a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
> > index aef40f7a41d4..b05f89d734f0 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-ufs-qmp-20nm.c
> > @@ -67,6 +67,16 @@ 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;
> > +
> > + if (phy_common->ufs_reset) {
> > + ret = reset_control_assert(phy_common->ufs_reset);
>
> It looks like you can always call this and set phy_common->ufs_reset to
> NULL when it should be a no-op. But it would never be NULL because we
> would always fail earlier in ufs_qcom_phy_get_reset()?
True, I'll do this unconditionally.
>
> > + 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..9cc8aa0b7a4f 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;
>
> I find this API weird that it doesn't return the pointer to the reset
> controller. It would be more kernel idiomatic if it returned the pointer
> and used error pointers.
>
> Are there now two places where we're getting the reset controller? I'm
> confused now.
Yeah, this was never meant to be an API, just a little static helper
function. But in order to structure the changes so that this change is
strictly "move the reset" (and not "combine init and poweron"), I had
to export this function and use it in the 14nm and 20nm init
functions. Those drivers do init() entirely themselves, and don't call
the common driver. In the next change, those init functions smoosh
into the common power_on() here, and then this function goes from
exported back to static. ... so yeah, temporary scaffolding to make
the series clearer.
>
> > +}
> > +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,14 @@ int ufs_qcom_phy_power_on(struct phy *generic_phy)
> > if (phy_common->is_powered_on)
> > return 0;
> >
> > + if (phy_common->ufs_reset) {
> > + err = reset_control_deassert(phy_common->ufs_reset);
> > + if (err) {
> > + dev_err(dev, "Failed to assert UFS PHY reset");
> > + return err;
> > + }
> > + }
> > +
> > if (!phy_common->is_started) {
> > err = ufs_qcom_phy_start_serdes(phy_common);
> > if (err)
> > @@ -620,6 +644,9 @@ 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);
> > + if (phy_common->ufs_reset)
> > + reset_control_assert(phy_common->ufs_reset);
>
> All these ifs can go away basically.
Righto.
>
> > +
> > phy_common->is_powered_on = false;
> >
> > return 0;
On Tue, Feb 5, 2019 at 10:59 AM Evan Green <[email protected]> wrote:
>
...
> 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 | 117 +++++++++---------
> 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, 202 insertions(+), 167 deletions(-)
>
> --
Hi Kishon,
A gentle ping here now that rc1 is out. Are you able to take these? I
can also repost if needed.
-Evan
Hi Evan,
On 20/03/19 1:34 AM, Evan Green wrote:
> On Tue, Feb 5, 2019 at 10:59 AM Evan Green <[email protected]> wrote:
>>
> ...
>> 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 | 117 +++++++++---------
>> 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, 202 insertions(+), 167 deletions(-)
>>
>> --
>
> Hi Kishon,
> A gentle ping here now that rc1 is out. Are you able to take these? I
> can also repost if needed.
Can you repost with the updated tags please?
Thanks
Kishon
On Wed, Mar 20, 2019 at 2:07 AM Kishon Vijay Abraham I <[email protected]> wrote:
>
> Hi Evan,
>
> On 20/03/19 1:34 AM, Evan Green wrote:
> > On Tue, Feb 5, 2019 at 10:59 AM Evan Green <[email protected]> wrote:
> >>
> > ...
> >> 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 | 117 +++++++++---------
> >> 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, 202 insertions(+), 167 deletions(-)
> >>
> >> --
> >
> > Hi Kishon,
> > A gentle ping here now that rc1 is out. Are you able to take these? I
> > can also repost if needed.
>
> Can you repost with the updated tags please?
>
Sure, done!
https://lore.kernel.org/lkml/[email protected]/T/#t