[U-Boot] [PATCH v2 00/12] Add PSCI support for Jetson TK1/Tegra124 + CNTFRQ fix

[U-Boot] [PATCH v2 00/12] Add PSCI support for Jetson TK1/Tegra124 + CNTFRQ fix

[Jan Kiszka]

Version 2 introduces CPU-local target PC (power-up vector), as pointed
out by Chen-Yu. It also includes a new patch to properly set CNTFRQ on
all Tegra124 CPUs, not only CPU0. This finally allows to run KVM guests
on TK1. The series was tested on the Banana Pi as well.

The patches are also available here:

https://github.com/siemens/u-boot/tree/jetson-tk1-v2

Jan


CC: Ian Campbell <ijc@hellion.org.uk>

Ian Campbell (4):
  tegra124: Add more registers to struct mc_ctlr
  virt-dt: Allow reservation of the secure region when it is in a RAM
    carveout.
  jetson-tk1: Add PSCI configuration options and reserve secure code
  tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

Jan Kiszka (8):
  ARM: Factor out reusable psci_cpu_off_common
  ARM: Factor out reusable psci_cpu_entry
  ARM: Factor out reusable psci_get_cpu_stack_top
  ARM: Put target PC for PSCI CPU_ON on per-CPU stack
  tegra: Make tegra_powergate_power_on public
  tegra: Add ap_pm_init hook
  tegra124: Add PSCI support for Tegra124
  tegra: Set CNTFRQ for secondary CPUs

 arch/arm/cpu/armv7/Makefile                 |   1 +
 arch/arm/cpu/armv7/psci.S                   | 100 ++++++++++++++++++++++++
 arch/arm/cpu/armv7/sunxi/psci.S             | 108 ++++----------------------
 arch/arm/cpu/armv7/tegra-common/Makefile    |   1 +
 arch/arm/cpu/armv7/tegra-common/psci.S      | 114 ++++++++++++++++++++++++++++
 arch/arm/cpu/armv7/tegra124/Kconfig         |   2 +
 arch/arm/cpu/armv7/tegra124/Makefile        |   7 ++
 arch/arm/cpu/armv7/tegra124/ap.c            |  44 +++++++++++
 arch/arm/cpu/armv7/virt-dt.c                |   5 ++
 arch/arm/cpu/armv7/virt-v7.c                |   5 ++
 arch/arm/cpu/tegra-common/ap.c              |  15 ++++
 arch/arm/cpu/tegra-common/powergate.c       |   2 +-
 arch/arm/include/asm/arch-tegra/ap.h        |   5 ++
 arch/arm/include/asm/arch-tegra/powergate.h |   1 +
 arch/arm/include/asm/arch-tegra124/flow.h   |   5 ++
 arch/arm/include/asm/arch-tegra124/mc.h     |  35 ++++++++-
 arch/arm/include/asm/system.h               |   1 +
 board/nvidia/common/board.c                 |   4 +
 include/configs/jetson-tk1.h                |   5 ++
 19 files changed, 364 insertions(+), 96 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/tegra-common/psci.S
 create mode 100644 arch/arm/cpu/armv7/tegra124/Makefile
 create mode 100644 arch/arm/cpu/armv7/tegra124/ap.c

-- 
2.1.4

[U-Boot] [PATCH v2 01/12] ARM: Factor out reusable psci_cpu_off_common

[Jan Kiszka]

Move parts of sunxi's psci_cpu_off into psci_cpu_off_common, namely
cache disabling and flushing, clrex and the disabling of SMP for the
dying CPU. These steps are apparently generic for ARMv7 and will be
reused for Tegra124 support.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/psci.S       | 71 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/cpu/armv7/sunxi/psci.S | 63 +-----------------------------------
 2 files changed, 72 insertions(+), 62 deletions(-)

diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
index bf11a34..d688607 100644
--- a/arch/arm/cpu/armv7/psci.S
+++ b/arch/arm/cpu/armv7/psci.S
@@ -99,4 +99,75 @@ _smc_psci:
 	pop	{r4-r7, lr}
 	movs	pc, lr			@ Return to the kernel
 
+/* Imported from Linux kernel */
+LENTRY(v7_flush_dcache_all)
+	dmb					@ ensure ordering with previous memory accesses
+	mrc	p15, 1, r0, c0, c0, 1		@ read clidr
+	ands	r3, r0, #0x7000000		@ extract loc from clidr
+	mov	r3, r3, lsr #23			@ left align loc bit field
+	beq	finished			@ if loc is 0, then no need to clean
+	mov	r10, #0				@ start clean at cache level 0
+flush_levels:
+	add	r2, r10, r10, lsr #1		@ work out 3x current cache level
+	mov	r1, r0, lsr r2			@ extract cache type bits from clidr
+	and	r1, r1, #7			@ mask of the bits for current cache only
+	cmp	r1, #2				@ see what cache we have at this level
+	blt	skip				@ skip if no cache, or just i-cache
+	mrs     r9, cpsr			@ make cssr&csidr read atomic
+	mcr	p15, 2, r10, c0, c0, 0		@ select current cache level in cssr
+	isb					@ isb to sych the new cssr&csidr
+	mrc	p15, 1, r1, c0, c0, 0		@ read the new csidr
+	msr     cpsr_c, r9
+	and	r2, r1, #7			@ extract the length of the cache lines
+	add	r2, r2, #4			@ add 4 (line length offset)
+	ldr	r4, =0x3ff
+	ands	r4, r4, r1, lsr #3		@ find maximum number on the way size
+	clz	r5, r4				@ find bit position of way size increment
+	ldr	r7, =0x7fff
+	ands	r7, r7, r1, lsr #13		@ extract max number of the index size
+loop1:
+	mov	r9, r7				@ create working copy of max index
+loop2:
+	orr	r11, r10, r4, lsl r5		@ factor way and cache number into r11
+	orr	r11, r11, r9, lsl r2		@ factor index number into r11
+	mcr	p15, 0, r11, c7, c14, 2		@ clean & invalidate by set/way
+	subs	r9, r9, #1			@ decrement the index
+	bge	loop2
+	subs	r4, r4, #1			@ decrement the way
+	bge	loop1
+skip:
+	add	r10, r10, #2			@ increment cache number
+	cmp	r3, r10
+	bgt	flush_levels
+finished:
+	mov	r10, #0				@ swith back to cache level 0
+	mcr	p15, 2, r10, c0, c0, 0		@ select current cache level in cssr
+	dsb	st
+	isb
+	bx	lr
+ENDPROC(v7_flush_dcache_all)
+
+ENTRY(psci_cpu_off_common)
+	push	{lr}
+
+	mrc	p15, 0, r0, c1, c0, 0		@ SCTLR
+	bic	r0, r0, #(1 << 2)		@ Clear C bit
+	mcr	p15, 0, r0, c1, c0, 0		@ SCTLR
+	isb
+	dsb
+
+	bl	v7_flush_dcache_all
+
+	clrex					@ Why???
+
+	mrc	p15, 0, r0, c1, c0, 1		@ ACTLR
+	bic	r0, r0, #(1 << 6)		@ Clear SMP bit
+	mcr	p15, 0, r0, c1, c0, 1		@ ACTLR
+	isb
+	dsb
+
+	pop	{lr}
+	bx	lr
+ENDPROC(psci_cpu_off_common)
+
 	.popsection
diff --git a/arch/arm/cpu/armv7/sunxi/psci.S b/arch/arm/cpu/armv7/sunxi/psci.S
index 5be497b..6785fdd 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.S
+++ b/arch/arm/cpu/armv7/sunxi/psci.S
@@ -199,53 +199,6 @@ psci_cpu_on:
 _target_pc:
 	.word	0
 
-/* Imported from Linux kernel */
-v7_flush_dcache_all:
-	dmb					@ ensure ordering with previous memory accesses
-	mrc	p15, 1, r0, c0, c0, 1		@ read clidr
-	ands	r3, r0, #0x7000000		@ extract loc from clidr
-	mov	r3, r3, lsr #23			@ left align loc bit field
-	beq	finished			@ if loc is 0, then no need to clean
-	mov	r10, #0				@ start clean at cache level 0
-flush_levels:
-	add	r2, r10, r10, lsr #1		@ work out 3x current cache level
-	mov	r1, r0, lsr r2			@ extract cache type bits from clidr
-	and	r1, r1, #7			@ mask of the bits for current cache only
-	cmp	r1, #2				@ see what cache we have@this level
-	blt	skip				@ skip if no cache, or just i-cache
-	mrs     r9, cpsr			@ make cssr&csidr read atomic
-	mcr	p15, 2, r10, c0, c0, 0		@ select current cache level in cssr
-	isb					@ isb to sych the new cssr&csidr
-	mrc	p15, 1, r1, c0, c0, 0		@ read the new csidr
-	msr     cpsr_c, r9
-	and	r2, r1, #7			@ extract the length of the cache lines
-	add	r2, r2, #4			@ add 4 (line length offset)
-	ldr	r4, =0x3ff
-	ands	r4, r4, r1, lsr #3		@ find maximum number on the way size
-	clz	r5, r4				@ find bit position of way size increment
-	ldr	r7, =0x7fff
-	ands	r7, r7, r1, lsr #13		@ extract max number of the index size
-loop1:
-	mov	r9, r7				@ create working copy of max index
-loop2:
-	orr	r11, r10, r4, lsl r5		@ factor way and cache number into r11
-	orr	r11, r11, r9, lsl r2		@ factor index number into r11
-	mcr	p15, 0, r11, c7, c14, 2		@ clean & invalidate by set/way
-	subs	r9, r9, #1			@ decrement the index
-	bge	loop2
-	subs	r4, r4, #1			@ decrement the way
-	bge	loop1
-skip:
-	add	r10, r10, #2			@ increment cache number
-	cmp	r3, r10
-	bgt	flush_levels
-finished:
-	mov	r10, #0				@ swith back to cache level 0
-	mcr	p15, 2, r10, c0, c0, 0		@ select current cache level in cssr
-	dsb	st
-	isb
-	bx	lr
-
 _sunxi_cpu_entry:
 	@ Set SMP bit
 	mrc	p15, 0, r0, c1, c0, 1
@@ -262,21 +215,7 @@ _sunxi_cpu_entry:
 
 .globl	psci_cpu_off
 psci_cpu_off:
-	mrc	p15, 0, r0, c1, c0, 0		@ SCTLR
-	bic	r0, r0, #(1 << 2)		@ Clear C bit
-	mcr	p15, 0, r0, c1, c0, 0		@ SCTLR
-	isb
-	dsb
-
-	bl	v7_flush_dcache_all
-
-	clrex					@ Why???
-
-	mrc	p15, 0, r0, c1, c0, 1		@ ACTLR
-	bic	r0, r0, #(1 << 6)		@ Clear SMP bit
-	mcr	p15, 0, r0, c1, c0, 1		@ ACTLR
-	isb
-	dsb
+	bl	psci_cpu_off_common
 
 	@ Ask CPU0 to pull the rug...
 	movw	r0, #(GICD_BASE & 0xffff)
-- 
2.1.4

[U-Boot] [PATCH v2 02/12] ARM: Factor out reusable psci_cpu_entry

[Jan Kiszka]

_sunxi_cpu_entry can be converted completely into a reusable
psci_cpu_entry. Tegra124 will use it as well.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/psci.S       | 19 +++++++++++++++++++
 arch/arm/cpu/armv7/sunxi/psci.S | 21 ++-------------------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
index d688607..e916d71 100644
--- a/arch/arm/cpu/armv7/psci.S
+++ b/arch/arm/cpu/armv7/psci.S
@@ -170,4 +170,23 @@ ENTRY(psci_cpu_off_common)
 	bx	lr
 ENDPROC(psci_cpu_off_common)
 
+ENTRY(psci_cpu_entry)
+	@ Set SMP bit
+	mrc	p15, 0, r0, c1, c0, 1		@ ACTLR
+	orr	r0, r0, #(1 << 6)		@ Set SMP bit
+	mcr	p15, 0, r0, c1, c0, 1		@ ACTLR
+	isb
+
+	bl	_nonsec_init
+	bl	psci_arch_init
+
+	adr	r0, _psci_target_pc
+	ldr	r0, [r0]
+	b	_do_nonsec_entry
+ENDPROC(psci_cpu_entry)
+
+.globl _psci_target_pc
+_psci_target_pc:
+	.word	0
+
 	.popsection
diff --git a/arch/arm/cpu/armv7/sunxi/psci.S b/arch/arm/cpu/armv7/sunxi/psci.S
index 6785fdd..c3a8dc1 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.S
+++ b/arch/arm/cpu/armv7/sunxi/psci.S
@@ -138,7 +138,7 @@ out:	mcr	p15, 0, r7, c1, c1, 0
 	@ r2 = target PC
 .globl	psci_cpu_on
 psci_cpu_on:
-	adr	r0, _target_pc
+	ldr	r0, =_psci_target_pc
 	str	r2, [r0]
 	dsb
 
@@ -150,7 +150,7 @@ psci_cpu_on:
 	mov	r4, #1
 	lsl	r4, r4, r1
 
