This series adds RISC-V CPU Idle support using SBI HSM suspend function.
The RISC-V SBI CPU idle driver added by this series is highly inspired
from the ARM PSCI CPU idle driver.
At high-level, this series includes the following changes:
1) Preparatory arch/riscv patches (Patches 1 to 3)
2) Defines for RISC-V SBI HSM suspend (Patch 4)
3) Preparatory patch to share code between RISC-V SBI CPU idle driver
and ARM PSCI CPU idle driver (Patch 5)
4) RISC-V SBI CPU idle driver and related DT bindings (Patches 6 to 7)
These patches can be found in riscv_sbi_hsm_suspend_v4 branch at
https://github.com/avpatel/linux
Special thanks Sandeep Tripathy for providing early feeback on SBI HSM
support in all above projects (RISC-V SBI specification, OpenSBI, and
Linux RISC-V).
Changes since v3:
- Rebased on Linux-5.13-rc2
- Fixed __cpu_resume_enter() which was broken due to XIP kernel support
- Removed "struct dt_idle_genpd_ops" abstraction which simplifies code
sharing between ARM PSCI and RISC-V SBI drivers in PATCH5
Changes since v2:
- Rebased on Linux-5.12-rc3
- Updated PATCH7 to add common DT bindings for both ARM and RISC-V
idle states
- Added "additionalProperties = false" for both idle-states node and
child nodes in PATCH7
Changes since v1:
- Fixex minor typo in PATCH1
- Use just "idle-states" as DT node name for CPU idle states
- Added documentation for "cpu-idle-states" DT property in
devicetree/bindings/riscv/cpus.yaml
- Added documentation for "riscv,sbi-suspend-param" DT property in
devicetree/bindings/riscv/idle-states.yaml
Anup Patel (8):
RISC-V: Enable CPU_IDLE drivers
RISC-V: Rename relocate() and make it global
RISC-V: Add arch functions for non-retentive suspend entry/exit
RISC-V: Add SBI HSM suspend related defines
cpuidle: Factor-out power domain related code from PSCI domain driver
cpuidle: Add RISC-V SBI CPU idle driver
dt-bindings: Add common bindings for ARM and RISC-V idle states
RISC-V: Enable RISC-V SBI CPU Idle driver for QEMU virt machine
.../bindings/{arm => cpu}/idle-states.yaml | 228 ++++++-
.../devicetree/bindings/riscv/cpus.yaml | 6 +
MAINTAINERS | 7 +
arch/riscv/Kconfig | 7 +
arch/riscv/Kconfig.socs | 3 +
arch/riscv/configs/defconfig | 13 +-
arch/riscv/configs/rv32_defconfig | 6 +-
arch/riscv/include/asm/asm.h | 17 +
arch/riscv/include/asm/cpuidle.h | 24 +
arch/riscv/include/asm/sbi.h | 27 +-
arch/riscv/include/asm/suspend.h | 35 +
arch/riscv/kernel/Makefile | 2 +
arch/riscv/kernel/asm-offsets.c | 3 +
arch/riscv/kernel/cpu_ops_sbi.c | 2 +-
arch/riscv/kernel/head.S | 18 +-
arch/riscv/kernel/process.c | 3 +-
arch/riscv/kernel/suspend.c | 86 +++
arch/riscv/kernel/suspend_entry.S | 123 ++++
drivers/cpuidle/Kconfig | 9 +
drivers/cpuidle/Kconfig.arm | 1 +
drivers/cpuidle/Kconfig.riscv | 15 +
drivers/cpuidle/Makefile | 5 +
drivers/cpuidle/cpuidle-psci-domain.c | 138 +---
drivers/cpuidle/cpuidle-psci.h | 15 +-
drivers/cpuidle/cpuidle-sbi.c | 625 ++++++++++++++++++
drivers/cpuidle/dt_idle_genpd.c | 182 +++++
drivers/cpuidle/dt_idle_genpd.h | 50 ++
27 files changed, 1467 insertions(+), 183 deletions(-)
rename Documentation/devicetree/bindings/{arm => cpu}/idle-states.yaml (74%)
create mode 100644 arch/riscv/include/asm/cpuidle.h
create mode 100644 arch/riscv/include/asm/suspend.h
create mode 100644 arch/riscv/kernel/suspend.c
create mode 100644 arch/riscv/kernel/suspend_entry.S
create mode 100644 drivers/cpuidle/Kconfig.riscv
create mode 100644 drivers/cpuidle/cpuidle-sbi.c
create mode 100644 drivers/cpuidle/dt_idle_genpd.c
create mode 100644 drivers/cpuidle/dt_idle_genpd.h
--
2.25.1
The low-level relocate() function enables mmu and relocates
execution to link-time addresses. We rename relocate() function
to relocate_enable_mmu() function which is more informative.
Also, the relocate_enable_mmu() function will be used in the
resume path when a CPU wakes-up from a non-retentive suspend
so we make it global symbol.
Signed-off-by: Anup Patel <[email protected]>
---
arch/riscv/kernel/head.S | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 89cc58ab52b4..a44c0bc9c2f3 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -79,7 +79,8 @@ pe_head_start:
.align 2
#ifdef CONFIG_MMU
-relocate:
+ .global relocate_enable_mmu
+relocate_enable_mmu:
/* Relocate return address */
la a1, kernel_virt_addr
XIP_FIXUP_OFFSET a1
@@ -174,7 +175,7 @@ secondary_start_common:
/* Enable virtual memory and relocate to virtual address */
la a0, swapper_pg_dir
XIP_FIXUP_OFFSET a0
- call relocate
+ call relocate_enable_mmu
#endif
call setup_trap_vector
tail smp_callin
@@ -311,7 +312,7 @@ clear_bss_done:
#ifdef CONFIG_MMU
la a0, early_pg_dir
XIP_FIXUP_OFFSET a0
- call relocate
+ call relocate_enable_mmu
#endif /* CONFIG_MMU */
call setup_trap_vector
--
2.25.1
We add defines related to SBI HSM suspend call and also
update HSM states naming as-per latest SBI specification.
Signed-off-by: Anup Patel <[email protected]>
---
arch/riscv/include/asm/sbi.h | 27 ++++++++++++++++++++++-----
arch/riscv/kernel/cpu_ops_sbi.c | 2 +-
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 289621da4a2a..ab9782f8da52 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -62,15 +62,32 @@ enum sbi_ext_hsm_fid {
SBI_EXT_HSM_HART_START = 0,
SBI_EXT_HSM_HART_STOP,
SBI_EXT_HSM_HART_STATUS,
+ SBI_EXT_HSM_HART_SUSPEND,
};
-enum sbi_hsm_hart_status {
- SBI_HSM_HART_STATUS_STARTED = 0,
- SBI_HSM_HART_STATUS_STOPPED,
- SBI_HSM_HART_STATUS_START_PENDING,
- SBI_HSM_HART_STATUS_STOP_PENDING,
+enum sbi_hsm_hart_state {
+ SBI_HSM_STATE_STARTED = 0,
+ SBI_HSM_STATE_STOPPED,
+ SBI_HSM_STATE_START_PENDING,
+ SBI_HSM_STATE_STOP_PENDING,
+ SBI_HSM_STATE_SUSPENDED,
+ SBI_HSM_STATE_SUSPEND_PENDING,
+ SBI_HSM_STATE_RESUME_PENDING,
};
+#define SBI_HSM_SUSP_BASE_MASK 0x7fffffff
+#define SBI_HSM_SUSP_NON_RET_BIT 0x80000000
+#define SBI_HSM_SUSP_PLAT_BASE 0x10000000
+
+#define SBI_HSM_SUSPEND_RET_DEFAULT 0x00000000
+#define SBI_HSM_SUSPEND_RET_PLATFORM SBI_HSM_SUSP_PLAT_BASE
+#define SBI_HSM_SUSPEND_RET_LAST SBI_HSM_SUSP_BASE_MASK
+#define SBI_HSM_SUSPEND_NON_RET_DEFAULT SBI_HSM_SUSP_NON_RET_BIT
+#define SBI_HSM_SUSPEND_NON_RET_PLATFORM (SBI_HSM_SUSP_NON_RET_BIT | \
+ SBI_HSM_SUSP_PLAT_BASE)
+#define SBI_HSM_SUSPEND_NON_RET_LAST (SBI_HSM_SUSP_NON_RET_BIT | \
+ SBI_HSM_SUSP_BASE_MASK)
+
enum sbi_ext_srst_fid {
SBI_EXT_SRST_RESET = 0,
};
diff --git a/arch/riscv/kernel/cpu_ops_sbi.c b/arch/riscv/kernel/cpu_ops_sbi.c
index 685fae72b7f5..5fd90f03a3e9 100644
--- a/arch/riscv/kernel/cpu_ops_sbi.c
+++ b/arch/riscv/kernel/cpu_ops_sbi.c
@@ -97,7 +97,7 @@ static int sbi_cpu_is_stopped(unsigned int cpuid)
rc = sbi_hsm_hart_get_status(hartid);
- if (rc == SBI_HSM_HART_STATUS_STOPPED)
+ if (rc == SBI_HSM_STATE_STOPPED)
return 0;
return rc;
}
--
2.25.1
The generic power domain related code in PSCI domain driver is largely
independent of PSCI and can be shared with RISC-V SBI domain driver
hence we factor-out this code into dt_idle_genpd.c and dt_idle_genpd.h.
Signed-off-by: Anup Patel <[email protected]>
---
drivers/cpuidle/Kconfig | 4 +
drivers/cpuidle/Kconfig.arm | 1 +
drivers/cpuidle/Makefile | 1 +
drivers/cpuidle/cpuidle-psci-domain.c | 138 +------------------
drivers/cpuidle/cpuidle-psci.h | 15 ++-
drivers/cpuidle/dt_idle_genpd.c | 182 ++++++++++++++++++++++++++
drivers/cpuidle/dt_idle_genpd.h | 50 +++++++
7 files changed, 256 insertions(+), 135 deletions(-)
create mode 100644 drivers/cpuidle/dt_idle_genpd.c
create mode 100644 drivers/cpuidle/dt_idle_genpd.h
diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c0aeedd66f02..f1afe7ab6b54 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -47,6 +47,10 @@ config CPU_IDLE_GOV_HALTPOLL
config DT_IDLE_STATES
bool
+config DT_IDLE_GENPD
+ depends on PM_GENERIC_DOMAINS_OF
+ bool
+
menu "ARM CPU Idle Drivers"
depends on ARM || ARM64
source "drivers/cpuidle/Kconfig.arm"
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 334f83e56120..be12a9ca78f0 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -27,6 +27,7 @@ config ARM_PSCI_CPUIDLE_DOMAIN
bool "PSCI CPU idle Domain"
depends on ARM_PSCI_CPUIDLE
depends on PM_GENERIC_DOMAINS_OF
+ select DT_IDLE_GENPD
default y
help
Select this to enable the PSCI based CPUidle driver to use PM domains,
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 26bbc5e74123..11a26cef279f 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -6,6 +6,7 @@
obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o
+obj-$(CONFIG_DT_IDLE_GENPD) += dt_idle_genpd.o
obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o
obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o
diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
index ff2c3f8e4668..3ceec0ee9f02 100644
--- a/drivers/cpuidle/cpuidle-psci-domain.c
+++ b/drivers/cpuidle/cpuidle-psci-domain.c
@@ -47,73 +47,14 @@ static int psci_pd_power_off(struct generic_pm_domain *pd)
return 0;
}
-static int psci_pd_parse_state_nodes(struct genpd_power_state *states,
- int state_count)
-{
- int i, ret;
- u32 psci_state, *psci_state_buf;
-
- for (i = 0; i < state_count; i++) {
- ret = psci_dt_parse_state_node(to_of_node(states[i].fwnode),
- &psci_state);
- if (ret)
- goto free_state;
-
- psci_state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
- if (!psci_state_buf) {
- ret = -ENOMEM;
- goto free_state;
- }
- *psci_state_buf = psci_state;
- states[i].data = psci_state_buf;
- }
-
- return 0;
-
-free_state:
- i--;
- for (; i >= 0; i--)
- kfree(states[i].data);
- return ret;
-}
-
-static int psci_pd_parse_states(struct device_node *np,
- struct genpd_power_state **states, int *state_count)
-{
- int ret;
-
- /* Parse the domain idle states. */
- ret = of_genpd_parse_idle_states(np, states, state_count);
- if (ret)
- return ret;
-
- /* Fill out the PSCI specifics for each found state. */
- ret = psci_pd_parse_state_nodes(*states, *state_count);
- if (ret)
- kfree(*states);
-
- return ret;
-}
-
-static void psci_pd_free_states(struct genpd_power_state *states,
- unsigned int state_count)
-{
- int i;
-
- for (i = 0; i < state_count; i++)
- kfree(states[i].data);
- kfree(states);
-}
-
static int psci_pd_init(struct device_node *np, bool use_osi)
{
struct generic_pm_domain *pd;
struct psci_pd_provider *pd_provider;
struct dev_power_governor *pd_gov;
- struct genpd_power_state *states = NULL;
int ret = -ENOMEM, state_count = 0;
- pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+ pd = dt_pd_alloc(np, psci_dt_parse_state_node);
if (!pd)
goto out;
@@ -121,22 +62,6 @@ static int psci_pd_init(struct device_node *np, bool use_osi)
if (!pd_provider)
goto free_pd;
- pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
- if (!pd->name)
- goto free_pd_prov;
-
- /*
- * Parse the domain idle states and let genpd manage the state selection
- * for those being compatible with "domain-idle-state".
- */
- ret = psci_pd_parse_states(np, &states, &state_count);
- if (ret)
- goto free_name;
-
- pd->free_states = psci_pd_free_states;
- pd->name = kbasename(pd->name);
- pd->states = states;
- pd->state_count = state_count;
pd->flags |= GENPD_FLAG_IRQ_SAFE | GENPD_FLAG_CPU_DOMAIN;
/* Allow power off when OSI has been successfully enabled. */
@@ -149,10 +74,8 @@ static int psci_pd_init(struct device_node *np, bool use_osi)
pd_gov = state_count > 0 ? &pm_domain_cpu_gov : NULL;
ret = pm_genpd_init(pd, pd_gov, false);
- if (ret) {
- psci_pd_free_states(states, state_count);
- goto free_name;
- }
+ if (ret)
+ goto free_pd_prov;
ret = of_genpd_add_provider_simple(np, pd);
if (ret)
@@ -166,12 +89,10 @@ static int psci_pd_init(struct device_node *np, bool use_osi)
remove_pd:
pm_genpd_remove(pd);
-free_name:
- kfree(pd->name);
free_pd_prov:
kfree(pd_provider);
free_pd:
- kfree(pd);
+ dt_pd_free(pd);
out:
pr_err("failed to init PM domain ret=%d %pOF\n", ret, np);
return ret;
@@ -195,30 +116,6 @@ static void psci_pd_remove(void)
}
}
-static int psci_pd_init_topology(struct device_node *np)
-{
- struct device_node *node;
- struct of_phandle_args child, parent;
- int ret;
-
- for_each_child_of_node(np, node) {
- if (of_parse_phandle_with_args(node, "power-domains",
- "#power-domain-cells", 0, &parent))
- continue;
-
- child.np = node;
- child.args_count = 0;
- ret = of_genpd_add_subdomain(&parent, &child);
- of_node_put(parent.np);
- if (ret) {
- of_node_put(node);
- return ret;
- }
- }
-
- return 0;
-}
-
static bool psci_pd_try_set_osi_mode(void)
{
int ret;
@@ -282,7 +179,7 @@ static int psci_cpuidle_domain_probe(struct platform_device *pdev)
goto no_pd;
/* Link genpd masters/subdomains to model the CPU topology. */
- ret = psci_pd_init_topology(np);
+ ret = dt_pd_init_topology(np);
if (ret)
goto remove_pd;
@@ -314,28 +211,3 @@ static int __init psci_idle_init_domains(void)
return platform_driver_register(&psci_cpuidle_domain_driver);
}
subsys_initcall(psci_idle_init_domains);
-
-struct device *psci_dt_attach_cpu(int cpu)
-{
- struct device *dev;
-
- dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
- if (IS_ERR_OR_NULL(dev))
- return dev;
-
- pm_runtime_irq_safe(dev);
- if (cpu_online(cpu))
- pm_runtime_get_sync(dev);
-
- dev_pm_syscore_device(dev, true);
-
- return dev;
-}
-
-void psci_dt_detach_cpu(struct device *dev)
-{
- if (IS_ERR_OR_NULL(dev))
- return;
-
- dev_pm_domain_detach(dev, false);
-}
diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
index d8e925e84c27..70de1e3c00af 100644
--- a/drivers/cpuidle/cpuidle-psci.h
+++ b/drivers/cpuidle/cpuidle-psci.h
@@ -10,8 +10,19 @@ void psci_set_domain_state(u32 state);
int psci_dt_parse_state_node(struct device_node *np, u32 *state);
#ifdef CONFIG_ARM_PSCI_CPUIDLE_DOMAIN
-struct device *psci_dt_attach_cpu(int cpu);
-void psci_dt_detach_cpu(struct device *dev);
+
+#include "dt_idle_genpd.h"
+
+static inline struct device *psci_dt_attach_cpu(int cpu)
+{
+ return dt_idle_genpd_attach_cpu(cpu, "psci");
+}
+
+static inline void psci_dt_detach_cpu(struct device *dev)
+{
+ dt_idle_genpd_detach_cpu(dev);
+}
+
#else
static inline struct device *psci_dt_attach_cpu(int cpu) { return NULL; }
static inline void psci_dt_detach_cpu(struct device *dev) { }
diff --git a/drivers/cpuidle/dt_idle_genpd.c b/drivers/cpuidle/dt_idle_genpd.c
new file mode 100644
index 000000000000..5a901773db60
--- /dev/null
+++ b/drivers/cpuidle/dt_idle_genpd.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * PM domains for CPUs via genpd.
+ *
+ * Copyright (C) 2019 Linaro Ltd.
+ * Author: Ulf Hansson <[email protected]>
+ *
+ * Copyright (c) 2021 Western Digital Corporation or its affiliates.
+ */
+
+#define pr_fmt(fmt) "dt-idle-genpd: " fmt
+
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+#include "dt_idle_genpd.h"
+
+static int dt_pd_parse_state_nodes(
+ int (*parse_state)(struct device_node *, u32 *),
+ struct genpd_power_state *states, int state_count)
+{
+ int i, ret;
+ u32 state, *state_buf;
+
+ for (i = 0; i < state_count; i++) {
+ ret = parse_state(to_of_node(states[i].fwnode), &state);
+ if (ret)
+ goto free_state;
+
+ state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
+ if (!state_buf) {
+ ret = -ENOMEM;
+ goto free_state;
+ }
+ *state_buf = state;
+ states[i].data = state_buf;
+ }
+
+ return 0;
+
+free_state:
+ i--;
+ for (; i >= 0; i--)
+ kfree(states[i].data);
+ return ret;
+}
+
+static int dt_pd_parse_states(struct device_node *np,
+ int (*parse_state)(struct device_node *, u32 *),
+ struct genpd_power_state **states,
+ int *state_count)
+{
+ int ret;
+
+ /* Parse the domain idle states. */
+ ret = of_genpd_parse_idle_states(np, states, state_count);
+ if (ret)
+ return ret;
+
+ /* Fill out the dt specifics for each found state. */
+ ret = dt_pd_parse_state_nodes(parse_state, *states, *state_count);
+ if (ret)
+ kfree(*states);
+
+ return ret;
+}
+
+static void dt_pd_free_states(struct genpd_power_state *states,
+ unsigned int state_count)
+{
+ int i;
+
+ for (i = 0; i < state_count; i++)
+ kfree(states[i].data);
+ kfree(states);
+}
+
+void dt_pd_free(struct generic_pm_domain *pd)
+{
+ dt_pd_free_states(pd->states, pd->state_count);
+ kfree(pd->name);
+ kfree(pd);
+}
+EXPORT_SYMBOL_GPL(dt_pd_free);
+
+struct generic_pm_domain *dt_pd_alloc(struct device_node *np,
+ int (*parse_state)(struct device_node *, u32 *))
+{
+ struct generic_pm_domain *pd;
+ struct genpd_power_state *states = NULL;
+ int ret, state_count = 0;
+
+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ goto out;
+
+ pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
+ if (!pd->name)
+ goto free_pd;
+
+ /*
+ * Parse the domain idle states and let genpd manage the state selection
+ * for those being compatible with "domain-idle-state".
+ */
+ ret = dt_pd_parse_states(np, parse_state, &states, &state_count);
+ if (ret)
+ goto free_name;
+
+ pd->free_states = dt_pd_free_states;
+ pd->name = kbasename(pd->name);
+ pd->states = states;
+ pd->state_count = state_count;
+
+ pr_debug("alloc PM domain %s\n", pd->name);
+ return pd;
+
+free_name:
+ kfree(pd->name);
+free_pd:
+ kfree(pd);
+out:
+ pr_err("failed to alloc PM domain %pOF\n", np);
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(dt_pd_alloc);
+
+int dt_pd_init_topology(struct device_node *np)
+{
+ struct device_node *node;
+ struct of_phandle_args child, parent;
+ int ret;
+
+ for_each_child_of_node(np, node) {
+ if (of_parse_phandle_with_args(node, "power-domains",
+ "#power-domain-cells", 0, &parent))
+ continue;
+
+ child.np = node;
+ child.args_count = 0;
+ ret = of_genpd_add_subdomain(&parent, &child);
+ of_node_put(parent.np);
+ if (ret) {
+ of_node_put(node);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dt_pd_init_topology);
+
+struct device *dt_idle_genpd_attach_cpu(int cpu, const char *name)
+{
+ struct device *dev;
+
+ dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), name);
+ if (IS_ERR_OR_NULL(dev))
+ return dev;
+
+ pm_runtime_irq_safe(dev);
+ if (cpu_online(cpu))
+ pm_runtime_get_sync(dev);
+
+ dev_pm_syscore_device(dev, true);
+
+ return dev;
+}
+EXPORT_SYMBOL_GPL(dt_idle_genpd_attach_cpu);
+
+void dt_idle_genpd_detach_cpu(struct device *dev)
+{
+ if (IS_ERR_OR_NULL(dev))
+ return;
+
+ dev_pm_domain_detach(dev, false);
+}
+EXPORT_SYMBOL_GPL(dt_idle_genpd_detach_cpu);
diff --git a/drivers/cpuidle/dt_idle_genpd.h b/drivers/cpuidle/dt_idle_genpd.h
new file mode 100644
index 000000000000..23ebf6050e74
--- /dev/null
+++ b/drivers/cpuidle/dt_idle_genpd.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __DT_IDLE_GENPD
+#define __DT_IDLE_GENPD
+
+struct device_node;
+struct generic_pm_domain;
+
+#ifdef CONFIG_DT_IDLE_GENPD
+
+void dt_pd_free(struct generic_pm_domain *pd);
+
+struct generic_pm_domain *dt_pd_alloc(struct device_node *np,
+ int (*parse_state)(struct device_node *, u32 *));
+
+int dt_pd_init_topology(struct device_node *np);
+
+struct device *dt_idle_genpd_attach_cpu(int cpu, const char *name);
+
+void dt_idle_genpd_detach_cpu(struct device *dev);
+
+#else
+
+static inline void dt_pd_free(struct generic_pm_domain *pd)
+{
+}
+
+static inline struct generic_pm_domain *dt_pd_alloc(struct device_node *np,
+ int (*parse_state)(struct device_node *, u32 *));
+{
+ return NULL;
+}
+
+static inline int dt_pd_init_topology(struct device_node *np)
+{
+ return 0;
+}
+
+static inline struct device *dt_idle_genpd_attach_cpu(int cpu,
+ const char *name)
+{
+ return NULL;
+}
+
+static inline void dt_idle_genpd_detach_cpu(struct device *dev)
+{
+}
+
+#endif
+
+#endif
--
2.25.1
On Mon, 17 May 2021 at 15:10, Anup Patel <[email protected]> wrote:
>
> The generic power domain related code in PSCI domain driver is largely
> independent of PSCI and can be shared with RISC-V SBI domain driver
> hence we factor-out this code into dt_idle_genpd.c and dt_idle_genpd.h.
>
> Signed-off-by: Anup Patel <[email protected]>
This is clearly a big step in the right direction. Just a couple minor
things, see more below.
Note that, I have a couple of patches in the pipe for the
cpuidle-psci-domain driver (not ready to be posted). I need a couple
of more days to confirm this restructuring still makes sense beyond
these potential new changes. I will let you know as soon as I can with
the outcome.
[...]
> diff --git a/drivers/cpuidle/dt_idle_genpd.c b/drivers/cpuidle/dt_idle_genpd.c
I think it would be a good idea to add a new section for this to the
MAINTAINERS file. Perhaps a "DT IDLE DOMAIN" section? Or perhaps you
have another idea?
In any case, I am happy to continue with maintenance of this code,
even in the new restructured form.
> new file mode 100644
> index 000000000000..5a901773db60
> --- /dev/null
> +++ b/drivers/cpuidle/dt_idle_genpd.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PM domains for CPUs via genpd.
> + *
> + * Copyright (C) 2019 Linaro Ltd.
> + * Author: Ulf Hansson <[email protected]>
> + *
> + * Copyright (c) 2021 Western Digital Corporation or its affiliates.
> + */
> +
> +#define pr_fmt(fmt) "dt-idle-genpd: " fmt
> +
> +#include <linux/cpu.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +
> +#include "dt_idle_genpd.h"
> +
> +static int dt_pd_parse_state_nodes(
> + int (*parse_state)(struct device_node *, u32 *),
> + struct genpd_power_state *states, int state_count)
> +{
> + int i, ret;
> + u32 state, *state_buf;
> +
> + for (i = 0; i < state_count; i++) {
> + ret = parse_state(to_of_node(states[i].fwnode), &state);
> + if (ret)
> + goto free_state;
> +
> + state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
> + if (!state_buf) {
> + ret = -ENOMEM;
> + goto free_state;
> + }
> + *state_buf = state;
> + states[i].data = state_buf;
> + }
> +
> + return 0;
> +
> +free_state:
> + i--;
> + for (; i >= 0; i--)
> + kfree(states[i].data);
> + return ret;
> +}
> +
> +static int dt_pd_parse_states(struct device_node *np,
> + int (*parse_state)(struct device_node *, u32 *),
> + struct genpd_power_state **states,
> + int *state_count)
> +{
> + int ret;
> +
> + /* Parse the domain idle states. */
> + ret = of_genpd_parse_idle_states(np, states, state_count);
> + if (ret)
> + return ret;
> +
> + /* Fill out the dt specifics for each found state. */
> + ret = dt_pd_parse_state_nodes(parse_state, *states, *state_count);
> + if (ret)
> + kfree(*states);
> +
> + return ret;
> +}
> +
> +static void dt_pd_free_states(struct genpd_power_state *states,
> + unsigned int state_count)
> +{
> + int i;
> +
> + for (i = 0; i < state_count; i++)
> + kfree(states[i].data);
> + kfree(states);
> +}
> +
> +void dt_pd_free(struct generic_pm_domain *pd)
> +{
> + dt_pd_free_states(pd->states, pd->state_count);
> + kfree(pd->name);
> + kfree(pd);
> +}
> +EXPORT_SYMBOL_GPL(dt_pd_free);
> +
> +struct generic_pm_domain *dt_pd_alloc(struct device_node *np,
> + int (*parse_state)(struct device_node *, u32 *))
> +{
> + struct generic_pm_domain *pd;
> + struct genpd_power_state *states = NULL;
> + int ret, state_count = 0;
> +
> + pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> + if (!pd)
> + goto out;
> +
> + pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
> + if (!pd->name)
> + goto free_pd;
> +
> + /*
> + * Parse the domain idle states and let genpd manage the state selection
> + * for those being compatible with "domain-idle-state".
> + */
> + ret = dt_pd_parse_states(np, parse_state, &states, &state_count);
> + if (ret)
> + goto free_name;
> +
> + pd->free_states = dt_pd_free_states;
> + pd->name = kbasename(pd->name);
> + pd->states = states;
> + pd->state_count = state_count;
> +
> + pr_debug("alloc PM domain %s\n", pd->name);
> + return pd;
> +
> +free_name:
> + kfree(pd->name);
> +free_pd:
> + kfree(pd);
> +out:
> + pr_err("failed to alloc PM domain %pOF\n", np);
> + return NULL;
> +}
> +EXPORT_SYMBOL_GPL(dt_pd_alloc);
> +
> +int dt_pd_init_topology(struct device_node *np)
> +{
> + struct device_node *node;
> + struct of_phandle_args child, parent;
> + int ret;
> +
> + for_each_child_of_node(np, node) {
> + if (of_parse_phandle_with_args(node, "power-domains",
> + "#power-domain-cells", 0, &parent))
> + continue;
> +
> + child.np = node;
> + child.args_count = 0;
> + ret = of_genpd_add_subdomain(&parent, &child);
> + of_node_put(parent.np);
> + if (ret) {
> + of_node_put(node);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dt_pd_init_topology);
May I suggest that we stick to dt_idle_* as the prefix for all of the
exported functions in this file. Static functions can just skip the
prefix altogether.
> +
> +struct device *dt_idle_genpd_attach_cpu(int cpu, const char *name)
> +{
> + struct device *dev;
> +
> + dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), name);
> + if (IS_ERR_OR_NULL(dev))
> + return dev;
> +
> + pm_runtime_irq_safe(dev);
> + if (cpu_online(cpu))
> + pm_runtime_get_sync(dev);
> +
> + dev_pm_syscore_device(dev, true);
> +
> + return dev;
> +}
> +EXPORT_SYMBOL_GPL(dt_idle_genpd_attach_cpu);
> +
> +void dt_idle_genpd_detach_cpu(struct device *dev)
> +{
> + if (IS_ERR_OR_NULL(dev))
> + return;
> +
> + dev_pm_domain_detach(dev, false);
> +}
> +EXPORT_SYMBOL_GPL(dt_idle_genpd_detach_cpu);
Again, a minor comment on the naming of the functions. How about
skipping "genpd" in the prefix, thus just dt_idle_attach|detach_cpu()?
[...]
Kind regards
Uffe
On Mon, May 24, 2021 at 11:31 PM Ulf Hansson <[email protected]> wrote:
>
> On Mon, 17 May 2021 at 15:10, Anup Patel <[email protected]> wrote:
> >
> > The generic power domain related code in PSCI domain driver is largely
> > independent of PSCI and can be shared with RISC-V SBI domain driver
> > hence we factor-out this code into dt_idle_genpd.c and dt_idle_genpd.h.
> >
> > Signed-off-by: Anup Patel <[email protected]>
>
> This is clearly a big step in the right direction. Just a couple minor
> things, see more below.
>
> Note that, I have a couple of patches in the pipe for the
> cpuidle-psci-domain driver (not ready to be posted). I need a couple
> of more days to confirm this restructuring still makes sense beyond
> these potential new changes. I will let you know as soon as I can with
> the outcome.
Sure, I will wait for more comments from you. I was thinking of sending
next revision of patches sometime next week with the renaming of
function names which you suggested.
>
> [...]
>
> > diff --git a/drivers/cpuidle/dt_idle_genpd.c b/drivers/cpuidle/dt_idle_genpd.c
>
> I think it would be a good idea to add a new section for this to the
> MAINTAINERS file. Perhaps a "DT IDLE DOMAIN" section? Or perhaps you
> have another idea?
>
> In any case, I am happy to continue with maintenance of this code,
> even in the new restructured form.
Yes, a separate "DT IDLE DOMAIN" section in MAINTAINERS file
sounds good to me.
Anyway the dt_idle_genpd is factored-out code from cpuidle-psci-domain.c
so I suggest you to maintain dt_idle_genpd as well.
Do you want me to add a "DT IDLE DOMAIN" section in the
MAINTAINERS file as part of this patch ??
>
> > new file mode 100644
> > index 000000000000..5a901773db60
> > --- /dev/null
> > +++ b/drivers/cpuidle/dt_idle_genpd.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * PM domains for CPUs via genpd.
> > + *
> > + * Copyright (C) 2019 Linaro Ltd.
> > + * Author: Ulf Hansson <[email protected]>
> > + *
> > + * Copyright (c) 2021 Western Digital Corporation or its affiliates.
> > + */
> > +
> > +#define pr_fmt(fmt) "dt-idle-genpd: " fmt
> > +
> > +#include <linux/cpu.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +
> > +#include "dt_idle_genpd.h"
> > +
> > +static int dt_pd_parse_state_nodes(
> > + int (*parse_state)(struct device_node *, u32 *),
> > + struct genpd_power_state *states, int state_count)
> > +{
> > + int i, ret;
> > + u32 state, *state_buf;
> > +
> > + for (i = 0; i < state_count; i++) {
> > + ret = parse_state(to_of_node(states[i].fwnode), &state);
> > + if (ret)
> > + goto free_state;
> > +
> > + state_buf = kmalloc(sizeof(u32), GFP_KERNEL);
> > + if (!state_buf) {
> > + ret = -ENOMEM;
> > + goto free_state;
> > + }
> > + *state_buf = state;
> > + states[i].data = state_buf;
> > + }
> > +
> > + return 0;
> > +
> > +free_state:
> > + i--;
> > + for (; i >= 0; i--)
> > + kfree(states[i].data);
> > + return ret;
> > +}
> > +
> > +static int dt_pd_parse_states(struct device_node *np,
> > + int (*parse_state)(struct device_node *, u32 *),
> > + struct genpd_power_state **states,
> > + int *state_count)
> > +{
> > + int ret;
> > +
> > + /* Parse the domain idle states. */
> > + ret = of_genpd_parse_idle_states(np, states, state_count);
> > + if (ret)
> > + return ret;
> > +
> > + /* Fill out the dt specifics for each found state. */
> > + ret = dt_pd_parse_state_nodes(parse_state, *states, *state_count);
> > + if (ret)
> > + kfree(*states);
> > +
> > + return ret;
> > +}
> > +
> > +static void dt_pd_free_states(struct genpd_power_state *states,
> > + unsigned int state_count)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < state_count; i++)
> > + kfree(states[i].data);
> > + kfree(states);
> > +}
> > +
> > +void dt_pd_free(struct generic_pm_domain *pd)
> > +{
> > + dt_pd_free_states(pd->states, pd->state_count);
> > + kfree(pd->name);
> > + kfree(pd);
> > +}
> > +EXPORT_SYMBOL_GPL(dt_pd_free);
> > +
> > +struct generic_pm_domain *dt_pd_alloc(struct device_node *np,
> > + int (*parse_state)(struct device_node *, u32 *))
> > +{
> > + struct generic_pm_domain *pd;
> > + struct genpd_power_state *states = NULL;
> > + int ret, state_count = 0;
> > +
> > + pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> > + if (!pd)
> > + goto out;
> > +
> > + pd->name = kasprintf(GFP_KERNEL, "%pOF", np);
> > + if (!pd->name)
> > + goto free_pd;
> > +
> > + /*
> > + * Parse the domain idle states and let genpd manage the state selection
> > + * for those being compatible with "domain-idle-state".
> > + */
> > + ret = dt_pd_parse_states(np, parse_state, &states, &state_count);
> > + if (ret)
> > + goto free_name;
> > +
> > + pd->free_states = dt_pd_free_states;
> > + pd->name = kbasename(pd->name);
> > + pd->states = states;
> > + pd->state_count = state_count;
> > +
> > + pr_debug("alloc PM domain %s\n", pd->name);
> > + return pd;
> > +
> > +free_name:
> > + kfree(pd->name);
> > +free_pd:
> > + kfree(pd);
> > +out:
> > + pr_err("failed to alloc PM domain %pOF\n", np);
> > + return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(dt_pd_alloc);
> > +
> > +int dt_pd_init_topology(struct device_node *np)
> > +{
> > + struct device_node *node;
> > + struct of_phandle_args child, parent;
> > + int ret;
> > +
> > + for_each_child_of_node(np, node) {
> > + if (of_parse_phandle_with_args(node, "power-domains",
> > + "#power-domain-cells", 0, &parent))
> > + continue;
> > +
> > + child.np = node;
> > + child.args_count = 0;
> > + ret = of_genpd_add_subdomain(&parent, &child);
> > + of_node_put(parent.np);
> > + if (ret) {
> > + of_node_put(node);
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(dt_pd_init_topology);
>
> May I suggest that we stick to dt_idle_* as the prefix for all of the
> exported functions in this file. Static functions can just skip the
> prefix altogether.
Sure, I will update the function names like you suggested in next
patch revision.
>
> > +
> > +struct device *dt_idle_genpd_attach_cpu(int cpu, const char *name)
> > +{
> > + struct device *dev;
> > +
> > + dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), name);
> > + if (IS_ERR_OR_NULL(dev))
> > + return dev;
> > +
> > + pm_runtime_irq_safe(dev);
> > + if (cpu_online(cpu))
> > + pm_runtime_get_sync(dev);
> > +
> > + dev_pm_syscore_device(dev, true);
> > +
> > + return dev;
> > +}
> > +EXPORT_SYMBOL_GPL(dt_idle_genpd_attach_cpu);
> > +
> > +void dt_idle_genpd_detach_cpu(struct device *dev)
> > +{
> > + if (IS_ERR_OR_NULL(dev))
> > + return;
> > +
> > + dev_pm_domain_detach(dev, false);
> > +}
> > +EXPORT_SYMBOL_GPL(dt_idle_genpd_detach_cpu);
>
> Again, a minor comment on the naming of the functions. How about
> skipping "genpd" in the prefix, thus just dt_idle_attach|detach_cpu()?
Sure, I will update.
>
> [...]
>
> Kind regards
> Uffe
Regards,
Anup
On Tue, 25 May 2021 at 07:39, Anup Patel <[email protected]> wrote:
>
> On Mon, May 24, 2021 at 11:31 PM Ulf Hansson <[email protected]> wrote:
> >
> > On Mon, 17 May 2021 at 15:10, Anup Patel <[email protected]> wrote:
> > >
> > > The generic power domain related code in PSCI domain driver is largely
> > > independent of PSCI and can be shared with RISC-V SBI domain driver
> > > hence we factor-out this code into dt_idle_genpd.c and dt_idle_genpd.h.
> > >
> > > Signed-off-by: Anup Patel <[email protected]>
> >
> > This is clearly a big step in the right direction. Just a couple minor
> > things, see more below.
> >
> > Note that, I have a couple of patches in the pipe for the
> > cpuidle-psci-domain driver (not ready to be posted). I need a couple
> > of more days to confirm this restructuring still makes sense beyond
> > these potential new changes. I will let you know as soon as I can with
> > the outcome.
>
> Sure, I will wait for more comments from you. I was thinking of sending
> next revision of patches sometime next week with the renaming of
> function names which you suggested.
Sounds good, that allows me a few more days this week.
>
> >
> > [...]
> >
> > > diff --git a/drivers/cpuidle/dt_idle_genpd.c b/drivers/cpuidle/dt_idle_genpd.c
> >
> > I think it would be a good idea to add a new section for this to the
> > MAINTAINERS file. Perhaps a "DT IDLE DOMAIN" section? Or perhaps you
> > have another idea?
> >
> > In any case, I am happy to continue with maintenance of this code,
> > even in the new restructured form.
>
> Yes, a separate "DT IDLE DOMAIN" section in MAINTAINERS file
> sounds good to me.
>
> Anyway the dt_idle_genpd is factored-out code from cpuidle-psci-domain.c
> so I suggest you to maintain dt_idle_genpd as well.
>
> Do you want me to add a "DT IDLE DOMAIN" section in the
> MAINTAINERS file as part of this patch ??
Yeah, that works for me. Perhaps extend it to .. PM DOMAIN though.
[...]
Kind regards
Uffe
On Tue, 25 May 2021 at 11:05, Ulf Hansson <[email protected]> wrote:
>
> On Tue, 25 May 2021 at 07:39, Anup Patel <[email protected]> wrote:
> >
> > On Mon, May 24, 2021 at 11:31 PM Ulf Hansson <[email protected]> wrote:
> > >
> > > On Mon, 17 May 2021 at 15:10, Anup Patel <[email protected]> wrote:
> > > >
> > > > The generic power domain related code in PSCI domain driver is largely
> > > > independent of PSCI and can be shared with RISC-V SBI domain driver
> > > > hence we factor-out this code into dt_idle_genpd.c and dt_idle_genpd.h.
> > > >
> > > > Signed-off-by: Anup Patel <[email protected]>
> > >
> > > This is clearly a big step in the right direction. Just a couple minor
> > > things, see more below.
> > >
> > > Note that, I have a couple of patches in the pipe for the
> > > cpuidle-psci-domain driver (not ready to be posted). I need a couple
> > > of more days to confirm this restructuring still makes sense beyond
> > > these potential new changes. I will let you know as soon as I can with
> > > the outcome.
> >
> > Sure, I will wait for more comments from you. I was thinking of sending
> > next revision of patches sometime next week with the renaming of
> > function names which you suggested.
>
> Sounds good, that allows me a few more days this week.
I don't have any further comments at this point. It looks good to me,
but let me have a quick review on the next version before I provide my
ack.
[...]
Kind regards
Uffe