The power domain scaling setup for QCS404 and MSM8909 in cpufreq-com-nvmem does not work correctly at the moment because the genpd core ignores all the performance state votes that are specified in the CPU OPP table. This happens because nothing in the driver makes the genpd core aware that the power domains are actively being consumed by the CPU.
Fix this by marking the devices as runtime active. Also mark the devices to be in the "awake path" during system suspend so that performance state votes necessary for the CPU are preserved during system suspend.
While all the patches in this series are needed for full functionality, the cpufreq and pmdomain patches can be merged independently. There is no compile-time dependency between those two.
Signed-off-by: Stephan Gerhold stephan.gerhold@kernkonzept.com --- Changes in v3: - Drop patches with MSM8909 definitions that were applied already - Add extra patch to fix system suspend properly by using device_set_awake_path() instead of dev_pm_syscore_device() - Set GENPD_FLAG_ACTIVE_WAKEUP for rpmpd so that performance state votes needed by the CPU are preserved during suspend - Link to v2: https://lore.kernel.org/r/20231018-msm8909-cpufreq-v2-0-0962df95f654@kernkon...
Changes in v2: - Reword commit messages based on discussion with Uffe - Use generic power domain name "perf" (Uffe) - Fix pm_runtime error handling (Uffe) - Add allocation cleanup patch as preparation - Fix ordering of qcom,msm8909 compatible (Konrad) - cpufreq-dt-platdev blocklist/dt-bindings patches were applied already - Link to v1: https://lore.kernel.org/r/20230912-msm8909-cpufreq-v1-0-767ce66b544b@kernkon...
--- Stephan Gerhold (3): cpufreq: qcom-nvmem: Enable virtual power domain devices cpufreq: qcom-nvmem: Preserve PM domain votes in system suspend pmdomain: qcom: rpmpd: Set GENPD_FLAG_ACTIVE_WAKEUP
drivers/cpufreq/qcom-cpufreq-nvmem.c | 73 ++++++++++++++++++++++++++++++++++-- drivers/pmdomain/qcom/rpmpd.c | 1 + 2 files changed, 71 insertions(+), 3 deletions(-) --- base-commit: b85ea95d086471afb4ad062012a4d73cd328fa86 change-id: 20230906-msm8909-cpufreq-dff238de9ff3
Best regards,
The genpd core caches performance state votes from devices that are runtime suspended as of commit 3c5a272202c2 ("PM: domains: Improve runtime PM performance state handling"). They get applied once the device becomes active again.
To attach the power domains needed by qcom-cpufreq-nvmem the OPP core calls genpd_dev_pm_attach_by_id(). This results in "virtual" dummy devices that use runtime PM only to control the enable and performance state for the attached power domain.
However, at the moment nothing ever resumes the virtual devices created for qcom-cpufreq-nvmem. They remain permanently runtime suspended. This means that performance state votes made during cpufreq scaling get always cached and never applied to the hardware.
Fix this by enabling the devices after attaching them.
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 | 46 +++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c index 6355a39418c5..d239a45ed497 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>
@@ -55,6 +56,7 @@ struct qcom_cpufreq_match_data {
struct qcom_cpufreq_drv_cpu { int opp_token; + struct device **virt_devs; };
struct qcom_cpufreq_drv { @@ -424,6 +426,18 @@ static const struct qcom_cpufreq_match_data match_data_ipq8074 = { .get_version = qcom_cpufreq_ipq8074_name_version, };
+static void qcom_cpufreq_put_virt_devs(struct qcom_cpufreq_drv *drv, unsigned int cpu) +{ + const char * const *name = drv->data->genpd_names; + int i; + + if (!drv->cpus[cpu].virt_devs) + return; + + for (i = 0; *name; i++, name++) + pm_runtime_put(drv->cpus[cpu].virt_devs[i]); +} + static int qcom_cpufreq_probe(struct platform_device *pdev) { struct qcom_cpufreq_drv *drv; @@ -478,6 +492,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev) of_node_put(np);
for_each_possible_cpu(cpu) { + struct device **virt_devs = NULL; struct dev_pm_opp_config config = { .supported_hw = NULL, }; @@ -498,7 +513,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) { @@ -509,6 +524,27 @@ static int qcom_cpufreq_probe(struct platform_device *pdev) goto free_opp; } } + + if (virt_devs) { + const char * const *name = config.genpd_names; + int i, j; + + 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); + + /* Rollback previous PM runtime calls */ + name = config.genpd_names; + for (j = 0; *name && j < i; j++, name++) + pm_runtime_put(virt_devs[j]); + + goto free_opp; + } + } + drv->cpus[cpu].virt_devs = virt_devs; + } }
cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1, @@ -522,8 +558,10 @@ static int qcom_cpufreq_probe(struct platform_device *pdev) dev_err(cpu_dev, "Failed to register platform device\n");
free_opp: - for_each_possible_cpu(cpu) + for_each_possible_cpu(cpu) { + qcom_cpufreq_put_virt_devs(drv, cpu); dev_pm_opp_clear_config(drv->cpus[cpu].opp_token); + } return ret; }
@@ -534,8 +572,10 @@ static void qcom_cpufreq_remove(struct platform_device *pdev)
platform_device_unregister(cpufreq_dt_pdev);
- for_each_possible_cpu(cpu) + for_each_possible_cpu(cpu) { + qcom_cpufreq_put_virt_devs(drv, cpu); dev_pm_opp_clear_config(drv->cpus[cpu].opp_token); + } }
static struct platform_driver qcom_cpufreq_driver = {
On Tue, 14 Nov 2023 at 11:08, Stephan Gerhold stephan.gerhold@kernkonzept.com wrote:
The genpd core caches performance state votes from devices that are runtime suspended as of commit 3c5a272202c2 ("PM: domains: Improve runtime PM performance state handling"). They get applied once the device becomes active again.
To attach the power domains needed by qcom-cpufreq-nvmem the OPP core calls genpd_dev_pm_attach_by_id(). This results in "virtual" dummy devices that use runtime PM only to control the enable and performance state for the attached power domain.
However, at the moment nothing ever resumes the virtual devices created for qcom-cpufreq-nvmem. They remain permanently runtime suspended. This means that performance state votes made during cpufreq scaling get always cached and never applied to the hardware.
Fix this by enabling the devices after attaching them.
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
Reviewed-by: Ulf Hansson ulf.hansson@linaro.org
Kind regards Uffe
drivers/cpufreq/qcom-cpufreq-nvmem.c | 46 +++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/qcom-cpufreq-nvmem.c b/drivers/cpufreq/qcom-cpufreq-nvmem.c index 6355a39418c5..d239a45ed497 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>
@@ -55,6 +56,7 @@ struct qcom_cpufreq_match_data {
struct qcom_cpufreq_drv_cpu { int opp_token;
struct device **virt_devs;
};
struct qcom_cpufreq_drv { @@ -424,6 +426,18 @@ static const struct qcom_cpufreq_match_data match_data_ipq8074 = { .get_version = qcom_cpufreq_ipq8074_name_version, };
+static void qcom_cpufreq_put_virt_devs(struct qcom_cpufreq_drv *drv, unsigned int cpu) +{
const char * const *name = drv->data->genpd_names;
int i;
if (!drv->cpus[cpu].virt_devs)
return;
for (i = 0; *name; i++, name++)
pm_runtime_put(drv->cpus[cpu].virt_devs[i]);
+}
static int qcom_cpufreq_probe(struct platform_device *pdev) { struct qcom_cpufreq_drv *drv; @@ -478,6 +492,7 @@ static int qcom_cpufreq_probe(struct platform_device *pdev) of_node_put(np);
for_each_possible_cpu(cpu) {
struct device **virt_devs = NULL; struct dev_pm_opp_config config = { .supported_hw = NULL, };
@@ -498,7 +513,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) {
@@ -509,6 +524,27 @@ static int qcom_cpufreq_probe(struct platform_device *pdev) goto free_opp; } }
if (virt_devs) {
const char * const *name = config.genpd_names;
int i, j;
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);
/* Rollback previous PM runtime calls */
name = config.genpd_names;
for (j = 0; *name && j < i; j++, name++)
pm_runtime_put(virt_devs[j]);
goto free_opp;
}
}
drv->cpus[cpu].virt_devs = virt_devs;
} } cpufreq_dt_pdev = platform_device_register_simple("cpufreq-dt", -1,
@@ -522,8 +558,10 @@ static int qcom_cpufreq_probe(struct platform_device *pdev) dev_err(cpu_dev, "Failed to register platform device\n");
free_opp:
for_each_possible_cpu(cpu)
for_each_possible_cpu(cpu) {
qcom_cpufreq_put_virt_devs(drv, cpu); dev_pm_opp_clear_config(drv->cpus[cpu].opp_token);
} return ret;
}
@@ -534,8 +572,10 @@ static void qcom_cpufreq_remove(struct platform_device *pdev)
platform_device_unregister(cpufreq_dt_pdev);
for_each_possible_cpu(cpu)
for_each_possible_cpu(cpu) {
qcom_cpufreq_put_virt_devs(drv, cpu); dev_pm_opp_clear_config(drv->cpus[cpu].opp_token);
}
}
static struct platform_driver qcom_cpufreq_driver = {
-- 2.39.2
On 14-11-23, 11:07, Stephan Gerhold wrote:
The power domain scaling setup for QCS404 and MSM8909 in cpufreq-com-nvmem does not work correctly at the moment because the genpd core ignores all the performance state votes that are specified in the CPU OPP table. This happens because nothing in the driver makes the genpd core aware that the power domains are actively being consumed by the CPU.
Fix this by marking the devices as runtime active. Also mark the devices to be in the "awake path" during system suspend so that performance state votes necessary for the CPU are preserved during system suspend.
While all the patches in this series are needed for full functionality, the cpufreq and pmdomain patches can be merged independently. There is no compile-time dependency between those two.
Signed-off-by: Stephan Gerhold stephan.gerhold@kernkonzept.com
Changes in v3:
- Drop patches with MSM8909 definitions that were applied already
- Add extra patch to fix system suspend properly by using device_set_awake_path() instead of dev_pm_syscore_device()
- Set GENPD_FLAG_ACTIVE_WAKEUP for rpmpd so that performance state votes needed by the CPU are preserved during suspend
- Link to v2: https://lore.kernel.org/r/20231018-msm8909-cpufreq-v2-0-0962df95f654@kernkon...
Applied. Thanks.
I picked the pmdomain patch too, lemme know if that needs to go via some other tree.
On Thu, 23 Nov 2023 at 08:39, Viresh Kumar viresh.kumar@linaro.org wrote:
On 14-11-23, 11:07, Stephan Gerhold wrote:
The power domain scaling setup for QCS404 and MSM8909 in cpufreq-com-nvmem does not work correctly at the moment because the genpd core ignores all the performance state votes that are specified in the CPU OPP table. This happens because nothing in the driver makes the genpd core aware that the power domains are actively being consumed by the CPU.
Fix this by marking the devices as runtime active. Also mark the devices to be in the "awake path" during system suspend so that performance state votes necessary for the CPU are preserved during system suspend.
While all the patches in this series are needed for full functionality, the cpufreq and pmdomain patches can be merged independently. There is no compile-time dependency between those two.
Signed-off-by: Stephan Gerhold stephan.gerhold@kernkonzept.com
Changes in v3:
- Drop patches with MSM8909 definitions that were applied already
- Add extra patch to fix system suspend properly by using device_set_awake_path() instead of dev_pm_syscore_device()
- Set GENPD_FLAG_ACTIVE_WAKEUP for rpmpd so that performance state votes needed by the CPU are preserved during suspend
- Link to v2: https://lore.kernel.org/r/20231018-msm8909-cpufreq-v2-0-0962df95f654@kernkon...
Applied. Thanks.
I picked the pmdomain patch too, lemme know if that needs to go via some other tree.
Usually I should pick the pmdomain patches. Although, I thought it may be better to keep this series together.
Assuming you are going to send these as fixes for 6.7-rc[n]? In that case, I can just rebase my tree on a later rc if I find any problems.
Kind regards Uffe
linux-stable-mirror@lists.linaro.org