-	adr	r6, _sunxi_cpu_entry
+	ldr	r6, =psci_cpu_entry
 	str	r6, [r0, #0x1a4] @ PRIVATE_REG (boot vector)
 
 	@ Assert reset on target CPU
@@ -196,23 +196,6 @@ psci_cpu_on:
 	mov	r0, #ARM_PSCI_RET_SUCCESS	@ Return PSCI_RET_SUCCESS
 	mov	pc, lr
 
-_target_pc:
-	.word	0
-
-_sunxi_cpu_entry:
-	@ Set SMP bit
-	mrc	p15, 0, r0, c1, c0, 1
-	orr	r0, r0, #0x40
-	mcr	p15, 0, r0, c1, c0, 1
-	isb
-
-	bl	_nonsec_init
-	bl	psci_arch_init
-
-	adr	r0, _target_pc
-	ldr	r0, [r0]
-	b	_do_nonsec_entry
-
 .globl	psci_cpu_off
 psci_cpu_off:
 	bl	psci_cpu_off_common
-- 
2.1.4

[U-Boot] [PATCH v2 03/12] ARM: Factor out reusable psci_get_cpu_stack_top

[Jan Kiszka]

This algorithm will be useful on Tegra as well, plus we will need it for
making _psci_target_pc per-CPU.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/psci.S       | 14 ++++++++++++++
 arch/arm/cpu/armv7/sunxi/psci.S | 17 +++++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
index e916d71..da47934 100644
--- a/arch/arm/cpu/armv7/psci.S
+++ b/arch/arm/cpu/armv7/psci.S
@@ -170,6 +170,20 @@ ENTRY(psci_cpu_off_common)
 	bx	lr
 ENDPROC(psci_cpu_off_common)
 
+@ expects CPU ID in r4 (will be overwritten) and returns stack top in r5
+ENTRY(psci_get_cpu_stack_top)
+	mov	r5, #0x400			@ 1kB of stack per CPU
+	mul	r4, r4, r5
+
+	ldr	r5, =psci_text_end		@ end of monitor text
+	add	r5, r5, #0x2000			@ Skip two pages
+	lsr	r5, r5, #12			@ Align to start of page
+	lsl	r5, r5, #12
+	sub	r5, r5, r4			@ here's our stack!
+
+	bx	lr
+ENDPROC(psci_get_cpu_stack_top)
+
 ENTRY(psci_cpu_entry)
 	@ Set SMP bit
 	mrc	p15, 0, r0, c1, c0, 1		@ ACTLR
diff --git a/arch/arm/cpu/armv7/sunxi/psci.S b/arch/arm/cpu/armv7/sunxi/psci.S
index c3a8dc1..4372022 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.S
+++ b/arch/arm/cpu/armv7/sunxi/psci.S
@@ -213,6 +213,8 @@ psci_cpu_off:
 
 .globl	psci_arch_init
 psci_arch_init:
+	mov	r6, lr
+
 	movw	r4, #(GICD_BASE & 0xffff)
 	movt	r4, #(GICD_BASE >> 16)
 
@@ -240,16 +242,11 @@ psci_arch_init:
 
 	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
 	and	r4, r4, #3		@ cpu number in cluster
-	mov	r5, #0x400		@ 1kB of stack per CPU
-	mul	r4, r4, r5
-
-	adr	r5, text_end		@ end of text
-	add	r5, r5, #0x2000		@ Skip two pages
-	lsr	r5, r5, #12		@ Align to start of page
-	lsl	r5, r5, #12
-	sub	sp, r5, r4		@ here's our stack!
+	bl	psci_get_cpu_stack_top
+	mov	sp, r5
 
-	bx	lr
+	bx	r6
 
-text_end:
+	.globl psci_text_end
+psci_text_end:
 	.popsection
-- 
2.1.4

[U-Boot] [PATCH v2 04/12] ARM: Put target PC for PSCI CPU_ON on per-CPU stack

[Jan Kiszka]

Use a per-CPU variable for saving the target PC during CPU_ON
operations. This allows us to run this service independently on targets
that have more than 2 cores and also core-local power control.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/psci.S       | 8 ++------
 arch/arm/cpu/armv7/sunxi/psci.S | 9 ++++++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
index da47934..9674503 100644
--- a/arch/arm/cpu/armv7/psci.S
+++ b/arch/arm/cpu/armv7/psci.S
@@ -179,6 +179,7 @@ ENTRY(psci_get_cpu_stack_top)
 	add	r5, r5, #0x2000			@ Skip two pages
 	lsr	r5, r5, #12			@ Align to start of page
 	lsl	r5, r5, #12
+	sub	r5, r5, #4			@ reserve 1 word for target PC
 	sub	r5, r5, r4			@ here's our stack!
 
 	bx	lr
@@ -194,13 +195,8 @@ ENTRY(psci_cpu_entry)
 	bl	_nonsec_init
 	bl	psci_arch_init
 
-	adr	r0, _psci_target_pc
-	ldr	r0, [r0]
+	ldr	r0, [sp]			@ target PC at stack top
 	b	_do_nonsec_entry
 ENDPROC(psci_cpu_entry)
 
-.globl _psci_target_pc
-_psci_target_pc:
-	.word	0
-
 	.popsection
diff --git a/arch/arm/cpu/armv7/sunxi/psci.S b/arch/arm/cpu/armv7/sunxi/psci.S
index 4372022..8d964d0 100644
--- a/arch/arm/cpu/armv7/sunxi/psci.S
+++ b/arch/arm/cpu/armv7/sunxi/psci.S
@@ -138,8 +138,11 @@ out:	mcr	p15, 0, r7, c1, c1, 0
 	@ r2 = target PC
 .globl	psci_cpu_on
 psci_cpu_on:
-	ldr	r0, =_psci_target_pc
-	str	r2, [r0]
+	push	{lr}
+
+	mov	r4, r1
+	bl	psci_get_cpu_stack_top	@ get stack top of target CPU
+	str	r2, [r5]		@ store target PC at stack top
 	dsb
 
 	movw	r0, #(SUN7I_CPUCFG_BASE & 0xffff)
@@ -194,7 +197,7 @@ psci_cpu_on:
 	str	r6, [r0, #0x1e4]
 
 	mov	r0, #ARM_PSCI_RET_SUCCESS	@ Return PSCI_RET_SUCCESS
-	mov	pc, lr
+	pop	{pc}
 
 .globl	psci_cpu_off
 psci_cpu_off:
-- 
2.1.4

[U-Boot] [PATCH v2 05/12] tegra124: Add more registers to struct mc_ctlr

[Jan Kiszka]

From: Ian Campbell <ijc@hellion.org.uk>

I will need mc_security_cfg0/1 in a future patch and I added the rest while
debugging, so thought I might as well commit them.

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/include/asm/arch-tegra124/mc.h | 35 +++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/arch-tegra124/mc.h b/arch/arm/include/asm/arch-tegra124/mc.h
index d526dfe..5557732 100644
--- a/arch/arm/include/asm/arch-tegra124/mc.h
+++ b/arch/arm/include/asm/arch-tegra124/mc.h
@@ -35,9 +35,40 @@ struct mc_ctlr {
 	u32 mc_emem_adr_cfg;			/* offset 0x54 */
 	u32 mc_emem_adr_cfg_dev0;		/* offset 0x58 */
 	u32 mc_emem_adr_cfg_dev1;		/* offset 0x5C */
-	u32 reserved3[12];			/* offset 0x60 - 0x8C */
+	u32 reserved3[4];			/* offset 0x60 - 0x6C */
+	u32 mc_security_cfg0;			/* offset 0x70 */
+	u32 mc_security_cfg1;			/* offset 0x74 */
+	u32 reserved4[6];			/* offset 0x7C - 0x8C */
 	u32 mc_emem_arb_reserved[28];		/* offset 0x90 - 0xFC */
-	u32 reserved4[338];			/* offset 0x100 - 0x644 */
+	u32 reserved5[74];			/* offset 0x100 - 0x224 */
+	u32 mc_smmu_translation_enable_0;	/* offset 0x228 */
+	u32 mc_smmu_translation_enable_1;	/* offset 0x22C */
+	u32 mc_smmu_translation_enable_2;	/* offset 0x230 */
+	u32 mc_smmu_translation_enable_3;	/* offset 0x234 */
+	u32 mc_smmu_afi_asid;			/* offset 0x238 */
+	u32 mc_smmu_avpc_asid;			/* offset 0x23C */
+	u32 mc_smmu_dc_asid;			/* offset 0x240 */
+	u32 mc_smmu_dcb_asid;			/* offset 0x244 */
+	u32 reserved6[2];                       /* offset 0x248 - 0x24C */
+	u32 mc_smmu_hc_asid;			/* offset 0x250 */
+	u32 mc_smmu_hda_asid;			/* offset 0x254 */
+	u32 mc_smmu_isp2_asid;			/* offset 0x258 */
+	u32 reserved7[2];                       /* offset 0x25C - 0x260 */
+	u32 mc_smmu_msenc_asid;			/* offset 0x264 */
+	u32 mc_smmu_nv_asid;			/* offset 0x268 */
+	u32 mc_smmu_nv2_asid;			/* offset 0x26C */
+	u32 mc_smmu_ppcs_asid;			/* offset 0x270 */
+	u32 mc_smmu_sata_asid;			/* offset 0x274 */
+	u32 reserved8[1];                       /* offset 0x278 */
+	u32 mc_smmu_vde_asid;			/* offset 0x27C */
+	u32 mc_smmu_vi_asid;			/* offset 0x280 */
+	u32 mc_smmu_vic_asid;			/* offset 0x284 */
+	u32 mc_smmu_xusb_host_asid;		/* offset 0x288 */
+	u32 mc_smmu_xusb_dev_asid;		/* offset 0x28C */
+	u32 reserved9[1];                       /* offset 0x290 */
+	u32 mc_smmu_tsec_asid;			/* offset 0x294 */
+	u32 mc_smmu_ppcs1_asid;			/* offset 0x298 */
+	u32 reserved10[235];			/* offset 0x29C - 0x644 */
 	u32 mc_video_protect_bom;		/* offset 0x648 */
 	u32 mc_video_protect_size_mb;		/* offset 0x64c */
 	u32 mc_video_protect_reg_ctrl;		/* offset 0x650 */
-- 
2.1.4

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

From: Ian Campbell <ijc@hellion.org.uk>

In this case the secure code lives in RAM, and hence needs to be reserved, but
it has been relocated, so the reservation of __secure_start does not apply.

Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
region.

This will be used in a subsequent patch for Jetson-TK1

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/virt-dt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
index ad19e4c..eb95031 100644
--- a/arch/arm/cpu/armv7/virt-dt.c
+++ b/arch/arm/cpu/armv7/virt-dt.c
@@ -96,6 +96,11 @@ int armv7_update_dt(void *fdt)
 	/* secure code lives in RAM, keep it alive */
 	fdt_add_mem_rsv(fdt, (unsigned long)__secure_start,
 			__secure_end - __secure_start);
+#elif defined(CONFIG_ARMV7_SECURE_RESERVE_SIZE)
+	/* secure code has been relocated into RAM carveout, keep it alive */
+	fdt_add_mem_rsv(fdt,
+			CONFIG_ARMV7_SECURE_BASE,
+			CONFIG_ARMV7_SECURE_RESERVE_SIZE);
 #endif
 
 	return fdt_psci(fdt);
-- 
2.1.4

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Mark Rutland]

On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
> From: Ian Campbell <ijc@hellion.org.uk>
> 
> In this case the secure code lives in RAM, and hence needs to be reserved, but
> it has been relocated, so the reservation of __secure_start does not apply.
> 
> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
> region.
> 
> This will be used in a subsequent patch for Jetson-TK1

Using a memreserve and allowing the OS to map the memory but not poke it
can be problematic due to the potential of mismatched attributes between
the monitor and the OS.

If you're able to carve out the "secure" memory from the memory node(s),
then you should be safe from that.

Thanks,
Mark.

> 
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm/cpu/armv7/virt-dt.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
> index ad19e4c..eb95031 100644
> --- a/arch/arm/cpu/armv7/virt-dt.c
> +++ b/arch/arm/cpu/armv7/virt-dt.c
> @@ -96,6 +96,11 @@ int armv7_update_dt(void *fdt)
>  	/* secure code lives in RAM, keep it alive */
>  	fdt_add_mem_rsv(fdt, (unsigned long)__secure_start,
>  			__secure_end - __secure_start);
> +#elif defined(CONFIG_ARMV7_SECURE_RESERVE_SIZE)
> +	/* secure code has been relocated into RAM carveout, keep it alive */
> +	fdt_add_mem_rsv(fdt,
> +			CONFIG_ARMV7_SECURE_BASE,
> +			CONFIG_ARMV7_SECURE_RESERVE_SIZE);
>  #endif
>  
>  	return fdt_psci(fdt);
> -- 
> 2.1.4
> 
> 

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

On 2015-02-16 14:42, Mark Rutland wrote:
> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
>> From: Ian Campbell <ijc@hellion.org.uk>
>>
>> In this case the secure code lives in RAM, and hence needs to be reserved, but
>> it has been relocated, so the reservation of __secure_start does not apply.
>>
>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
>> region.
>>
>> This will be used in a subsequent patch for Jetson-TK1
> 
> Using a memreserve and allowing the OS to map the memory but not poke it
> can be problematic due to the potential of mismatched attributes between
> the monitor and the OS.

OK, here my knowledge is not yet sufficient to process this remark. What
kind of problems can arise from what kind of attribute mismatch? And why
should the OS be able to cause problems for the monitor?

> 
> If you're able to carve out the "secure" memory from the memory node(s),
> then you should be safe from that.

Do you have a pointer to an example how to do it instead?

Jan

> 
> Thanks,
> Mark.
> 
>>
>> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/arm/cpu/armv7/virt-dt.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
>> index ad19e4c..eb95031 100644
>> --- a/arch/arm/cpu/armv7/virt-dt.c
>> +++ b/arch/arm/cpu/armv7/virt-dt.c
>> @@ -96,6 +96,11 @@ int armv7_update_dt(void *fdt)
>>  	/* secure code lives in RAM, keep it alive */
>>  	fdt_add_mem_rsv(fdt, (unsigned long)__secure_start,
>>  			__secure_end - __secure_start);
>> +#elif defined(CONFIG_ARMV7_SECURE_RESERVE_SIZE)
>> +	/* secure code has been relocated into RAM carveout, keep it alive */
>> +	fdt_add_mem_rsv(fdt,
>> +			CONFIG_ARMV7_SECURE_BASE,
>> +			CONFIG_ARMV7_SECURE_RESERVE_SIZE);
>>  #endif
>>  
>>  	return fdt_psci(fdt);
>> -- 
>> 2.1.4
>>
>>

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Mark Rutland]

On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
> On 2015-02-16 14:42, Mark Rutland wrote:
> > On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
> >> From: Ian Campbell <ijc@hellion.org.uk>
> >>
> >> In this case the secure code lives in RAM, and hence needs to be reserved, but
> >> it has been relocated, so the reservation of __secure_start does not apply.
> >>
> >> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
> >> region.
> >>
> >> This will be used in a subsequent patch for Jetson-TK1
> > 
> > Using a memreserve and allowing the OS to map the memory but not poke it
> > can be problematic due to the potential of mismatched attributes between
> > the monitor and the OS.
> 
> OK, here my knowledge is not yet sufficient to process this remark. What
> kind of problems can arise from what kind of attribute mismatch? And why
> should the OS be able to cause problems for the monitor?

For example, consider the case of the region being mapped cacheable by
the OS but not by the monitor. The monitor communicates between cores
expecting to never hit in a cache (because it uses a non-cacheable
mapping), but the mapping used by the OS can cause the region to be
allocated into caches at any point in time even if it never accesses the
region explicitly.

The CPU _may_ hit in a cache even if making a non-cacheable access (this
is called an "unexepcted data cache hit"), so the cache allocations
caused by the OS can mask data other CPUs wrote straight to memory.

Other than that case, I believe the rules given in the ARM ARM for
mismatched memory attributes may apply for similar reasons.  Thus
allowing the OS to map this memory can cause a loss of coherency on the
monitor side, if the OS and monitor map the region with different
attributes.

This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
system you're dealing with. I don't immediately know whether that is the
case, however. Never telling the OS about the memory in the first place
avoids the possibility in all cases.

> > If you're able to carve out the "secure" memory from the memory node(s),
> > then you should be safe from that.
> 
> Do you have a pointer to an example how to do it instead?

Unfortunately not; I don't know whether there's an existing primitive
for doing that. In general you might need to split a memory region in
two to carve out the portion in the middle.

Thanks,
Mark.

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

On 2015-02-16 15:25, Mark Rutland wrote:
> On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
>> On 2015-02-16 14:42, Mark Rutland wrote:
>>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
>>>> From: Ian Campbell <ijc@hellion.org.uk>
>>>>
>>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
>>>> it has been relocated, so the reservation of __secure_start does not apply.
>>>>
>>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
>>>> region.
>>>>
>>>> This will be used in a subsequent patch for Jetson-TK1
>>>
>>> Using a memreserve and allowing the OS to map the memory but not poke it
>>> can be problematic due to the potential of mismatched attributes between
>>> the monitor and the OS.
>>
>> OK, here my knowledge is not yet sufficient to process this remark. What
>> kind of problems can arise from what kind of attribute mismatch? And why
>> should the OS be able to cause problems for the monitor?
> 
> For example, consider the case of the region being mapped cacheable by
> the OS but not by the monitor. The monitor communicates between cores
> expecting to never hit in a cache (because it uses a non-cacheable
> mapping), but the mapping used by the OS can cause the region to be
> allocated into caches at any point in time even if it never accesses the
> region explicitly.
> 
> The CPU _may_ hit in a cache even if making a non-cacheable access (this
> is called an "unexepcted data cache hit"), so the cache allocations
> caused by the OS can mask data other CPUs wrote straight to memory.
> 
> Other than that case, I believe the rules given in the ARM ARM for
> mismatched memory attributes may apply for similar reasons.  Thus
> allowing the OS to map this memory can cause a loss of coherency on the
> monitor side, if the OS and monitor map the region with different
> attributes.
> 
> This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
> system you're dealing with. I don't immediately know whether that is the
> case, however. Never telling the OS about the memory in the first place
> avoids the possibility in all cases.

But from a security point of view, it must not matter if the OS maps the
memory or not - the monitor must be robust against that, no? If the
architecture cannot provide such guarantees, it has to be worked around
in software in the monitor (I hope you can do so...).

Jan

> 
>>> If you're able to carve out the "secure" memory from the memory node(s),
>>> then you should be safe from that.
>>
>> Do you have a pointer to an example how to do it instead?
> 
> Unfortunately not; I don't know whether there's an existing primitive
> for doing that. In general you might need to split a memory region in
> two to carve out the portion in the middle.
> 
> Thanks,
> Mark.
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Mark Rutland]

On Mon, Feb 16, 2015 at 02:31:21PM +0000, Jan Kiszka wrote:
> On 2015-02-16 15:25, Mark Rutland wrote:
> > On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
> >> On 2015-02-16 14:42, Mark Rutland wrote:
> >>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
> >>>> From: Ian Campbell <ijc@hellion.org.uk>
> >>>>
> >>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
> >>>> it has been relocated, so the reservation of __secure_start does not apply.
> >>>>
> >>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
> >>>> region.
> >>>>
> >>>> This will be used in a subsequent patch for Jetson-TK1
> >>>
> >>> Using a memreserve and allowing the OS to map the memory but not poke it
> >>> can be problematic due to the potential of mismatched attributes between
> >>> the monitor and the OS.
> >>
> >> OK, here my knowledge is not yet sufficient to process this remark. What
> >> kind of problems can arise from what kind of attribute mismatch? And why
> >> should the OS be able to cause problems for the monitor?
> > 
> > For example, consider the case of the region being mapped cacheable by
> > the OS but not by the monitor. The monitor communicates between cores
> > expecting to never hit in a cache (because it uses a non-cacheable
> > mapping), but the mapping used by the OS can cause the region to be
> > allocated into caches at any point in time even if it never accesses the
> > region explicitly.
> > 
> > The CPU _may_ hit in a cache even if making a non-cacheable access (this
> > is called an "unexepcted data cache hit"), so the cache allocations
> > caused by the OS can mask data other CPUs wrote straight to memory.
> > 
> > Other than that case, I believe the rules given in the ARM ARM for
> > mismatched memory attributes may apply for similar reasons.  Thus
> > allowing the OS to map this memory can cause a loss of coherency on the
> > monitor side, if the OS and monitor map the region with different
> > attributes.
> > 
> > This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
> > system you're dealing with. I don't immediately know whether that is the
> > case, however. Never telling the OS about the memory in the first place
> > avoids the possibility in all cases.
> 
> But from a security point of view, it must not matter if the OS maps the
> memory or not - the monitor must be robust against that, no? If the
> architecture cannot provide such guarantees, it has to be worked around
> in software in the monitor (I hope you can do so...).

Well, yes and no.

In this case it sounds like due to the security controller you should
never encounter the mismatched attributes issue in the first place,
though you may encounter issues w.r.t. speculative accesses triggering
violations arbitrarily. Not telling the OS about the secure memory means
that said violations shouldn't occur in normal operation; only when the
non-secure OS is trying to do something bad.

If the OS has access to the memory, then you're already trusting it to
not write to there or you can't trust that memory at all (and hence
cannot use it). Given that means you must already assume that the OS is
cooperative, it's simpler to not tell it about the memory than to add
cache maintenance around every memory access within the monitor. You can
never make things secure in this case, but you can at least offer the
abstraction provided by PSCI.

So as far as I can see in either case it's better to not tell the OS
about the memory you wish to use from the monitor. If you have no HW
protection and can't trust the OS then you've already lost, and if you
do have HW protection you don't want it to trigger
continuously/spuriously as a result of speculation.

Thanks,
Mark.

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

On 2015-02-16 15:56, Mark Rutland wrote:
> On Mon, Feb 16, 2015 at 02:31:21PM +0000, Jan Kiszka wrote:
>> On 2015-02-16 15:25, Mark Rutland wrote:
>>> On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
>>>> On 2015-02-16 14:42, Mark Rutland wrote:
>>>>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
>>>>>> From: Ian Campbell <ijc@hellion.org.uk>
>>>>>>
>>>>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
>>>>>> it has been relocated, so the reservation of __secure_start does not apply.
>>>>>>
>>>>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
>>>>>> region.
>>>>>>
>>>>>> This will be used in a subsequent patch for Jetson-TK1
>>>>>
>>>>> Using a memreserve and allowing the OS to map the memory but not poke it
>>>>> can be problematic due to the potential of mismatched attributes between
>>>>> the monitor and the OS.
>>>>
>>>> OK, here my knowledge is not yet sufficient to process this remark. What
>>>> kind of problems can arise from what kind of attribute mismatch? And why
>>>> should the OS be able to cause problems for the monitor?
>>>
>>> For example, consider the case of the region being mapped cacheable by
>>> the OS but not by the monitor. The monitor communicates between cores
>>> expecting to never hit in a cache (because it uses a non-cacheable
>>> mapping), but the mapping used by the OS can cause the region to be
>>> allocated into caches at any point in time even if it never accesses the
>>> region explicitly.
>>>
>>> The CPU _may_ hit in a cache even if making a non-cacheable access (this
>>> is called an "unexepcted data cache hit"), so the cache allocations
>>> caused by the OS can mask data other CPUs wrote straight to memory.
>>>
>>> Other than that case, I believe the rules given in the ARM ARM for
>>> mismatched memory attributes may apply for similar reasons.  Thus
>>> allowing the OS to map this memory can cause a loss of coherency on the
>>> monitor side, if the OS and monitor map the region with different
>>> attributes.
>>>
>>> This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
>>> system you're dealing with. I don't immediately know whether that is the
>>> case, however. Never telling the OS about the memory in the first place
>>> avoids the possibility in all cases.
>>
>> But from a security point of view, it must not matter if the OS maps the
>> memory or not - the monitor must be robust against that, no? If the
>> architecture cannot provide such guarantees, it has to be worked around
>> in software in the monitor (I hope you can do so...).
> 
> Well, yes and no.
> 
> In this case it sounds like due to the security controller you should
> never encounter the mismatched attributes issue in the first place,
> though you may encounter issues w.r.t. speculative accesses triggering
> violations arbitrarily. Not telling the OS about the secure memory means
> that said violations shouldn't occur in normal operation; only when the
> non-secure OS is trying to do something bad.
> 
> If the OS has access to the memory, then you're already trusting it to
> not write to there or you can't trust that memory at all (and hence
> cannot use it). Given that means you must already assume that the OS is
> cooperative, it's simpler to not tell it about the memory than to add
> cache maintenance around every memory access within the monitor. You can
> never make things secure in this case, but you can at least offer the
> abstraction provided by PSCI.
> 
> So as far as I can see in either case it's better to not tell the OS
> about the memory you wish to use from the monitor. If you have no HW
> protection and can't trust the OS then you've already lost, and if you
> do have HW protection you don't want it to trigger
> continuously/spuriously as a result of speculation.

OK, that makes sense again.

Now I just need to figure out how to split/adjust the memory node
instead of adding a reservation region.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

On 2015-02-16 16:38, Jan Kiszka wrote:
> On 2015-02-16 15:56, Mark Rutland wrote:
>> On Mon, Feb 16, 2015 at 02:31:21PM +0000, Jan Kiszka wrote:
>>> On 2015-02-16 15:25, Mark Rutland wrote:
>>>> On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
>>>>> On 2015-02-16 14:42, Mark Rutland wrote:
>>>>>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
>>>>>>> From: Ian Campbell <ijc@hellion.org.uk>
>>>>>>>
>>>>>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
>>>>>>> it has been relocated, so the reservation of __secure_start does not apply.
>>>>>>>
>>>>>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
>>>>>>> region.
>>>>>>>
>>>>>>> This will be used in a subsequent patch for Jetson-TK1
>>>>>>
>>>>>> Using a memreserve and allowing the OS to map the memory but not poke it
>>>>>> can be problematic due to the potential of mismatched attributes between
>>>>>> the monitor and the OS.
>>>>>
>>>>> OK, here my knowledge is not yet sufficient to process this remark. What
>>>>> kind of problems can arise from what kind of attribute mismatch? And why
>>>>> should the OS be able to cause problems for the monitor?
>>>>
>>>> For example, consider the case of the region being mapped cacheable by
>>>> the OS but not by the monitor. The monitor communicates between cores
>>>> expecting to never hit in a cache (because it uses a non-cacheable
>>>> mapping), but the mapping used by the OS can cause the region to be
>>>> allocated into caches at any point in time even if it never accesses the
>>>> region explicitly.
>>>>
>>>> The CPU _may_ hit in a cache even if making a non-cacheable access (this
>>>> is called an "unexepcted data cache hit"), so the cache allocations
>>>> caused by the OS can mask data other CPUs wrote straight to memory.
>>>>
>>>> Other than that case, I believe the rules given in the ARM ARM for
>>>> mismatched memory attributes may apply for similar reasons.  Thus
>>>> allowing the OS to map this memory can cause a loss of coherency on the
>>>> monitor side, if the OS and monitor map the region with different
>>>> attributes.
>>>>
>>>> This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
>>>> system you're dealing with. I don't immediately know whether that is the
>>>> case, however. Never telling the OS about the memory in the first place
>>>> avoids the possibility in all cases.
>>>
>>> But from a security point of view, it must not matter if the OS maps the
>>> memory or not - the monitor must be robust against that, no? If the
>>> architecture cannot provide such guarantees, it has to be worked around
>>> in software in the monitor (I hope you can do so...).
>>
>> Well, yes and no.
>>
>> In this case it sounds like due to the security controller you should
>> never encounter the mismatched attributes issue in the first place,
>> though you may encounter issues w.r.t. speculative accesses triggering
>> violations arbitrarily. Not telling the OS about the secure memory means
>> that said violations shouldn't occur in normal operation; only when the
>> non-secure OS is trying to do something bad.
>>
>> If the OS has access to the memory, then you're already trusting it to
>> not write to there or you can't trust that memory at all (and hence
>> cannot use it). Given that means you must already assume that the OS is
>> cooperative, it's simpler to not tell it about the memory than to add
>> cache maintenance around every memory access within the monitor. You can
>> never make things secure in this case, but you can at least offer the
>> abstraction provided by PSCI.
>>
>> So as far as I can see in either case it's better to not tell the OS
>> about the memory you wish to use from the monitor. If you have no HW
>> protection and can't trust the OS then you've already lost, and if you
>> do have HW protection you don't want it to trigger
>> continuously/spuriously as a result of speculation.
> 
> OK, that makes sense again.
> 
> Now I just need to figure out how to split/adjust the memory node
> instead of adding a reservation region.

