Message-ID: <161ad092-4e7c-4ca1-ade7-d512a0b39799@rock-chips.com>
Date: Mon, 4 Nov 2024 14:21:45 +0800
From: Shawn Lin <shawn.lin@...k-chips.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: shawn.lin@...k-chips.com, 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

在 2024/11/1 23:12, Ulf Hansson 写道:
> 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.

Thanks, Ulf.  I'll fold it into my v4 series and fix the code in UFS 
driver and genpd provider according to your suggestions.

> 
> 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

Powered by blists - more mailing lists