From: Lina Iyer <ilina@codeaurora.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: linux-pm@vger.kernel.org, "Rafael J. Wysocki" <rjw@rjwysocki.net>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
linux-arm-msm@vger.kernel.org,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Ulf Hansson <ulf.hansson@linaro.org>,
~postmarketos/upstreaming@lists.sr.ht,
Luca Weiss <luca@z3ntu.xyz>
Subject: Re: [PATCH v2] cpuidle: Convert Qualcomm SPM driver to a generic CPUidle driver
Date: Mon, 20 Apr 2020 08:58:19 -0600 [thread overview]
Message-ID: <20200420145819.GE3469@codeaurora.org> (raw)
In-Reply-To: <20200416085821.108778-1-stephan@gerhold.net>
On Thu, Apr 16 2020 at 03:02 -0600, Stephan Gerhold wrote:
>The Qualcomm SPM cpuidle driver seems to be the last driver still
>using the generic ARM CPUidle infrastructure.
>
>Converting it actually allows us to simplify the driver,
>and we end up being able to remove more lines than adding new ones:
>
> - We can parse the CPUidle states in the device tree directly
> with dt_idle_states (and don't need to duplicate that
> functionality into the spm driver).
>
> - Each "saw" device managed by the SPM driver now directly
> registers its own cpuidle driver, removing the need for
> any global (per cpu) state.
>
>The device tree binding is the same, so the driver stays
>compatible with all old device trees.
>
>Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Thanks for doing this. I don't see any obvious issues with the patch.
Reviewed-by: Lina Iyer <ilina@codeaurora.org>
>---
>Related change for the PSCI cpuidle driver:
> https://lore.kernel.org/linux-pm/cover.1565348376.git.lorenzo.pieralisi@arm.com/
>(converting the QCOM SPM driver was mentioned there)
>
>Changes in v2:
> - Rebase on top of linux-next, fix conflicts
> (Apparently sending shortly before the end of the merge window
> was a bad idea... ;) )
>
>v1: https://lore.kernel.org/linux-arm-msm/20200405162052.53622-1-stephan@gerhold.net/
>---
> MAINTAINERS | 1 +
> drivers/cpuidle/Kconfig.arm | 13 ++
> drivers/cpuidle/Makefile | 1 +
> .../qcom/spm.c => cpuidle/cpuidle-qcom-spm.c} | 138 +++++++-----------
> drivers/soc/qcom/Kconfig | 10 --
> drivers/soc/qcom/Makefile | 1 -
> 6 files changed, 67 insertions(+), 97 deletions(-)
> rename drivers/{soc/qcom/spm.c => cpuidle/cpuidle-qcom-spm.c} (75%)
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index e64e5db31497..2fd05a6835a6 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -2223,6 +2223,7 @@ F: drivers/*/qcom*
> F: drivers/*/qcom/
> F: drivers/bluetooth/btqcomsmd.c
> F: drivers/clocksource/timer-qcom.c
>+F: drivers/cpuidle/cpuidle-qcom-spm.c
> F: drivers/extcon/extcon-qcom*
> F: drivers/i2c/busses/i2c-qcom-geni.c
> F: drivers/i2c/busses/i2c-qup.c
>diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>index 99a2d72ac02b..51a7e89085c0 100644
>--- a/drivers/cpuidle/Kconfig.arm
>+++ b/drivers/cpuidle/Kconfig.arm
>@@ -94,3 +94,16 @@ config ARM_TEGRA_CPUIDLE
> select ARM_CPU_SUSPEND
> help
> Select this to enable cpuidle for NVIDIA Tegra20/30/114/124 SoCs.
>+
>+config ARM_QCOM_SPM_CPUIDLE
>+ bool "CPU Idle Driver for Qualcomm Subsystem Power Manager (SPM)"
>+ depends on (ARCH_QCOM || COMPILE_TEST) && !ARM64
>+ select ARM_CPU_SUSPEND
>+ select CPU_IDLE_MULTIPLE_DRIVERS
>+ select DT_IDLE_STATES
>+ select QCOM_SCM
>+ help
>+ Select this to enable cpuidle for Qualcomm processors.
>+ The Subsystem Power Manager (SPM) controls low power modes for the
>+ CPU and L2 cores. It interface with various system drivers to put
>+ the cores in low power modes.
>diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
>index 55a464f6a78b..f07800cbb43f 100644
>--- a/drivers/cpuidle/Makefile
>+++ b/drivers/cpuidle/Makefile
>@@ -25,6 +25,7 @@ obj-$(CONFIG_ARM_PSCI_CPUIDLE) += cpuidle_psci.o
> cpuidle_psci-y := cpuidle-psci.o
> cpuidle_psci-$(CONFIG_PM_GENERIC_DOMAINS_OF) += cpuidle-psci-domain.o
> obj-$(CONFIG_ARM_TEGRA_CPUIDLE) += cpuidle-tegra.o
>+obj-$(CONFIG_ARM_QCOM_SPM_CPUIDLE) += cpuidle-qcom-spm.o
>
> ###############################################################################
> # MIPS drivers
>diff --git a/drivers/soc/qcom/spm.c b/drivers/cpuidle/cpuidle-qcom-spm.c
>similarity index 75%
>rename from drivers/soc/qcom/spm.c
>rename to drivers/cpuidle/cpuidle-qcom-spm.c
>index 8e10e02c6aa5..adf91a6e4d7d 100644
>--- a/drivers/soc/qcom/spm.c
>+++ b/drivers/cpuidle/cpuidle-qcom-spm.c
>@@ -19,10 +19,11 @@
> #include <linux/cpu_pm.h>
> #include <linux/qcom_scm.h>
>
>-#include <asm/cpuidle.h>
> #include <asm/proc-fns.h>
> #include <asm/suspend.h>
>
>+#include "dt_idle_states.h"
>+
> #define MAX_PMIC_DATA 2
> #define MAX_SEQ_DATA 64
> #define SPM_CTL_INDEX 0x7f
>@@ -62,6 +63,7 @@ struct spm_reg_data {
> };
>
> struct spm_driver_data {
>+ struct cpuidle_driver cpuidle_driver;
> void __iomem *reg_base;
> const struct spm_reg_data *reg_data;
> };
>@@ -107,11 +109,6 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
> .start_index[PM_SLEEP_MODE_SPC] = 2,
> };
>
>-static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
>-
>-typedef int (*idle_fn)(void);
>-static DEFINE_PER_CPU(idle_fn*, qcom_idle_ops);
>-
> static inline void spm_register_write(struct spm_driver_data *drv,
> enum spm_reg reg, u32 val)
> {
>@@ -172,10 +169,9 @@ static int qcom_pm_collapse(unsigned long int unused)
> return -1;
> }
>
>-static int qcom_cpu_spc(void)
>+static int qcom_cpu_spc(struct spm_driver_data *drv)
> {
> int ret;
>- struct spm_driver_data *drv = __this_cpu_read(cpu_spm_drv);
>
> spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
> ret = cpu_suspend(0, qcom_pm_collapse);
>@@ -190,94 +186,49 @@ static int qcom_cpu_spc(void)
> return ret;
> }
>
>-static int qcom_idle_enter(unsigned long index)
>+static int spm_enter_idle_state(struct cpuidle_device *dev,
>+ struct cpuidle_driver *drv, int idx)
> {
>- return __this_cpu_read(qcom_idle_ops)[index]();
>+ struct spm_driver_data *data = container_of(drv, struct spm_driver_data,
>+ cpuidle_driver);
>+
>+ return CPU_PM_CPU_IDLE_ENTER_PARAM(qcom_cpu_spc, idx, data);
> }
>
>-static const struct of_device_id qcom_idle_state_match[] __initconst = {
>- { .compatible = "qcom,idle-state-spc", .data = qcom_cpu_spc },
>+static struct cpuidle_driver qcom_spm_idle_driver = {
>+ .name = "qcom_spm",
>+ .owner = THIS_MODULE,
>+ .states[0] = {
>+ .enter = spm_enter_idle_state,
>+ .exit_latency = 1,
>+ .target_residency = 1,
>+ .power_usage = UINT_MAX,
>+ .name = "WFI",
>+ .desc = "ARM WFI",
>+ }
>+};
>+
>+static const struct of_device_id qcom_idle_state_match[] = {
>+ { .compatible = "qcom,idle-state-spc", .data = spm_enter_idle_state },
> { },
> };
>
>-static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
>+static int spm_cpuidle_init(struct cpuidle_driver *drv, int cpu)
> {
>- const struct of_device_id *match_id;
>- struct device_node *state_node;
>- int i;
>- int state_count = 1;
>- idle_fn idle_fns[CPUIDLE_STATE_MAX];
>- idle_fn *fns;
>- cpumask_t mask;
>- bool use_scm_power_down = false;
>-
>- if (!qcom_scm_is_available())
>- return -EPROBE_DEFER;
>-
>- for (i = 0; ; i++) {
>- state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>- if (!state_node)
>- break;
>-
>- if (!of_device_is_available(state_node))
>- continue;
>-
>- if (i == CPUIDLE_STATE_MAX) {
>- pr_warn("%s: cpuidle states reached max possible\n",
>- __func__);
>- break;
>- }
>-
>- match_id = of_match_node(qcom_idle_state_match, state_node);
>- if (!match_id)
>- return -ENODEV;
>-
>- idle_fns[state_count] = match_id->data;
>-
>- /* Check if any of the states allow power down */
>- if (match_id->data == qcom_cpu_spc)
>- use_scm_power_down = true;
>-
>- state_count++;
>- }
>-
>- if (state_count == 1)
>- goto check_spm;
>-
>- fns = devm_kcalloc(get_cpu_device(cpu), state_count, sizeof(*fns),
>- GFP_KERNEL);
>- if (!fns)
>- return -ENOMEM;
>-
>- for (i = 1; i < state_count; i++)
>- fns[i] = idle_fns[i];
>+ int ret;
>
>- if (use_scm_power_down) {
>- /* We have atleast one power down mode */
>- cpumask_clear(&mask);
>- cpumask_set_cpu(cpu, &mask);
>- qcom_scm_set_warm_boot_addr(cpu_resume_arm, &mask);
>- }
>+ memcpy(drv, &qcom_spm_idle_driver, sizeof(*drv));
>+ drv->cpumask = (struct cpumask *)cpumask_of(cpu);
>
>- per_cpu(qcom_idle_ops, cpu) = fns;
>+ /* Parse idle states from device tree */
>+ ret = dt_init_idle_driver(drv, qcom_idle_state_match, 1);
>+ if (ret <= 0)
>+ return ret ? : -ENODEV;
>
>- /*
>- * SPM probe for the cpu should have happened by now, if the
>- * SPM device does not exist, return -ENXIO to indicate that the
>- * cpu does not support idle states.
>- */
>-check_spm:
>- return per_cpu(cpu_spm_drv, cpu) ? 0 : -ENXIO;
>+ /* We have atleast one power down mode */
>+ return qcom_scm_set_warm_boot_addr(cpu_resume_arm, drv->cpumask);
> }
>
>-static const struct cpuidle_ops qcom_cpuidle_ops __initconst = {
>- .suspend = qcom_idle_enter,
>- .init = qcom_cpuidle_init,
>-};
>-
>-CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops);
>-CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops);
>-
> static struct spm_driver_data *spm_get_drv(struct platform_device *pdev,
> int *spm_cpu)
> {
>@@ -323,11 +274,15 @@ static int spm_dev_probe(struct platform_device *pdev)
> struct resource *res;
> const struct of_device_id *match_id;
> void __iomem *addr;
>- int cpu;
>+ int cpu, ret;
>+
>+ if (!qcom_scm_is_available())
>+ return -EPROBE_DEFER;
>
> drv = spm_get_drv(pdev, &cpu);
> if (!drv)
> return -EINVAL;
>+ platform_set_drvdata(pdev, drv);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
>@@ -340,6 +295,10 @@ static int spm_dev_probe(struct platform_device *pdev)
>
> drv->reg_data = match_id->data;
>
>+ ret = spm_cpuidle_init(&drv->cpuidle_driver, cpu);
>+ if (ret)
>+ return ret;
>+
> /* Write the SPM sequences first.. */
> addr = drv->reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY];
> __iowrite32_copy(addr, drv->reg_data->seq,
>@@ -362,13 +321,20 @@ static int spm_dev_probe(struct platform_device *pdev)
> /* Set up Standby as the default low power mode */
> spm_set_low_power_mode(drv, PM_SLEEP_MODE_STBY);
>
>- per_cpu(cpu_spm_drv, cpu) = drv;
>+ return cpuidle_register(&drv->cpuidle_driver, NULL);
>+}
>+
>+static int spm_dev_remove(struct platform_device *pdev)
>+{
>+ struct spm_driver_data *drv = platform_get_drvdata(pdev);
>
>+ cpuidle_unregister(&drv->cpuidle_driver);
> return 0;
> }
>
> static struct platform_driver spm_driver = {
> .probe = spm_dev_probe,
>+ .remove = spm_dev_remove,
> .driver = {
> .name = "saw",
> .of_match_table = spm_match_table,
>diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>index bf42a17a45de..285baa7e474e 100644
>--- a/drivers/soc/qcom/Kconfig
>+++ b/drivers/soc/qcom/Kconfig
>@@ -80,16 +80,6 @@ config QCOM_PDR_HELPERS
> tristate
> select QCOM_QMI_HELPERS
>
>-config QCOM_PM
>- bool "Qualcomm Power Management"
>- depends on ARCH_QCOM && !ARM64
>- select ARM_CPU_SUSPEND
>- select QCOM_SCM
>- help
>- QCOM Platform specific power driver to manage cores and L2 low power
>- modes. It interface with various system drivers to put the cores in
>- low power modes.
>-
> config QCOM_QMI_HELPERS
> tristate
> depends on NET
>diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>index 5d6b83dc58e8..92cc4232d72c 100644
>--- a/drivers/soc/qcom/Makefile
>+++ b/drivers/soc/qcom/Makefile
>@@ -8,7 +8,6 @@ obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
> obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o
> obj-$(CONFIG_QCOM_OCMEM) += ocmem.o
> obj-$(CONFIG_QCOM_PDR_HELPERS) += pdr_interface.o
>-obj-$(CONFIG_QCOM_PM) += spm.o
> obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o
> qmi_helpers-y += qmi_encdec.o qmi_interface.o
> obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
>--
>2.26.1
>
next prev parent reply other threads:[~2020-04-20 14:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-16 8:58 [PATCH v2] cpuidle: Convert Qualcomm SPM driver to a generic CPUidle driver Stephan Gerhold
2020-04-20 14:58 ` Lina Iyer [this message]
2020-04-20 19:10 ` Ulf Hansson
2020-04-20 19:26 ` Bjorn Andersson
2020-05-26 8:24 ` Ulf Hansson
2020-05-26 8:42 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200420145819.GE3469@codeaurora.org \
--to=ilina@codeaurora.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=daniel.lezcano@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=luca@z3ntu.xyz \
--cc=rjw@rjwysocki.net \
--cc=stephan@gerhold.net \
--cc=ulf.hansson@linaro.org \
--cc=~postmarketos/upstreaming@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).