This is getting invasive:

If I add carveouts via adjusting memory banks, I need to account for the
case that an existing bank is split into two halves, creating additional
banks this way. But then current fdt_fixup_memory_banks will no longer
work due to its limitation to the number of physical banks. I could
always add one spare bank to that service, ok, but then the next use
case for carveouts will hit the wall again. So I better double that
limit, or so.

Also, are there any architectural or OS-implementation related
restrictions on the alignment of bank start addresses and sizes? Just to
make sure we don't stumble over some side effects of punching holes into
that device tree node.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Mark Rutland]

On Tue, Feb 17, 2015 at 08:09:57AM +0000, Jan Kiszka wrote:
> On 2015-02-16 16:38, Jan Kiszka wrote:
> > On 2015-02-16 15:56, Mark Rutland wrote:
> >> On Mon, Feb 16, 2015 at 02:31:21PM +0000, Jan Kiszka wrote:
> >>> On 2015-02-16 15:25, Mark Rutland wrote:
> >>>> On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
> >>>>> On 2015-02-16 14:42, Mark Rutland wrote:
> >>>>>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
> >>>>>>> From: Ian Campbell <ijc@hellion.org.uk>
> >>>>>>>
> >>>>>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
> >>>>>>> it has been relocated, so the reservation of __secure_start does not apply.
> >>>>>>>
> >>>>>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
> >>>>>>> region.
> >>>>>>>
> >>>>>>> This will be used in a subsequent patch for Jetson-TK1
> >>>>>>
> >>>>>> Using a memreserve and allowing the OS to map the memory but not poke it
> >>>>>> can be problematic due to the potential of mismatched attributes between
> >>>>>> the monitor and the OS.
> >>>>>
> >>>>> OK, here my knowledge is not yet sufficient to process this remark. What
> >>>>> kind of problems can arise from what kind of attribute mismatch? And why
> >>>>> should the OS be able to cause problems for the monitor?
> >>>>
> >>>> For example, consider the case of the region being mapped cacheable by
> >>>> the OS but not by the monitor. The monitor communicates between cores
> >>>> expecting to never hit in a cache (because it uses a non-cacheable
> >>>> mapping), but the mapping used by the OS can cause the region to be
> >>>> allocated into caches at any point in time even if it never accesses the
> >>>> region explicitly.
> >>>>
> >>>> The CPU _may_ hit in a cache even if making a non-cacheable access (this
> >>>> is called an "unexepcted data cache hit"), so the cache allocations
> >>>> caused by the OS can mask data other CPUs wrote straight to memory.
> >>>>
> >>>> Other than that case, I believe the rules given in the ARM ARM for
> >>>> mismatched memory attributes may apply for similar reasons.  Thus
> >>>> allowing the OS to map this memory can cause a loss of coherency on the
> >>>> monitor side, if the OS and monitor map the region with different
> >>>> attributes.
> >>>>
> >>>> This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
> >>>> system you're dealing with. I don't immediately know whether that is the
> >>>> case, however. Never telling the OS about the memory in the first place
> >>>> avoids the possibility in all cases.
> >>>
> >>> But from a security point of view, it must not matter if the OS maps the
> >>> memory or not - the monitor must be robust against that, no? If the
> >>> architecture cannot provide such guarantees, it has to be worked around
> >>> in software in the monitor (I hope you can do so...).
> >>
> >> Well, yes and no.
> >>
> >> In this case it sounds like due to the security controller you should
> >> never encounter the mismatched attributes issue in the first place,
> >> though you may encounter issues w.r.t. speculative accesses triggering
> >> violations arbitrarily. Not telling the OS about the secure memory means
> >> that said violations shouldn't occur in normal operation; only when the
> >> non-secure OS is trying to do something bad.
> >>
> >> If the OS has access to the memory, then you're already trusting it to
> >> not write to there or you can't trust that memory at all (and hence
> >> cannot use it). Given that means you must already assume that the OS is
> >> cooperative, it's simpler to not tell it about the memory than to add
> >> cache maintenance around every memory access within the monitor. You can
> >> never make things secure in this case, but you can at least offer the
> >> abstraction provided by PSCI.
> >>
> >> So as far as I can see in either case it's better to not tell the OS
> >> about the memory you wish to use from the monitor. If you have no HW
> >> protection and can't trust the OS then you've already lost, and if you
> >> do have HW protection you don't want it to trigger
> >> continuously/spuriously as a result of speculation.
> > 
> > OK, that makes sense again.
> > 
> > Now I just need to figure out how to split/adjust the memory node
> > instead of adding a reservation region.
> 
> This is getting invasive:
> 
> If I add carveouts via adjusting memory banks, I need to account for the
> case that an existing bank is split into two halves, creating additional
> banks this way. But then current fdt_fixup_memory_banks will no longer
> work due to its limitation to the number of physical banks. I could
> always add one spare bank to that service, ok, but then the next use
> case for carveouts will hit the wall again. So I better double that
> limit, or so.

Yeah, not fun.

If the code is position-independent then you might be able to simply
carve out a sufficient proportion from the start of the first entry or
the end of the last one, which would avoid splitting. If either of said
regions are too small for the monitor code then it's questionable as to
whether the OS can make use of it.

> Also, are there any architectural or OS-implementation related
> restrictions on the alignment of bank start addresses and sizes? Just to
> make sure we don't stumble over some side effects of punching holes into
> that device tree node.

I would guess that we need to at least pad the carevout to page-aligned
to prevent any particular OS from mapping a page for the sake of a few
bytes left unused by the monitor.

From a quick look at the Linux arm_add_memory and memblock code it looks
like Linux won't map partial pages, but I don't know what Xen and others
do, and given we know that we want to keep the relevant pages exclusive
to the monitor anyway padding to age boundaries seems like a sensible
thing to do.

My one concern would be early mappings; I believe that the initial page
tables use (2MiB) section/block mappings to map the kernel and some
initial memory (including the DTB) before the memory nodes are parsed,
so the carevout would need to be placed away from where the kernel and
DTB were loaded in order to prevent those early mappings from covering
it. I'm unfortunately not sure on the full details there.

Thanks,
Mark.

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

On 2015-02-17 11:46, Mark Rutland wrote:
> On Tue, Feb 17, 2015 at 08:09:57AM +0000, Jan Kiszka wrote:
>> On 2015-02-16 16:38, Jan Kiszka wrote:
>>> On 2015-02-16 15:56, Mark Rutland wrote:
>>>> On Mon, Feb 16, 2015 at 02:31:21PM +0000, Jan Kiszka wrote:
>>>>> On 2015-02-16 15:25, Mark Rutland wrote:
>>>>>> On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
>>>>>>> On 2015-02-16 14:42, Mark Rutland wrote:
>>>>>>>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
>>>>>>>>> From: Ian Campbell <ijc@hellion.org.uk>
>>>>>>>>>
>>>>>>>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
>>>>>>>>> it has been relocated, so the reservation of __secure_start does not apply.
>>>>>>>>>
>>>>>>>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
>>>>>>>>> region.
>>>>>>>>>
>>>>>>>>> This will be used in a subsequent patch for Jetson-TK1
>>>>>>>>
>>>>>>>> Using a memreserve and allowing the OS to map the memory but not poke it
>>>>>>>> can be problematic due to the potential of mismatched attributes between
>>>>>>>> the monitor and the OS.
>>>>>>>
>>>>>>> OK, here my knowledge is not yet sufficient to process this remark. What
>>>>>>> kind of problems can arise from what kind of attribute mismatch? And why
>>>>>>> should the OS be able to cause problems for the monitor?
>>>>>>
>>>>>> For example, consider the case of the region being mapped cacheable by
>>>>>> the OS but not by the monitor. The monitor communicates between cores
>>>>>> expecting to never hit in a cache (because it uses a non-cacheable
>>>>>> mapping), but the mapping used by the OS can cause the region to be
>>>>>> allocated into caches at any point in time even if it never accesses the
>>>>>> region explicitly.
>>>>>>
>>>>>> The CPU _may_ hit in a cache even if making a non-cacheable access (this
>>>>>> is called an "unexepcted data cache hit"), so the cache allocations
>>>>>> caused by the OS can mask data other CPUs wrote straight to memory.
>>>>>>
>>>>>> Other than that case, I believe the rules given in the ARM ARM for
>>>>>> mismatched memory attributes may apply for similar reasons.  Thus
>>>>>> allowing the OS to map this memory can cause a loss of coherency on the
>>>>>> monitor side, if the OS and monitor map the region with different
>>>>>> attributes.
>>>>>>
>>>>>> This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
>>>>>> system you're dealing with. I don't immediately know whether that is the
>>>>>> case, however. Never telling the OS about the memory in the first place
>>>>>> avoids the possibility in all cases.
>>>>>
>>>>> But from a security point of view, it must not matter if the OS maps the
>>>>> memory or not - the monitor must be robust against that, no? If the
>>>>> architecture cannot provide such guarantees, it has to be worked around
>>>>> in software in the monitor (I hope you can do so...).
>>>>
>>>> Well, yes and no.
>>>>
>>>> In this case it sounds like due to the security controller you should
>>>> never encounter the mismatched attributes issue in the first place,
>>>> though you may encounter issues w.r.t. speculative accesses triggering
>>>> violations arbitrarily. Not telling the OS about the secure memory means
>>>> that said violations shouldn't occur in normal operation; only when the
>>>> non-secure OS is trying to do something bad.
>>>>
>>>> If the OS has access to the memory, then you're already trusting it to
>>>> not write to there or you can't trust that memory at all (and hence
>>>> cannot use it). Given that means you must already assume that the OS is
>>>> cooperative, it's simpler to not tell it about the memory than to add
>>>> cache maintenance around every memory access within the monitor. You can
>>>> never make things secure in this case, but you can at least offer the
>>>> abstraction provided by PSCI.
>>>>
>>>> So as far as I can see in either case it's better to not tell the OS
>>>> about the memory you wish to use from the monitor. If you have no HW
>>>> protection and can't trust the OS then you've already lost, and if you
>>>> do have HW protection you don't want it to trigger
>>>> continuously/spuriously as a result of speculation.
>>>
>>> OK, that makes sense again.
>>>
>>> Now I just need to figure out how to split/adjust the memory node
>>> instead of adding a reservation region.
>>
>> This is getting invasive:
>>
>> If I add carveouts via adjusting memory banks, I need to account for the
>> case that an existing bank is split into two halves, creating additional
>> banks this way. But then current fdt_fixup_memory_banks will no longer
>> work due to its limitation to the number of physical banks. I could
>> always add one spare bank to that service, ok, but then the next use
>> case for carveouts will hit the wall again. So I better double that
>> limit, or so.
> 
> Yeah, not fun.
> 
> If the code is position-independent then you might be able to simply
> carve out a sufficient proportion from the start of the first entry or
> the end of the last one, which would avoid splitting. If either of said
> regions are too small for the monitor code then it's questionable as to
> whether the OS can make use of it.

The code /seems/ to be position-independent, but locations are so far
hard-coded in those places that prepare it and move it around. Maybe we
can decide about the location at runtime, maybe we can simply demand it
to be at the end or the beginning of some bank.

> 
>> Also, are there any architectural or OS-implementation related
>> restrictions on the alignment of bank start addresses and sizes? Just to
>> make sure we don't stumble over some side effects of punching holes into
>> that device tree node.
> 
> I would guess that we need to at least pad the carevout to page-aligned
> to prevent any particular OS from mapping a page for the sake of a few
> bytes left unused by the monitor.
> 
> From a quick look at the Linux arm_add_memory and memblock code it looks
> like Linux won't map partial pages, but I don't know what Xen and others
> do, and given we know that we want to keep the relevant pages exclusive
> to the monitor anyway padding to age boundaries seems like a sensible
> thing to do.
> 
> My one concern would be early mappings; I believe that the initial page
> tables use (2MiB) section/block mappings to map the kernel and some
> initial memory (including the DTB) before the memory nodes are parsed,
> so the carevout would need to be placed away from where the kernel and
> DTB were loaded in order to prevent those early mappings from covering
> it. I'm unfortunately not sure on the full details there.

That makes be wonder again if we are trying to solve real issues: What
is the OS supposed to do with a memory reserve map, what does it have to
avoid doing with it? Is the semantic really so weak that we cannot use
it here?

Jan

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Mark Rutland]

[...]

> >> This is getting invasive:
> >>
> >> If I add carveouts via adjusting memory banks, I need to account for the
> >> case that an existing bank is split into two halves, creating additional
> >> banks this way. But then current fdt_fixup_memory_banks will no longer
> >> work due to its limitation to the number of physical banks. I could
> >> always add one spare bank to that service, ok, but then the next use
> >> case for carveouts will hit the wall again. So I better double that
> >> limit, or so.
> > 
> > Yeah, not fun.
> > 
> > If the code is position-independent then you might be able to simply
> > carve out a sufficient proportion from the start of the first entry or
> > the end of the last one, which would avoid splitting. If either of said
> > regions are too small for the monitor code then it's questionable as to
> > whether the OS can make use of it.
> 
> The code /seems/ to be position-independent, but locations are so far
> hard-coded in those places that prepare it and move it around. Maybe we
> can decide about the location at runtime, maybe we can simply demand it
> to be at the end or the beginning of some bank.

If it's possible to do so, it would seem like the nicest option to me.

> >> Also, are there any architectural or OS-implementation related
> >> restrictions on the alignment of bank start addresses and sizes? Just to
> >> make sure we don't stumble over some side effects of punching holes into
> >> that device tree node.
> > 
> > I would guess that we need to at least pad the carevout to page-aligned
> > to prevent any particular OS from mapping a page for the sake of a few
> > bytes left unused by the monitor.
> > 
> > From a quick look at the Linux arm_add_memory and memblock code it looks
> > like Linux won't map partial pages, but I don't know what Xen and others
> > do, and given we know that we want to keep the relevant pages exclusive
> > to the monitor anyway padding to age boundaries seems like a sensible
> > thing to do.
> > 
> > My one concern would be early mappings; I believe that the initial page
> > tables use (2MiB) section/block mappings to map the kernel and some
> > initial memory (including the DTB) before the memory nodes are parsed,
> > so the carevout would need to be placed away from where the kernel and
> > DTB were loaded in order to prevent those early mappings from covering
> > it. I'm unfortunately not sure on the full details there.
> 
> That makes be wonder again if we are trying to solve real issues: What
> is the OS supposed to do with a memory reserve map, what does it have to
> avoid doing with it?

Per ePAPR, memory reservation block entries may not be explicitly
accessed by the operating system (unless told to elsewhere). The OS may
map any reserved entries with cacheable attributes (potentially leading
to the issues I described earlier)

> Is the semantic really so weak that we cannot use it here?

In general, the semantic is too weak. In fact, it's not even strictly
defined for the ARM architecture w.r.t. memory attributes, so we have
very little guarantee as to what what an OS will do beyond that it will
not perform any explicit accesses to the region.

In practice, Linux will currently map the region as cacheable, and it
may or may not map it shareable depending on SMP/UP (which could be a
problem if you want to use a UP Linux to load and kexec an SMP kernel
for some reason).

It may be that on a given CPU/system implemetation that a memreserve
entry is sufficient; but unfortunately this depends on IMPLEMENTATION
DEFINED details.

Thanks,
Mark.

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Thierry Reding]

On Tue, Feb 17, 2015 at 11:55:24AM +0000, Mark Rutland wrote:
> [...]
> 
> > >> This is getting invasive:
> > >>
> > >> If I add carveouts via adjusting memory banks, I need to account for the
> > >> case that an existing bank is split into two halves, creating additional
> > >> banks this way. But then current fdt_fixup_memory_banks will no longer
> > >> work due to its limitation to the number of physical banks. I could
> > >> always add one spare bank to that service, ok, but then the next use
> > >> case for carveouts will hit the wall again. So I better double that
> > >> limit, or so.
> > > 
> > > Yeah, not fun.
> > > 
> > > If the code is position-independent then you might be able to simply
> > > carve out a sufficient proportion from the start of the first entry or
> > > the end of the last one, which would avoid splitting. If either of said
> > > regions are too small for the monitor code then it's questionable as to
> > > whether the OS can make use of it.
> > 
> > The code /seems/ to be position-independent, but locations are so far
> > hard-coded in those places that prepare it and move it around. Maybe we
> > can decide about the location at runtime, maybe we can simply demand it
> > to be at the end or the beginning of some bank.
> 
> If it's possible to do so, it would seem like the nicest option to me.

Using the top of memory for this seems like the most natural choice, so
I think if we restrict ourselves to that for now we should be fine. The
code could probably just get the top of memory from the memory node and
adjust it according to the size specified for the secure monitor.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/1d99ae00/attachment.sig>

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Ian Campbell]

On Thu, 2015-02-19 at 09:28 +0100, Thierry Reding wrote:
> On Tue, Feb 17, 2015 at 11:55:24AM +0000, Mark Rutland wrote:
> > [...]
> > 
> > > >> This is getting invasive:
> > > >>
> > > >> If I add carveouts via adjusting memory banks, I need to account for the
> > > >> case that an existing bank is split into two halves, creating additional
> > > >> banks this way. But then current fdt_fixup_memory_banks will no longer
> > > >> work due to its limitation to the number of physical banks. I could
> > > >> always add one spare bank to that service, ok, but then the next use
> > > >> case for carveouts will hit the wall again. So I better double that
> > > >> limit, or so.
> > > > 
> > > > Yeah, not fun.
> > > > 
> > > > If the code is position-independent then you might be able to simply
> > > > carve out a sufficient proportion from the start of the first entry or
> > > > the end of the last one, which would avoid splitting. If either of said
> > > > regions are too small for the monitor code then it's questionable as to
> > > > whether the OS can make use of it.
> > > 
> > > The code /seems/ to be position-independent, but locations are so far
> > > hard-coded in those places that prepare it and move it around. Maybe we
> > > can decide about the location at runtime, maybe we can simply demand it
> > > to be at the end or the beginning of some bank.
> > 
> > If it's possible to do so, it would seem like the nicest option to me.
> 
> Using the top of memory for this seems like the most natural choice,

I think it needs to still be below 4G, doesn't it? So on large mem/LPAE
systems some care might be needed.

It was suggested by Mark earlier in the thread that this stuff is
IMPLEMENTATION DEFINED. Is it possible that we simply don't need to
worry about these cross-world cache issues on Tegra?

(I must confess that until now I'd assumed that the cache lines were
tagged with the world which populated them to stop them interfering with
each other in this sort of way...)

>  so
> I think if we restrict ourselves to that for now we should be fine. The
> code could probably just get the top of memory from the memory node and
> adjust it according to the size specified for the secure monitor.
> 
> Thierry

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

On 2015-02-19 10:19, Ian Campbell wrote:
> On Thu, 2015-02-19 at 09:28 +0100, Thierry Reding wrote:
>> On Tue, Feb 17, 2015 at 11:55:24AM +0000, Mark Rutland wrote:
>>> [...]
>>>
>>>>>> This is getting invasive:
>>>>>>
>>>>>> If I add carveouts via adjusting memory banks, I need to account for the
>>>>>> case that an existing bank is split into two halves, creating additional
>>>>>> banks this way. But then current fdt_fixup_memory_banks will no longer
>>>>>> work due to its limitation to the number of physical banks. I could
>>>>>> always add one spare bank to that service, ok, but then the next use
>>>>>> case for carveouts will hit the wall again. So I better double that
>>>>>> limit, or so.
>>>>>
>>>>> Yeah, not fun.
>>>>>
>>>>> If the code is position-independent then you might be able to simply
>>>>> carve out a sufficient proportion from the start of the first entry or
>>>>> the end of the last one, which would avoid splitting. If either of said
>>>>> regions are too small for the monitor code then it's questionable as to
>>>>> whether the OS can make use of it.
>>>>
>>>> The code /seems/ to be position-independent, but locations are so far
>>>> hard-coded in those places that prepare it and move it around. Maybe we
>>>> can decide about the location at runtime, maybe we can simply demand it
>>>> to be at the end or the beginning of some bank.
>>>
>>> If it's possible to do so, it would seem like the nicest option to me.
>>
>> Using the top of memory for this seems like the most natural choice,
> 
> I think it needs to still be below 4G, doesn't it? So on large mem/LPAE
> systems some care might be needed.

Argh. That would likely mean we had to split a bank (unless >2G comes in
multiple banks), something I'd like to avoid having to implement.

