| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c2ec6b7c-421d-43c3-8c0a-de4f7bdd867c@oss.qualcomm.com>
Date: Tue, 15 Apr 2025 12:29:23 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Nitin Rawat <quic_nitirawa@...cinc.com>
Cc: vkoul@...nel.org, kishon@...nel.org, manivannan.sadhasivam@...aro.org,
James.Bottomley@...senpartnership.com, martin.petersen@...cle.com,
bvanassche@....org, bjorande@...cinc.com, neil.armstrong@...aro.org,
konrad.dybcio@....qualcomm.com, quic_rdwivedi@...cinc.com,
linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org
Subject: Re: [PATCH V3 4/9] phy: qcom-qmp-ufs: Refactor UFS PHY reset
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@...cinc.com>
>> 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@...cinc.com>
>>>>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
>>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@...cinc.com>
>>>>> ---
>>>>> 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
Powered by blists - more mailing lists