Re: [PATCH V2 3/6] phy: qcom-qmp-ufs: Refactor UFS PHY reset
From: Nitin Rawat
Date: Thu Apr 10 2025 - 11:39:20 EST
On 3/19/2025 1:16 AM, Bjorn Andersson wrote:
On Tue, Mar 18, 2025 at 08:19:41PM +0530, Nitin Rawat wrote:
Refactor the UFS PHY reset handling to parse the reset logic only once
during probe, instead of every resume.
This looks very reasonable! But it would be preferred to see the commit
messages following the what format outlines in
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
with a clear problem description followed by a description of the
technical solution.
Move the UFS PHY reset parsing logic from qmp_phy_power_on to
qmp_ufs_probe to avoid unnecessary parsing during resume.
Please add ()-suffix to function names in your commit messages.
Also, this series moves things around a lot, can you confirm that UFS is
working inbetween each one of this patches, so that the branch is
bisectable when this is being picked up?
Hi Bjorn,
Thanks for the review. I've addressed the bisectability compliance in my
latest patch set (patchset #3) that I posted today. I just realized I
missed your other comments about adding the ()-suffix to function names
in commit messages. Sorry about that. I'll make sure to include this in
my next patch set.
Thanks,
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 | 104 ++++++++++++------------
1 file changed, 50 insertions(+), 54 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index 0089ee80f852..3a80c2c110d2 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1757,32 +1757,6 @@ static void qmp_ufs_init_registers(struct qmp_ufs *qmp, const struct qmp_phy_cfg
qmp_ufs_init_all(qmp, &cfg->tbls_hs_b);
}
-static int qmp_ufs_com_init(struct qmp_ufs *qmp)
-{
- const struct qmp_phy_cfg *cfg = qmp->cfg;
- void __iomem *pcs = qmp->pcs;
- int ret;
-
- ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
- if (ret) {
- dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
- return ret;
- }
-
- ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
- if (ret)
- goto err_disable_regulators;
-
- qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
-
- return 0;
-
-err_disable_regulators:
- regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
-
- return ret;
-}
-
static int qmp_ufs_com_exit(struct qmp_ufs *qmp)
{
const struct qmp_phy_cfg *cfg = qmp->cfg;
@@ -1800,41 +1774,27 @@ 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;
+ void __iomem *pcs = qmp->pcs;
This is only used once, perhaps not worth a local variable to save 5
characters on that line?
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 = reset_control_assert(qmp->ufs_reset);
- if (ret)
- return ret;
+ ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
+ if (ret) {
+ dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
regulator_bulk_enable() will already have printed a more useful error
message, letting you know which of the vregs[] it was that failed to
enable.
+ return ret;
}
- ret = qmp_ufs_com_init(qmp);
+ ret = clk_bulk_prepare_enable(qmp->num_clks, qmp->clks);
if (ret)
- return ret;
+ goto err_disable_regulators;
+
+ qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL], SW_PWRDN);
return 0;
+
+err_disable_regulators:
+ regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
+
+ return ret;
}
static int qmp_ufs_phy_calibrate(struct phy *phy)
@@ -1846,6 +1806,10 @@ static int qmp_ufs_phy_calibrate(struct phy *phy)
unsigned int val;
int ret;
+ ret = reset_control_assert(qmp->ufs_reset);
+ if (ret)
+ return ret;
+
qmp_ufs_init_registers(qmp, cfg);
ret = reset_control_deassert(qmp->ufs_reset);
@@ -2088,6 +2052,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.
This is invoked only once, from qcom_ufs_probe(), so it doesn't seem
accurate anymore. How come this is no longer needed? Please describe
what changed int he commit message.
+ */
+ if (!qmp->ufs_reset) {
+ qmp->ufs_reset =
+ devm_reset_control_get_exclusive(qmp->dev, "ufsphy");
The line break here is really weird, are you sure checkpatch --strict
didn't complain about this one?
+
+ if (IS_ERR(qmp->ufs_reset)) {
+ ret = PTR_ERR(qmp->ufs_reset);
+ dev_err(qmp->dev, "failed to get PHY reset: %d\n", ret);
return dev_err_probe(qmp->dev, PTR_ERR(qmp->ufs_reset), "failed to...: %pe\n", qmp->ufs_reset);
While being more succinct, it also stores the reason for failing the
probe so that you can find it in /sys/kernel/debug/devices_deferred
+ qmp->ufs_reset = NULL;
Use a local variable if you're worried about someone accessing the stale
error code after returning here.
Regards,
Bjorn
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
static int qmp_ufs_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -2114,6 +2106,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