[RFC] arm32: introduce suspend/resume common functions #1729

MrVan · 2017-08-02T02:33:52Z

This is to introduce low power suspend/resume support for arm32. The work flow is similar as linux suspend/resume, the different is linux suspend/resume saves/restores linux side contexts and system registers, while the optee suspend/resume saves/restores tee side contexts and system registers. Also some differences that ttbr0 is saved/restored.
Now only 1 cluster max 4 cores supported and with Cortex-A7/A9 MP core tested.
This piece code is not the whole picture, but these are all the common code I implemented, others are platform specific code, such as platform normally works at svc mode when reset, but when tee resume we need to switch to monitor mode because psci frameworks runs in monitor mode. And code handling platform suspend/resume coping into onchip ram that could survive cpu suspend/resume.

MrVan · 2017-08-08T10:07:05Z

Are there any comments about this RFC patch set?

igoropaniuk · 2017-08-08T15:42:21Z

core/arch/arm/sm/suspend.S

+	read_nmrr r5
+	read_vbar r6
+	read_nsacr r7
+	stmia	r0, {r4 - r7}


stmia r0!, {r4 - r7} (! missed) ?

There is no need to use r0! here, this is the last inst to store register to stack and then r0 is not needed.

igoropaniuk · 2017-08-08T15:44:37Z

core/arch/arm/include/sm/pm.h

+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+


header guards

jforissier · 2017-08-10T09:11:14Z

@MrVan FYI the Travis build failure can be fixed by rebasing on top of the current master branch, in order to to get 9aea3dc ("Travis: add -p to mkdir $HOME/bin").

MrVan · 2017-08-14T01:40:05Z

Patch updated with addressing @igoropaniuk 's comments and rebased with 9aea3dc ("Travis: add -p to mkdir $HOME/bin"). Any comments?

etienne-lms · 2017-08-14T09:54:01Z

core/arch/arm/sm/suspend.S

+.section .text
+FUNC cpu_suspend, :
+UNWIND( .fnstart)
+	stmfd	sp!, {r4 - r11, lr}


Recommend push {r4-r11, lr}
Push one more register for 8byte stack alignment.
Add UNWIND(.save {r4-r11, lr})

I'll try this. Thanks.

etienne-lms · 2017-08-14T10:01:48Z

core/arch/arm/sm/suspend.S

+	 */
+	and	r6, r6, #0x3
+	lsl	r6, r6, #0x3
+	stmfd	sp!, {r0, r1}


recommend push {r0, r1}

etienne-lms · 2017-08-14T10:02:59Z

core/arch/arm/sm/suspend.S

+	ldr	r4, =suspend_reg_size
+	add	r4, r4, #12
+	add	sp, sp, r4
+	ldmia	sp!, {r4 - r11, lr}


recommend pop {r4-r11, lr}

etienne-lms · 2017-08-14T10:04:14Z

core/arch/arm/sm/suspend.S

+	ldr	r4, =suspend_reg_size
+	mov	r5, sp
+	add	r4, r4, #12
+	sub	sp, sp, r4


I wonder how unwind could handle this.

This is to reserve space in stack for saving regs that will lost when cpu losing power. unwind may not able to handle this. I may need to add KEEP_PAGER the functions.

etienne-lms · 2017-08-14T16:00:50Z

core/arch/arm/sm/suspend.S

+	 * the &sleep_save_sp_virt = ((u32)suspend_sp + 4 * cpu_id)
+	 */
+	and	r6, r6, #0x3
+	lsl	r6, r6, #0x3


lsl r6, r6, #0x2 ?

No. Each cpu needs 2 words(8 bytes), lsl r6, r6, #3, means r6 * 8

etienne-lms · 2017-08-14T16:07:18Z

core/arch/arm/sm/suspend.S

+	ldr	r3, =cpu_resume_after_mmu
+	isb
+	write_sctlr r0
+	mrc     p15, 0, r0, c0, c0, 0


