Message-ID: <CAPDyKFouCV3hCcJ9VuS0App34YyBd6vVNSJr6JZbYGGpffwaWA@mail.gmail.com>
Date: Fri, 1 Nov 2024 16:12:41 +0100
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 Mon, 21 Oct 2024 at 02:43, Shawn Lin <shawn.lin@...k-chips.com> wrote:
>
> 在 2024/10/18 18:03, Ulf Hansson 写道:
> > On Fri, 18 Oct 2024 at 11:20, Shawn Lin <shawn.lin@...k-chips.com> 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.
> >
> > 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.

Apologize for the delay! I needed to think a bit more carefully about
how to suggest moving this forward.

My conclusion is that we need to extend the PM domain infrastructure
(genpd in particular), to allow drivers to dynamically inform whether
it's okay to turn on/off the PM domain in runtime.

There is a similar thing already available, which is to use dev PM qos
along with the genpd governor, but that would not work in this case
because it may prevent runtime suspend for the device in question too.
I have therefore cooked a patch for genpd, see below. I think you can
fold it into your next version of the series. See also additional
suggestions below the patch.

From: Ulf Hansson <ulf.hansson@...aro.org>
Date: Fri, 1 Nov 2024 15:55:56 +0100
Subject: [PATCH] pmdomain: core: Introduce dev_pm_genpd_rpm_always_on()

For some usecases a consumer driver requires its device to remain power-on
from the PM domain perspective during runtime. Using dev PM qos along with
the genpd governors, doesn't work for this case as would potentially
prevent the device from being runtime suspended too.

To support these usecases, let's introduce dev_pm_genpd_rpm_always_on() to
allow consumers drivers to dynamically control the behaviour in genpd for a
device that is attached to it.

Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
---
 drivers/pmdomain/core.c   | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h |  7 +++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index a6c8b85dd024..e86e270b7eb9 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -697,6 +697,36 @@ bool dev_pm_genpd_get_hwmode(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(dev_pm_genpd_get_hwmode);

+/**
+ * dev_pm_genpd_rpm_always_on() - Control if the PM domain can be powered off.
+ *
+ * @dev: Device for which the PM domain may need to stay on for.
+ * @on: Value to set or unset for the condition.
+ *
+ * For some usecases a consumer driver requires its device to remain power-on
+ * from the PM domain perspective during runtime. This function allows the
+ * behaviour to be dynamically controlled for a device attached to a genpd.
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Return: Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
+{
+       struct generic_pm_domain *genpd;
+
+       genpd = dev_to_genpd_safe(dev);
+       if (!genpd)
+               return -ENODEV;
+
+       genpd_lock(genpd);
+       dev_gpd_data(dev)->rpm_always_on = on;
+       genpd_unlock(genpd);
+
+       return 0;
+}
+
 static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
        unsigned int state_idx = genpd->state_idx;
@@ -868,6 +898,10 @@ static int genpd_power_off(struct
generic_pm_domain *genpd, bool one_dev_on,
                if (!pm_runtime_suspended(pdd->dev) ||
                        irq_safe_dev_in_sleep_domain(pdd->dev, genpd))
                        not_suspended++;
+
+               /* The device may need its PM domain to stay powered on. */
+               if (to_gpd_data(pdd)->rpm_always_on)
+                       return -EBUSY;
        }

        if (not_suspended > 1 || (not_suspended == 1 && !one_dev_on))
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 45646bfcaf1a..d4c4a7cf34bd 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -260,6 +260,7 @@ struct generic_pm_domain_data {
        unsigned int rpm_pstate;
        unsigned int opp_token;
        bool hw_mode;
+       bool rpm_always_on;
        void *data;
 };

@@ -292,6 +293,7 @@ ktime_t dev_pm_genpd_get_next_hrtimer(struct device *dev);
 void dev_pm_genpd_synced_poweroff(struct device *dev);
 int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
 bool dev_pm_genpd_get_hwmode(struct device *dev);
+int dev_pm_genpd_rpm_always_on(struct device *dev, bool on);

 extern struct dev_power_governor simple_qos_governor;
 extern struct dev_power_governor pm_domain_always_on_gov;
@@ -375,6 +377,11 @@ static inline bool dev_pm_genpd_get_hwmode(struct
device *dev)
        return false;
 }

+static inline int dev_pm_genpd_rpm_always_on(struct device *dev, bool on)
+{
+       return -EOPNOTSUPP;
+}
+
 #define simple_qos_governor            (*(struct dev_power_governor *)(NULL))
 #define pm_domain_always_on_gov                (*(struct
dev_power_governor *)(NULL))
 #endif
-- 
2.43.0

Furthermore, I believe the way forward could be like this:

*) The arm smc call to inform the FW whether it's okay to turn off (or
not turn off) the PM domain for the UFS controller really belongs in
the genpd provider. More precisely, from the corresponding genpd's
->power_on() callback, we should send the smc call that prevents
power-off and in the ->power_off() callback we should send the smc
call that allows power-off again. No matter of what the spm|rpm_level
is set to. If you think caching of the state is important, I suggest
looking into that as an improvement on top.

*) In the UFS controller driver, we should call the new
dev_pm_genpd_rpm_always_on() to control whether the PM domain should
remain on during runtime or is allowed to be turned off.

*) In the system suspend callback, based on the spm|rpm_level (I guess
only one of those levels is really needed?), we may call
device_awake_path(). This can prevent genpd from turning off the PM
domain during system suspend in those cases when that is needed. See
genpd_finish_suspend() more details. Also note that, the genpd in
question also needs the GENPD_FLAG_ACTIVE_WAKEUP bit set for it, if
not already.

Kind regards
Uffe

Powered by blists - more mailing lists