From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel.lezcano@linaro.org (Daniel Lezcano) Date: Wed, 18 Mar 2015 09:13:27 +0100 Subject: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT In-Reply-To: <5508D160.6090205@codeaurora.org> References: <1425385777-14766-1-git-send-email-daniel.lezcano@linaro.org> <1425385777-14766-3-git-send-email-daniel.lezcano@linaro.org> <55075453.2070503@codeaurora.org> <20150317112905.GA14984@red-moon> <5508D160.6090205@codeaurora.org> Message-ID: <550933A7.9030401@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.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 >>>> +#include >>>> +#include >>>> #include >>>> >>>> +#define CPUIDLE_OPS(__x) extern struct cpuidle_ops __x ## _cpuidle_ops; >>>> +#include >>>> +#undef CPUIDLE_OPS >>>> + >>>> +#define CPUIDLE_OPS(__x) __x ## _cpuidle_ops_id, >>>> +enum cpuidle_ops_id { >>>> +#include >>>> + 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 >>>> +}; >>>> +#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. > -- Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog