[PATCH v3 0/4] Use ARM SMC Calling Convention when OP-TEE is available

[PATCH v3 0/4] Use ARM SMC Calling Convention when OP-TEE is available

[Andrew F. Davis]

Hello all,

This is the reworked patch turned into a series to allow upstream kernels
to make use of OP-TEE on the OMAP2+ platform.

Thanks,
Andrew

Changes from v1:
 - Split into logical patches
 - Check for OP-TEE in DT only once
 - Check the OP-TEE node is "okay"

Changes from v2:
 - Add HS patch using 'optee_available'

Andrew F. Davis (4):
  ARM: OMAP2+: Add omap_secure_init callback hook for secure
    initialization
  ARM: OMAP2+: Introduce check for OP-TEE in omap_secure_init()
  ARM: OMAP2+: Use ARM SMC Calling Convention when OP-TEE is available
  ARM: OMAP2+: sleep43xx: Call secure suspend/resume handlers

 arch/arm/mach-omap2/common.h      |  2 +-
 arch/arm/mach-omap2/io.c          | 11 ++++++++
 arch/arm/mach-omap2/omap-secure.c | 45 +++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/omap-secure.h | 10 +++++++
 arch/arm/mach-omap2/omap-smc.S    |  6 ++---
 arch/arm/mach-omap2/pm33xx-core.c | 17 ++++++++++++
 6 files changed, 87 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] ARM: OMAP2+: Add omap_secure_init callback hook for secure initialization

[Andrew F. Davis]

This can be used for detecting secure features or making early device
init sequence changes based on device security type.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 arch/arm/mach-omap2/io.c          | 11 +++++++++++
 arch/arm/mach-omap2/omap-secure.c |  4 ++++
 arch/arm/mach-omap2/omap-secure.h |  2 ++
 3 files changed, 17 insertions(+)

diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 349e48042982..f28047233665 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -51,6 +51,7 @@
 #include "prm33xx.h"
 #include "prm44xx.h"
 #include "opp2xxx.h"