Could you cross check this (from here to the 2nd mov r0, r0 below. Looks useless.

write_sctlr r0 actually will enable MMU. Yeah, that could removed on ARMV7 cpus. This could be

write_tlbiall write_sctlr r0 dsb isb ``

etienne-lms · 2017-08-14T16:08:29Z

core/arch/arm/sm/suspend.S

+	 */
+	read_diag r10
+	teq     r5, r10
+	mcrne   p15, 0, r5, c15, c0, 1


Here and below: is it harmful to restore an already updated value ?

No. This could be simplified. No one actually restore it before this point.

jenswi-linaro · 2017-08-17T09:24:39Z

core/arch/arm/sm/pm.c

+ * Each cpu needs 2 words to save ptr and virt_to_phys(ptr).
+ * We max support 4 cpus now,so need 4 * 2 * sizeof(uint32_t);
+ */
+uint32_t suspend_sp[4 * 2] __aligned(8);


This would be more convenient to store in struct thread_core_local. What's the alignment for?

I am not sure. it maybe a bit hard to refer thread_core_local, i'll try. No specific requirement on the alignment, could be removed.

After a thought, thread_core_local could not be used here. Because suspend_sp will be used when cpu resume at very early stage to restore registers from sp(physical address). At this time,accessing thread_core_local is not easy, also have to fix the layout of thread_core_local for the saved sp pointer. I prefer use suspend_sp here.

thread_core_local could be made a global variable just as suspend_sp is here, I don't see what's different.
core/arch/arm/kernel/asm-defines.c can be used to find offsets of different elements in the struct.

ok. I'll try this. please hold on for the review patch, I missed this comments when updating the patch.

jenswi-linaro · 2017-08-17T09:26:47Z

core/arch/arm/sm/pm.c

+	 * The content will be passed to cpu_do_resume
+	 * as register r1 and sp
+	 */
+	*ptr++ = 0; /* Not used now */


Use a proper struct for this instead.

I would not add a struct here. The layout will be refered in asm code, It is clear using current method.

It's not entirely clear to me.
We use structs at several places in the code that are accessed from assembly code, both for convenience and as documentation. There's core/arch/arm/kernel/asm-defines.c to help if needed.

jenswi-linaro · 2017-08-17T09:55:34Z

core/arch/arm/sm/suspend.S

+ */
+.align 2
+.global	suspend_reg_size
+.equ	suspend_reg_size, 4 * 16


Couldn't a regular define be used instead?

I'll give a try.

MrVan · 2017-08-19T04:19:40Z

@jenswi-linaro @etienne-lms Update with your review comments addressed. Please check whether this is ok for you. I have not do board test for the new review patch, later I'll test it.

jenswi-linaro · 2017-08-21T08:47:00Z

core/arch/arm/kernel/asm-defines.c

@@ -52,6 +53,14 @@ DEFINES
 	DEFINE(THREAD_SVC_REG_R0, offsetof(struct thread_svc_regs, r0));
 	DEFINE(THREAD_SVC_REG_R5, offsetof(struct thread_svc_regs, r5));
 	DEFINE(THREAD_SVC_REG_R6, offsetof(struct thread_svc_regs, r6));
+
+	DEFINE(PM_CTX_SP, offsetof(struct sm_pm_ctx, sp));


s/PM_CTX/SM_PM_CTX/ for these four lines

jenswi-linaro · 2017-08-21T08:55:39Z

core/arch/arm/sm/pm.c

+	 * as register r1 and sp
+	 */
+	ptr->sp = sp;
+	ptr->cpu_resume_addr = virt_to_phys(u.resume_addr);


If you'd cast as (void *)(vaddr_t)cpu_do_resume instead you wouldn't need to involve a union.

jenswi-linaro · 2017-08-21T09:06:32Z

core/arch/arm/sm/suspend.S

+	read_mpidr r1
+	and	r1, r1, #0x3
+
+	adr	r0, _suspend_sp


ldr r0, =suspend_sp instead?

If you we're using thread_core_local instead the sequence would be something like (and then we'd get rid of the assumption above too):

bl get_core_pos mul r0, r0, #THREAD_CORE_LOCAL_SIZE ldr r1, =thread_core_local add r1, r1, r0

At this point mmu is not enabled. bl get_core_pos could not be used here. I'll use thread_core_local here.

Isn't get_core_pos() flat mapped?

Edit:
By the way, get_core_pos() is called quite early in the boot before mmu is enabled.

I could not rely on physical address equals to virtual address. I could use bl get_core_pos here, but when virtual address is not same physical address, this needs to rewrite. I could add a piece code just as get thread_core_local phyiscal address to get get_core_pos physical address here.

jenswi-linaro · 2017-08-21T09:10:53Z

core/arch/arm/sm/suspend.S

+#include <keep.h>
+#include <kernel/unwind.h>
+
+#define PRRR			0x034


Unused defines?

jenswi-linaro · 2017-08-21T09:17:18Z

core/arch/arm/sm/suspend.S

+FUNC cpu_resume, :
+UNWIND(.fnstart)
+	/* Disable IRQ/FIQ */
+	mrs	r1, cpsr


cpsid if is only one instruction. I'd recommend disabling async aborts too, so then it would be cpsid aif.

jenswi-linaro · 2017-08-21T09:19:06Z

core/arch/arm/sm/suspend.S

+	write_sctlr r0
+	dsb
+	isb
+	nop


Any reference to why you need exactly three nops?

No specific reference, just would like to flush instruction pipe.

Why is that needed? Handling an erratum?

jenswi-linaro · 2017-08-21T09:43:02Z

core/arch/arm/sm/suspend.S

+	adr	lr, cpu_suspend_panic
+	/* Jump to arch specific suspend*/
+	pop	{r0, pc}
+	/* cpu not suspended */


Can this happen, considering that lr is pointing to cpu_suspend_panic()

This is bug here. This will be fixed in new version patches.

jenswi-linaro · 2017-08-21T09:47:29Z

core/arch/arm/sm/suspend.S

+	blx	cpu_suspend_save
+	adr	lr, cpu_suspend_panic
+	/* Jump to arch specific suspend*/
+	pop	{r0, pc}


Where is the value of pc supposed to be written?

cpu_suspend(arg, platform_suspend_func), Here pc is platform_suspend_func.

jenswi-linaro · 2017-08-21T09:50:01Z

core/arch/arm/sm/suspend.S

+	push	{r4, r5}
+	read_midr r4
+	ubfx	r5, r4, #4, #12
+	ldr	r4, =0xc07


This magic number could be a defined in <arm32.h> instead.
The assignment could also use mov_imm instead.

Using mov_imm, "Error: invalid constant (c07) after fixup"

MrVan · 2017-08-22T03:02:36Z

@jenswi-linaro I updated the patches, not review patch. I have tested it on mx7dsabresd, with suspend and dual-core low power idle, works well. I have switched to use thread_core_local and addresses a few bugs in the previous review patch which not tested.

