Message-ID: <CAPDyKFqMuFMf0+2+mPZaGGtBRfavg0LTkhbrCeqh7kHeqq-yZQ@mail.gmail.com>
Date: Mon, 4 Nov 2024 10:51:45 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Shawn Lin <shawn.lin@...k-chips.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 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>, 
	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 Mon, 4 Nov 2024 at 07:38, Shawn Lin <shawn.lin@...k-chips.com> wrote:
>
> 在 2024/11/3 20:02, Manivannan Sadhasivam 写道:
> > On Fri, Oct 18, 2024 at 05:20:08PM +0800, Shawn Lin 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.
> >>
> >
> > We had a requirement to notify the genpd provider from consumer to not turn off
> > the power domain during system suspend. So Ulf came up with an API for
> > consumers, device_set_wakeup_path() setting the 'dev->power.wakeup_path' which
> > will be honored by the genpd core. Will that work for you?
>
> Yes, that works. And we may need a symmetrical call, for instance,
> device_clr_wakeup_path() to allow genpd provider to turn off the power
> domain as well.

The PM core clears the flag in device_prepare(). The flag is typically
supposed to be set from a ->suspend() callback, so there should be no
need for an additional function that clears the flag, I think.

[...]

Kind regards
Uffe

Powered by blists - more mailing lists