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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 03/18/2015 02:14 AM, Stephen Boyd wrote:
> 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.

I will be happy to do that but right now it would be nice to keep 
focused on bringing the cpuidle ops first, even if we have a bit of code 
duplicated, in order to unblock the cpuidle drivers awaiting for this 
code to be merged.

>>> 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.
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog