Re: [PATCH v3 5/5] scsi: ufs: rockchip: initial support for UFS

[Shawn Lin <shawn.lin@rock-chips.com>]

Hi Ulf,

在 2024/10/18 17:07, Ulf Hansson 写道:
> On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@rock-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.

>>
>> Per your comment at patch 4, should I use GENPD_FLAG_ALWAYS_ON +
>> arm_smccc_smc here in system suspend?
>>
>>>> +
>>>> +       return ufshcd_system_suspend(dev);
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops ufs_rockchip_pm_ops = {
>>>> +       SET_SYSTEM_SLEEP_PM_OPS(ufs_rockchip_system_suspend, ufshcd_system_resume)
>>>> +       SET_RUNTIME_PM_OPS(ufs_rockchip_runtime_suspend, ufs_rockchip_runtime_resume, NULL)
>>>> +       .prepare         = ufshcd_suspend_prepare,
>>>> +       .complete        = ufshcd_resume_complete,
>>>> +};
>>>> +
>>>> +static struct platform_driver ufs_rockchip_pltform = {
>>>> +       .probe = ufs_rockchip_probe,
>>>> +       .remove = ufs_rockchip_remove,
>>>> +       .driver = {
>>>> +               .name = "ufshcd-rockchip",
>>>> +               .pm = &ufs_rockchip_pm_ops,
>>>> +               .of_match_table = ufs_rockchip_of_match,
>>>> +       },
>>>> +};
>>>> +module_platform_driver(ufs_rockchip_pltform);
>>>> +
>>>
>>> [...]
> 
> Kind regards
> Uffe
>