Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Stephen Boyd]

On 03/17/15 04:29, Lorenzo Pieralisi wrote:
> On Mon, Mar 16, 2015 at 10:08:19PM +0000, Stephen Boyd wrote:
>> On 03/03/15 04:29, Daniel Lezcano wrote:
>>> 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.
>> [...]
>>> 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];
>> Is there any reason why we aren't putting these structures into a linker
>> section like we do for the smp operations structures?
> I think it can be done with an OF_TABLE, it is a bit of shame cpuidle_ops
> should work on UP too otherwise they could have been merged in
> smp_ops to create cpu_ops, like arm64 does.

We should merge the two and remove the SMP dependency on arm32.

>> The nice thing about using the linker is it makes it clearer at the
>> location where we define the structure that it's actually used by
>> something. Right now the structures are defined non-static in a file and
>> then we have to know that a CPUIDLE_OPS() define has been made in
>> another architecture specific asm header file so that this macro magic
>> works. The commit text says something about multiple declarations and
>> ops spread across header files, which shouldn't apply if we're using the
>> linker to find these ops and merge them into an array we can iterate over.
> It makes sense, see above for UP vs SMP. I wonder if we can't find
> something to overcome the UP limitation nicely, the init code in
> arch/arm/kernel/devtree.c is identical for smp_ops and cpuidle_ops,
> apart from the CONFIG_SMP ifdeffery.

It should be possible to replace the arm32 smp_operations structure with
something like the arm64 cpu_operations structure. Yes we would have to
drop the SMP dependency, but that will be ok. It would require some work
to make arm32 and arm64 the same, but for these purposes that isn't
really required as long as we can put the cpu idle hook there.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/