Message-ID: <90ff835d-f3b2-4b7c-aa1a-575e231a57e6@rock-chips.com>
Date: Mon, 21 Oct 2024 08:43:26 +0800
From: Shawn Lin <shawn.lin@...k-chips.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: shawn.lin@...k-chips.com, Rob Herring <robh+dt@...nel.org>,
 "James E . J . Bottomley" <James.Bottomley@...senpartnership.com>,
 "Martin K . Petersen" <martin.petersen@...cle.com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Alim Akhtar <alim.akhtar@...sung.com>, Avri Altman <avri.altman@....com>,
 Bart Van Assche <bvanassche@....org>, YiFeng Zhao <zyf@...k-chips.com>,
 Liang Chen <cl@...k-chips.com>, linux-scsi@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

在 2024/10/18 18:03, Ulf Hansson 写道:
> On Fri, 18 Oct 2024 at 11:20, Shawn Lin <shawn.lin@...k-chips.com> wrote:
>>
>> Hi Ulf,
>>
>> 在 2024/10/18 17:07, Ulf Hansson 写道:
>>> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@...k-chips.com> wrote:
>>>>
>>>> Hi Ulf
>>>>
>>>> 在 2024/10/9 21:15, Ulf Hansson 写道:
>>>>> [...]
>>>>>
>>>>>> +
>>>>>> +static int ufs_rockchip_runtime_suspend(struct device *dev)
>>>>>> +{
>>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>>>>>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>>>>>
>>>>> pd_to_genpd() isn't safe to use like this. It's solely to be used by
>>>>> genpd provider drivers.
>>>>>
>>>>>> +
>>>>>> +       clk_disable_unprepare(host->ref_out_clk);
>>>>>> +
>>>>>> +       /*
>>>>>> +        * Shouldn't power down if rpm_lvl is less than level 5.
>>>>>
>>>>> Can you elaborate on why we must not power-off the power-domain when
>>>>> level is less than 5?
>>>>>
>>>>
>>>> Because ufshcd driver assume the controller is active and the link is on
>>>> if level is less than 5. So the default resume policy will not try to
>>>> recover the registers until the first error happened. Otherwise if the
>>>> level is >=5, it assumes the controller is off and the link is down,
>>>> then it will restore the registers and link.
>>>>
>>>> And the level is changeable via sysfs.
>>>
>>> Okay, thanks for clarifying.
>>>
>>>>
>>>>> What happens if we power-off anyway when the level is less than 5?
>>>>>
>>>>>> +        * This flag will be passed down to platform power-domain driver
>>>>>> +        * which has the final decision.
>>>>>> +        */
>>>>>> +       if (hba->rpm_lvl < UFS_PM_LVL_5)
>>>>>> +               genpd->flags |= GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>> +       else
>>>>>> +               genpd->flags &= ~GENPD_FLAG_RPM_ALWAYS_ON;
>>>>>
>>>>> The genpd->flags is not supposed to be changed like this - and
>>>>> especially not from a genpd consumer driver.
>>>>>
>>>>> I am trying to understand a bit more of the use case here. Let's see
>>>>> if that helps me to potentially suggest an alternative approach.
>>>>>
>>>>
>>>> I was not familiar with the genpd part, so I haven't come up with
>>>> another solution. It would be great if you can guide me to the right
>>>> way.
>>>
>>> I have been playing with the existing infrastructure we have at hand
>>> to support this, but I need a few more days to be able to propose
>>> something for you.
>>>
>>
>> Much appreciate.
>>
>>>>
>>>>>> +
>>>>>> +       return ufshcd_runtime_suspend(dev);
>>>>>> +}
>>>>>> +
>>>>>> +static int ufs_rockchip_runtime_resume(struct device *dev)
>>>>>> +{
>>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>>>>>> +       int err;
>>>>>> +
>>>>>> +       err = clk_prepare_enable(host->ref_out_clk);
>>>>>> +       if (err) {
>>>>>> +               dev_err(hba->dev, "failed to enable ref out clock %d\n", err);
>>>>>> +               return err;
>>>>>> +       }
>>>>>> +
>>>>>> +       reset_control_assert(host->rst);
>>>>>> +       usleep_range(1, 2);
>>>>>> +       reset_control_deassert(host->rst);
>>>>>> +
>>>>>> +       return ufshcd_runtime_resume(dev);
>>>>>> +}
>>>>>> +
>>>>>> +static int ufs_rockchip_system_suspend(struct device *dev)
>>>>>> +{
>>>>>> +       struct ufs_hba *hba = dev_get_drvdata(dev);
>>>>>> +       struct ufs_rockchip_host *host = ufshcd_get_variant(hba);
>>>>>> +
>>>>>> +       /* Pass down desired spm_lvl to Firmware */
>>>>>> +       arm_smccc_smc(ROCKCHIP_SIP_SUSPEND_MODE, ROCKCHIP_SLEEP_PD_CONFIG,
>>>>>> +                       host->pd_id, hba->spm_lvl < 5 ? 1 : 0, 0, 0, 0, 0, NULL);
>>>>>
>>>>> Can you please elaborate on what goes on here? Is this turning off the
>>>>> power-domain that the dev is attached to - or what is actually
>>>>> happening?
>>>>>
>>>>
>>>> This smc call is trying to ask firmware not to turn off the power-domian
>>>> that the UFS is attached to and also not to turn off the power of UFS
>>>> conntroller.
>>>
>>> Okay, thanks for clarifying!
>>>
>>> A follow up question, don't you need to make a corresponding smc call
>>> to inform the FW that it's okay to turn off the power-domain at some
>>> point?
>>>
>>
>> Yes. Each time entering sleep, we teach FW if it need to turn off or
>> keep power-domain, for instance "hba->spm_lvl < 5 ? 1 : 0" , 0 means
>> off and 1 means on.
> 
> I see. So you need to make the call each time when entering the system suspend?
> 
> Or would it be okay to just make it once, when the spm_lvl is changed?

Thers is no nofity when changing spm_lvl.

> 
> Another way to deal with it, would be to make the smc call each time
> the power-domain is turned-on, based on spm_lvl too of course.
> 
> Would that work?

Yes, that works. Another option is to cache power-domain states and
check spm_lvl locally. If it doesn't change, we skip smc call.

> 
> [...]
> 
> Kind regards
> Uffe
> 

Powered by blists - more mailing lists