LinuxLists.cc - [PATCH v3 0/3] arm: kernel: implement cpuidle

[permalink] [raw]

Subject: [PATCH v3 1/3] firmware: psci: move cpu_suspend handling to generic code

Functions implemented on arm64 to suspend cpu and translate the idle
state index passed by the cpu_suspend core call to a valid PSCI state
are not arm64 specific and should be moved to generic code so that they
can be reused on arm systems too.

This patch moves these functions to generic PSCI firmware layer code.

Signed-off-by: Jisheng Zhang <[email protected]>
---
arch/arm64/kernel/psci.c | 95 ----------------------------------------------
drivers/firmware/psci.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/psci.h | 4 +-
3 files changed, 99 insertions(+), 99 deletions(-)

diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 4be5972..06e5c9a 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -20,7 +20,6 @@
#include <linux/smp.h>
#include <linux/delay.h>
#include <linux/psci.h>
-#include <linux/slab.h>

#include <uapi/linux/psci.h>

@@ -28,73 +27,6 @@
#include <asm/cpu_ops.h>
#include <asm/errno.h>
#include <asm/smp_plat.h>
-#include <asm/suspend.h>
-
-static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
-
-static int __maybe_unused cpu_psci_cpu_init_idle(unsigned int cpu)
-{
- int i, ret, count = 0;
- u32 *psci_states;
- struct device_node *state_node, *cpu_node;
-
- cpu_node = of_get_cpu_node(cpu, NULL);
- if (!cpu_node)
- return -ENODEV;
-
- /*
- * If the PSCI cpu_suspend function hook has not been initialized
- * idle states must not be enabled, so bail out
- */
- if (!psci_ops.cpu_suspend)
- return -EOPNOTSUPP;
-
- /* Count idle states */
- while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
- count))) {
- count++;
- of_node_put(state_node);
- }
-
- if (!count)
- return -ENODEV;
-
- psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
- if (!psci_states)
- return -ENOMEM;
-
- for (i = 0; i < count; i++) {
- u32 state;
-
- state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
-
- ret = of_property_read_u32(state_node,
- "arm,psci-suspend-param",
- &state);
- if (ret) {
- pr_warn(" * %s missing arm,psci-suspend-param property\n",
- state_node->full_name);
- of_node_put(state_node);
- goto free_mem;
- }
-
- of_node_put(state_node);
- pr_debug("psci-power-state %#x index %d\n", state, i);
- if (!psci_power_state_is_valid(state)) {
- pr_warn("Invalid PSCI power state %#x\n", state);
- ret = -EINVAL;
- goto free_mem;
- }
- psci_states[i] = state;
- }
- /* Idle states parsed correctly, initialize per-cpu pointer */
- per_cpu(psci_power_state, cpu) = psci_states;
- return 0;
-
-free_mem:
- kfree(psci_states);
- return ret;
-}

#ifdef CONFIG_SMP

@@ -181,33 +113,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
#endif
#endif

-static int psci_suspend_finisher(unsigned long index)
-{
- u32 *state = __this_cpu_read(psci_power_state);
-
- return psci_ops.cpu_suspend(state[index - 1],
- virt_to_phys(cpu_resume));
-}
-
-static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index)
-{
- int ret;
- u32 *state = __this_cpu_read(psci_power_state);
- /*
- * idle state index 0 corresponds to wfi, should never be called
- * from the cpu_suspend operations
- */
- if (WARN_ON_ONCE(!index))
- return -EINVAL;
-
- if (!psci_power_state_loses_context(state[index - 1]))
- ret = psci_ops.cpu_suspend(state[index - 1], 0);
- else
- ret = cpu_suspend(index, psci_suspend_finisher);
-
- return ret;
-}
-
const struct cpu_operations cpu_psci_ops = {
.name = "psci",
#ifdef CONFIG_CPU_IDLE
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 5b544d7..8bcae4c 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -20,12 +20,14 @@
#include <linux/printk.h>
#include <linux/psci.h>
#include <linux/reboot.h>
+#include <linux/slab.h>

#include <uapi/linux/psci.h>

#include <asm/cputype.h>
#include <asm/system_misc.h>
#include <asm/smp_plat.h>
+#include <asm/suspend.h>

/*
* While a 64-bit OS can make calls with SMC32 calling conventions, for some
@@ -81,13 +83,15 @@ static u32 psci_function_id[PSCI_FN_MAX];

static u32 psci_cpu_suspend_feature;

+static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
+
static inline bool psci_has_ext_power_state(void)
{
return psci_cpu_suspend_feature &
PSCI_1_0_FEATURES_CPU_SUSPEND_PF_MASK;
}

-bool psci_power_state_loses_context(u32 state)
+static bool psci_power_state_loses_context(u32 state)
{
const u32 mask = psci_has_ext_power_state() ?
PSCI_1_0_EXT_POWER_STATE_TYPE_MASK :
@@ -96,7 +100,7 @@ bool psci_power_state_loses_context(u32 state)
return state & mask;
}

-bool psci_power_state_is_valid(u32 state)
+static bool psci_power_state_is_valid(u32 state)
{
const u32 valid_mask = psci_has_ext_power_state() ?
PSCI_1_0_EXT_POWER_STATE_MASK :
@@ -217,6 +221,97 @@ static void psci_sys_poweroff(void)
invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
}

+int cpu_psci_cpu_init_idle(unsigned int cpu)
+{
+ int i, ret, count = 0;
+ u32 *psci_states;
+ struct device_node *state_node, *cpu_node;
+
+ cpu_node = of_get_cpu_node(cpu, NULL);
+ if (!cpu_node)
+ return -ENODEV;
+
+ /*
+ * If the PSCI cpu_suspend function hook has not been initialized
+ * idle states must not be enabled, so bail out
+ */
+ if (!psci_ops.cpu_suspend)
+ return -EOPNOTSUPP;
+
+ /* Count idle states */
+ while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
+ count))) {
+ count++;
+ of_node_put(state_node);
+ }
+
+ if (!count)
+ return -ENODEV;
+
+ psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+ if (!psci_states)
+ return -ENOMEM;
+
+ for (i = 0; i < count; i++) {
+ u32 state;
+
+ state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+
+ ret = of_property_read_u32(state_node,
+ "arm,psci-suspend-param",
+ &state);
+ if (ret) {
+ pr_warn(" * %s missing arm,psci-suspend-param property\n",
+ state_node->full_name);
+ of_node_put(state_node);
+ goto free_mem;
+ }
+
+ of_node_put(state_node);
+ pr_debug("psci-power-state %#x index %d\n", state, i);
+ if (!psci_power_state_is_valid(state)) {
+ pr_warn("Invalid PSCI power state %#x\n", state);
+ ret = -EINVAL;
+ goto free_mem;
+ }
+ psci_states[i] = state;
+ }
+ /* Idle states parsed correctly, initialize per-cpu pointer */
+ per_cpu(psci_power_state, cpu) = psci_states;
+ return 0;
+
+free_mem:
+ kfree(psci_states);
+ return ret;
+}
+
+static int psci_suspend_finisher(unsigned long index)
+{
+ u32 *state = __this_cpu_read(psci_power_state);
+
+ return psci_ops.cpu_suspend(state[index - 1],
+ virt_to_phys(cpu_resume));
+}
+
+int cpu_psci_cpu_suspend(unsigned long index)
+{
+ int ret;
+ u32 *state = __this_cpu_read(psci_power_state);
+ /*
+ * idle state index 0 corresponds to wfi, should never be called
+ * from the cpu_suspend operations
+ */
+ if (WARN_ON_ONCE(!index))
+ return -EINVAL;
+
+ if (!psci_power_state_loses_context(state[index - 1]))
+ ret = psci_ops.cpu_suspend(state[index - 1], 0);
+ else
+ ret = cpu_suspend(index, psci_suspend_finisher);
+
+ return ret;
+}
+
static int __init psci_features(u32 psci_func_id)
{
return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
diff --git a/include/linux/psci.h b/include/linux/psci.h
index 12c4865..9fa8629 100644
--- a/include/linux/psci.h
+++ b/include/linux/psci.h
@@ -21,8 +21,8 @@
#define PSCI_POWER_STATE_TYPE_POWER_DOWN 1

bool psci_tos_resident_on(int cpu);
-bool psci_power_state_loses_context(u32 state);
-bool psci_power_state_is_valid(u32 state);
+int cpu_psci_cpu_init_idle(unsigned int cpu);
+int cpu_psci_cpu_suspend(unsigned long index);

struct psci_operations {
int (*cpu_suspend)(u32 state, unsigned long entry_point);
--
2.1.4

2015-07-09 08:32:35

[permalink] [raw]

Subject: [PATCH v3 2/3] ARM: cpuidle: refine cpuidle_ops member's parameters.

As for the suspend member function, the to-be-suspended cpu is always
the calling cpu itself, so the 'cpu' parameter may not be necessary, let
driver get 'cpu' itself if need.

As for the init member function, the device_node here may not be
necessary either, because we can get the node via. of_get_cpu_node().

Signed-off-by: Jisheng Zhang <[email protected]>
---
arch/arm/include/asm/cpuidle.h | 6 +++---
arch/arm/kernel/cpuidle.c | 8 ++++----
drivers/soc/qcom/spm.c | 17 ++++++++++++-----
3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
index 0f84249..ca5dfe6 100644
--- a/arch/arm/include/asm/cpuidle.h
+++ b/arch/arm/include/asm/cpuidle.h
@@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
struct device_node;

struct cpuidle_ops {
- int (*suspend)(int cpu, unsigned long arg);
- int (*init)(struct device_node *, int cpu);
+ int (*suspend)(unsigned long arg);
+ int (*init)(unsigned int cpu);
};

struct of_cpuidle_method {
@@ -46,6 +46,6 @@ struct of_cpuidle_method {

extern int arm_cpuidle_suspend(int index);

-extern int arm_cpuidle_init(int cpu);
+extern int arm_cpuidle_init(unsigned int cpu);

#endif
diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
index 318da33..7bc7bc6 100644
--- a/arch/arm/kernel/cpuidle.c
+++ b/arch/arm/kernel/cpuidle.c
@@ -53,10 +53,10 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
int arm_cpuidle_suspend(int index)
{
int ret = -EOPNOTSUPP;
- int cpu = smp_processor_id();
+ unsigned int cpu = smp_processor_id();

if (cpuidle_ops[cpu].suspend)
- ret = cpuidle_ops[cpu].suspend(cpu, index);
+ ret = cpuidle_ops[cpu].suspend(index);

return ret;
}
@@ -134,7 +134,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
* -ENXIO if the HW reports a failure or a misconfiguration,
* -ENOMEM if the HW report an memory allocation failure
*/
-int __init arm_cpuidle_init(int cpu)
+int __init arm_cpuidle_init(unsigned int cpu)
{
struct device_node *cpu_node = of_cpu_device_node_get(cpu);
int ret;
@@ -144,7 +144,7 @@ int __init arm_cpuidle_init(int cpu)

ret = arm_cpuidle_read_ops(cpu_node, cpu);
if (!ret && cpuidle_ops[cpu].init)
- ret = cpuidle_ops[cpu].init(cpu_node, cpu);
+ ret = cpuidle_ops[cpu].init(cpu);

of_node_put(cpu_node);

diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
index b04b05a..1649ec3 100644
--- a/drivers/soc/qcom/spm.c
+++ b/drivers/soc/qcom/spm.c
@@ -116,7 +116,7 @@ static const struct spm_reg_data spm_reg_8064_cpu = {

static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);

-typedef int (*idle_fn)(int);
+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,
@@ -179,9 +179,10 @@ static int qcom_pm_collapse(unsigned long int unused)
return -1;
}

-static int qcom_cpu_spc(int cpu)
+static int qcom_cpu_spc(void)
{
int ret;
+ int cpu = smp_processor_id();
struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);

spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
@@ -197,9 +198,11 @@ static int qcom_cpu_spc(int cpu)
return ret;
}

-static int qcom_idle_enter(int cpu, unsigned long index)
+static int qcom_idle_enter(unsigned long index)
{
- return per_cpu(qcom_idle_ops, cpu)[index](cpu);
+ unsigned int cpu = smp_processor_id();
+
+ return per_cpu(qcom_idle_ops, cpu)[index]();
}

static const struct of_device_id qcom_idle_state_match[] __initconst = {
@@ -207,7 +210,7 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
{ },
};

-static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
+static int __init qcom_cpuidle_init(unsigned int cpu)
{
const struct of_device_id *match_id;
struct device_node *state_node;
@@ -217,6 +220,10 @@ static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
idle_fn *fns;
cpumask_t mask;
bool use_scm_power_down = false;
+ struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
+
+ if (!cpu_node)
+ return -ENODEV;

for (i = 0; ; i++) {
state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
--
2.1.4

2015-07-09 08:32:27

[permalink] [raw]

Subject: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

This patch implements cpuidle_ops using psci. After this patch, we can
use cpuidle-arm.c with psci backend for both arm and arm64.

Signed-off-by: Jisheng Zhang <[email protected]>
---
arch/arm/kernel/Makefile | 1 +
arch/arm/kernel/psci_cpuidle.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)
create mode 100644 arch/arm/kernel/psci_cpuidle.c

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 3b995f5..96383d8 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_ARM_VIRT_EXT) += hyp-stub.o
ifeq ($(CONFIG_ARM_PSCI),y)
obj-y += psci-call.o
obj-$(CONFIG_SMP) += psci_smp.o
+obj-$(CONFIG_CPU_IDLE) += psci_cpuidle.o
endif

extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/psci_cpuidle.c b/arch/arm/kernel/psci_cpuidle.c
new file mode 100644
index 0000000..e7146d2
--- /dev/null
+++ b/arch/arm/kernel/psci_cpuidle.c
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ * Author: Jisheng Zhang <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/psci.h>
+
+#include <asm/cpuidle.h>
+
+static struct cpuidle_ops psci_cpuidle_ops __initdata = {
+ .suspend = cpu_psci_cpu_suspend,
+ .init = cpu_psci_cpu_init_idle,
+};
+CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
--
2.1.4

2015-07-09 08:43:51

[permalink] [raw]

Subject: Re: [PATCH v3 2/3] ARM: cpuidle: refine cpuidle_ops member's parameters.

On Thu, 9 Jul 2015 16:31:24 +0800
Jisheng Zhang <[email protected]> wrote:

> As for the suspend member function, the to-be-suspended cpu is always
> the calling cpu itself, so the 'cpu' parameter may not be necessary, let
> driver get 'cpu' itself if need.
>
> As for the init member function, the device_node here may not be
> necessary either, because we can get the node via. of_get_cpu_node().

This patch also changes drivers/soc/qcom/spm.c accordingly, but I have no
qcom platform, so I can't test it, just make sure it can pass kernel building.
Could anyone kindly help me to test?

Thanks a lot,
Jisheng

>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> arch/arm/include/asm/cpuidle.h | 6 +++---
> arch/arm/kernel/cpuidle.c | 8 ++++----
> drivers/soc/qcom/spm.c | 17 ++++++++++++-----
> 3 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> index 0f84249..ca5dfe6 100644
> --- a/arch/arm/include/asm/cpuidle.h
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> struct device_node;
>
> struct cpuidle_ops {
> - int (*suspend)(int cpu, unsigned long arg);
> - int (*init)(struct device_node *, int cpu);
> + int (*suspend)(unsigned long arg);
> + int (*init)(unsigned int cpu);
> };
>
> struct of_cpuidle_method {
> @@ -46,6 +46,6 @@ struct of_cpuidle_method {
>
> extern int arm_cpuidle_suspend(int index);
>
> -extern int arm_cpuidle_init(int cpu);
> +extern int arm_cpuidle_init(unsigned int cpu);
>
> #endif
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index 318da33..7bc7bc6 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -53,10 +53,10 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> int arm_cpuidle_suspend(int index)
> {
> int ret = -EOPNOTSUPP;
> - int cpu = smp_processor_id();
> + unsigned int cpu = smp_processor_id();
>
> if (cpuidle_ops[cpu].suspend)
> - ret = cpuidle_ops[cpu].suspend(cpu, index);
> + ret = cpuidle_ops[cpu].suspend(index);
>
> return ret;
> }
> @@ -134,7 +134,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
> * -ENXIO if the HW reports a failure or a misconfiguration,
> * -ENOMEM if the HW report an memory allocation failure
> */
> -int __init arm_cpuidle_init(int cpu)
> +int __init arm_cpuidle_init(unsigned int cpu)
> {
> struct device_node *cpu_node = of_cpu_device_node_get(cpu);
> int ret;
> @@ -144,7 +144,7 @@ int __init arm_cpuidle_init(int cpu)
>
> ret = arm_cpuidle_read_ops(cpu_node, cpu);
> if (!ret && cpuidle_ops[cpu].init)
> - ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> + ret = cpuidle_ops[cpu].init(cpu);
>
> of_node_put(cpu_node);
>
> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> index b04b05a..1649ec3 100644
> --- a/drivers/soc/qcom/spm.c
> +++ b/drivers/soc/qcom/spm.c
> @@ -116,7 +116,7 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
>
> static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
>
> -typedef int (*idle_fn)(int);
> +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,
> @@ -179,9 +179,10 @@ static int qcom_pm_collapse(unsigned long int unused)
> return -1;
> }
>
> -static int qcom_cpu_spc(int cpu)
> +static int qcom_cpu_spc(void)
> {
> int ret;
> + int cpu = smp_processor_id();
> struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);
>
> spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
> @@ -197,9 +198,11 @@ static int qcom_cpu_spc(int cpu)
> return ret;
> }
>
> -static int qcom_idle_enter(int cpu, unsigned long index)
> +static int qcom_idle_enter(unsigned long index)
> {
> - return per_cpu(qcom_idle_ops, cpu)[index](cpu);
> + unsigned int cpu = smp_processor_id();
> +
> + return per_cpu(qcom_idle_ops, cpu)[index]();
> }
>
> static const struct of_device_id qcom_idle_state_match[] __initconst = {
> @@ -207,7 +210,7 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
> { },
> };
>
> -static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
> +static int __init qcom_cpuidle_init(unsigned int cpu)
> {
> const struct of_device_id *match_id;
> struct device_node *state_node;
> @@ -217,6 +220,10 @@ static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
> idle_fn *fns;
> cpumask_t mask;
> bool use_scm_power_down = false;
> + struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
> +
> + if (!cpu_node)
> + return -ENODEV;
>
> for (i = 0; ; i++) {
> state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);

2015-07-09 09:28:24

[permalink] [raw]

Subject: Re: [PATCH v3 2/3] ARM: cpuidle: refine cpuidle_ops member's parameters.

On Thu, Jul 09, 2015 at 09:43:04AM +0100, Jisheng Zhang wrote:
> On Thu, 9 Jul 2015 16:31:24 +0800
> Jisheng Zhang <[email protected]> wrote:
>
> > As for the suspend member function, the to-be-suspended cpu is always
> > the calling cpu itself, so the 'cpu' parameter may not be necessary, let
> > driver get 'cpu' itself if need.
> >
> > As for the init member function, the device_node here may not be
> > necessary either, because we can get the node via. of_get_cpu_node().
>
> This patch also changes drivers/soc/qcom/spm.c accordingly, but I have no
> qcom platform, so I can't test it, just make sure it can pass kernel building.
> Could anyone kindly help me to test?

CC'ing Lina who should be able to help you test it.

Thanks,
Lorenzo

> Thanks a lot,
> Jisheng
>
> >
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > arch/arm/include/asm/cpuidle.h | 6 +++---
> > arch/arm/kernel/cpuidle.c | 8 ++++----
> > drivers/soc/qcom/spm.c | 17 ++++++++++++-----
> > 3 files changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> > index 0f84249..ca5dfe6 100644
> > --- a/arch/arm/include/asm/cpuidle.h
> > +++ b/arch/arm/include/asm/cpuidle.h
> > @@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> > struct device_node;
> >
> > struct cpuidle_ops {
> > - int (*suspend)(int cpu, unsigned long arg);
> > - int (*init)(struct device_node *, int cpu);
> > + int (*suspend)(unsigned long arg);
> > + int (*init)(unsigned int cpu);
> > };
> >
> > struct of_cpuidle_method {
> > @@ -46,6 +46,6 @@ struct of_cpuidle_method {
> >
> > extern int arm_cpuidle_suspend(int index);
> >
> > -extern int arm_cpuidle_init(int cpu);
> > +extern int arm_cpuidle_init(unsigned int cpu);
> >
> > #endif
> > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> > index 318da33..7bc7bc6 100644
> > --- a/arch/arm/kernel/cpuidle.c
> > +++ b/arch/arm/kernel/cpuidle.c
> > @@ -53,10 +53,10 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> > int arm_cpuidle_suspend(int index)
> > {
> > int ret = -EOPNOTSUPP;
> > - int cpu = smp_processor_id();
> > + unsigned int cpu = smp_processor_id();
> >
> > if (cpuidle_ops[cpu].suspend)
> > - ret = cpuidle_ops[cpu].suspend(cpu, index);
> > + ret = cpuidle_ops[cpu].suspend(index);
> >
> > return ret;
> > }
> > @@ -134,7 +134,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
> > * -ENXIO if the HW reports a failure or a misconfiguration,
> > * -ENOMEM if the HW report an memory allocation failure
> > */
> > -int __init arm_cpuidle_init(int cpu)
> > +int __init arm_cpuidle_init(unsigned int cpu)
> > {
> > struct device_node *cpu_node = of_cpu_device_node_get(cpu);
> > int ret;
> > @@ -144,7 +144,7 @@ int __init arm_cpuidle_init(int cpu)
> >
> > ret = arm_cpuidle_read_ops(cpu_node, cpu);
> > if (!ret && cpuidle_ops[cpu].init)
> > - ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> > + ret = cpuidle_ops[cpu].init(cpu);
> >
> > of_node_put(cpu_node);
> >
> > diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> > index b04b05a..1649ec3 100644
> > --- a/drivers/soc/qcom/spm.c
> > +++ b/drivers/soc/qcom/spm.c
> > @@ -116,7 +116,7 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
> >
> > static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
> >
> > -typedef int (*idle_fn)(int);
> > +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,
> > @@ -179,9 +179,10 @@ static int qcom_pm_collapse(unsigned long int unused)
> > return -1;
> > }
> >
> > -static int qcom_cpu_spc(int cpu)
> > +static int qcom_cpu_spc(void)
> > {
> > int ret;
> > + int cpu = smp_processor_id();
> > struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);
> >
> > spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
> > @@ -197,9 +198,11 @@ static int qcom_cpu_spc(int cpu)
> > return ret;
> > }
> >
> > -static int qcom_idle_enter(int cpu, unsigned long index)
> > +static int qcom_idle_enter(unsigned long index)
> > {
> > - return per_cpu(qcom_idle_ops, cpu)[index](cpu);
> > + unsigned int cpu = smp_processor_id();
> > +
> > + return per_cpu(qcom_idle_ops, cpu)[index]();
> > }
> >
> > static const struct of_device_id qcom_idle_state_match[] __initconst = {
> > @@ -207,7 +210,7 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
> > { },
> > };
> >
> > -static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
> > +static int __init qcom_cpuidle_init(unsigned int cpu)
> > {
> > const struct of_device_id *match_id;
> > struct device_node *state_node;
> > @@ -217,6 +220,10 @@ static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
> > idle_fn *fns;
> > cpumask_t mask;
> > bool use_scm_power_down = false;
> > + struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
> > +
> > + if (!cpu_node)
> > + return -ENODEV;
> >
> > for (i = 0; ; i++) {
> > state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>

2015-07-10 15:07:50

by Lina Iyer

[permalink] [raw]

Subject: Re: [PATCH v3 2/3] ARM: cpuidle: refine cpuidle_ops member's parameters.

On Thu, Jul 09 2015 at 03:28 -0600, Lorenzo Pieralisi wrote:
>On Thu, Jul 09, 2015 at 09:43:04AM +0100, Jisheng Zhang wrote:
>> On Thu, 9 Jul 2015 16:31:24 +0800
>> Jisheng Zhang <[email protected]> wrote:
>>
>> > As for the suspend member function, the to-be-suspended cpu is always
>> > the calling cpu itself, so the 'cpu' parameter may not be necessary, let
>> > driver get 'cpu' itself if need.
>> >
>> > As for the init member function, the device_node here may not be
>> > necessary either, because we can get the node via. of_get_cpu_node().
>>
>> This patch also changes drivers/soc/qcom/spm.c accordingly, but I have no
>> qcom platform, so I can't test it, just make sure it can pass kernel building.
>> Could anyone kindly help me to test?
>
>CC'ing Lina who should be able to help you test it.
>
Sure, will test and let you know.

-- Lina

>Thanks,
>Lorenzo
>
>> Thanks a lot,
>> Jisheng
>>
>> >
>> > Signed-off-by: Jisheng Zhang <[email protected]>
>> > ---
>> > arch/arm/include/asm/cpuidle.h | 6 +++---
>> > arch/arm/kernel/cpuidle.c | 8 ++++----
>> > drivers/soc/qcom/spm.c | 17 ++++++++++++-----
>> > 3 files changed, 19 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
>> > index 0f84249..ca5dfe6 100644
>> > --- a/arch/arm/include/asm/cpuidle.h
>> > +++ b/arch/arm/include/asm/cpuidle.h
>> > @@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>> > struct device_node;
>> >
>> > struct cpuidle_ops {
>> > - int (*suspend)(int cpu, unsigned long arg);
>> > - int (*init)(struct device_node *, int cpu);
>> > + int (*suspend)(unsigned long arg);
>> > + int (*init)(unsigned int cpu);
>> > };
>> >
>> > struct of_cpuidle_method {
>> > @@ -46,6 +46,6 @@ struct of_cpuidle_method {
>> >
>> > extern int arm_cpuidle_suspend(int index);
>> >
>> > -extern int arm_cpuidle_init(int cpu);
>> > +extern int arm_cpuidle_init(unsigned int cpu);
>> >
>> > #endif
>> > diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
>> > index 318da33..7bc7bc6 100644
>> > --- a/arch/arm/kernel/cpuidle.c
>> > +++ b/arch/arm/kernel/cpuidle.c
>> > @@ -53,10 +53,10 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>> > int arm_cpuidle_suspend(int index)
>> > {
>> > int ret = -EOPNOTSUPP;
>> > - int cpu = smp_processor_id();
>> > + unsigned int cpu = smp_processor_id();
>> >
>> > if (cpuidle_ops[cpu].suspend)
>> > - ret = cpuidle_ops[cpu].suspend(cpu, index);
>> > + ret = cpuidle_ops[cpu].suspend(index);
>> >
>> > return ret;
>> > }
>> > @@ -134,7 +134,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
>> > * -ENXIO if the HW reports a failure or a misconfiguration,
>> > * -ENOMEM if the HW report an memory allocation failure
>> > */
>> > -int __init arm_cpuidle_init(int cpu)
>> > +int __init arm_cpuidle_init(unsigned int cpu)
>> > {
>> > struct device_node *cpu_node = of_cpu_device_node_get(cpu);
>> > int ret;
>> > @@ -144,7 +144,7 @@ int __init arm_cpuidle_init(int cpu)
>> >
>> > ret = arm_cpuidle_read_ops(cpu_node, cpu);
>> > if (!ret && cpuidle_ops[cpu].init)
>> > - ret = cpuidle_ops[cpu].init(cpu_node, cpu);
>> > + ret = cpuidle_ops[cpu].init(cpu);
>> >
>> > of_node_put(cpu_node);
>> >
>> > diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
>> > index b04b05a..1649ec3 100644
>> > --- a/drivers/soc/qcom/spm.c
>> > +++ b/drivers/soc/qcom/spm.c
>> > @@ -116,7 +116,7 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
>> >
>> > static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
>> >
>> > -typedef int (*idle_fn)(int);
>> > +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,
>> > @@ -179,9 +179,10 @@ static int qcom_pm_collapse(unsigned long int unused)
>> > return -1;
>> > }
>> >
>> > -static int qcom_cpu_spc(int cpu)
>> > +static int qcom_cpu_spc(void)
>> > {
>> > int ret;
>> > + int cpu = smp_processor_id();
>> > struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);
>> >
>> > spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
>> > @@ -197,9 +198,11 @@ static int qcom_cpu_spc(int cpu)
>> > return ret;
>> > }
>> >
>> > -static int qcom_idle_enter(int cpu, unsigned long index)
>> > +static int qcom_idle_enter(unsigned long index)
>> > {
>> > - return per_cpu(qcom_idle_ops, cpu)[index](cpu);
>> > + unsigned int cpu = smp_processor_id();
>> > +
>> > + return per_cpu(qcom_idle_ops, cpu)[index]();
>> > }
>> >
>> > static const struct of_device_id qcom_idle_state_match[] __initconst = {
>> > @@ -207,7 +210,7 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
>> > { },
>> > };
>> >
>> > -static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
>> > +static int __init qcom_cpuidle_init(unsigned int cpu)
>> > {
>> > const struct of_device_id *match_id;
>> > struct device_node *state_node;
>> > @@ -217,6 +220,10 @@ static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
>> > idle_fn *fns;
>> > cpumask_t mask;
>> > bool use_scm_power_down = false;
>> > + struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
>> > +
>> > + if (!cpu_node)
>> > + return -ENODEV;
>> >
>> > for (i = 0; ; i++) {
>> > state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>>

2015-07-10 17:37:27

by Lina Iyer

[permalink] [raw]

Subject: Re: [PATCH v3 2/3] ARM: cpuidle: refine cpuidle_ops member's parameters.

On Thu, Jul 09 2015 at 02:33 -0600, Jisheng Zhang wrote:
>As for the suspend member function, the to-be-suspended cpu is always
>the calling cpu itself, so the 'cpu' parameter may not be necessary, let
>driver get 'cpu' itself if need.
>
>As for the init member function, the device_node here may not be
>necessary either, because we can get the node via. of_get_cpu_node().
>
>Signed-off-by: Jisheng Zhang <[email protected]>

Tested-by: Lina Iyer <[email protected]>

>---
> arch/arm/include/asm/cpuidle.h | 6 +++---
> arch/arm/kernel/cpuidle.c | 8 ++++----
> drivers/soc/qcom/spm.c | 17 ++++++++++++-----
> 3 files changed, 19 insertions(+), 12 deletions(-)
>
>diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
>index 0f84249..ca5dfe6 100644
>--- a/arch/arm/include/asm/cpuidle.h
>+++ b/arch/arm/include/asm/cpuidle.h
>@@ -30,8 +30,8 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> struct device_node;
>
> struct cpuidle_ops {
>- int (*suspend)(int cpu, unsigned long arg);
>- int (*init)(struct device_node *, int cpu);
>+ int (*suspend)(unsigned long arg);
>+ int (*init)(unsigned int cpu);
> };
>
> struct of_cpuidle_method {
>@@ -46,6 +46,6 @@ struct of_cpuidle_method {
>
> extern int arm_cpuidle_suspend(int index);
>
>-extern int arm_cpuidle_init(int cpu);
>+extern int arm_cpuidle_init(unsigned int cpu);
>
> #endif
>diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
>index 318da33..7bc7bc6 100644
>--- a/arch/arm/kernel/cpuidle.c
>+++ b/arch/arm/kernel/cpuidle.c
>@@ -53,10 +53,10 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> int arm_cpuidle_suspend(int index)
> {
> int ret = -EOPNOTSUPP;
>- int cpu = smp_processor_id();
>+ unsigned int cpu = smp_processor_id();
>
> if (cpuidle_ops[cpu].suspend)
>- ret = cpuidle_ops[cpu].suspend(cpu, index);
>+ ret = cpuidle_ops[cpu].suspend(index);
>
> return ret;
> }
>@@ -134,7 +134,7 @@ static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
> * -ENXIO if the HW reports a failure or a misconfiguration,
> * -ENOMEM if the HW report an memory allocation failure
> */
>-int __init arm_cpuidle_init(int cpu)
>+int __init arm_cpuidle_init(unsigned int cpu)
> {
> struct device_node *cpu_node = of_cpu_device_node_get(cpu);
> int ret;
>@@ -144,7 +144,7 @@ int __init arm_cpuidle_init(int cpu)
>
> ret = arm_cpuidle_read_ops(cpu_node, cpu);
> if (!ret && cpuidle_ops[cpu].init)
>- ret = cpuidle_ops[cpu].init(cpu_node, cpu);
>+ ret = cpuidle_ops[cpu].init(cpu);
>
> of_node_put(cpu_node);
>
>diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
>index b04b05a..1649ec3 100644
>--- a/drivers/soc/qcom/spm.c
>+++ b/drivers/soc/qcom/spm.c
>@@ -116,7 +116,7 @@ static const struct spm_reg_data spm_reg_8064_cpu = {
>
> static DEFINE_PER_CPU(struct spm_driver_data *, cpu_spm_drv);
>
>-typedef int (*idle_fn)(int);
>+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,
>@@ -179,9 +179,10 @@ static int qcom_pm_collapse(unsigned long int unused)
> return -1;
> }
>
>-static int qcom_cpu_spc(int cpu)
>+static int qcom_cpu_spc(void)
> {
> int ret;
>+ int cpu = smp_processor_id();
> struct spm_driver_data *drv = per_cpu(cpu_spm_drv, cpu);
>
> spm_set_low_power_mode(drv, PM_SLEEP_MODE_SPC);
>@@ -197,9 +198,11 @@ static int qcom_cpu_spc(int cpu)
> return ret;
> }
>
>-static int qcom_idle_enter(int cpu, unsigned long index)
>+static int qcom_idle_enter(unsigned long index)
> {
>- return per_cpu(qcom_idle_ops, cpu)[index](cpu);
>+ unsigned int cpu = smp_processor_id();
>+
>+ return per_cpu(qcom_idle_ops, cpu)[index]();
> }
>
> static const struct of_device_id qcom_idle_state_match[] __initconst = {
>@@ -207,7 +210,7 @@ static const struct of_device_id qcom_idle_state_match[] __initconst = {
> { },
> };
>
>-static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
>+static int __init qcom_cpuidle_init(unsigned int cpu)
> {
> const struct of_device_id *match_id;
> struct device_node *state_node;
>@@ -217,6 +220,10 @@ static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
> idle_fn *fns;
> cpumask_t mask;
> bool use_scm_power_down = false;
>+ struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
>+
>+ if (!cpu_node)
>+ return -ENODEV;
>
> for (i = 0; ; i++) {
> state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>--
>2.1.4
>
>
>_______________________________________________
>linux-arm-kernel mailing list
>[email protected]
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2015-07-14 10:34:42

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Thu, Jul 09, 2015 at 04:31:25PM +0800, Jisheng Zhang wrote:
> This patch implements cpuidle_ops using psci. After this patch, we can
> use cpuidle-arm.c with psci backend for both arm and arm64.
>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> arch/arm/kernel/Makefile | 1 +
> arch/arm/kernel/psci_cpuidle.c | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+)
> create mode 100644 arch/arm/kernel/psci_cpuidle.c
>
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 3b995f5..96383d8 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -91,6 +91,7 @@ obj-$(CONFIG_ARM_VIRT_EXT) += hyp-stub.o
> ifeq ($(CONFIG_ARM_PSCI),y)
> obj-y += psci-call.o
> obj-$(CONFIG_SMP) += psci_smp.o
> +obj-$(CONFIG_CPU_IDLE) += psci_cpuidle.o
> endif
>
> extra-y := $(head-y) vmlinux.lds
> diff --git a/arch/arm/kernel/psci_cpuidle.c b/arch/arm/kernel/psci_cpuidle.c
> new file mode 100644
> index 0000000..e7146d2
> --- /dev/null
> +++ b/arch/arm/kernel/psci_cpuidle.c
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (C) 2015 Marvell Technology Group Ltd.
> + * Author: Jisheng Zhang <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/cpuidle.h>
> +#include <linux/psci.h>
> +
> +#include <asm/cpuidle.h>
> +
> +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> + .suspend = cpu_psci_cpu_suspend,
> + .init = cpu_psci_cpu_init_idle,
> +};
> +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);

Is there any reason this can't live in the drivers sub-tree? Is there
anything specific to 32-bit ARM about this?

It looks to me like the right thing to do is to have it as part of
drivers/firmware/psci.c.

We don't do this for other stuff - we don't have IRQ_CHIP_OF_DECLARE
stuff in arch/arm but have the IRQ chip drivers in drivers/irqchip.
We keep it all togehter in drivers/irqchip, even when the IRQ chip
driver is only useful on ARM.

--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

2015-07-14 11:02:32

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 11:34:21AM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 09, 2015 at 04:31:25PM +0800, Jisheng Zhang wrote:
> > This patch implements cpuidle_ops using psci. After this patch, we can
> > use cpuidle-arm.c with psci backend for both arm and arm64.
> >
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > arch/arm/kernel/Makefile | 1 +
> > arch/arm/kernel/psci_cpuidle.c | 19 +++++++++++++++++++
> > 2 files changed, 20 insertions(+)
> > create mode 100644 arch/arm/kernel/psci_cpuidle.c
> >
> > diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> > index 3b995f5..96383d8 100644
> > --- a/arch/arm/kernel/Makefile
> > +++ b/arch/arm/kernel/Makefile
> > @@ -91,6 +91,7 @@ obj-$(CONFIG_ARM_VIRT_EXT) += hyp-stub.o
> > ifeq ($(CONFIG_ARM_PSCI),y)
> > obj-y += psci-call.o
> > obj-$(CONFIG_SMP) += psci_smp.o
> > +obj-$(CONFIG_CPU_IDLE) += psci_cpuidle.o
> > endif
> >
> > extra-y := $(head-y) vmlinux.lds
> > diff --git a/arch/arm/kernel/psci_cpuidle.c b/arch/arm/kernel/psci_cpuidle.c
> > new file mode 100644
> > index 0000000..e7146d2
> > --- /dev/null
> > +++ b/arch/arm/kernel/psci_cpuidle.c
> > @@ -0,0 +1,19 @@
> > +/*
> > + * Copyright (C) 2015 Marvell Technology Group Ltd.
> > + * Author: Jisheng Zhang <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/cpuidle.h>
> > +#include <linux/psci.h>
> > +
> > +#include <asm/cpuidle.h>
> > +
> > +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> > + .suspend = cpu_psci_cpu_suspend,
> > + .init = cpu_psci_cpu_init_idle,
> > +};
> > +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
>
> Is there any reason this can't live in the drivers sub-tree? Is there
> anything specific to 32-bit ARM about this?
>
> It looks to me like the right thing to do is to have it as part of
> drivers/firmware/psci.c.

I take this as an ACK to M.Rutland's PSCI code move to drivers/firmware,
right ?

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355643.html

> We don't do this for other stuff - we don't have IRQ_CHIP_OF_DECLARE
> stuff in arch/arm but have the IRQ chip drivers in drivers/irqchip.
> We keep it all togehter in drivers/irqchip, even when the IRQ chip
> driver is only useful on ARM.

CPUidle operations are ARM only, they are not used on ARM64, so
they belong in arch/arm (that's the same thing as SMP ops, on ARM64
SMP ops and CPUidle ops are unified through CPU operations).

Thanks,
Lorenzo

2015-07-14 12:29:32

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 12:03:02PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Jul 14, 2015 at 11:34:21AM +0100, Russell King - ARM Linux wrote:
> > > +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> > > + .suspend = cpu_psci_cpu_suspend,
> > > + .init = cpu_psci_cpu_init_idle,
> > > +};
> > > +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
>
> I take this as an ACK to M.Rutland's PSCI code move to drivers/firmware,
> right ?

No, that's not something I've particularly looked at. PSCI doesn't really
interest me because I have no systems which (afaik) support it.

> > We don't do this for other stuff - we don't have IRQ_CHIP_OF_DECLARE
> > stuff in arch/arm but have the IRQ chip drivers in drivers/irqchip.
> > We keep it all togehter in drivers/irqchip, even when the IRQ chip
> > driver is only useful on ARM.
>
> CPUidle operations are ARM only, they are not used on ARM64, so
> they belong in arch/arm (that's the same thing as SMP ops, on ARM64
> SMP ops and CPUidle ops are unified through CPU operations).

I don't agree with that. CPU idle isn't an "ARM thing" at all, it's
generic kernel infrastructure. OF is generic kernel infrastructure too.

Yet, we're stuffing _all_ the PSCI CPU idle code into
drivers/firmware/psci.c, but then stuffing the PSCI OF data structures
into arch/arm. This is utterly _insane_.

There is nothing ARM specific about these CPU idle ops. They don't
belong on arch/arm.

NAK on this series (and the move of the PSCI code to drivers/firmware)
until people start seeing sense with stuff like this. Having stuff split
between arch/arm/ and drivers/ like this is totally crap. It makes code
unnecessary complex for no reason what so ever.

Find a solution which doesn't involve leaving _just_ data structures to
connect stuff to OF in arch/arm.

--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

2015-07-14 14:55:17

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 01:29:04PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 14, 2015 at 12:03:02PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Jul 14, 2015 at 11:34:21AM +0100, Russell King - ARM Linux wrote:
> > > > +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> > > > + .suspend = cpu_psci_cpu_suspend,
> > > > + .init = cpu_psci_cpu_init_idle,
> > > > +};
> > > > +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
> >
> > I take this as an ACK to M.Rutland's PSCI code move to drivers/firmware,
> > right ?
>
> No, that's not something I've particularly looked at. PSCI doesn't really
> interest me because I have no systems which (afaik) support it.

PSCI implementation is currently part of arch/arm which means it is
used on arch/arm (and related platforms) and you should be interested in it
for that specific reason.

It is not about what you are interested on, it is about the code you
maintain, so please have a look at it, thank you.

> > > We don't do this for other stuff - we don't have IRQ_CHIP_OF_DECLARE
> > > stuff in arch/arm but have the IRQ chip drivers in drivers/irqchip.
> > > We keep it all togehter in drivers/irqchip, even when the IRQ chip
> > > driver is only useful on ARM.
> >
> > CPUidle operations are ARM only, they are not used on ARM64, so
> > they belong in arch/arm (that's the same thing as SMP ops, on ARM64
> > SMP ops and CPUidle ops are unified through CPU operations).
>
> I don't agree with that. CPU idle isn't an "ARM thing" at all, it's
> generic kernel infrastructure. OF is generic kernel infrastructure too.

I said "CPUidle operations", not CPUidle, we know CPUidle is not an
ARM thing.

> Yet, we're stuffing _all_ the PSCI CPU idle code into
> drivers/firmware/psci.c, but then stuffing the PSCI OF data structures
> into arch/arm. This is utterly _insane_.

Ok, so we will copy the ARM64 PSCI idle related code to arch/arm
and we are done with this, or we will have to ifdef drivers/firmware
code, take your pick.

> There is nothing ARM specific about these CPU idle ops. They don't
> belong on arch/arm.

See above.

> NAK on this series (and the move of the PSCI code to drivers/firmware)

I do not accept that. You may NAK this series but you can't object to
moving PSCI firmware code to drivers/firmware for that reason, so
please have a look at Mark's patches again and ACK/NAK them for
a valid reason, it has been a while since he asked.

> until people start seeing sense with stuff like this. Having stuff split
> between arch/arm/ and drivers/ like this is totally crap. It makes code
> unnecessary complex for no reason what so ever.

That's rather vague and just your opinion. If you have a solution better
than this one you tell us about it otherwise you keep your comments for
yourself, thank you.

> Find a solution which doesn't involve leaving _just_ data structures to
> connect stuff to OF in arch/arm.

I will copy the PSCI CPUidle related functions from arm64 to arch/arm so that
it is not _just_ OF data structures and I hope we are done with this.

Lorenzo

2015-07-14 20:42:04

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 03:55:46PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Jul 14, 2015 at 01:29:04PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jul 14, 2015 at 12:03:02PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Jul 14, 2015 at 11:34:21AM +0100, Russell King - ARM Linux wrote:
> > > > > +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> > > > > + .suspend = cpu_psci_cpu_suspend,
> > > > > + .init = cpu_psci_cpu_init_idle,
> > > > > +};
> > > > > +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
> > >
> > > I take this as an ACK to M.Rutland's PSCI code move to drivers/firmware,
> > > right ?
> >
> > No, that's not something I've particularly looked at. PSCI doesn't really
> > interest me because I have no systems which (afaik) support it.
>
> PSCI implementation is currently part of arch/arm which means it is
> used on arch/arm (and related platforms) and you should be interested in it
> for that specific reason.
>
> It is not about what you are interested on, it is about the code you
> maintain, so please have a look at it, thank you.

I don't maintain it, ARM Ltd does. ARM Ltd submitted it. ARM Ltd does
the testing of it. I'm only someone who passes patches through when
necessary.

"Maintaining" a chunk of code when you have no way to test it is no way
to do maintanence.

If ARM Ltd want me to maintain it, provide me with the tools and knowledge
to do so.

> > > > We don't do this for other stuff - we don't have IRQ_CHIP_OF_DECLARE
> > > > stuff in arch/arm but have the IRQ chip drivers in drivers/irqchip.
> > > > We keep it all togehter in drivers/irqchip, even when the IRQ chip
> > > > driver is only useful on ARM.
> > >
> > > CPUidle operations are ARM only, they are not used on ARM64, so
> > > they belong in arch/arm (that's the same thing as SMP ops, on ARM64
> > > SMP ops and CPUidle ops are unified through CPU operations).
> >
> > I don't agree with that. CPU idle isn't an "ARM thing" at all, it's
> > generic kernel infrastructure. OF is generic kernel infrastructure too.
>
> I said "CPUidle operations", not CPUidle, we know CPUidle is not an
> ARM thing.
>
> > Yet, we're stuffing _all_ the PSCI CPU idle code into
> > drivers/firmware/psci.c, but then stuffing the PSCI OF data structures
> > into arch/arm. This is utterly _insane_.
>
> Ok, so we will copy the ARM64 PSCI idle related code to arch/arm
> and we are done with this, or we will have to ifdef drivers/firmware
> code, take your pick.

What the fsck is up with you. You're talking utter nonsense.

> > There is nothing ARM specific about these CPU idle ops. They don't
> > belong on arch/arm.
>
> See above.
>
> > NAK on this series (and the move of the PSCI code to drivers/firmware)
>
> I do not accept that. You may NAK this series but you can't object to
> moving PSCI firmware code to drivers/firmware for that reason, so
> please have a look at Mark's patches again and ACK/NAK them for
> a valid reason, it has been a while since he asked.

Sorry, NAK, and end of discussion. There is nothing more to be said
here.

--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

2015-07-15 13:45:31

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 09:41:38PM +0100, Russell King - ARM Linux wrote:

[...]

> > > Yet, we're stuffing _all_ the PSCI CPU idle code into
> > > drivers/firmware/psci.c, but then stuffing the PSCI OF data structures
> > > into arch/arm. This is utterly _insane_.
> >
> > Ok, so we will copy the ARM64 PSCI idle related code to arch/arm
> > and we are done with this, or we will have to ifdef drivers/firmware
> > code, take your pick.
>
> What the fsck is up with you. You're talking utter nonsense.
>
> > > There is nothing ARM specific about these CPU idle ops. They don't
> > > belong on arch/arm.
> >
> > See above.
> >
> > > NAK on this series (and the move of the PSCI code to drivers/firmware)
> >
> > I do not accept that. You may NAK this series but you can't object to
> > moving PSCI firmware code to drivers/firmware for that reason, so
> > please have a look at Mark's patches again and ACK/NAK them for
> > a valid reason, it has been a while since he asked.
>
> Sorry, NAK, and end of discussion. There is nothing more to be said
> here.

I beg to differ. To solve the issue that you brought up with this series,
we can create an arch function that allows to set CPUidle operations, which
would remove the need for the OF construct you did not like, this mirrors
what's done for PSCI smp operations (something similar to smp_set_ops),
does that sound a reasonable approach to you ?

It is not an issue related to CPUidle only, other PSCI functions
(eg psci_cpu_{die/kill} arch/arm/kernel/psci_smp.c) can be shared between
ARM/ARM64 - so moved to drivers/firmware), we would end up with SMP
operations that are initialized with functions that live in
drivers/firmware, if it is done for SMP ops I do not see why it
can't be done for CPUidle operations.

Is the problem related to renaming psci_smp.c to psci.c and adding
CPU_IDLE and SMP ifdeffery in there - as it is done on arm64:

arch/arm64/kernel/psci.c

?

Please let us know, I think we can easily find a way that is acceptable
to you.

As for M.Rutland's series[1], and the patch that moves common PSCI code to
drivers/firmware I reiterate my point, please review it[1] and provide us
with feedback if any otherwise acknowledge the code move, which is the
basis on top of which everything else can be developed, I do not understand
why you are stopping [1] from getting into the kernel since it moves
code from arch/arm to drivers/firmware to have it in a common and shared
place between ARM and ARM64, what's the issue you have with that or put
it differently why are you NAKing it ?

Thank you,
Lorenzo

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355643.html

2015-07-15 14:45:33

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Wed, Jul 15, 2015 at 02:46:03PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Jul 14, 2015 at 09:41:38PM +0100, Russell King - ARM Linux wrote:
> > Sorry, NAK, and end of discussion. There is nothing more to be said
> > here.
>
> I beg to differ. To solve the issue that you brought up with this series,
> we can create an arch function that allows to set CPUidle operations, which
> would remove the need for the OF construct you did not like, this mirrors
> what's done for PSCI smp operations (something similar to smp_set_ops),
> does that sound a reasonable approach to you ?

What's the point of that?

This is _all_ that arch/arm/kernel/psci_cpuidle.c will contain after this
series:

+#include <linux/cpuidle.h>
+#include <linux/psci.h>
+
+#include <asm/cpuidle.h>
+
+static struct cpuidle_ops psci_cpuidle_ops __initdata = {
+ .suspend = cpu_psci_cpu_suspend,
+ .init = cpu_psci_cpu_init_idle,
+};
+CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);

There is _nothing_ ARM specific there. All it's doing is providing an
entry in the DT linker-built table for that data structure, and that
data structure contains a pair of function pointers to code which _will_
be located in drivers/firmware/psci.c.

I'm saying _that_ is totally pointless, that data structure and
CPUIDLE_METHOD_OF_DECLARE() _should_ live with the code in
drivers/firmware/psci.c.

There's nothing in the above quoted code which points anything to any
32bit ARM specific stuff at all (so there's no need for a smp_set_ops()
type thing), it's all about giving DT a way to specify the CPU idle
operations which should be used, in this case, allowing DT to specify
that the _generic_ PSCI CPU idle operations are to be used.

> As for M.Rutland's series[1], and the patch that moves common PSCI code to
> drivers/firmware I reiterate my point, please review it[1] and provide us
> with feedback if any otherwise acknowledge the code move, which is the
> basis on top of which everything else can be developed, I do not understand
> why you are stopping [1] from getting into the kernel since it moves
> code from arch/arm to drivers/firmware to have it in a common and shared
> place between ARM and ARM64, what's the issue you have with that or put
> it differently why are you NAKing it ?

I'm NAKing everything related to moving the PSCI code around until
someone in the PSCI maintainer pool (that's not me - that's ARM Ltd)
clearly demonstrates that they know what they're talking about, and
has a plan behind this reorganisation.

Right now, I'm getting the impression that there is _no_ plan, or if
there is, it's an absurd plan which results in data structures which
should not be in arch/arm being left behind there.

--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

2015-07-15 15:40:23

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Wed, Jul 15, 2015 at 03:45:07PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 15, 2015 at 02:46:03PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Jul 14, 2015 at 09:41:38PM +0100, Russell King - ARM Linux wrote:
> > > Sorry, NAK, and end of discussion. There is nothing more to be said
> > > here.
> >
> > I beg to differ. To solve the issue that you brought up with this series,
> > we can create an arch function that allows to set CPUidle operations, which
> > would remove the need for the OF construct you did not like, this mirrors
> > what's done for PSCI smp operations (something similar to smp_set_ops),
> > does that sound a reasonable approach to you ?
>
> What's the point of that?
>
> This is _all_ that arch/arm/kernel/psci_cpuidle.c will contain after this
> series:
>
> +#include <linux/cpuidle.h>
> +#include <linux/psci.h>
> +
> +#include <asm/cpuidle.h>
> +
> +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> + .suspend = cpu_psci_cpu_suspend,
> + .init = cpu_psci_cpu_init_idle,
> +};
> +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
>
> There is _nothing_ ARM specific there. All it's doing is providing an
> entry in the DT linker-built table for that data structure, and that
> data structure contains a pair of function pointers to code which _will_
> be located in drivers/firmware/psci.c.
>
> I'm saying _that_ is totally pointless, that data structure and
> CPUIDLE_METHOD_OF_DECLARE() _should_ live with the code in
> drivers/firmware/psci.c.
>
> There's nothing in the above quoted code which points anything to any
> 32bit ARM specific stuff at all (so there's no need for a smp_set_ops()
> type thing), it's all about giving DT a way to specify the CPU idle
> operations which should be used, in this case, allowing DT to specify
> that the _generic_ PSCI CPU idle operations are to be used.

No it is not. struct cpuidle_ops (as struct smp_operations) is ARM
specific. You can't add that to generic code (unless guarded
by CONFIG_ARM).

You can't move that struct declaration above to drivers/firmware
for the same reason you can't move CPU_METHOD_OF_DECLARE code (that
is used as a mechanism to initialize SMP ops on ARM) to
drivers/firmware.

On ARM64 this is done differently. On ARM64 SMP operations and
CPUidle operations are merged into struct cpu_operations, that
is initialized through DT at boot (it does not use linker script
mechanism because all enable-method are statically defined in
the kernel in an array to avoid SMP methods du jour
proliferation - see arch/arm64/kernel/cpu_ops.c and the
supported_cpu_ops array).

Now, we have to initialize cpuidle_ops on ARM (and SMP ops too) if
those operations are implemented with PSCI.

For SMP operations, this is done in arch/arm/kernel/setup.c through
smp_set_ops. What I was saying is that we can do the same, without
resorting to the linker script based approch above for CPUidle operations.

We can't move:

static struct cpuidle_ops psci_cpuidle_ops __initdata = {
.suspend = cpu_psci_cpu_suspend,
.init = cpu_psci_cpu_init_idle,
};
CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);

to drivers/firmware unless it is guarded by CONFIG_ARM, that's what
I am saying, is that clear ?

How do you want us to do it ?

> > As for M.Rutland's series[1], and the patch that moves common PSCI code to
> > drivers/firmware I reiterate my point, please review it[1] and provide us
> > with feedback if any otherwise acknowledge the code move, which is the
> > basis on top of which everything else can be developed, I do not understand
> > why you are stopping [1] from getting into the kernel since it moves
> > code from arch/arm to drivers/firmware to have it in a common and shared
> > place between ARM and ARM64, what's the issue you have with that or put
> > it differently why are you NAKing it ?
>
> I'm NAKing everything related to moving the PSCI code around until
> someone in the PSCI maintainer pool (that's not me - that's ARM Ltd)
> clearly demonstrates that they know what they're talking about, and
> has a plan behind this reorganisation.

M.Rutland's patch series represents the code reorganisation and as far
as I am concerned that's a complete and well defined plan, I still do
not understand why you are NAKing that piece of code, it is completely
orthogonal to what we are debating above, please have a look at it
otherwise we are going around in circles here.

> Right now, I'm getting the impression that there is _no_ plan, or if
> there is, it's an absurd plan which results in data structures which
> should not be in arch/arm being left behind there.

Mark's series does not leave any data structure behind, it is moving
code to a common place in the kernel so that the _current_ PSCI
implementation can be shared between ARM and ARM64, again please
have a look at it, we can tackle how to define cpuidle_ops in a way
that you deem reasonable later, it is a separate issue, please
understand.

Thank you,
Lorenzo

2015-07-26 21:46:21

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Wed, Jul 15, 2015 at 04:40:56PM +0100, Lorenzo Pieralisi wrote:
> static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> .suspend = cpu_psci_cpu_suspend,
> .init = cpu_psci_cpu_init_idle,
> };
> CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
>
> to drivers/firmware unless it is guarded by CONFIG_ARM, that's what
> I am saying, is that clear ?

I'd appreciate it if you didn't go bitching to Philippe, who then phones
me to discuss this problem. That's not the way "business" is done.

You definitely _can_ move this into drivers/. There's absolutely nothing
stopping that. Yes, CPUIDLE_METHOD_OF_DECLARE() may be an ARM-only thing,
that doesn't stop drivers using it - and the hint there is in the name.
Drivers. They belong in the drivers/ subdirectory, not the arch/
subdirectory.

What's so difficult to get about that?

All two _existing_ users of CPUIDLE_METHOD_OF_DECLARE() are in drivers/
already:

$ git grep CPUIDLE_METHOD_OF_DECLARE
arch/arm/include/asm/cpuidle.h:#define CPUIDLE_METHOD_OF_DECLARE(name, _method,_ops) \
drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops);
drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops);

So there's absolutely no argument you can possibly make about "it's in
asm/ so code using it must be in arch/arm."

> How do you want us to do it ?

I want this under drivers/, like I've said already several times in
this thread.

> M.Rutland's patch series represents the code reorganisation and as far
> as I am concerned that's a complete and well defined plan, I still do
> not understand why you are NAKing that piece of code, it is completely
> orthogonal to what we are debating above, please have a look at it
> otherwise we are going around in circles here.

It's not a well-defined plan if it involves dispersing related code or
data structures across the kernel tree, especially when there is very
little reason for it to be dispersed.

> > Right now, I'm getting the impression that there is _no_ plan, or if
> > there is, it's an absurd plan which results in data structures which
> > should not be in arch/arm being left behind there.
>
> Mark's series does not leave any data structure behind, it is moving
> code to a common place in the kernel so that the _current_ PSCI
> implementation can be shared between ARM and ARM64, again please
> have a look at it, we can tackle how to define cpuidle_ops in a way
> that you deem reasonable later, it is a separate issue, please
> understand.

You misunderstand me. Yes, Mark's series _on its own_ doesn't leave it
behind, but it _has the effect_ that when combined with subsequent
patches, it _causes_ that to happen.

Look, there's a simple solution to this: if the plan is to move CPU idle
PSCI support into drivers/ then lets move the whole thing into drivers.
That's a sane plan. What isn't a sane plan is to mvoe all the CPU idle
PSCI code into drivers/ and then have a PSCI data structure left in
arch/arm/.

It can live in a separate file which is only built for ARM, rather than
having ifdefs surround it, but the important thing is that it is localised
with the rest of the code, so as changes happen, it gets noticed. No one
in years to come will remember to look in arch/arm/ when making changes to
the PSCI CPU idle code.

This is no different to what drivers/soc/qcom/spm.c is doing.

So. Sane plan: "let's move all the PSCI stuff into drivers/whatever/psci/".
That's something I'm happy with.

Insane plan: "let's move the PSCI code into drivers/whatever/psci/, let's
keep PSCI data structures using that code in arch/arm". That plan (in its
entirety) I'm NAKing, because it is _no_ plan.

Lastly, I didn't realise that is an ARM only thing - that was merged
without my involvement or ACK, the change wasn't even CC'd to me (so
it's no wonder I know little about it.) I'd have NAKed that change too
had I seen it, suggesting that the contents of it (which are used by
drivers/soc/qcom/spm.c) should be located elsewhere - something which
I've done in the past when people have tried to shove stuff into
arch/arm/include/asm which gets used by stuff outside of arch/arm.

Are we clear?

--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

2015-07-27 09:15:22

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Sun, Jul 26, 2015 at 10:45:54PM +0100, Russell King - ARM Linux wrote:
> On Wed, Jul 15, 2015 at 04:40:56PM +0100, Lorenzo Pieralisi wrote:
> > static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> > .suspend = cpu_psci_cpu_suspend,
> > .init = cpu_psci_cpu_init_idle,
> > };
> > CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
> >
> > to drivers/firmware unless it is guarded by CONFIG_ARM, that's what
> > I am saying, is that clear ?
>
> I'd appreciate it if you didn't go bitching to Philippe, who then phones
> me to discuss this problem. That's not the way "business" is done.
>
> You definitely _can_ move this into drivers/. There's absolutely nothing
> stopping that. Yes, CPUIDLE_METHOD_OF_DECLARE() may be an ARM-only thing,
> that doesn't stop drivers using it - and the hint there is in the name.
> Drivers. They belong in the drivers/ subdirectory, not the arch/
> subdirectory.
>
> What's so difficult to get about that?
>
> All two _existing_ users of CPUIDLE_METHOD_OF_DECLARE() are in drivers/
> already:
>
> $ git grep CPUIDLE_METHOD_OF_DECLARE
> arch/arm/include/asm/cpuidle.h:#define CPUIDLE_METHOD_OF_DECLARE(name, _method,_ops) \
> drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops);
> drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops);
>
> So there's absolutely no argument you can possibly make about "it's in
> asm/ so code using it must be in arch/arm."
>
> > How do you want us to do it ?
>
> I want this under drivers/, like I've said already several times in
> this thread.o

We will move it there.

> > M.Rutland's patch series represents the code reorganisation and as far
> > as I am concerned that's a complete and well defined plan, I still do
> > not understand why you are NAKing that piece of code, it is completely
> > orthogonal to what we are debating above, please have a look at it
> > otherwise we are going around in circles here.
>
> It's not a well-defined plan if it involves dispersing related code or
> data structures across the kernel tree, especially when there is very
> little reason for it to be dispersed.
>
> > > Right now, I'm getting the impression that there is _no_ plan, or if
> > > there is, it's an absurd plan which results in data structures which
> > > should not be in arch/arm being left behind there.
> >
> > Mark's series does not leave any data structure behind, it is moving
> > code to a common place in the kernel so that the _current_ PSCI
> > implementation can be shared between ARM and ARM64, again please
> > have a look at it, we can tackle how to define cpuidle_ops in a way
> > that you deem reasonable later, it is a separate issue, please
> > understand.
>
> You misunderstand me. Yes, Mark's series _on its own_ doesn't leave it
> behind, but it _has the effect_ that when combined with subsequent
> patches, it _causes_ that to happen.

Let's forget the issues related to CPUidle, we have an agreement on the way
forward, I will make sure it is implemented.

I just wanted to say, please do not block Mark's series because of this patch,
that series makes sense standalone and is a step in the right direction, I
will make sure we implement the changes you requested in this thread on
top of it if you allow it to get into the kernel.

> Look, there's a simple solution to this: if the plan is to move CPU idle
> PSCI support into drivers/ then lets move the whole thing into drivers.
> That's a sane plan. What isn't a sane plan is to mvoe all the CPU idle
> PSCI code into drivers/ and then have a PSCI data structure left in
> arch/arm/.
>
> It can live in a separate file which is only built for ARM, rather than
> having ifdefs surround it, but the important thing is that it is localised
> with the rest of the code, so as changes happen, it gets noticed. No one
> in years to come will remember to look in arch/arm/ when making changes to
> the PSCI CPU idle code.
>
> This is no different to what drivers/soc/qcom/spm.c is doing.
>
> So. Sane plan: "let's move all the PSCI stuff into drivers/whatever/psci/".
> That's something I'm happy with.

I am ok with that too, so I think we are in agreement.

> Insane plan: "let's move the PSCI code into drivers/whatever/psci/, let's
> keep PSCI data structures using that code in arch/arm". That plan (in its
> entirety) I'm NAKing, because it is _no_ plan.
>
> Lastly, I didn't realise that is an ARM only thing - that was merged
> without my involvement or ACK, the change wasn't even CC'd to me (so
> it's no wonder I know little about it.) I'd have NAKed that change too
> had I seen it, suggesting that the contents of it (which are used by
> drivers/soc/qcom/spm.c) should be located elsewhere - something which
> I've done in the past when people have tried to shove stuff into
> arch/arm/include/asm which gets used by stuff outside of arch/arm.
>
> Are we clear?

Yes, I would only ask you, if the plan above (which can be implemented
in two steps) makes sense to you please consider accepting Mark's change
to consolidate PSCI code into drivers/firmware/psci, it is a stepping stone
without which the changes above can't happen, I will take charge of completing
the move of CPUidle code and create the required shim layer into
drivers to make this happen.

Are you ok with this ?

Thanks !!
Lorenzo

2015-07-27 09:45:52

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Mon, Jul 27, 2015 at 10:16:02AM +0100, Lorenzo Pieralisi wrote:
> Yes, I would only ask you, if the plan above (which can be implemented
> in two steps) makes sense to you please consider accepting Mark's change
> to consolidate PSCI code into drivers/firmware/psci, it is a stepping stone
> without which the changes above can't happen, I will take charge of completing
> the move of CPUidle code and create the required shim layer into
> drivers to make this happen.

Why can't Jisheng Zhang base his patches on top of Mark's changes and
place the new file directly under drivers/ ?

Why do it as a two-step process with it first appearing in arch/arm,
and then having to generate another patch at a later date to move it
elsewhere. That just creates more noise, and we should be avoiding
generating noise in arch/arm.

This is what Linus has said in his -rc4 release notes yesterday:

Other than that issue, it's mostly drivers and networking. USB, gpu,
mmc, network drivers, sound. With some ARM noise (but even that is
mostly driver-related: dts updates due to MMC fixes). And a few small
filesystem fixes.

and we can infer from the phrase "ARM noise" that Linus' opinion of
arch/arm is still fairly low, and still doesn't regard the "churn" in
arch/arm as being useful.

--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

2015-07-27 10:00:21

[permalink] [raw]

Subject: Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Mon, Jul 27, 2015 at 10:45:07AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 27, 2015 at 10:16:02AM +0100, Lorenzo Pieralisi wrote:
> > Yes, I would only ask you, if the plan above (which can be implemented
> > in two steps) makes sense to you please consider accepting Mark's change
> > to consolidate PSCI code into drivers/firmware/psci, it is a stepping stone
> > without which the changes above can't happen, I will take charge of completing
> > the move of CPUidle code and create the required shim layer into
> > drivers to make this happen.
>
> Why can't Jisheng Zhang base his patches on top of Mark's changes and
> place the new file directly under drivers/ ?
>
> Why do it as a two-step process with it first appearing in arch/arm,
> and then having to generate another patch at a later date to move it
> elsewhere. That just creates more noise, and we should be avoiding
> generating noise in arch/arm.

Nothing will appear in arch/arm, I promise, I said two-step process
because Mark's series is standalone/ready-to-be-merged while Jisheng
patch series has still some bits to be debated (that won't affect
what we discussed in relation to the code split and do not require
any change in Mark's series at all).

No code move, nothing in arch/arm, we will stick to the plan as I said
before and I fully agree with that, please do not block one mature patch
series for another one that still has some bits to be settled, and they
are totally independent.

> This is what Linus has said in his -rc4 release notes yesterday:
>
> Other than that issue, it's mostly drivers and networking. USB, gpu,
> mmc, network drivers, sound. With some ARM noise (but even that is
> mostly driver-related: dts updates due to MMC fixes). And a few small
> filesystem fixes.
>
> and we can infer from the phrase "ARM noise" that Linus' opinion of
> arch/arm is still fairly low, and still doesn't regard the "churn" in
> arch/arm as being useful.

Mark's series consolidate ARM/ARM64 PSCI implementations, it does not
require creating anything in arch/arm actually it moves code in arch/arm
to drivers/firmware, consolidating it, it is definitely the right
thing to do in this respect.

The CPUidle code comes as a second series on top of Mark's one and it
will _not_ add anything in arch/arm (if you allow Mark to proceed), you
have my word :)

Does it sound ok to you ?

Thanks !
Lorenzo

2015-07-27 10:10:03