Re: [PATCH 1/4] cpufreq: qcom-nvmem: Enable virtual power domain devices

[Ulf Hansson]

On Tue, 12 Sept 2023 at 11:40, Stephan Gerhold
<stephan.gerhold@xxxxxxxxxxxxxxx> wrote:
>
> The genpd core ignores performance state votes from devices that are
> runtime suspended as of commit 5937c3ce2122 ("PM: domains: Drop/restore
> performance state votes for devices at runtime PM").

I think you are referring to the wrong commit above. Please have a
look at commit 3c5a272202c2 ("PM: domains: Improve runtime PM
performance state handling"), instead.

I also suggest rephrasing the above into saying that the performance
state vote for a device is cached rather than carried out, if
pm_runtime_suspended() returns true for it.

Another relevant information in the commit message would be to add
that during device-attach (genpd_dev_pm_attach_by_id()), calls
pm_runtime_enable() the device.

> However, at the
> moment nothing ever enables the virtual devices created in
> qcom-cpufreq-nvmem for the cpufreq power domain scaling, so they are
> permanently runtime-suspended.
>
> Fix this by enabling the devices after attaching them and use
> dev_pm_syscore_device() to ensure the power domain also stays on when
> going to suspend. Since it supplies the CPU we can never turn it off
> from Linux. There are other mechanisms to turn it off when needed,
> usually in the RPM firmware or the cpuidle path.
>
> Without this fix performance states votes are silently ignored, and the
> CPU/CPR voltage is never adjusted. This has been broken since 5.14 but
> for some reason no one noticed this on QCS404 so far.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 1cb8339ca225 ("cpufreq: qcom: Add support for qcs404 on nvmem driver")
> Signed-off-by: Stephan Gerhold <stephan.gerhold@xxxxxxxxxxxxxxx>
> ---
>  drivers/cpufreq/qcom-cpufreq-nvmem.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> index 84d7033e5efe..17d6ab14c909 100644
> --- a/drivers/cpufreq/qcom-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/qcom-cpufreq-nvmem.c
> @@ -25,6 +25,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/soc/qcom/smem.h>
>
> @@ -280,6 +281,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>         }
>
>         for_each_possible_cpu(cpu) {
> +               struct device **virt_devs = NULL;
>                 struct dev_pm_opp_config config = {
>                         .supported_hw = NULL,
>                 };
> @@ -300,7 +302,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>
>                 if (drv->data->genpd_names) {
>                         config.genpd_names = drv->data->genpd_names;
> -                       config.virt_devs = NULL;
> +                       config.virt_devs = &virt_devs;
>                 }
>
>                 if (config.supported_hw || config.genpd_names) {
> @@ -311,6 +313,23 @@ static int qcom_cpufreq_probe(struct platform_device *pdev)
>                                 goto free_opp;
>                         }
>                 }
> +
> +               if (virt_devs) {
> +                       const char * const *name = config.genpd_names;
> +                       int i;
> +
> +                       for (i = 0; *name; i++, name++) {
> +                               ret = pm_runtime_resume_and_get(virt_devs[i]);
> +                               if (ret) {
> +                                       dev_err(cpu_dev, "failed to resume %s: %d\n",
> +                                               *name, ret);
> +                                       goto free_opp;
> +                               }

Shouldn't we restore the usage count at ->remove() too?

> +
> +                               /* Keep CPU power domain always-on */
> +                               dev_pm_syscore_device(virt_devs[i], true);

Is this really correct? cpufreq is suspended/resumed by the PM core
during system wide suspend/resume. See dpm_suspend|resume(). Isn't
that sufficient?

Moreover, it looks like the cpr genpd provider supports genpd's
->power_on|off() callbacks. Is there something wrong with this, that I
am missing?


> +                       }
> +               }
>         }
>
>         cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
>

Kind regards
Uffe