jenswi-linaro · 2017-08-22T07:59:58Z

core/arch/arm/include/arm32.h

@@ -33,6 +33,8 @@
 #include <stdint.h>
 #include <util.h>

+#define A7_PART_NUM	0xC07


`s/A7_PART_NUM/CORTEX_A7_PART_NUM/
Please add

#define MIDR_PRIMARY_PART_NUM_SHIFT 4 #define MIDR_PRIMAGE_PART_NUM_WIDTH 12

To be able to get rid of the other two magic numbers in the assembly code concerning MIDR below.

jenswi-linaro · 2017-08-22T08:01:39Z

core/arch/arm/include/sm/pm.h

+};
+
+/* suspend/resume core functions */
+void cpu_suspend_init(void);


I'd prefer a "sm_pm" prefix over a "cpu" prefix for these functions.

This function is just a place holder, I could remove it, since not used now. Actually I would prefer cpu means this piece code is only related to cpu core, not about device in a SoC. This function will be removed in new version patches.

jenswi-linaro · 2017-08-22T08:04:02Z

core/arch/arm/sm/pm.c

+#include <sm/sm.h>
+#include <sm/pm.h>
+
+#include "../kernel/thread_private.h"


Please export struct thread_core_local in <thread.h> instead of including "../kernel/thread_private.h" here.

jenswi-linaro · 2017-08-22T08:07:37Z

core/arch/arm/sm/suspend.S

+
+FUNC cpu_v7_suspend, :
+UNWIND(.fnstart)
+	push	{r4 - r12, lr}


No need to save r12 and lr

r12 may not needed, but lr is needed. cpu_suspend_save->cpu_do_suspend->cpu_v7_suspend. lr is needed to back to cpu_suspend_save. r12 here is just make stack 8 bytes aligned.

I see no calls in this function

ok. I'll refine this.

jenswi-linaro · 2017-08-22T08:17:44Z

core/arch/arm/sm/suspend.S

+#include <kernel/unwind.h>
+
+.section .text
+FUNC cpu_suspend, :


Please add a comment describing arguments and return value of each exported function in this file.
Something like:

/* * int cpu_suspend(uint32_t arg, int (*fn)(uint32_t)); */

Is enough. It doesn't hurt to describe the local functions in the same manner but it's not as important.

jenswi-linaro · 2017-08-22T08:54:03Z

core/arch/arm/sm/suspend.S

+FUNC cpu_resume_mmu, :
+UNWIND(.fnstart)
+	ldr	r3, =cpu_resume_after_mmu
+	write_tlbiall


TLB was already invalidated in cpu_do_resume(), why is that needed again?

Not needed. I'll drop it.

jenswi-linaro · 2017-08-22T08:54:52Z

core/arch/arm/sm/suspend.S

+	nop
+	nop
+	nop
+	bx	r3


Why not just a simple b cpu_resume_after_mmu?

write_sctlr r0 will enable MMU. After mmu enabled, bx r3 to jump to cpu_resume_after_mmu.

The address you jump to will be the same so why make it complicated?

ok. I'll try it.

jenswi-linaro · 2017-08-22T08:56:09Z

core/arch/arm/sm/suspend.S

+UNWIND( .fnend)
+END_FUNC cpu_resume
+
+FUNC cpu_resume_after_mmu, :


LOCAL_FUNC

jenswi-linaro · 2017-08-22T08:56:52Z

core/arch/arm/sm/suspend.S

+UNWIND( .fnend)
+END_FUNC cpu_resume_after_mmu
+
+FUNC cpu_resume_mmu, :


LOCAL_FUNC

jenswi-linaro · 2017-08-22T08:57:36Z

core/arch/arm/sm/suspend.S

+UNWIND( .fnend)
+END_FUNC cpu_do_suspend
+
+FUNC cpu_resume, :


LOCAL_FUNC

MrVan · 2017-08-24T10:10:29Z

@jenswi-linaro Updated with a single review patch. I understand code is a bit hard to read, thanks for your detailed reviewing. Patch has tested on mx7dsabresd board, with suspend/resume low power idle and xtest.

MrVan · 2017-08-25T07:32:15Z

Any more comments on this? In my side, it passed 24 hours test on mx7dsabresd.

jenswi-linaro · 2017-08-25T07:43:31Z

I guess we're all waiting for the remaining patches to get the full picture.

MrVan · 2017-08-25T09:19:36Z

ok. I'll clean up platform specific code and send out as whole for review.

MrVan · 2017-08-28T05:50:11Z

Updated the patchset to include platform specific code. The PATCH 9/9 is not good enough to upstream, if ok, I would prefer other 8 patches go upstream and leave the last patch. welcome comments.

jbech-linaro · 2017-08-29T10:47:24Z

core/arch/arm/sm/suspend.S

+	ldr	r2, [r0]
+	add	r0, r0, r2
+
+	/* Assmue max 4 cpus and 1 cluster */


Since this is generic code, can't we try to support more cpus and more than 1 cluster from start? 4x2 seems pretty common these days.

@jbech-linaro This patchset is to support armv7 which mostly only contains 4 cores. Regarding armv8 aarch32, I do not have platforms to test. I could use simliar method as accessing thread_core_local here to use get_core_pos.

True, well I think this would serve as a good start and we can deal with that question later on. I'm not familiar with imx-specfics, but all in all I think this looks good, thanks!

jenswi-linaro · 2017-08-29T12:53:37Z

core/arch/arm/plat-imx/pm/imx7_suspend.c

+
+	/*
+	 * Store sec ctx regs
+	 * Now we are in SVC sec mode


Is this comment accurate? I can't find where the switch to SVC sec mode is done.

This should be monitor mode. Also in the asm mode switch code, it should be back to monitor code and save svc mode, as the following:

- "mov r3, #0x16\r\n" /* MON */ + "mov r3, #0x13\r\n" /* SVC */ "orr r3, r3, r4\r\n" "msr cpsr, r3\r\n" "mrs r1, spsr\r\n" "str r1, [r2], #4\r\n" "str sp, [r2], #4\r\n" "str lr, [r2], #4\r\n" - "msr cpsr, r0\r\n" /* SVC mode */ + "msr cpsr, r0\r\n" /* Mon mode */

When pushed to github, missed to fix this.

jenswi-linaro · 2017-08-29T13:34:24Z

core/arch/arm/plat-imx/pm/imx7_suspend.c

+	 * Store sec ctx regs
+	 * Now we are in SVC sec mode
+	 */
+	asm volatile (


This looks a lot like what sm_save_modes_regs() is doing, couldn't sm_save_modes_regs(&nsec->mode_regs) be used instead?

I'll check that. After reviewing patch 9/9, you could get the whole picture of how it works:)

jenswi-linaro · 2017-08-29T13:35:04Z

core/arch/arm/plat-imx/pm/imx7_suspend.c

+	regs = &reg_stack[0];
+
+	/*
+	 * Store sec ctx regs


It's actually nonsecure ctx regs.

non secure regs are saved by non-secure world by itself. This is to save secure ctx, because cpu lose power, all regs will be lost, so we need to save secure ctx.

The secure registers hasn't been loaded by the secure monitor yet so the content is actually non-secure, unless I'm missing something.

I misunderstand. yes. secure registers not loaded. This is save non-secure context. I'll correct the comment.

jenswi-linaro · 2017-08-29T13:35:51Z

core/arch/arm/plat-imx/pm/imx7_suspend.c

+	plat_cpu_reset_late();
+
+	/* Restore register of different mode in secure world */
+	asm volatile (


If sm_save_modes_regs() is used above, use sm_restore_modes_regs(&nsec->mode_regs) instead.

This is restore secure world context. Ditto, I'll check using sm_restore_modes_regs.

jenswi-linaro · 2017-08-29T13:52:35Z

core/arch/arm/plat-imx/pm/psci-suspend-imx7.S

+	str	r7, [r11, #0x38]
+wait_shutdown:
+	wfi
+	nop


Are the nop instructions really needed?

This similar code has been in FSL/NXP releases for long time. I do not want to remove them for potential risk here.

jenswi-linaro · 2017-08-29T13:54:42Z

core/arch/arm/plat-imx/pm/psci-suspend-imx7.S

+wfi:
+	/* Enter stop mode */
+	wfi
+	nop


Are all these nop instructions needed?

jenswi-linaro · 2017-08-29T13:55:23Z

core/arch/arm/plat-imx/pm/psci-suspend-imx7.S

+	mov	r4, #(BIT32(6) | BIT32(7))
+	orr	r3, r3, r4
+	msr	cpsr, r3
+	nop


nop for a particular reason?

No. I'll remove it.

jenswi-linaro · 2017-08-29T13:55:49Z

core/arch/arm/plat-imx/pm/psci-suspend-imx7.S

+	bne     _inv_nextway
+
+	dsb                             @ ; synchronise
+	nop


Wouldn't an isb instead of these two nop be more appropriate?

jenswi-linaro · 2017-08-29T13:58:51Z

core/arch/arm/plat-imx/pm/psci-suspend-imx7.S

+
+resume:
+	write_iciallu
+	write_bpiall


Should perhaps have dsb isb after this?

ok. will add them.

jenswi-linaro · 2017-08-29T13:59:48Z

core/arch/arm/plat-imx/pm/psci-suspend-imx7.S

+	isb
+
+	/* switch to monitor mode from svc mode */
+	mov	r3, #CPSR_MODE_MON


cps #CPSR_MODE_MON instead of this long sequence?

MrVan · 2017-08-30T06:25:43Z

@jbech-linaro Updated with using get_core_pos in suspend.S @jenswi-linaro Updated with addressing comments for psci-suspend-imx7.S.

Sending out the imx specific things to let people could get a whole picture, but here the first thing I would move forward is upstreaming the common parts, saying patch 1/9-8/9. I am glad, if you think the last patch 9/9 is good to be accepted. In next version patch, I will extract the psci macros into standalone patch from patch 9/9.

jenswi-linaro · 2017-08-30T06:38:10Z

core/arch/arm/plat-imx/pm/imx7_suspend.c

-		: "r0", "r1", "r2", "r3", "r4"
-		);
+	/* Store non-sec ctx regs */
+	sm_save_modes_regs(&mode_regs[0]);


&nsec->mode_regs is not used at this stage and could be used instead of the local variable mode_regs.

Thanks. I'll use that in next version patch after receiving more comments.

etienne-lms · 2017-08-25T07:48:42Z

core/arch/arm/sm/suspend.S

+
+FUNC cpu_do_suspend, :
+UNWIND(.fnstart)
+	push	{r4, r5}


Either UNWIND(.cantunwind) above or add UNWIND(.save {r4, r5}) below.

etienne-lms · 2017-08-25T09:14:59Z

core/arch/arm/sm/suspend.S

+	add	r0, sp, #8
+	blx	cpu_suspend_save
+	adr	lr, aborted
+	/* Jump to arch specific suspend*/


minor: space before */

etienne-lms · 2017-08-25T09:23:46Z

core/arch/arm/sm/suspend.S

+	push	{r4 - r12, lr}
+UNWIND(.save	{r4 - r12, lr})
+	mov	r5, sp
+	sub	sp, sp, #SM_PM_CTX_SIZE


unwinding sequence will get lost with this, no?

I do not have much knowledge about unwind. Here it may lost. I would not like to change the design here.

We should replace .save {r4 - r12, lr} with .cantunwind

etienne-lms · 2017-08-25T09:24:54Z

core/arch/arm/sm/suspend.S

+END_FUNC cpu_suspend
+
+LOCAL_FUNC cpu_v7_suspend, :
+UNWIND(.fnstart)


UNWIND(.fnstart) or UNWIND(.cantunwind) ?

etienne-lms · 2017-08-30T08:25:01Z

core/arch/arm/include/mm/core_mmu.h

@@ -481,6 +481,8 @@ static inline TEE_Result cache_op_outer(enum cache_op op __unused,
 /* Check cpu mmu enabled or not */
 bool cpu_mmu_enabled(void);

+void map_memarea_sections(const struct tee_mmap_region *mm, uint32_t *ttb);


This is core_mmu_v7.c, specific not LPAE. A generic cpu_mmu_map_memarea() could direct to the mmu_v7/lpae map functions. Lpae would be not supported at first I guess.
minor: can add a description since the function is now exported.

I do not have platforms with LPAE enabled. I'll add a description.

etienne-lms · 2017-08-30T08:28:48Z

core/arch/arm/plat-imx/conf.mk

@@ -78,4 +78,8 @@ $(call force,CFG_SECURE_TIME_SOURCE_REE,y)
 CFG_BOOT_SECONDARY_REQUEST ?= y
 endif

+ifeq ($(filter y, $(CFG_PSCI_ARM32)), y)
+CFG_CORE_RWDATA_NOEXEC ?= n
+endif


I recommend CFG_HWSUPP_MEM_PERM_WXN=n and keep CFG_CORE_RWDATA_NOEXEC=y.
The later insures optee_os mapping splits executable and data, and the former does not enforce write-implies-noexec from SCTLR.

ok. I'll use CFG_HWSUPP_MEM_PERM_WXN ?= n in next version.

etienne-lms · 2017-08-30T08:33:12Z

core/arch/arm/plat-imx/pm/pm-imx7.c

+	}
+
+	/* Note IRAM_S_BASE is not 1M aligned, so take care */
+	map.pa = ROUNDDOWN(IRAM_S_BASE, 0x100000);


minor: s/0x100000/CORE_MMU_PGDIR_SIZE/, here and below.

fix in next version.

etienne-lms · 2017-08-30T08:38:08Z

core/arch/arm/plat-imx/pm/pm-imx7.c

+	uint32_t (*ddrc_phy_offset_array)[2];
+	uint32_t suspend_ocram_base = (uint32_t)phys_to_virt(
+			(paddr_t)TRUSTZONE_OCRAM_START +
+			SUSPEND_OCRAM_OFFSET, MEM_AREA_TEE_COHERENT);


Since this is coherent memory, we can expect phys=virt. Maybe use phys_to_virt() only under an assert().

I could not get you. Could you please explain more?

jenswi-linaro · 2017-08-30T07:19:19Z

core/arch/arm/sm/suspend.S

+	isb
+	mov	r0, #0
+	/* Back to caller of cpu_suspend, with return value 0 */
+	pop	{r4 - r12, pc}


This depends on the push in cpu_suspend() above. It should be noted somehow. Perhaps we could just jump to the pop instruction in cpu_suspend()?

This is back to the next instruction just after saying bl cpu_suspend. The pop instruction in cpu_suspend is that cpu not suspended. Here the pop in cpu_resume path is cpu really suspened. They are in different code path. Let's not use jump to mix the two path.

jenswi-linaro · 2017-08-30T07:34:16Z

core/arch/arm/sm/suspend.S

+
+FUNC cpu_resume_mmu, :
+UNWIND(.fnstart)
+	isb


isb not needed, done just before jumping here.
I'm not sure we need this function at all since it doesn't do that much. I think the code would be easier to follow/understand if cpu_do_suspend() and cpu_do_resume() did match each other more. The helper functions cpu_v7_suspend() and cpu_resume_mmu() changes the symmetry more than isolating different problems.

write_sctlr r0 will turn on mmu. To keep safe, I prefer we keep isb here. cpu_resume_mmu here is to turn on mmu, and run in virtual address, before cpu_resume_mmu, all is runing in physical address. cpu_v7_suspend is common for armv7 here. But regarding a7/a9/a8/a15 and etc, cpu_do_suspend could handle. I think better to keep them.

MrVan · 2017-09-11T03:27:32Z

@etienne-lms Updated. struct thread_core_local from thread_private.h to thread.h was done in a specific patch; r2 and r3 are not removed in psci-suspend-imx7.S at the beginning, they are not used now; add comments for _core_pos; sorted the includes for psci-sus-end-imx7.S.
Tested on mx7dsabresd board.
Thanks for revewing.

etienne-lms

Some more minor comments + check bp cache sync before restoring sctlr (requitred on c-a9).

Comments on commit messages:

2nd commit "core: arm: psci: pass nsec ctx to psci" rephrase proposal (expands the shortcuts)
Pass non-secure context to psci functions. When cpu/system suspends,
cpu may loose power, so when back to linux from tee, tee
needs to return to a linux resume point, not the usual return address
after "smc" instruction. So we need to modify the mon_lr
value in non-secure context.

~~The function in linux side is psci_system_suspend or~~
~~psci_ops.cpu_suspend(state, __pa(cpu_resume)).~~ (remove this part)

Psci runs in monitor mode, sm_get_nsec_ctx can not be used,
so pass the non-secure context pointer to the psci suspend function.
commit3: "core: arm: psci: add suspend resume common functions"
s/platform/Platform/
s/that not losing/that is not loosing/

Last sentence: The "arg" passed to "func" as "r0"
Proposal: Argument "arg" is passed to function "func" as argument through register "r0".
core: imx: simplify code
s/registeration/registration/

etienne-lms · 2017-09-11T09:29:23Z

core/arch/arm/include/sm/psci.h

@@ -1,5 +1,6 @@
 #include <kernel/thread.h>
 #include <stdint.h>
+#include <sm/sm.h>


minor: move before #include <stdint.h>`

