linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation
@ 2015-01-13  2:15 Zi Shen Lim
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1 Zi Shen Lim
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Zi Shen Lim @ 2015-01-13  2:15 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Mark,

The first couple patches are cleanups.

The third patch switches to using v0.2 function ID values for the
existing cpu_{on,off} implementations. This intermediate step is
arguably unnecessary given the next patch...

The fourth patch provides a simple implementation of PSCI v0.2.
The deviations from spec (primarily cpu_suspend not supported)
is noted in code comment as well as in commit log.

Please consider merging.

Thanks,
z

Zi Shen Lim (4):
  psci: use MPIDR_INVALID instead of -1
  psci: remove sentinel from id_table
  psci: use PSCI v0.2 function IDs
  psci: implement PSCI v0.2

 Makefile.am |   4 +-
 psci.S      | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 143 insertions(+), 31 deletions(-)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1
  2015-01-13  2:15 [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Zi Shen Lim
@ 2015-01-13  2:15 ` Zi Shen Lim
  2015-01-14 11:03   ` Mark Rutland
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 2/4] psci: remove sentinel from id_table Zi Shen Lim
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Zi Shen Lim @ 2015-01-13  2:15 UTC (permalink / raw)
  To: linux-arm-kernel

MPIDR_INVALID was already defined, so use it.

Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
 psci.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/psci.S b/psci.S
index 856095b..234493d 100644
--- a/psci.S
+++ b/psci.S
@@ -126,7 +126,7 @@ psci_cpu_on:
 	mov	x0, x1
 
 	bl	find_logical_id
-	cmp	x0, #-1
+	cmp	x0, #MPIDR_INVALID
 	b.eq	1f
 
 	adr	x3, branch_table
@@ -172,7 +172,7 @@ __find_logical_index:
 	b 1b
 2:	mov	x0, x1
 	ret
-3:	mov	x0, #-1
+3:	mov	x0, #MPIDR_INVALID
 	ret
 
 setup_vector:
@@ -207,7 +207,7 @@ spin:
 	ldr	x1, =MPIDR_ID_BITS
 	and	x0, x0, x1
 	bl	find_logical_id
-	cmp	x0, #-1
+	cmp	x0, #MPIDR_INVALID
 	b.eq	spin_dead
 
 	adr	x1, branch_table
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 2/4] psci: remove sentinel from id_table
  2015-01-13  2:15 [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Zi Shen Lim
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1 Zi Shen Lim
@ 2015-01-13  2:15 ` Zi Shen Lim
  2015-01-14 11:10   ` Mark Rutland
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 3/4] psci: use PSCI v0.2 function IDs Zi Shen Lim
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Zi Shen Lim @ 2015-01-13  2:15 UTC (permalink / raw)
  To: linux-arm-kernel

Tweak the limit check in find_logical_id so we can
do away with the sentinel from id_table.

While at it, also remove unused label and fix up spacing.

Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
 psci.S | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/psci.S b/psci.S
index 234493d..d045e56 100644
--- a/psci.S
+++ b/psci.S
@@ -69,7 +69,6 @@ vector:
 id_table:
 	.quad CPU_IDS
 __id_end:
-	.quad MPIDR_INVALID
 
 .equ	nr_cpus, ((__id_end - id_table) / 8)
 
@@ -159,17 +158,16 @@ psci_cpu_on:
  * Clobbers x1, x2, x3
  */
 find_logical_id:
-__find_logical_index:
 	adr	x2, id_table
 	mov	x1, xzr
 1:	mov	x3, #nr_cpus	// check we haven't walked off the end of the array
 	cmp	x1, x3