> 
> It was suggested by Mark earlier in the thread that this stuff is
> IMPLEMENTATION DEFINED. Is it possible that we simply don't need to
> worry about these cross-world cache issues on Tegra?
> 
> (I must confess that until now I'd assumed that the cache lines were
> tagged with the world which populated them to stop them interfering with
> each other in this sort of way...)

I'm pretty sure that is no such thing as a cross-world cache problem.
Otherwise the architecture or some implementation would have serious
security issues as discussed earlier. To my understanding, Mark's
suggestion is now targeting the concern that Linux may accidentally
trigger accesses and, thus, stumble or create warnings at least.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Ian Campbell]

On Thu, 2015-02-19 at 10:25 +0100, Jan Kiszka wrote:
> On 2015-02-19 10:19, Ian Campbell wrote:
> > On Thu, 2015-02-19 at 09:28 +0100, Thierry Reding wrote:
> >> On Tue, Feb 17, 2015 at 11:55:24AM +0000, Mark Rutland wrote:
> >>> [...]
> >>>
> >>>>>> This is getting invasive:
> >>>>>>
> >>>>>> If I add carveouts via adjusting memory banks, I need to account for the
> >>>>>> case that an existing bank is split into two halves, creating additional
> >>>>>> banks this way. But then current fdt_fixup_memory_banks will no longer
> >>>>>> work due to its limitation to the number of physical banks. I could
> >>>>>> always add one spare bank to that service, ok, but then the next use
> >>>>>> case for carveouts will hit the wall again. So I better double that
> >>>>>> limit, or so.
> >>>>>
> >>>>> Yeah, not fun.
> >>>>>
> >>>>> If the code is position-independent then you might be able to simply
> >>>>> carve out a sufficient proportion from the start of the first entry or
> >>>>> the end of the last one, which would avoid splitting. If either of said
> >>>>> regions are too small for the monitor code then it's questionable as to
> >>>>> whether the OS can make use of it.
> >>>>
> >>>> The code /seems/ to be position-independent, but locations are so far
> >>>> hard-coded in those places that prepare it and move it around. Maybe we
> >>>> can decide about the location at runtime, maybe we can simply demand it
> >>>> to be at the end or the beginning of some bank.
> >>>
> >>> If it's possible to do so, it would seem like the nicest option to me.
> >>
> >> Using the top of memory for this seems like the most natural choice,
> > 
> > I think it needs to still be below 4G, doesn't it? So on large mem/LPAE
> > systems some care might be needed.
> 
> Argh. That would likely mean we had to split a bank (unless >2G comes in
> multiple banks), something I'd like to avoid having to implement.

I expect it is usual for the 4G boundary to coincide with a bank
boundary, even if memory spans the gap -- but I also don't think we can
rely on that.

> > It was suggested by Mark earlier in the thread that this stuff is
> > IMPLEMENTATION DEFINED. Is it possible that we simply don't need to
> > worry about these cross-world cache issues on Tegra?
> > 
> > (I must confess that until now I'd assumed that the cache lines were
> > tagged with the world which populated them to stop them interfering with
> > each other in this sort of way...)
> 
> I'm pretty sure that is no such thing as a cross-world cache problem.
> Otherwise the architecture or some implementation would have serious
> security issues as discussed earlier. To my understanding, Mark's
> suggestion is now targeting the concern that Linux may accidentally
> trigger accesses and, thus, stumble or create warnings at least.

Ah, then I've misunderstood/misremembered.

I think it is fair to say that if a NS-world OS deliberately touches a
region marked reserved or not described at all then it deserves whatever
fault it gets. But I think what Mark is saying is that just mapping the
region but never explicitly accessing it can still result in errors due
to e.g. prefetching or speculative behaviour which may use those
mappings.

IOW it is architecturally allowable for faults arising from accesses
never explicitly occurring in the code to result in OS visible faults.
Rather than, say, requiring such faults to be squashed until the
real/non-speculative access actually really occurs in the program.

Ian.

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Mark Rutland]

On Thu, Feb 19, 2015 at 10:13:58AM +0000, Ian Campbell wrote:
> On Thu, 2015-02-19 at 10:25 +0100, Jan Kiszka wrote:
> > On 2015-02-19 10:19, Ian Campbell wrote:
> > > On Thu, 2015-02-19 at 09:28 +0100, Thierry Reding wrote:
> > >> On Tue, Feb 17, 2015 at 11:55:24AM +0000, Mark Rutland wrote:
> > >>> [...]
> > >>>
> > >>>>>> This is getting invasive:
> > >>>>>>
> > >>>>>> If I add carveouts via adjusting memory banks, I need to account for the
> > >>>>>> case that an existing bank is split into two halves, creating additional
> > >>>>>> banks this way. But then current fdt_fixup_memory_banks will no longer
> > >>>>>> work due to its limitation to the number of physical banks. I could
> > >>>>>> always add one spare bank to that service, ok, but then the next use
> > >>>>>> case for carveouts will hit the wall again. So I better double that
> > >>>>>> limit, or so.
> > >>>>>
> > >>>>> Yeah, not fun.
> > >>>>>
> > >>>>> If the code is position-independent then you might be able to simply
> > >>>>> carve out a sufficient proportion from the start of the first entry or
> > >>>>> the end of the last one, which would avoid splitting. If either of said
> > >>>>> regions are too small for the monitor code then it's questionable as to
> > >>>>> whether the OS can make use of it.
> > >>>>
> > >>>> The code /seems/ to be position-independent, but locations are so far
> > >>>> hard-coded in those places that prepare it and move it around. Maybe we
> > >>>> can decide about the location at runtime, maybe we can simply demand it
> > >>>> to be at the end or the beginning of some bank.
> > >>>
> > >>> If it's possible to do so, it would seem like the nicest option to me.
> > >>
> > >> Using the top of memory for this seems like the most natural choice,
> > > 
> > > I think it needs to still be below 4G, doesn't it? So on large mem/LPAE
> > > systems some care might be needed.
> > 
> > Argh. That would likely mean we had to split a bank (unless >2G comes in
> > multiple banks), something I'd like to avoid having to implement.
> 
> I expect it is usual for the 4G boundary to coincide with a bank
> boundary, even if memory spans the gap -- but I also don't think we can
> rely on that.
> 
> > > It was suggested by Mark earlier in the thread that this stuff is
> > > IMPLEMENTATION DEFINED. Is it possible that we simply don't need to
> > > worry about these cross-world cache issues on Tegra?
> > > 
> > > (I must confess that until now I'd assumed that the cache lines were
> > > tagged with the world which populated them to stop them interfering with
> > > each other in this sort of way...)
> > 
> > I'm pretty sure that is no such thing as a cross-world cache problem.
> > Otherwise the architecture or some implementation would have serious
> > security issues as discussed earlier. To my understanding, Mark's
> > suggestion is now targeting the concern that Linux may accidentally
> > trigger accesses and, thus, stumble or create warnings at least.
> 
> Ah, then I've misunderstood/misremembered.
> 
> I think it is fair to say that if a NS-world OS deliberately touches a
> region marked reserved or not described at all then it deserves whatever
> fault it gets. But I think what Mark is saying is that just mapping the
> region but never explicitly accessing it can still result in errors due
> to e.g. prefetching or speculative behaviour which may use those
> mappings.

In this case the external memory security controller block likely can't
tell the difference (speculative and explicit accesses will look the
same on the bus). So any speculative access can trigger violations and
bring down the non-secure world (or just DoS the secure world if it
signals the secure world but provides a dummy result to non-secure
reads).

> IOW it is architecturally allowable for faults arising from accesses
> never explicitly occurring in the code to result in OS visible faults.
> Rather than, say, requiring such faults to be squashed until the
> real/non-speculative access actually really occurs in the program.

Yes. Anything mapped cacheable (or non-executable) can be speculatively
read, and if those accesses trigger some error on the bus (e.g. due to a
security controller), you'll get some kind of asynchronous abort
reported by the CPU.

Thanks,
Mark.

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Thierry Reding]

On Thu, Feb 19, 2015 at 10:25:56AM +0100, Jan Kiszka wrote:
> On 2015-02-19 10:19, Ian Campbell wrote:
> > On Thu, 2015-02-19 at 09:28 +0100, Thierry Reding wrote:
> >> On Tue, Feb 17, 2015 at 11:55:24AM +0000, Mark Rutland wrote:
> >>> [...]
> >>>
> >>>>>> This is getting invasive:
> >>>>>>
> >>>>>> If I add carveouts via adjusting memory banks, I need to account for the
> >>>>>> case that an existing bank is split into two halves, creating additional
> >>>>>> banks this way. But then current fdt_fixup_memory_banks will no longer
> >>>>>> work due to its limitation to the number of physical banks. I could
> >>>>>> always add one spare bank to that service, ok, but then the next use
> >>>>>> case for carveouts will hit the wall again. So I better double that
> >>>>>> limit, or so.
> >>>>>
> >>>>> Yeah, not fun.
> >>>>>
> >>>>> If the code is position-independent then you might be able to simply
> >>>>> carve out a sufficient proportion from the start of the first entry or
> >>>>> the end of the last one, which would avoid splitting. If either of said
> >>>>> regions are too small for the monitor code then it's questionable as to
> >>>>> whether the OS can make use of it.
> >>>>
> >>>> The code /seems/ to be position-independent, but locations are so far
> >>>> hard-coded in those places that prepare it and move it around. Maybe we
> >>>> can decide about the location at runtime, maybe we can simply demand it
> >>>> to be at the end or the beginning of some bank.
> >>>
> >>> If it's possible to do so, it would seem like the nicest option to me.
> >>
> >> Using the top of memory for this seems like the most natural choice,
> > 
> > I think it needs to still be below 4G, doesn't it? So on large mem/LPAE
> > systems some care might be needed.
> 
> Argh. That would likely mean we had to split a bank (unless >2G comes in
> multiple banks), something I'd like to avoid having to implement.

So if we wanted to (properly) support systems with more than 2 GiB of
RAM we'd need to make U-Boot LPAE aware. That would cause other kinds of
problems, though, and I'm not sure that it's even possible. Looking at
the register sets involved it seems like we can't have a reset vector
pointing at anything beyond the 4 GiB boundary anyway. It would also
mean that a bunch of things might break since I think there are many
occurrences in U-Boot where pointers are assumed to be 32-bit.

Perhaps a compromise for now would be to truncate the amount of
available memory to 2 GiB if PSCI is used? That's not really optimal but
should give us something to experiment. I don't think any of the boards
supported upstream have more than 2 GiB, though I'm sure there will be
in the not-so-distant future. Most of those may be 64-bit ARM, though,
in which case the problem goes away somewhat.

Unfortunately it seems like even for 64-bit SoCs the 32-bit registers
for the reset vectors remain, so it would seem like at least for Tegra
we're going to need code to split up the memory node eventually. Either
that or we move the monitor to the beginning of RAM. But I don't think
we can do that either because the Linux kernel will decompress to an
offset of 0x00008000 after the start of RAM.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/7f8a0bd2/attachment.sig>

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Mark Rutland]

On Thu, Feb 19, 2015 at 09:25:56AM +0000, Jan Kiszka wrote:
> On 2015-02-19 10:19, Ian Campbell wrote:
> > On Thu, 2015-02-19 at 09:28 +0100, Thierry Reding wrote:
> >> On Tue, Feb 17, 2015 at 11:55:24AM +0000, Mark Rutland wrote:
> >>> [...]
> >>>
> >>>>>> This is getting invasive:
> >>>>>>
> >>>>>> If I add carveouts via adjusting memory banks, I need to account for the
> >>>>>> case that an existing bank is split into two halves, creating additional
> >>>>>> banks this way. But then current fdt_fixup_memory_banks will no longer
> >>>>>> work due to its limitation to the number of physical banks. I could
> >>>>>> always add one spare bank to that service, ok, but then the next use
> >>>>>> case for carveouts will hit the wall again. So I better double that
> >>>>>> limit, or so.
> >>>>>
> >>>>> Yeah, not fun.
> >>>>>
> >>>>> If the code is position-independent then you might be able to simply
> >>>>> carve out a sufficient proportion from the start of the first entry or
> >>>>> the end of the last one, which would avoid splitting. If either of said
> >>>>> regions are too small for the monitor code then it's questionable as to
> >>>>> whether the OS can make use of it.
> >>>>
> >>>> The code /seems/ to be position-independent, but locations are so far
> >>>> hard-coded in those places that prepare it and move it around. Maybe we
> >>>> can decide about the location at runtime, maybe we can simply demand it
> >>>> to be at the end or the beginning of some bank.
> >>>
> >>> If it's possible to do so, it would seem like the nicest option to me.
> >>
> >> Using the top of memory for this seems like the most natural choice,
> > 
> > I think it needs to still be below 4G, doesn't it? So on large mem/LPAE
> > systems some care might be needed.
> 
> Argh. That would likely mean we had to split a bank (unless >2G comes in
> multiple banks), something I'd like to avoid having to implement.
> 
> > 
> > It was suggested by Mark earlier in the thread that this stuff is
> > IMPLEMENTATION DEFINED. Is it possible that we simply don't need to
> > worry about these cross-world cache issues on Tegra?
> > 
> > (I must confess that until now I'd assumed that the cache lines were
> > tagged with the world which populated them to stop them interfering with
> > each other in this sort of way...)
> 
> I'm pretty sure that is no such thing as a cross-world cache problem.
> Otherwise the architecture or some implementation would have serious
> security issues as discussed earlier. To my understanding, Mark's
> suggestion is now targeting the concern that Linux may accidentally
> trigger accesses and, thus, stumble or create warnings at least.

Yup.

If the memory is protected by some configurable security controller (as
seems to be the case on Tegra), the non-secure side accessing any memory
protected by it will result in a violation (and presumably bring down
the non-secure world). We need to prevent speculative accesses (the
security controller can't tell the difference), and therefore cannot map
the memory at all (so a /memreserve/ is insufficient).

Depending on implementation details there are other potential problems,
and carving out the memory explicitly solves all that I am aware of
without having to rely on implementation-specific details.

Thanks,
Mark.

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Thierry Reding]

On Tue, Feb 17, 2015 at 09:09:57AM +0100, Jan Kiszka wrote:
> On 2015-02-16 16:38, Jan Kiszka wrote:
> > On 2015-02-16 15:56, Mark Rutland wrote:
> >> On Mon, Feb 16, 2015 at 02:31:21PM +0000, Jan Kiszka wrote:
> >>> On 2015-02-16 15:25, Mark Rutland wrote:
> >>>> On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
> >>>>> On 2015-02-16 14:42, Mark Rutland wrote:
> >>>>>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
> >>>>>>> From: Ian Campbell <ijc@hellion.org.uk>
> >>>>>>>
> >>>>>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
> >>>>>>> it has been relocated, so the reservation of __secure_start does not apply.
> >>>>>>>
> >>>>>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
> >>>>>>> region.
> >>>>>>>
> >>>>>>> This will be used in a subsequent patch for Jetson-TK1
> >>>>>>
> >>>>>> Using a memreserve and allowing the OS to map the memory but not poke it
> >>>>>> can be problematic due to the potential of mismatched attributes between
> >>>>>> the monitor and the OS.
> >>>>>
> >>>>> OK, here my knowledge is not yet sufficient to process this remark. What
> >>>>> kind of problems can arise from what kind of attribute mismatch? And why
> >>>>> should the OS be able to cause problems for the monitor?
> >>>>
> >>>> For example, consider the case of the region being mapped cacheable by
> >>>> the OS but not by the monitor. The monitor communicates between cores
> >>>> expecting to never hit in a cache (because it uses a non-cacheable
> >>>> mapping), but the mapping used by the OS can cause the region to be
> >>>> allocated into caches at any point in time even if it never accesses the
> >>>> region explicitly.
> >>>>
> >>>> The CPU _may_ hit in a cache even if making a non-cacheable access (this
> >>>> is called an "unexepcted data cache hit"), so the cache allocations
> >>>> caused by the OS can mask data other CPUs wrote straight to memory.
> >>>>
> >>>> Other than that case, I believe the rules given in the ARM ARM for
> >>>> mismatched memory attributes may apply for similar reasons.  Thus
> >>>> allowing the OS to map this memory can cause a loss of coherency on the
> >>>> monitor side, if the OS and monitor map the region with different
> >>>> attributes.
> >>>>
> >>>> This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
> >>>> system you're dealing with. I don't immediately know whether that is the
> >>>> case, however. Never telling the OS about the memory in the first place
> >>>> avoids the possibility in all cases.
> >>>
> >>> But from a security point of view, it must not matter if the OS maps the
> >>> memory or not - the monitor must be robust against that, no? If the
> >>> architecture cannot provide such guarantees, it has to be worked around
> >>> in software in the monitor (I hope you can do so...).
> >>
> >> Well, yes and no.
> >>
> >> In this case it sounds like due to the security controller you should
> >> never encounter the mismatched attributes issue in the first place,
> >> though you may encounter issues w.r.t. speculative accesses triggering
> >> violations arbitrarily. Not telling the OS about the secure memory means
> >> that said violations shouldn't occur in normal operation; only when the
> >> non-secure OS is trying to do something bad.
> >>
> >> If the OS has access to the memory, then you're already trusting it to
> >> not write to there or you can't trust that memory at all (and hence
> >> cannot use it). Given that means you must already assume that the OS is
> >> cooperative, it's simpler to not tell it about the memory than to add
> >> cache maintenance around every memory access within the monitor. You can
> >> never make things secure in this case, but you can at least offer the
> >> abstraction provided by PSCI.
> >>
> >> So as far as I can see in either case it's better to not tell the OS
> >> about the memory you wish to use from the monitor. If you have no HW
> >> protection and can't trust the OS then you've already lost, and if you
> >> do have HW protection you don't want it to trigger
> >> continuously/spuriously as a result of speculation.
> > 
> > OK, that makes sense again.
> > 
> > Now I just need to figure out how to split/adjust the memory node
> > instead of adding a reservation region.
> 
> This is getting invasive:
> 
> If I add carveouts via adjusting memory banks, I need to account for the
> case that an existing bank is split into two halves, creating additional
> banks this way. But then current fdt_fixup_memory_banks will no longer
> work due to its limitation to the number of physical banks. I could
> always add one spare bank to that service, ok, but then the next use
> case for carveouts will hit the wall again. So I better double that
> limit, or so.

fdt_fixup_memory_banks() will shout if it doesn't have enough banks, so
I suppose we could put that problem off to the configuration files. For
example we could add something like:

	#ifdef CONFIG_ARMV7_PSCI
	#  define CONFIG_NR_DRAM_BANKS 2
	#else
	#  define CONFIG_NR_DRAM_BANKS 1
	#endif

to tegra-common.h and make sure to define CONFIG_ARMV7_PSCI before that
is included. That could easily be extended using something like the
following if you're concerned about there being many carveout use-cases:

	#ifdef CONFIG_ARMV7_PSCI
	#  define PSCI_EXTRA_DRAM_BANKS 1
	#else
	#  define PSCI_EXTRA_DRAM_BANKS 0
	#endif

	#ifdef CONFIG_FOO
	#  define FOO_EXTRA_DRAM_BANKS 1
	#else
	#  define FOO_EXTRA_DRAM_BANKS 0
	#endif

	#define CONFIG_NR_DRAM_BANKS (1 +
				      PSCI_EXTRA_DRAM_BANKS +
				      FOO_EXTRA_DRAM_BANKS)

But I think it'd be good enough for now to go with the first snippet.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/4070be9a/attachment.sig>

[U-Boot] [PATCH v2 06/12] virt-dt: Allow reservation of the secure region when it is in a RAM carveout.

[Jan Kiszka]

