Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset
From: Dmitry Baryshkov
Date: Tue Apr 15 2025 - 05:29:45 EST
On 14/04/2025 23:34, Nitin Rawat wrote:
On 4/11/2025 4:38 PM, Dmitry Baryshkov wrote:
On Fri, 11 Apr 2025 at 13:50, Nitin Rawat <quic_nitirawa@xxxxxxxxxxx>
wrote:
On 4/11/2025 1:38 AM, Dmitry Baryshkov wrote:
On Thu, Apr 10, 2025 at 02:30:57PM +0530, Nitin Rawat wrote:
Refactor the UFS PHY reset handling to parse the reset logic only once
during probe, instead of every resume.
Move the UFS PHY reset parsing logic from qmp_phy_power_on to
qmp_ufs_probe to avoid unnecessary parsing during resume.
How did you solve the circular dependency issue being noted below?
Hi Dmitry,
As part of my patch, I moved the parsing logic from qmp_phy_power_on to
qmp_ufs_probe to avoid unnecessary parsing during resume. I'm uncertain
about the circular dependency issue and whether if it still exists.
It surely does. The reset controller is registered in the beginning of
ufs_qcom_init() and the PHY is acquired only a few lines below. It
creates a very small window for PHY driver to probe.
Which means, NAK, this patch doesn't look acceptable.
Hi Dmitry,
Thanks for pointing this out. I agree that it leaves very little time
for the PHY to probe, which may cause issues with targets where
no_pcs_sw_reset is set to true.
As an experiment, I kept no_pcs_sw_reset set to true for the SM8750, and
it caused bootup probe issues in some of the iterations I ran.
To address this, I propose updating the patch to move the
qmp_ufs_get_phy_reset call to phy_calibrate, just before the
reset_control_assert call.
Will it cause an issue if we move it to phy_init() instead of
phy_calibrate()?
This change will delay the UFS PHY reset as much as possible in the
code. Additionally, moving it from phy_power_on to calibrate will ensure
that qmp_ufs_get_phy_reset is called only once during boot, rather than
during each phy_power_on call.
Please let me know your thoughts.
=====================================================================================================
static int qmp_ufs_phy_calibrate(struct phy *phy)
{
struct qmp_ufs *qmp = phy_get_drvdata(phy);
@@ -1793,6 +1826,12 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
unsigned int val;
int ret;
+ pr_err("%s %d\n", __func__, __LINE__);
+
+ ret = qmp_ufs_get_phy_reset(qmp);
+ if (ret)
+ return ret;
+
ret = reset_control_assert(qmp->ufs_reset);
if (ret)
return ret;
@@ -1817,7 +1856,7 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
dev_err(qmp->dev, "phy initialization timed-out\n");
return ret;
=====================================================================================================
Regards.
Nitin
Regards,
Nitin
Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@xxxxxxxxxxx>
Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@xxxxxxxxxxx>
Signed-off-by: Nitin Rawat <quic_nitirawa@xxxxxxxxxxx>
---
drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 61 ++++++++++++
+------------
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/
qualcomm/phy-qcom-qmp-ufs.c
index 636dc3dc3ea8..12dad28cc1bd 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1799,38 +1799,11 @@ static int qmp_ufs_com_exit(struct qmp_ufs
*qmp)
static int qmp_ufs_power_on(struct phy *phy)
{
struct qmp_ufs *qmp = phy_get_drvdata(phy);
- const struct qmp_phy_cfg *cfg = qmp->cfg;
int ret;
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 = qmp_ufs_com_init(qmp);
- if (ret)
- return ret;
-
- return 0;
+ return ret;
}
static int qmp_ufs_phy_calibrate(struct phy *phy)
@@ -2088,6 +2061,34 @@ static int qmp_ufs_parse_dt(struct qmp_ufs
*qmp)
return 0;
}
+static int qmp_ufs_get_phy_reset(struct qmp_ufs *qmp)
+{
+ const struct qmp_phy_cfg *cfg = qmp->cfg;
+ int ret;
+
+ if (!cfg->no_pcs_sw_reset)
+ return 0;
+
+ /*
+ * 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");
Strange indentation.
+
+ if (IS_ERR(qmp->ufs_reset)) {
+ ret = PTR_ERR(qmp->ufs_reset);
+ dev_err(qmp->dev, "failed to get PHY reset:
%d\n", ret);
+ qmp->ufs_reset = NULL;
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
static int qmp_ufs_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -2114,6 +2115,10 @@ static int qmp_ufs_probe(struct
platform_device *pdev)
if (ret)
return ret;
+ ret = qmp_ufs_get_phy_reset(qmp);
+ if (ret)
+ return ret;
+
/* Check for legacy binding with child node. */
np = of_get_next_available_child(dev->of_node, NULL);
if (np) {
--
2.48.1
--
With best wishes
Dmitry