etienne-lms · 2017-09-11T09:31:36Z

core/arch/arm/include/kernel/thread.h

+
+#ifdef ARM64
+/*
+ * struct thread_core_local need to have alignment suitable for a stack


minor: s/need/needs/

This is just moving code from thread_private.h to thread.h. Anyway I'll fix it.

etienne-lms · 2017-09-11T09:40:00Z

core/arch/arm/sm/pm.c

+#include <stdint.h>
+#include <sm/optee_smc.h>
+#include <sm/psci.h>
+#include <sm/sm.h>


minor: fix include order:

#include <sm/optee_smc.h> #include <sm/psci.h> #include <sm/pm.h> ... moved up #include <sm/sm.h> #include <stdint.h> ... moved down

etienne-lms · 2017-09-11T09:41:53Z

core/arch/arm/sm/pm.c

+	ctx->cpu_resume_addr =
+		virt_to_phys((void *)(vaddr_t)sm_pm_cpu_do_resume);
+
+	sm_pm_cpu_do_suspend(&ctx->suspend_regs[0]);


minor: s/&ctx->suspend_regs[0]/ctx->suspend_regs/

etienne-lms · 2017-09-11T09:43:30Z

core/arch/arm/sm/pm_a32.S

+#include <kernel/asan.h>
+#include <platform_config.h>
+#include <keep.h>
+#include <kernel/unwind.h>


minor: remove #include <kernel/unwind.h> at list top and fix includes ordering.

etienne-lms · 2017-09-11T09:58:04Z

core/arch/arm/sm/pm_a32.S

+	isb
+	dsb
+	/* MMU will be enabled here */
+	write_sctlr r8


This will enable branch prediction. Needs a write_bpiall before dsb/isb ?

This was done psci-suspend-imx7.S, not in this pm_a32.S, is this ok?

From psci-suspend-imx7.S,

if cores stay up (standby case): the sequence enables bp from imx7_suspend, then enables mmu here. I think bp should be enabled only after mmu is enabled (for cores requiring a manul enable of bp).

In cpu shutdown case, cpu wakes to resume. In this case, it seems to me that both mmu and bp will be both enables from here write_sctlr r8, without previous bp cache cleanup (write_bpiall).

If cores stay up, it will not run into cpu resume. In psci-suspend-imx7.S, there are two wfi. If cpu stay up, cpu will directly runs into the following instruction of wfi.

If cpu shutdown, write_sctlr will restore branch predictor. In psci-suspend-imx7.S, line 645. If you think it is required to add a write_bpiall here before dsb/isb, I could add one here to handle the case that platform specific code missed to add write_bpiall in resume path.

etienne-lms · 2017-09-11T10:00:29Z

core/arch/arm/sm/pm_a32.S

+
+FUNC sm_pm_cpu_do_suspend, :
+UNWIND(.fnstart)
+UNWIND(.cantunwind)


minor: for consistency, add a tab after UNWIND(, here and there.

etienne-lms · 2017-09-11T10:02:14Z

core/arch/arm/plat-imx/imx7.c

@@ -85,7 +85,7 @@ void plat_cpu_reset_late(void)
 	write32(0x00FF0033, core_mmu_get_va(CSU_CSL_15, MEM_AREA_IO_SEC));
 	/*
 	 * Proect SRC


maybe s/Proect/Protect/

etienne-lms · 2017-09-11T10:04:14Z

core/arch/arm/plat-imx/main.c

 register_phys_mem(MEM_AREA_IO_SEC, ANATOP_BASE, CORE_MMU_DEVICE_SIZE);
+#endif
+#ifdef GICD_BASE


minor: GIC_BASE is likely to cover GICC and GICD.

On i.MX8, there is GICD and GICR, no GICC used here. Since this piece code is shared to i.mx family. I seperated them.

etienne-lms · 2017-09-11T10:11:34Z

core/arch/arm/plat-imx/pm/imx7_suspend.c

+	if (!suspended_init) {
+		imx7_suspend_init();
+		suspended_init = 1;
+	}


Minor: imx7_suspend_init() could be registered through init_service().

I would not change it now in this patchset. Later with new patches, I could change it and do a new round stress test.

MrVan · 2017-09-12T12:35:04Z

Update the commit messages according to @etienne-lms 's suggestion. Reordered the include files. need->needs in thread.h Use ctx->suspend_regs to replace &ctx->suspend_regs[0] in pm.c. using tab in UNWIND. Add a write_bpiall before enabling mmu in resume path. And a few minor fixes.

Thanks for reviewing.

MrVan · 2017-09-15T01:07:54Z

It has been added in the new patchset. 2017-09-14 21:13 GMT+08:00 Andrew F. Davis <[email protected]>:

…

***@***.**** commented on this pull request. ------------------------------ In core/arch/arm/sm/pm_a32.S <#1729 (comment)>: > + write_ttbcr r7 +#endif + + ldmia r0, {r4 - r7} + write_prrr r4 + write_nmrr r5 + write_vbar r6 + write_nsacr r7 + + write_actlr r9 + write_cpacr r10 + write_mvbar r11 + isb + dsb + /* MMU will be enabled here */ + write_sctlr r8 I think it makes the most sense to add the write_bpiall here as the branch predictor is enabled here. This will reduce the number of needed assumptions on the platform specific code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1729 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECMaE8vP1hdZQRc0xlNuN-oLKXZh_4Wks5siSZigaJpZM4Oqhq-> .

MrVan · 2017-09-15T01:19:34Z

@jenswi-linaro @etienne-lms do you have more comments? Is this patchset ok to have your r-b/a-b tags?

jenswi-linaro

With my minor comments addressed:

For commits that changes below core/arch/arm/plat-imx:
Acked-by: Jens Wiklander <[email protected]>
For the other changes:
Reviewed-by: Jens Wiklander <[email protected]>

jenswi-linaro · 2017-09-15T06:18:31Z

core/arch/arm/include/kernel/thread.h

@@ -279,6 +280,8 @@ struct thread_handlers {
 void thread_init_primary(const struct thread_handlers *handlers);
 void thread_init_per_cpu(void);

+struct thread_core_local *thread_get_core_local(void);


This change should go into the "core: arm: kernel: make thread_core_local public" commit

jenswi-linaro · 2017-09-15T06:18:59Z

core/arch/arm/kernel/thread.c

@@ -89,7 +89,7 @@

 struct thread_ctx threads[CFG_NUM_THREADS];

-static struct thread_core_local thread_core_local[CFG_TEE_CORE_NB_CORE];
+struct thread_core_local thread_core_local[CFG_TEE_CORE_NB_CORE];


This change should go into the "core: arm: kernel: make thread_core_local public" commit

jenswi-linaro · 2017-09-15T06:19:07Z

core/arch/arm/kernel/thread_private.h

@@ -163,8 +163,6 @@ void thread_init_vbar(void);
 /* Handles a stdcall, r0-r7 holds the parameters */
 void thread_std_smc_entry(void);

-struct thread_core_local *thread_get_core_local(void);


This change should go into the "core: arm: kernel: make thread_core_local public" commit

Thanks @jenswi-linaro. I'll fix this, then add your tags.

MrVan · 2017-09-15T12:07:59Z

"struct thread_core_local *thread_get_core_local(void);" changes moved to "core: arm: kernel: make thread_core_local public" commit.
Rebased to master
Added r-b/a-b tags

MrVan · 2017-09-16T00:13:49Z

Missed one comment from @jenswi-linaro in last patchset:

-static struct thread_core_local thread_core_local[CFG_TEE_CORE_NB_CORE];
+struct thread_core_local thread_core_local[CFG_TEE_CORE_NB_CORE];

This change has been address in lastest patchset, with TAGS applied. Thanks.

etienne-lms

Checkpatch warn on (a fix from a comment of mine)

commit3: "core: arm: psci: add suspend resume common functions"
WARNING: 'loosing' may be misspelled - perhaps 'losing'?
=> s/that not loosing/that is not losing/

reviewed-by
core: mmu: export map_memarea_sections
core: arm: psci: pass nsec ctx to psci
core: arm: kernel: make thread_core_local public
core: arm: psci: add suspend resume common functions (see last comment)
core: arm: imx: move psci code to pm
core: arm: sm: add psci power state macros

acked-by
core: imx7: fix comments
core: imx: simplify code
core: arm: imx: get mmdc type
core: arm: imx7d: remove soc_is_imx7d/s functions
core: arm: imx7d: add psci suspend support

etienne-lms · 2017-09-18T07:52:43Z

core/arch/arm/kernel/asm-defines.c

+	DEFINE(THREAD_CORE_LOCAL_SIZE, sizeof(struct thread_core_local));
+
+	DEFINE(SM_PM_CTX_SP, offsetof(struct sm_pm_ctx, sp));
+	DEFINE(SM_PM_CTX_RESUME_ADDR,


minor: SM_PM_CTX_SP, SM_PM_CTX_RESUME_ADDR and SM_PM_CTX_SUSPEND_ADDR and not used.
SM_PM_CTX_SUSPEND_ADDR store the resume address. Maybe just remove these defines.

ok, I'll remove them.

Export map_memarea_sections. We need a mmu table dedicated for low power feature, so export map_memarea_sections to create that section mapping. Signed-off-by: Peng Fan <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Pass non-secure context to psci functions. When cpu/system suspends, cpu may loose power, so when back to linux from tee, tee needs to return to a linux resume point, not the usual return address after "smc" instruction. So we need to modify the mon_lr value in non-secure context. Psci runs in monitor mode, sm_get_nsec_ctx can not be used, so pass the non-secure context pointer to the psci suspend function. Signed-off-by: Peng Fan <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Move the struture of thread_core_local from thread_private.h to thread.h to make it public. Signed-off-by: Peng Fan <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Add cpu suspend/resume common functions. Platform psci suspend functions need to call sm_pm_cpu_suspend(arg, platform_suspend) to runs into suspend. The i.MX flow is: psci_cpu_suspend->imx7_cpu_suspend->sm_pm_cpu_suspend(arg, func) The "func" runs in on-chip ram that not losing power when system runs into suspend or low power state. Argument "arg" is passed to function "func" as argument through register "r0". Signed-off-by: Peng Fan <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Move psci code to pm. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Fix comments. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Wrap memory registration using macros to make it easy to add new soc/arch support. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Add get mmdc type support, this will be used when configuring ddr into self refresh for low power feature. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Add PSCI_POWER_STATE_X macros Signed-off-by: Peng Fan <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

Remove soc_is_imx7d/s functions. Not needed. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

Implement i.MX7D suspend/resume support. When the first time runs into suspend, some initialization work needs to be done, such as code copy, iram translation table. Since we only have 32K on chip RAM for suspend/resume usage, we have to put code and data together and use section mapping and WXN is set to false. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

MrVan · 2017-09-18T09:23:22Z

checkpatch warning fixed for commit3; uneeded SM_PM_CTX_xx macros removed.
Applied @etienne-lms 's r-b/a-b tags;

jforissier · 2017-09-18T12:52:28Z

@MrVan thanks for this valuable contribution.

jbech-linaro · 2017-09-18T12:58:51Z

@MrVan thanks for this valuable contribution.

Indeed, thanks a lot for all the patience here!

igoropaniuk reviewed Aug 8, 2017

View reviewed changes

MrVan force-pushed the lpm branch from 4e8ff5c to 4f37139 Compare August 9, 2017 09:28

MrVan force-pushed the lpm branch from 4f37139 to 2040815 Compare August 11, 2017 00:50

etienne-lms reviewed Aug 14, 2017

View reviewed changes

jenswi-linaro reviewed Aug 17, 2017

View reviewed changes

jenswi-linaro reviewed Aug 21, 2017

View reviewed changes

MrVan force-pushed the lpm branch 3 times, most recently from 6a32dcf to 5b54219 Compare August 22, 2017 02:55

jenswi-linaro reviewed Aug 22, 2017

View reviewed changes

MrVan force-pushed the lpm branch from 76c6162 to 42d55b9 Compare August 28, 2017 05:36

jbech-linaro reviewed Aug 29, 2017

View reviewed changes

jenswi-linaro reviewed Aug 29, 2017

View reviewed changes

jenswi-linaro reviewed Aug 30, 2017

View reviewed changes

etienne-lms reviewed Aug 30, 2017

View reviewed changes

jenswi-linaro reviewed Aug 30, 2017

View reviewed changes

etienne-lms reviewed Sep 12, 2017

View reviewed changes

MrVan force-pushed the lpm branch from aab308b to cf42d13 Compare September 12, 2017 12:30

jenswi-linaro reviewed Sep 15, 2017

View reviewed changes

MrVan force-pushed the lpm branch from cf42d13 to a9af470 Compare September 15, 2017 09:49

MrVan force-pushed the lpm branch from a9af470 to e22beee Compare September 16, 2017 00:10

etienne-lms reviewed Sep 18, 2017

View reviewed changes

MrVan added 11 commits September 18, 2017 16:27

core: arm: kernel: make thread_core_local public

3ddbb7f

Move the struture of thread_core_local from thread_private.h to thread.h to make it public. Signed-off-by: Peng Fan <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

core: arm: imx: move psci code to pm

8ca9edc

Move psci code to pm. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

core: imx7: fix comments

8af2207

Fix comments. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

core: imx: simplify code

382e20e

Wrap memory registration using macros to make it easy to add new soc/arch support. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

core: arm: imx: get mmdc type

45998b6

Add get mmdc type support, this will be used when configuring ddr into self refresh for low power feature. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

core: arm: sm: add psci power state macros

27009ca

Add PSCI_POWER_STATE_X macros Signed-off-by: Peng Fan <[email protected]> Reviewed-by: Jens Wiklander <[email protected]> Reviewed-by: Etienne Carriere <[email protected]>

core: arm: imx7d: remove soc_is_imx7d/s functions

5a4b957

Remove soc_is_imx7d/s functions. Not needed. Signed-off-by: Peng Fan <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

MrVan force-pushed the lpm branch from e22beee to 8e34262 Compare September 18, 2017 09:15

jforissier merged commit 1295874 into OP-TEE:master Sep 18, 2017

glneo mentioned this pull request Sep 18, 2017

AM43xx Suspend/Resume #1822

Merged

MrVan deleted the lpm branch September 19, 2017 00:51

[RFC] arm32: introduce suspend/resume common functions #1729

[RFC] arm32: introduce suspend/resume common functions #1729

Conversation

MrVan commented Aug 2, 2017

MrVan commented Aug 8, 2017

igoropaniuk Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Aug 10, 2017

MrVan commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Aug 24, 2017

MrVan commented Aug 25, 2017

jenswi-linaro commented Aug 25, 2017

igoropaniuk Aug 8, 2017 •

edited

Loading

jenswi-linaro Aug 22, 2017 •

edited

Loading