On 2015-02-19 11:34, Thierry Reding wrote:
> On Tue, Feb 17, 2015 at 09:09:57AM +0100, Jan Kiszka wrote:
>> On 2015-02-16 16:38, Jan Kiszka wrote:
>>> On 2015-02-16 15:56, Mark Rutland wrote:
>>>> On Mon, Feb 16, 2015 at 02:31:21PM +0000, Jan Kiszka wrote:
>>>>> On 2015-02-16 15:25, Mark Rutland wrote:
>>>>>> On Mon, Feb 16, 2015 at 01:51:37PM +0000, Jan Kiszka wrote:
>>>>>>> On 2015-02-16 14:42, Mark Rutland wrote:
>>>>>>>> On Mon, Feb 16, 2015 at 12:54:43PM +0000, Jan Kiszka wrote:
>>>>>>>>> From: Ian Campbell <ijc@hellion.org.uk>
>>>>>>>>>
>>>>>>>>> In this case the secure code lives in RAM, and hence needs to be reserved, but
>>>>>>>>> it has been relocated, so the reservation of __secure_start does not apply.
>>>>>>>>>
>>>>>>>>> Add support for setting CONFIG_ARMV7_SECURE_RESERVE_SIZE to reserve such a
>>>>>>>>> region.
>>>>>>>>>
>>>>>>>>> This will be used in a subsequent patch for Jetson-TK1
>>>>>>>>
>>>>>>>> Using a memreserve and allowing the OS to map the memory but not poke it
>>>>>>>> can be problematic due to the potential of mismatched attributes between
>>>>>>>> the monitor and the OS.
>>>>>>>
>>>>>>> OK, here my knowledge is not yet sufficient to process this remark. What
>>>>>>> kind of problems can arise from what kind of attribute mismatch? And why
>>>>>>> should the OS be able to cause problems for the monitor?
>>>>>>
>>>>>> For example, consider the case of the region being mapped cacheable by
>>>>>> the OS but not by the monitor. The monitor communicates between cores
>>>>>> expecting to never hit in a cache (because it uses a non-cacheable
>>>>>> mapping), but the mapping used by the OS can cause the region to be
>>>>>> allocated into caches at any point in time even if it never accesses the
>>>>>> region explicitly.
>>>>>>
>>>>>> The CPU _may_ hit in a cache even if making a non-cacheable access (this
>>>>>> is called an "unexepcted data cache hit"), so the cache allocations
>>>>>> caused by the OS can mask data other CPUs wrote straight to memory.
>>>>>>
>>>>>> Other than that case, I believe the rules given in the ARM ARM for
>>>>>> mismatched memory attributes may apply for similar reasons.  Thus
>>>>>> allowing the OS to map this memory can cause a loss of coherency on the
>>>>>> monitor side, if the OS and monitor map the region with different
>>>>>> attributes.
>>>>>>
>>>>>> This is all IMPLEMENTATION DEFINED, so it may be that you're fine on the
>>>>>> system you're dealing with. I don't immediately know whether that is the
>>>>>> case, however. Never telling the OS about the memory in the first place
>>>>>> avoids the possibility in all cases.
>>>>>
>>>>> But from a security point of view, it must not matter if the OS maps the
>>>>> memory or not - the monitor must be robust against that, no? If the
>>>>> architecture cannot provide such guarantees, it has to be worked around
>>>>> in software in the monitor (I hope you can do so...).
>>>>
>>>> Well, yes and no.
>>>>
>>>> In this case it sounds like due to the security controller you should
>>>> never encounter the mismatched attributes issue in the first place,
>>>> though you may encounter issues w.r.t. speculative accesses triggering
>>>> violations arbitrarily. Not telling the OS about the secure memory means
>>>> that said violations shouldn't occur in normal operation; only when the
>>>> non-secure OS is trying to do something bad.
>>>>
>>>> If the OS has access to the memory, then you're already trusting it to
>>>> not write to there or you can't trust that memory at all (and hence
>>>> cannot use it). Given that means you must already assume that the OS is
>>>> cooperative, it's simpler to not tell it about the memory than to add
>>>> cache maintenance around every memory access within the monitor. You can
>>>> never make things secure in this case, but you can at least offer the
>>>> abstraction provided by PSCI.
>>>>
>>>> So as far as I can see in either case it's better to not tell the OS
>>>> about the memory you wish to use from the monitor. If you have no HW
>>>> protection and can't trust the OS then you've already lost, and if you
>>>> do have HW protection you don't want it to trigger
>>>> continuously/spuriously as a result of speculation.
>>>
>>> OK, that makes sense again.
>>>
>>> Now I just need to figure out how to split/adjust the memory node
>>> instead of adding a reservation region.
>>
>> This is getting invasive:
>>
>> If I add carveouts via adjusting memory banks, I need to account for the
>> case that an existing bank is split into two halves, creating additional
>> banks this way. But then current fdt_fixup_memory_banks will no longer
>> work due to its limitation to the number of physical banks. I could
>> always add one spare bank to that service, ok, but then the next use
>> case for carveouts will hit the wall again. So I better double that
>> limit, or so.
> 
> fdt_fixup_memory_banks() will shout if it doesn't have enough banks, so
> I suppose we could put that problem off to the configuration files. For
> example we could add something like:
> 
> 	#ifdef CONFIG_ARMV7_PSCI
> 	#  define CONFIG_NR_DRAM_BANKS 2
> 	#else
> 	#  define CONFIG_NR_DRAM_BANKS 1
> 	#endif
> 
> to tegra-common.h and make sure to define CONFIG_ARMV7_PSCI before that
> is included. That could easily be extended using something like the
> following if you're concerned about there being many carveout use-cases:
> 
> 	#ifdef CONFIG_ARMV7_PSCI
> 	#  define PSCI_EXTRA_DRAM_BANKS 1
> 	#else
> 	#  define PSCI_EXTRA_DRAM_BANKS 0
> 	#endif
> 
> 	#ifdef CONFIG_FOO
> 	#  define FOO_EXTRA_DRAM_BANKS 1
> 	#else
> 	#  define FOO_EXTRA_DRAM_BANKS 0
> 	#endif
> 
> 	#define CONFIG_NR_DRAM_BANKS (1 +
> 				      PSCI_EXTRA_DRAM_BANKS +
> 				      FOO_EXTRA_DRAM_BANKS)
> 
> But I think it'd be good enough for now to go with the first snippet.

Well, I rather hope we can get away with v3 from the time being, i.e.
enforced beginning/end of bank.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 07/12] tegra: Make tegra_powergate_power_on public

[Jan Kiszka]

Will be used for unpowergating CPUs.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/tegra-common/powergate.c       | 2 +-
 arch/arm/include/asm/arch-tegra/powergate.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/cpu/tegra-common/powergate.c b/arch/arm/cpu/tegra-common/powergate.c
index 439cff3..6331cd4 100644
--- a/arch/arm/cpu/tegra-common/powergate.c
+++ b/arch/arm/cpu/tegra-common/powergate.c
@@ -44,7 +44,7 @@ static int tegra_powergate_set(enum tegra_powergate id, bool state)
 	return -ETIMEDOUT;
 }
 
-static int tegra_powergate_power_on(enum tegra_powergate id)
+int tegra_powergate_power_on(enum tegra_powergate id)
 {
 	return tegra_powergate_set(id, true);
 }
diff --git a/arch/arm/include/asm/arch-tegra/powergate.h b/arch/arm/include/asm/arch-tegra/powergate.h
index 130b58b..2e491f1 100644
--- a/arch/arm/include/asm/arch-tegra/powergate.h
+++ b/arch/arm/include/asm/arch-tegra/powergate.h
@@ -33,6 +33,7 @@ enum tegra_powergate {
 
 int tegra_powergate_sequence_power_up(enum tegra_powergate id,
 				      enum periph_id periph);
+int tegra_powergate_power_on(enum tegra_powergate id);
 int tegra_powergate_power_off(enum tegra_powergate id);
 
 #endif
-- 
2.1.4

[U-Boot] [PATCH v2 08/12] tegra: Add ap_pm_init hook

[Jan Kiszka]

This function will be used to initialize CPU power management for Tegra
SOCs. For now it does nothing.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/include/asm/arch-tegra/ap.h | 5 +++++
 board/nvidia/common/board.c          | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/arch/arm/include/asm/arch-tegra/ap.h b/arch/arm/include/asm/arch-tegra/ap.h
index 5c8be94..208db90 100644
--- a/arch/arm/include/asm/arch-tegra/ap.h
+++ b/arch/arm/include/asm/arch-tegra/ap.h
@@ -63,6 +63,11 @@ int tegra_get_chip(void);
  */
 int tegra_get_sku_info(void);
 
+/**
+ * Initialize power management for application processors
+ */
+void ap_pm_init(void);
+
 /* Do any chip-specific cache config */
 void config_cache(void);
 
diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index 80ef8fd..c62b3da 100644
--- a/board/nvidia/common/board.c
+++ b/board/nvidia/common/board.c
@@ -21,6 +21,7 @@
 #include <asm/arch/pwm.h>
 #endif
 #include <asm/arch/tegra.h>
+#include <asm/arch-tegra/ap.h>
 #include <asm/arch-tegra/board.h>
 #include <asm/arch-tegra/clk_rst.h>
 #include <asm/arch-tegra/pmc.h>
@@ -56,6 +57,7 @@ const struct tegra_sysinfo sysinfo = {
 	CONFIG_TEGRA_BOARD_STRING
 };
 
+__weak void ap_pm_init(void) {}
 __weak void pinmux_init(void) {}
 __weak void pin_mux_usb(void) {}
 __weak void pin_mux_spi(void) {}
@@ -96,6 +98,8 @@ int board_init(void)
 	clock_init();
 	clock_verify();
 
+	ap_pm_init();
+
 #ifdef CONFIG_TEGRA_SPI
 	pin_mux_spi();
 #endif
-- 
2.1.4

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Jan Kiszka]

This is based on Thierry Reding's work and uses Ian Campell's
preparatory patches. It comes with full support for CPU_ON/OFF PSCI
services. The algorithm used in this version for turning CPUs on and
off was proposed by Thierry Reding in
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
consists of first enabling CPU1..3 via the PMC, just to powergate them
again with the help of the Flow Controller. Once the Flow Controller is
in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
PSCI requests.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/Makefile               |   1 +
 arch/arm/cpu/armv7/tegra-common/Makefile  |   1 +
 arch/arm/cpu/armv7/tegra-common/psci.S    | 101 ++++++++++++++++++++++++++++++
 arch/arm/cpu/armv7/tegra124/Makefile      |   7 +++
 arch/arm/cpu/armv7/tegra124/ap.c          |  44 +++++++++++++
 arch/arm/include/asm/arch-tegra124/flow.h |   5 ++
 6 files changed, 159 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/tegra-common/psci.S
 create mode 100644 arch/arm/cpu/armv7/tegra124/Makefile
 create mode 100644 arch/arm/cpu/armv7/tegra124/ap.c

diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
index 409e6f5..616b6cc 100644
--- a/arch/arm/cpu/armv7/Makefile
+++ b/arch/arm/cpu/armv7/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_SOCFPGA) += socfpga/
 obj-$(if $(filter stv0991,$(SOC)),y) += stv0991/
 obj-$(CONFIG_ARCH_SUNXI) += sunxi/
 obj-$(CONFIG_TEGRA20) += tegra20/
+obj-$(CONFIG_TEGRA124) += tegra124/
 obj-$(CONFIG_U8500) += u8500/
 obj-$(CONFIG_ARCH_UNIPHIER) += uniphier/
 obj-$(CONFIG_VF610) += vf610/
diff --git a/arch/arm/cpu/armv7/tegra-common/Makefile b/arch/arm/cpu/armv7/tegra-common/Makefile
index 463c260..89355ca 100644
--- a/arch/arm/cpu/armv7/tegra-common/Makefile
+++ b/arch/arm/cpu/armv7/tegra-common/Makefile
@@ -7,4 +7,5 @@
 # SPDX-License-Identifier:	GPL-2.0+
 #
 
+obj-$(CONFIG_ARMV7_PSCI) += psci.o
 obj-$(CONFIG_CMD_ENTERRCM) += cmd_enterrcm.o
diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
new file mode 100644
index 0000000..b7501fb
--- /dev/null
+++ b/arch/arm/cpu/armv7/tegra-common/psci.S
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2014, NVIDIA
+ * Copyright (C) 2015, Siemens AG
+ *
+ * Authors:
+ *  Thierry Reding <treding@nvidia.com>
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <linux/linkage.h>
+#include <asm/psci.h>
+
+	.pushsection ._secure.text, "ax"
+	.arch_extension sec
+
+#define TEGRA_SB_CSR_0			0x6000c200
+#define NS_RST_VEC_WR_DIS		(1 << 1)
+
+#define TEGRA_RESET_EXCEPTION_VECTOR	0x6000f100
+
+#define TEGRA_FLOW_CTRL_BASE		0x60007000
+#define FLOW_CTRL_CPU_CSR		0x08
+#define CSR_ENABLE			(1 << 0)
+#define CSR_IMMEDIATE_WAKE		(1 << 3)
+#define CSR_WAIT_WFI_SHIFT		8
+#define FLOW_CTRL_CPU1_CSR		0x18
+
+@ converts CPU ID into FLOW_CTRL_CPUn_CSR offset
+.macro get_csr_reg cpu, ofs, tmp
+	cmp	\cpu, #0		@ CPU0?
+	lsl	\tmp, \cpu, #3	@ multiple by 8 (register offset CPU1-3)
+	moveq	\ofs, #FLOW_CTRL_CPU_CSR
+	addne	\ofs, \tmp, #FLOW_CTRL_CPU1_CSR - 8
+.endm
+
+ENTRY(psci_arch_init)
+	mov	r6, lr
+
+	mrc	p15, 0, r5, c1, c1, 0	@ Read SCR
+	bic	r5, r5, #1		@ Secure mode
+	mcr	p15, 0, r5, c1, c1, 0	@ Write SCR
+	isb
+
+	@ lock reset vector
+	ldr	r4, =TEGRA_SB_CSR_0
+	ldr	r5, [r4]
+	orr	r5, r5, #NS_RST_VEC_WR_DIS
+	str	r5, [r4]
+
+	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
+	and	r4, r4, #7		@ number of CPUs in cluster
+	bl	psci_get_cpu_stack_top
+	mov	sp, r5
+
+	bx	r6
+ENDPROC(psci_arch_init)
+
+ENTRY(psci_cpu_off)
+	bl psci_cpu_off_common
+
+	mrc	p15, 0, r1, c0, c0, 5		@ MPIDR
+	and	r1, r1, #7			@ number of CPUs in cluster
+
+	get_csr_reg r1, r2, r3
+
+	ldr	r6, =TEGRA_FLOW_CTRL_BASE
+	mov	r5, #(CSR_ENABLE)
+	add	r5, r1, lsl #CSR_WAIT_WFI_SHIFT
+	str	r5, [r6, r2]
+
+_loop:	wfi
+	b	_loop
+ENDPROC(psci_cpu_off)
+
+ENTRY(psci_cpu_on)
+	push	{lr}
+
+	mov	r4, r1
+	bl	psci_get_cpu_stack_top	@ get stack top of target CPU
+	str	r2, [r5]		@ store target PC at stack top
+	dsb
+
+	ldr	r6, =TEGRA_RESET_EXCEPTION_VECTOR
+	ldr	r5, =psci_cpu_entry
+	str	r5, [r6]
+
+	get_csr_reg r1, r2, r3
+
+	ldr	r6, =TEGRA_FLOW_CTRL_BASE
+	mov	r5, #(CSR_IMMEDIATE_WAKE | CSR_ENABLE)
+	str	r5, [r6, r2]
+
+	mov	r0, #ARM_PSCI_RET_SUCCESS	@ Return PSCI_RET_SUCCESS
+	pop	{pc}
+ENDPROC(psci_cpu_on)
+
+	.globl psci_text_end
+psci_text_end:
+	.popsection
diff --git a/arch/arm/cpu/armv7/tegra124/Makefile b/arch/arm/cpu/armv7/tegra124/Makefile
new file mode 100644
index 0000000..b907277
--- /dev/null
+++ b/arch/arm/cpu/armv7/tegra124/Makefile
@@ -0,0 +1,7 @@
+#
+# (C) Copyright 2015, Siemens AG
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+
+obj-$(CONFIG_ARMV7_PSCI) += ap.o
diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
new file mode 100644
index 0000000..eebc0ea
--- /dev/null
+++ b/arch/arm/cpu/armv7/tegra124/ap.c
@@ -0,0 +1,44 @@
+/*
+ * (C) Copyright 2015, Siemens AG
+ * Author: Jan Kiszka <jan.kiszka@siemens.com>
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/arch/flow.h>
+#include <asm/arch/powergate.h>
+#include <asm/arch-tegra/ap.h>
+#include <asm/arch-tegra/pmc.h>
+
+static void park_cpu(void)
+{
+	while (1)
+		asm volatile("wfi");
+}
+
+void ap_pm_init(void)
+{
+	struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
+	struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
+
+	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
+
+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
+
+	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
+	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
+	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
+
+	writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
+	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
+	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
+
+	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
+						  (1 << TEGRA_POWERGATE_CPU2) |
+						  (1 << TEGRA_POWERGATE_CPU3)))
+		/* wait */;
+}
diff --git a/arch/arm/include/asm/arch-tegra124/flow.h b/arch/arm/include/asm/arch-tegra124/flow.h
index 0db1881..d5f24a0 100644
--- a/arch/arm/include/asm/arch-tegra124/flow.h
+++ b/arch/arm/include/asm/arch-tegra124/flow.h
@@ -37,4 +37,9 @@ struct flow_ctlr {
 /* FLOW_CTLR_CLUSTER_CONTROL_0 0x2c */
 #define ACTIVE_LP		(1 << 0)
 
+/* CPUn_CSR_0 */
+#define CSR_ENABLE		(1 << 0)
+#define CSR_IMMEDIATE_WAKE	(1 << 3)
+#define CSR_WAIT_WFI_SHIFT	8
+
 #endif	/*  _TEGRA124_FLOW_H_ */
-- 
2.1.4

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Stephen Warren]

On 02/16/2015 05:54 AM, Jan Kiszka wrote:
> This is based on Thierry Reding's work and uses Ian Campell's
> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
> services. The algorithm used in this version for turning CPUs on and
> off was proposed by Thierry Reding in
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
> consists of first enabling CPU1..3 via the PMC, just to powergate them
> again with the help of the Flow Controller. Once the Flow Controller is
> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
> PSCI requests.

> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c

> +void ap_pm_init(void)
> +{
> +	struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
> +	struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
> +
> +	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
> +
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> +
> +	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
> +	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
> +	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
> +
> +	writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
> +	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
> +	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);

I would expect to set up halt_cpu*_events before powering on the CPUs, 
to make sure that they do the expected action on the very first WFI. So, 
shouldn't the order above be:

Write to halt_cpu*_events
Write to cpu*_csr
power_on

I didn't review the assembly code at all really; I assume Thierry will.

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Jan Kiszka]

On 2015-02-17 22:03, Stephen Warren wrote:
> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>> This is based on Thierry Reding's work and uses Ian Campell's
>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
>> services. The algorithm used in this version for turning CPUs on and
>> off was proposed by Thierry Reding in
>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
>> consists of first enabling CPU1..3 via the PMC, just to powergate them
>> again with the help of the Flow Controller. Once the Flow Controller is
>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
>> PSCI requests.
> 
>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c
>> b/arch/arm/cpu/armv7/tegra124/ap.c
> 
>> +void ap_pm_init(void)
>> +{
>> +    struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
>> +    struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
>> +
>> +    writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>> +
>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>> +
>> +    writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>> +    writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>> +    writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>> +
>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
> 
> I would expect to set up halt_cpu*_events before powering on the CPUs,
> to make sure that they do the expected action on the very first WFI. So,
> shouldn't the order above be:
> 
> Write to halt_cpu*_events
> Write to cpu*_csr
> power_on

Yeah, that was my original expectation as well. But

diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
index eebc0ea..240c71d 100644
--- a/arch/arm/cpu/armv7/tegra124/ap.c
+++ b/arch/arm/cpu/armv7/tegra124/ap.c
@@ -25,10 +25,6 @@ void ap_pm_init(void)
 
 	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
 
-	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
-	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
-	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
-
 	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
 	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
 	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
@@ -37,6 +33,10 @@ void ap_pm_init(void)
 	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
 	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
 
+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
+
 	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
 						  (1 << TEGRA_POWERGATE_CPU2) |
 						  (1 << TEGRA_POWERGATE_CPU3)))

doesn't work in practice. I suspect the power-on overwrites what the
flow controller configures in the PMC beforehand. But maybe someone can
explain this better than me.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Stephen Warren]

On 02/17/2015 11:13 PM, Jan Kiszka wrote:
> On 2015-02-17 22:03, Stephen Warren wrote:
>> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>>> This is based on Thierry Reding's work and uses Ian Campell's
>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
>>> services. The algorithm used in this version for turning CPUs on and
>>> off was proposed by Thierry Reding in
>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
>>> consists of first enabling CPU1..3 via the PMC, just to powergate them
>>> again with the help of the Flow Controller. Once the Flow Controller is
>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
>>> PSCI requests.
>>
>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c
>>> b/arch/arm/cpu/armv7/tegra124/ap.c
>>
>>> +void ap_pm_init(void)
>>> +{
>>> +    struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
>>> +    struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
>>> +
>>> +    writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>>> +
>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>> +
>>> +    writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>>> +    writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>>> +    writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>>> +
>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>>
>> I would expect to set up halt_cpu*_events before powering on the CPUs,
>> to make sure that they do the expected action on the very first WFI. So,
>> shouldn't the order above be:
>>
>> Write to halt_cpu*_events
>> Write to cpu*_csr
>> power_on
>
> Yeah, that was my original expectation as well. But
>
> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
> index eebc0ea..240c71d 100644
> --- a/arch/arm/cpu/armv7/tegra124/ap.c
> +++ b/arch/arm/cpu/armv7/tegra124/ap.c
> @@ -25,10 +25,6 @@ void ap_pm_init(void)
>
>   	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>
> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> -
>   	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>   	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>   	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
> @@ -37,6 +33,10 @@ void ap_pm_init(void)
>   	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>   	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> +
>   	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
>   						  (1 << TEGRA_POWERGATE_CPU2) |
>   						  (1 << TEGRA_POWERGATE_CPU3)))
>
> doesn't work in practice. I suspect the power-on overwrites what the
> flow controller configures in the PMC beforehand. But maybe someone can
> explain this better than me.

