The PSCI v0.2+ spec mandates specific values of Function IDs for ARM32 and ARM64. Use DT bindings of Function IDs only when using older versions. Use static values otherwise. This patch also prepares the code to easily use the ACPI API which is based off of PSCI v0.2+.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org --- arch/arm/include/asm/psci.h | 15 ++++++ arch/arm/kernel/psci.c | 110 +++++++++++++++++++++++++++++++++++++----- arch/arm64/include/asm/psci.h | 12 +++++ arch/arm64/kernel/psci.c | 106 +++++++++++++++++++++++++++++++++++----- 4 files changed, 218 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index c4ae171..3f8ebf9 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -17,6 +17,19 @@ #define PSCI_POWER_STATE_TYPE_STANDBY 0 #define PSCI_POWER_STATE_TYPE_POWER_DOWN 1
+/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */ + +#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0x84000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0x84000003 +#define PSCI_ID_AFFINITY_INFO 0x84000004 +#define PSCI_ID_CPU_MIGRATE 0x84000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0x84000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009 + struct psci_power_state { u16 id; u8 type; @@ -36,9 +49,11 @@ extern struct smp_operations psci_smp_ops;
#ifdef CONFIG_ARM_PSCI void psci_init(void); +int psci_get_version(void); bool psci_smp_available(void); #else static inline void psci_init(void) { } +static inline int psci_get_version(void) { } static inline bool psci_smp_available(void) { return false; } #endif
diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c index 4693188..48e0b0a 100644 --- a/arch/arm/kernel/psci.c +++ b/arch/arm/kernel/psci.c @@ -27,6 +27,7 @@ struct psci_operations psci_ops;
static int (*invoke_psci_fn)(u32, u32, u32, u32); +typedef int (*psci_initcall_t)(const struct device_node *);
enum psci_function { PSCI_FN_CPU_SUSPEND, @@ -110,6 +111,20 @@ static noinline int __invoke_psci_fn_smc(u32 function_id, u32 arg0, u32 arg1, return function_id; }
+#define PSCI_VER_MAJOR_MASK 0xffff0000 +#define PSCI_VER_MINOR_MASK 0x0000ffff +#define PSCI_VER_MAJOR_SHIFT 16 + +static int psci_get_version(void) +{ + int err; + u32 fn; + + fn = PSCI_ID_VERSION; + err = invoke_psci_fn(fn, 0, 0, 0); + return err; +} + static int psci_cpu_suspend(struct psci_power_state state, unsigned long entry_point) { @@ -153,25 +168,65 @@ static int psci_migrate(unsigned long cpuid) return psci_to_linux_errno(err); }
-static const struct of_device_id psci_of_match[] __initconst = { - { .compatible = "arm,psci", }, - {}, -}; +/* + * PSCI Function ID's for v0.2+ are well defined so use + * static values. + */ +static int psci_static_init(struct device_node *np) +{ + const char *method; + int err = 0; + + pr_info("probing for conduit method from DT.\n"); + + if (of_property_read_string(np, "method", &method)) { + pr_warn("missing "method" property\n"); + err = -ENXIO; + goto out_put_node; + } + + if (!strcmp("hvc", method)) { + invoke_psci_fn = __invoke_psci_fn_hvc; + } else if (!strcmp("smc", method)) { + invoke_psci_fn = __invoke_psci_fn_smc; + } else { + pr_warn("invalid "method" property: %s\n", method); + err = -EINVAL; + goto out_put_node; + }
-void __init psci_init(void) + pr_info("Using static PSCIv0.2+ function IDs\n"); + psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_ID_CPU_SUSPEND; + psci_ops.cpu_suspend = psci_cpu_suspend; + + psci_function_id[PSCI_FN_CPU_OFF] = PSCI_ID_CPU_OFF; + psci_ops.cpu_off = psci_cpu_off; + + psci_function_id[PSCI_FN_CPU_ON] = PSCI_ID_CPU_ON; + psci_ops.cpu_on = psci_cpu_on; + + psci_function_id[PSCI_FN_MIGRATE] = PSCI_ID_CPU_MIGRATE; + psci_ops.migrate = psci_migrate; + +out_put_node: + of_node_put(np); + return err; +} + +/* + * PSCI < v0.2 can override PSCI function ID's via DT. + */ +static int psci_of_init(struct device_node *np) { - struct device_node *np; const char *method; u32 id; - - np = of_find_matching_node(NULL, psci_of_match); - if (!np) - return; + int err = 0;
pr_info("probing function IDs from device-tree\n");
if (of_property_read_string(np, "method", &method)) { - pr_warning("missing "method" property\n"); + pr_warn("missing "method" property\n"); + err = -EINVAL; goto out_put_node; }
@@ -180,7 +235,8 @@ void __init psci_init(void) } else if (!strcmp("smc", method)) { invoke_psci_fn = __invoke_psci_fn_smc; } else { - pr_warning("invalid "method" property: %s\n", method); + pr_warn("invalid "method" property: %s\n", method); + err = -ENXIO; goto out_put_node; }
@@ -206,5 +262,33 @@ void __init psci_init(void)
out_put_node: of_node_put(np); - return; + return err; +} + +static const struct of_device_id psci_of_match[] __initconst = { + { .compatible = "arm,psci", .data = psci_of_init}, + { .compatible = "arm,psci-0.2", .data = psci_static_init}, + {}, +}; + +int __init psci_init(void) +{ + struct device_node *np; + const struct of_device_id *matched_np; + psci_initcall_t init_fn; + int ver = 0; + + np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np); + if (!np) + return -ENODEV; + + ver = psci_get_version(); + + pr_info("using static PSCIv%d.%d Function IDs\n", + (ver & PSCI_VER_MAJOR_MASK) >> PSCI_VER_MAJOR_SHIFT, + (ver & PSCI_VER_MINOR_MASK)); + + + init_fn = (psci_initcall_t)matched_np->data; + return init_fn(np); } diff --git a/arch/arm64/include/asm/psci.h b/arch/arm64/include/asm/psci.h index e5312ea..b72678f 100644 --- a/arch/arm64/include/asm/psci.h +++ b/arch/arm64/include/asm/psci.h @@ -16,4 +16,16 @@
int psci_init(void);
+/* PSCI Function ID's for ARM64 as per PSCI spec v0.2 */ +#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0xc4000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0xc4000003 +#define PSCI_ID_AFFINITY_INFO 0xc4000004 +#define PSCI_ID_CPU_MIGRATE 0xc4000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0xc4000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009 + #endif /* __ASM_PSCI_H */ diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index 4f97db3..d4cff4e 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -45,12 +45,14 @@ struct psci_operations { static struct psci_operations psci_ops;
static int (*invoke_psci_fn)(u64, u64, u64, u64); +typedef int (*psci_initcall_t)(const struct device_node *);
enum psci_function { PSCI_FN_CPU_SUSPEND, PSCI_FN_CPU_ON, PSCI_FN_CPU_OFF, PSCI_FN_MIGRATE, + PSCI_FN_AFFINITY_INFO, PSCI_FN_MAX, };
@@ -128,6 +130,20 @@ static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64 arg1, return function_id; }
+#define PSCI_VER_MAJOR_MASK 0xffff0000 +#define PSCI_VER_MINOR_MASK 0x0000ffff +#define PSCI_VER_MAJOR_SHIFT 16 + +static int psci_get_version(void) +{ + int err; + u32 fn; + + fn = PSCI_ID_VERSION; + err = invoke_psci_fn(fn, 0, 0, 0); + return err; +} + static int psci_cpu_suspend(struct psci_power_state state, unsigned long entry_point) { @@ -171,26 +187,64 @@ static int psci_migrate(unsigned long cpuid) return psci_to_linux_errno(err); }
-static const struct of_device_id psci_of_match[] __initconst = { - { .compatible = "arm,psci", }, - {}, -}; +/* + * PSCI Function ID's for v0.2+ are well defined so use + * static values. + */ +static int psci_static_init(struct device_node *np) +{ + const char *method; + int err = 0;
-int __init psci_init(void) + pr_info("probing for conduit method from DT.\n"); + + if (of_property_read_string(np, "method", &method)) { + pr_warn("missing "method" property\n"); + err = -ENXIO; + goto out_put_node; + } + + if (!strcmp("hvc", method)) { + invoke_psci_fn = __invoke_psci_fn_hvc; + } else if (!strcmp("smc", method)) { + invoke_psci_fn = __invoke_psci_fn_smc; + } else { + pr_warn("invalid "method" property: %s\n", method); + err = -EINVAL; + goto out_put_node; + } + + pr_info("Using static PSCIv0.2+ function IDs\n"); + psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_ID_CPU_SUSPEND; + psci_ops.cpu_suspend = psci_cpu_suspend; + + psci_function_id[PSCI_FN_CPU_OFF] = PSCI_ID_CPU_OFF; + psci_ops.cpu_off = psci_cpu_off; + + psci_function_id[PSCI_FN_CPU_ON] = PSCI_ID_CPU_ON; + psci_ops.cpu_on = psci_cpu_on; + + psci_function_id[PSCI_FN_MIGRATE] = PSCI_ID_CPU_MIGRATE; + psci_ops.migrate = psci_migrate; + +out_put_node: + of_node_put(np); + return err; +} + +/* + * PSCI < v0.2 can override PSCI function ID's via DT. + */ +static int psci_of_init(struct device_node *np) { - struct device_node *np; const char *method; u32 id; int err = 0;
- np = of_find_matching_node(NULL, psci_of_match); - if (!np) - return -ENODEV; - pr_info("probing function IDs from device-tree\n");
if (of_property_read_string(np, "method", &method)) { - pr_warning("missing "method" property\n"); + pr_warn("missing "method" property\n"); err = -ENXIO; goto out_put_node; } @@ -200,7 +254,7 @@ int __init psci_init(void) } else if (!strcmp("smc", method)) { invoke_psci_fn = __invoke_psci_fn_smc; } else { - pr_warning("invalid "method" property: %s\n", method); + pr_warn("invalid "method" property: %s\n", method); err = -EINVAL; goto out_put_node; } @@ -230,6 +284,34 @@ out_put_node: return err; }
+static const struct of_device_id psci_of_match[] __initconst = { + { .compatible = "arm,psci", .data = psci_of_init}, + { .compatible = "arm,psci-0.2", .data = psci_static_init}, + {}, +}; + +int __init psci_init(void) +{ + struct device_node *np; + const struct of_device_id *matched_np; + psci_initcall_t init_fn; + int ver = 0; + + np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np); + + if (!np) + return -ENODEV; + + ver = psci_get_version(); + + pr_info("PSCIv%d.%d detected in firmware.\n", + (ver & PSCI_VER_MAJOR_MASK) >> PSCI_VER_MAJOR_SHIFT, + (ver & PSCI_VER_MINOR_MASK)); + + init_fn = (psci_initcall_t)matched_np->data; + return init_fn(np); +} + #ifdef CONFIG_SMP
static int __init cpu_psci_cpu_init(struct device_node *dn, unsigned int cpu)
The PSCI v0.2+ spec defines static values for PSCI function IDs. Add a new binding entry so that pre v0.2 implementations can use DT entries for function IDs and v0.2+ implementations use static entries as defined by the PSCIv0.2 specification.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org --- Documentation/devicetree/bindings/arm/psci.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt index 433afe9..a808284 100644 --- a/Documentation/devicetree/bindings/arm/psci.txt +++ b/Documentation/devicetree/bindings/arm/psci.txt @@ -21,7 +21,11 @@ to #0.
Main node required properties:
- - compatible : Must be "arm,psci" + - compatible : "arm,psci" : pre PSCIv0.2 spec implementations use this + to get the function IDs from DT entries. + + "arm,psci-0.2": PSCIv0.2+ spec implementations use static + values for function IDs as defined in the spec.
- method : The method of calling the PSCI firmware. Permitted values are:
On Wed, Mar 19, 2014 at 08:54:25PM -0400, Ashwin Chaugule wrote:
The PSCI v0.2+ spec defines static values for PSCI function IDs. Add a new binding entry so that pre v0.2 implementations can use DT entries for function IDs and v0.2+ implementations use static entries as defined by the PSCIv0.2 specification.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
Documentation/devicetree/bindings/arm/psci.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt index 433afe9..a808284 100644 --- a/Documentation/devicetree/bindings/arm/psci.txt +++ b/Documentation/devicetree/bindings/arm/psci.txt @@ -21,7 +21,11 @@ to #0. Main node required properties:
- compatible : Must be "arm,psci"
- compatible : "arm,psci" : pre PSCIv0.2 spec implementations use this
to get the function IDs from DT entries."arm,psci-0.2": PSCIv0.2+ spec implementations use staticvalues for function IDs as defined in the spec.
Maybe specify that in this case no function IDs are passed in the device tree?
-Christoffer
On Thu, Mar 20, 2014 at 03:03:19AM +0000, Christoffer Dall wrote:
On Wed, Mar 19, 2014 at 08:54:25PM -0400, Ashwin Chaugule wrote:
The PSCI v0.2+ spec defines static values for PSCI function IDs. Add a new binding entry so that pre v0.2 implementations can use DT entries for function IDs and v0.2+ implementations use static entries as defined by the PSCIv0.2 specification.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
Documentation/devicetree/bindings/arm/psci.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt index 433afe9..a808284 100644 --- a/Documentation/devicetree/bindings/arm/psci.txt +++ b/Documentation/devicetree/bindings/arm/psci.txt @@ -21,7 +21,11 @@ to #0. Main node required properties:
- compatible : Must be "arm,psci"
- compatible : "arm,psci" : pre PSCIv0.2 spec implementations use this
to get the function IDs from DT entries."arm,psci-0.2": PSCIv0.2+ spec implementations use staticvalues for function IDs as defined in the spec.
How about:
- compatible: should contain at least one of:
* "arm,psci" : for implementations complying to PSCI versions prior to 0.2. For these cases function IDs must be provided.
* "arm,psci-0.2" : for implementations complying to PSCI 0.2. Function IDs are not required and should be ignored by an OS with PSCI 0.2 support, but are permitted to be present for compatibility with existing software when "arm,psci" is later in the compatible list.
Maybe specify that in this case no function IDs are passed in the device tree?
As described above, for compatibility with existing kernels, the hypervisor will likely want to provide IDs, e.g.
psci { compatible = "arm,psci-0.2", "arm,psci"; method = "hvc";
cpu_on = < arbitrary value >; cpu_off = < arbitrary value >;
... };
In this case I'd expect an OS with PSCI 0.2 support to use the PSCI 0.2 IDs, but an older OS would use the IDs from the DT (which may or may not match the PSCI 0.2 IDs).
So while we should say is any function IDs should be ignored for "arm,psci-0.2", we shouldn't say that they are disallowed in the node.
The document should have examples of "arm,psci" only, "arm,psci-0.2" only, and the compatibility case.
Cheers, Mark.
On 20 March 2014 07:22, Mark Rutland mark.rutland@arm.com wrote:
On Thu, Mar 20, 2014 at 03:03:19AM +0000, Christoffer Dall wrote:
On Wed, Mar 19, 2014 at 08:54:25PM -0400, Ashwin Chaugule wrote:
The PSCI v0.2+ spec defines static values for PSCI function IDs. Add a new binding entry so that pre v0.2 implementations can use DT entries for function IDs and v0.2+ implementations use static entries as defined by the PSCIv0.2 specification.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
Documentation/devicetree/bindings/arm/psci.txt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt index 433afe9..a808284 100644 --- a/Documentation/devicetree/bindings/arm/psci.txt +++ b/Documentation/devicetree/bindings/arm/psci.txt @@ -21,7 +21,11 @@ to #0.
Main node required properties:
- compatible : Must be "arm,psci"
- compatible : "arm,psci" : pre PSCIv0.2 spec implementations use this
to get the function IDs from DT entries."arm,psci-0.2": PSCIv0.2+ spec implementations use staticvalues for function IDs as defined in the spec.How about:
compatible: should contain at least one of:
"arm,psci" : for implementations complying to PSCI versions prior to 0.2. For these cases function IDs must be provided.
"arm,psci-0.2" : for implementations complying to PSCI 0.2. Function IDs are not required and should be ignored by an OS with PSCI 0.2 support, but are permitted to be present for compatibility with existing software when "arm,psci" is later in the compatible list.
Maybe specify that in this case no function IDs are passed in the device tree?
As described above, for compatibility with existing kernels, the hypervisor will likely want to provide IDs, e.g.
psci { compatible = "arm,psci-0.2", "arm,psci"; method = "hvc";
cpu_on = < arbitrary value >; cpu_off = < arbitrary value >; ...};
In this case I'd expect an OS with PSCI 0.2 support to use the PSCI 0.2 IDs, but an older OS would use the IDs from the DT (which may or may not match the PSCI 0.2 IDs).
So while we should say is any function IDs should be ignored for "arm,psci-0.2", we shouldn't say that they are disallowed in the node.
The document should have examples of "arm,psci" only, "arm,psci-0.2" only, and the compatibility case.
That solves the ambiguity I read with the first draft, thanks.
-Chritoffer
On Wed, Mar 19, 2014 at 7:54 PM, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
The PSCI v0.2+ spec mandates specific values of Function IDs for ARM32 and ARM64. Use DT bindings of Function IDs only when using older versions. Use static values otherwise. This patch also prepares the code to easily use the ACPI API which is based off of PSCI v0.2+.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
arch/arm/include/asm/psci.h | 15 ++++++ arch/arm/kernel/psci.c | 110 +++++++++++++++++++++++++++++++++++++----- arch/arm64/include/asm/psci.h | 12 +++++ arch/arm64/kernel/psci.c | 106 +++++++++++++++++++++++++++++++++++----- 4 files changed, 218 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index c4ae171..3f8ebf9 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -17,6 +17,19 @@ #define PSCI_POWER_STATE_TYPE_STANDBY 0 #define PSCI_POWER_STATE_TYPE_POWER_DOWN 1
+/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
+#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0x84000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0x84000003 +#define PSCI_ID_AFFINITY_INFO 0x84000004 +#define PSCI_ID_CPU_MIGRATE 0x84000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0x84000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009
With the KVM patches[1], this makes 4 copies of these defines in the kernel (and then QEMU has more). This needs to go into a common header under include/linux. Probably under uapi so QEMU can use. There is some sync of headers between the kernel and QEMU I think.
Rob
On 19 March 2014 18:25, Rob Herring robherring2@gmail.com wrote:
On Wed, Mar 19, 2014 at 7:54 PM, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
The PSCI v0.2+ spec mandates specific values of Function IDs for ARM32 and ARM64. Use DT bindings of Function IDs only when using older versions. Use static values otherwise. This patch also prepares the code to easily use the ACPI API which is based off of PSCI v0.2+.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
arch/arm/include/asm/psci.h | 15 ++++++ arch/arm/kernel/psci.c | 110 +++++++++++++++++++++++++++++++++++++----- arch/arm64/include/asm/psci.h | 12 +++++ arch/arm64/kernel/psci.c | 106 +++++++++++++++++++++++++++++++++++----- 4 files changed, 218 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index c4ae171..3f8ebf9 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -17,6 +17,19 @@ #define PSCI_POWER_STATE_TYPE_STANDBY 0 #define PSCI_POWER_STATE_TYPE_POWER_DOWN 1
+/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
+#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0x84000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0x84000003 +#define PSCI_ID_AFFINITY_INFO 0x84000004 +#define PSCI_ID_CPU_MIGRATE 0x84000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0x84000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009
With the KVM patches[1], this makes 4 copies of these defines in the kernel (and then QEMU has more). This needs to go into a common header under include/linux. Probably under uapi so QEMU can use. There is some sync of headers between the kernel and QEMU I think.
yep, whenever something is in kvm/next or linus/master we can run a script in the qemu directory which updates the headers and we commit them in qemu with a reference to the kernel commit the headers come from.
-Christoffer
On Thu, Mar 20, 2014 at 01:25:53AM +0000, Rob Herring wrote:
On Wed, Mar 19, 2014 at 7:54 PM, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
The PSCI v0.2+ spec mandates specific values of Function IDs for ARM32 and ARM64. Use DT bindings of Function IDs only when using older versions. Use static values otherwise. This patch also prepares the code to easily use the ACPI API which is based off of PSCI v0.2+.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
arch/arm/include/asm/psci.h | 15 ++++++ arch/arm/kernel/psci.c | 110 +++++++++++++++++++++++++++++++++++++----- arch/arm64/include/asm/psci.h | 12 +++++ arch/arm64/kernel/psci.c | 106 +++++++++++++++++++++++++++++++++++----- 4 files changed, 218 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index c4ae171..3f8ebf9 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -17,6 +17,19 @@ #define PSCI_POWER_STATE_TYPE_STANDBY 0 #define PSCI_POWER_STATE_TYPE_POWER_DOWN 1
+/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
+#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0x84000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0x84000003 +#define PSCI_ID_AFFINITY_INFO 0x84000004 +#define PSCI_ID_CPU_MIGRATE 0x84000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0x84000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009
With the KVM patches[1], this makes 4 copies of these defines in the kernel (and then QEMU has more). This needs to go into a common header under include/linux. Probably under uapi so QEMU can use. There is some sync of headers between the kernel and QEMU I think.
I agree we should have a common header, but I don't think this is uapi material. This is most definitely _not_ part of the user API, and it's from an external spec that we're not in control of -- I don't see why we should have to export a set of numbers forever because it happens to be marginally useful at this point in time.
Having the IDs handled by the kernel and those used by hypervisors also encourages people to validate the use of the correct IDs rather than everyone making the same mistake.
Cheers, Mark.
Rob
On Thu, Mar 20, 2014 at 9:26 AM, Mark Rutland mark.rutland@arm.com wrote:
On Thu, Mar 20, 2014 at 01:25:53AM +0000, Rob Herring wrote:
On Wed, Mar 19, 2014 at 7:54 PM, Ashwin Chaugule ashwin.chaugule@linaro.org wrote:
The PSCI v0.2+ spec mandates specific values of Function IDs for ARM32 and ARM64. Use DT bindings of Function IDs only when using older versions. Use static values otherwise. This patch also prepares the code to easily use the ACPI API which is based off of PSCI v0.2+.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
arch/arm/include/asm/psci.h | 15 ++++++ arch/arm/kernel/psci.c | 110 +++++++++++++++++++++++++++++++++++++----- arch/arm64/include/asm/psci.h | 12 +++++ arch/arm64/kernel/psci.c | 106 +++++++++++++++++++++++++++++++++++----- 4 files changed, 218 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index c4ae171..3f8ebf9 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -17,6 +17,19 @@ #define PSCI_POWER_STATE_TYPE_STANDBY 0 #define PSCI_POWER_STATE_TYPE_POWER_DOWN 1
+/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
+#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0x84000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0x84000003 +#define PSCI_ID_AFFINITY_INFO 0x84000004 +#define PSCI_ID_CPU_MIGRATE 0x84000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0x84000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009
With the KVM patches[1], this makes 4 copies of these defines in the kernel (and then QEMU has more). This needs to go into a common header under include/linux. Probably under uapi so QEMU can use. There is some sync of headers between the kernel and QEMU I think.
I agree we should have a common header, but I don't think this is uapi material. This is most definitely _not_ part of the user API, and it's from an external spec that we're not in control of -- I don't see why we should have to export a set of numbers forever because it happens to be marginally useful at this point in time.
I mention uapi only in that it is expected to be stable (in an ABI sense) from kernel release to release, and I would assume that QEMU would only sync ABI stable headers. Someone familiar with the QEMU import process should comment on where this stuff should live.
Wouldn't this fall into the same category as something like include/uapi/linux/pci_regs.h?
Having the IDs handled by the kernel and those used by hypervisors also encourages people to validate the use of the correct IDs rather than everyone making the same mistake.
Duplicate copies seems like a poor way to validate correctness.
Rob
On Thu, Mar 20, 2014 at 12:04:45PM -0500, Rob Herring wrote:
[...]
With the KVM patches[1], this makes 4 copies of these defines in the kernel (and then QEMU has more). This needs to go into a common header under include/linux. Probably under uapi so QEMU can use. There is some sync of headers between the kernel and QEMU I think.
I agree we should have a common header, but I don't think this is uapi material. This is most definitely _not_ part of the user API, and it's from an external spec that we're not in control of -- I don't see why we should have to export a set of numbers forever because it happens to be marginally useful at this point in time.
I mention uapi only in that it is expected to be stable (in an ABI sense) from kernel release to release, and I would assume that QEMU would only sync ABI stable headers. Someone familiar with the QEMU import process should comment on where this stuff should live.
AFAIK, only uapi headers are imported to QEMU.
On Wed, Mar 19, 2014 at 08:54:24PM -0400, Ashwin Chaugule wrote:
The PSCI v0.2+ spec mandates specific values of Function IDs for ARM32 and ARM64. Use DT bindings of Function IDs only when using older versions. Use static values otherwise. This patch also prepares the code to easily use the ACPI API which is based off of PSCI v0.2+.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
arch/arm/include/asm/psci.h | 15 ++++++ arch/arm/kernel/psci.c | 110 +++++++++++++++++++++++++++++++++++++----- arch/arm64/include/asm/psci.h | 12 +++++ arch/arm64/kernel/psci.c | 106 +++++++++++++++++++++++++++++++++++----- 4 files changed, 218 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index c4ae171..3f8ebf9 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -17,6 +17,19 @@ #define PSCI_POWER_STATE_TYPE_STANDBY 0 #define PSCI_POWER_STATE_TYPE_POWER_DOWN 1 +/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
+#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0x84000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0x84000003 +#define PSCI_ID_AFFINITY_INFO 0x84000004 +#define PSCI_ID_CPU_MIGRATE 0x84000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0x84000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009
struct psci_power_state { u16 id; u8 type; @@ -36,9 +49,11 @@ extern struct smp_operations psci_smp_ops; #ifdef CONFIG_ARM_PSCI void psci_init(void); +int psci_get_version(void); bool psci_smp_available(void); #else static inline void psci_init(void) { } +static inline int psci_get_version(void) { } static inline bool psci_smp_available(void) { return false; } #endif diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c index 4693188..48e0b0a 100644 --- a/arch/arm/kernel/psci.c +++ b/arch/arm/kernel/psci.c @@ -27,6 +27,7 @@ struct psci_operations psci_ops; static int (*invoke_psci_fn)(u32, u32, u32, u32); +typedef int (*psci_initcall_t)(const struct device_node *); enum psci_function { PSCI_FN_CPU_SUSPEND, @@ -110,6 +111,20 @@ static noinline int __invoke_psci_fn_smc(u32 function_id, u32 arg0, u32 arg1, return function_id; } +#define PSCI_VER_MAJOR_MASK 0xffff0000 +#define PSCI_VER_MINOR_MASK 0x0000ffff +#define PSCI_VER_MAJOR_SHIFT 16
+static int psci_get_version(void) +{
- int err;
- u32 fn;
- fn = PSCI_ID_VERSION;
- err = invoke_psci_fn(fn, 0, 0, 0);
- return err;
+}
static int psci_cpu_suspend(struct psci_power_state state, unsigned long entry_point) { @@ -153,25 +168,65 @@ static int psci_migrate(unsigned long cpuid) return psci_to_linux_errno(err); } -static const struct of_device_id psci_of_match[] __initconst = {
- { .compatible = "arm,psci", },
- {},
-}; +/*
- PSCI Function ID's for v0.2+ are well defined so use
- static values.
- */
+static int psci_static_init(struct device_node *np) +{
- const char *method;
- int err = 0;
- pr_info("probing for conduit method from DT.\n");
- if (of_property_read_string(np, "method", &method)) {
pr_warn("missing \"method\" property\n");err = -ENXIO;goto out_put_node;- }
- if (!strcmp("hvc", method)) {
invoke_psci_fn = __invoke_psci_fn_hvc;- } else if (!strcmp("smc", method)) {
invoke_psci_fn = __invoke_psci_fn_smc;- } else {
pr_warn("invalid \"method\" property: %s\n", method);err = -EINVAL;goto out_put_node;- }
-void __init psci_init(void)
- pr_info("Using static PSCIv0.2+ function IDs\n");
- psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_ID_CPU_SUSPEND;
- psci_ops.cpu_suspend = psci_cpu_suspend;
- psci_function_id[PSCI_FN_CPU_OFF] = PSCI_ID_CPU_OFF;
- psci_ops.cpu_off = psci_cpu_off;
- psci_function_id[PSCI_FN_CPU_ON] = PSCI_ID_CPU_ON;
- psci_ops.cpu_on = psci_cpu_on;
- psci_function_id[PSCI_FN_MIGRATE] = PSCI_ID_CPU_MIGRATE;
- psci_ops.migrate = psci_migrate;
+out_put_node:
- of_node_put(np);
- return err;
+}
+/*
- PSCI < v0.2 can override PSCI function ID's via DT.
- */
+static int psci_of_init(struct device_node *np) {
- struct device_node *np; const char *method; u32 id;
- np = of_find_matching_node(NULL, psci_of_match);
- if (!np)
return;
- int err = 0;
pr_info("probing function IDs from device-tree\n"); if (of_property_read_string(np, "method", &method)) {
pr_warning("missing \"method\" property\n");
pr_warn("missing \"method\" property\n");err = -EINVAL;@@ -180,7 +235,8 @@ void __init psci_init(void) } else if (!strcmp("smc", method)) { invoke_psci_fn = __invoke_psci_fn_smc; } else {
pr_warning("invalid \"method\" property: %s\n", method);
pr_warn("invalid \"method\" property: %s\n", method);err = -ENXIO;@@ -206,5 +262,33 @@ void __init psci_init(void) out_put_node: of_node_put(np);
- return;
- return err;
+}
+static const struct of_device_id psci_of_match[] __initconst = {
- { .compatible = "arm,psci", .data = psci_of_init},
- { .compatible = "arm,psci-0.2", .data = psci_static_init},
- {},
+};
+int __init psci_init(void) +{
- struct device_node *np;
- const struct of_device_id *matched_np;
- psci_initcall_t init_fn;
- int ver = 0;
- np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
- if (!np)
return -ENODEV;- ver = psci_get_version();
- pr_info("using static PSCIv%d.%d Function IDs\n",
(ver & PSCI_VER_MAJOR_MASK) >> PSCI_VER_MAJOR_SHIFT,(ver & PSCI_VER_MINOR_MASK));- init_fn = (psci_initcall_t)matched_np->data;
- return init_fn(np);
} diff --git a/arch/arm64/include/asm/psci.h b/arch/arm64/include/asm/psci.h index e5312ea..b72678f 100644 --- a/arch/arm64/include/asm/psci.h +++ b/arch/arm64/include/asm/psci.h @@ -16,4 +16,16 @@ int psci_init(void); +/* PSCI Function ID's for ARM64 as per PSCI spec v0.2 */ +#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0xc4000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0xc4000003 +#define PSCI_ID_AFFINITY_INFO 0xc4000004 +#define PSCI_ID_CPU_MIGRATE 0xc4000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0xc4000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009
#endif /* __ASM_PSCI_H */ diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index 4f97db3..d4cff4e 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -45,12 +45,14 @@ struct psci_operations { static struct psci_operations psci_ops; static int (*invoke_psci_fn)(u64, u64, u64, u64); +typedef int (*psci_initcall_t)(const struct device_node *); enum psci_function { PSCI_FN_CPU_SUSPEND, PSCI_FN_CPU_ON, PSCI_FN_CPU_OFF, PSCI_FN_MIGRATE,
- PSCI_FN_AFFINITY_INFO, PSCI_FN_MAX,
}; @@ -128,6 +130,20 @@ static noinline int __invoke_psci_fn_smc(u64 function_id, u64 arg0, u64 arg1, return function_id; } +#define PSCI_VER_MAJOR_MASK 0xffff0000 +#define PSCI_VER_MINOR_MASK 0x0000ffff +#define PSCI_VER_MAJOR_SHIFT 16
+static int psci_get_version(void) +{
- int err;
- u32 fn;
- fn = PSCI_ID_VERSION;
- err = invoke_psci_fn(fn, 0, 0, 0);
- return err;
+}
static int psci_cpu_suspend(struct psci_power_state state, unsigned long entry_point) { @@ -171,26 +187,64 @@ static int psci_migrate(unsigned long cpuid) return psci_to_linux_errno(err); } -static const struct of_device_id psci_of_match[] __initconst = {
- { .compatible = "arm,psci", },
- {},
-}; +/*
- PSCI Function ID's for v0.2+ are well defined so use
- static values.
- */
+static int psci_static_init(struct device_node *np) +{
- const char *method;
- int err = 0;
-int __init psci_init(void)
- pr_info("probing for conduit method from DT.\n");
- if (of_property_read_string(np, "method", &method)) {
pr_warn("missing \"method\" property\n");err = -ENXIO;goto out_put_node;- }
- if (!strcmp("hvc", method)) {
invoke_psci_fn = __invoke_psci_fn_hvc;- } else if (!strcmp("smc", method)) {
invoke_psci_fn = __invoke_psci_fn_smc;- } else {
pr_warn("invalid \"method\" property: %s\n", method);err = -EINVAL;goto out_put_node;- }
- pr_info("Using static PSCIv0.2+ function IDs\n");
- psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_ID_CPU_SUSPEND;
- psci_ops.cpu_suspend = psci_cpu_suspend;
- psci_function_id[PSCI_FN_CPU_OFF] = PSCI_ID_CPU_OFF;
- psci_ops.cpu_off = psci_cpu_off;
- psci_function_id[PSCI_FN_CPU_ON] = PSCI_ID_CPU_ON;
- psci_ops.cpu_on = psci_cpu_on;
- psci_function_id[PSCI_FN_MIGRATE] = PSCI_ID_CPU_MIGRATE;
- psci_ops.migrate = psci_migrate;
+out_put_node:
- of_node_put(np);
- return err;
Can any of this code not be shared between arm and arm64?
Looks almost identical.
-Christoffer
On Thu, Mar 20, 2014 at 12:54:24AM +0000, Ashwin Chaugule wrote:
The PSCI v0.2+ spec mandates specific values of Function IDs for ARM32 and ARM64. Use DT bindings of Function IDs only when using older versions. Use static values otherwise. This patch also prepares the code to easily use the ACPI API which is based off of PSCI v0.2+.
Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org
arch/arm/include/asm/psci.h | 15 ++++++ arch/arm/kernel/psci.c | 110 +++++++++++++++++++++++++++++++++++++----- arch/arm64/include/asm/psci.h | 12 +++++ arch/arm64/kernel/psci.c | 106 +++++++++++++++++++++++++++++++++++----- 4 files changed, 218 insertions(+), 25 deletions(-)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index c4ae171..3f8ebf9 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -17,6 +17,19 @@ #define PSCI_POWER_STATE_TYPE_STANDBY 0 #define PSCI_POWER_STATE_TYPE_POWER_DOWN 1 +/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
Please drop the superfluous apostrophes, we aren't greengrocers [1].
%s/ID's/IDs/
+#define PSCI_ID_VERSION 0x84000000 +#define PSCI_ID_CPU_SUSPEND 0x84000001 +#define PSCI_ID_CPU_OFF 0x84000002 +#define PSCI_ID_CPU_ON 0x84000003 +#define PSCI_ID_AFFINITY_INFO 0x84000004 +#define PSCI_ID_CPU_MIGRATE 0x84000005 +#define PSCI_ID_MIGRATE_INFO_TYPE 0x84000006 +#define PSCI_ID_MIGRATE_INFO_UP_CPU 0x84000007 +#define PSCI_ID_SYSTEM_OFF 0x84000008 +#define PSCI_ID_SYSTEM_RESET 0x84000009
struct psci_power_state { u16 id; u8 type; @@ -36,9 +49,11 @@ extern struct smp_operations psci_smp_ops; #ifdef CONFIG_ARM_PSCI void psci_init(void); +int psci_get_version(void); bool psci_smp_available(void); #else static inline void psci_init(void) { } +static inline int psci_get_version(void) { } static inline bool psci_smp_available(void) { return false; } #endif diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c index 4693188..48e0b0a 100644 --- a/arch/arm/kernel/psci.c +++ b/arch/arm/kernel/psci.c @@ -27,6 +27,7 @@ struct psci_operations psci_ops; static int (*invoke_psci_fn)(u32, u32, u32, u32); +typedef int (*psci_initcall_t)(const struct device_node *); enum psci_function { PSCI_FN_CPU_SUSPEND, @@ -110,6 +111,20 @@ static noinline int __invoke_psci_fn_smc(u32 function_id, u32 arg0, u32 arg1, return function_id; } +#define PSCI_VER_MAJOR_MASK 0xffff0000 +#define PSCI_VER_MINOR_MASK 0x0000ffff +#define PSCI_VER_MAJOR_SHIFT 16
+static int psci_get_version(void) +{
- int err;
- u32 fn;
- fn = PSCI_ID_VERSION;
- err = invoke_psci_fn(fn, 0, 0, 0);
Do we need fn here? Can't we pass PSCI_ID_VERSION directly?
- return err;
+}
static int psci_cpu_suspend(struct psci_power_state state, unsigned long entry_point) { @@ -153,25 +168,65 @@ static int psci_migrate(unsigned long cpuid) return psci_to_linux_errno(err); } -static const struct of_device_id psci_of_match[] __initconst = {
- { .compatible = "arm,psci", },
- {},
-}; +/*
- PSCI Function ID's for v0.2+ are well defined so use
- static values.
- */
+static int psci_static_init(struct device_node *np) +{
- const char *method;
- int err = 0;
- pr_info("probing for conduit method from DT.\n");
- if (of_property_read_string(np, "method", &method)) {
pr_warn("missing \"method\" property\n");err = -ENXIO;goto out_put_node;- }
- if (!strcmp("hvc", method)) {
invoke_psci_fn = __invoke_psci_fn_hvc;- } else if (!strcmp("smc", method)) {
invoke_psci_fn = __invoke_psci_fn_smc;- } else {
pr_warn("invalid \"method\" property: %s\n", method);err = -EINVAL;goto out_put_node;- }
-void __init psci_init(void)
- pr_info("Using static PSCIv0.2+ function IDs\n");
How about "Using standard PSCI 0.2 function IDs"?
We're not using anything beyond PSCI 0.2 (yet), and "static" implies that there are dynamic IDs also.
- psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_ID_CPU_SUSPEND;
- psci_ops.cpu_suspend = psci_cpu_suspend;
- psci_function_id[PSCI_FN_CPU_OFF] = PSCI_ID_CPU_OFF;
- psci_ops.cpu_off = psci_cpu_off;
- psci_function_id[PSCI_FN_CPU_ON] = PSCI_ID_CPU_ON;
- psci_ops.cpu_on = psci_cpu_on;
- psci_function_id[PSCI_FN_MIGRATE] = PSCI_ID_CPU_MIGRATE;
- psci_ops.migrate = psci_migrate;
+out_put_node:
- of_node_put(np);
- return err;
+}
I'd very much like to see us use AFFINITY_INFO from the start, or we've got a racy down path (as we have for all PSCI implementations prior to 0.2), and implementors are likely to skip implementing it if Linux seems to work, so we'll never be able to rely on it.
Additionally it'd be nice to check what MIGRATE_INFO_TYPE returns. If it returns a value other than 2 (MP or not present), then there's some additional work that we need to perform around a down path or we might not be able to shut down some CPUs.
+/*
- PSCI < v0.2 can override PSCI function ID's via DT.
- */
+static int psci_of_init(struct device_node *np) {
- struct device_node *np; const char *method; u32 id;
- np = of_find_matching_node(NULL, psci_of_match);
- if (!np)
return;
- int err = 0;
pr_info("probing function IDs from device-tree\n"); if (of_property_read_string(np, "method", &method)) {
pr_warning("missing \"method\" property\n");
pr_warn("missing \"method\" property\n");err = -EINVAL;@@ -180,7 +235,8 @@ void __init psci_init(void) } else if (!strcmp("smc", method)) { invoke_psci_fn = __invoke_psci_fn_smc; } else {
pr_warning("invalid \"method\" property: %s\n", method);
pr_warn("invalid \"method\" property: %s\n", method);err = -ENXIO;
We should definitely factor this into a helper rather than duplicating it.
@@ -206,5 +262,33 @@ void __init psci_init(void) out_put_node: of_node_put(np);
- return;
- return err;
+}
+static const struct of_device_id psci_of_match[] __initconst = {
- { .compatible = "arm,psci", .data = psci_of_init},
- { .compatible = "arm,psci-0.2", .data = psci_static_init},
- {},
+};
+int __init psci_init(void) +{
- struct device_node *np;
- const struct of_device_id *matched_np;
- psci_initcall_t init_fn;
- int ver = 0;
- np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
- if (!np)
return -ENODEV;- ver = psci_get_version();
- pr_info("using static PSCIv%d.%d Function IDs\n",
(ver & PSCI_VER_MAJOR_MASK) >> PSCI_VER_MAJOR_SHIFT,(ver & PSCI_VER_MINOR_MASK));
Thanks for adding the version information.
Cheers, Mark.
[1] http://en.wikipedia.org/wiki/Apostrophe#Superfluous_apostrophes_.28.22greeng...
Hi Mark,
On 20 March 2014 10:57, Mark Rutland mark.rutland@arm.com wrote:
+/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
Please drop the superfluous apostrophes, we aren't greengrocers [1].
%s/ID's/IDs/
Ok. Will fix all the uncouth bacilli. ;)
fn = PSCI_ID_VERSION;err = invoke_psci_fn(fn, 0, 0, 0);Do we need fn here? Can't we pass PSCI_ID_VERSION directly?
yep.
-void __init psci_init(void)
pr_info("Using static PSCIv0.2+ function IDs\n");How about "Using standard PSCI 0.2 function IDs"?
We're not using anything beyond PSCI 0.2 (yet), and "static" implies that there are dynamic IDs also.
Ok.
psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_ID_CPU_SUSPEND;psci_ops.cpu_suspend = psci_cpu_suspend;psci_function_id[PSCI_FN_CPU_OFF] = PSCI_ID_CPU_OFF;psci_ops.cpu_off = psci_cpu_off;psci_function_id[PSCI_FN_CPU_ON] = PSCI_ID_CPU_ON;psci_ops.cpu_on = psci_cpu_on;psci_function_id[PSCI_FN_MIGRATE] = PSCI_ID_CPU_MIGRATE;psci_ops.migrate = psci_migrate;+out_put_node:
of_node_put(np);return err;+}
I'd very much like to see us use AFFINITY_INFO from the start, or we've got a racy down path (as we have for all PSCI implementations prior to 0.2), and implementors are likely to skip implementing it if Linux seems to work, so we'll never be able to rely on it.
Additionally it'd be nice to check what MIGRATE_INFO_TYPE returns. If it returns a value other than 2 (MP or not present), then there's some additional work that we need to perform around a down path or we might not be able to shut down some CPUs.
The main intention of this patch is to separate function ID detection depending on PSCI versions. So, I think the additional functionality of AFFINITY_INFO and MIGRATE_INFO_TYPE should go in as separate patches. Do you agree?
+/*
- PSCI < v0.2 can override PSCI function ID's via DT.
- */
+static int psci_of_init(struct device_node *np) {
struct device_node *np; const char *method; u32 id;np = of_find_matching_node(NULL, psci_of_match);if (!np)return;
int err = 0; pr_info("probing function IDs from device-tree\n"); if (of_property_read_string(np, "method", &method)) {
pr_warning("missing \"method\" property\n");
pr_warn("missing \"method\" property\n");err = -EINVAL; goto out_put_node; }@@ -180,7 +235,8 @@ void __init psci_init(void) } else if (!strcmp("smc", method)) { invoke_psci_fn = __invoke_psci_fn_smc; } else {
pr_warning("invalid \"method\" property: %s\n", method);
pr_warn("invalid \"method\" property: %s\n", method);err = -ENXIO; goto out_put_node; }We should definitely factor this into a helper rather than duplicating it.
Ok I'll look into this.
Cheers, Ashwin
On Thu, Mar 20, 2014 at 03:39:30PM +0000, Ashwin Chaugule wrote:
Hi Mark,
On 20 March 2014 10:57, Mark Rutland mark.rutland@arm.com wrote:
+/* PSCI Function ID's for ARM32 as per PSCI spec v0.2 */
Please drop the superfluous apostrophes, we aren't greengrocers [1].
%s/ID's/IDs/
Ok. Will fix all the uncouth bacilli. ;)
Cheers :)
psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_ID_CPU_SUSPEND;psci_ops.cpu_suspend = psci_cpu_suspend;psci_function_id[PSCI_FN_CPU_OFF] = PSCI_ID_CPU_OFF;psci_ops.cpu_off = psci_cpu_off;psci_function_id[PSCI_FN_CPU_ON] = PSCI_ID_CPU_ON;psci_ops.cpu_on = psci_cpu_on;psci_function_id[PSCI_FN_MIGRATE] = PSCI_ID_CPU_MIGRATE;psci_ops.migrate = psci_migrate;+out_put_node:
of_node_put(np);return err;+}
I'd very much like to see us use AFFINITY_INFO from the start, or we've got a racy down path (as we have for all PSCI implementations prior to 0.2), and implementors are likely to skip implementing it if Linux seems to work, so we'll never be able to rely on it.
Additionally it'd be nice to check what MIGRATE_INFO_TYPE returns. If it returns a value other than 2 (MP or not present), then there's some additional work that we need to perform around a down path or we might not be able to shut down some CPUs.
The main intention of this patch is to separate function ID detection depending on PSCI versions. So, I think the additional functionality of AFFINITY_INFO and MIGRATE_INFO_TYPE should go in as separate patches. Do you agree?
Unfortunately this has the side effect of seemingly adding support for PSCI 0.2 while not actually doing so. I fear that people will begin using the "arm,psci-0.2" string before we end up implementing code using the other required functionality, and it will turn out we can't make use of any of it, because it will cause someone's firmware to blow up.
I do not feel comfortable adding support for a not-quite-psci-0.2, especially when adding trivial use of some required functionality does not seem that difficult.
Cheers, Mark.
On 21 March 2014 06:51, Mark Rutland mark.rutland@arm.com wrote:
The main intention of this patch is to separate function ID detection depending on PSCI versions. So, I think the additional functionality of AFFINITY_INFO and MIGRATE_INFO_TYPE should go in as separate patches. Do you agree?
Unfortunately this has the side effect of seemingly adding support for PSCI 0.2 while not actually doing so. I fear that people will begin using the "arm,psci-0.2" string before we end up implementing code using the other required functionality, and it will turn out we can't make use of any of it, because it will cause someone's firmware to blow up.
Ah. I've only seen PSCI-v0.2. Didnt know that this stuff was part of the new additions to the spec.
Ashwin
-
Ashwin Chaugule -
Christoffer Dall -
Mark Rutland -
Rob Herring