On Tue, Mar 03, 2015 at 12:29:33PM +0000, Daniel Lezcano wrote:
> The current state of the different cpuidle drivers is the different PM
Nit: "The current state of cpuidle drivers is such that different ..."
> operations are passed via the platform_data using the platform driver
> paradigm.
>
> This approach allowed to split the low level PM code from the arch specific
> and the generic cpuidle code.
>
> Unfortunately there are complains about this approach as, in the context of the
Nit: s/complains/complaints
> single kernel image, we have multiple drivers loaded in memory for nothing and
> the platform driver is not adequate for cpuidle.
>
> This patch provides a common interface via cpuidle ops for all new cpuidle
> driver and a definition for the device tree.
>
> It will allow with the next patches to a have a common definition with ARM64
> and share the same cpuidle driver.
>
> The code is optimized to use the __init section intensively in order to reduce
> the memory footprint after the driver is initialized and unify the function
> names with ARM64.
>
> In order to prevent multiple declarations and the specific cpuidle ops to be
> spread across the different headers, a mechanism, similar to the cgroup subsys,
> has been introduced.
>
> A new platform willing to add its cpuidle ops must add an entry in the file
> cpuidle_ops.h in the current form:
>
> #if IS_ENABLED(CONFIG_ARM_FOO_CPUIDLE)
> CPUIDLE_OPS(foo)
> #endif
>
> ... and use the variable name in the specific low level code:
>
> struct cpuidle_ops foo_cpuidle_ops;
>
> The CPUIDLE_OPS macro will be processed in different way in the cpuidle.c file,
> thus allowing to keep untouched the arm cpuidle core code in the future when
> a new platform is added.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> ---
> arch/arm/include/asm/cpuidle.h | 10 +++++
> arch/arm/include/asm/cpuidle_ops.h | 3 ++
> arch/arm/kernel/cpuidle.c | 85 ++++++++++++++++++++++++++++++++++++++
> arch/arm64/include/asm/cpuidle.h | 5 ++-
> 4 files changed, 102 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/include/asm/cpuidle_ops.h
>
> diff --git a/arch/arm/include/asm/cpuidle.h b/arch/arm/include/asm/cpuidle.h
> index 348dc81..3d31459 100644
> --- a/arch/arm/include/asm/cpuidle.h
> +++ b/arch/arm/include/asm/cpuidle.h
> @@ -27,4 +27,14 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> */
> #define ARM_CPUIDLE_WFI_STATE ARM_CPUIDLE_WFI_STATE_PWR(UINT_MAX)
>
> +struct cpuidle_ops {
> + const char *name;
> + int (*suspend)(int cpu, unsigned long arg);
> + int (*init)(struct device_node *, int cpu);
> +};
> +
> +extern int arm_cpuidle_suspend(int index);
> +
> +extern int arm_cpuidle_init(int cpu);
idle_cpu_suspend()
idle_cpu_init()
?
I am really not fussed about the naming.
To make this and x86 driver name compliant (well, function signatures
are a bit different) we could use:
arm_idle()
arm_idle_cpu_init()
even though I think the arch prefix is useless.
Side note: why is the x86 driver in drivers/idle ? To have another dir :) ?
> +
> #endif
> diff --git a/arch/arm/include/asm/cpuidle_ops.h b/arch/arm/include/asm/cpuidle_ops.h
> new file mode 100644
> index 0000000..be0a612
> --- /dev/null
> +++ b/arch/arm/include/asm/cpuidle_ops.h
> @@ -0,0 +1,3 @@
> +/*
> + * List of cpuidle operations
> + */
> diff --git a/arch/arm/kernel/cpuidle.c b/arch/arm/kernel/cpuidle.c
> index 45969f8..25e9789c 100644
> --- a/arch/arm/kernel/cpuidle.c
> +++ b/arch/arm/kernel/cpuidle.c
> @@ -10,8 +10,29 @@
> */
>
> #include <linux/cpuidle.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> #include <asm/cpuidle.h>
>
> +#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops;
> +#include <asm/cpuidle_ops.h>
> +#undef CPUIDLE_OPS
> +
> +#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id,
> +enum cpuidle_ops_id {
> +#include <asm/cpuidle_ops.h>
> + CPUIDLE_OPS_COUNT,
> +};
> +#undef CPUIDLE_OPS
> +
> +#define CPUIDLE_OPS(__x) [__x ## _cpuidle_ops_id ] = &__x ## _cpuidle_ops,
> +static struct cpuidle_ops *supported_cpuidle_ops[] __initconst = {
> +#include <asm/cpuidle_ops.h>
> +};
> +#undef CPUIDLE_OPS
> +
> +static struct cpuidle_ops cpuidle_ops[NR_CPUS];
That's because you want platform cpuidle_ops to be __initdata ?
It should not be a big overhead on arm32 to have a number of
structs equal to NR_CPUS, on arm64 it is the other way around
there are few cpu_ops, but number of CPUs can be high so it
is an array of pointers.
I think it is ok to leave it as it is (or probably make cpuidle_ops
a single struct, I expect enable-method to be common across cpus).
> +
> int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int index)
> {
> @@ -19,3 +40,67 @@ int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
>
> return index;
> }
> +
> +int arm_cpuidle_suspend(int index)
> +{
> + int ret = -EOPNOTSUPP;
> + int cpu = smp_processor_id();
> +
> + if (cpuidle_ops[cpu].suspend)
> + ret = cpuidle_ops[cpu].suspend(cpu, index);
> +
> + return ret;
> +}
> +
> +static struct cpuidle_ops *__init arm_cpuidle_get_ops(const char *name)
> +{
> + int i;
> +
> + for (i = 0; i < CPUIDLE_OPS_COUNT; i++) {
> + if (!strcmp(name, supported_cpuidle_ops[i]->name))
> + return supported_cpuidle_ops[i];
> + }
> +
> + return NULL;
> +}
> +
> +static int __init arm_cpuidle_read_ops(struct device_node *dn, int cpu)
> +{
> + const char *enable_method;
> + struct cpuidle_ops *ops;
> +
> + enable_method = of_get_property(dn, "enable-method", NULL);
> + if (!enable_method)
> + return -ENOENT;
> +
> + ops = arm_cpuidle_get_ops(enable_method);
> + if (!ops) {
> + pr_warn("%s: unsupported enable-method property: %s\n",
> + dn->full_name, enable_method);
> + return -EOPNOTSUPP;
> + }
> +
> + cpuidle_ops[cpu] = *ops; /* structure copy */
See above.
> +
> + pr_notice("cpuidle: enable-method property '%s'"
> + " found operations\n", ops->name);
> +
> + return 0;
> +}
> +
> +int __init arm_cpuidle_init(int cpu)
> +{
> + int ret = -EOPNOTSUPP;
Nit: You always assign ret, so there is no point in initializing it.
Lorenzo
> + struct device_node *cpu_node = of_cpu_device_node_get(cpu);
> +
> + if (!cpu_node)
> + return -ENODEV;
> +
> + ret = arm_cpuidle_read_ops(cpu_node, cpu);
> + if (!ret && cpuidle_ops[cpu].init)
> + ret = cpuidle_ops[cpu].init(cpu_node, cpu);
> +
> + of_node_put(cpu_node);
> +
> + return ret;
> +}
> diff --git a/arch/arm64/include/asm/cpuidle.h b/arch/arm64/include/asm/cpuidle.h
> index 0710654..1bee287 100644
> --- a/arch/arm64/include/asm/cpuidle.h
> +++ b/arch/arm64/include/asm/cpuidle.h
> @@ -15,5 +15,8 @@ static inline int cpu_suspend(unsigned long arg)
> return -EOPNOTSUPP;
> }
> #endif
> -
> +static inline int arm_cpuidle_suspend(int index)
> +{
> + return cpu_suspend(index);
> +}
> #endif
> --
> 1.9.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