Thierry, Peter, can you comment on why that is, and whether the original 
code sequence is safe; does it matter that the target CPU executes WFI 
before the flow controller is configured what to do on WFI?

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Thierry Reding]

On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote:
> On 02/17/2015 11:13 PM, Jan Kiszka wrote:
> >On 2015-02-17 22:03, Stephen Warren wrote:
> >>On 02/16/2015 05:54 AM, Jan Kiszka wrote:
> >>>This is based on Thierry Reding's work and uses Ian Campell's
> >>>preparatory patches. It comes with full support for CPU_ON/OFF PSCI
> >>>services. The algorithm used in this version for turning CPUs on and
> >>>off was proposed by Thierry Reding in
> >>>http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
> >>>consists of first enabling CPU1..3 via the PMC, just to powergate them
> >>>again with the help of the Flow Controller. Once the Flow Controller is
> >>>in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
> >>>PSCI requests.
> >>
> >>>diff --git a/arch/arm/cpu/armv7/tegra124/ap.c
> >>>b/arch/arm/cpu/armv7/tegra124/ap.c
> >>
> >>>+void ap_pm_init(void)
> >>>+{
> >>>+    struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
> >>>+    struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
> >>>+
> >>>+    writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
> >>>+
> >>>+    tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> >>>+    tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> >>>+    tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> >>>+
> >>>+    writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
> >>>+    writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
> >>>+    writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
> >>>+
> >>>+    writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
> >>>+    writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
> >>>+    writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
> >>
> >>I would expect to set up halt_cpu*_events before powering on the CPUs,
> >>to make sure that they do the expected action on the very first WFI. So,
> >>shouldn't the order above be:
> >>
> >>Write to halt_cpu*_events
> >>Write to cpu*_csr
> >>power_on
> >
> >Yeah, that was my original expectation as well. But
> >
> >diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
> >index eebc0ea..240c71d 100644
> >--- a/arch/arm/cpu/armv7/tegra124/ap.c
> >+++ b/arch/arm/cpu/armv7/tegra124/ap.c
> >@@ -25,10 +25,6 @@ void ap_pm_init(void)
> >
> >  	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
> >
> >-	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> >-	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> >-	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> >-
> >  	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
> >  	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
> >  	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
> >@@ -37,6 +33,10 @@ void ap_pm_init(void)
> >  	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
> >  	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
> >
> >+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> >+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> >+	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> >+
> >  	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
> >  						  (1 << TEGRA_POWERGATE_CPU2) |
> >  						  (1 << TEGRA_POWERGATE_CPU3)))
> >
> >doesn't work in practice. I suspect the power-on overwrites what the
> >flow controller configures in the PMC beforehand. But maybe someone can
> >explain this better than me.
> 
> Thierry, Peter, can you comment on why that is, and whether the original
> code sequence is safe; does it matter that the target CPU executes WFI
> before the flow controller is configured what to do on WFI?

As I mentioned before, I don't think it's safe to change the powergate
status of more than one partition at once. I'm not sure that this will
change anything regarding the relative positioning of powergate on vs.
writing CPU halt events, but I agree with Stephen that running the CPU
without the halt events being programmed could cause them to simply go
into a WFI without them actually being turned off.

Perhaps if unpowergating after writing the halt events registers doesn't
work a safer way would be to go and forcibly wake up all CPUs again
after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)?

I haven't seen anything in the documentation regarding why unpowergating
after writing halt event registers wouldn't work. I'm sure I haven't
looked at all the documentation, but this is about as knowledgeable as I
am regarding the CPUs and the flow controller. Perhaps Peter will indeed
know more than that.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/eb00ec00/attachment.sig>

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Jan Kiszka]

On 2015-02-19 10:14, Thierry Reding wrote:
> On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote:
>> On 02/17/2015 11:13 PM, Jan Kiszka wrote:
>>> On 2015-02-17 22:03, Stephen Warren wrote:
>>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>>>>> This is based on Thierry Reding's work and uses Ian Campell's
>>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
>>>>> services. The algorithm used in this version for turning CPUs on and
>>>>> off was proposed by Thierry Reding in
>>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
>>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them
>>>>> again with the help of the Flow Controller. Once the Flow Controller is
>>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
>>>>> PSCI requests.
>>>>
>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c
>>>>> b/arch/arm/cpu/armv7/tegra124/ap.c
>>>>
>>>>> +void ap_pm_init(void)
>>>>> +{
>>>>> +    struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
>>>>> +    struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
>>>>> +
>>>>> +    writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>>>>> +
>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>>>> +
>>>>> +    writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>>>>> +    writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>>>>> +    writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>>>>> +
>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>>>>
>>>> I would expect to set up halt_cpu*_events before powering on the CPUs,
>>>> to make sure that they do the expected action on the very first WFI. So,
>>>> shouldn't the order above be:
>>>>
>>>> Write to halt_cpu*_events
>>>> Write to cpu*_csr
>>>> power_on
>>>
>>> Yeah, that was my original expectation as well. But
>>>
>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
>>> index eebc0ea..240c71d 100644
>>> --- a/arch/arm/cpu/armv7/tegra124/ap.c
>>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c
>>> @@ -25,10 +25,6 @@ void ap_pm_init(void)
>>>
>>>  	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>>>
>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>> -
>>>  	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>>>  	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>>>  	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>>> @@ -37,6 +33,10 @@ void ap_pm_init(void)
>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>>>
>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>> +
>>>  	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
>>>  						  (1 << TEGRA_POWERGATE_CPU2) |
>>>  						  (1 << TEGRA_POWERGATE_CPU3)))
>>>
>>> doesn't work in practice. I suspect the power-on overwrites what the
>>> flow controller configures in the PMC beforehand. But maybe someone can
>>> explain this better than me.
>>
>> Thierry, Peter, can you comment on why that is, and whether the original
>> code sequence is safe; does it matter that the target CPU executes WFI
>> before the flow controller is configured what to do on WFI?
> 
> As I mentioned before, I don't think it's safe to change the powergate
> status of more than one partition at once. I'm not sure that this will

tegra_powergate_set() already synchronizes the caller on the completion
of the switch. So the existing code is safe in this regard.

However, the K1 manual also states that the START bit of the toggle
register should be checked prior to starting a request. This is not done
by tegra_powergate_set() - probably because it is a K1-only requirement,
not applying to older CPUs. Not sure, though, if waiting for START=0 is
practically required when already waiting for the switch to be processed
by the PMC before continuing.

> change anything regarding the relative positioning of powergate on vs.
> writing CPU halt events, but I agree with Stephen that running the CPU
> without the halt events being programmed could cause them to simply go
> into a WFI without them actually being turned off.

The CPUs most probably go into WFI first, because we wait for the
partition to be reported as powered up, but it seems they can be turned
off while in WFI as well. I'm not basing this on anything stated in the
manual, just on experiments.

> 
> Perhaps if unpowergating after writing the halt events registers doesn't
> work a safer way would be to go and forcibly wake up all CPUs again
> after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)?
> 
> I haven't seen anything in the documentation regarding why unpowergating
> after writing halt event registers wouldn't work. I'm sure I haven't
> looked at all the documentation, but this is about as knowledgeable as I
> am regarding the CPUs and the flow controller. Perhaps Peter will indeed
> know more than that.

Yes, more insights would indeed be welcome!

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Jan Kiszka]

On 2015-02-20 10:36, Jan Kiszka wrote:
> On 2015-02-19 10:14, Thierry Reding wrote:
>> On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote:
>>> On 02/17/2015 11:13 PM, Jan Kiszka wrote:
>>>> On 2015-02-17 22:03, Stephen Warren wrote:
>>>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>>>>>> This is based on Thierry Reding's work and uses Ian Campell's
>>>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
>>>>>> services. The algorithm used in this version for turning CPUs on and
>>>>>> off was proposed by Thierry Reding in
>>>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
>>>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them
>>>>>> again with the help of the Flow Controller. Once the Flow Controller is
>>>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
>>>>>> PSCI requests.
>>>>>
>>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c
>>>>>> b/arch/arm/cpu/armv7/tegra124/ap.c
>>>>>
>>>>>> +void ap_pm_init(void)
>>>>>> +{
>>>>>> +    struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
>>>>>> +    struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
>>>>>> +
>>>>>> +    writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>>>>>> +
>>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>>>>> +
>>>>>> +    writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>>>>>> +    writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>>>>>> +    writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>>>>>> +
>>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
>>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>>>>>
>>>>> I would expect to set up halt_cpu*_events before powering on the CPUs,
>>>>> to make sure that they do the expected action on the very first WFI. So,
>>>>> shouldn't the order above be:
>>>>>
>>>>> Write to halt_cpu*_events
>>>>> Write to cpu*_csr
>>>>> power_on
>>>>
>>>> Yeah, that was my original expectation as well. But
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
>>>> index eebc0ea..240c71d 100644
>>>> --- a/arch/arm/cpu/armv7/tegra124/ap.c
>>>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c
>>>> @@ -25,10 +25,6 @@ void ap_pm_init(void)
>>>>
>>>>  	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>>>>
>>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>>> -
>>>>  	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>>>>  	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>>>>  	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>>>> @@ -37,6 +33,10 @@ void ap_pm_init(void)
>>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>>>>
>>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>>> +
>>>>  	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
>>>>  						  (1 << TEGRA_POWERGATE_CPU2) |
>>>>  						  (1 << TEGRA_POWERGATE_CPU3)))
>>>>
>>>> doesn't work in practice. I suspect the power-on overwrites what the
>>>> flow controller configures in the PMC beforehand. But maybe someone can
>>>> explain this better than me.
>>>
>>> Thierry, Peter, can you comment on why that is, and whether the original
>>> code sequence is safe; does it matter that the target CPU executes WFI
>>> before the flow controller is configured what to do on WFI?
>>
>> As I mentioned before, I don't think it's safe to change the powergate
>> status of more than one partition at once. I'm not sure that this will
> 
> tegra_powergate_set() already synchronizes the caller on the completion
> of the switch. So the existing code is safe in this regard.
> 
> However, the K1 manual also states that the START bit of the toggle
> register should be checked prior to starting a request. This is not done
> by tegra_powergate_set() - probably because it is a K1-only requirement,
> not applying to older CPUs. Not sure, though, if waiting for START=0 is
> practically required when already waiting for the switch to be processed
> by the PMC before continuing.
> 
>> change anything regarding the relative positioning of powergate on vs.
>> writing CPU halt events, but I agree with Stephen that running the CPU
>> without the halt events being programmed could cause them to simply go
>> into a WFI without them actually being turned off.
> 
> The CPUs most probably go into WFI first, because we wait for the
> partition to be reported as powered up, but it seems they can be turned
> off while in WFI as well. I'm not basing this on anything stated in the
> manual, just on experiments.
> 
>>
>> Perhaps if unpowergating after writing the halt events registers doesn't
>> work a safer way would be to go and forcibly wake up all CPUs again
>> after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)?
>>
>> I haven't seen anything in the documentation regarding why unpowergating
>> after writing halt event registers wouldn't work. I'm sure I haven't
>> looked at all the documentation, but this is about as knowledgeable as I
>> am regarding the CPUs and the flow controller. Perhaps Peter will indeed
>> know more than that.
> 
> Yes, more insights would indeed be welcome!

Ping regarding this last open point. I'm sitting on v4 of this series -
v3 had a nasty bug in the CNTFRQ fixup, I addressed the reviews and
added further cleanups - and I would like to close this topic eventually.

Jan

PS: After KVM, I was also able to get the Jailhouse hypervisor running
on the TK1 - thanks to this series (v4, to be precise):
http://thread.gmane.org/gmane.linux.jailhouse/2580

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Thierry Reding]

On Tue, Feb 24, 2015 at 08:23:55AM +0100, Jan Kiszka wrote:
> On 2015-02-20 10:36, Jan Kiszka wrote:
> > On 2015-02-19 10:14, Thierry Reding wrote:
> >> On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote:
> >>> On 02/17/2015 11:13 PM, Jan Kiszka wrote:
> >>>> On 2015-02-17 22:03, Stephen Warren wrote:
> >>>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
> >>>>>> This is based on Thierry Reding's work and uses Ian Campell's
> >>>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
> >>>>>> services. The algorithm used in this version for turning CPUs on and
> >>>>>> off was proposed by Thierry Reding in
> >>>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
> >>>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them
> >>>>>> again with the help of the Flow Controller. Once the Flow Controller is
> >>>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
> >>>>>> PSCI requests.
> >>>>>
> >>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c
> >>>>>> b/arch/arm/cpu/armv7/tegra124/ap.c
> >>>>>
> >>>>>> +void ap_pm_init(void)
> >>>>>> +{
> >>>>>> +    struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
> >>>>>> +    struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
> >>>>>> +
> >>>>>> +    writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
> >>>>>> +
> >>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> >>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> >>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> >>>>>> +
> >>>>>> +    writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
> >>>>>> +    writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
> >>>>>> +    writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
> >>>>>> +
> >>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
> >>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
> >>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
> >>>>>
> >>>>> I would expect to set up halt_cpu*_events before powering on the CPUs,
> >>>>> to make sure that they do the expected action on the very first WFI. So,
> >>>>> shouldn't the order above be:
> >>>>>
> >>>>> Write to halt_cpu*_events
> >>>>> Write to cpu*_csr
> >>>>> power_on
> >>>>
> >>>> Yeah, that was my original expectation as well. But
> >>>>
> >>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
> >>>> index eebc0ea..240c71d 100644
> >>>> --- a/arch/arm/cpu/armv7/tegra124/ap.c
> >>>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c
> >>>> @@ -25,10 +25,6 @@ void ap_pm_init(void)
> >>>>
> >>>>  	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
> >>>>
> >>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> >>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> >>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> >>>> -
> >>>>  	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
> >>>>  	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
> >>>>  	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
> >>>> @@ -37,6 +33,10 @@ void ap_pm_init(void)
> >>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
> >>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
> >>>>
> >>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> >>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> >>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> >>>> +
> >>>>  	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
> >>>>  						  (1 << TEGRA_POWERGATE_CPU2) |
> >>>>  						  (1 << TEGRA_POWERGATE_CPU3)))
> >>>>
> >>>> doesn't work in practice. I suspect the power-on overwrites what the
> >>>> flow controller configures in the PMC beforehand. But maybe someone can
> >>>> explain this better than me.
> >>>
> >>> Thierry, Peter, can you comment on why that is, and whether the original
> >>> code sequence is safe; does it matter that the target CPU executes WFI
> >>> before the flow controller is configured what to do on WFI?
> >>
> >> As I mentioned before, I don't think it's safe to change the powergate
> >> status of more than one partition at once. I'm not sure that this will
> > 
> > tegra_powergate_set() already synchronizes the caller on the completion
> > of the switch. So the existing code is safe in this regard.
> > 
> > However, the K1 manual also states that the START bit of the toggle
> > register should be checked prior to starting a request. This is not done
> > by tegra_powergate_set() - probably because it is a K1-only requirement,
> > not applying to older CPUs. Not sure, though, if waiting for START=0 is
> > practically required when already waiting for the switch to be processed
> > by the PMC before continuing.
> > 
> >> change anything regarding the relative positioning of powergate on vs.
> >> writing CPU halt events, but I agree with Stephen that running the CPU
> >> without the halt events being programmed could cause them to simply go
> >> into a WFI without them actually being turned off.
> > 
> > The CPUs most probably go into WFI first, because we wait for the
> > partition to be reported as powered up, but it seems they can be turned
> > off while in WFI as well. I'm not basing this on anything stated in the
> > manual, just on experiments.
> > 
> >>
> >> Perhaps if unpowergating after writing the halt events registers doesn't
> >> work a safer way would be to go and forcibly wake up all CPUs again
> >> after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)?
> >>
> >> I haven't seen anything in the documentation regarding why unpowergating
> >> after writing halt event registers wouldn't work. I'm sure I haven't
> >> looked at all the documentation, but this is about as knowledgeable as I
> >> am regarding the CPUs and the flow controller. Perhaps Peter will indeed
> >> know more than that.
> > 
> > Yes, more insights would indeed be welcome!
> 
> Ping regarding this last open point. I'm sitting on v4 of this series -
> v3 had a nasty bug in the CNTFRQ fixup, I addressed the reviews and
> added further cleanups - and I would like to close this topic eventually.

I applied your series (though I had to apply some patches manually
because I was applying on top of the latest master branch where a bunch
of Tegra-related files have moved around) and tested on Jetson TK1 as
well as Dalmore (Tegra114). I have a couple of follow-up patches that
shuffle around some code to share the PSCI implementation with Tegra114
since it should be compatible in that regard.

One of the things I also did was try to use the more natural sequence
that Stephen had pointed out. It turns out that, at least for me, that
works just fine both on Tegra114 and Tegra124. Do you think you could
look at the branch I uploaded and see if it works for you as well? One
other thing I've had to do was enable CONFIG_ARMV7_NONSEC (I also threw
in CONFIG_ARMV7_VIRT for good measure) so that the FDT would get
updated, which is no longer the case with only CONFIG_ARMV7_PSCI. The
last change I did was enable the SMMU from within U-Boot, which is
necessary if the kernel runs in non-secure mode because some of the
registers are secure-only.

My branch is here:

	https://github.com/thierryreding/u-boot/commits/staging/psci

It contains your v3 plus the above changes on top. I might have fumbled
the rebase, so it might not actually build until somewhere near the top
but I assumed you were going to do that anyway and therefore focused on
other parts.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150224/b90ea8e9/attachment.sig>

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Jan Kiszka]