-	b.gt	3f
+	b.ge	3f
 	ldr	x3, [x2, x1, lsl #3]
 	cmp	x3, x0
 	b.eq	2f
 	add	x1, x1, #1
-	b 1b
+	b	1b
 2:	mov	x0, x1
 	ret
 3:	mov	x0, #MPIDR_INVALID
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 3/4] psci: use PSCI v0.2 function IDs
  2015-01-13  2:15 [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Zi Shen Lim
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1 Zi Shen Lim
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 2/4] psci: remove sentinel from id_table Zi Shen Lim
@ 2015-01-13  2:15 ` Zi Shen Lim
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 4/4] psci: implement PSCI v0.2 Zi Shen Lim
  2015-01-14 11:53 ` [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Mark Rutland
  4 siblings, 0 replies; 10+ messages in thread
From: Zi Shen Lim @ 2015-01-13  2:15 UTC (permalink / raw)
  To: linux-arm-kernel

Update PSCI function IDs to match those defined in
v0.2 of the specification.

Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
 Makefile.am | 4 ++--
 psci.S      | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 0c174e6..4f5bfdd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -25,8 +25,8 @@ BOOTMETHOD	:= psci.o
 PSCI_NODE	:= psci {				\
 			compatible = \"arm,psci\";	\
 			method = \"smc\";		\
-			cpu_on = <0x84000002>;		\
-			cpu_off = <0x84000001>;		\
+			cpu_on = <0xc4000003>;		\
+			cpu_off = <0x84000002>;		\
 		   };
 CPU_NODES	:= $(shell $(top_srcdir)/gen-cpu-nodes.sh $(CPU_IDS))
 CPUS_NODE	:= cpus {		\
diff --git a/psci.S b/psci.S
index d045e56..c51e125 100644
--- a/psci.S
+++ b/psci.S
@@ -8,8 +8,8 @@
  */
 #include "common.S"
 
-#define PSCI_CPU_OFF		0x84000001
-#define PSCI_CPU_ON		0x84000002
+#define PSCI_CPU_OFF		0x84000002
+#define PSCI_CPU_ON		0xc4000003
 
 #define PSCI_RET_SUCCESS	0
 #define PSCI_RET_NOT_IMPL	(-1)
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 4/4] psci: implement PSCI v0.2
  2015-01-13  2:15 [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Zi Shen Lim
                   ` (2 preceding siblings ...)
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 3/4] psci: use PSCI v0.2 function IDs Zi Shen Lim
@ 2015-01-13  2:15 ` Zi Shen Lim
  2015-01-14 11:53 ` [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Mark Rutland
  4 siblings, 0 replies; 10+ messages in thread
From: Zi Shen Lim @ 2015-01-13  2:15 UTC (permalink / raw)
  To: linux-arm-kernel

In this simple implementation:
* cpu_suspend is not supported, and as such is not fully
  compliant with the spec.
* The system_{off,reset} functions currently just spin on
  one CPU, when it should instead power {down,cycle}
  the system.

Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
 Makefile.am |   4 +-
 psci.S      | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 138 insertions(+), 24 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 4f5bfdd..471462a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -23,10 +23,8 @@ BOOTLOADER	:= boot.S
 if PSCI
 BOOTMETHOD	:= psci.o
 PSCI_NODE	:= psci {				\
-			compatible = \"arm,psci\";	\
+			compatible = \"arm,psci-0.2\";	\
 			method = \"smc\";		\
-			cpu_on = <0xc4000003>;		\
-			cpu_off = <0x84000002>;		\
 		   };
 CPU_NODES	:= $(shell $(top_srcdir)/gen-cpu-nodes.sh $(CPU_IDS))
 CPUS_NODE	:= cpus {		\
diff --git a/psci.S b/psci.S
index c51e125..b8afb0e 100644
--- a/psci.S
+++ b/psci.S
@@ -8,13 +8,26 @@
  */
 #include "common.S"
 
-#define PSCI_CPU_OFF		0x84000002
-#define PSCI_CPU_ON		0xc4000003
-
-#define PSCI_RET_SUCCESS	0
-#define PSCI_RET_NOT_IMPL	(-1)
-#define PSCI_RET_INVALID	(-2)
-#define PSCI_RET_DENIED		(-3)
+#define PSCI_VERSION			0x84000000
+#define PSCI_CPU_SUSPEND		0xc4000001
+#define PSCI_CPU_OFF			0x84000002
+#define PSCI_CPU_ON			0xc4000003
+#define PSCI_AFFINITY_INFO		0xc4000004
+#define PSCI_MIGRATE			0xc4000005
+#define PSCI_MIGRATE_INFO_TYPE		0x84000006
+#define PSCI_MIGRATE_INFO_UP_CPU	0xc4000007
+#define PSCI_SYSTEM_OFF			0x84000008
+#define PSCI_SYSTEM_RESET		0x84000009
+
+#define PSCI_RET_SUCCESS		0
+#define PSCI_RET_NOT_SUPPORTED		(-1)
+#define PSCI_RET_INVALID_PARAMETERS	(-2)
+#define PSCI_RET_DENIED			(-3)
+#define PSCI_RET_ALREADY_ON		(-4)
+#define PSCI_RET_ON_PENDING		(-5)
+#define PSCI_RET_INTERNAL_FAILURE	(-6)
+#define PSCI_RET_NOT_PRESENT		(-7)
+#define PSCI_RET_DISABLED		(-8)
 
 #ifndef CPU_IDS
 #error No CPU MPIDRs provided.
@@ -77,6 +90,16 @@ branch_table:
 	.quad ADDR_INVALID
 	.endr
 
+#define AFFINFO_ON		0
+#define AFFINFO_OFF		1
+#define AFFINFO_ON_PENDING	2
+#define AFFINFO_DISABLED	PSCI_RET_DISABLED
+
+affinfo_table:
+	.rept (nr_cpus)
+	.quad AFFINFO_DISABLED
+	.endr
+
 	.text
 
 	.globl start_no_el3
@@ -86,10 +109,18 @@ err_exception:
 	b err_exception
 
 psci_call32:
-	mov	w0, PSCI_RET_NOT_IMPL
+	mov	w0, PSCI_RET_NOT_SUPPORTED
 	eret
 
 psci_call64:
+	ldr	x7, =PSCI_VERSION
+	cmp	x0, x7
+	b.eq	psci_version
+
+	ldr	x7, =PSCI_CPU_SUSPEND
+	cmp	x0, x7
+	b.eq	psci_not_supported	// XXX: not spec compliant
+
 	ldr	x7, =PSCI_CPU_OFF
 	cmp	x0, x7
 	b.eq	psci_cpu_off
@@ -98,7 +129,36 @@ psci_call64:
 	cmp	x0, x7
 	b.eq	psci_cpu_on
 
-	mov	x0, PSCI_RET_NOT_IMPL
+	ldr	x7, =PSCI_AFFINITY_INFO
+	cmp	x0, x7
+	b.eq	psci_affinity_info
+
+	ldr	x7, =PSCI_MIGRATE
+	cmp	x0, x7
+	b.eq	psci_not_supported
+
+	ldr	x7, =PSCI_MIGRATE_INFO_TYPE
+	cmp	x0, x7
+	b.eq	psci_migrate_info_type
+
+	ldr	x7, =PSCI_MIGRATE_INFO_UP_CPU
+	cmp	x0, x7
+	b.eq	psci_not_supported
+
+	ldr	x7, =PSCI_SYSTEM_OFF
+	cmp	x0, x7
+	b.eq	psci_system_off
+
+	ldr	x7, =PSCI_SYSTEM_RESET
+	cmp	x0, x7
+	b.eq	psci_system_reset
+
+psci_not_supported:
+	mov	x0, PSCI_RET_NOT_SUPPORTED
+	eret
+
+psci_version:
+	mov	x0, #((0 << 16) | (2 << 0))
 	eret
 
 /*
@@ -109,6 +169,11 @@ psci_cpu_off:
 	ldr	x1, =MPIDR_ID_BITS
 	and	x0, x0, x1
 	bl	find_logical_id
+
+	adr	x1, affinfo_table
+	mov	x2, #AFFINFO_OFF
+	str	x2, [x1, x0, lsl #3]
+
 	adr	x1, branch_table
 	mov	x2, #ADDR_INVALID
 	str	x2, [x1, x0, lsl #3]
@@ -118,39 +183,85 @@ psci_cpu_off:
 /*
  * x1 - target cpu
  * x2 - address
+ * x3 - context id (Note: currently ignored)
  */
 psci_cpu_on:
-	mov	x15, x30
 	mov	x14, x2
-	mov	x0, x1
 
+	mov	x0, x1
+	mov	x15, x30
 	bl	find_logical_id
+	mov	x30, x15
 	cmp	x0, #MPIDR_INVALID
 	b.eq	1f
 
+	adr	x1, affinfo_table
+	ldr	x2, [x1, x0, lsl #3]
+	cmp	x2, #AFFINFO_ON
+	b.eq	2f
+	cmp	x2, #AFFINFO_ON_PENDING
+	b.eq	3f
+	cmp	x2, #AFFINFO_DISABLED
+	b.eq	1f
+
+	mov	x2, #AFFINFO_ON_PENDING
+	str	x2, [x1, x0, lsl #3]
+
 	adr	x3, branch_table
 	add	x3, x3, x0, lsl #3
 
-	ldr	x4, =ADDR_INVALID
-
 	ldxr	x5, [x3]
-	cmp	x4, x5
-	b.ne	1f
+	cmp	x5, #ADDR_INVALID
+	b.ne	2f
 
 	stxr	w4, x14, [x3]
-	cbnz	w4, 1f
+	cbnz	w4, 2f
 
 	dsb	ishst
 	sev
 
 	mov	x0, #PSCI_RET_SUCCESS
-	mov	x30, x15
 	eret
 
-1:	mov	x0, #PSCI_RET_DENIED
+1:	mov	x0, #PSCI_RET_INVALID_PARAMETERS
+	eret
+
+2:	mov	x0, #PSCI_RET_ALREADY_ON
+	eret
+
+3:	mov	x0, #PSCI_RET_ON_PENDING
+	eret
+
+/*
+ * x1 - target affinity (same as target cpu)
+ * x2 - lowest affinity level (Note: only 0 is supported)
+ */
+psci_affinity_info:
+	cbnz	x2, 1f
+
+	mov	x0, x1
+	mov	x15, x30
+	bl	find_logical_id
 	mov	x30, x15
+	cmp	x0, #MPIDR_INVALID
+	b.eq	1f
+
+	adr	x3, affinfo_table
+	ldr	x0, [x3, x0, lsl #3]
 	eret
 
+1:	mov	x0, #PSCI_RET_INVALID_PARAMETERS
+	eret
+
+psci_migrate_info_type:
+	mov	x0, #2	// Trusted OS not present, doesn't require migration.
+	eret
+
+psci_system_off:
+	b	spin_dead	// XXX: need to power down system
+
+psci_system_reset:
+	b	spin_dead	// XXX: need to power cycle system
 
 /*
  * Takes masked MPIDR in x0, returns logical id in x0
@@ -208,16 +319,21 @@ spin:
 	cmp	x0, #MPIDR_INVALID
 	b.eq	spin_dead
 
-	adr	x1, branch_table
-	mov	x3, #ADDR_INVALID
+	adr	x15, affinfo_table
+	mov	x14, #AFFINFO_OFF
+	str	x14, [x15, x0, lsl #3]
 
+	adr	x1, branch_table
 	add	x1, x1, x0, lsl #3
 
 1:	wfe
 	ldr	x2, [x1]
-	cmp	x2, x3
+	cmp	x2, #ADDR_INVALID
 	b.eq	1b
 
+	mov	x14, #AFFINFO_ON
+	str	x14, [x15, x0, lsl #3]
+
 	ldr	x0, =SCTLR_EL2_RESET
 	msr	sctlr_el2, x0
 
-- 
2.1.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1 Zi Shen Lim
@ 2015-01-14 11:03   ` Mark Rutland
  2015-01-14 18:24     ` Z Lim
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2015-01-14 11:03 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On Tue, Jan 13, 2015 at 02:15:01AM +0000, Zi Shen Lim wrote:
> MPIDR_INVALID was already defined, so use it.

MPIDR_INVALID is meant to be an invalid MPIDR value, rather than an
error code in the case of an invalid MPIDR.

__find_logical_index is meant to return a logical CPU ID, so returning
MPIDR_INVALID isn't quite right.

I'd be happier if we used a separate macro for the logical IDs (e.g.
ID_INVALID).

Thanks,
Mark.

> 
> Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
> ---
>  psci.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/psci.S b/psci.S
> index 856095b..234493d 100644
> --- a/psci.S
> +++ b/psci.S
> @@ -126,7 +126,7 @@ psci_cpu_on:
>  	mov	x0, x1
>  
>  	bl	find_logical_id
> -	cmp	x0, #-1
> +	cmp	x0, #MPIDR_INVALID
>  	b.eq	1f
>  
>  	adr	x3, branch_table
> @@ -172,7 +172,7 @@ __find_logical_index:
>  	b 1b
>  2:	mov	x0, x1
>  	ret
> -3:	mov	x0, #-1
> +3:	mov	x0, #MPIDR_INVALID
>  	ret
>  
>  setup_vector:
> @@ -207,7 +207,7 @@ spin:
>  	ldr	x1, =MPIDR_ID_BITS
>  	and	x0, x0, x1
>  	bl	find_logical_id
> -	cmp	x0, #-1
> +	cmp	x0, #MPIDR_INVALID
>  	b.eq	spin_dead
>  
>  	adr	x1, branch_table
> -- 
> 2.1.0
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 2/4] psci: remove sentinel from id_table
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 2/4] psci: remove sentinel from id_table Zi Shen Lim
@ 2015-01-14 11:10   ` Mark Rutland
  0 siblings, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2015-01-14 11:10 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

On Tue, Jan 13, 2015 at 02:15:02AM +0000, Zi Shen Lim wrote:
> Tweak the limit check in find_logical_id so we can
> do away with the sentinel from id_table.
> 
> While at it, also remove unused label and fix up spacing.
> 
> Signed-off-by: Zi Shen Lim <zlim@broadcom.com>

This looks like a nice cleanup to me.

So long as this passes build and boot testing I'll apply this shortly.

Thanks,
Mark.

> ---
>  psci.S | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/psci.S b/psci.S
> index 234493d..d045e56 100644
> --- a/psci.S
> +++ b/psci.S
> @@ -69,7 +69,6 @@ vector:
>  id_table:
>  	.quad CPU_IDS
>  __id_end:
> -	.quad MPIDR_INVALID
>  
>  .equ	nr_cpus, ((__id_end - id_table) / 8)
>  
> @@ -159,17 +158,16 @@ psci_cpu_on:
>   * Clobbers x1, x2, x3
>   */
>  find_logical_id:
> -__find_logical_index:
>  	adr	x2, id_table
>  	mov	x1, xzr
>  1:	mov	x3, #nr_cpus	// check we haven't walked off the end of the array
>  	cmp	x1, x3
> -	b.gt	3f
> +	b.ge	3f
>  	ldr	x3, [x2, x1, lsl #3]
>  	cmp	x3, x0
>  	b.eq	2f
>  	add	x1, x1, #1
> -	b 1b
> +	b	1b
>  2:	mov	x0, x1
>  	ret
>  3:	mov	x0, #MPIDR_INVALID
> -- 
> 2.1.0
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation
  2015-01-13  2:15 [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Zi Shen Lim
                   ` (3 preceding siblings ...)
  2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 4/4] psci: implement PSCI v0.2 Zi Shen Lim
@ 2015-01-14 11:53 ` Mark Rutland
  2015-01-14 18:46   ` Z Lim
  4 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2015-01-14 11:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Jan 13, 2015 at 02:15:00AM +0000, Zi Shen Lim wrote:
> Hi Mark,

Hi,

> The first couple patches are cleanups.
> 
> The third patch switches to using v0.2 function ID values for the
> existing cpu_{on,off} implementations. This intermediate step is
> arguably unnecessary given the next patch...
> 
> The fourth patch provides a simple implementation of PSCI v0.2.
> The deviations from spec (primarily cpu_suspend not supported)
> is noted in code comment as well as in commit log.

While I'm not opposed to implementing PSCI 0.2, the two major reasons
for not having implemented it so far were simplicity and
spec-compliance. Unless implementing PSCI 0.2 gains us something, I
don't see the point in moving from PSCI 0.1. 

I think not having any supported CPU_SUSPEND states is fine. For spec
compliance I'm more worried by the behaviour of AFFINITY_INFO (as this
implementation seems to ignore the lowest_affinity_level parameter), and
the behaviour of SYSTEM_{OFF,RESET} -- I wonder if these could be
implemented via semihosting.

Given that SYSTEM_OFF and SYSTEM_RESET are effectively not implemented,
the only thing that I see we gain from this PSCI 0.2 implementation is
AFFINITY_INFO to close a race with CPU_OFF. Is PSCI 0.2 just a
nice-to-have feature, or is there something it provides that you
particularly want?

I occasionally need to use a new boot-wrapper with old kernels which
only handle PSCI 0.1, so we also need to maintain compatiblity in the
way the DT binding doc describes:

psci {
	compatible = "arm,psci-0.2", "arm,psci";
	method = "smc";
	cpu_on = < ... >;
	cpu_off = < ... >;
};

Thanks,
Mark.

> 
> Please consider merging.
> 
> Thanks, z
> 
> Zi Shen Lim (4): psci: use MPIDR_INVALID instead of -1 psci: remove
> sentinel from id_table psci: use PSCI v0.2 function IDs psci:
> implement PSCI v0.2
> 
>  Makefile.am |   4 +- psci.S      | 170
>  ++++++++++++++++++++++++++++++++++++++++++++++++++---------- 2 files
>  changed, 143 insertions(+), 31 deletions(-)
> 
> -- 2.1.0
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1
  2015-01-14 11:03   ` Mark Rutland
@ 2015-01-14 18:24     ` Z Lim
  0 siblings, 0 replies; 10+ messages in thread
From: Z Lim @ 2015-01-14 18:24 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Mark,

On Wed, Jan 14, 2015 at 3:03 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Tue, Jan 13, 2015 at 02:15:01AM +0000, Zi Shen Lim wrote:
>> MPIDR_INVALID was already defined, so use it.
>
> MPIDR_INVALID is meant to be an invalid MPIDR value, rather than an
> error code in the case of an invalid MPIDR.
>
> __find_logical_index is meant to return a logical CPU ID, so returning
> MPIDR_INVALID isn't quite right.
>
> I'd be happier if we used a separate macro for the logical IDs (e.g.
> ID_INVALID).

I follow your argument here.

I guess I should've reordered patches 1 and 2.
In patch 2, following removal of the sentinel, MPIDR_INVALID is no
longer found in id_table itself.
So MPIDR_INVALID is indeed the "ID_INVALID" you're suggesting.

Are you comfortable with this repurposing of MPIDR_INVALID?
Now, MPIDR_INVALID (-1) is consistently used as return value of
find_logical_id for an invalid/unknown MPIDR.

>
> Thanks,
> Mark.
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation
  2015-01-14 11:53 ` [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Mark Rutland
@ 2015-01-14 18:46   ` Z Lim
  0 siblings, 0 replies; 10+ messages in thread
From: Z Lim @ 2015-01-14 18:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jan 14, 2015 at 3:53 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jan 13, 2015 at 02:15:00AM +0000, Zi Shen Lim wrote:
>> Hi Mark,
>
> Hi,
>
>> The first couple patches are cleanups.
>>
>> The third patch switches to using v0.2 function ID values for the
>> existing cpu_{on,off} implementations. This intermediate step is
>> arguably unnecessary given the next patch...
>>
>> The fourth patch provides a simple implementation of PSCI v0.2.
>> The deviations from spec (primarily cpu_suspend not supported)
>> is noted in code comment as well as in commit log.
>
> While I'm not opposed to implementing PSCI 0.2, the two major reasons
> for not having implemented it so far were simplicity and
> spec-compliance. Unless implementing PSCI 0.2 gains us something, I
> don't see the point in moving from PSCI 0.1.

I'm in full agreement here. I don't think we'd want boot-wrapper to
become overly complicated - at some point we might as well use ARM
Trusted Firmware.

>
> I think not having any supported CPU_SUSPEND states is fine. For spec

Okay :)

> compliance I'm more worried by the behaviour of AFFINITY_INFO (as this
> implementation seems to ignore the lowest_affinity_level parameter), and

The simple implementation of psci_affinity_info only supports
lowest_affinity_level==0.
Motivation: the known user of this (arch/arm64/kernel/psci.c) only
uses lowest_affinity_level=0.
Do we need something more complex?

> the behaviour of SYSTEM_{OFF,RESET} -- I wonder if these could be
> implemented via semihosting.

Good point. Haven't thought about that option.
I also wonder if linux will use PSCI for these functions.

>
> Given that SYSTEM_OFF and SYSTEM_RESET are effectively not implemented,
> the only thing that I see we gain from this PSCI 0.2 implementation is
> AFFINITY_INFO to close a race with CPU_OFF. Is PSCI 0.2 just a
> nice-to-have feature, or is there something it provides that you
> particularly want?

I'm currently using this as baseline for PSCI 0.2 related work.
I have other (non-FVP) platform-specific changes as well, but figured
at least the generic/common portions could be shared upstream.

>
> I occasionally need to use a new boot-wrapper with old kernels which
> only handle PSCI 0.1, so we also need to maintain compatiblity in the
> way the DT binding doc describes:
>
> psci {
>         compatible = "arm,psci-0.2", "arm,psci";
>         method = "smc";
>         cpu_on = < ... >;
>         cpu_off = < ... >;
> };

Ok. In that case, along with the DT compatibility above, we'll also
need patch 3.

>
> Thanks,
> Mark.
>
>>
>> Please consider merging.
>>
>> Thanks, z
>>
>> Zi Shen Lim (4): psci: use MPIDR_INVALID instead of -1 psci: remove
>> sentinel from id_table psci: use PSCI v0.2 function IDs psci:
>> implement PSCI v0.2
>>
>>  Makefile.am |   4 +- psci.S      | 170
>>  ++++++++++++++++++++++++++++++++++++++++++++++++++---------- 2 files
>>  changed, 143 insertions(+), 31 deletions(-)
>>
>> -- 2.1.0
>>
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-01-14 18:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-13  2:15 [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Zi Shen Lim
2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 1/4] psci: use MPIDR_INVALID instead of -1 Zi Shen Lim
2015-01-14 11:03   ` Mark Rutland
2015-01-14 18:24     ` Z Lim
2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 2/4] psci: remove sentinel from id_table Zi Shen Lim
2015-01-14 11:10   ` Mark Rutland
2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 3/4] psci: use PSCI v0.2 function IDs Zi Shen Lim
2015-01-13  2:15 ` [PATCH boot-wrapper-aarch64 4/4] psci: implement PSCI v0.2 Zi Shen Lim
2015-01-14 11:53 ` [PATCH boot-wrapper-aarch64 0/4] psci cleanups + simple v0.2 implementation Mark Rutland
2015-01-14 18:46   ` Z Lim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).