[PATCH v2 6/6] arm/imx6q: add suspend/resume support

Shawn Guo shawn.guo at freescale.com
Sat Sep 17 04:30:09 EDT 2011


On Fri, Sep 16, 2011 at 03:45:39PM +0100, Lorenzo Pieralisi wrote:
> Hi Shawn,
> 
> On Fri, Sep 16, 2011 at 07:09:00AM +0100, Shawn Guo wrote:
> > Hi Lorenzo,
> > 
> > On Thu, Sep 15, 2011 at 05:28:29PM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, Sep 15, 2011 at 03:45:26PM +0100, Shawn Guo wrote:
> > > > It adds suspend/resume support for imx6q.
> > > > 
> > > > Signed-off-by: Anson Huang <b20788 at freescale.com>
> > > > Signed-off-by: Shawn Guo <shawn.guo at linaro.org>
> > > > ---
> 
> [...]
> 
> > > > +ENTRY(v7_cpu_resume)
> > > > +	bl	v7_invalidate_l1
> > > > +
> > > > +	/*
> > > > +	 * Restore L2 AUX_CTRL register saved by suspend procedure
> > > > +	 * and enable L2
> > > > +	 */
> > > > +	adr	r4, 1f
> > > > +	ldmia	r4, {r5, r6, r7}
> > > > +	sub	r4, r4, r5
> > > > +	add	r6, r6, r4
> > > > +	add	r7, r7, r4
> > > > +	ldr	r0, [r6]
> > > > +	ldr	r7, [r7]
> > > > +	ldr	r1, [r7]
> > > > +	str	r1, [r0, #L2X0_AUX_CTRL]
> > > > +	ldr	r1, =0x1
> > > > +	str	r1, [r0, #L2X0_CTRL]
> > > > +
> > > > +	b	cpu_resume
> > > > +
> > > > +	.align
> > > > +1:	.long	.
> > > > +	.long	pl310_pbase
> > > > +	.long	pl310_aux_ctrl_paddr
> > > 
> > > Would not something like:
> > > 
> > > 	adr	r4, pl310_pbase
> > > 	ldmia	r4, {r6, r7}
> > > 	[...]
> > > 
> > > pl310_pbase:
> > > 	.long 0
> > > pl310_aux_ctrl:
> > > 	.long 0
> > > 
> > > be better and faster ? Why play with virtual addresses ?
> > > Of course you should initialize the values, but then you can access them
> > > through a PC relative load when running physical.
> > 
> > Thanks for the comment.  I agree with you that it's better, though my
> > first thought on the existing approach is I can access the global
> > variables defined in C file directly from assembly code.
> > 
> 
> You can even access assembly variables from C code, declare them
> 
> 	.globl pl310_base
> 
> and 
> 
> extern unsigned int pl310_base;
> 
> in your C code and you are all set. But I would avoid global variables,
> comments below.
> 
Ah, thanks.  As you can see, I'm still at entry level of assembly :)

> > > Your code should be in the .data section for it to be writable (adr does not
> > > work across sections), have a look at Russell's code in sleep.S it is
> > > very well commented and similar to what you need.
> > 
> > Thanks for pointing me the example.
> > 
> 
> You are welcome, my pleasure.
> 
> > > 
> > > > +ENDPROC(v7_cpu_resume)
> > > > diff --git a/arch/arm/mach-imx/pm-imx6q.c b/arch/arm/mach-imx/pm-imx6q.c
> > > > new file mode 100644
> > > > index 0000000..124bcd5
> > > > --- /dev/null
> > > > +++ b/arch/arm/mach-imx/pm-imx6q.c
> > > > @@ -0,0 +1,88 @@
> > > > +/*
> > > > + * Copyright 2011 Freescale Semiconductor, Inc.
> > > > + * Copyright 2011 Linaro Ltd.
> > > > + *
> > > > + * The code contained herein is licensed under the GNU General Public
> > > > + * License. You may obtain a copy of the GNU General Public License
> > > > + * Version 2 or later at the following locations:
> > > > + *
> > > > + * http://www.opensource.org/licenses/gpl-license.html
> > > > + * http://www.gnu.org/copyleft/gpl.html
> > > > + */
> > > > +
> > > > +#include <linux/init.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/suspend.h>
> > > > +#include <asm/proc-fns.h>
> > > > +#include <asm/suspend.h>
> > > > +#include <asm/hardware/cache-l2x0.h>
> > > > +#include <mach/common.h>
> > > > +#include <mach/hardware.h>
> > > > +
> > > > +static void __iomem *pl310_vbase;
> > > > +void __iomem *pl310_pbase;
> > > > +
> > > > +static volatile unsigned long pl310_aux_ctrl;
> > > > +volatile unsigned long pl310_aux_ctrl_paddr;
> > > 
> > > I think that by defining those variables in assembly you would make
> > > your life much simpler.
> > 
> > Yes.  But I need a function call to learn the address of those variables
> > from assembly now.
> > 
> 
> No, you do not, see above.
> 
> > > I think you know your L2 is already initialized here to make sure you
> > > save the right aux value. Hence you should clean the variables above from
> > > L2 to make sure they are available at reset from DRAM (L2 is retained
> > > and you do not clean it on suspend, correct ?)
> > 
> > Yes, agreed.  It's right thing to do for being safe.  Actually, I did it
> > when I saved the variables during suspend.  If I do not do, it simply
> > does not work.  But later, when I moved the saving to init function
> > since it needs to be done for only once, I found it works even without
> > the cache clean.  Then I dropped it.  To be safe, now I'm adding it
> > back with following your comment.
> > 
> > > 
> > > I do not think that code to save/restore L2 config belongs here though.
> > > More below.
> > > 
> > > > +
> > > > +static int imx6q_suspend_finish(unsigned long val)
> > > > +{
> > > > +	cpu_do_idle();
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int imx6q_pm_enter(suspend_state_t state)
> > > > +{
> > > > +	switch (state) {
> > > > +	case PM_SUSPEND_MEM:
> > > > +		imx6q_set_lpm(STOP_POWER_OFF);
> > > > +		imx_gpc_pre_suspend();
> > > > +		imx_set_cpu_jump(0, v7_cpu_resume);
> > > > +		/* Zzz ... */
> > > > +		cpu_suspend(0, imx6q_suspend_finish);
> > > > +		imx_smp_prepare();
> > > > +		imx_gpc_post_resume();
> > > > +		break;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static const struct platform_suspend_ops imx6q_pm_ops = {
> > > > +	.enter = imx6q_pm_enter,
> > > > +	.valid = suspend_valid_only_mem,
> > > > +};
> > > > +
> > > > +void __init imx6q_pm_init(void)
> > > > +{
> > > > +	struct device_node *np;
> > > > +	u32 reg[2];
> > > > +
> > > > +	np = of_find_compatible_node(NULL, NULL, "arm,pl310-cache");
> > > > +	of_property_read_u32_array(np, "reg", reg, ARRAY_SIZE(reg));
> > > > +	pl310_vbase = ioremap(reg[0], reg[1]);
> > > 
> > > Mmmm...is this vma ever released ? L2 is already mapped in the L2
> > > driver from DT or through static mappings. 
> > 
> > I think we can do another mapping even it's been done in the L2 driver,
> > no?
> > 
> 
> I still think that it is not really clean. But more importantly you should
> release the mapping when you are done with it (iounmap).
> 
Yes, I should iounmap it.

> > > Overall, I think that code to restore PL310 belongs in cache-l2x0.c, not here.
> > > We can easily write an assembly stub that reinitialize L2 before
> > > resume if that's something we should and can do (security ?).
> > > 
> > I would be definitely happy to see that, but before rmk agrees on that,
> > I have to find a way around in the platform code.
> 
> I am working on that; your point is fair, but security notwithstanding
> there is nothing that can prevent us from having an assembly hook to
> resume L2 in a platform independent manner. More to come.
> 
Great.  I will definitely migrate imx6q to that once it gets merged.

> > 
> > Here is the updated patch.  If it looks better to you, I will
> > incorporate it in the v3 of the series.
> > 
> > 8<---
> > diff --git a/arch/arm/mach-imx/head-v7.S b/arch/arm/mach-imx/head-v7.S
> > index ede908b..5a486a9 100644
> > --- a/arch/arm/mach-imx/head-v7.S
> > +++ b/arch/arm/mach-imx/head-v7.S
> > @@ -69,3 +69,35 @@ ENTRY(v7_secondary_startup)
> >  	b	secondary_startup
> >  ENDPROC(v7_secondary_startup)
> >  #endif
> > +
> > +ENTRY(pl310_get_save_ptr)
> > +	ldr	r0, =pl310_pbase
> > +	mov	pc, lr
> > +ENDPROC(pl310_get_save_ptr)
> > +
> 
> You do not need a function to do that. Just declare the label as 
> 
> 	.globl
> 
> and from C you can access those values. See above.
> 
> It would be nicer to avoid global variables altogether though.
> You can write an assembly function taking the values to be saved and
> returning the pointer to flush from L1 and L2.
> 
Thanks for the suggestion.  But I feel good about the current
implementation.

> > +ENTRY(v7_cpu_resume)
> > +	bl	v7_invalidate_l1
> > +	bl	pl310_resume
> > +	b	cpu_resume
> > +ENDPROC(v7_cpu_resume)
> > +
> > +/*
> > + * The following code is located into the .data section.  This is to
> > + * allow pl310_pbase and pl310_aux_ctrl to be accessed with a relative
> > + * load as we are running on physical address here.
> > + */
> > +	.data
> > +	.align
> > +ENTRY(pl310_resume)
> > +	adr	r2, pl310_pbase
> > +	ldmia	r2, {r0, r1}
> > +	str	r1, [r0, #L2X0_AUX_CTRL]	@ restore aux_ctrl
> > +	mov	r1, #0x1
> > +	str	r1, [r0, #L2X0_CTRL]		@ re-enable L2
> > +	mov	pc, lr
> > +ENDPROC(pl310_resume)
> > +
> > +pl310_pbase:
> > +	.long	0
> > +pl310_aux_ctrl:
> > +	.long	0
> 
> You might want to inline it and avoid the jump.
> 
One reason that I implemented pl310_resume as a function call is that
I was trying to minimize the code that we have to put in .data section.
Now I do not think it's a point that really matters.  So following your
suggestion, here it is.  Please let me know it is not what you meant to
see.

/*
 * The following code is located into the .data section.  This is to
 * allow pl310_pbase and pl310_aux_ctrl to be accessed with a relative
 * load as we are running on physical address here.
 */
        .data
        .align

        .macro  pl310_resume
        adr     r2, pl310_pbase
        ldmia   r2, {r0, r1}
        str     r1, [r0, #L2X0_AUX_CTRL]        @ restore aux_ctrl
        mov     r1, #0x1
        str     r1, [r0, #L2X0_CTRL]            @ re-enable L2
        .endm

ENTRY(v7_cpu_resume)
        bl      v7_invalidate_l1
        pl310_resume
        b       cpu_resume
ENDPROC(v7_cpu_resume)

.globl
pl310_pbase:
        .long   0
pl310_aux_ctrl:
        .long   0

-- 
Regards,
Shawn




More information about the linux-arm-kernel mailing list