On 2015-02-24 09:18, Thierry Reding wrote:
> On Tue, Feb 24, 2015 at 08:23:55AM +0100, Jan Kiszka wrote:
>> On 2015-02-20 10:36, Jan Kiszka wrote:
>>> On 2015-02-19 10:14, Thierry Reding wrote:
>>>> On Wed, Feb 18, 2015 at 09:34:53AM -0700, Stephen Warren wrote:
>>>>> On 02/17/2015 11:13 PM, Jan Kiszka wrote:
>>>>>> On 2015-02-17 22:03, Stephen Warren wrote:
>>>>>>> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>>>>>>>> This is based on Thierry Reding's work and uses Ian Campell's
>>>>>>>> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
>>>>>>>> services. The algorithm used in this version for turning CPUs on and
>>>>>>>> off was proposed by Thierry Reding in
>>>>>>>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It
>>>>>>>> consists of first enabling CPU1..3 via the PMC, just to powergate them
>>>>>>>> again with the help of the Flow Controller. Once the Flow Controller is
>>>>>>>> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
>>>>>>>> PSCI requests.
>>>>>>>
>>>>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c
>>>>>>>> b/arch/arm/cpu/armv7/tegra124/ap.c
>>>>>>>
>>>>>>>> +void ap_pm_init(void)
>>>>>>>> +{
>>>>>>>> +    struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
>>>>>>>> +    struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
>>>>>>>> +
>>>>>>>> +    writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>>>>>>>> +
>>>>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>>>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>>>>>>> +    tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>>>>>>> +
>>>>>>>> +    writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>>>>>>>> +    writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>>>>>>>> +    writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>>>>>>>> +
>>>>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
>>>>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>>>>>>>> +    writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>>>>>>>
>>>>>>> I would expect to set up halt_cpu*_events before powering on the CPUs,
>>>>>>> to make sure that they do the expected action on the very first WFI. So,
>>>>>>> shouldn't the order above be:
>>>>>>>
>>>>>>> Write to halt_cpu*_events
>>>>>>> Write to cpu*_csr
>>>>>>> power_on
>>>>>>
>>>>>> Yeah, that was my original expectation as well. But
>>>>>>
>>>>>> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
>>>>>> index eebc0ea..240c71d 100644
>>>>>> --- a/arch/arm/cpu/armv7/tegra124/ap.c
>>>>>> +++ b/arch/arm/cpu/armv7/tegra124/ap.c
>>>>>> @@ -25,10 +25,6 @@ void ap_pm_init(void)
>>>>>>
>>>>>>  	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
>>>>>>
>>>>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>>>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>>>>> -	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>>>>> -
>>>>>>  	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
>>>>>>  	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
>>>>>>  	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
>>>>>> @@ -37,6 +33,10 @@ void ap_pm_init(void)
>>>>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
>>>>>>  	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
>>>>>>
>>>>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
>>>>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
>>>>>> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
>>>>>> +
>>>>>>  	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
>>>>>>  						  (1 << TEGRA_POWERGATE_CPU2) |
>>>>>>  						  (1 << TEGRA_POWERGATE_CPU3)))
>>>>>>
>>>>>> doesn't work in practice. I suspect the power-on overwrites what the
>>>>>> flow controller configures in the PMC beforehand. But maybe someone can
>>>>>> explain this better than me.
>>>>>
>>>>> Thierry, Peter, can you comment on why that is, and whether the original
>>>>> code sequence is safe; does it matter that the target CPU executes WFI
>>>>> before the flow controller is configured what to do on WFI?
>>>>
>>>> As I mentioned before, I don't think it's safe to change the powergate
>>>> status of more than one partition at once. I'm not sure that this will
>>>
>>> tegra_powergate_set() already synchronizes the caller on the completion
>>> of the switch. So the existing code is safe in this regard.
>>>
>>> However, the K1 manual also states that the START bit of the toggle
>>> register should be checked prior to starting a request. This is not done
>>> by tegra_powergate_set() - probably because it is a K1-only requirement,
>>> not applying to older CPUs. Not sure, though, if waiting for START=0 is
>>> practically required when already waiting for the switch to be processed
>>> by the PMC before continuing.
>>>
>>>> change anything regarding the relative positioning of powergate on vs.
>>>> writing CPU halt events, but I agree with Stephen that running the CPU
>>>> without the halt events being programmed could cause them to simply go
>>>> into a WFI without them actually being turned off.
>>>
>>> The CPUs most probably go into WFI first, because we wait for the
>>> partition to be reported as powered up, but it seems they can be turned
>>> off while in WFI as well. I'm not basing this on anything stated in the
>>> manual, just on experiments.
>>>
>>>>
>>>> Perhaps if unpowergating after writing the halt events registers doesn't
>>>> work a safer way would be to go and forcibly wake up all CPUs again
>>>> after they are powered up (using the IMMEDIATE_WAKE bit in the CSR)?
>>>>
>>>> I haven't seen anything in the documentation regarding why unpowergating
>>>> after writing halt event registers wouldn't work. I'm sure I haven't
>>>> looked at all the documentation, but this is about as knowledgeable as I
>>>> am regarding the CPUs and the flow controller. Perhaps Peter will indeed
>>>> know more than that.
>>>
>>> Yes, more insights would indeed be welcome!
>>
>> Ping regarding this last open point. I'm sitting on v4 of this series -
>> v3 had a nasty bug in the CNTFRQ fixup, I addressed the reviews and
>> added further cleanups - and I would like to close this topic eventually.
> 
> I applied your series (though I had to apply some patches manually
> because I was applying on top of the latest master branch where a bunch
> of Tegra-related files have moved around) and tested on Jetson TK1 as
> well as Dalmore (Tegra114). I have a couple of follow-up patches that
> shuffle around some code to share the PSCI implementation with Tegra114
> since it should be compatible in that regard.

Can you use v4 on github instead
(https://github.com/siemens/u-boot/commits/jetson-tk1-v4)? It is based
on today's master and known to work better than v3.

> 
> One of the things I also did was try to use the more natural sequence
> that Stephen had pointed out. It turns out that, at least for me, that
> works just fine both on Tegra114 and Tegra124. Do you think you could
> look at the branch I uploaded and see if it works for you as well? One
> other thing I've had to do was enable CONFIG_ARMV7_NONSEC (I also threw
> in CONFIG_ARMV7_VIRT for good measure) so that the FDT would get
> updated, which is no longer the case with only CONFIG_ARMV7_PSCI. The
> last change I did was enable the SMMU from within U-Boot, which is
> necessary if the kernel runs in non-secure mode because some of the
> registers are secure-only.
> 
> My branch is here:
> 
> 	https://github.com/thierryreding/u-boot/commits/staging/psci
> 
> It contains your v3 plus the above changes on top. I might have fumbled
> the rebase, so it might not actually build until somewhere near the top
> but I assumed you were going to do that anyway and therefore focused on
> other parts.

I'll look into your changes in parallel.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Thierry Reding]

On Mon, Feb 16, 2015 at 01:54:46PM +0100, Jan Kiszka wrote:
> This is based on Thierry Reding's work and uses Ian Campell's
> preparatory patches. It comes with full support for CPU_ON/OFF PSCI
> services. The algorithm used in this version for turning CPUs on and
> off was proposed by Thierry Reding in
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/210881. It

I can't take full credit for this algorithm, it was originally Peter De
Schrijver who proposed it.

> consists of first enabling CPU1..3 via the PMC, just to powergate them
> again with the help of the Flow Controller. Once the Flow Controller is
> in place, we can leave the PMC alone while processing CPU_ON and CPU_OFF
> PSCI requests.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm/cpu/armv7/Makefile               |   1 +
>  arch/arm/cpu/armv7/tegra-common/Makefile  |   1 +
>  arch/arm/cpu/armv7/tegra-common/psci.S    | 101 ++++++++++++++++++++++++++++++
>  arch/arm/cpu/armv7/tegra124/Makefile      |   7 +++
>  arch/arm/cpu/armv7/tegra124/ap.c          |  44 +++++++++++++
>  arch/arm/include/asm/arch-tegra124/flow.h |   5 ++
>  6 files changed, 159 insertions(+)
>  create mode 100644 arch/arm/cpu/armv7/tegra-common/psci.S
>  create mode 100644 arch/arm/cpu/armv7/tegra124/Makefile
>  create mode 100644 arch/arm/cpu/armv7/tegra124/ap.c
> 
> diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
> index 409e6f5..616b6cc 100644
> --- a/arch/arm/cpu/armv7/Makefile
> +++ b/arch/arm/cpu/armv7/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_SOCFPGA) += socfpga/
>  obj-$(if $(filter stv0991,$(SOC)),y) += stv0991/
>  obj-$(CONFIG_ARCH_SUNXI) += sunxi/
>  obj-$(CONFIG_TEGRA20) += tegra20/
> +obj-$(CONFIG_TEGRA124) += tegra124/
>  obj-$(CONFIG_U8500) += u8500/
>  obj-$(CONFIG_ARCH_UNIPHIER) += uniphier/
>  obj-$(CONFIG_VF610) += vf610/
> diff --git a/arch/arm/cpu/armv7/tegra-common/Makefile b/arch/arm/cpu/armv7/tegra-common/Makefile
> index 463c260..89355ca 100644
> --- a/arch/arm/cpu/armv7/tegra-common/Makefile
> +++ b/arch/arm/cpu/armv7/tegra-common/Makefile
> @@ -7,4 +7,5 @@
>  # SPDX-License-Identifier:	GPL-2.0+
>  #
>  
> +obj-$(CONFIG_ARMV7_PSCI) += psci.o
>  obj-$(CONFIG_CMD_ENTERRCM) += cmd_enterrcm.o
> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
> new file mode 100644
> index 0000000..b7501fb
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (C) 2014, NVIDIA
> + * Copyright (C) 2015, Siemens AG
> + *
> + * Authors:
> + *  Thierry Reding <treding@nvidia.com>
> + *  Jan Kiszka <jan.kiszka@siemens.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/psci.h>
> +
> +	.pushsection ._secure.text, "ax"
> +	.arch_extension sec
> +
> +#define TEGRA_SB_CSR_0			0x6000c200
> +#define NS_RST_VEC_WR_DIS		(1 << 1)
> +
> +#define TEGRA_RESET_EXCEPTION_VECTOR	0x6000f100
> +
> +#define TEGRA_FLOW_CTRL_BASE		0x60007000
> +#define FLOW_CTRL_CPU_CSR		0x08
> +#define CSR_ENABLE			(1 << 0)
> +#define CSR_IMMEDIATE_WAKE		(1 << 3)
> +#define CSR_WAIT_WFI_SHIFT		8
> +#define FLOW_CTRL_CPU1_CSR		0x18
> +
> +@ converts CPU ID into FLOW_CTRL_CPUn_CSR offset
> +.macro get_csr_reg cpu, ofs, tmp
> +	cmp	\cpu, #0		@ CPU0?
> +	lsl	\tmp, \cpu, #3	@ multiple by 8 (register offset CPU1-3)
> +	moveq	\ofs, #FLOW_CTRL_CPU_CSR
> +	addne	\ofs, \tmp, #FLOW_CTRL_CPU1_CSR - 8
> +.endm
> +
> +ENTRY(psci_arch_init)
> +	mov	r6, lr
> +
> +	mrc	p15, 0, r5, c1, c1, 0	@ Read SCR
> +	bic	r5, r5, #1		@ Secure mode
> +	mcr	p15, 0, r5, c1, c1, 0	@ Write SCR
> +	isb
> +
> +	@ lock reset vector
> +	ldr	r4, =TEGRA_SB_CSR_0
> +	ldr	r5, [r4]
> +	orr	r5, r5, #NS_RST_VEC_WR_DIS
> +	str	r5, [r4]
> +
> +	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
> +	and	r4, r4, #7		@ number of CPUs in cluster

The comment here is somewhat confusing. Should this perhaps be something
like "index of CPU in cluster"?

> +	bl	psci_get_cpu_stack_top
> +	mov	sp, r5
> +
> +	bx	r6
> +ENDPROC(psci_arch_init)
> +
> +ENTRY(psci_cpu_off)
> +	bl psci_cpu_off_common
> +
> +	mrc	p15, 0, r1, c0, c0, 5		@ MPIDR
> +	and	r1, r1, #7			@ number of CPUs in cluster

Same here.

> diff --git a/arch/arm/cpu/armv7/tegra124/ap.c b/arch/arm/cpu/armv7/tegra124/ap.c
[...]

I think this code should work on Tegra114 as well. I'll go try them out
and confirm that. If it works out it would be nice to share this across
the two generations. It might even work on Tegra30, too.

> @@ -0,0 +1,44 @@
> +/*
> + * (C) Copyright 2015, Siemens AG
> + * Author: Jan Kiszka <jan.kiszka@siemens.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <asm/arch/flow.h>
> +#include <asm/arch/powergate.h>
> +#include <asm/arch-tegra/ap.h>
> +#include <asm/arch-tegra/pmc.h>
> +
> +static void park_cpu(void)
> +{
> +	while (1)
> +		asm volatile("wfi");
> +}
> +
> +void ap_pm_init(void)
> +{
> +	struct flow_ctlr *flow = (struct flow_ctlr *)NV_PA_FLOW_BASE;
> +	struct pmc_ctlr *pmc = (struct pmc_ctlr *)NV_PA_PMC_BASE;
> +
> +	writel((u32)park_cpu, EXCEP_VECTOR_CPU_RESET_VECTOR);
> +
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU1);
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU2);
> +	tegra_powergate_power_on(TEGRA_POWERGATE_CPU3);
> +
> +	writel((2 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu1_csr);
> +	writel((4 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu2_csr);
> +	writel((8 << CSR_WAIT_WFI_SHIFT) | CSR_ENABLE, &flow->cpu3_csr);
> +
> +	writel(EVENT_MODE_STOP, &flow->halt_cpu1_events);
> +	writel(EVENT_MODE_STOP, &flow->halt_cpu2_events);
> +	writel(EVENT_MODE_STOP, &flow->halt_cpu3_events);
> +
> +	while (readl(&pmc->pmc_pwrgate_status) & ((1 << TEGRA_POWERGATE_CPU1) |
> +						  (1 << TEGRA_POWERGATE_CPU2) |
> +						  (1 << TEGRA_POWERGATE_CPU3)))
> +		/* wait */;

Perhaps the wait should be folded into tegra_powergate_power_on()? I'm
not sure it's allowed to queue changes for more than a single partition
at a time.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/d3ee01f5/attachment.sig>

[U-Boot] [PATCH v2 09/12] tegra124: Add PSCI support for Tegra124

[Thierry Reding]

One more thing...

On Mon, Feb 16, 2015 at 01:54:46PM +0100, Jan Kiszka wrote:
> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
[...]
> +ENTRY(psci_arch_init)
> +	mov	r6, lr
> +
> +	mrc	p15, 0, r5, c1, c1, 0	@ Read SCR
> +	bic	r5, r5, #1		@ Secure mode
> +	mcr	p15, 0, r5, c1, c1, 0	@ Write SCR
> +	isb
> +
> +	@ lock reset vector
> +	ldr	r4, =TEGRA_SB_CSR_0
> +	ldr	r5, [r4]
> +	orr	r5, r5, #NS_RST_VEC_WR_DIS
> +	str	r5, [r4]

Perhaps extend the comment to mention that this locks the reset vector
for accesses from non-secure mode, otherwise it might be confusing that
psci_cpu_on actually writes the reset vector.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150219/a2586eb1/attachment.sig>

[U-Boot] [PATCH v2 10/12] jetson-tk1: Add PSCI configuration options and reserve secure code

[Jan Kiszka]

From: Ian Campbell <ijc@hellion.org.uk>

The secure world code is relocated to the MB just below the top of 4G, we
reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is
not protected in h/w. See next patch.

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/tegra124/Kconfig | 2 ++
 include/configs/jetson-tk1.h        | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/arch/arm/cpu/armv7/tegra124/Kconfig b/arch/arm/cpu/armv7/tegra124/Kconfig
index 88f627c..5114299 100644
--- a/arch/arm/cpu/armv7/tegra124/Kconfig
+++ b/arch/arm/cpu/armv7/tegra124/Kconfig
@@ -5,6 +5,8 @@ choice
 
 config TARGET_JETSON_TK1
 	bool "NVIDIA Tegra124 Jetson TK1 board"
+	select CPU_V7_HAS_NONSEC if !SPL_BUILD
+	select CPU_V7_HAS_VIRT if !SPL_BUILD
 
 config TARGET_NYAN_BIG
 	bool "Google/NVIDIA Nyan-big Chrombook"
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
index 0a79c7c..80c2952 100644
--- a/include/configs/jetson-tk1.h
+++ b/include/configs/jetson-tk1.h
@@ -81,4 +81,9 @@
 #include "tegra-common-usb-gadget.h"
 #include "tegra-common-post.h"
 
+#define CONFIG_ARMV7_PSCI			1
+/* Reserve top 1M for secure RAM */
+#define CONFIG_ARMV7_SECURE_BASE		0xfff00000
+#define CONFIG_ARMV7_SECURE_RESERVE_SIZE	0x00100000
+
 #endif /* __CONFIG_H */
-- 
2.1.4

[U-Boot] [PATCH v2 10/12] jetson-tk1: Add PSCI configuration options and reserve secure code

[Stephen Warren]

On 02/16/2015 05:54 AM, Jan Kiszka wrote:
> The secure world code is relocated to the MB just below the top of 4G, we
> reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE) but it is
> not protected in h/w. See next patch.

> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h

> +#define CONFIG_ARMV7_PSCI			1
> +/* Reserve top 1M for secure RAM */
> +#define CONFIG_ARMV7_SECURE_BASE		0xfff00000

Can that not be dynamic? What if the system only has 1GB RAM not 2GB. 
It's true that all shipped/public versions of this board do have 2GB 
AFAIK, but we have had internal versions with different amounts of RAM, 
and I don't think there's anything else in Tegra U-Boots that hard-codes 
RAM sizes.

> +#define CONFIG_ARMV7_SECURE_RESERVE_SIZE	0x00100000

[U-Boot] [PATCH v2 10/12] jetson-tk1: Add PSCI configuration options and reserve secure code

[Jan Kiszka]

On 2015-02-17 22:05, Stephen Warren wrote:
> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>> The secure world code is relocated to the MB just below the top of 4G, we
>> reserve it in the FDT (by setting CONFIG_ARMV7_SECURE_RESERVE_SIZE)
>> but it is
>> not protected in h/w. See next patch.
> 
>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
> 
>> +#define CONFIG_ARMV7_PSCI            1
>> +/* Reserve top 1M for secure RAM */
>> +#define CONFIG_ARMV7_SECURE_BASE        0xfff00000
> 
> Can that not be dynamic? What if the system only has 1GB RAM not 2GB.
> It's true that all shipped/public versions of this board do have 2GB
> AFAIK, but we have had internal versions with different amounts of RAM,
> and I don't think there's anything else in Tegra U-Boots that hard-codes
> RAM sizes.

I tested and checked the PSCI code again, and it turned out to be not
position independent yet (vector tables, functions pointers). Everything
can be fixed, but I wonder if this shouldn't be done on top, when we
have a concrete need.

Locating the monitor may involve more factors on different SoCs and
boards. So a general solution may be more complicated, even when dynamic
relocation itself is solved.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 11/12] tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

[Jan Kiszka]

From: Ian Campbell <ijc@hellion.org.uk>

These registers can be used to prevent non-secure world from accessing a
megabyte aligned region of RAM, use them to protect the u-boot secure monitor
code.

At first I tried to do this from s_init(), however this inexplicably causes
u-boot's networking (e.g. DHCP) to fail, while networking under Linux was fine.

So instead I have added a new weak arch function protect_secure_section()
called from relocate_secure_section() and reserved the region there. This is
better overall since it defers the reservation until after the sec vs. non-sec
decision (which can be influenced by an envvar) has been made when booting the
os.

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/virt-v7.c   |  5 +++++
 arch/arm/cpu/tegra-common/ap.c | 15 +++++++++++++++
 arch/arm/include/asm/system.h  |  1 +
 3 files changed, 21 insertions(+)

diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
index b69fd37..eb6195c 100644
--- a/arch/arm/cpu/armv7/virt-v7.c
+++ b/arch/arm/cpu/armv7/virt-v7.c
@@ -46,6 +46,10 @@ static unsigned long get_gicd_base_address(void)
 #endif
 }
 
+/* Define a specific version of this function to enable any available
+ * hardware protections for the reserved region */
+void __weak protect_secure_section(void) {}
+
 static void relocate_secure_section(void)
 {
 #ifdef CONFIG_ARMV7_SECURE_BASE
@@ -54,6 +58,7 @@ static void relocate_secure_section(void)
 	memcpy((void *)CONFIG_ARMV7_SECURE_BASE, __secure_start, sz);
 	flush_dcache_range(CONFIG_ARMV7_SECURE_BASE,
 			   CONFIG_ARMV7_SECURE_BASE + sz + 1);
+	protect_secure_section();
 	invalidate_icache_all();
 #endif
 }
diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c
index a17dfd1..f1d3070 100644
--- a/arch/arm/cpu/tegra-common/ap.c
+++ b/arch/arm/cpu/tegra-common/ap.c
@@ -10,6 +10,7 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/arch/gp_padctrl.h>
+#include <asm/arch/mc.h>
 #include <asm/arch-tegra/ap.h>
 #include <asm/arch-tegra/clock.h>
 #include <asm/arch-tegra/fuse.h>
@@ -154,6 +155,20 @@ static void init_pmc_scratch(void)
 	writel(odmdata, &pmc->pmc_scratch20);
 }
 
