[PATCH v3 0/7] PSCI support for highbank

[PATCH v3 0/7] PSCI support for highbank

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

This is an updated series converting highbank platform to use PSCI calls
for smp_ops, cpuidle, suspend, reset and poweroff. Since the last posting,
the PSCI spec has been updated by ARM to version 0.2 [1]. The primary
kernel visible change in the specification is the addition of system level
reset and poweroff functions.

Rob

[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0022b/index.html

v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/159519.html
v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2013-February/151820.html

Rob Herring (7):
  dt: update PSCI binding documentation for v0.2
  ARM: PSCI: remove unnecessary include of arm-gic.h
  ARM: PSCI: add ops for system restart and power off
  cpuidle: calxeda: add support to use PSCI calls
  ARM: highbank: clean-up some unused includes
  ARM: highbank: adapt to use ARM PSCI calls
  dts: calxeda: add ARM PSCI binding

 Documentation/devicetree/bindings/arm/psci.txt | 25 ++++++--
 arch/arm/boot/dts/ecx-common.dtsi              | 10 +++
 arch/arm/include/asm/psci.h                    |  2 +
 arch/arm/kernel/psci.c                         | 40 ++++++++++++
 arch/arm/kernel/psci_smp.c                     |  1 -
 arch/arm/mach-highbank/Kconfig                 |  2 +-
 arch/arm/mach-highbank/Makefile                |  4 +-
 arch/arm/mach-highbank/core.h                  |  9 ---
 arch/arm/mach-highbank/highbank.c              | 55 +---------------
 arch/arm/mach-highbank/hotplug.c               | 37 -----------
 arch/arm/mach-highbank/platsmp.c               | 68 --------------------
 arch/arm/mach-highbank/pm.c                    | 27 +++-----
 arch/arm/mach-highbank/sysregs.h               | 86 --------------------------
 arch/arm/mach-highbank/system.c                | 33 ----------
 drivers/cpuidle/cpuidle-calxeda.c              | 40 ++----------
 15 files changed, 89 insertions(+), 350 deletions(-)
 delete mode 100644 arch/arm/mach-highbank/hotplug.c
 delete mode 100644 arch/arm/mach-highbank/platsmp.c
 delete mode 100644 arch/arm/mach-highbank/sysregs.h
 delete mode 100644 arch/arm/mach-highbank/system.c

-- 
1.8.1.2

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

The PSCI spec from ARM has been updated to 0.2 version. Update the
binding document to reflect the spec changes. For the binding, the
major changes are addition of system reset and poweroff functions.
The recommended function id numbering has also changed.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: devicetree at vger.kernel.org
---
 Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index 433afe9..b8b4d9f 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -21,7 +21,7 @@ to #0.
 
 Main node required properties:
 
- - compatible    : Must be "arm,psci"
+ - compatible    : Must be "arm,psci-0.2" or "arm,psci"
 
  - method        : The method of calling the PSCI firmware. Permitted
                    values are:
@@ -32,6 +32,9 @@ Main node required properties:
                    "hvc" : HVC #0, with the register assignments specified
 		           in this binding.
 
+ - psci_version  : Function ID for PSCI_VERSION operation. Required for
+                   "arm,psci-0.2" compatible version or later.
+
 Main node optional properties:
 
  - cpu_suspend   : Function ID for CPU_SUSPEND operation
@@ -42,14 +45,24 @@ Main node optional properties:
 
  - migrate       : Function ID for MIGRATE operation
 
+ - system_reset  : Function ID for SYSTEM_RESET operation
+
+ - system_off    : Function ID for SYSTEM_OFF operation
+
 
 Example:
 
 	psci {
-		compatible	= "arm,psci";
+		compatible	= "arm,psci-0.2";
 		method		= "smc";
-		cpu_suspend	= <0x95c10000>;
-		cpu_off		= <0x95c10001>;
-		cpu_on		= <0x95c10002>;
-		migrate		= <0x95c10003>;
+		psci_version	= <0x84000000>;
+		cpu_suspend	= <0x84000001>;
+		cpu_off		= <0x84000002>;
+		cpu_on		= <0x84000003>;
+		affinity_info	= <0x84000004>; 
+		migrate		= <0x84000005>;
+		migrate_info_type = <0x84000006>; 
+		migrate_info_up_cpu = <0x84000007>; 
+		system_off	= <0x84000008>; 
+		system_reset	= <0x84000009>; 
 	};
-- 
1.8.1.2

[PATCH 2/7] ARM: PSCI: remove unnecessary include of arm-gic.h

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Now that gic_secondary_init is no longer needed to be called by SMP init
functions, the header is not needed.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/kernel/psci_smp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
index 70ded3f..570a48c 100644
--- a/arch/arm/kernel/psci_smp.c
+++ b/arch/arm/kernel/psci_smp.c
@@ -14,7 +14,6 @@
  */
 
 #include <linux/init.h>
-#include <linux/irqchip/arm-gic.h>
 #include <linux/smp.h>
 #include <linux/of.h>
 
-- 
1.8.1.2

[PATCH 3/7] ARM: PSCI: add ops for system restart and power off

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

In PSCI v0.2 spec, operations for system level restart and power off are
added. This adds kernel support for those operations. The behavior for
picking restart function matches the psci_smp_ops such that the PSCI
restart function will be used when present even if a platform defines
a mdesc->restart entry. As pm_power_off depends on platform init code to
setup, using the PSCI version relies on the platform code to not override
it.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: Russell King <linux@arm.linux.org.uk>
---
 arch/arm/include/asm/psci.h |  2 ++
 arch/arm/kernel/psci.c      | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
index c4ae171..04ee1b2 100644
--- a/arch/arm/include/asm/psci.h
+++ b/arch/arm/include/asm/psci.h
@@ -29,6 +29,8 @@ struct psci_operations {
 	int (*cpu_off)(struct psci_power_state state);
 	int (*cpu_on)(unsigned long cpuid, unsigned long entry_point);
 	int (*migrate)(unsigned long cpuid);
+	void (*system_off)(void);
+	void (*system_reset)(void);
 };
 
 extern struct psci_operations psci_ops;
diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index 4693188..ffc4a7c 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -17,12 +17,14 @@
 
 #include <linux/init.h>
 #include <linux/of.h>
+#include <linux/pm.h>
 
 #include <asm/compiler.h>
 #include <asm/errno.h>
 #include <asm/opcodes-sec.h>
 #include <asm/opcodes-virt.h>
 #include <asm/psci.h>
+#include <asm/system_misc.h>
 
 struct psci_operations psci_ops;
 
@@ -33,6 +35,8 @@ enum psci_function {
 	PSCI_FN_CPU_ON,
 	PSCI_FN_CPU_OFF,
 	PSCI_FN_MIGRATE,
+	PSCI_FN_SYS_OFF,
+	PSCI_FN_SYS_RESET,
 	PSCI_FN_MAX,
 };
 
@@ -153,6 +157,30 @@ static int psci_migrate(unsigned long cpuid)
 	return psci_to_linux_errno(err);
 }
 
+static void psci_system_off(void)
+{
+	u32 fn = psci_function_id[PSCI_FN_SYS_OFF];
+	invoke_psci_fn(fn, 0, 0, 0);
+}
+
+static void psci_system_reset(void)
+{
+	u32 fn = psci_function_id[PSCI_FN_SYS_RESET];
+	invoke_psci_fn(fn, 0, 0, 0);
+}
+
+void psci_pm_power_off(void)
+{
+	if (psci_ops.system_off)
+		psci_ops.system_off();
+}
+
+void psci_restart(enum reboot_mode reboot_mode, const char *cmd)
+{
+	if (psci_ops.system_off)
+		psci_ops.system_reset();
+}
+
 static const struct of_device_id psci_of_match[] __initconst = {
 	{ .compatible = "arm,psci",	},
 	{},
@@ -204,6 +232,18 @@ void __init psci_init(void)
 		psci_ops.migrate = psci_migrate;
 	}
 
+	if (!of_property_read_u32(np, "system_off", &id)) {
+		psci_function_id[PSCI_FN_SYS_OFF] = id;
+		psci_ops.system_off = psci_system_off;
+		pm_power_off = psci_pm_power_off;
+	}
+
+	if (!of_property_read_u32(np, "system_reset", &id)) {
+		psci_function_id[PSCI_FN_SYS_RESET] = id;
+		psci_ops.system_reset = psci_system_reset;
+		arm_pm_restart = psci_restart;
+	}
+
 out_put_node:
 	of_node_put(np);
 	return;
-- 
1.8.1.2

[PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

This updates the Calxeda cpuidle driver to use PSCI calls to powergate
cores. This is needed to enable cpuidle for the ECX-2000.

This could possibly become a generic PSCI driver, but there are no other
PSCI users in the kernel other than mach-virt.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
---
 drivers/cpuidle/cpuidle-calxeda.c | 40 ++++++---------------------------------
 1 file changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c
index 0e6e408..01cfecf3 100644
--- a/drivers/cpuidle/cpuidle-calxeda.c
+++ b/drivers/cpuidle/cpuidle-calxeda.c
@@ -22,51 +22,23 @@
 
 #include <linux/cpuidle.h>
 #include <linux/init.h>
-#include <linux/io.h>
 #include <linux/of.h>
-#include <linux/time.h>
-#include <linux/delay.h>
-#include <linux/suspend.h>
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
-#include <asm/smp_scu.h>
 #include <asm/suspend.h>
-#include <asm/cacheflush.h>
-#include <asm/cp15.h>
-
-extern void highbank_set_cpu_jump(int cpu, void *jump_addr);
-extern void *scu_base_addr;
-
-static noinline void calxeda_idle_restore(void)
-{
-	set_cr(get_cr() | CR_C);
-	set_auxcr(get_auxcr() | 0x40);
-	scu_power_mode(scu_base_addr, SCU_PM_NORMAL);
-}
+#include <asm/psci.h>
 
 static int calxeda_idle_finish(unsigned long val)
 {
-	/* Already flushed cache, but do it again as the outer cache functions
-	 * dirty the cache with spinlocks */
-	flush_cache_all();
-
-	set_auxcr(get_auxcr() & ~0x40);
-	set_cr(get_cr() & ~CR_C);
-
-	scu_power_mode(scu_base_addr, SCU_PM_DORMANT);
-
-	cpu_do_idle();
-
-	/* Restore things if we didn't enter power-gating */
-	calxeda_idle_restore();
-	return 1;
+	const struct psci_power_state ps = {
+		.type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
+	};
+	return psci_ops.cpu_suspend(ps, __pa(cpu_resume));
 }
 
 static int calxeda_pwrdown_idle(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index)
 {
-	highbank_set_cpu_jump(smp_processor_id(), cpu_resume);
 	cpu_suspend(0, calxeda_idle_finish);
 	return index;
 }
@@ -90,7 +62,7 @@ static struct cpuidle_driver calxeda_idle_driver = {
 
 static int __init calxeda_cpuidle_init(void)
 {
-	if (!of_machine_is_compatible("calxeda,highbank"))
+	if (!of_machine_is_compatible("calxeda,highbank") || !psci_ops.cpu_suspend)
 		return -ENODEV;
 
 	return cpuidle_register(&calxeda_idle_driver, NULL);
-- 
1.8.1.2

[PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

This updates the Calxeda cpuidle driver to use PSCI calls to powergate
cores. This is needed to enable cpuidle for the ECX-2000.

This could possibly become a generic PSCI driver, but there are no other
PSCI users in the kernel other than mach-virt.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm at vger.kernel.org
---
 drivers/cpuidle/cpuidle-calxeda.c | 40 ++++++---------------------------------
 1 file changed, 6 insertions(+), 34 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c
index 0e6e408..01cfecf3 100644
--- a/drivers/cpuidle/cpuidle-calxeda.c
+++ b/drivers/cpuidle/cpuidle-calxeda.c
@@ -22,51 +22,23 @@
 
 #include <linux/cpuidle.h>
 #include <linux/init.h>
-#include <linux/io.h>
 #include <linux/of.h>
-#include <linux/time.h>
-#include <linux/delay.h>
-#include <linux/suspend.h>
 #include <asm/cpuidle.h>
-#include <asm/proc-fns.h>
-#include <asm/smp_scu.h>
 #include <asm/suspend.h>
-#include <asm/cacheflush.h>
-#include <asm/cp15.h>
-
-extern void highbank_set_cpu_jump(int cpu, void *jump_addr);
-extern void *scu_base_addr;
-
-static noinline void calxeda_idle_restore(void)
-{
-	set_cr(get_cr() | CR_C);
-	set_auxcr(get_auxcr() | 0x40);
-	scu_power_mode(scu_base_addr, SCU_PM_NORMAL);
-}
+#include <asm/psci.h>
 
 static int calxeda_idle_finish(unsigned long val)
 {
-	/* Already flushed cache, but do it again as the outer cache functions
-	 * dirty the cache with spinlocks */
-	flush_cache_all();
-
-	set_auxcr(get_auxcr() & ~0x40);
-	set_cr(get_cr() & ~CR_C);
-
-	scu_power_mode(scu_base_addr, SCU_PM_DORMANT);
-
-	cpu_do_idle();
-
-	/* Restore things if we didn't enter power-gating */
-	calxeda_idle_restore();
-	return 1;
+	const struct psci_power_state ps = {
+		.type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
+	};
+	return psci_ops.cpu_suspend(ps, __pa(cpu_resume));
 }
 
 static int calxeda_pwrdown_idle(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index)
 {
-	highbank_set_cpu_jump(smp_processor_id(), cpu_resume);
 	cpu_suspend(0, calxeda_idle_finish);
 	return index;
 }
@@ -90,7 +62,7 @@ static struct cpuidle_driver calxeda_idle_driver = {
 
 static int __init calxeda_cpuidle_init(void)
 {
-	if (!of_machine_is_compatible("calxeda,highbank"))
+	if (!of_machine_is_compatible("calxeda,highbank") || !psci_ops.cpu_suspend)
 		return -ENODEV;
 
 	return cpuidle_register(&calxeda_idle_driver, NULL);
-- 
1.8.1.2

[PATCH 5/7] ARM: highbank: clean-up some unused includes

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/mach-highbank/highbank.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/arch/arm/mach-highbank/highbank.c b/arch/arm/mach-highbank/highbank.c
index dc5d6be..0f5c661 100644
--- a/arch/arm/mach-highbank/highbank.c
+++ b/arch/arm/mach-highbank/highbank.c
@@ -18,14 +18,11 @@
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
-#include <linux/irq.h>
 #include <linux/irqchip.h>
-#include <linux/irqdomain.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/of_address.h>
-#include <linux/smp.h>
 #include <linux/amba/bus.h>
 #include <linux/clk-provider.h>
 
@@ -35,7 +32,6 @@
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
-#include <asm/mach/time.h>
 
 #include "core.h"
 #include "sysregs.h"
-- 
1.8.1.2

[PATCH 6/7] ARM: highbank: adapt to use ARM PSCI calls

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

This adapts highbank to use the psci_smp_ops. Additionally,
suspend/resume, reset and poweroff are all converted to use PSCI calls.
Doing this removes direct access to the SCU and factors out the A9 vs.
A15 differences.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/mach-highbank/Kconfig    |  2 +-
 arch/arm/mach-highbank/Makefile   |  4 +-
 arch/arm/mach-highbank/core.h     |  9 ----
 arch/arm/mach-highbank/highbank.c | 51 +----------------------
 arch/arm/mach-highbank/hotplug.c  | 37 -----------------
 arch/arm/mach-highbank/platsmp.c  | 68 -------------------------------
 arch/arm/mach-highbank/pm.c       | 27 ++++--------
 arch/arm/mach-highbank/sysregs.h  | 86 ---------------------------------------
 arch/arm/mach-highbank/system.c   | 33 ---------------
 9 files changed, 12 insertions(+), 305 deletions(-)
 delete mode 100644 arch/arm/mach-highbank/hotplug.c
 delete mode 100644 arch/arm/mach-highbank/platsmp.c
 delete mode 100644 arch/arm/mach-highbank/sysregs.h
 delete mode 100644 arch/arm/mach-highbank/system.c

diff --git a/arch/arm/mach-highbank/Kconfig b/arch/arm/mach-highbank/Kconfig
index cd9fcb1..9c61924 100644
--- a/arch/arm/mach-highbank/Kconfig
+++ b/arch/arm/mach-highbank/Kconfig
@@ -5,13 +5,13 @@ config ARCH_HIGHBANK
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select ARM_AMBA
 	select ARM_GIC
+	select ARM_PSCI
 	select ARM_TIMER_SP804
 	select CACHE_L2X0
 	select CLKDEV_LOOKUP
 	select COMMON_CLK
 	select CPU_V7
 	select GENERIC_CLOCKEVENTS
-	select HAVE_ARM_SCU
 	select HAVE_ARM_TWD if LOCAL_TIMERS
 	select HAVE_SMP
 	select MAILBOX
diff --git a/arch/arm/mach-highbank/Makefile b/arch/arm/mach-highbank/Makefile
index 8a1ef57..aeabc32 100644
--- a/arch/arm/mach-highbank/Makefile
+++ b/arch/arm/mach-highbank/Makefile
@@ -1,8 +1,6 @@
-obj-y					:= highbank.o system.o smc.o
+obj-y					:= highbank.o smc.o
 
 plus_sec := $(call as-instr,.arch_extension sec,+sec)
 AFLAGS_smc.o				:=-Wa,-march=armv7-a$(plus_sec)
 
-obj-$(CONFIG_SMP)			+= platsmp.o
-obj-$(CONFIG_HOTPLUG_CPU)		+= hotplug.o
 obj-$(CONFIG_PM_SLEEP)			+= pm.o
diff --git a/arch/arm/mach-highbank/core.h b/arch/arm/mach-highbank/core.h
index aea1ec5..52b713a 100644
--- a/arch/arm/mach-highbank/core.h
+++ b/arch/arm/mach-highbank/core.h
@@ -1,12 +1,6 @@
 #ifndef __HIGHBANK_CORE_H
 #define __HIGHBANK_CORE_H
 
-#include <linux/reboot.h>
-
-extern void highbank_set_cpu_jump(int cpu, void *jump_addr);
-extern void highbank_restart(enum reboot_mode, const char *);
-extern void __iomem *scu_base_addr;
-
 #ifdef CONFIG_PM_SLEEP
 extern void highbank_pm_init(void);
 #else
@@ -14,8 +8,5 @@ static inline void highbank_pm_init(void) {}
 #endif
 
 extern void highbank_smc1(int fn, int arg);
-extern void highbank_cpu_die(unsigned int cpu);
-
-extern struct smp_operations highbank_smp_ops;
 
 #endif
diff --git a/arch/arm/mach-highbank/highbank.c b/arch/arm/mach-highbank/highbank.c
index 0f5c661..fabc5dd 100644
--- a/arch/arm/mach-highbank/highbank.c
+++ b/arch/arm/mach-highbank/highbank.c
@@ -26,61 +26,23 @@
 #include <linux/amba/bus.h>
 #include <linux/clk-provider.h>
 
-#include <asm/cacheflush.h>
-#include <asm/cputype.h>
-#include <asm/smp_plat.h>
+#include <asm/psci.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
 
 #include "core.h"
-#include "sysregs.h"
 
 void __iomem *sregs_base;
-void __iomem *scu_base_addr;
-
-static void __init highbank_scu_map_io(void)
-{
-	unsigned long base;
-
-	/* Get SCU base */
-	asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (base));
-
-	scu_base_addr = ioremap(base, SZ_4K);
-}
-
-#define HB_JUMP_TABLE_PHYS(cpu)		(0x40 + (0x10 * (cpu)))
-#define HB_JUMP_TABLE_VIRT(cpu)		phys_to_virt(HB_JUMP_TABLE_PHYS(cpu))
-
-void highbank_set_cpu_jump(int cpu, void *jump_addr)
-{
-	cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(cpu), 0);
-	writel(virt_to_phys(jump_addr), HB_JUMP_TABLE_VIRT(cpu));
-	__cpuc_flush_dcache_area(HB_JUMP_TABLE_VIRT(cpu), 16);
-	outer_clean_range(HB_JUMP_TABLE_PHYS(cpu),
-			  HB_JUMP_TABLE_PHYS(cpu) + 15);
-}
-
-#ifdef CONFIG_CACHE_L2X0
-static void highbank_l2x0_disable(void)
-{
-	/* Disable PL310 L2 Cache controller */
-	highbank_smc1(0x102, 0x0);
-}
-#endif
 
 static void __init highbank_init_irq(void)
 {
 	irqchip_init();
 
-	if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
-		highbank_scu_map_io();
-
 #ifdef CONFIG_CACHE_L2X0
 	/* Enable PL310 L2 Cache controller */
 	highbank_smc1(0x102, 0x1);
 	l2x0_of_init(0, ~0UL);
-	outer_cache.disable = highbank_l2x0_disable;
 #endif
 }
 
@@ -98,14 +60,6 @@ static void __init highbank_timer_init(void)
 	clocksource_of_init();
 }
 
-static void highbank_power_off(void)
-{
-	highbank_set_pwr_shutdown();
-
-	while (1)
-		cpu_do_idle();
-}
-
 static int highbank_platform_notifier(struct notifier_block *nb,
 				  unsigned long event, void *__dev)
 {
@@ -155,7 +109,6 @@ static struct notifier_block highbank_platform_nb = {
 
 static void __init highbank_init(void)
 {
-	pm_power_off = highbank_power_off;
 	highbank_pm_init();
 
 	bus_register_notifier(&platform_bus_type, &highbank_platform_nb);
@@ -171,10 +124,8 @@ static const char *highbank_match[] __initconst = {
 };
 
 DT_MACHINE_START(HIGHBANK, "Highbank")
-	.smp		= smp_ops(highbank_smp_ops),
 	.init_irq	= highbank_init_irq,
 	.init_time	= highbank_timer_init,
 	.init_machine	= highbank_init,
 	.dt_compat	= highbank_match,
-	.restart	= highbank_restart,
 MACHINE_END
diff --git a/arch/arm/mach-highbank/hotplug.c b/arch/arm/mach-highbank/hotplug.c
deleted file mode 100644
index a019e4e..0000000
--- a/arch/arm/mach-highbank/hotplug.c
+++ /dev/null
@@ -1,37 +0,0 @@
-/*
- * Copyright 2011 Calxeda, Inc.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#include <linux/kernel.h>
-#include <asm/cacheflush.h>
-
-#include "core.h"
-#include "sysregs.h"
-
-extern void secondary_startup(void);
-
-/*
- * platform-specific code to shutdown a CPU
- *
- */
-void __ref highbank_cpu_die(unsigned int cpu)
-{
-	highbank_set_cpu_jump(cpu, phys_to_virt(0));
-
-	flush_cache_louis();
-	highbank_set_core_pwr();
-
-	while (1)
-		cpu_do_idle();
-}
diff --git a/arch/arm/mach-highbank/platsmp.c b/arch/arm/mach-highbank/platsmp.c
deleted file mode 100644
index 32d75cf5..0000000
--- a/arch/arm/mach-highbank/platsmp.c
+++ /dev/null
@@ -1,68 +0,0 @@
-/*
- * Copyright 2010-2011 Calxeda, Inc.
- * Based on platsmp.c, Copyright (C) 2002 ARM Ltd.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#include <linux/init.h>
-#include <linux/smp.h>
-#include <linux/io.h>
-
-#include <asm/smp_scu.h>
-
-#include "core.h"
-
-extern void secondary_startup(void);
-
-static int highbank_boot_secondary(unsigned int cpu, struct task_struct *idle)
-{
-	highbank_set_cpu_jump(cpu, secondary_startup);
-	arch_send_wakeup_ipi_mask(cpumask_of(cpu));
-	return 0;
-}
-
-/*
- * Initialise the CPU possible map early - this describes the CPUs
- * which may be present or become present in the system.
- */
-static void __init highbank_smp_init_cpus(void)
-{
-	unsigned int i, ncores = 4;
-
-	/* sanity check */
-	if (ncores > NR_CPUS) {
-		printk(KERN_WARNING
-		       "highbank: no. of cores (%d) greater than configured "
-		       "maximum of %d - clipping\n",
-		       ncores, NR_CPUS);
-		ncores = NR_CPUS;
-	}
-
-	for (i = 0; i < ncores; i++)
-		set_cpu_possible(i, true);
-}
-
-static void __init highbank_smp_prepare_cpus(unsigned int max_cpus)
-{
-	if (scu_base_addr)
-		scu_enable(scu_base_addr);
-}
-
-struct smp_operations highbank_smp_ops __initdata = {
-	.smp_init_cpus		= highbank_smp_init_cpus,
-	.smp_prepare_cpus	= highbank_smp_prepare_cpus,
-	.smp_boot_secondary	= highbank_boot_secondary,
-#ifdef CONFIG_HOTPLUG_CPU
-	.cpu_die		= highbank_cpu_die,
-#endif
-};
diff --git a/arch/arm/mach-highbank/pm.c b/arch/arm/mach-highbank/pm.c
index 04eddb4..7f2bd85 100644
--- a/arch/arm/mach-highbank/pm.c
+++ b/arch/arm/mach-highbank/pm.c
@@ -16,27 +16,19 @@
 
 #include <linux/cpu_pm.h>
 #include <linux/init.h>
-#include <linux/io.h>
 #include <linux/suspend.h>
 
-#include <asm/cacheflush.h>
-#include <asm/proc-fns.h>
 #include <asm/suspend.h>
-
-#include "core.h"
-#include "sysregs.h"
+#include <asm/psci.h>
 
 static int highbank_suspend_finish(unsigned long val)
 {
-	outer_flush_all();
-	outer_disable();
-
-	highbank_set_pwr_suspend();
-
-	cpu_do_idle();
+	const struct psci_power_state ps = {
+		.type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
+		.affinity_level = 1,
+	};
 
-	highbank_clear_pwr_request();
-	return 0;
+	return psci_ops.cpu_suspend(ps, __pa(cpu_resume));
 }
 
 static int highbank_pm_enter(suspend_state_t state)
@@ -44,15 +36,11 @@ static int highbank_pm_enter(suspend_state_t state)
 	cpu_pm_enter();
 	cpu_cluster_pm_enter();
 
-	highbank_set_cpu_jump(0, cpu_resume);
 	cpu_suspend(0, highbank_suspend_finish);
 
 	cpu_cluster_pm_exit();
 	cpu_pm_exit();
 
-	highbank_smc1(0x102, 0x1);
-	if (scu_base_addr)
-		scu_enable(scu_base_addr);
 	return 0;
 }
 
@@ -63,5 +51,8 @@ static const struct platform_suspend_ops highbank_pm_ops = {
 
 void __init highbank_pm_init(void)
 {
+	if (!psci_ops.cpu_suspend)
+		return;
+
 	suspend_set_ops(&highbank_pm_ops);
 }
diff --git a/arch/arm/mach-highbank/sysregs.h b/arch/arm/mach-highbank/sysregs.h
deleted file mode 100644
index 5995df7..0000000
--- a/arch/arm/mach-highbank/sysregs.h
+++ /dev/null
@@ -1,86 +0,0 @@
-/*
- * Copyright 2011 Calxeda, Inc.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#ifndef _MACH_HIGHBANK__SYSREGS_H_
-#define _MACH_HIGHBANK__SYSREGS_H_
-
-#include <linux/io.h>
-#include <linux/smp.h>
-#include <asm/smp_plat.h>
-#include <asm/smp_scu.h>
-#include "core.h"
-
-extern void __iomem *sregs_base;
-
-#define HB_SREG_A9_PWR_REQ		0xf00
-#define HB_SREG_A9_BOOT_STAT		0xf04
-#define HB_SREG_A9_BOOT_DATA		0xf08
-
-#define HB_PWR_SUSPEND			0
-#define HB_PWR_SOFT_RESET		1
-#define HB_PWR_HARD_RESET		2
-#define HB_PWR_SHUTDOWN			3
-
-#define SREG_CPU_PWR_CTRL(c)		(0x200 + ((c) * 4))
-
-static inline void highbank_set_core_pwr(void)
-{
-	int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
-	if (scu_base_addr)
-		scu_power_mode(scu_base_addr, SCU_PM_POWEROFF);
-	else
-		writel_relaxed(1, sregs_base + SREG_CPU_PWR_CTRL(cpu));
-}
-
-static inline void highbank_clear_core_pwr(void)
-{
-	int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
-	if (scu_base_addr)
-		scu_power_mode(scu_base_addr, SCU_PM_NORMAL);
-	else
-		writel_relaxed(0, sregs_base + SREG_CPU_PWR_CTRL(cpu));
-}
-
-static inline void highbank_set_pwr_suspend(void)
-{
-	writel(HB_PWR_SUSPEND, sregs_base + HB_SREG_A9_PWR_REQ);
-	highbank_set_core_pwr();
-}
-
-static inline void highbank_set_pwr_shutdown(void)
-{
-	writel(HB_PWR_SHUTDOWN, sregs_base + HB_SREG_A9_PWR_REQ);
-	highbank_set_core_pwr();
-}
-
-static inline void highbank_set_pwr_soft_reset(void)
-{
-	writel(HB_PWR_SOFT_RESET, sregs_base + HB_SREG_A9_PWR_REQ);
-	highbank_set_core_pwr();
-}
-
-static inline void highbank_set_pwr_hard_reset(void)
-{
-	writel(HB_PWR_HARD_RESET, sregs_base + HB_SREG_A9_PWR_REQ);
-	highbank_set_core_pwr();
-}
-
-static inline void highbank_clear_pwr_request(void)
-{
-	writel(~0UL, sregs_base + HB_SREG_A9_PWR_REQ);
-	highbank_clear_core_pwr();
-}
-
-#endif
diff --git a/arch/arm/mach-highbank/system.c b/arch/arm/mach-highbank/system.c
deleted file mode 100644
index 2df5870..0000000
--- a/arch/arm/mach-highbank/system.c
+++ /dev/null
@@ -1,33 +0,0 @@
-/*
- * Copyright 2011 Calxeda, Inc.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License along with
- * this program.  If not, see <http://www.gnu.org/licenses/>.
- */
-#include <linux/io.h>
-#include <asm/proc-fns.h>
-#include <linux/reboot.h>
-
-#include "core.h"
-#include "sysregs.h"
-
-void highbank_restart(enum reboot_mode mode, const char *cmd)
-{
-	if (mode == REBOOT_HARD)
-		highbank_set_pwr_hard_reset();
-	else
-		highbank_set_pwr_soft_reset();
-
-	while (1)
-		cpu_do_idle();
-}
-
-- 
1.8.1.2

[PATCH 7/7] dts: calxeda: add ARM PSCI binding

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Add the PSCI binding for Calxeda SOCs. The PSCI function numbers are
different from the DT binding example because the numbering changed in
revisions of the PSCI spec and are already fixed in highbank firmware.
Since the numbering is transparent to the kernel, this difference is not
significant.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/boot/dts/ecx-common.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index e8559b7..f1dfc09 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -19,6 +19,16 @@
 		bootargs = "console=ttyAMA0";
 	};
 
+	psci {
+		compatible	= "arm,psci";
+		method		= "smc";
+		cpu_suspend	= <0x84000002>;
+		cpu_off		= <0x84000004>;
+		cpu_on		= <0x84000006>;
+		system_off	= <0x84000100>;
+		system_reset	= <0x84000101>;
+	};
+
 	soc {
 		#address-cells = <1>;
 		#size-cells = <1>;
-- 
1.8.1.2

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Mark Rutland]

Hi Rob,

On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> The PSCI spec from ARM has been updated to 0.2 version. Update the
> binding document to reflect the spec changes. For the binding, the
> major changes are addition of system reset and poweroff functions.
> The recommended function id numbering has also changed.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Cc: devicetree at vger.kernel.org
> ---
>  Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> index 433afe9..b8b4d9f 100644
> --- a/Documentation/devicetree/bindings/arm/psci.txt
> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> @@ -21,7 +21,7 @@ to #0.
>  
>  Main node required properties:
>  
> - - compatible    : Must be "arm,psci"
> + - compatible    : Must be "arm,psci-0.2" or "arm,psci"

For the purposes of handling different firmware implementations (which
may have different bugs to work around and may require different
arguments to cpu_suspend), it may be better to state "must contain"
rather than "must be". 

>  
>   - method        : The method of calling the PSCI firmware. Permitted
>                     values are:
> @@ -32,6 +32,9 @@ Main node required properties:
>                     "hvc" : HVC #0, with the register assignments specified
>  		           in this binding.
>  
> + - psci_version  : Function ID for PSCI_VERSION operation. Required for
> +                   "arm,psci-0.2" compatible version or later.
> +
>  Main node optional properties:
>  
>   - cpu_suspend   : Function ID for CPU_SUSPEND operation

The low bits of CPU_SUSPEND's power_state argument are platform specific
and we'll need to deal with them if an implementation actually uses them
for something. We can probably associate this with a specific
implementation's compatible string, as we'll almost certainly need
special code to handle the differences between them.

If we have a compatible string for the first platform that ignores the
argument (or just uses zero), that can be shared by all those
implementations that don't give any fine-grained control here.

> @@ -42,14 +45,24 @@ Main node optional properties:
>  
>   - migrate       : Function ID for MIGRATE operation
>  
> + - system_reset  : Function ID for SYSTEM_RESET operation
> +
> + - system_off    : Function ID for SYSTEM_OFF operation
> +
>  
>  Example:
>  
>  	psci {
> -		compatible	= "arm,psci";
> +		compatible	= "arm,psci-0.2";
>  		method		= "smc";
> -		cpu_suspend	= <0x95c10000>;
> -		cpu_off		= <0x95c10001>;
> -		cpu_on		= <0x95c10002>;
> -		migrate		= <0x95c10003>;
> +		psci_version	= <0x84000000>;
> +		cpu_suspend	= <0x84000001>;
> +		cpu_off		= <0x84000002>;
> +		cpu_on		= <0x84000003>;
> +		affinity_info	= <0x84000004>; 
> +		migrate		= <0x84000005>;
> +		migrate_info_type = <0x84000006>; 
> +		migrate_info_up_cpu = <0x84000007>; 
> +		system_off	= <0x84000008>; 
> +		system_reset	= <0x84000009>; 
>  	};

One of the things changed in PSCI 0.2 was the SMC calling convention,
though this isn't clear in the PSCI document. The function IDs for 32bit
and 64bit callers may differ, and we need to support describing an
arbitrary configuration of the two (same ID for both, different across
32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).

I'd like to ensure the binding can deal with that from the start. We
could do this by having -32 and -64 variants of each function id (e.g.
cpu_off-64) , if the IDs actually differ, and use the regular combined
ID if they don't.

Thanks,
Mark.

[PATCH 7/7] dts: calxeda: add ARM PSCI binding

[Mark Rutland]

On Sun, Jul 28, 2013 at 10:56:38PM +0100, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Add the PSCI binding for Calxeda SOCs. The PSCI function numbers are
> different from the DT binding example because the numbering changed in
> revisions of the PSCI spec and are already fixed in highbank firmware.
> Since the numbering is transparent to the kernel, this difference is not
> significant.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
>  arch/arm/boot/dts/ecx-common.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
> index e8559b7..f1dfc09 100644
> --- a/arch/arm/boot/dts/ecx-common.dtsi
> +++ b/arch/arm/boot/dts/ecx-common.dtsi
> @@ -19,6 +19,16 @@
>  		bootargs = "console=ttyAMA0";
>  	};
>  
> +	psci {
> +		compatible	= "arm,psci";
> +		method		= "smc";
> +		cpu_suspend	= <0x84000002>;
> +		cpu_off		= <0x84000004>;
> +		cpu_on		= <0x84000006>;
> +		system_off	= <0x84000100>;
> +		system_reset	= <0x84000101>;
> +	};
> +

I see this is somewhat beyond the original PSCI, given the addition of
SYSTEM_OFF and SYSTEM_RESET, but not quite PSCI 0.2 as you don't have
PSCI_VERSION.

It would be nice it the compatible string were more specific here to
deal with this and any implementation-specific power_state ID values.
Something like "calxeda,highbank-psci", "arm,psci" ?

Thanks,
Mark.

[PATCH 7/7] dts: calxeda: add ARM PSCI binding

[Rob Herring]

On 07/29/2013 05:24 AM, Mark Rutland wrote:
> On Sun, Jul 28, 2013 at 10:56:38PM +0100, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> Add the PSCI binding for Calxeda SOCs. The PSCI function numbers are
>> different from the DT binding example because the numbering changed in
>> revisions of the PSCI spec and are already fixed in highbank firmware.
>> Since the numbering is transparent to the kernel, this difference is not
>> significant.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> ---
>>  arch/arm/boot/dts/ecx-common.dtsi | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
>> index e8559b7..f1dfc09 100644
>> --- a/arch/arm/boot/dts/ecx-common.dtsi
>> +++ b/arch/arm/boot/dts/ecx-common.dtsi
>> @@ -19,6 +19,16 @@
>>  		bootargs = "console=ttyAMA0";
>>  	};
>>  
>> +	psci {
>> +		compatible	= "arm,psci";
>> +		method		= "smc";
>> +		cpu_suspend	= <0x84000002>;
>> +		cpu_off		= <0x84000004>;
>> +		cpu_on		= <0x84000006>;
>> +		system_off	= <0x84000100>;
>> +		system_reset	= <0x84000101>;
>> +	};
>> +
> 
> I see this is somewhat beyond the original PSCI, given the addition of
> SYSTEM_OFF and SYSTEM_RESET, but not quite PSCI 0.2 as you don't have
> PSCI_VERSION.

I do actually support PSCI_VERSION. Forgot to add that to the dts.

> It would be nice it the compatible string were more specific here to
> deal with this and any implementation-specific power_state ID values.
> Something like "calxeda,highbank-psci", "arm,psci" ?

Is that something we always want to do? You can't really know the
implementation quirks up front.

Rob

Re: [PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Daniel Lezcano]

On 07/28/2013 11:56 PM, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> This updates the Calxeda cpuidle driver to use PSCI calls to powergate
> cores. This is needed to enable cpuidle for the ECX-2000.
> 
> This could possibly become a generic PSCI driver, but there are no other
> PSCI users in the kernel other than mach-virt.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Rob,

Shall I take the patch in my tree ?

Thanks
  -- Daniel


>  drivers/cpuidle/cpuidle-calxeda.c | 40 ++++++---------------------------------
>  1 file changed, 6 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c
> index 0e6e408..01cfecf3 100644
> --- a/drivers/cpuidle/cpuidle-calxeda.c
> +++ b/drivers/cpuidle/cpuidle-calxeda.c
> @@ -22,51 +22,23 @@
>  
>  #include <linux/cpuidle.h>
>  #include <linux/init.h>
> -#include <linux/io.h>
>  #include <linux/of.h>
> -#include <linux/time.h>
> -#include <linux/delay.h>
> -#include <linux/suspend.h>
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
> -#include <asm/smp_scu.h>
>  #include <asm/suspend.h>
> -#include <asm/cacheflush.h>
> -#include <asm/cp15.h>
> -
> -extern void highbank_set_cpu_jump(int cpu, void *jump_addr);
> -extern void *scu_base_addr;
> -
> -static noinline void calxeda_idle_restore(void)
> -{
> -	set_cr(get_cr() | CR_C);
> -	set_auxcr(get_auxcr() | 0x40);
> -	scu_power_mode(scu_base_addr, SCU_PM_NORMAL);
> -}
> +#include <asm/psci.h>
>  
>  static int calxeda_idle_finish(unsigned long val)
>  {
> -	/* Already flushed cache, but do it again as the outer cache functions
> -	 * dirty the cache with spinlocks */
> -	flush_cache_all();
> -
> -	set_auxcr(get_auxcr() & ~0x40);
> -	set_cr(get_cr() & ~CR_C);
> -
> -	scu_power_mode(scu_base_addr, SCU_PM_DORMANT);
> -
> -	cpu_do_idle();
> -
> -	/* Restore things if we didn't enter power-gating */
> -	calxeda_idle_restore();
> -	return 1;
> +	const struct psci_power_state ps = {
> +		.type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
> +	};
> +	return psci_ops.cpu_suspend(ps, __pa(cpu_resume));
>  }
>  
>  static int calxeda_pwrdown_idle(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv,
>  				int index)
>  {
> -	highbank_set_cpu_jump(smp_processor_id(), cpu_resume);
>  	cpu_suspend(0, calxeda_idle_finish);
>  	return index;
>  }
> @@ -90,7 +62,7 @@ static struct cpuidle_driver calxeda_idle_driver = {
>  
>  static int __init calxeda_cpuidle_init(void)
>  {
> -	if (!of_machine_is_compatible("calxeda,highbank"))
> +	if (!of_machine_is_compatible("calxeda,highbank") || !psci_ops.cpu_suspend)
>  		return -ENODEV;
>  
>  	return cpuidle_register(&calxeda_idle_driver, NULL);
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Daniel Lezcano]

On 07/28/2013 11:56 PM, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> This updates the Calxeda cpuidle driver to use PSCI calls to powergate
> cores. This is needed to enable cpuidle for the ECX-2000.
> 
> This could possibly become a generic PSCI driver, but there are no other
> PSCI users in the kernel other than mach-virt.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm at vger.kernel.org
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Rob,

Shall I take the patch in my tree ?

Thanks
  -- Daniel


>  drivers/cpuidle/cpuidle-calxeda.c | 40 ++++++---------------------------------
>  1 file changed, 6 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-calxeda.c b/drivers/cpuidle/cpuidle-calxeda.c
> index 0e6e408..01cfecf3 100644
> --- a/drivers/cpuidle/cpuidle-calxeda.c
> +++ b/drivers/cpuidle/cpuidle-calxeda.c
> @@ -22,51 +22,23 @@
>  
>  #include <linux/cpuidle.h>
>  #include <linux/init.h>
> -#include <linux/io.h>
>  #include <linux/of.h>
> -#include <linux/time.h>
> -#include <linux/delay.h>
> -#include <linux/suspend.h>
>  #include <asm/cpuidle.h>
> -#include <asm/proc-fns.h>
> -#include <asm/smp_scu.h>
>  #include <asm/suspend.h>
> -#include <asm/cacheflush.h>
> -#include <asm/cp15.h>
> -
> -extern void highbank_set_cpu_jump(int cpu, void *jump_addr);
> -extern void *scu_base_addr;
> -
> -static noinline void calxeda_idle_restore(void)
> -{
> -	set_cr(get_cr() | CR_C);
> -	set_auxcr(get_auxcr() | 0x40);
> -	scu_power_mode(scu_base_addr, SCU_PM_NORMAL);
> -}
> +#include <asm/psci.h>
>  
>  static int calxeda_idle_finish(unsigned long val)
>  {
> -	/* Already flushed cache, but do it again as the outer cache functions
> -	 * dirty the cache with spinlocks */
> -	flush_cache_all();
> -
> -	set_auxcr(get_auxcr() & ~0x40);
> -	set_cr(get_cr() & ~CR_C);
> -
> -	scu_power_mode(scu_base_addr, SCU_PM_DORMANT);
> -
> -	cpu_do_idle();
> -
> -	/* Restore things if we didn't enter power-gating */
> -	calxeda_idle_restore();
> -	return 1;
> +	const struct psci_power_state ps = {
> +		.type = PSCI_POWER_STATE_TYPE_POWER_DOWN,
> +	};
> +	return psci_ops.cpu_suspend(ps, __pa(cpu_resume));
>  }
>  
>  static int calxeda_pwrdown_idle(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv,
>  				int index)
>  {
> -	highbank_set_cpu_jump(smp_processor_id(), cpu_resume);
>  	cpu_suspend(0, calxeda_idle_finish);
>  	return index;
>  }
> @@ -90,7 +62,7 @@ static struct cpuidle_driver calxeda_idle_driver = {
>  
>  static int __init calxeda_cpuidle_init(void)
>  {
> -	if (!of_machine_is_compatible("calxeda,highbank"))
> +	if (!of_machine_is_compatible("calxeda,highbank") || !psci_ops.cpu_suspend)
>  		return -ENODEV;
>  
>  	return cpuidle_register(&calxeda_idle_driver, NULL);
> 


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 7/7] dts: calxeda: add ARM PSCI binding

[Mark Rutland]

On Mon, Jul 29, 2013 at 02:13:22PM +0100, Rob Herring wrote:
> On 07/29/2013 05:24 AM, Mark Rutland wrote:
> > On Sun, Jul 28, 2013 at 10:56:38PM +0100, Rob Herring wrote:
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> Add the PSCI binding for Calxeda SOCs. The PSCI function numbers are
> >> different from the DT binding example because the numbering changed in
> >> revisions of the PSCI spec and are already fixed in highbank firmware.
> >> Since the numbering is transparent to the kernel, this difference is not
> >> significant.
> >>
> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> >> ---
> >>  arch/arm/boot/dts/ecx-common.dtsi | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
> >> index e8559b7..f1dfc09 100644
> >> --- a/arch/arm/boot/dts/ecx-common.dtsi
> >> +++ b/arch/arm/boot/dts/ecx-common.dtsi
> >> @@ -19,6 +19,16 @@
> >>  		bootargs = "console=ttyAMA0";
> >>  	};
> >>  
> >> +	psci {
> >> +		compatible	= "arm,psci";
> >> +		method		= "smc";
> >> +		cpu_suspend	= <0x84000002>;
> >> +		cpu_off		= <0x84000004>;
> >> +		cpu_on		= <0x84000006>;
> >> +		system_off	= <0x84000100>;
> >> +		system_reset	= <0x84000101>;
> >> +	};
> >> +
> > 
> > I see this is somewhat beyond the original PSCI, given the addition of
> > SYSTEM_OFF and SYSTEM_RESET, but not quite PSCI 0.2 as you don't have
> > PSCI_VERSION.
> 
> I do actually support PSCI_VERSION. Forgot to add that to the dts.

Ah, ok. Can you upgrade the compatible string to include "arm,psci-0.2"
while you're at it?

> 
> > It would be nice it the compatible string were more specific here to
> > deal with this and any implementation-specific power_state ID values.
> > Something like "calxeda,highbank-psci", "arm,psci" ?
> 
> Is that something we always want to do? You can't really know the
> implementation quirks up front.

While we might not know the quirks up front, it'll mean that when we
discover one we have a greater chance of being able to find out the
presence of the quirk from an old dts.

Thanks,
Mark.

Re: [PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Rob Herring]

On 07/29/2013 09:14 AM, Daniel Lezcano wrote:
> On 07/28/2013 11:56 PM, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> This updates the Calxeda cpuidle driver to use PSCI calls to powergate
>> cores. This is needed to enable cpuidle for the ECX-2000.
>>
>> This could possibly become a generic PSCI driver, but there are no other
>> PSCI users in the kernel other than mach-virt.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: linux-pm@vger.kernel.org
>> ---
> 
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks.

> 
> Rob,
> 
> Shall I take the patch in my tree ?

No, there are interdependencies with the rest of the series, so I plan
to send to arm-soc.

Rob

[PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Rob Herring]

On 07/29/2013 09:14 AM, Daniel Lezcano wrote:
> On 07/28/2013 11:56 PM, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> This updates the Calxeda cpuidle driver to use PSCI calls to powergate
>> cores. This is needed to enable cpuidle for the ECX-2000.
>>
>> This could possibly become a generic PSCI driver, but there are no other
>> PSCI users in the kernel other than mach-virt.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: linux-pm at vger.kernel.org
>> ---
> 
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Thanks.

> 
> Rob,
> 
> Shall I take the patch in my tree ?

No, there are interdependencies with the rest of the series, so I plan
to send to arm-soc.

Rob

Re: [PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Daniel Lezcano]

On 07/29/2013 04:39 PM, Rob Herring wrote:
> On 07/29/2013 09:14 AM, Daniel Lezcano wrote:
>> On 07/28/2013 11:56 PM, Rob Herring wrote:
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> This updates the Calxeda cpuidle driver to use PSCI calls to powergate
>>> cores. This is needed to enable cpuidle for the ECX-2000.
>>>
>>> This could possibly become a generic PSCI driver, but there are no other
>>> PSCI users in the kernel other than mach-virt.
>>>
>>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: linux-pm@vger.kernel.org
>>> ---
>>
>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Thanks.
> 
>>
>> Rob,
>>
>> Shall I take the patch in my tree ?
> 
> No, there are interdependencies with the rest of the series, so I plan
> to send to arm-soc.

Ok, thanks.


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 4/7] cpuidle: calxeda: add support to use PSCI calls

[Daniel Lezcano]

On 07/29/2013 04:39 PM, Rob Herring wrote:
> On 07/29/2013 09:14 AM, Daniel Lezcano wrote:
>> On 07/28/2013 11:56 PM, Rob Herring wrote:
>>> From: Rob Herring <rob.herring@calxeda.com>
>>>
>>> This updates the Calxeda cpuidle driver to use PSCI calls to powergate
>>> cores. This is needed to enable cpuidle for the ECX-2000.
>>>
>>> This could possibly become a generic PSCI driver, but there are no other
>>> PSCI users in the kernel other than mach-virt.
>>>
>>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: linux-pm at vger.kernel.org
>>> ---
>>
>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Thanks.
> 
>>
>> Rob,
>>
>> Shall I take the patch in my tree ?
> 
> No, there are interdependencies with the rest of the series, so I plan
> to send to arm-soc.

Ok, thanks.


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Rob Herring]

On 07/29/2013 05:13 AM, Mark Rutland wrote:
> Hi Rob,
> 
> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> The PSCI spec from ARM has been updated to 0.2 version. Update the
>> binding document to reflect the spec changes. For the binding, the
>> major changes are addition of system reset and poweroff functions.
>> The recommended function id numbering has also changed.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> Cc: devicetree at vger.kernel.org
>> ---
>>  Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
>> index 433afe9..b8b4d9f 100644
>> --- a/Documentation/devicetree/bindings/arm/psci.txt
>> +++ b/Documentation/devicetree/bindings/arm/psci.txt
>> @@ -21,7 +21,7 @@ to #0.
>>  
>>  Main node required properties:
>>  
>> - - compatible    : Must be "arm,psci"
>> + - compatible    : Must be "arm,psci-0.2" or "arm,psci"
> 
> For the purposes of handling different firmware implementations (which
> may have different bugs to work around and may require different
> arguments to cpu_suspend), it may be better to state "must contain"
> rather than "must be". 
> 
>>  
>>   - method        : The method of calling the PSCI firmware. Permitted
>>                     values are:
>> @@ -32,6 +32,9 @@ Main node required properties:
>>                     "hvc" : HVC #0, with the register assignments specified
>>  		           in this binding.
>>  
>> + - psci_version  : Function ID for PSCI_VERSION operation. Required for
>> +                   "arm,psci-0.2" compatible version or later.
>> +
>>  Main node optional properties:
>>  
>>   - cpu_suspend   : Function ID for CPU_SUSPEND operation
> 
> The low bits of CPU_SUSPEND's power_state argument are platform specific
> and we'll need to deal with them if an implementation actually uses them
> for something. We can probably associate this with a specific
> implementation's compatible string, as we'll almost certainly need
> special code to handle the differences between them.

I'm not a fan of

> If we have a compatible string for the first platform that ignores the
> argument (or just uses zero), that can be shared by all those
> implementations that don't give any fine-grained control here.
> 
>> @@ -42,14 +45,24 @@ Main node optional properties:
>>  
>>   - migrate       : Function ID for MIGRATE operation
>>  
>> + - system_reset  : Function ID for SYSTEM_RESET operation
>> +
>> + - system_off    : Function ID for SYSTEM_OFF operation
>> +
>>  
>>  Example:
>>  
>>  	psci {
>> -		compatible	= "arm,psci";
>> +		compatible	= "arm,psci-0.2";
>>  		method		= "smc";
>> -		cpu_suspend	= <0x95c10000>;
>> -		cpu_off		= <0x95c10001>;
>> -		cpu_on		= <0x95c10002>;
>> -		migrate		= <0x95c10003>;
>> +		psci_version	= <0x84000000>;
>> +		cpu_suspend	= <0x84000001>;
>> +		cpu_off		= <0x84000002>;
>> +		cpu_on		= <0x84000003>;
>> +		affinity_info	= <0x84000004>; 
>> +		migrate		= <0x84000005>;
>> +		migrate_info_type = <0x84000006>; 
>> +		migrate_info_up_cpu = <0x84000007>; 
>> +		system_off	= <0x84000008>; 
>> +		system_reset	= <0x84000009>; 
>>  	};
> 
> One of the things changed in PSCI 0.2 was the SMC calling convention,
> though this isn't clear in the PSCI document. The function IDs for 32bit
> and 64bit callers may differ, and we need to support describing an
> arbitrary configuration of the two (same ID for both, different across
> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> 
> I'd like to ensure the binding can deal with that from the start. We
> could do this by having -32 and -64 variants of each function id (e.g.
> cpu_off-64) , if the IDs actually differ, and use the regular combined
> ID if they don't.

Uggg. I guess I should have read the SMC calling convention doc... I was
simply documenting what is already in the PSCI doc, but obviously that
is not fully flushed out.

How about something like this (for the complicated case of both 32 and
64 bit):

	method		= "smc", "smc64";
	psci_version	= <0x84000000 0xc4000000>;
	cpu_suspend	= <0x84000001 0xc4000001>;
	cpu_off		= <0x84000002 0xc4000002>;
	cpu_on		= <0x84000003 0xc4000003>;

"smc" is a synonym for smc32 for compatibility. The number and order of
methods determines the number and order of function IDs.

A variation on this would be keep method as is and add a "#psci-cells"
property to specify the number of function IDs. You can determine the
64-bit vs. 32-bit support based on the function ID itself.

Rob

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Mark Rutland]

On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> > Hi Rob,
> > 
> > On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> The PSCI spec from ARM has been updated to 0.2 version. Update the
> >> binding document to reflect the spec changes. For the binding, the
> >> major changes are addition of system reset and poweroff functions.
> >> The recommended function id numbering has also changed.
> >>
> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> >> Cc: devicetree at vger.kernel.org
> >> ---
> >>  Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
> >>  1 file changed, 19 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> >> index 433afe9..b8b4d9f 100644
> >> --- a/Documentation/devicetree/bindings/arm/psci.txt
> >> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> >> @@ -21,7 +21,7 @@ to #0.
> >>  
> >>  Main node required properties:
> >>  
> >> - - compatible    : Must be "arm,psci"
> >> + - compatible    : Must be "arm,psci-0.2" or "arm,psci"
> > 
> > For the purposes of handling different firmware implementations (which
> > may have different bugs to work around and may require different
> > arguments to cpu_suspend), it may be better to state "must contain"
> > rather than "must be". 
> > 
> >>  
> >>   - method        : The method of calling the PSCI firmware. Permitted
> >>                     values are:
> >> @@ -32,6 +32,9 @@ Main node required properties:
> >>                     "hvc" : HVC #0, with the register assignments specified
> >>  		           in this binding.
> >>  
> >> + - psci_version  : Function ID for PSCI_VERSION operation. Required for
> >> +                   "arm,psci-0.2" compatible version or later.
> >> +
> >>  Main node optional properties:
> >>  
> >>   - cpu_suspend   : Function ID for CPU_SUSPEND operation
> > 
> > The low bits of CPU_SUSPEND's power_state argument are platform specific
> > and we'll need to deal with them if an implementation actually uses them
> > for something. We can probably associate this with a specific
> > implementation's compatible string, as we'll almost certainly need
> > special code to handle the differences between them.
> 
> I'm not a fan of

Me neither. The only alternative I can think of would rely on platform
information from elsewhere to let you know how to call cpu_suspend,
(e.g. a board's compatible string).

As far as I can see, without some knowledge of the platform you can't
use CPU_SUSPEND safely.

> 
> > If we have a compatible string for the first platform that ignores the
> > argument (or just uses zero), that can be shared by all those
> > implementations that don't give any fine-grained control here.
> > 
> >> @@ -42,14 +45,24 @@ Main node optional properties:
> >>  
> >>   - migrate       : Function ID for MIGRATE operation
> >>  
> >> + - system_reset  : Function ID for SYSTEM_RESET operation
> >> +
> >> + - system_off    : Function ID for SYSTEM_OFF operation
> >> +
> >>  
> >>  Example:
> >>  
> >>  	psci {
> >> -		compatible	= "arm,psci";
> >> +		compatible	= "arm,psci-0.2";
> >>  		method		= "smc";
> >> -		cpu_suspend	= <0x95c10000>;
> >> -		cpu_off		= <0x95c10001>;
> >> -		cpu_on		= <0x95c10002>;
> >> -		migrate		= <0x95c10003>;
> >> +		psci_version	= <0x84000000>;
> >> +		cpu_suspend	= <0x84000001>;
> >> +		cpu_off		= <0x84000002>;
> >> +		cpu_on		= <0x84000003>;
> >> +		affinity_info	= <0x84000004>; 
> >> +		migrate		= <0x84000005>;
> >> +		migrate_info_type = <0x84000006>; 
> >> +		migrate_info_up_cpu = <0x84000007>; 
> >> +		system_off	= <0x84000008>; 
> >> +		system_reset	= <0x84000009>; 
> >>  	};
> > 
> > One of the things changed in PSCI 0.2 was the SMC calling convention,
> > though this isn't clear in the PSCI document. The function IDs for 32bit
> > and 64bit callers may differ, and we need to support describing an
> > arbitrary configuration of the two (same ID for both, different across
> > 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> > 
> > I'd like to ensure the binding can deal with that from the start. We
> > could do this by having -32 and -64 variants of each function id (e.g.
> > cpu_off-64) , if the IDs actually differ, and use the regular combined
> > ID if they don't.
> 
> Uggg. I guess I should have read the SMC calling convention doc... I was
> simply documenting what is already in the PSCI doc, but obviously that
> is not fully flushed out.
> 
> How about something like this (for the complicated case of both 32 and
> 64 bit):
> 
> 	method		= "smc", "smc64";
> 	psci_version	= <0x84000000 0xc4000000>;
> 	cpu_suspend	= <0x84000001 0xc4000001>;
> 	cpu_off		= <0x84000002 0xc4000002>;
> 	cpu_on		= <0x84000003 0xc4000003>;
> 
> "smc" is a synonym for smc32 for compatibility. The number and order of
> methods determines the number and order of function IDs.

While this may be compatible with the arm implementation, it won't be
compatible with the arm64 implementation, which assumes smc64 by
default.

As far as I am aware, the implementations currently in use (KVM and Xen)
use the same ID for both, so I think "smc" should cover an ID valid for
a native register width calling convention, and "smc64" and "smc32"
describing values only valid for 64-bit wide and 32-bit wide calling
conventions respectively.

I've added Christoffer, Marc, and Stefano to Cc in case they have any
comments.

> 
> A variation on this would be keep method as is and add a "#psci-cells"
> property to specify the number of function IDs. You can determine the
> 64-bit vs. 32-bit support based on the function ID itself.

I don't think that's a good idea - part of the reasoning for specifying
the IDs is to cater for those not aligned with the ID guidelines in the
spec, so we can't assume their choice of ID value gives us any useful
information as to how they may be used.

Thanks,
Mark.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Dave Martin]

On Mon, Jul 29, 2013 at 03:18:43PM -0500, Rob Herring wrote:
> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> > Hi Rob,
> > 
> > On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> The PSCI spec from ARM has been updated to 0.2 version. Update the
> >> binding document to reflect the spec changes. For the binding, the
> >> major changes are addition of system reset and poweroff functions.
> >> The recommended function id numbering has also changed.
> >>
> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> >> Cc: devicetree at vger.kernel.org
> >> ---
> >>  Documentation/devicetree/bindings/arm/psci.txt | 25 +++++++++++++++++++------
> >>  1 file changed, 19 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
> >> index 433afe9..b8b4d9f 100644
> >> --- a/Documentation/devicetree/bindings/arm/psci.txt
> >> +++ b/Documentation/devicetree/bindings/arm/psci.txt
> >> @@ -21,7 +21,7 @@ to #0.
> >>  
> >>  Main node required properties:
> >>  
> >> - - compatible    : Must be "arm,psci"
> >> + - compatible    : Must be "arm,psci-0.2" or "arm,psci"
> > 
> > For the purposes of handling different firmware implementations (which
> > may have different bugs to work around and may require different
> > arguments to cpu_suspend), it may be better to state "must contain"
> > rather than "must be". 
> > 
> >>  
> >>   - method        : The method of calling the PSCI firmware. Permitted
> >>                     values are:
> >> @@ -32,6 +32,9 @@ Main node required properties:
> >>                     "hvc" : HVC #0, with the register assignments specified
> >>  		           in this binding.
> >>  
> >> + - psci_version  : Function ID for PSCI_VERSION operation. Required for
> >> +                   "arm,psci-0.2" compatible version or later.
> >> +
> >>  Main node optional properties:
> >>  
> >>   - cpu_suspend   : Function ID for CPU_SUSPEND operation
> > 
> > The low bits of CPU_SUSPEND's power_state argument are platform specific
> > and we'll need to deal with them if an implementation actually uses them
> > for something. We can probably associate this with a specific
> > implementation's compatible string, as we'll almost certainly need
> > special code to handle the differences between them.
> 
> I'm not a fan of

...what?

> 
> > If we have a compatible string for the first platform that ignores the
> > argument (or just uses zero), that can be shared by all those
> > implementations that don't give any fine-grained control here.
> > 
> >> @@ -42,14 +45,24 @@ Main node optional properties:
> >>  
> >>   - migrate       : Function ID for MIGRATE operation
> >>  
> >> + - system_reset  : Function ID for SYSTEM_RESET operation
> >> +
> >> + - system_off    : Function ID for SYSTEM_OFF operation
> >> +
> >>  
> >>  Example:
> >>  
> >>  	psci {
> >> -		compatible	= "arm,psci";
> >> +		compatible	= "arm,psci-0.2";
> >>  		method		= "smc";
> >> -		cpu_suspend	= <0x95c10000>;
> >> -		cpu_off		= <0x95c10001>;
> >> -		cpu_on		= <0x95c10002>;
> >> -		migrate		= <0x95c10003>;
> >> +		psci_version	= <0x84000000>;
> >> +		cpu_suspend	= <0x84000001>;
> >> +		cpu_off		= <0x84000002>;
> >> +		cpu_on		= <0x84000003>;
> >> +		affinity_info	= <0x84000004>; 
> >> +		migrate		= <0x84000005>;
> >> +		migrate_info_type = <0x84000006>; 
> >> +		migrate_info_up_cpu = <0x84000007>; 
> >> +		system_off	= <0x84000008>; 
> >> +		system_reset	= <0x84000009>; 
> >>  	};
> > 
> > One of the things changed in PSCI 0.2 was the SMC calling convention,
> > though this isn't clear in the PSCI document. The function IDs for 32bit
> > and 64bit callers may differ, and we need to support describing an
> > arbitrary configuration of the two (same ID for both, different across
> > 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> > 
> > I'd like to ensure the binding can deal with that from the start. We
> > could do this by having -32 and -64 variants of each function id (e.g.
> > cpu_off-64) , if the IDs actually differ, and use the regular combined
> > ID if they don't.
> 
> Uggg. I guess I should have read the SMC calling convention doc... I was
> simply documenting what is already in the PSCI doc, but obviously that
> is not fully flushed out.
> 
> How about something like this (for the complicated case of both 32 and
> 64 bit):
> 
> 	method		= "smc", "smc64";
> 	psci_version	= <0x84000000 0xc4000000>;
> 	cpu_suspend	= <0x84000001 0xc4000001>;
> 	cpu_off		= <0x84000002 0xc4000002>;
> 	cpu_on		= <0x84000003 0xc4000003>;
> 
> "smc" is a synonym for smc32 for compatibility. The number and order of
> methods determines the number and order of function IDs.
> 
> A variation on this would be keep method as is and add a "#psci-cells"
> property to specify the number of function IDs. You can determine the
> 64-bit vs. 32-bit support based on the function ID itself.

Just to point out, these arguments all apply equally to hvc.  So, if
there is an "smc64" method, we should likewise define "hvc64" etc, with
parallel meaning.

Cheers
---Dave

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Rob Herring]

On 07/30/2013 04:49 AM, Mark Rutland wrote:
> On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
>> On 07/29/2013 05:13 AM, Mark Rutland wrote:
>>> Hi Rob,
>>>
>>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
>>>> From: Rob Herring <rob.herring@calxeda.com>

[snip]

>>> One of the things changed in PSCI 0.2 was the SMC calling convention,
>>> though this isn't clear in the PSCI document. The function IDs for 32bit
>>> and 64bit callers may differ, and we need to support describing an
>>> arbitrary configuration of the two (same ID for both, different across
>>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
>>>
>>> I'd like to ensure the binding can deal with that from the start. We
>>> could do this by having -32 and -64 variants of each function id (e.g.
>>> cpu_off-64) , if the IDs actually differ, and use the regular combined
>>> ID if they don't.
>>
>> Uggg. I guess I should have read the SMC calling convention doc... I was
>> simply documenting what is already in the PSCI doc, but obviously that
>> is not fully flushed out.
>>
>> How about something like this (for the complicated case of both 32 and
>> 64 bit):
>>
>> 	method		= "smc", "smc64";
>> 	psci_version	= <0x84000000 0xc4000000>;
>> 	cpu_suspend	= <0x84000001 0xc4000001>;
>> 	cpu_off		= <0x84000002 0xc4000002>;
>> 	cpu_on		= <0x84000003 0xc4000003>;
>>
>> "smc" is a synonym for smc32 for compatibility. The number and order of
>> methods determines the number and order of function IDs.
> 
> While this may be compatible with the arm implementation, it won't be
> compatible with the arm64 implementation, which assumes smc64 by
> default.
> 
> As far as I am aware, the implementations currently in use (KVM and Xen)
> use the same ID for both, so I think "smc" should cover an ID valid for
> a native register width calling convention, and "smc64" and "smc32"
> describing values only valid for 64-bit wide and 32-bit wide calling
> conventions respectively.

The problem is that does not work for a 32-bit kernel on 64-bit h/w as
native from the dts perspective is smc64. Just like the cpu bindings,
the binding cannot change based on 32 or 64 bit OS. I don't think we
really have to deal with that here. We can simply say "smc" is only for
"arm,psci" and deprecated for "arm,psci-0.2".

> I've added Christoffer, Marc, and Stefano to Cc in case they have any
> comments.
> 
>>
>> A variation on this would be keep method as is and add a "#psci-cells"
>> property to specify the number of function IDs. You can determine the
>> 64-bit vs. 32-bit support based on the function ID itself.
> 
> I don't think that's a good idea - part of the reasoning for specifying
> the IDs is to cater for those not aligned with the ID guidelines in the
> spec, so we can't assume their choice of ID value gives us any useful
> information as to how they may be used.

Kind of pointless to encode information into the IDs if you cannot rely
on that...

Rob

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Mark Rutland]

On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
> On 07/30/2013 04:49 AM, Mark Rutland wrote:
> > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
> >> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> >>> Hi Rob,
> >>>
> >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> >>>> From: Rob Herring <rob.herring@calxeda.com>
> 
> [snip]
> 
> >>> One of the things changed in PSCI 0.2 was the SMC calling convention,
> >>> though this isn't clear in the PSCI document. The function IDs for 32bit
> >>> and 64bit callers may differ, and we need to support describing an
> >>> arbitrary configuration of the two (same ID for both, different across
> >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> >>>
> >>> I'd like to ensure the binding can deal with that from the start. We
> >>> could do this by having -32 and -64 variants of each function id (e.g.
> >>> cpu_off-64) , if the IDs actually differ, and use the regular combined
> >>> ID if they don't.
> >>
> >> Uggg. I guess I should have read the SMC calling convention doc... I was
> >> simply documenting what is already in the PSCI doc, but obviously that
> >> is not fully flushed out.
> >>
> >> How about something like this (for the complicated case of both 32 and
> >> 64 bit):
> >>
> >> 	method		= "smc", "smc64";
> >> 	psci_version	= <0x84000000 0xc4000000>;
> >> 	cpu_suspend	= <0x84000001 0xc4000001>;
> >> 	cpu_off		= <0x84000002 0xc4000002>;
> >> 	cpu_on		= <0x84000003 0xc4000003>;
> >>
> >> "smc" is a synonym for smc32 for compatibility. The number and order of
> >> methods determines the number and order of function IDs.
> > 
> > While this may be compatible with the arm implementation, it won't be
> > compatible with the arm64 implementation, which assumes smc64 by
> > default.
> > 
> > As far as I am aware, the implementations currently in use (KVM and Xen)
> > use the same ID for both, so I think "smc" should cover an ID valid for
> > a native register width calling convention, and "smc64" and "smc32"
> > describing values only valid for 64-bit wide and 32-bit wide calling
> > conventions respectively.
> 
> The problem is that does not work for a 32-bit kernel on 64-bit h/w as
> native from the dts perspective is smc64. Just like the cpu bindings,
> the binding cannot change based on 32 or 64 bit OS. I don't think we
> really have to deal with that here. We can simply say "smc" is only for
> "arm,psci" and deprecated for "arm,psci-0.2".

Agreed. I'd be happy with only having "smc32" and "smc64" for
"arm,psci-0.2".

Thanks,
Mark.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Mark Rutland]

On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote:
> On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
> > On 07/30/2013 04:49 AM, Mark Rutland wrote:
> > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
> > >> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> > >>> Hi Rob,
> > >>>
> > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> > >>>> From: Rob Herring <rob.herring@calxeda.com>
> > 
> > [snip]
> > 
> > >>> One of the things changed in PSCI 0.2 was the SMC calling convention,
> > >>> though this isn't clear in the PSCI document. The function IDs for 32bit
> > >>> and 64bit callers may differ, and we need to support describing an
> > >>> arbitrary configuration of the two (same ID for both, different across
> > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> > >>>
> > >>> I'd like to ensure the binding can deal with that from the start. We
> > >>> could do this by having -32 and -64 variants of each function id (e.g.
> > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined
> > >>> ID if they don't.
> > >>
> > >> Uggg. I guess I should have read the SMC calling convention doc... I was
> > >> simply documenting what is already in the PSCI doc, but obviously that
> > >> is not fully flushed out.
> > >>
> > >> How about something like this (for the complicated case of both 32 and
> > >> 64 bit):
> > >>
> > >> 	method		= "smc", "smc64";
> > >> 	psci_version	= <0x84000000 0xc4000000>;
> > >> 	cpu_suspend	= <0x84000001 0xc4000001>;
> > >> 	cpu_off		= <0x84000002 0xc4000002>;
> > >> 	cpu_on		= <0x84000003 0xc4000003>;
> > >>
> > >> "smc" is a synonym for smc32 for compatibility. The number and order of
> > >> methods determines the number and order of function IDs.
> > > 
> > > While this may be compatible with the arm implementation, it won't be
> > > compatible with the arm64 implementation, which assumes smc64 by
> > > default.
> > > 
> > > As far as I am aware, the implementations currently in use (KVM and Xen)
> > > use the same ID for both, so I think "smc" should cover an ID valid for
> > > a native register width calling convention, and "smc64" and "smc32"
> > > describing values only valid for 64-bit wide and 32-bit wide calling
> > > conventions respectively.
> > 
> > The problem is that does not work for a 32-bit kernel on 64-bit h/w as
> > native from the dts perspective is smc64. Just like the cpu bindings,
> > the binding cannot change based on 32 or 64 bit OS. I don't think we
> > really have to deal with that here. We can simply say "smc" is only for
> > "arm,psci" and deprecated for "arm,psci-0.2".
> 
> Agreed. I'd be happy with only having "smc32" and "smc64" for
> "arm,psci-0.2".

Actually, from some quick discussion with Marc and Dave, I think we can
handle this better, in a way that leaves this backwards compatible.

Rather than relying on the method string to tell us the calling
convention, I think we should rely on the function ID, as I proposed
earlier. The existing function ids provided in the "arm,psci" binding
are implicitly relying on the PSCI implementation to detect the register
width and act accordingly. This is trivially true on 32bit hardware, KVM
(where the same ID is used for 32-bit and 64-bit guests), and while I'm
not entirely sure about Xen I believe it's true there. We can make this
explicit as we extend the binding.

Having a -64 and -32 variant of each ID (while not pretty) allows us to
add additional IDs for functions that might only have a 32-bit or 64-bit
interface implemented, in addition to functions with common IDs:

psci {
	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
	cpu_off = <12345678>;
	cpu_on = <01234567>;
	system_reset-32 = <02222222>;
	system_reset-64 = <12222222>;
	affinity_info-64 = <15555555>;
};

This means that hypervisors could update their PSCI implementation while
keeping their DTS compatible with existing kernels.

Thanks,
Mark.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Stefano Stabellini]

On Tue, 30 Jul 2013, Mark Rutland wrote:
> On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote:
> > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
> > > On 07/30/2013 04:49 AM, Mark Rutland wrote:
> > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
> > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> > > >>> Hi Rob,
> > > >>>
> > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> > > >>>> From: Rob Herring <rob.herring@calxeda.com>
> > > 
> > > [snip]
> > > 
> > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention,
> > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit
> > > >>> and 64bit callers may differ, and we need to support describing an
> > > >>> arbitrary configuration of the two (same ID for both, different across
> > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> > > >>>
> > > >>> I'd like to ensure the binding can deal with that from the start. We
> > > >>> could do this by having -32 and -64 variants of each function id (e.g.
> > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined
> > > >>> ID if they don't.
> > > >>
> > > >> Uggg. I guess I should have read the SMC calling convention doc... I was
> > > >> simply documenting what is already in the PSCI doc, but obviously that
> > > >> is not fully flushed out.
> > > >>
> > > >> How about something like this (for the complicated case of both 32 and
> > > >> 64 bit):
> > > >>
> > > >> 	method		= "smc", "smc64";
> > > >> 	psci_version	= <0x84000000 0xc4000000>;
> > > >> 	cpu_suspend	= <0x84000001 0xc4000001>;
> > > >> 	cpu_off		= <0x84000002 0xc4000002>;
> > > >> 	cpu_on		= <0x84000003 0xc4000003>;
> > > >>
> > > >> "smc" is a synonym for smc32 for compatibility. The number and order of
> > > >> methods determines the number and order of function IDs.
> > > > 
> > > > While this may be compatible with the arm implementation, it won't be
> > > > compatible with the arm64 implementation, which assumes smc64 by
> > > > default.
> > > > 
> > > > As far as I am aware, the implementations currently in use (KVM and Xen)
> > > > use the same ID for both, so I think "smc" should cover an ID valid for
> > > > a native register width calling convention, and "smc64" and "smc32"
> > > > describing values only valid for 64-bit wide and 32-bit wide calling
> > > > conventions respectively.
> > > 
> > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as
> > > native from the dts perspective is smc64. Just like the cpu bindings,
> > > the binding cannot change based on 32 or 64 bit OS. I don't think we
> > > really have to deal with that here. We can simply say "smc" is only for
> > > "arm,psci" and deprecated for "arm,psci-0.2".
> > 
> > Agreed. I'd be happy with only having "smc32" and "smc64" for
> > "arm,psci-0.2".

I would be happy with that too (Xen only uses HVC as "method").


> Actually, from some quick discussion with Marc and Dave, I think we can
> handle this better, in a way that leaves this backwards compatible.
> 
> Rather than relying on the method string to tell us the calling
> convention, I think we should rely on the function ID, as I proposed
> earlier. The existing function ids provided in the "arm,psci" binding
> are implicitly relying on the PSCI implementation to detect the register
> width and act accordingly. This is trivially true on 32bit hardware, KVM
> (where the same ID is used for 32-bit and 64-bit guests), and while I'm
> not entirely sure about Xen I believe it's true there. We can make this
> explicit as we extend the binding.
> 
> Having a -64 and -32 variant of each ID (while not pretty) allows us to
> add additional IDs for functions that might only have a 32-bit or 64-bit
> interface implemented, in addition to functions with common IDs:
> 
> psci {
> 	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
> 	cpu_off = <12345678>;
> 	cpu_on = <01234567>;
> 	system_reset-32 = <02222222>;
> 	system_reset-64 = <12222222>;
> 	affinity_info-64 = <15555555>;
> };
> 
> This means that hypervisors could update their PSCI implementation while
> keeping their DTS compatible with existing kernels.

Are you proposing of getting rid of "method" completely and therefore
have 4 possible function IDs for each function:

system_reset-32-HVC
system_reset-64-SMC
system_reset-32-HVC
system_reset-64-SMC

or are you proposing of choosing just the register width via function
IDs?

Honestly I think it would be cleaner to introduce a new field called
"width" can that be 32 or 64 and represent the register width, rather
than having an explosion of function IDs.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Ian Campbell]

On Tue, 2013-07-30 at 15:33 +0100, Stefano Stabellini wrote:
> On Tue, 30 Jul 2013, Mark Rutland wrote:
> > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote:
> > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
> > > > On 07/30/2013 04:49 AM, Mark Rutland wrote:
> > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
> > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> > > > >>> Hi Rob,
> > > > >>>
> > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> > > > >>>> From: Rob Herring <rob.herring@calxeda.com>
> > > > 
> > > > [snip]
> > > > 
> > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention,
> > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit
> > > > >>> and 64bit callers may differ, and we need to support describing an
> > > > >>> arbitrary configuration of the two (same ID for both, different across
> > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> > > > >>>
> > > > >>> I'd like to ensure the binding can deal with that from the start. We
> > > > >>> could do this by having -32 and -64 variants of each function id (e.g.
> > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined
> > > > >>> ID if they don't.
> > > > >>
> > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was
> > > > >> simply documenting what is already in the PSCI doc, but obviously that
> > > > >> is not fully flushed out.
> > > > >>
> > > > >> How about something like this (for the complicated case of both 32 and
> > > > >> 64 bit):
> > > > >>
> > > > >> 	method		= "smc", "smc64";
> > > > >> 	psci_version	= <0x84000000 0xc4000000>;
> > > > >> 	cpu_suspend	= <0x84000001 0xc4000001>;
> > > > >> 	cpu_off		= <0x84000002 0xc4000002>;
> > > > >> 	cpu_on		= <0x84000003 0xc4000003>;
> > > > >>
> > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of
> > > > >> methods determines the number and order of function IDs.
> > > > > 
> > > > > While this may be compatible with the arm implementation, it won't be
> > > > > compatible with the arm64 implementation, which assumes smc64 by
> > > > > default.
> > > > > 
> > > > > As far as I am aware, the implementations currently in use (KVM and Xen)
> > > > > use the same ID for both, so I think "smc" should cover an ID valid for
> > > > > a native register width calling convention, and "smc64" and "smc32"
> > > > > describing values only valid for 64-bit wide and 32-bit wide calling
> > > > > conventions respectively.
> > > > 
> > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as
> > > > native from the dts perspective is smc64. Just like the cpu bindings,
> > > > the binding cannot change based on 32 or 64 bit OS. I don't think we
> > > > really have to deal with that here. We can simply say "smc" is only for
> > > > "arm,psci" and deprecated for "arm,psci-0.2".
> > > 
> > > Agreed. I'd be happy with only having "smc32" and "smc64" for
> > > "arm,psci-0.2".
> 
> I would be happy with that too (Xen only uses HVC as "method").

Presumably we have the same issues with hvc vs hvc32 vs hvc64 though?

And does smcX imply hvcX or does that also need to be documented here?

> 
> > Actually, from some quick discussion with Marc and Dave, I think we can
> > handle this better, in a way that leaves this backwards compatible.
> > 
> > Rather than relying on the method string to tell us the calling
> > convention, I think we should rely on the function ID, as I proposed
> > earlier. The existing function ids provided in the "arm,psci" binding
> > are implicitly relying on the PSCI implementation to detect the register
> > width and act accordingly. This is trivially true on 32bit hardware, KVM
> > (where the same ID is used for 32-bit and 64-bit guests), and while I'm
> > not entirely sure about Xen I believe it's true there. We can make this
> > explicit as we extend the binding.
> > 
> > Having a -64 and -32 variant of each ID (while not pretty) allows us to
> > add additional IDs for functions that might only have a 32-bit or 64-bit
> > interface implemented, in addition to functions with common IDs:
> > 
> > psci {
> > 	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
> > 	cpu_off = <12345678>;
> > 	cpu_on = <01234567>;
> > 	system_reset-32 = <02222222>;
> > 	system_reset-64 = <12222222>;
> > 	affinity_info-64 = <15555555>;
> > };
> > 
> > This means that hypervisors could update their PSCI implementation while
> > keeping their DTS compatible with existing kernels.
> 
> Are you proposing of getting rid of "method" completely and therefore
> have 4 possible function IDs for each function:
> 
> system_reset-32-HVC
> system_reset-64-SMC
> system_reset-32-HVC
> system_reset-64-SMC
> 
> or are you proposing of choosing just the register width via function
> IDs?
> 
> Honestly I think it would be cleaner to introduce a new field called
> "width" can that be 32 or 64 and represent the register width, rather
> than having an explosion of function IDs.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Matt Sealey]

Isn't this being overthought?

"method" (I would have re-used model, but that's by-the-by) should
just determine which call interface to use. Only one should end up in
a device tree - it doesn't make a lot of sense to specify more than
one, to me, given my read of the specs and the SMC calling convention.

For AArch64 chips they should implement 64-bit SMC interface or 32-bit
SMC interface. Either way you get there, the function number is
encoded with the type of call anyway so AArch64 secure monitors or
hypervisors will know which one was intended, given the SMC calling
convention defines this "64-bit" bit pretty clearly. The "method" is
an instruction to the "guest OS" in how to fill it's registers more
than it is in function names.

* An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird
and in any case, the device tree would use the 32-bit convention (any
SMC64 call in 32-bit state should return "Unknown SMC Function
Identifier" if that bit is set)
* An AArch32 guest under an AArch32  hypervisor/sm would use the
32-bit convention
* An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit
convention
* An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit
convention or the 32-bit convention depending on how the sm is
written, but it doesn't matter which is used if both can be
supported.. but you'd only want to use one of them.

TLDR; Supply your (EL1) guests with the preferred method, not a list
of all possible methods, as above. If you're the vendor and you
defined any part of EL3 or EL2 you're in complete control of which
method you want subordinate levels to use by way of implementation,
right?

BTW according to the specs you can't have EL2 without EL3 on ARMv7. I
would think the issues with hvc vs hvc64 need to be thought out;
hypervisors can trap SMC from their guests and do the right thing
without the guest ever knowing that a hypervisor is above it. If it is
the case that every Xen DomU kernel needs to know that it is
virtualized and to be told to use HVC instead of SMC? What's the
benefit of that?

-- Matt

On Tue, Jul 30, 2013 at 9:42 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-07-30 at 15:33 +0100, Stefano Stabellini wrote:
>> On Tue, 30 Jul 2013, Mark Rutland wrote:
>> > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote:
>> > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
>> > > > On 07/30/2013 04:49 AM, Mark Rutland wrote:
>> > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
>> > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote:
>> > > > >>> Hi Rob,
>> > > > >>>
>> > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
>> > > > >>>> From: Rob Herring <rob.herring@calxeda.com>
>> > > >
>> > > > [snip]
>> > > >
>> > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention,
>> > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit
>> > > > >>> and 64bit callers may differ, and we need to support describing an
>> > > > >>> arbitrary configuration of the two (same ID for both, different across
>> > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
>> > > > >>>
>> > > > >>> I'd like to ensure the binding can deal with that from the start. We
>> > > > >>> could do this by having -32 and -64 variants of each function id (e.g.
>> > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined
>> > > > >>> ID if they don't.
>> > > > >>
>> > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was
>> > > > >> simply documenting what is already in the PSCI doc, but obviously that
>> > > > >> is not fully flushed out.
>> > > > >>
>> > > > >> How about something like this (for the complicated case of both 32 and
>> > > > >> 64 bit):
>> > > > >>
>> > > > >>      method          = "smc", "smc64";
>> > > > >>      psci_version    = <0x84000000 0xc4000000>;
>> > > > >>      cpu_suspend     = <0x84000001 0xc4000001>;
>> > > > >>      cpu_off         = <0x84000002 0xc4000002>;
>> > > > >>      cpu_on          = <0x84000003 0xc4000003>;
>> > > > >>
>> > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of
>> > > > >> methods determines the number and order of function IDs.
>> > > > >
>> > > > > While this may be compatible with the arm implementation, it won't be
>> > > > > compatible with the arm64 implementation, which assumes smc64 by
>> > > > > default.
>> > > > >
>> > > > > As far as I am aware, the implementations currently in use (KVM and Xen)
>> > > > > use the same ID for both, so I think "smc" should cover an ID valid for
>> > > > > a native register width calling convention, and "smc64" and "smc32"
>> > > > > describing values only valid for 64-bit wide and 32-bit wide calling
>> > > > > conventions respectively.
>> > > >
>> > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as
>> > > > native from the dts perspective is smc64. Just like the cpu bindings,
>> > > > the binding cannot change based on 32 or 64 bit OS. I don't think we
>> > > > really have to deal with that here. We can simply say "smc" is only for
>> > > > "arm,psci" and deprecated for "arm,psci-0.2".
>> > >
>> > > Agreed. I'd be happy with only having "smc32" and "smc64" for
>> > > "arm,psci-0.2".
>>
>> I would be happy with that too (Xen only uses HVC as "method").
>
> Presumably we have the same issues with hvc vs hvc32 vs hvc64 though?
>
> And does smcX imply hvcX or does that also need to be documented here?
>
>>
>> > Actually, from some quick discussion with Marc and Dave, I think we can
>> > handle this better, in a way that leaves this backwards compatible.
>> >
>> > Rather than relying on the method string to tell us the calling
>> > convention, I think we should rely on the function ID, as I proposed
>> > earlier. The existing function ids provided in the "arm,psci" binding
>> > are implicitly relying on the PSCI implementation to detect the register
>> > width and act accordingly. This is trivially true on 32bit hardware, KVM
>> > (where the same ID is used for 32-bit and 64-bit guests), and while I'm
>> > not entirely sure about Xen I believe it's true there. We can make this
>> > explicit as we extend the binding.
>> >
>> > Having a -64 and -32 variant of each ID (while not pretty) allows us to
>> > add additional IDs for functions that might only have a 32-bit or 64-bit
>> > interface implemented, in addition to functions with common IDs:
>> >
>> > psci {
>> >     compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
>> >     cpu_off = <12345678>;
>> >     cpu_on = <01234567>;
>> >     system_reset-32 = <02222222>;
>> >     system_reset-64 = <12222222>;
>> >     affinity_info-64 = <15555555>;
>> > };
>> >
>> > This means that hypervisors could update their PSCI implementation while
>> > keeping their DTS compatible with existing kernels.
>>
>> Are you proposing of getting rid of "method" completely and therefore
>> have 4 possible function IDs for each function:
>>
>> system_reset-32-HVC
>> system_reset-64-SMC
>> system_reset-32-HVC
>> system_reset-64-SMC
>>
>> or are you proposing of choosing just the register width via function
>> IDs?
>>
>> Honestly I think it would be cleaner to introduce a new field called
>> "width" can that be 32 or 64 and represent the register width, rather
>> than having an explosion of function IDs.
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Rob Herring]

On 07/30/2013 09:33 AM, Stefano Stabellini wrote:
> On Tue, 30 Jul 2013, Mark Rutland wrote:
>> On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote:
>>> On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
>>>> On 07/30/2013 04:49 AM, Mark Rutland wrote:
>>>>> On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
>>>>>> On 07/29/2013 05:13 AM, Mark Rutland wrote:
>>>>>>> Hi Rob,
>>>>>>>
>>>>>>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
>>>>>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>
>>>> [snip]
>>>>
>>>>>>> One of the things changed in PSCI 0.2 was the SMC calling convention,
>>>>>>> though this isn't clear in the PSCI document. The function IDs for 32bit
>>>>>>> and 64bit callers may differ, and we need to support describing an
>>>>>>> arbitrary configuration of the two (same ID for both, different across
>>>>>>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
>>>>>>>
>>>>>>> I'd like to ensure the binding can deal with that from the start. We
>>>>>>> could do this by having -32 and -64 variants of each function id (e.g.
>>>>>>> cpu_off-64) , if the IDs actually differ, and use the regular combined
>>>>>>> ID if they don't.
>>>>>>
>>>>>> Uggg. I guess I should have read the SMC calling convention doc... I was
>>>>>> simply documenting what is already in the PSCI doc, but obviously that
>>>>>> is not fully flushed out.
>>>>>>
>>>>>> How about something like this (for the complicated case of both 32 and
>>>>>> 64 bit):
>>>>>>
>>>>>> 	method		= "smc", "smc64";
>>>>>> 	psci_version	= <0x84000000 0xc4000000>;
>>>>>> 	cpu_suspend	= <0x84000001 0xc4000001>;
>>>>>> 	cpu_off		= <0x84000002 0xc4000002>;
>>>>>> 	cpu_on		= <0x84000003 0xc4000003>;
>>>>>>
>>>>>> "smc" is a synonym for smc32 for compatibility. The number and order of
>>>>>> methods determines the number and order of function IDs.
>>>>>
>>>>> While this may be compatible with the arm implementation, it won't be
>>>>> compatible with the arm64 implementation, which assumes smc64 by
>>>>> default.
>>>>>
>>>>> As far as I am aware, the implementations currently in use (KVM and Xen)
>>>>> use the same ID for both, so I think "smc" should cover an ID valid for
>>>>> a native register width calling convention, and "smc64" and "smc32"
>>>>> describing values only valid for 64-bit wide and 32-bit wide calling
>>>>> conventions respectively.
>>>>
>>>> The problem is that does not work for a 32-bit kernel on 64-bit h/w as
>>>> native from the dts perspective is smc64. Just like the cpu bindings,
>>>> the binding cannot change based on 32 or 64 bit OS. I don't think we
>>>> really have to deal with that here. We can simply say "smc" is only for
>>>> "arm,psci" and deprecated for "arm,psci-0.2".
>>>
>>> Agreed. I'd be happy with only having "smc32" and "smc64" for
>>> "arm,psci-0.2".
> 
> I would be happy with that too (Xen only uses HVC as "method").
> 
> 
>> Actually, from some quick discussion with Marc and Dave, I think we can
>> handle this better, in a way that leaves this backwards compatible.
>>
>> Rather than relying on the method string to tell us the calling
>> convention, I think we should rely on the function ID, as I proposed
>> earlier. The existing function ids provided in the "arm,psci" binding
>> are implicitly relying on the PSCI implementation to detect the register
>> width and act accordingly. This is trivially true on 32bit hardware, KVM
>> (where the same ID is used for 32-bit and 64-bit guests), and while I'm
>> not entirely sure about Xen I believe it's true there. We can make this
>> explicit as we extend the binding.
>>
>> Having a -64 and -32 variant of each ID (while not pretty) allows us to
>> add additional IDs for functions that might only have a 32-bit or 64-bit
>> interface implemented, in addition to functions with common IDs:
>>
>> psci {
>> 	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
>> 	cpu_off = <12345678>;
>> 	cpu_on = <01234567>;
>> 	system_reset-32 = <02222222>;
>> 	system_reset-64 = <12222222>;
>> 	affinity_info-64 = <15555555>;
>> };
>>
>> This means that hypervisors could update their PSCI implementation while
>> keeping their DTS compatible with existing kernels.
> 
> Are you proposing of getting rid of "method" completely and therefore
> have 4 possible function IDs for each function:
> 
> system_reset-32-HVC
> system_reset-64-SMC
> system_reset-32-HVC
> system_reset-64-SMC

No. you should never have a DTB with both hvc and smc. The h/w
(firmware) to OS interface is smc. The hypervisor to guest interface is
a separate ABI and would be hvc. They are independent of each other.

> or are you proposing of choosing just the register width via function
> IDs?
> 
> Honestly I think it would be cleaner to introduce a new field called
> "width" can that be 32 or 64 and represent the register width, rather
> than having an explosion of function IDs.

I can think of lots of ways it would be cleaner, but that is not what
ARM defined. There are 3 cases to handle: 32-bit only calls, 64-bit only
calls and both 32/64-bit calls. I think hypervisors have the same issue
as firmware. Do you know the guest is 32 or 64 bit up front and can
provide it a dtb based on that? If not, then you can never use the
64-bit only calls. So Xen has to use 32-bit only or provide both 32 and
64 bit interfaces.

Rob

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Ian Campbell]

On Tue, 2013-07-30 at 12:48 -0500, Matt Sealey wrote:
> * An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird
> and in any case, the device tree would use the 32-bit convention (any
> SMC64 call in 32-bit state should return "Unknown SMC Function
> Identifier" if that bit is set)

I don't believe 64-bit EL1 under 32-bit EL2 is allowed by the
architecture. Each EL must be at least the width of the one below it.

> TLDR; Supply your (EL1) guests with the preferred method, not a list
> of all possible methods, as above. If you're the vendor and you
> defined any part of EL3 or EL2 you're in complete control of which
> method you want subordinate levels to use by way of implementation,
> right?

I haven't seen the entirety of this discussion, but that seems logical
to me.

> 
> BTW according to the specs you can't have EL2 without EL3 on ARMv7. I
> would think the issues with hvc vs hvc64 need to be thought out;
> hypervisors can trap SMC from their guests and do the right thing
> without the guest ever knowing that a hypervisor is above it.

The issue is that SMC traps don't provide you with the #imm in the
syndrome register so you need to fetch and decode the instruction in the
hypervisor manually, which is possible but faffsome and relatively
costly (mostly due to the need to map guest memory on demand).

> If it is the case that every Xen DomU kernel needs to know that it is
> virtualized and to be told to use HVC instead of SMC? What's the
> benefit of that?

Just the avoidance of a fetch and decode, I think.

I'm also not sure if Secure world can't prevent NS EL2 from tapping SMC,
would need to check the specs.

Ian.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Ian Campbell]

On Tue, 2013-07-30 at 14:34 -0500, Rob Herring wrote:

> I can think of lots of ways it would be cleaner, but that is not what
> ARM defined. There are 3 cases to handle: 32-bit only calls, 64-bit only
> calls and both 32/64-bit calls. I think hypervisors have the same issue
> as firmware. Do you know the guest is 32 or 64 bit up front and can
> provide it a dtb based on that?

Yes. An EL cannot change its own bit width and needs to rely on the EL
above returning in the right mode. IOW a hypervisor needs to know if it
is launching a 32- or 64-bit guest already.

I suppose in theory we could supply a mode switch hypercall, but we
don't and I think that is rather out of scope here...

Ian.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Mark Rutland]

On Tue, Jul 30, 2013 at 03:33:34PM +0100, Stefano Stabellini wrote:
> On Tue, 30 Jul 2013, Mark Rutland wrote:
> > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote:
> > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
> > > > On 07/30/2013 04:49 AM, Mark Rutland wrote:
> > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
> > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> > > > >>> Hi Rob,
> > > > >>>
> > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> > > > >>>> From: Rob Herring <rob.herring@calxeda.com>
> > > > 
> > > > [snip]
> > > > 
> > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention,
> > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit
> > > > >>> and 64bit callers may differ, and we need to support describing an
> > > > >>> arbitrary configuration of the two (same ID for both, different across
> > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> > > > >>>
> > > > >>> I'd like to ensure the binding can deal with that from the start. We
> > > > >>> could do this by having -32 and -64 variants of each function id (e.g.
> > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined
> > > > >>> ID if they don't.
> > > > >>
> > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was
> > > > >> simply documenting what is already in the PSCI doc, but obviously that
> > > > >> is not fully flushed out.
> > > > >>
> > > > >> How about something like this (for the complicated case of both 32 and
> > > > >> 64 bit):
> > > > >>
> > > > >> 	method		= "smc", "smc64";
> > > > >> 	psci_version	= <0x84000000 0xc4000000>;
> > > > >> 	cpu_suspend	= <0x84000001 0xc4000001>;
> > > > >> 	cpu_off		= <0x84000002 0xc4000002>;
> > > > >> 	cpu_on		= <0x84000003 0xc4000003>;
> > > > >>
> > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of
> > > > >> methods determines the number and order of function IDs.
> > > > > 
> > > > > While this may be compatible with the arm implementation, it won't be
> > > > > compatible with the arm64 implementation, which assumes smc64 by
> > > > > default.
> > > > > 
> > > > > As far as I am aware, the implementations currently in use (KVM and Xen)
> > > > > use the same ID for both, so I think "smc" should cover an ID valid for
> > > > > a native register width calling convention, and "smc64" and "smc32"
> > > > > describing values only valid for 64-bit wide and 32-bit wide calling
> > > > > conventions respectively.
> > > > 
> > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as
> > > > native from the dts perspective is smc64. Just like the cpu bindings,
> > > > the binding cannot change based on 32 or 64 bit OS. I don't think we
> > > > really have to deal with that here. We can simply say "smc" is only for
> > > > "arm,psci" and deprecated for "arm,psci-0.2".
> > > 
> > > Agreed. I'd be happy with only having "smc32" and "smc64" for
> > > "arm,psci-0.2".
> 
> I would be happy with that too (Xen only uses HVC as "method").
> 
> 
> > Actually, from some quick discussion with Marc and Dave, I think we can
> > handle this better, in a way that leaves this backwards compatible.
> > 
> > Rather than relying on the method string to tell us the calling
> > convention, I think we should rely on the function ID, as I proposed
> > earlier. The existing function ids provided in the "arm,psci" binding
> > are implicitly relying on the PSCI implementation to detect the register
> > width and act accordingly. This is trivially true on 32bit hardware, KVM
> > (where the same ID is used for 32-bit and 64-bit guests), and while I'm
> > not entirely sure about Xen I believe it's true there. We can make this
> > explicit as we extend the binding.
> > 
> > Having a -64 and -32 variant of each ID (while not pretty) allows us to
> > add additional IDs for functions that might only have a 32-bit or 64-bit
> > interface implemented, in addition to functions with common IDs:
> > 
> > psci {
> > 	compatible = "$VENDOR,psci-0.2", "arm,psci-0.2", "arm,psci";
> > 	cpu_off = <12345678>;
> > 	cpu_on = <01234567>;
> > 	system_reset-32 = <02222222>;
> > 	system_reset-64 = <12222222>;
> > 	affinity_info-64 = <15555555>;
> > };
> > 
> > This means that hypervisors could update their PSCI implementation while
> > keeping their DTS compatible with existing kernels.
> 
> Are you proposing of getting rid of "method" completely and therefore
> have 4 possible function IDs for each function:
> 
> system_reset-32-HVC
> system_reset-64-SMC
> system_reset-32-HVC
> system_reset-64-SMC
> 
> or are you proposing of choosing just the register width via function
> IDs?

Just the register width via the function ID's property name.

> 
> Honestly I think it would be cleaner to introduce a new field called
> "width" can that be 32 or 64 and represent the register width, rather
> than having an explosion of function IDs.
> 

Possibly. I'm worried people may create a mixture of shared, 64-bit
only, and 32-bit only IDs.

Thanks,
Mark.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Mark Rutland]

On Tue, Jul 30, 2013 at 03:42:34PM +0100, Ian Campbell wrote:
> On Tue, 2013-07-30 at 15:33 +0100, Stefano Stabellini wrote:
> > On Tue, 30 Jul 2013, Mark Rutland wrote:
> > > On Tue, Jul 30, 2013 at 01:56:50PM +0100, Mark Rutland wrote:
> > > > On Tue, Jul 30, 2013 at 01:42:49PM +0100, Rob Herring wrote:
> > > > > On 07/30/2013 04:49 AM, Mark Rutland wrote:
> > > > > > On Mon, Jul 29, 2013 at 09:18:43PM +0100, Rob Herring wrote:
> > > > > >> On 07/29/2013 05:13 AM, Mark Rutland wrote:
> > > > > >>> Hi Rob,
> > > > > >>>
> > > > > >>> On Sun, Jul 28, 2013 at 10:56:32PM +0100, Rob Herring wrote:
> > > > > >>>> From: Rob Herring <rob.herring@calxeda.com>
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > >>> One of the things changed in PSCI 0.2 was the SMC calling convention,
> > > > > >>> though this isn't clear in the PSCI document. The function IDs for 32bit
> > > > > >>> and 64bit callers may differ, and we need to support describing an
> > > > > >>> arbitrary configuration of the two (same ID for both, different across
> > > > > >>> 32-bit/64-bit, only supported for 64-bit, only supported for 32-bit).
> > > > > >>>
> > > > > >>> I'd like to ensure the binding can deal with that from the start. We
> > > > > >>> could do this by having -32 and -64 variants of each function id (e.g.
> > > > > >>> cpu_off-64) , if the IDs actually differ, and use the regular combined
> > > > > >>> ID if they don't.
> > > > > >>
> > > > > >> Uggg. I guess I should have read the SMC calling convention doc... I was
> > > > > >> simply documenting what is already in the PSCI doc, but obviously that
> > > > > >> is not fully flushed out.
> > > > > >>
> > > > > >> How about something like this (for the complicated case of both 32 and
> > > > > >> 64 bit):
> > > > > >>
> > > > > >> 	method		= "smc", "smc64";
> > > > > >> 	psci_version	= <0x84000000 0xc4000000>;
> > > > > >> 	cpu_suspend	= <0x84000001 0xc4000001>;
> > > > > >> 	cpu_off		= <0x84000002 0xc4000002>;
> > > > > >> 	cpu_on		= <0x84000003 0xc4000003>;
> > > > > >>
> > > > > >> "smc" is a synonym for smc32 for compatibility. The number and order of
> > > > > >> methods determines the number and order of function IDs.
> > > > > > 
> > > > > > While this may be compatible with the arm implementation, it won't be
> > > > > > compatible with the arm64 implementation, which assumes smc64 by
> > > > > > default.
> > > > > > 
> > > > > > As far as I am aware, the implementations currently in use (KVM and Xen)
> > > > > > use the same ID for both, so I think "smc" should cover an ID valid for
> > > > > > a native register width calling convention, and "smc64" and "smc32"
> > > > > > describing values only valid for 64-bit wide and 32-bit wide calling
> > > > > > conventions respectively.
> > > > > 
> > > > > The problem is that does not work for a 32-bit kernel on 64-bit h/w as
> > > > > native from the dts perspective is smc64. Just like the cpu bindings,
> > > > > the binding cannot change based on 32 or 64 bit OS. I don't think we
> > > > > really have to deal with that here. We can simply say "smc" is only for
> > > > > "arm,psci" and deprecated for "arm,psci-0.2".
> > > > 
> > > > Agreed. I'd be happy with only having "smc32" and "smc64" for
> > > > "arm,psci-0.2".
> > 
> > I would be happy with that too (Xen only uses HVC as "method").
> 
> Presumably we have the same issues with hvc vs hvc32 vs hvc64 though?

Yes. I'd not considered this with my original reply, but we have the
exact same issue for hvc.

> 
> And does smcX imply hvcX or does that also need to be documented here?

The two are mutually exclusive, neither implies the other (KVM will barf
if you use an SMC iirc).

Thanks,
Mark.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Mark Rutland]

On Tue, Jul 30, 2013 at 06:48:59PM +0100, Matt Sealey wrote:
> Isn't this being overthought?
> 
> "method" (I would have re-used model, but that's by-the-by) should
> just determine which call interface to use. Only one should end up in
> a device tree - it doesn't make a lot of sense to specify more than
> one, to me, given my read of the specs and the SMC calling convention.
> 
> For AArch64 chips they should implement 64-bit SMC interface or 32-bit
> SMC interface. Either way you get there, the function number is
> encoded with the type of call anyway so AArch64 secure monitors or
> hypervisors will know which one was intended, given the SMC calling
> convention defines this "64-bit" bit pretty clearly. The "method" is
> an instruction to the "guest OS" in how to fill it's registers more
> than it is in function names.

Implementations may not follow the recommended ID allocations, and thus
we cannot rely on the function ID to provide us with any useful
information. We certainly cannot rely on it for hvcs, where for an
existing implementation (KVM) uses the same IDs for 32-bit and 64-bit
guests.

If we force the use of said bit, then new kernels cannot necessarily
work on existing PSCI implementations (which may not handle the use of
said bit as you expect). If we make the binding for psci-0.2 explicitly
require the use of this bit, then future firmware/virtualisation code
cannot provide new functionality withoot breaking backwards compatiblity
with existing kernels, as an "arm,psci-0.2" node would be incompatible
with an "arm,psci" node (which is all an existing kernel can handle).

> 
> * An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird
> and in any case, the device tree would use the 32-bit convention (any
> SMC64 call in 32-bit state should return "Unknown SMC Function
> Identifier" if that bit is set)

This is not allowed by the architecture.

> * An AArch32 guest under an AArch32  hypervisor/sm would use the
> 32-bit convention
> * An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit
> convention

Both of these cases are trivially mandated by the architecture.

> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit
> convention or the 32-bit convention depending on how the sm is
> written, but it doesn't matter which is used if both can be
> supported.. but you'd only want to use one of them.

Not all implementation will implement both, so there needs to be a way
to describe that. Which is actually used is up to the OS.

> 
> TLDR; Supply your (EL1) guests with the preferred method, not a list
> of all possible methods, as above. If you're the vendor and you
> defined any part of EL3 or EL2 you're in complete control of which
> method you want subordinate levels to use by way of implementation,
> right?

As far as I can tell, no-one was arguing against this strategy. However,
for those cases where functionality is implemented for only one
register-width, or requires a different ID per register-width, this
needs to be described somehow.

Thanks,
Mark.

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Matt Sealey]

On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>
> If we force the use of said bit, then new kernels cannot necessarily
> work on existing PSCI implementations (which may not handle the use of
> said bit as you expect). If we make the binding for psci-0.2 explicitly
> require the use of this bit, then future firmware/virtualisation code
> cannot provide new functionality withoot breaking backwards compatiblity
> with existing kernels, as an "arm,psci-0.2" node would be incompatible
> with an "arm,psci" node (which is all an existing kernel can handle).

Okay so fighting backwards compatibility.. what I wasn''t suggesting
was just giving bits and then knowing what the function type was from
the bit - the SMC ABI in play calls that shot. But you don't need to
(Rob's suggestion) supply two function ids for each in one property or
provide two properties for each function variant in the same node.
Doesn't that complicate parsing a little too much?

>> * An AArch64 guest under an AArch32 hypervisor/sm seems pretty weird
>> and in any case, the device tree would use the 32-bit convention (any
>> SMC64 call in 32-bit state should return "Unknown SMC Function
>> Identifier" if that bit is set)
>
> This is not allowed by the architecture.

Understood..

>> * An AArch32 guest under an AArch32  hypervisor/sm would use the
>> 32-bit convention
>> * An AArch32 guest under an AArch64 hypervisor/sm would use the 32-bit
>> convention
>
> Both of these cases are trivially mandated by the architecture.
>
>> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit
>> convention or the 32-bit convention depending on how the sm is
>> written, but it doesn't matter which is used if both can be
>> supported.. but you'd only want to use one of them.
>
> Not all implementation will implement both, so there needs to be a way
> to describe that. Which is actually used is up to the OS.

Okay but putting both ways in a single node, and describing two
function ids (per property or with two properties) is what I was
getting at.. for the one case where you can use both at once, the
device tree description is king here.

I am a little concerned that the support for this is going down the
route of telling the OS all possible ways to do the same thing instead
of trying to get into using a single, preferred way in the common
cases.

Putting both call methods in the same node, doubling the function id
property lengths, or suffixing function id properties to do it seems
like putting information in the tree purely for the sake of it. In
what cases would it (uncommon cases only being existing, pre-PCSI
pre-SMC-conventions potentially for other things). In the case of PSCI
there's an opportunity to be strict about it.. why would it be sane to
allow a mixed implementation? Dictate that either support all the
64-bit versions or all the 32-bit versions or both? And if both are
supported, dictate in the device tree which the OS should be using.

What about having two nodes? There is nothing in device trees that
says you can't have two psci { compatible="arm,psci-0.2" } nodes, one
with method=smc and one with method=smc64 or hvc64 or what have you.
Parsing and setup of the PSCI code can be quit early if it's not the
"desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then
each node follows the exact same definition, and the differentiator is
the call method and not the property names or complicating their
contents.

> As far as I can tell, no-one was arguing against this strategy. However,
> for those cases where functionality is implemented for only one
> register-width, or requires a different ID per register-width, this
> needs to be described somehow.

Do implementations like this really exist and who do we need to
waterboard to make them stop doing it? :)

-- 
Matt

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Rob Herring]

On 07/31/2013 12:24 PM, Matt Sealey wrote:
> On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:

[snip]

>>> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit
>>> convention or the 32-bit convention depending on how the sm is
>>> written, but it doesn't matter which is used if both can be
>>> supported.. but you'd only want to use one of them.
>>
>> Not all implementation will implement both, so there needs to be a way
>> to describe that. Which is actually used is up to the OS.
> 
> Okay but putting both ways in a single node, and describing two
> function ids (per property or with two properties) is what I was
> getting at.. for the one case where you can use both at once, the
> device tree description is king here.
> 
> I am a little concerned that the support for this is going down the
> route of telling the OS all possible ways to do the same thing instead
> of trying to get into using a single, preferred way in the common
> cases.
> 
> Putting both call methods in the same node, doubling the function id
> property lengths, or suffixing function id properties to do it seems
> like putting information in the tree purely for the sake of it. In
> what cases would it (uncommon cases only being existing, pre-PCSI
> pre-SMC-conventions potentially for other things). In the case of PSCI
> there's an opportunity to be strict about it.. why would it be sane to
> allow a mixed implementation? Dictate that either support all the
> 64-bit versions or all the 32-bit versions or both? And if both are
> supported, dictate in the device tree which the OS should be using.
> 
> What about having two nodes? There is nothing in device trees that
> says you can't have two psci { compatible="arm,psci-0.2" } nodes, one
> with method=smc and one with method=smc64 or hvc64 or what have you.
> Parsing and setup of the PSCI code can be quit early if it's not the
> "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then
> each node follows the exact same definition, and the differentiator is
> the call method and not the property names or complicating their
> contents.

+1

This would certainly be easier for things like a hypervisor to parse and
update.

Rob

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Dave Martin]

On Wed, Jul 31, 2013 at 12:49:20PM -0500, Rob Herring wrote:
> On 07/31/2013 12:24 PM, Matt Sealey wrote:
> > On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> 
> [snip]
> 
> >>> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit
> >>> convention or the 32-bit convention depending on how the sm is
> >>> written, but it doesn't matter which is used if both can be
> >>> supported.. but you'd only want to use one of them.
> >>
> >> Not all implementation will implement both, so there needs to be a way
> >> to describe that. Which is actually used is up to the OS.
> > 
> > Okay but putting both ways in a single node, and describing two
> > function ids (per property or with two properties) is what I was
> > getting at.. for the one case where you can use both at once, the
> > device tree description is king here.
> > 
> > I am a little concerned that the support for this is going down the
> > route of telling the OS all possible ways to do the same thing instead
> > of trying to get into using a single, preferred way in the common
> > cases.
> > 
> > Putting both call methods in the same node, doubling the function id
> > property lengths, or suffixing function id properties to do it seems
> > like putting information in the tree purely for the sake of it. In
> > what cases would it (uncommon cases only being existing, pre-PCSI
> > pre-SMC-conventions potentially for other things). In the case of PSCI
> > there's an opportunity to be strict about it.. why would it be sane to
> > allow a mixed implementation? Dictate that either support all the
> > 64-bit versions or all the 32-bit versions or both? And if both are
> > supported, dictate in the device tree which the OS should be using.

The DT is about description, not policy.  If multiple usable flavours of
the PSCI interface exist, then they exist, and there's no special reason
to hide one of them from the DT that I can see.

I take the point that we shouldn't describe useless or redundant
information for no good reason, though.

> > What about having two nodes? There is nothing in device trees that
> > says you can't have two psci { compatible="arm,psci-0.2" } nodes, one
> > with method=smc and one with method=smc64 or hvc64 or what have you.
> > Parsing and setup of the PSCI code can be quit early if it's not the
> > "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then
> > each node follows the exact same definition, and the differentiator is
> > the call method and not the property names or complicating their
> > contents.
> 
> +1
> 
> This would certainly be easier for things like a hypervisor to parse and
> update.

Can you elaborate on that?

Wouldn't a hypervisor always rip out and replace PSCI node(s) with
its own?  Allowing the firmware's PSCI interface to "show through" to a
guest VM sounds unwise.

I'm not sure why having multiple nodes makes this easier.

Having two nodes just moves information around.  Does it really simplify
anything?


With the multiple nodes approach, you might actually need 3 nodes if
you are providing backwards compatible support for psci-0.1 callers: one
for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1.
Maybe not though ... I'd have to think about it a bit more.

CHeers
---Dave

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Rob Herring]

On Thu, Aug 1, 2013 at 12:51 PM, Dave Martin <Dave.Martin@arm.com> wrote:
> On Wed, Jul 31, 2013 at 12:49:20PM -0500, Rob Herring wrote:
>> On 07/31/2013 12:24 PM, Matt Sealey wrote:
>> > On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>
>> [snip]
>>
>> >>> * An AArch64 guest under an AArch64 hypervisor/sm would use the 64-bit
>> >>> convention or the 32-bit convention depending on how the sm is
>> >>> written, but it doesn't matter which is used if both can be
>> >>> supported.. but you'd only want to use one of them.
>> >>
>> >> Not all implementation will implement both, so there needs to be a way
>> >> to describe that. Which is actually used is up to the OS.
>> >
>> > Okay but putting both ways in a single node, and describing two
>> > function ids (per property or with two properties) is what I was
>> > getting at.. for the one case where you can use both at once, the
>> > device tree description is king here.
>> >
>> > I am a little concerned that the support for this is going down the
>> > route of telling the OS all possible ways to do the same thing instead
>> > of trying to get into using a single, preferred way in the common
>> > cases.
>> >
>> > Putting both call methods in the same node, doubling the function id
>> > property lengths, or suffixing function id properties to do it seems
>> > like putting information in the tree purely for the sake of it. In
>> > what cases would it (uncommon cases only being existing, pre-PCSI
>> > pre-SMC-conventions potentially for other things). In the case of PSCI
>> > there's an opportunity to be strict about it.. why would it be sane to
>> > allow a mixed implementation? Dictate that either support all the
>> > 64-bit versions or all the 32-bit versions or both? And if both are
>> > supported, dictate in the device tree which the OS should be using.
>
> The DT is about description, not policy.  If multiple usable flavours of
> the PSCI interface exist, then they exist, and there's no special reason
> to hide one of them from the DT that I can see.
>
> I take the point that we shouldn't describe useless or redundant
> information for no good reason, though.
>
>> > What about having two nodes? There is nothing in device trees that
>> > says you can't have two psci { compatible="arm,psci-0.2" } nodes, one
>> > with method=smc and one with method=smc64 or hvc64 or what have you.
>> > Parsing and setup of the PSCI code can be quit early if it's not the
>> > "desirable" method for the OS (i.e. 64-bit on a 32-bit kernel). Then
>> > each node follows the exact same definition, and the differentiator is
>> > the call method and not the property names or complicating their
>> > contents.
>>
>> +1
>>
>> This would certainly be easier for things like a hypervisor to parse and
>> update.
>
> Can you elaborate on that?
>
> Wouldn't a hypervisor always rip out and replace PSCI node(s) with
> its own?  Allowing the firmware's PSCI interface to "show through" to a
> guest VM sounds unwise.

Perhaps. Minimally, the hypervisor could just replace smc with hvc for
the method.

> I'm not sure why having multiple nodes makes this easier.
>
> Having two nodes just moves information around.  Does it really simplify
> anything?

You can add 'status = "disabled";' easily and have it apply to the
whole node. Also, if you split-up by method, then it is clear which
properties apply to which method. I don't really care for the <none>,
-32 and -64 distinction on function ID properties.

> With the multiple nodes approach, you might actually need 3 nodes if
> you are providing backwards compatible support for psci-0.1 callers: one
> for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1.
> Maybe not though ... I'd have to think about it a bit more.

I'd hope we can transition away from psci-0.1. The things that rely on
it are not really mature anyway.

Rob

[PATCH 1/7] dt: update PSCI binding documentation for v0.2

[Matt Sealey]

>On Thu, Aug 1, 2013 at 2:02 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Thu, Aug 1, 2013 at 12:51 PM, Dave Martin <Dave.Martin@arm.com> wrote:
>> On Wed, Jul 31, 2013 at 12:49:20PM -0500, Rob Herring wrote:
>>> On 07/31/2013 12:24 PM, Matt Sealey wrote:
>>> > On Wed, Jul 31, 2013 at 8:49 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>
>>> [snip]
>>>
>>
>> The DT is about description, not policy.  If multiple usable flavours of
>> the PSCI interface exist, then they exist, and there's no special reason
>> to hide one of them from the DT that I can see.
>>
>> I take the point that we shouldn't describe useless or redundant
>> information for no good reason, though.

Good, that's the point I was trying to get across ;)

Ideally though, the DT describes what is there to be used, not what is
just there. If we had DT describing an entire SoC, on most of them
half of it would be redundant, disabled devices which cannot be muxed
out simultaneously with the active ones. There aren't many reasons to
do so (and electrically, very few ways you can even do it at runtime
anyway for the vast majority of things).

What I am trying to figure out is if someone implements PSCI 0.1, 0.2
or some future version, then the device tree could explain those - one
for each version. As Rob pointed out, you can status=disabled the ones
you don't want people to use (or, leave them all in...). Maybe the
idea here would be not to version the compatible property but use
"model" (standard property!) for the version of PSCI, or an encoded
version. If you parse PSCI nodes and find a 0.2 node, initialize it..
then stop looking. If your secure monitor guys didn't implement all of
PSCI 0.2 you should never have defined a node for it in the first
place. It's not useful to half-describe something in the device tree -
device trees are meant to remove the guesswork of just having some
chip and some firmware interface, and simplify finding and using the
correct functional blocks and interfaces.

I can't imagine a situation where someone would implement half of PSCI
0.2 and fall back to PSCI 0.1 in another node for other functions..

.. or implement half 32-bit and half 64-bit functions for a system,
except at the point PSCI specifically falls down in that (PSCI 0.2
p5.4.1);.

"""The SMC calling convention [4], provides support for calls using
only 32-bit parameters (SMC32), and for calls using 32 and 64-bit
parameters (SMC64). Some of the PSCI functions defined above use only
SMC32, some of the functions have both an SMC32 and an SMC64 version.

For PSCI functions that use only 32-bit parameters, the arguments are
passed in R0 to R3 (AArch32) or W0 to W3 (AArch64), with return values
in R0 or W0.

For versions using 64-bit parameters, the arguments are passed in X0
to X3, with return values in X0/W0 (depending on the return parameter
size)."""

Yes, some of them aren't defined as smc64 functions. This sooooo needs
to be fixed for PSCI 0.3.. in essence the spec implies that 64-bit
variants are a second cousin twice removed of the really important
32-bit ABI. The spec itself dictates that the function IDs are
identical except for the magic 64-bit 'bit'.

>> Wouldn't a hypervisor always rip out and replace PSCI node(s) with
>> its own?  Allowing the firmware's PSCI interface to "show through" to a
>> guest VM sounds unwise.
>
> Perhaps. Minimally, the hypervisor could just replace smc with hvc for
> the method.

And that would be a special case for the hypervisor. Since the calling
convention is *identical* apart from the instruction that causes that
privilege level jump, this is quite easily afforded by the
implementation.

I still think that, as documented, hypervisors should just trap smc as
the way in. It may be a pain in the backside to decode the immediate
value, but this all got discussed and changed with syscalls and svc
anyway a long while back - that immediate value is a monster and since
there are some defined for very specific vendor interfaces (like
semi-hosting) it is unwise to even use it in case some other magic
firmware/debug/emulation process decides it is going to override your
trap and go do something else than you intended. I don't think the smc
calling convention really takes that into account, it just assumes
you're trustworthy enough to not implement things like that and take
it all into account. PSCI actually recommends you use 0 for that
immediate value..

Same section as above:

"""In line with the SMC calling convention, the immediate value used
with an SMC instruction must be 0.

ARM recommends that a hypervisor providing a PSCI functions through an
HVC conduit uses the same approach to the immediate value and
parameter passing. This aids the generalization of client code."""

So, no need to decode smc #imm4, just do what it says in the
registers. And no need for a Xen DomU to know it is even under a
hypervisor, either, by having special code to cover the case.

For what a particular hypervisor needs to do, if it intends to run on
ARM, it should be tending to use a particular range of SMC function
IDs (OEM services?) which are in any event going to be hypervisor
specific anyway (Xen, QEMU, OKL4, whatever) and PSCI specifically
keeps well away from them..

>> I'm not sure why having multiple nodes makes this easier.
>>
>> Having two nodes just moves information around.  Does it really simplify
>> anything?
>
> You can add 'status = "disabled";' easily and have it apply to the
> whole node. Also, if you split-up by method, then it is clear which
> properties apply to which method. I don't really care for the <none>,
> -32 and -64 distinction on function ID properties.
>
>> With the multiple nodes approach, you might actually need 3 nodes if
>> you are providing backwards compatible support for psci-0.1 callers: one
>> for psci-0.2 32-bit, one for psci-0.2 64-bit, and one for psci-0.1.
>> Maybe not though ... I'd have to think about it a bit more.
>
> I'd hope we can transition away from psci-0.1. The things that rely on
> it are not really mature anyway.

Right, but I have to agree with Mark that backwards compatibility is a
key concept here too. It can be left in - if you supply psci-0.2
nodes, new kernels that support it can use those. Older kernels will
find the older nodes and keep working. Even newer kernels with even
newer nodes will use those.. in my opinion it shouldn't bind to
psci-0.3 and psci-0.2 at the same time, though, and the trade-off is
at the vendor level. If you want to have psci-0.2 ONLY, then they'd
have to either provide patches for older kernels or just put a lower
limit on the version you support.

The whole "there are 64-bit functions but some of them aren't" thing
is just a mess waiting to happen, though. Who's responsible for the
spec? :D

-- Matt