在 2024/10/18 18:03, Ulf Hansson 写道:
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?
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