+#ifdef CONFIG_ARMV7_SECURE_RESERVE_SIZE
+void protect_secure_section(void)
+{
+	struct mc_ctlr *mc = (struct mc_ctlr *)NV_PA_MC_BASE;
+
+	/* Must be MB aligned */
+	BUILD_BUG_ON(CONFIG_ARMV7_SECURE_BASE & 0xFFFFF);
+	BUILD_BUG_ON(CONFIG_ARMV7_SECURE_RESERVE_SIZE & 0xFFFFF);
+
+	writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0);
+	writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1);
+}
+#endif
+
 void s_init(void)
 {
 	/* Init PMC scratch memory */
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 89f2294..21be69d 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -76,6 +76,7 @@ void armv8_switch_to_el1(void);
 void gic_init(void);
 void gic_send_sgi(unsigned long sgino);
 void wait_for_wakeup(void);
+void protect_secure_region(void);
 void smp_kick_all_cpus(void);
 
 void flush_l3_cache(void);
-- 
2.1.4

[U-Boot] [PATCH v2 11/12] tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

[Mark Rutland]

On Mon, Feb 16, 2015 at 12:54:48PM +0000, Jan Kiszka wrote:
> From: Ian Campbell <ijc@hellion.org.uk>
> 
> These registers can be used to prevent non-secure world from accessing a
> megabyte aligned region of RAM, use them to protect the u-boot secure monitor
> code.

What happens if the CPU tried to read this memory from the non-secure
world? If the OS has it mapped then the CPU could perform speculative
reads at any point in time.

If that can raise an abort then the OS needs to not map the region.

I take it U-Boot uses a secure mapping for the region (which I believe
should avoid the mismatched attributes issue I mentioned in my other
reply).

Thanks,
Mark.

> At first I tried to do this from s_init(), however this inexplicably causes
> u-boot's networking (e.g. DHCP) to fail, while networking under Linux was fine.
> 
> So instead I have added a new weak arch function protect_secure_section()
> called from relocate_secure_section() and reserved the region there. This is
> better overall since it defers the reservation until after the sec vs. non-sec
> decision (which can be influenced by an envvar) has been made when booting the
> os.
> 
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm/cpu/armv7/virt-v7.c   |  5 +++++
>  arch/arm/cpu/tegra-common/ap.c | 15 +++++++++++++++
>  arch/arm/include/asm/system.h  |  1 +
>  3 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
> index b69fd37..eb6195c 100644
> --- a/arch/arm/cpu/armv7/virt-v7.c
> +++ b/arch/arm/cpu/armv7/virt-v7.c
> @@ -46,6 +46,10 @@ static unsigned long get_gicd_base_address(void)
>  #endif
>  }
>  
> +/* Define a specific version of this function to enable any available
> + * hardware protections for the reserved region */
> +void __weak protect_secure_section(void) {}
> +
>  static void relocate_secure_section(void)
>  {
>  #ifdef CONFIG_ARMV7_SECURE_BASE
> @@ -54,6 +58,7 @@ static void relocate_secure_section(void)
>  	memcpy((void *)CONFIG_ARMV7_SECURE_BASE, __secure_start, sz);
>  	flush_dcache_range(CONFIG_ARMV7_SECURE_BASE,
>  			   CONFIG_ARMV7_SECURE_BASE + sz + 1);
> +	protect_secure_section();
>  	invalidate_icache_all();
>  #endif
>  }
> diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c
> index a17dfd1..f1d3070 100644
> --- a/arch/arm/cpu/tegra-common/ap.c
> +++ b/arch/arm/cpu/tegra-common/ap.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <asm/io.h>
>  #include <asm/arch/gp_padctrl.h>
> +#include <asm/arch/mc.h>
>  #include <asm/arch-tegra/ap.h>
>  #include <asm/arch-tegra/clock.h>
>  #include <asm/arch-tegra/fuse.h>
> @@ -154,6 +155,20 @@ static void init_pmc_scratch(void)
>  	writel(odmdata, &pmc->pmc_scratch20);
>  }
>  
> +#ifdef CONFIG_ARMV7_SECURE_RESERVE_SIZE
> +void protect_secure_section(void)
> +{
> +	struct mc_ctlr *mc = (struct mc_ctlr *)NV_PA_MC_BASE;
> +
> +	/* Must be MB aligned */
> +	BUILD_BUG_ON(CONFIG_ARMV7_SECURE_BASE & 0xFFFFF);
> +	BUILD_BUG_ON(CONFIG_ARMV7_SECURE_RESERVE_SIZE & 0xFFFFF);
> +
> +	writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0);
> +	writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1);
> +}
> +#endif
> +
>  void s_init(void)
>  {
>  	/* Init PMC scratch memory */
> diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
> index 89f2294..21be69d 100644
> --- a/arch/arm/include/asm/system.h
> +++ b/arch/arm/include/asm/system.h
> @@ -76,6 +76,7 @@ void armv8_switch_to_el1(void);
>  void gic_init(void);
>  void gic_send_sgi(unsigned long sgino);
>  void wait_for_wakeup(void);
> +void protect_secure_region(void);
>  void smp_kick_all_cpus(void);
>  
>  void flush_l3_cache(void);
> -- 
> 2.1.4
> 
> 

[U-Boot] [PATCH v2 11/12] tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

[Jan Kiszka]

On 2015-02-16 14:49, Mark Rutland wrote:
> On Mon, Feb 16, 2015 at 12:54:48PM +0000, Jan Kiszka wrote:
>> From: Ian Campbell <ijc@hellion.org.uk>
>>
>> These registers can be used to prevent non-secure world from accessing a
>> megabyte aligned region of RAM, use them to protect the u-boot secure monitor
>> code.
> 
> What happens if the CPU tried to read this memory from the non-secure
> world? If the OS has it mapped then the CPU could perform speculative
> reads at any point in time.
> 
> If that can raise an abort then the OS needs to not map the region.
> 
> I take it U-Boot uses a secure mapping for the region (which I believe
> should avoid the mismatched attributes issue I mentioned in my other
> reply).

What I can contribute to this are kernel messages due to a
misconfiguration of our hypervisor Jailhouse (while Linux was still
trying to boot it):

[   61.896860] tegra-mc 70019000.memory-controller: mpcorew: write @0x00000000fff00040: Security violation (TrustZone violation)
[   61.896888] tegra-mc 70019000.memory-controller: mpcorew: write @0x00000000fff2d340: Security violation (TrustZone violation)

So it seems that Linux is receiving a violation report here when trying
to access the memory.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 11/12] tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

[Stephen Warren]

On 02/16/2015 05:54 AM, Jan Kiszka wrote:
> From: Ian Campbell <ijc@hellion.org.uk>
>
> These registers can be used to prevent non-secure world from accessing a
> megabyte aligned region of RAM, use them to protect the u-boot secure monitor
> code.
>
> At first I tried to do this from s_init(), however this inexplicably causes
> u-boot's networking (e.g. DHCP) to fail, while networking under Linux was fine.
>
> So instead I have added a new weak arch function protect_secure_section()
> called from relocate_secure_section() and reserved the region there. This is
> better overall since it defers the reservation until after the sec vs. non-sec
> decision (which can be influenced by an envvar) has been made when booting the
> os.

> diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c

> +void protect_secure_section(void)

> +	writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0);
> +	writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1);

Spaces around the >> ?

[U-Boot] [PATCH v2 11/12] tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

[Jan Kiszka]

On 2015-02-17 22:06, Stephen Warren wrote:
> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>> From: Ian Campbell <ijc@hellion.org.uk>
>>
>> These registers can be used to prevent non-secure world from accessing a
>> megabyte aligned region of RAM, use them to protect the u-boot secure
>> monitor
>> code.
>>
>> At first I tried to do this from s_init(), however this inexplicably
>> causes
>> u-boot's networking (e.g. DHCP) to fail, while networking under Linux
>> was fine.
>>
>> So instead I have added a new weak arch function protect_secure_section()
>> called from relocate_secure_section() and reserved the region there.
>> This is
>> better overall since it defers the reservation until after the sec vs.
>> non-sec
>> decision (which can be influenced by an envvar) has been made when
>> booting the
>> os.
> 
>> diff --git a/arch/arm/cpu/tegra-common/ap.c
>> b/arch/arm/cpu/tegra-common/ap.c
> 
>> +void protect_secure_section(void)
> 
>> +    writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0);
>> +    writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1);
> 
> Spaces around the >> ?

Fixed for v3.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Jan Kiszka]

We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
happen for all cores.

Fixing this resolves problems of KVM with emulating the generic
timer/counter.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
index b7501fb..119c246 100644
--- a/arch/arm/cpu/armv7/tegra-common/psci.S
+++ b/arch/arm/cpu/armv7/tegra-common/psci.S
@@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
 
 	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
 	and	r4, r4, #7		@ number of CPUs in cluster
+
+	adr	r5, _sys_clock_freq
+	cmp	r4, #0
+
+	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
+	streq	r7, [r5]
+
+	ldrne	r7, [r5]
+	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3
+
 	bl	psci_get_cpu_stack_top
 	mov	sp, r5
 
 	bx	r6
 ENDPROC(psci_arch_init)
 
+_sys_clock_freq:
+	.word
+
 ENTRY(psci_cpu_off)
 	bl psci_cpu_off_common
 
-- 
2.1.4

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Mark Rutland]

On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote:
> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
> happen for all cores.
> 
> Fixing this resolves problems of KVM with emulating the generic
> timer/counter.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
> index b7501fb..119c246 100644
> --- a/arch/arm/cpu/armv7/tegra-common/psci.S
> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
>  
>  	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
>  	and	r4, r4, #7		@ number of CPUs in cluster
> +
> +	adr	r5, _sys_clock_freq
> +	cmp	r4, #0
> +
> +	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
> +	streq	r7, [r5]
> +
> +	ldrne	r7, [r5]
> +	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3

Is it not possible to have a hook that uses the same variable as
arch_timer_init rather than doing a here copy? It seems a shame to
duplicate the effort.

Thanks,
Mark.

> +
>  	bl	psci_get_cpu_stack_top
>  	mov	sp, r5
>  
>  	bx	r6
>  ENDPROC(psci_arch_init)
>  
> +_sys_clock_freq:
> +	.word
> +
>  ENTRY(psci_cpu_off)
>  	bl psci_cpu_off_common
>  
> -- 
> 2.1.4
> 
> 

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Jan Kiszka]

On 2015-02-16 14:37, Mark Rutland wrote:
> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote:
>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
>> happen for all cores.
>>
>> Fixing this resolves problems of KVM with emulating the generic
>> timer/counter.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
>> index b7501fb..119c246 100644
>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S
>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
>>  
>>  	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
>>  	and	r4, r4, #7		@ number of CPUs in cluster
>> +
>> +	adr	r5, _sys_clock_freq
>> +	cmp	r4, #0
>> +
>> +	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
>> +	streq	r7, [r5]
>> +
>> +	ldrne	r7, [r5]
>> +	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3
> 
> Is it not possible to have a hook that uses the same variable as
> arch_timer_init rather than doing a here copy? It seems a shame to
> duplicate the effort.

The problem is related to the different address spaces. Here we run in
the secure monitor, arch_timer_init - to my understanding - in
non-secure mode. Didn't find a pattern so far how to transfer data (and
that shouldn't be more complex than the above code).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Mark Rutland]

On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote:
> On 2015-02-16 14:37, Mark Rutland wrote:
> > On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote:
> >> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
> >> happen for all cores.
> >>
> >> Fixing this resolves problems of KVM with emulating the generic
> >> timer/counter.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
> >> index b7501fb..119c246 100644
> >> --- a/arch/arm/cpu/armv7/tegra-common/psci.S
> >> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
> >> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
> >>  
> >>  	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
> >>  	and	r4, r4, #7		@ number of CPUs in cluster
> >> +
> >> +	adr	r5, _sys_clock_freq
> >> +	cmp	r4, #0
> >> +
> >> +	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
> >> +	streq	r7, [r5]
> >> +
> >> +	ldrne	r7, [r5]
> >> +	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3
> > 
> > Is it not possible to have a hook that uses the same variable as
> > arch_timer_init rather than doing a here copy? It seems a shame to
> > duplicate the effort.
> 
> The problem is related to the different address spaces. Here we run in
> the secure monitor, arch_timer_init - to my understanding - in
> non-secure mode. Didn't find a pattern so far how to transfer data (and
> that shouldn't be more complex than the above code).

Surely arch_timer_init must be run in a secure mode in order to be
allowed to write to CNTFRQ?

If this is simply the easiest way of moving the data around then there's
no real problem with it; it's just a shame that it only happens in the
PSCI case.

Thanks,
Mark.

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Jan Kiszka]

On 2015-02-16 14:51, Mark Rutland wrote:
> On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote:
>> On 2015-02-16 14:37, Mark Rutland wrote:
>>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote:
>>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
>>>> happen for all cores.
>>>>
>>>> Fixing this resolves problems of KVM with emulating the generic
>>>> timer/counter.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
>>>> index b7501fb..119c246 100644
>>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S
>>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
>>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
>>>>  
>>>>  	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
>>>>  	and	r4, r4, #7		@ number of CPUs in cluster
>>>> +
>>>> +	adr	r5, _sys_clock_freq
>>>> +	cmp	r4, #0
>>>> +
>>>> +	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
>>>> +	streq	r7, [r5]
>>>> +
>>>> +	ldrne	r7, [r5]
>>>> +	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3
>>>
>>> Is it not possible to have a hook that uses the same variable as
>>> arch_timer_init rather than doing a here copy? It seems a shame to
>>> duplicate the effort.
>>
>> The problem is related to the different address spaces. Here we run in
>> the secure monitor, arch_timer_init - to my understanding - in
>> non-secure mode. Didn't find a pattern so far how to transfer data (and
>> that shouldn't be more complex than the above code).
> 
> Surely arch_timer_init must be run in a secure mode in order to be
> allowed to write to CNTFRQ?

Ah, right.

> 
> If this is simply the easiest way of moving the data around then there's
> no real problem with it; it's just a shame that it only happens in the
> PSCI case.

OK, I'll check again. Maybe it's easier than I thought.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Jan Kiszka]

On 2015-02-16 15:02, Jan Kiszka wrote:
> On 2015-02-16 14:51, Mark Rutland wrote:
>> On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote:
>>> On 2015-02-16 14:37, Mark Rutland wrote:
>>>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote:
>>>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
>>>>> happen for all cores.
>>>>>
>>>>> Fixing this resolves problems of KVM with emulating the generic
>>>>> timer/counter.
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>  arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
>>>>>  1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
>>>>> index b7501fb..119c246 100644
>>>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S
>>>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
>>>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
>>>>>  
>>>>>  	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
>>>>>  	and	r4, r4, #7		@ number of CPUs in cluster
>>>>> +
>>>>> +	adr	r5, _sys_clock_freq
>>>>> +	cmp	r4, #0
>>>>> +
>>>>> +	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
>>>>> +	streq	r7, [r5]
>>>>> +
>>>>> +	ldrne	r7, [r5]
>>>>> +	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3
>>>>
>>>> Is it not possible to have a hook that uses the same variable as
>>>> arch_timer_init rather than doing a here copy? It seems a shame to
>>>> duplicate the effort.
>>>
>>> The problem is related to the different address spaces. Here we run in
>>> the secure monitor, arch_timer_init - to my understanding - in
>>> non-secure mode. Didn't find a pattern so far how to transfer data (and
>>> that shouldn't be more complex than the above code).
>>
>> Surely arch_timer_init must be run in a secure mode in order to be
>> allowed to write to CNTFRQ?
> 
> Ah, right.
> 
>>
>> If this is simply the easiest way of moving the data around then there's
>> no real problem with it; it's just a shame that it only happens in the
>> PSCI case.
> 
> OK, I'll check again. Maybe it's easier than I thought.

It isn't: the variable we would have to write - conditionally, i.e. not
for the SPL build - is not yet where it will finally be. The monitor is
copied later on, but the symbol points to the destination already. One
can account for that, but the result won't get simpler and cleaner IMHO.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Mark Rutland]

On Tue, Feb 17, 2015 at 07:01:57AM +0000, Jan Kiszka wrote:
> On 2015-02-16 15:02, Jan Kiszka wrote:
> > On 2015-02-16 14:51, Mark Rutland wrote:
> >> On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote:
> >>> On 2015-02-16 14:37, Mark Rutland wrote:
> >>>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote:
> >>>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
> >>>>> happen for all cores.
> >>>>>
> >>>>> Fixing this resolves problems of KVM with emulating the generic
> >>>>> timer/counter.
> >>>>>
> >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>>>> ---
> >>>>>  arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
> >>>>>  1 file changed, 13 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
> >>>>> index b7501fb..119c246 100644
> >>>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S
> >>>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
> >>>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
> >>>>>  
> >>>>>  	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
> >>>>>  	and	r4, r4, #7		@ number of CPUs in cluster
> >>>>> +
> >>>>> +	adr	r5, _sys_clock_freq
> >>>>> +	cmp	r4, #0
> >>>>> +
> >>>>> +	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
> >>>>> +	streq	r7, [r5]
> >>>>> +
> >>>>> +	ldrne	r7, [r5]
> >>>>> +	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3
> >>>>
> >>>> Is it not possible to have a hook that uses the same variable as
> >>>> arch_timer_init rather than doing a here copy? It seems a shame to
> >>>> duplicate the effort.
> >>>
> >>> The problem is related to the different address spaces. Here we run in
> >>> the secure monitor, arch_timer_init - to my understanding - in
> >>> non-secure mode. Didn't find a pattern so far how to transfer data (and
> >>> that shouldn't be more complex than the above code).
> >>
> >> Surely arch_timer_init must be run in a secure mode in order to be
> >> allowed to write to CNTFRQ?
> > 
> > Ah, right.
> > 
> >>
> >> If this is simply the easiest way of moving the data around then there's
> >> no real problem with it; it's just a shame that it only happens in the
> >> PSCI case.
> > 
> > OK, I'll check again. Maybe it's easier than I thought.
> 
> It isn't: the variable we would have to write - conditionally, i.e. not
> for the SPL build - is not yet where it will finally be. The monitor is
> copied later on, but the symbol points to the destination already. One
> can account for that, but the result won't get simpler and cleaner IMHO.

Fair enough. As I mentioned earlier there's no real problem with the
above, it just seemed a shame that this was only done in the PSCI case.

I take it for the quoted sequence above that the primary CPU runs
through this path before sending the secondaries through (and that
either there's a DSB somewhere between the write and waking up
secondaries or memory accesses are strongly ordered at this point).

Thanks,
Mark.

[U-Boot] [PATCH v2 12/12] tegra: Set CNTFRQ for secondary CPUs

[Jan Kiszka]

On 2015-02-17 11:21, Mark Rutland wrote:
> On Tue, Feb 17, 2015 at 07:01:57AM +0000, Jan Kiszka wrote:
>> On 2015-02-16 15:02, Jan Kiszka wrote:
>>> On 2015-02-16 14:51, Mark Rutland wrote:
>>>> On Mon, Feb 16, 2015 at 01:44:36PM +0000, Jan Kiszka wrote:
>>>>> On 2015-02-16 14:37, Mark Rutland wrote:
>>>>>> On Mon, Feb 16, 2015 at 12:54:49PM +0000, Jan Kiszka wrote:
>>>>>>> We only set CNTFRQ in arch_timer_init for the boot CPU. But this has to
>>>>>>> happen for all cores.
>>>>>>>
>>>>>>> Fixing this resolves problems of KVM with emulating the generic
>>>>>>> timer/counter.
>>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>> ---
>>>>>>>  arch/arm/cpu/armv7/tegra-common/psci.S | 13 +++++++++++++
>>>>>>>  1 file changed, 13 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/cpu/armv7/tegra-common/psci.S b/arch/arm/cpu/armv7/tegra-common/psci.S
>>>>>>> index b7501fb..119c246 100644
>>>>>>> --- a/arch/arm/cpu/armv7/tegra-common/psci.S
>>>>>>> +++ b/arch/arm/cpu/armv7/tegra-common/psci.S
>>>>>>> @@ -51,12 +51,25 @@ ENTRY(psci_arch_init)
>>>>>>>  
>>>>>>>  	mrc	p15, 0, r4, c0, c0, 5	@ MPIDR
>>>>>>>  	and	r4, r4, #7		@ number of CPUs in cluster
>>>>>>> +
>>>>>>> +	adr	r5, _sys_clock_freq
>>>>>>> +	cmp	r4, #0
>>>>>>> +
>>>>>>> +	mrceq   p15, 0, r7, c14, c0, 0	@ read CNTFRQ from CPU0
>>>>>>> +	streq	r7, [r5]
>>>>>>> +
>>>>>>> +	ldrne	r7, [r5]
>>>>>>> +	mcrne   p15, 0, r7, c14, c0, 0	@ write CNTFRQ to CPU1..3
>>>>>>
>>>>>> Is it not possible to have a hook that uses the same variable as
>>>>>> arch_timer_init rather than doing a here copy? It seems a shame to
>>>>>> duplicate the effort.
>>>>>
>>>>> The problem is related to the different address spaces. Here we run in
>>>>> the secure monitor, arch_timer_init - to my understanding - in
>>>>> non-secure mode. Didn't find a pattern so far how to transfer data (and
>>>>> that shouldn't be more complex than the above code).
>>>>
>>>> Surely arch_timer_init must be run in a secure mode in order to be
>>>> allowed to write to CNTFRQ?
>>>
>>> Ah, right.
>>>
>>>>
>>>> If this is simply the easiest way of moving the data around then there's
>>>> no real problem with it; it's just a shame that it only happens in the
>>>> PSCI case.
>>>
>>> OK, I'll check again. Maybe it's easier than I thought.
>>
>> It isn't: the variable we would have to write - conditionally, i.e. not
>> for the SPL build - is not yet where it will finally be. The monitor is
>> copied later on, but the symbol points to the destination already. One
>> can account for that, but the result won't get simpler and cleaner IMHO.
> 
> Fair enough. As I mentioned earlier there's no real problem with the
> above, it just seemed a shame that this was only done in the PSCI case.
> 
> I take it for the quoted sequence above that the primary CPU runs
> through this path before sending the secondaries through (and that
> either there's a DSB somewhere between the write and waking up
> secondaries or memory accesses are strongly ordered at this point).

Yes to both: The primary performs this init before starting the target
OS, and the dsb is at latest in psci_cpu_on before kicking off a
secondary CPU.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux