[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAPDyKFo=GcHG2sGQBrXJ7VWyp59QOmbLCAvHQ3krUympEkid_A@mail.gmail.com>
Date: Fri, 18 Oct 2024 11:07:48 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Shawn Lin <shawn.lin@...k-chips.com>
Cc: 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
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.
>
> >> +
> >> + 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?
>
> 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
Powered by blists - more mailing lists