+#include "omap-secure.h"
 
 /*
  * omap_clk_soc_init: points to a function that does the SoC-specific
@@ -430,6 +431,7 @@ void __init omap2420_init_early(void)
 	omap_hwmod_init_postsetup();
 	omap_clk_soc_init = omap2420_dt_clk_init;
 	rate_table = omap2420_rate_table;
+	omap_secure_init();
 }
 
 void __init omap2420_init_late(void)
@@ -454,6 +456,7 @@ void __init omap2430_init_early(void)
 	omap_hwmod_init_postsetup();
 	omap_clk_soc_init = omap2430_dt_clk_init;
 	rate_table = omap2430_rate_table;
+	omap_secure_init();
 }
 
 void __init omap2430_init_late(void)
@@ -481,6 +484,7 @@ void __init omap3_init_early(void)
 	omap3xxx_clockdomains_init();
 	omap3xxx_hwmod_init();
 	omap_hwmod_init_postsetup();
+	omap_secure_init();
 }
 
 void __init omap3430_init_early(void)
@@ -533,6 +537,7 @@ void __init ti814x_init_early(void)
 	dm814x_hwmod_init();
 	omap_hwmod_init_postsetup();
 	omap_clk_soc_init = dm814x_dt_clk_init;
+	omap_secure_init();
 }
 
 void __init ti816x_init_early(void)
@@ -549,6 +554,7 @@ void __init ti816x_init_early(void)
 	dm816x_hwmod_init();
 	omap_hwmod_init_postsetup();
 	omap_clk_soc_init = dm816x_dt_clk_init;
+	omap_secure_init();
 }
 #endif
 
@@ -566,6 +572,7 @@ void __init am33xx_init_early(void)
 	am33xx_hwmod_init();
 	omap_hwmod_init_postsetup();
 	omap_clk_soc_init = am33xx_dt_clk_init;
+	omap_secure_init();
 }
 
 void __init am33xx_init_late(void)
@@ -589,6 +596,7 @@ void __init am43xx_init_early(void)
 	omap_hwmod_init_postsetup();
 	omap_l2_cache_init();
 	omap_clk_soc_init = am43xx_dt_clk_init;
+	omap_secure_init();
 }
 
 void __init am43xx_init_late(void)
@@ -617,6 +625,7 @@ void __init omap4430_init_early(void)
 	omap_hwmod_init_postsetup();
 	omap_l2_cache_init();
 	omap_clk_soc_init = omap4xxx_dt_clk_init;
+	omap_secure_init();
 }
 
 void __init omap4430_init_late(void)
@@ -643,6 +652,7 @@ void __init omap5_init_early(void)
 	omap54xx_hwmod_init();
 	omap_hwmod_init_postsetup();
 	omap_clk_soc_init = omap5xxx_dt_clk_init;
+	omap_secure_init();
 }
 
 void __init omap5_init_late(void)
@@ -666,6 +676,7 @@ void __init dra7xx_init_early(void)
 	dra7xx_hwmod_init();
 	omap_hwmod_init_postsetup();
 	omap_clk_soc_init = dra7xx_dt_clk_init;
+	omap_secure_init();
 }
 
 void __init dra7xx_init_late(void)
diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
index 24298e47b9f1..e936732cdc4f 100644
--- a/arch/arm/mach-omap2/omap-secure.c
+++ b/arch/arm/mach-omap2/omap-secure.c
@@ -163,3 +163,7 @@ u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag)
 				      NO_FLAG,
 				      3, ptr, count, flag, 0);
 }
+
+void __init omap_secure_init(void)
+{
+}
diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
index 20046e8f8ecb..9aeeb236a224 100644
--- a/arch/arm/mach-omap2/omap-secure.h
+++ b/arch/arm/mach-omap2/omap-secure.h
@@ -72,6 +72,8 @@ extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 flag, u32 nargs,
 extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
 extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
 
+void omap_secure_init(void);
+
 #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
 void set_cntfreq(void);
 #else
-- 
2.17.1

[PATCH v3 2/4] ARM: OMAP2+: Introduce check for OP-TEE in omap_secure_init()

[Andrew F. Davis]

This check and associated flag can be used to signal the presence
of OP-TEE on the platform. This can be used to determine which
SMC calls to make to perform secure operations.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 arch/arm/mach-omap2/omap-secure.c | 14 ++++++++++++++
 arch/arm/mach-omap2/omap-secure.h |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
index e936732cdc4f..39d8070aede6 100644
--- a/arch/arm/mach-omap2/omap-secure.c
+++ b/arch/arm/mach-omap2/omap-secure.c
@@ -12,6 +12,7 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/memblock.h>
+#include <linux/of.h>
 
 #include <asm/cacheflush.h>
 #include <asm/memblock.h>
@@ -20,6 +21,18 @@
 
 static phys_addr_t omap_secure_memblock_base;
 
+bool optee_available;
+
+static void __init omap_optee_init_check(void)
+{
+	struct device_node *np;
+
+	np = of_find_node_by_path("/firmware/optee");
+	if (np && of_device_is_available(np))
+		optee_available = true;
+	of_node_put(np);
+}
+
 /**
  * omap_sec_dispatcher: Routine to dispatch low power secure
  * service routines
@@ -166,4 +179,5 @@ u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag)
 
 void __init omap_secure_init(void)
 {
+	omap_optee_init_check();
 }
diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
index 9aeeb236a224..78a1c4f04bbe 100644
--- a/arch/arm/mach-omap2/omap-secure.h
+++ b/arch/arm/mach-omap2/omap-secure.h
@@ -10,6 +10,8 @@
 #ifndef OMAP_ARCH_OMAP_SECURE_H
 #define OMAP_ARCH_OMAP_SECURE_H
 
+#include <linux/types.h>
+
 /* Monitor error code */
 #define  API_HAL_RET_VALUE_NS2S_CONVERSION_ERROR	0xFFFFFFFE
 #define  API_HAL_RET_VALUE_SERVICE_UNKNWON		0xFFFFFFFF
@@ -72,6 +74,7 @@ extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 flag, u32 nargs,
 extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
 extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
 
+extern bool optee_available;
 void omap_secure_init(void);
 
 #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
-- 
2.17.1

[PATCH v3 3/4] ARM: OMAP2+: Use ARM SMC Calling Convention when OP-TEE is available

[Andrew F. Davis]

On High-Security(HS) OMAP2+ class devices a couple actions must be
performed from the ARM TrustZone during boot. These traditionally can be
performed by calling into the secure ROM code resident in this secure
world using legacy SMC calls. Optionally OP-TEE can replace this secure
world functionality by replacing the ROM after boot. ARM recommends a
standard calling convention is used for this interaction (SMC Calling
Convention). We check for the presence of OP-TEE and use this type of
call to perform the needed actions, falling back to the legacy OMAP ROM
call if OP-TEE is not available.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 arch/arm/mach-omap2/common.h      |  2 +-
 arch/arm/mach-omap2/omap-secure.c | 27 +++++++++++++++++++++++++++
 arch/arm/mach-omap2/omap-secure.h |  2 ++
 arch/arm/mach-omap2/omap-smc.S    |  6 +++---
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
index 223b37c48389..3b1fd8e7d705 100644
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -255,7 +255,7 @@ extern void gic_dist_disable(void);
 extern void gic_dist_enable(void);
 extern bool gic_dist_disabled(void);
 extern void gic_timer_retrigger(void);
