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