psci cleanups + simple v0.2 implementation

MPIDR_INVALID was already defined, so use it.

Signed-off-by Zi Shen Lim <[email protected]>

---
 psci.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

psci.S +3 -3

index 856095b..234493d 100644
@@ -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

Signature · 1 line

2.1.0

Hi,

Quoted 1 line · Zi Shen Lim

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.

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 <[email protected]>

---
 psci.S | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

psci.S +2 -4

index 234493d..d045e56 100644
@@ -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

Signature · 1 line

2.1.0

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

Quoted 1 line · Zi Shen Lim

Hi Mark,

Hi,

Quoted 6 lines

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...

Quoted 3 lines

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.

Quoted 16 lines

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