-extern void omap_smc1(u32 fn, u32 arg);
+extern void _omap_smc1(u32 fn, u32 arg);
 extern void omap4_sar_ram_init(void);
 extern void __iomem *omap4_get_sar_ram_base(void);
 extern void omap4_mpuss_early_init(void);
diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
index 39d8070aede6..3a09d860c7a9 100644
--- a/arch/arm/mach-omap2/omap-secure.c
+++ b/arch/arm/mach-omap2/omap-secure.c
@@ -8,6 +8,7 @@
  * Copyright (C) 2013 Pali Rohár <pali.rohar@gmail.com>
  */
 
+#include <linux/arm-smccc.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -17,12 +18,17 @@
 #include <asm/cacheflush.h>
 #include <asm/memblock.h>
 
+#include "common.h"
 #include "omap-secure.h"
 
 static phys_addr_t omap_secure_memblock_base;
 
 bool optee_available;
 
+#define OMAP_SIP_SMC_STD_CALL_VAL(func_num) \
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_32, \
+	ARM_SMCCC_OWNER_SIP, (func_num))
+
 static void __init omap_optee_init_check(void)
 {
 	struct device_node *np;
@@ -66,6 +72,27 @@ u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, u32 arg1, u32 arg2,
 	return ret;
 }
 
+void omap_smccc_smc(u32 fn, u32 arg)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(OMAP_SIP_SMC_STD_CALL_VAL(fn), arg,
+		      0, 0, 0, 0, 0, 0, &res);
+	WARN(res.a0, "Secure function call 0x%08x failed\n", fn);
+}
+
+void omap_smc1(u32 fn, u32 arg)
+{
+	/*
+	 * If this platform has OP-TEE installed we use ARM SMC calls
+	 * otherwise fall back to the OMAP ROM style calls.
+	 */
+	if (optee_available)
+		omap_smccc_smc(fn, arg);
+	else
+		_omap_smc1(fn, arg);
+}
+
 /* Allocate the memory to save secure ram */
 int __init omap_secure_ram_reserve_memblock(void)
 {
diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
index 78a1c4f04bbe..736e594365f4 100644
--- a/arch/arm/mach-omap2/omap-secure.h
+++ b/arch/arm/mach-omap2/omap-secure.h
@@ -62,6 +62,8 @@
 
 extern u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs,
 				u32 arg1, u32 arg2, u32 arg3, u32 arg4);
+extern void omap_smccc_smc(u32 fn, u32 arg);
+extern void omap_smc1(u32 fn, u32 arg);
 extern u32 omap_smc2(u32 id, u32 falg, u32 pargs);
 extern u32 omap_smc3(u32 id, u32 process, u32 flag, u32 pargs);
 extern phys_addr_t omap_secure_ram_mempool_base(void);
diff --git a/arch/arm/mach-omap2/omap-smc.S b/arch/arm/mach-omap2/omap-smc.S
index fd2bcd91f4a1..d4832845a4e8 100644
--- a/arch/arm/mach-omap2/omap-smc.S
+++ b/arch/arm/mach-omap2/omap-smc.S
@@ -18,18 +18,18 @@
  * the monitor API number. It uses few CPU registers
  * internally and hence they need be backed up including
  * link register "lr".
- * Function signature : void omap_smc1(u32 fn, u32 arg)
+ * Function signature : void _omap_smc1(u32 fn, u32 arg)
  */
 	.arch armv7-a
 	.arch_extension sec
-ENTRY(omap_smc1)
+ENTRY(_omap_smc1)
 	stmfd   sp!, {r2-r12, lr}
 	mov	r12, r0
 	mov 	r0, r1
 	dsb
 	smc	#0
 	ldmfd   sp!, {r2-r12, pc}
-ENDPROC(omap_smc1)
+ENDPROC(_omap_smc1)
 
 /**
  * u32 omap_smc2(u32 id, u32 falg, u32 pargs)
-- 
2.17.1

[PATCH v3 4/4] ARM: OMAP2+: sleep43xx: Call secure suspend/resume handlers

[Andrew F. Davis]

During suspend CPU context may be lost in both non-secure and secure CPU
states. The kernel can handle saving and restoring the non-secure context
but must call into the secure side to allow it to save any context it may
lose. Add these calls here.

Note that on systems with OP-TEE available the suspend call is issued to
OP-TEE using the ARM SMCCC, but the resume call is always issued to the
ROM. This is because on waking from suspend the ROM is restored as the
secure monitor. It is this resume call that instructs the ROM to restore
OP-TEE, all subsequent calls will be handled by OP-TEE and should use the
ARM SMCCC.

Signed-off-by: Andrew F. Davis <afd@ti.com>
Acked-by: Dave Gerlach <d-gerlach@ti.com>
---
 arch/arm/mach-omap2/omap-secure.h |  3 +++
 arch/arm/mach-omap2/pm33xx-core.c | 17 +++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
index 736e594365f4..ba8c486c0454 100644
--- a/arch/arm/mach-omap2/omap-secure.h
+++ b/arch/arm/mach-omap2/omap-secure.h
@@ -53,6 +53,9 @@
 #define OMAP4_PPA_L2_POR_INDEX		0x23
 #define OMAP4_PPA_CPU_ACTRL_SMP_INDEX	0x25
 
+#define AM43xx_PPA_SVC_PM_SUSPEND	0x71
+#define AM43xx_PPA_SVC_PM_RESUME	0x72
+
 /* Secure RX-51 PPA (Primary Protected Application) APIs */
 #define RX51_PPA_HWRNG			29
 #define RX51_PPA_L2_INVAL		40
diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
index f11442ed3eff..4a564f676ff9 100644
--- a/arch/arm/mach-omap2/pm33xx-core.c
+++ b/arch/arm/mach-omap2/pm33xx-core.c
@@ -28,6 +28,7 @@
 #include "prm33xx.h"
 #include "soc.h"
 #include "sram.h"
+#include "omap-secure.h"
 
 static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
 static struct clockdomain *gfx_l4ls_clkdm;
@@ -166,6 +167,16 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
 {
 	int ret = 0;
 
+	/* Suspend secure side on HS devices */
+	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
+		if (optee_available)
+			omap_smccc_smc(AM43xx_PPA_SVC_PM_SUSPEND, 0);
+		else
+			omap_secure_dispatcher(AM43xx_PPA_SVC_PM_SUSPEND,
+					       FLAG_START_CRITICAL,
+					       0, 0, 0, 0, 0);
+	}
+
 	amx3_pre_suspend_common();
 	scu_power_mode(scu_base, SCU_PM_POWEROFF);
 	ret = cpu_suspend(args, fn);
@@ -174,6 +185,12 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
 	if (!am43xx_check_off_mode_enable())
 		amx3_post_suspend_common();
 
+	/* Resume secure side on HS devices */
+	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
+		omap_secure_dispatcher(AM43xx_PPA_SVC_PM_RESUME,
+				       FLAG_START_CRITICAL,
+				       0, 0, 0, 0, 0);
+
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH v3 4/4] ARM: OMAP2+: sleep43xx: Call secure suspend/resume handlers

[Lokesh Vutla]

On 31/12/19 12:20 AM, Andrew F. Davis wrote:
> During suspend CPU context may be lost in both non-secure and secure CPU
> states. The kernel can handle saving and restoring the non-secure context
> but must call into the secure side to allow it to save any context it may
> lose. Add these calls here.
> 
> Note that on systems with OP-TEE available the suspend call is issued to
> OP-TEE using the ARM SMCCC, but the resume call is always issued to the
> ROM. This is because on waking from suspend the ROM is restored as the
> secure monitor. It is this resume call that instructs the ROM to restore
> OP-TEE, all subsequent calls will be handled by OP-TEE and should use the
> ARM SMCCC.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> Acked-by: Dave Gerlach <d-gerlach@ti.com>
> ---
>  arch/arm/mach-omap2/omap-secure.h |  3 +++
>  arch/arm/mach-omap2/pm33xx-core.c | 17 +++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
> index 736e594365f4..ba8c486c0454 100644
> --- a/arch/arm/mach-omap2/omap-secure.h
> +++ b/arch/arm/mach-omap2/omap-secure.h
> @@ -53,6 +53,9 @@
>  #define OMAP4_PPA_L2_POR_INDEX		0x23
>  #define OMAP4_PPA_CPU_ACTRL_SMP_INDEX	0x25
>  
> +#define AM43xx_PPA_SVC_PM_SUSPEND	0x71
> +#define AM43xx_PPA_SVC_PM_RESUME	0x72
> +
>  /* Secure RX-51 PPA (Primary Protected Application) APIs */
>  #define RX51_PPA_HWRNG			29
>  #define RX51_PPA_L2_INVAL		40
> diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
> index f11442ed3eff..4a564f676ff9 100644
> --- a/arch/arm/mach-omap2/pm33xx-core.c
> +++ b/arch/arm/mach-omap2/pm33xx-core.c
> @@ -28,6 +28,7 @@
>  #include "prm33xx.h"
>  #include "soc.h"
>  #include "sram.h"
> +#include "omap-secure.h"
>  
>  static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
>  static struct clockdomain *gfx_l4ls_clkdm;
> @@ -166,6 +167,16 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>  {
>  	int ret = 0;
>  
> +	/* Suspend secure side on HS devices */
> +	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
> +		if (optee_available)
> +			omap_smccc_smc(AM43xx_PPA_SVC_PM_SUSPEND, 0);
> +		else
> +			omap_secure_dispatcher(AM43xx_PPA_SVC_PM_SUSPEND,
> +					       FLAG_START_CRITICAL,
> +					       0, 0, 0, 0, 0);
> +	}
> +
>  	amx3_pre_suspend_common();
>  	scu_power_mode(scu_base, SCU_PM_POWEROFF);
>  	ret = cpu_suspend(args, fn);
> @@ -174,6 +185,12 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>  	if (!am43xx_check_off_mode_enable())
>  		amx3_post_suspend_common();
>  
> +	/* Resume secure side on HS devices */
> +	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
> +		omap_secure_dispatcher(AM43xx_PPA_SVC_PM_RESUME,
> +				       FLAG_START_CRITICAL,
> +				       0, 0, 0, 0, 0);

Don't you need to check optee_available here?

Thanks and regards,
Lokesh

> +
>  	return ret;
>  }
>  
> 

Re: [PATCH v3 2/4] ARM: OMAP2+: Introduce check for OP-TEE in omap_secure_init()

[Lokesh Vutla]

On 31/12/19 12:20 AM, Andrew F. Davis wrote:
> This check and associated flag can be used to signal the presence
> of OP-TEE on the platform. This can be used to determine which
> SMC calls to make to perform secure operations.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  arch/arm/mach-omap2/omap-secure.c | 14 ++++++++++++++
>  arch/arm/mach-omap2/omap-secure.h |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
> index e936732cdc4f..39d8070aede6 100644
> --- a/arch/arm/mach-omap2/omap-secure.c
> +++ b/arch/arm/mach-omap2/omap-secure.c
> @@ -12,6 +12,7 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/memblock.h>
> +#include <linux/of.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/memblock.h>
> @@ -20,6 +21,18 @@
>  
>  static phys_addr_t omap_secure_memblock_base;
>  
> +bool optee_available;
> +
> +static void __init omap_optee_init_check(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_node_by_path("/firmware/optee");
> +	if (np && of_device_is_available(np))

This doesn't guarantee that optee driver is probed successfully or firmware
installed correctly. Isn't there a better way to detect? Doesn't tee core layer
exposes anything?

Thanks and regards,
Lokesh

> +		optee_available = true;
> +	of_node_put(np);
> +}
> +
>  /**
>   * omap_sec_dispatcher: Routine to dispatch low power secure
>   * service routines
> @@ -166,4 +179,5 @@ u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag)
>  
>  void __init omap_secure_init(void)
>  {
> +	omap_optee_init_check();
>  }
> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
> index 9aeeb236a224..78a1c4f04bbe 100644
> --- a/arch/arm/mach-omap2/omap-secure.h
> +++ b/arch/arm/mach-omap2/omap-secure.h
> @@ -10,6 +10,8 @@
>  #ifndef OMAP_ARCH_OMAP_SECURE_H
>  #define OMAP_ARCH_OMAP_SECURE_H
>  
> +#include <linux/types.h>
> +
>  /* Monitor error code */
>  #define  API_HAL_RET_VALUE_NS2S_CONVERSION_ERROR	0xFFFFFFFE
>  #define  API_HAL_RET_VALUE_SERVICE_UNKNWON		0xFFFFFFFF
> @@ -72,6 +74,7 @@ extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 flag, u32 nargs,
>  extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
>  extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
>  
> +extern bool optee_available;
>  void omap_secure_init(void);
>  
>  #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
> 

Re: [PATCH v3 2/4] ARM: OMAP2+: Introduce check for OP-TEE in omap_secure_init()

[Andrew F. Davis]

On 12/31/19 1:32 AM, Lokesh Vutla wrote:
> 
> 
> On 31/12/19 12:20 AM, Andrew F. Davis wrote:
>> This check and associated flag can be used to signal the presence
>> of OP-TEE on the platform. This can be used to determine which
>> SMC calls to make to perform secure operations.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>  arch/arm/mach-omap2/omap-secure.c | 14 ++++++++++++++
>>  arch/arm/mach-omap2/omap-secure.h |  3 +++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/omap-secure.c b/arch/arm/mach-omap2/omap-secure.c
>> index e936732cdc4f..39d8070aede6 100644
>> --- a/arch/arm/mach-omap2/omap-secure.c
>> +++ b/arch/arm/mach-omap2/omap-secure.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/init.h>
>>  #include <linux/io.h>
>>  #include <linux/memblock.h>
>> +#include <linux/of.h>
>>  
>>  #include <asm/cacheflush.h>
>>  #include <asm/memblock.h>
>> @@ -20,6 +21,18 @@
>>  
>>  static phys_addr_t omap_secure_memblock_base;
>>  
>> +bool optee_available;
>> +
>> +static void __init omap_optee_init_check(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	np = of_find_node_by_path("/firmware/optee");
>> +	if (np && of_device_is_available(np))
> 
> This doesn't guarantee that optee driver is probed successfully or firmware
> installed correctly. Isn't there a better way to detect? Doesn't tee core layer
> exposes anything?


We don't actually need the kernel-side OP-TEE driver at all here, we are
making raw SMCCC calls which get handled by OP-TEE using platform
specific code then emulates the function previously handled by ROM[0]
and execution is returned. No driver involved for these types of calls.

U-Boot will not add this node to the DT unless OP-TEE is installed
correctly, but you are right that is no perfect guarantee. OP-TEE's
kernel driver does do a handshake to verify it is working but this is
not exposed outside of that driver and happens *way* too late for our
uses here. Plus as above, we don't need the OP-TEE driver at all and we
should boot the same without it even enabled.

So my opinion is that if DT says OP-TEE is installed, but it is not,
then that is a misconfiguration and we usually just have to trust DT for
most things. If DT is wrong here then the only thing that happens is
this call safely fails, a message is printed informing the user of the
problem, and kernel keeps booting (although probably not stable given we
need these calls for important system configuration).

Andrew

[0]
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-ti/sm_platform_handler_a9.c
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/plat-ti/sm_platform_handler_a15.c


> 
> Thanks and regards,
> Lokesh
> 
>> +		optee_available = true;
>> +	of_node_put(np);
>> +}
>> +
>>  /**
>>   * omap_sec_dispatcher: Routine to dispatch low power secure
>>   * service routines
>> @@ -166,4 +179,5 @@ u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag)
>>  
>>  void __init omap_secure_init(void)
>>  {
>> +	omap_optee_init_check();
>>  }
>> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
>> index 9aeeb236a224..78a1c4f04bbe 100644
>> --- a/arch/arm/mach-omap2/omap-secure.h
>> +++ b/arch/arm/mach-omap2/omap-secure.h
>> @@ -10,6 +10,8 @@
>>  #ifndef OMAP_ARCH_OMAP_SECURE_H
>>  #define OMAP_ARCH_OMAP_SECURE_H
>>  
>> +#include <linux/types.h>
>> +
>>  /* Monitor error code */
>>  #define  API_HAL_RET_VALUE_NS2S_CONVERSION_ERROR	0xFFFFFFFE
>>  #define  API_HAL_RET_VALUE_SERVICE_UNKNWON		0xFFFFFFFF
>> @@ -72,6 +74,7 @@ extern u32 rx51_secure_dispatcher(u32 idx, u32 process, u32 flag, u32 nargs,
>>  extern u32 rx51_secure_update_aux_cr(u32 set_bits, u32 clear_bits);
>>  extern u32 rx51_secure_rng_call(u32 ptr, u32 count, u32 flag);
>>  
>> +extern bool optee_available;
>>  void omap_secure_init(void);
>>  
>>  #ifdef CONFIG_SOC_HAS_REALTIME_COUNTER
>>

Re: [PATCH v3 4/4] ARM: OMAP2+: sleep43xx: Call secure suspend/resume handlers

[Andrew F. Davis]

On 12/31/19 1:20 AM, Lokesh Vutla wrote:
> 
> 
> On 31/12/19 12:20 AM, Andrew F. Davis wrote:
>> During suspend CPU context may be lost in both non-secure and secure CPU
>> states. The kernel can handle saving and restoring the non-secure context
>> but must call into the secure side to allow it to save any context it may
>> lose. Add these calls here.
>>
>> Note that on systems with OP-TEE available the suspend call is issued to
>> OP-TEE using the ARM SMCCC, but the resume call is always issued to the
>> ROM. This is because on waking from suspend the ROM is restored as the
>> secure monitor. It is this resume call that instructs the ROM to restore
>> OP-TEE, all subsequent calls will be handled by OP-TEE and should use the
>> ARM SMCCC.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> Acked-by: Dave Gerlach <d-gerlach@ti.com>
>> ---
>>  arch/arm/mach-omap2/omap-secure.h |  3 +++
>>  arch/arm/mach-omap2/pm33xx-core.c | 17 +++++++++++++++++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
>> index 736e594365f4..ba8c486c0454 100644
>> --- a/arch/arm/mach-omap2/omap-secure.h
>> +++ b/arch/arm/mach-omap2/omap-secure.h
>> @@ -53,6 +53,9 @@
>>  #define OMAP4_PPA_L2_POR_INDEX		0x23
>>  #define OMAP4_PPA_CPU_ACTRL_SMP_INDEX	0x25
>>  
>> +#define AM43xx_PPA_SVC_PM_SUSPEND	0x71
>> +#define AM43xx_PPA_SVC_PM_RESUME	0x72
>> +
>>  /* Secure RX-51 PPA (Primary Protected Application) APIs */
>>  #define RX51_PPA_HWRNG			29
>>  #define RX51_PPA_L2_INVAL		40
>> diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
>> index f11442ed3eff..4a564f676ff9 100644
>> --- a/arch/arm/mach-omap2/pm33xx-core.c
>> +++ b/arch/arm/mach-omap2/pm33xx-core.c
>> @@ -28,6 +28,7 @@
>>  #include "prm33xx.h"
>>  #include "soc.h"
>>  #include "sram.h"
>> +#include "omap-secure.h"
>>  
>>  static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
>>  static struct clockdomain *gfx_l4ls_clkdm;
>> @@ -166,6 +167,16 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>>  {
>>  	int ret = 0;
>>  
>> +	/* Suspend secure side on HS devices */
>> +	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
>> +		if (optee_available)
>> +			omap_smccc_smc(AM43xx_PPA_SVC_PM_SUSPEND, 0);
>> +		else
>> +			omap_secure_dispatcher(AM43xx_PPA_SVC_PM_SUSPEND,
>> +					       FLAG_START_CRITICAL,
>> +					       0, 0, 0, 0, 0);
>> +	}
>> +
>>  	amx3_pre_suspend_common();
>>  	scu_power_mode(scu_base, SCU_PM_POWEROFF);
>>  	ret = cpu_suspend(args, fn);
>> @@ -174,6 +185,12 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>>  	if (!am43xx_check_off_mode_enable())
>>  		amx3_post_suspend_common();
>>  
>> +	/* Resume secure side on HS devices */
>> +	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
>> +		omap_secure_dispatcher(AM43xx_PPA_SVC_PM_RESUME,
>> +				       FLAG_START_CRITICAL,
>> +				       0, 0, 0, 0, 0);
> 
> Don't you need to check optee_available here?
> 


I address this in the second paragraph of the commit message. I can add
a comment in code also if you think anyone will find it confusing.

Andrew


> Thanks and regards,
> Lokesh
> 
>> +
>>  	return ret;
>>  }
>>  
>>

