On Tue, 12 Sept 2023 at 11:40, Stephan Gerhold stephan.gerhold@kernkonzept.com 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@vger.kernel.org Fixes: 1cb8339ca225 ("cpufreq: qcom: Add support for qcs404 on nvmem driver") Signed-off-by: Stephan Gerhold stephan.gerhold@kernkonzept.com
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