On 21 December 2010 05:43, MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> wrote:
> Hibernation (Suspend-To-Disk) support for ARM Cortex A8 and A9.
>
> This patch is based on the work of Hiroshi DOYU at Nokia, which is
> stated to be based on the original work of Romit and Raghu at TI.
>
> The hibernation support is tested with S5PC210 (ARM Cortex A9 MP2). In
> order to add support for Cortex A9 on the original patch of Hiroshi
> DOYU, which support Cortex A8 only, we have edited the list of registers
> to be saved in the case of A9.
>
> In the original work of Hiroshi DOYU, arch/arm/kernel/vmlinux.lds.S was
> modified; however, we have reverted it because it only created build
> failure in our tested system.
>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
Please note that I only had a quick look at the code and I won't have
time to do a proper review before the end of the year. But I have a
few comments which are consider essential.
> --- /dev/null
> +++ b/arch/arm/kernel/hibernate.c
> @@ -0,0 +1,323 @@
> +/*
> + * Hibernation support specific for ARM
> + *
> + * Copyright (C) 2010 Nokia Corporation
> + * Copyright (C) 2010 Texas Instruments, Inc.
> + * Copyright (C) 2006 Rafael J. Wysocki <rjw@xxxxxxx>
> + *
> + * Contact: Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mm.h>
> +
> +
> +/* The following architectures are known to be CORTEX_A9 */
> +#if defined(CONFIG_ARCH_S5PV310) || defined(CONFIG_ARCH_U8500)
> +#define CORTEX_A9
> +#else
> +/* Assume CORTEX_A8 as default */
> +#define CORTEX_A8
> +#endif
We should use run-time detection of the CPU rather than #ifdef's. We
have the ability to build multiple processors support into the same
kernel image.
> +struct saved_context {
> +/*
> + * FIXME: Only support for Cortex A8 and Cortex A9 now
> + */
> + Â Â Â /* CR0 */
> + Â Â Â u32 cssr; Â Â Â /* Cache Size Selection */
> + Â Â Â /* CR1 */
> +#ifdef CORTEX_A8
> + Â Â Â u32 cr; Â Â Â Â /* Control */
> + Â Â Â u32 cacr; Â Â Â /* Coprocessor Access Control */
> +#elif defined(CORTEX_A9)
> + Â Â Â u32 cr;
> + Â Â Â u32 actlr;
> + Â Â Â u32 cacr;
> + Â Â Â u32 sder;
> + Â Â Â u32 vcr;
> +#endif
We could follow the ARM ARM and define a save_context structure which
has architected registers and implementation-defined ones. For the
latter you could use a union inside this structure to differentiate
between A8, A9 and other registers for different implementations.
> + Â Â Â /* CR2 */
> + Â Â Â u32 ttb_0r; Â Â /* Translation Table Base 0 */
> + Â Â Â u32 ttb_1r; Â Â /* Translation Table Base 1 */
> + Â Â Â u32 ttbcr; Â Â Â/* Translation Talbe Base Control */
> + Â Â Â /* CR3 */
> + Â Â Â u32 dacr; Â Â Â /* Domain Access Control */
> + Â Â Â /* CR5 */
> + Â Â Â u32 d_fsr; Â Â Â/* Data Fault Status */
> + Â Â Â u32 i_fsr; Â Â Â/* Instruction Fault Status */
> + Â Â Â u32 d_afsr; Â Â /* Data Auxilirary Fault Status */ Â Â Â ;
> + Â Â Â u32 i_afsr; Â Â /* Instruction Auxilirary Fault Status */;
> + /* CR6 */
> + u32 d_far; /* Data Fault Address */
> + u32 i_far; /* Instruction Fault Address */
I haven't checked the ARM ARM but some registers may be read-only and
not really needing saving/restoring. Some of them also don't have any
information that is essential when coming back from hibernation (like
the FSR/FAR ones).
> +static inline void __save_processor_state(struct saved_context *ctxt)
> +{
> + Â Â Â /* CR0 */
> + Â Â Â asm volatile ("mrc p15, 2, %0, c0, c0, 0" : "=r"(ctxt->cssr));
> + Â Â Â /* CR1 */
> +#ifdef CORTEX_A8
> + Â Â Â asm volatile ("mrc p15, 0, %0, c1, c0, 0" : "=r"(ctxt->cr));
> + Â Â Â asm volatile ("mrc p15, 0, %0, c1, c0, 2" : "=r"(ctxt->cacr));
> +#elif defined(CORTEX_A9)
> + Â Â Â asm volatile ("mrc p15, 0, %0, c1, c0, 0" : "=r"(ctxt->cr));
> + Â Â Â asm volatile ("mrc p15, 0, %0, c1, c0, 1" : "=r"(ctxt->actlr));
> + Â Â Â asm volatile ("mrc p15, 0, %0, c1, c0, 2" : "=r"(ctxt->cacr));
> + Â Â Â asm volatile ("mrc p15, 0, %0, c1, c1, 1" : "=r"(ctxt->sder));
> + Â Â Â asm volatile ("mrc p15, 0, %0, c1, c1, 3" : "=r"(ctxt->vcr));
> +#endif
I think this could be split into a generic state saving function and a
per-processor one. You can then do some checks on the CPU ID to and
call the corresponding implementation-specific save function.
> +static inline void __restore_processor_state(struct saved_context *ctxt)
> +{
[...]
> +#ifdef CORTEX_A8
> + Â Â Â asm volatile ("mcr p15, 1, %0, c9, c0, 0" : : "r"(ctxt->l2clr));
> +#endif
This register (and maybe others) is not always writable in non-secure
mode. If running in non-secure mode (which cannot easily be detected
at run-time), we should first check the Nonsecure Access Control
Register.
--
Catalin
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