Re: [PATCH v3 4/4] ARM: OMAP2+: sleep43xx: Call secure suspend/resume handlers

[Lokesh Vutla]

On 31/12/19 7:47 PM, Andrew F. Davis wrote:
> On 12/31/19 1:20 AM, Lokesh Vutla wrote:
>>
>>
>> On 31/12/19 12:20 AM, Andrew F. Davis wrote:
>>> During suspend CPU context may be lost in both non-secure and secure CPU
>>> states. The kernel can handle saving and restoring the non-secure context
>>> but must call into the secure side to allow it to save any context it may
>>> lose. Add these calls here.
>>>
>>> Note that on systems with OP-TEE available the suspend call is issued to
>>> OP-TEE using the ARM SMCCC, but the resume call is always issued to the
>>> ROM. This is because on waking from suspend the ROM is restored as the
>>> secure monitor. It is this resume call that instructs the ROM to restore
>>> OP-TEE, all subsequent calls will be handled by OP-TEE and should use the
>>> ARM SMCCC.
>>>
>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>>> Acked-by: Dave Gerlach <d-gerlach@ti.com>
>>> ---
>>>  arch/arm/mach-omap2/omap-secure.h |  3 +++
>>>  arch/arm/mach-omap2/pm33xx-core.c | 17 +++++++++++++++++
>>>  2 files changed, 20 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap-secure.h b/arch/arm/mach-omap2/omap-secure.h
>>> index 736e594365f4..ba8c486c0454 100644
>>> --- a/arch/arm/mach-omap2/omap-secure.h
>>> +++ b/arch/arm/mach-omap2/omap-secure.h
>>> @@ -53,6 +53,9 @@
>>>  #define OMAP4_PPA_L2_POR_INDEX		0x23
>>>  #define OMAP4_PPA_CPU_ACTRL_SMP_INDEX	0x25
>>>  
>>> +#define AM43xx_PPA_SVC_PM_SUSPEND	0x71
>>> +#define AM43xx_PPA_SVC_PM_RESUME	0x72
>>> +
>>>  /* Secure RX-51 PPA (Primary Protected Application) APIs */
>>>  #define RX51_PPA_HWRNG			29
>>>  #define RX51_PPA_L2_INVAL		40
>>> diff --git a/arch/arm/mach-omap2/pm33xx-core.c b/arch/arm/mach-omap2/pm33xx-core.c
>>> index f11442ed3eff..4a564f676ff9 100644
>>> --- a/arch/arm/mach-omap2/pm33xx-core.c
>>> +++ b/arch/arm/mach-omap2/pm33xx-core.c
>>> @@ -28,6 +28,7 @@
>>>  #include "prm33xx.h"
>>>  #include "soc.h"
>>>  #include "sram.h"
>>> +#include "omap-secure.h"
>>>  
>>>  static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm;
>>>  static struct clockdomain *gfx_l4ls_clkdm;
>>> @@ -166,6 +167,16 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>>>  {
>>>  	int ret = 0;
>>>  
>>> +	/* Suspend secure side on HS devices */
>>> +	if (omap_type() != OMAP2_DEVICE_TYPE_GP) {
>>> +		if (optee_available)
>>> +			omap_smccc_smc(AM43xx_PPA_SVC_PM_SUSPEND, 0);
>>> +		else
>>> +			omap_secure_dispatcher(AM43xx_PPA_SVC_PM_SUSPEND,
>>> +					       FLAG_START_CRITICAL,
>>> +					       0, 0, 0, 0, 0);
>>> +	}
>>> +
>>>  	amx3_pre_suspend_common();
>>>  	scu_power_mode(scu_base, SCU_PM_POWEROFF);
>>>  	ret = cpu_suspend(args, fn);
>>> @@ -174,6 +185,12 @@ static int am43xx_suspend(unsigned int state, int (*fn)(unsigned long),
>>>  	if (!am43xx_check_off_mode_enable())
>>>  		amx3_post_suspend_common();
>>>  
>>> +	/* Resume secure side on HS devices */
>>> +	if (omap_type() != OMAP2_DEVICE_TYPE_GP)
>>> +		omap_secure_dispatcher(AM43xx_PPA_SVC_PM_RESUME,
>>> +				       FLAG_START_CRITICAL,
>>> +				       0, 0, 0, 0, 0);
>>
>> Don't you need to check optee_available here?
>>
> 
> 
> I address this in the second paragraph of the commit message. I can add
> a comment in code also if you think anyone will find it confusing.

Yeah, a comment would help.

Thanks and regards,
Lokesh

> 
> Andrew
> 
> 
>> Thanks and regards,
>> Lokesh
>>
>>> +
>>>  	return ret;
>>>  }
>>>  
>>>

Re: [PATCH v3 2/4] ARM: OMAP2+: Introduce check for OP-TEE in omap_secure_init()

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [191231 14:16]:
> On 12/31/19 1:32 AM, Lokesh Vutla wrote:
> > This doesn't guarantee that optee driver is probed successfully or firmware
> > installed correctly. Isn't there a better way to detect? Doesn't tee core layer
> > exposes anything?
> 
> We don't actually need the kernel-side OP-TEE driver at all here, we are
> making raw SMCCC calls which get handled by OP-TEE using platform
> specific code then emulates the function previously handled by ROM[0]
> and execution is returned. No driver involved for these types of calls.
> 
> U-Boot will not add this node to the DT unless OP-TEE is installed
> correctly, but you are right that is no perfect guarantee. OP-TEE's
> kernel driver does do a handshake to verify it is working but this is
> not exposed outside of that driver and happens *way* too late for our
> uses here. Plus as above, we don't need the OP-TEE driver at all and we
> should boot the same without it even enabled.
> 
> So my opinion is that if DT says OP-TEE is installed, but it is not,
> then that is a misconfiguration and we usually just have to trust DT for
> most things. If DT is wrong here then the only thing that happens is
> this call safely fails, a message is printed informing the user of the
> problem, and kernel keeps booting (although probably not stable given we
> need these calls for important system configuration).

OK, please add comments to omap_optee_init_check(), it's not obvious
to anybody not dealing with optee directly.

Regards,

Tony

Re: [PATCH v3 2/4] ARM: OMAP2+: Introduce check for OP-TEE in omap_secure_init()

[Andrew F. Davis]

On 1/2/20 12:14 PM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [191231 14:16]:
>> On 12/31/19 1:32 AM, Lokesh Vutla wrote:
>>> This doesn't guarantee that optee driver is probed successfully or firmware
>>> installed correctly. Isn't there a better way to detect? Doesn't tee core layer
>>> exposes anything?
>>
>> We don't actually need the kernel-side OP-TEE driver at all here, we are
>> making raw SMCCC calls which get handled by OP-TEE using platform
>> specific code then emulates the function previously handled by ROM[0]
>> and execution is returned. No driver involved for these types of calls.
>>
>> U-Boot will not add this node to the DT unless OP-TEE is installed
>> correctly, but you are right that is no perfect guarantee. OP-TEE's
>> kernel driver does do a handshake to verify it is working but this is
>> not exposed outside of that driver and happens *way* too late for our
>> uses here. Plus as above, we don't need the OP-TEE driver at all and we
>> should boot the same without it even enabled.
>>
>> So my opinion is that if DT says OP-TEE is installed, but it is not,
>> then that is a misconfiguration and we usually just have to trust DT for
>> most things. If DT is wrong here then the only thing that happens is
>> this call safely fails, a message is printed informing the user of the
>> problem, and kernel keeps booting (although probably not stable given we
>> need these calls for important system configuration).
> 
> OK, please add comments to omap_optee_init_check(), it's not obvious
> to anybody not dealing with optee directly.
> 


Okay, will add this comment and the one suggested by Lokesh for v4.

Andrew


> Regards,
> 
> Tony
>