On Fri, 18 Oct 2024 at 11:20, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
>
> Hi Ulf,
>
> 在 2024/10/18 17:07, Ulf Hansson 写道:
> > On Thu, 10 Oct 2024 at 03:21, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> 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?
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?
[...]
Kind regards
Uffe
