Re: [PATCH V5 04/11] phy: qcom-qmp-ufs: Refactor UFS PHY reset

Nitin Rawat
2025-05-21T21:48:07Z

On 5/21/2025 6:56 PM, Manivannan Sadhasivam wrote:
> On Thu, May 15, 2025 at 09:57:15PM +0530, Nitin Rawat wrote:
>> Refactor the UFS PHY reset handling to parse the reset logic only once
>> during initialization, instead of every resume.
>>
>> As part of this change, move the UFS PHY reset parsing logic from
>> qmp_phy_power_on to the new qmp_ufs_phy_init function.
>>

Sure. I'll update the commit text in next patchset.

>
> More importantly, you are introducing the phy_ops::init callback, which
> should've been mentioned.
>
>> Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>> ---
>> drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 59 +++++++++++++------------
>> 1 file changed, 31 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> index ade8e9c4b9ae..33d238cf49aa 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
>> @@ -1800,38 +1800,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;
>
> This can't be:
> return qmp_ufs_com_init; ?

This is already taken care in next patch (#6) of this series.

Thanks,
Nitin

>
> - Mani
>