* [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).