[PATCH v2 0/3] ARM: tegra: add basic support for Trusted Foundations

[PATCH v2 0/3] ARM: tegra: add basic support for Trusted Foundations

[Alexandre Courbot]

New revision of the initial patch, fixed according to the many suggestions
received. (thanks!)

Changes since v1:
- Split patch into logical chunks as suggested by Tomasz
- Simplified smc function according to comments from Russel and David
- Use proper "Trusted Foundations" naming for firmware instead of "SecureOS"
- Very simple firmware infrastructure in mach-tegra to support the use of other
  firmwares
- Presence of secure monitor is signaled through a DT node instead of a "chosen"
  property
- Complain if support for a declared secure monitor is not compiled in

Alexandre Courbot (3):
  ARM: tegra: basic support for Trusted Foundations
  ARM: tegra: split setting of CPU reset handler
  ARM: tegra: set CPU reset handler with firmware op

 Documentation/devicetree/bindings/arm/tegra.txt    | 11 +++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 arch/arm/configs/tegra_defconfig                   |  1 +
 arch/arm/mach-tegra/Kconfig                        | 14 ++++++
 arch/arm/mach-tegra/Makefile                       |  3 ++
 arch/arm/mach-tegra/common.c                       |  2 +
 arch/arm/mach-tegra/firmware.c                     | 40 +++++++++++++++++
 arch/arm/mach-tegra/firmware.h                     | 19 ++++++++
 arch/arm/mach-tegra/reset.c                        | 40 ++++++++++++-----
 arch/arm/mach-tegra/trusted_foundations.c          | 51 ++++++++++++++++++++++
 10 files changed, 171 insertions(+), 11 deletions(-)
 create mode 100644 arch/arm/mach-tegra/firmware.c
 create mode 100644 arch/arm/mach-tegra/firmware.h
 create mode 100644 arch/arm/mach-tegra/trusted_foundations.c

-- 
1.8.3

[PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Alexandre Courbot]

Add basic support for booting secondary processors on Tegra devices
using the Trusted Foundations secure monitor.

Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/tegra.txt    | 11 +++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 arch/arm/configs/tegra_defconfig                   |  1 +
 arch/arm/mach-tegra/Kconfig                        | 14 ++++++
 arch/arm/mach-tegra/Makefile                       |  3 ++
 arch/arm/mach-tegra/common.c                       |  2 +
 arch/arm/mach-tegra/firmware.c                     | 40 +++++++++++++++++
 arch/arm/mach-tegra/firmware.h                     | 19 ++++++++
 arch/arm/mach-tegra/trusted_foundations.c          | 51 ++++++++++++++++++++++
 9 files changed, 142 insertions(+)
 create mode 100644 arch/arm/mach-tegra/firmware.c
 create mode 100644 arch/arm/mach-tegra/firmware.h
 create mode 100644 arch/arm/mach-tegra/trusted_foundations.c

diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
index ed9c853..d59bc19 100644
--- a/Documentation/devicetree/bindings/arm/tegra.txt
+++ b/Documentation/devicetree/bindings/arm/tegra.txt
@@ -32,3 +32,14 @@ board-specific compatible values:
   nvidia,whistler
   toradex,colibri_t20-512
   toradex,iris
+
+Optional nodes
+-------------------------------------------
+
+Trusted Foundations:
+Boards using the Trusted Foundations secure monitor should signal its
+presence by declaring a node compatible with "tl,trusted-foundations":
+
+	firmware {
+		compatible = "tl,trusted-foundations";
+	};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 6931c43..c21e1e9 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -58,6 +58,7 @@ st	STMicroelectronics
 ste	ST-Ericsson
 stericsson	ST-Ericsson
 ti	Texas Instruments
+tl	Trusted Logic
 toshiba	Toshiba Corporation
 via	VIA Technologies, Inc.
 wlf	Wolfson Microelectronics
diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 4de3bce..3bf158a 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -28,6 +28,7 @@ CONFIG_ARCH_TEGRA_3x_SOC=y
 CONFIG_ARCH_TEGRA_114_SOC=y
 CONFIG_TEGRA_PCI=y
 CONFIG_TEGRA_EMC_SCALING_ENABLE=y
+CONFIG_TEGRA_TRUSTED_FOUNDATIONS=y
 CONFIG_SMP=y
 CONFIG_PREEMPT=y
 CONFIG_AEABI=y
diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index 84d72fc..055f496 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -87,4 +87,18 @@ config TEGRA_AHB
 config TEGRA_EMC_SCALING_ENABLE
 	bool "Enable scaling the memory frequency"
 
+config TEGRA_TRUSTED_FOUNDATIONS
+	bool "Trusted Foundations secure monitor support"
+	help
+	  Many Tegra devices are booted with the Trusted Foundations secure
+	  monitor active, requiring some core operations to be performed by
+	  the secure monitor instead of the kernel.
+
+	  This option allows the kernel to invoke the secure monitor when
+	  required on devices using Trusted Foundations.
+
+	  Devices using Trusted Foundations should pass a device tree containing
+	  a node compatible with "tl,trusted-foundations" to signal the presence
+	  of the secure monitor.
+
 endmenu
diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index d011f0a..9fdc004 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -12,6 +12,7 @@ obj-y					+= pm.o
 obj-y					+= reset.o
 obj-y					+= reset-handler.o
 obj-y					+= sleep.o
+obj-y					+= firmware.o
 obj-y					+= tegra.o
 obj-$(CONFIG_CPU_IDLE)			+= cpuidle.o
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra20_speedo.o
@@ -37,3 +38,5 @@ endif
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= board-harmony-pcie.o
 
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= board-paz00.o
+
+obj-$(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)	+= trusted_foundations.o
diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
index 9f852c6..4796bb0 100644
--- a/arch/arm/mach-tegra/common.c
+++ b/arch/arm/mach-tegra/common.c
@@ -37,6 +37,7 @@
 #include "sleep.h"
 #include "pm.h"
 #include "reset.h"
+#include "firmware.h"
 
 /*
  * Storage for debug-macro.S's state.
@@ -97,6 +98,7 @@ static void __init tegra_init_cache(void)
 
 void __init tegra_init_early(void)
 {
+	tegra_init_firmware();
 	tegra_cpu_reset_handler_init();
 	tegra_apb_io_init();
 	tegra_init_fuse();
diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c
new file mode 100644
index 0000000..ea861ca
--- /dev/null
+++ b/arch/arm/mach-tegra/firmware.c
@@ -0,0 +1,40 @@
+/*
+ * SecureOS support for Tegra CPUs
+ *
+ * Copyright (c) 2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/printk.h>
+#include <linux/kconfig.h>
+#include <linux/of.h>
+#include <asm/firmware.h>
+
+#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
+extern const struct firmware_ops tegra_trusted_foundations_ops;
+#endif
+
+void __init tegra_init_firmware(void)
+{
+	struct device_node *node;
+
+	if (!of_have_populated_dt())
+		return;
+
+	node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
+	if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
+		pr_warn("Trusted Foundations detected but support missing!\n");
+#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
+	else if (node)
+		register_firmware_ops(&tegra_trusted_foundations_ops);
+#endif
+}
diff --git a/arch/arm/mach-tegra/firmware.h b/arch/arm/mach-tegra/firmware.h
new file mode 100644
index 0000000..77c62fb
--- /dev/null
+++ b/arch/arm/mach-tegra/firmware.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) 2013, NVIDIA Corporation. All rights reserved.
+ *
+ * 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.
+ */
+
+#ifndef __TEGRA_FIRMWARE_H
+#define __TEGRA_FIRMWARE_H
+
+void tegra_init_firmware(void);
+
+#endif
diff --git a/arch/arm/mach-tegra/trusted_foundations.c b/arch/arm/mach-tegra/trusted_foundations.c
new file mode 100644
index 0000000..411044f
--- /dev/null
+++ b/arch/arm/mach-tegra/trusted_foundations.c
@@ -0,0 +1,51 @@
+/*
+ * SecureOS support for Tegra CPUs
+ *
+ * Copyright (c) 2013, NVIDIA Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/of.h>
+#include <asm/firmware.h>
+
+#define TF_SET_CPU_BOOT_ADDR_SMC 0xfffff200
+
+static void __attribute__((naked)) tegra_tf_generic_smc(u32 type, u32 subtype,
+							u32 arg)
+{
+	asm volatile(
+		".arch_extension	sec\n\t"
+		"stmfd	sp!, {r3 - r11, lr}\n\t"
+		__asmeq("%0", "r0")
+		__asmeq("%1", "r1")
+		__asmeq("%2", "r2")
+		"mov	r3, #0\n\t"
+		"mov	r4, #0\n\t"
+		"smc	#0\n\t"
+		"ldmfd	sp!, {r3 - r11, pc}"
+		:
+		: "r" (type), "r" (subtype), "r" (arg)
+		: "memory");
+}
+
+static int tegra_tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
+{
+	tegra_tf_generic_smc(TF_SET_CPU_BOOT_ADDR_SMC, boot_addr, 0);
+
+	return 0;
+}
+
+const struct firmware_ops tegra_trusted_foundations_ops = {
+	.set_cpu_boot_addr = tegra_tf_set_cpu_boot_addr,
+};
-- 
1.8.3

Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Dave Martin]

On Thu, Jun 13, 2013 at 06:12:23PM +0900, Alexandre Courbot wrote:
> Add basic support for booting secondary processors on Tegra devices
> using the Trusted Foundations secure monitor.
> 
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/tegra.txt    | 11 +++++
>  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>  arch/arm/configs/tegra_defconfig                   |  1 +
>  arch/arm/mach-tegra/Kconfig                        | 14 ++++++
>  arch/arm/mach-tegra/Makefile                       |  3 ++
>  arch/arm/mach-tegra/common.c                       |  2 +
>  arch/arm/mach-tegra/firmware.c                     | 40 +++++++++++++++++
>  arch/arm/mach-tegra/firmware.h                     | 19 ++++++++
>  arch/arm/mach-tegra/trusted_foundations.c          | 51 ++++++++++++++++++++++
>  9 files changed, 142 insertions(+)
>  create mode 100644 arch/arm/mach-tegra/firmware.c
>  create mode 100644 arch/arm/mach-tegra/firmware.h
>  create mode 100644 arch/arm/mach-tegra/trusted_foundations.c
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
> index ed9c853..d59bc19 100644
> --- a/Documentation/devicetree/bindings/arm/tegra.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra.txt
> @@ -32,3 +32,14 @@ board-specific compatible values:
>    nvidia,whistler
>    toradex,colibri_t20-512
>    toradex,iris
> +
> +Optional nodes
> +-------------------------------------------
> +
> +Trusted Foundations:
> +Boards using the Trusted Foundations secure monitor should signal its
> +presence by declaring a node compatible with "tl,trusted-foundations":
> +
> +	firmware {

You need to make a clear statement about whether the node name is

I think it shouldn't required to be exactly equal to "firmware", because
that would lead to problems if there were multiple independent firmware
APIs present (which is certainly possible, whether or not it is true
on Tegra).

> +		compatible = "tl,trusted-foundations";
> +	};

For now, it might make more sense to make this binding tegra-specific,
and to interpret the node is only implying the presence of the low-
level SoC functions you are using on Tegra, not TF as a whole. 

Otherwise, it feels too generic.  There is no description of exactly
what functionality will be available if this node it present: if
this is going to be a generic binding for TF, then it needs to work
for all deployments of TF, not just your specific case.  For example,
how to you find out what functionality is present?  Will there be
a standard probing ABI for all versions and deployments of TF, or
would extra information need to be described in the DT?

Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will
be present, working, and with compatible ABI and semantics, on every SoC
where an implementation of TF is present.  Someone from Trusted Logic, or
someone with visibility of the relevant ABI/API specs would need to
judge on that -- do you have that info?

> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 6931c43..c21e1e9 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -58,6 +58,7 @@ st	STMicroelectronics
>  ste	ST-Ericsson
>  stericsson	ST-Ericsson
>  ti	Texas Instruments
> +tl	Trusted Logic
>  toshiba	Toshiba Corporation
>  via	VIA Technologies, Inc.
>  wlf	Wolfson Microelectronics
> diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
> index 4de3bce..3bf158a 100644
> --- a/arch/arm/configs/tegra_defconfig
> +++ b/arch/arm/configs/tegra_defconfig
> @@ -28,6 +28,7 @@ CONFIG_ARCH_TEGRA_3x_SOC=y
>  CONFIG_ARCH_TEGRA_114_SOC=y
>  CONFIG_TEGRA_PCI=y
>  CONFIG_TEGRA_EMC_SCALING_ENABLE=y
> +CONFIG_TEGRA_TRUSTED_FOUNDATIONS=y
>  CONFIG_SMP=y
>  CONFIG_PREEMPT=y
>  CONFIG_AEABI=y
> diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
> index 84d72fc..055f496 100644
> --- a/arch/arm/mach-tegra/Kconfig
> +++ b/arch/arm/mach-tegra/Kconfig
> @@ -87,4 +87,18 @@ config TEGRA_AHB
>  config TEGRA_EMC_SCALING_ENABLE
>  	bool "Enable scaling the memory frequency"
>  
> +config TEGRA_TRUSTED_FOUNDATIONS
> +	bool "Trusted Foundations secure monitor support"
> +	help
> +	  Many Tegra devices are booted with the Trusted Foundations secure
> +	  monitor active, requiring some core operations to be performed by
> +	  the secure monitor instead of the kernel.
> +
> +	  This option allows the kernel to invoke the secure monitor when
> +	  required on devices using Trusted Foundations.
> +
> +	  Devices using Trusted Foundations should pass a device tree containing
> +	  a node compatible with "tl,trusted-foundations" to signal the presence
> +	  of the secure monitor.
> +
>  endmenu
> diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
> index d011f0a..9fdc004 100644
> --- a/arch/arm/mach-tegra/Makefile
> +++ b/arch/arm/mach-tegra/Makefile
> @@ -12,6 +12,7 @@ obj-y					+= pm.o
>  obj-y					+= reset.o
>  obj-y					+= reset-handler.o
>  obj-y					+= sleep.o
> +obj-y					+= firmware.o
>  obj-y					+= tegra.o
>  obj-$(CONFIG_CPU_IDLE)			+= cpuidle.o
>  obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra20_speedo.o
> @@ -37,3 +38,5 @@ endif
>  obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= board-harmony-pcie.o
>  
>  obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= board-paz00.o
> +
> +obj-$(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)	+= trusted_foundations.o
> diff --git a/arch/arm/mach-tegra/common.c b/arch/arm/mach-tegra/common.c
> index 9f852c6..4796bb0 100644
> --- a/arch/arm/mach-tegra/common.c
> +++ b/arch/arm/mach-tegra/common.c
> @@ -37,6 +37,7 @@
>  #include "sleep.h"
>  #include "pm.h"
>  #include "reset.h"
> +#include "firmware.h"
>  
>  /*
>   * Storage for debug-macro.S's state.
> @@ -97,6 +98,7 @@ static void __init tegra_init_cache(void)
>  
>  void __init tegra_init_early(void)
>  {
> +	tegra_init_firmware();
>  	tegra_cpu_reset_handler_init();
>  	tegra_apb_io_init();
>  	tegra_init_fuse();
> diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c
> new file mode 100644
> index 0000000..ea861ca
> --- /dev/null
> +++ b/arch/arm/mach-tegra/firmware.c
> @@ -0,0 +1,40 @@
> +/*
> + * SecureOS support for Tegra CPUs

Should the name "SecureOS" change in these comment headers? (affects
multiple files)

> + *
> + * Copyright (c) 2013, NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + */
> +
> +#include <linux/printk.h>
> +#include <linux/kconfig.h>
> +#include <linux/of.h>
> +#include <asm/firmware.h>
> +
> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
> +extern const struct firmware_ops tegra_trusted_foundations_ops;
> +#endif
> +
> +void __init tegra_init_firmware(void)
> +{
> +	struct device_node *node;
> +
> +	if (!of_have_populated_dt())
> +		return;
> +
> +	node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
> +	if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
> +		pr_warn("Trusted Foundations detected but support missing!\n");

Should this be more than just a warning?

It looks to me like tegra_cpu_reset_handler_set() might either silently
fail or trigger an external abort in this situation, depending on the
hardware and on how TF sets things up.

There seems to be no way to signal an error when attempting to set a
CPU's reset address.

> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
> +	else if (node)
> +		register_firmware_ops(&tegra_trusted_foundations_ops);
> +#endif
> +}


IS_ENABLED() allows #ifdefs to be avoided -- maybe this would be clearer
as:

	node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
	if (!node)
		return;

	if (WARN_ON(!IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))) {
		pr_warn("Trusted Foundations detected but support missing!\n");
		return;
	}

	register_firmware_ops(&tegra_trusted_foundations_ops);
	

> diff --git a/arch/arm/mach-tegra/firmware.h b/arch/arm/mach-tegra/firmware.h
> new file mode 100644
> index 0000000..77c62fb
> --- /dev/null
> +++ b/arch/arm/mach-tegra/firmware.h
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (c) 2013, NVIDIA Corporation. All rights reserved.
> + *
> + * 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.
> + */
> +
> +#ifndef __TEGRA_FIRMWARE_H
> +#define __TEGRA_FIRMWARE_H
> +
> +void tegra_init_firmware(void);
> +
> +#endif
> diff --git a/arch/arm/mach-tegra/trusted_foundations.c b/arch/arm/mach-tegra/trusted_foundations.c
> new file mode 100644
> index 0000000..411044f
> --- /dev/null
> +++ b/arch/arm/mach-tegra/trusted_foundations.c
> @@ -0,0 +1,51 @@
> +/*
> + * SecureOS support for Tegra CPUs
> + *
> + * Copyright (c) 2013, NVIDIA Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <asm/firmware.h>
> +
> +#define TF_SET_CPU_BOOT_ADDR_SMC 0xfffff200
> +
> +static void __attribute__((naked)) tegra_tf_generic_smc(u32 type, u32 subtype,
> +							u32 arg)
> +{
> +	asm volatile(
> +		".arch_extension	sec\n\t"
> +		"stmfd	sp!, {r3 - r11, lr}\n\t"

I think you don't need to save r3: the ARM PCS makes r3 caller-save, so
it shouldn't matter if the SMC call causes it to get trashed.

> +		__asmeq("%0", "r0")
> +		__asmeq("%1", "r1")
> +		__asmeq("%2", "r2")
> +		"mov	r3, #0\n\t"
> +		"mov	r4, #0\n\t"
> +		"smc	#0\n\t"
> +		"ldmfd	sp!, {r3 - r11, pc}"
> +		:
> +		: "r" (type), "r" (subtype), "r" (arg)
> +		: "memory");
> +}
> +
> +static int tegra_tf_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
> +{
> +	tegra_tf_generic_smc(TF_SET_CPU_BOOT_ADDR_SMC, boot_addr, 0);
> +
> +	return 0;
> +}
> +
> +const struct firmware_ops tegra_trusted_foundations_ops = {
> +	.set_cpu_boot_addr = tegra_tf_set_cpu_boot_addr,
> +};
> -- 
> 1.8.3
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Alexandre Courbot]

On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin <Dave.Martin-5wv7dgnIgG8@public.gmane.org> wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
>> index ed9c853..d59bc19 100644
>> --- a/Documentation/devicetree/bindings/arm/tegra.txt
>> +++ b/Documentation/devicetree/bindings/arm/tegra.txt
>> @@ -32,3 +32,14 @@ board-specific compatible values:
>>    nvidia,whistler
>>    toradex,colibri_t20-512
>>    toradex,iris
>> +
>> +Optional nodes
>> +-------------------------------------------
>> +
>> +Trusted Foundations:
>> +Boards using the Trusted Foundations secure monitor should signal its
>> +presence by declaring a node compatible with "tl,trusted-foundations":
>> +
>> +     firmware {
>
> You need to make a clear statement about whether the node name is
>
> I think it shouldn't required to be exactly equal to "firmware", because
> that would lead to problems if there were multiple independent firmware
> APIs present (which is certainly possible, whether or not it is true
> on Tegra).

Yes, the name of the node is not fixed (doing so would make its lookup
faster, but the gain is not obvious). Will make it explicit in the
doc.

>> +             compatible = "tl,trusted-foundations";
>> +     };
>
> For now, it might make more sense to make this binding tegra-specific,
> and to interpret the node is only implying the presence of the low-
> level SoC functions you are using on Tegra, not TF as a whole.

Do you mean the vendor should be changed from "tl" to "nvidia" here?

> Otherwise, it feels too generic.  There is no description of exactly
> what functionality will be available if this node it present: if
> this is going to be a generic binding for TF, then it needs to work
> for all deployments of TF, not just your specific case.  For example,
> how to you find out what functionality is present?  Will there be
> a standard probing ABI for all versions and deployments of TF, or
> would extra information need to be described in the DT?
>
> Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will
> be present, working, and with compatible ABI and semantics, on every SoC
> where an implementation of TF is present.  Someone from Trusted Logic, or
> someone with visibility of the relevant ABI/API specs would need to
> judge on that -- do you have that info?

I'm currently looking into that. This patch makes the assumption that
all TF implementations have the same features and the same interface -
if this is the case, do you agree this binding is ok as it is?

If indeed TF's functionality and ABI is the same no matter whether we
are on Tegra on not, then its support should even be moved outside of
mach-tegra.


>> --- /dev/null
>> +++ b/arch/arm/mach-tegra/firmware.c
>> @@ -0,0 +1,40 @@
>> +/*
>> + * SecureOS support for Tegra CPUs
>
> Should the name "SecureOS" change in these comment headers? (affects
> multiple files)

Yes, I missed these ones, thanks. Another missed opportunity to use git grep...

>> +     node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>> +     if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
>> +             pr_warn("Trusted Foundations detected but support missing!\n");
>
> Should this be more than just a warning?
>
> It looks to me like tegra_cpu_reset_handler_set() might either silently
> fail or trigger an external abort in this situation, depending on the
> hardware and on how TF sets things up.

What will happen (from 3.10) is that tegra_cpu_reset_handler_set()
will output a "CPUX: failed to come online" for each secondary CPU and
boot will continue (albeit on one CPU). The system's features are
degraded, but it is not fatal, so I think it is reasonable to continue
here.

> There seems to be no way to signal an error when attempting to set a
> CPU's reset address.

Indeed. But even if that fails the system can still survive, at least on Tegra.

>> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
>> +     else if (node)
>> +             register_firmware_ops(&tegra_trusted_foundations_ops);
>> +#endif
>> +}
>
>
> IS_ENABLED() allows #ifdefs to be avoided -- maybe this would be clearer
> as:
>
>         node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>         if (!node)
>                 return;
>
>         if (WARN_ON(!IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))) {
>                 pr_warn("Trusted Foundations detected but support missing!\n");
>                 return;
>         }
>
>         register_firmware_ops(&tegra_trusted_foundations_ops);

But then you will get a link error when TF support is not compiled in
because tegra_trusted_foundations_ops will not be defined. That's why
I used a #ifdef here - I agree it is terribly inelegant though.

>> +     asm volatile(
>> +             ".arch_extension        sec\n\t"
>> +             "stmfd  sp!, {r3 - r11, lr}\n\t"
>
> I think you don't need to save r3: the ARM PCS makes r3 caller-save, so
> it shouldn't matter if the SMC call causes it to get trashed.

One less register to save, I take it! :)

Thanks,
Alex.

Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Stephen Warren]

On 06/14/2013 02:43 AM, Alexandre Courbot wrote:
> On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin <Dave.Martin-5wv7dgnIgG8@public.gmane.org> wrote:
...
>>> +             compatible = "tl,trusted-foundations";
>>> +     };
>>
>> For now, it might make more sense to make this binding tegra-specific,
>> and to interpret the node is only implying the presence of the low-
>> level SoC functions you are using on Tegra, not TF as a whole.
> 
> Do you mean the vendor should be changed from "tl" to "nvidia" here?
> 
>> Otherwise, it feels too generic.  There is no description of exactly
>> what functionality will be available if this node it present: if
>> this is going to be a generic binding for TF, then it needs to work
>> for all deployments of TF, not just your specific case.  For example,
>> how to you find out what functionality is present?  Will there be
>> a standard probing ABI for all versions and deployments of TF, or
>> would extra information need to be described in the DT?
>>
>> Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will
>> be present, working, and with compatible ABI and semantics, on every SoC
>> where an implementation of TF is present.  Someone from Trusted Logic, or
>> someone with visibility of the relevant ABI/API specs would need to
>> judge on that -- do you have that info?
> 
> I'm currently looking into that. This patch makes the assumption that
> all TF implementations have the same features and the same interface -
> if this is the case, do you agree this binding is ok as it is?
> 
> If indeed TF's functionality and ABI is the same no matter whether we
> are on Tegra on not, then its support should even be moved outside of
> mach-tegra.

I expect we at least need a version number in the compatible value, even
if we don't need a representation of the SoC or vendor for which the ABI
was built.

>>> +     node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>>> +     if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
>>> +             pr_warn("Trusted Foundations detected but support missing!\n");
>>
>> Should this be more than just a warning?
>>
>> It looks to me like tegra_cpu_reset_handler_set() might either silently
>> fail or trigger an external abort in this situation, depending on the
>> hardware and on how TF sets things up.
> 
> What will happen (from 3.10) is that tegra_cpu_reset_handler_set()
> will output a "CPUX: failed to come online" for each secondary CPU and
> boot will continue (albeit on one CPU). The system's features are
> degraded, but it is not fatal, so I think it is reasonable to continue
> here.
> 
>> There seems to be no way to signal an error when attempting to set a
>> CPU's reset address.
> 
> Indeed. But even if that fails the system can still survive, at least on Tegra.

One more thought: Setting the CPU reset address isn't the only operation
that'll be performed via firmware_ops; we'd also need to e.g. disable
CPU power-gating and perhaps other things. Can that all be done at
run-time? I guess it shouldn't be hard to fix that if we can't yet.

Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Stephen Warren]

On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
> Add basic support for booting secondary processors on Tegra devices
> using the Trusted Foundations secure monitor.
> 
> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/tegra.txt    | 11 +++++
>  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>  arch/arm/configs/tegra_defconfig                   |  1 +

The defconfig change should be a separate patch, so that I can squash it
into any other defconfig updates separately from all the code changes.

> diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c

> +void __init tegra_init_firmware(void)
> +{
> +	struct device_node *node;
> +
> +	if (!of_have_populated_dt())
> +		return;
> +
> +	node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
> +	if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
> +		pr_warn("Trusted Foundations detected but support missing!\n");
> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
> +	else if (node)
> +		register_firmware_ops(&tegra_trusted_foundations_ops);
> +#endif
> +}

Is it worth continuing on in the node && !IS_ENABLED case here? After
all, we can be pretty certain that the write to the CPU reset vector is
immediately going to trap...

I suppose that perhaps without SMP, cpuidle, suspend, ... we could keep
running, but that seems a little niche.

Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Alexandre Courbot]

On Fri, Jun 14, 2013 at 4:19 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
>> Add basic support for booting secondary processors on Tegra devices
>> using the Trusted Foundations secure monitor.
>>
>> Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/arm/tegra.txt    | 11 +++++
>>  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
>>  arch/arm/configs/tegra_defconfig                   |  1 +
>
> The defconfig change should be a separate patch, so that I can squash it
> into any other defconfig updates separately from all the code changes.

Ok, moved that part into its own patch.

>> diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c
>
>> +void __init tegra_init_firmware(void)
>> +{
>> +     struct device_node *node;
>> +
>> +     if (!of_have_populated_dt())
>> +             return;
>> +
>> +     node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>> +     if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
>> +             pr_warn("Trusted Foundations detected but support missing!\n");
>> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
>> +     else if (node)
>> +             register_firmware_ops(&tegra_trusted_foundations_ops);
>> +#endif
>> +}
>
> Is it worth continuing on in the node && !IS_ENABLED case here? After
> all, we can be pretty certain that the write to the CPU reset vector is
> immediately going to trap...

That's what was happening until 3.9, but from 3.10 on the trap is
apparently handled and the boot completes (although with only one
processor).

> I suppose that perhaps without SMP, cpuidle, suspend, ... we could keep
> running, but that seems a little niche.

If we can keep running, even in degraded mode, I see no reason to
panic. The problem is well reported in the kernel log, and having a
running system might be helpful to analyze the issue.

Thanks,
Alex.

Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Stephen Warren]

On 06/14/2013 02:27 AM, Alexandre Courbot wrote:
> On Fri, Jun 14, 2013 at 4:19 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
>>> Add basic support for booting secondary processors on Tegra devices
>>> using the Trusted Foundations secure monitor.

>>> diff --git a/arch/arm/mach-tegra/firmware.c b/arch/arm/mach-tegra/firmware.c
>>
>>> +void __init tegra_init_firmware(void)
...
>>> +     node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
>>> +     if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
>>> +             pr_warn("Trusted Foundations detected but support missing!\n");
>>> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
>>> +     else if (node)
>>> +             register_firmware_ops(&tegra_trusted_foundations_ops);
>>> +#endif
>>> +}
>>
>> Is it worth continuing on in the node && !IS_ENABLED case here? After
>> all, we can be pretty certain that the write to the CPU reset vector is
>> immediately going to trap...
> 
> That's what was happening until 3.9, but from 3.10 on the trap is
> apparently handled and the boot completes (although with only one
> processor).

Why does that happen; surely the kernel shouldn't be ignoring failures?
How does it recover?

Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations

[Dave Martin]

On Fri, Jun 14, 2013 at 05:43:03PM +0900, Alexandre Courbot wrote:
> On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin <Dave.Martin-5wv7dgnIgG8@public.gmane.org> wrote:
> >> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt
> >> index ed9c853..d59bc19 100644
> >> --- a/Documentation/devicetree/bindings/arm/tegra.txt
> >> +++ b/Documentation/devicetree/bindings/arm/tegra.txt
> >> @@ -32,3 +32,14 @@ board-specific compatible values:
> >>    nvidia,whistler
> >>    toradex,colibri_t20-512
> >>    toradex,iris
> >> +
> >> +Optional nodes
> >> +-------------------------------------------
> >> +
> >> +Trusted Foundations:
> >> +Boards using the Trusted Foundations secure monitor should signal its
> >> +presence by declaring a node compatible with "tl,trusted-foundations":
> >> +
> >> +     firmware {
> >
> > You need to make a clear statement about whether the node name is
> >
> > I think it shouldn't required to be exactly equal to "firmware", because
> > that would lead to problems if there were multiple independent firmware
> > APIs present (which is certainly possible, whether or not it is true
> > on Tegra).
> 
> Yes, the name of the node is not fixed (doing so would make its lookup
> faster, but the gain is not obvious). Will make it explicit in the
> doc.
> 
> >> +             compatible = "tl,trusted-foundations";
> >> +     };
> >
> > For now, it might make more sense to make this binding tegra-specific,
> > and to interpret the node is only implying the presence of the low-
> > level SoC functions you are using on Tegra, not TF as a whole.
> 
> Do you mean the vendor should be changed from "tl" to "nvidia" here?

Since this is a Tegra-specific integration, I think that would be wise,
unless you can confirm by looking at the API specs that the functionality
you are trying to describe and use it truly intended to be generic across
all deployments of TF (either always present, or at least as a well-
specified optional feature).

> > Otherwise, it feels too generic.  There is no description of exactly
> > what functionality will be available if this node it present: if
> > this is going to be a generic binding for TF, then it needs to work
> > for all deployments of TF, not just your specific case.  For example,
> > how to you find out what functionality is present?  Will there be
> > a standard probing ABI for all versions and deployments of TF, or
> > would extra information need to be described in the DT?
> >
> > Partly, this depends on whether the TF_SET_CPU_BOOT_ADDR_SMC call will
> > be present, working, and with compatible ABI and semantics, on every SoC
> > where an implementation of TF is present.  Someone from Trusted Logic, or
> > someone with visibility of the relevant ABI/API specs would need to
> > judge on that -- do you have that info?
> 
> I'm currently looking into that. This patch makes the assumption that
> all TF implementations have the same features and the same interface -
> if this is the case, do you agree this binding is ok as it is?

It's a judgement call, but if the SMC you use is a mandatory part of
TF after a certain version, and if it's guaranteed to have a fixed ABI
(including function ID) and behaviour, then that binding works.

If the function is an optional or SoC-specific feature, then it's not
sufficient to say that TF firmware is present -- we need to describe
something about the SoC-specific and optional features present, unless
there's a well-defined generic way to probe for them.

> If indeed TF's functionality and ABI is the same no matter whether we
> are on Tegra on not, then its support should even be moved outside of
> mach-tegra.

Agreed, that's the ideal outcome.  I'm just not convinced we're ready
for that just now (but I'm happy to be corrected).

> >> --- /dev/null
> >> +++ b/arch/arm/mach-tegra/firmware.c
> >> @@ -0,0 +1,40 @@
> >> +/*
> >> + * SecureOS support for Tegra CPUs
> >
> > Should the name "SecureOS" change in these comment headers? (affects
> > multiple files)
> 
> Yes, I missed these ones, thanks. Another missed opportunity to use git grep...
> 
> >> +     node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
> >> +     if (node && !IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))
> >> +             pr_warn("Trusted Foundations detected but support missing!\n");
> >
> > Should this be more than just a warning?
> >
> > It looks to me like tegra_cpu_reset_handler_set() might either silently
> > fail or trigger an external abort in this situation, depending on the
> > hardware and on how TF sets things up.
> 
> What will happen (from 3.10) is that tegra_cpu_reset_handler_set()
> will output a "CPUX: failed to come online" for each secondary CPU and
> boot will continue (albeit on one CPU). The system's features are
> degraded, but it is not fatal, so I think it is reasonable to continue
> here.

I don't think we should rely on ignoring in imprecise external abort,
because such aborts indicate serious or potentially fatal system errors
or bugs.

If you're getting a precise abort (so that we know the faulted address)
or no abort at all, that's less harmful, though it's hard to guarantee
across all SoCs, because the ARM Architecture doesn't guarantee synchronous
aborts for slave errors on writes.

What was heppening differently on 3.9 compared with 3.10?

> > There seems to be no way to signal an error when attempting to set a
> > CPU's reset address.
> 
> Indeed. But even if that fails the system can still survive, at least on Tegra.
> 
> >> +#if IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS)
> >> +     else if (node)
> >> +             register_firmware_ops(&tegra_trusted_foundations_ops);
> >> +#endif
> >> +}
> >
> >
> > IS_ENABLED() allows #ifdefs to be avoided -- maybe this would be clearer
> > as:
> >
> >         node = of_find_compatible_node(NULL, NULL, "tl,trusted-foundations");
> >         if (!node)
> >                 return;
> >
> >         if (WARN_ON(!IS_ENABLED(CONFIG_TEGRA_TRUSTED_FOUNDATIONS))) {
> >                 pr_warn("Trusted Foundations detected but support missing!\n");
> >                 return;
> >         }
> >
> >         register_firmware_ops(&tegra_trusted_foundations_ops);
> 
> But then you will get a link error when TF support is not compiled in
> because tegra_trusted_foundations_ops will not be defined. That's why
> I used a #ifdef here - I agree it is terribly inelegant though.

Hmmm, fair point.  Dead code elimination in the compiler may solve that,
but I've never been too keen on relying on that.

Cheers
---Dave

[PATCH v2 3/3] ARM: tegra: set CPU reset handler with firmware op

[Alexandre Courbot]

Use a firmware operation to set the CPU reset handler and only resort to
doing it ourselves if there is none defined.

This supports the booting of secondary CPUs on devices using a TrustZone
secure monitor.

Signed-off-by: Alexandre Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-tegra/reset.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
index 6964117..40f110c 100644
--- a/arch/arm/mach-tegra/reset.c
+++ b/arch/arm/mach-tegra/reset.c
@@ -21,6 +21,7 @@
 
 #include <asm/cacheflush.h>
 #include <asm/hardware/cache-l2x0.h>
+#include <asm/firmware.h>
 
 #include "iomap.h"
 #include "irammap.h"
@@ -65,6 +66,7 @@ static void __init tegra_cpu_reset_handler_enable(void)
 	void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
 	const u32 reset_address = TEGRA_IRAM_RESET_BASE +
 						tegra_cpu_reset_handler_offset;
+	int err;
 
 	BUG_ON(is_enabled);
 	BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
@@ -72,9 +74,18 @@ static void __init tegra_cpu_reset_handler_enable(void)
 	memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
 			tegra_cpu_reset_handler_size);
 
-	tegra_cpu_reset_handler_set(reset_address);
-
-	is_enabled = true;
+	err = call_firmware_op(set_cpu_boot_addr, 0, reset_address);
+	switch (err) {
+	case -ENOSYS:
+		tegra_cpu_reset_handler_set(reset_address);
+		/* pass-through */
+	case 0:
+		is_enabled = true;
+		break;
+	default:
+		pr_err("Cannot set CPU reset handler: %d\n", err);
+		break;
+	}
 }
 
 void __init tegra_cpu_reset_handler_init(void)
-- 
1.8.3

Re: [PATCH v2 3/3] ARM: tegra: set CPU reset handler with firmware op

[Stephen Warren]

On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
> Use a firmware operation to set the CPU reset handler and only resort to
> doing it ourselves if there is none defined.
> 
> This supports the booting of secondary CPUs on devices using a TrustZone
> secure monitor.

> diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c

> +	err = call_firmware_op(set_cpu_boot_addr, 0, reset_address);
> +	switch (err) {
> +	case -ENOSYS:
> +		tegra_cpu_reset_handler_set(reset_address);
> +		/* pass-through */

Rather than detecting -ENOSYS and falling back to the custom
tegra_cpu_reset_handler_set(), does it make sense to plug in
tegra_cpu_reset_handler_set as the firmware op when there is no secure
firmware detected? That way, this code wouldn't need the special case;
that would be isolated to firmware.c.

Re: [PATCH v2 3/3] ARM: tegra: set CPU reset handler with firmware op

[Alexandre Courbot]

On Fri, Jun 14, 2013 at 4:23 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
>> Use a firmware operation to set the CPU reset handler and only resort to
>> doing it ourselves if there is none defined.
>>
>> This supports the booting of secondary CPUs on devices using a TrustZone
>> secure monitor.
>
>> diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
>
>> +     err = call_firmware_op(set_cpu_boot_addr, 0, reset_address);
>> +     switch (err) {
>> +     case -ENOSYS:
>> +             tegra_cpu_reset_handler_set(reset_address);
>> +             /* pass-through */
>
> Rather than detecting -ENOSYS and falling back to the custom
> tegra_cpu_reset_handler_set(), does it make sense to plug in
> tegra_cpu_reset_handler_set as the firmware op when there is no secure
> firmware detected? That way, this code wouldn't need the special case;
> that would be isolated to firmware.c.

Mmmm I admit I just followed what Exynos did without thinking much
about it. I don't see any reason why your suggestion wouldn't work,
but on second thought tegra_cpu_reset_handler_set() is not a firmware
operation - wouldn't it be unexpected (and maybe confusing) to have it
called through call_firmware_op()?

Alex.

Re: [PATCH v2 3/3] ARM: tegra: set CPU reset handler with firmware op

[Stephen Warren]

On 06/14/2013 02:54 AM, Alexandre Courbot wrote:
> On Fri, Jun 14, 2013 at 4:23 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
>>> Use a firmware operation to set the CPU reset handler and only resort to
>>> doing it ourselves if there is none defined.
>>>
>>> This supports the booting of secondary CPUs on devices using a TrustZone
>>> secure monitor.
>>
>>> diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
>>
>>> +     err = call_firmware_op(set_cpu_boot_addr, 0, reset_address);
>>> +     switch (err) {
>>> +     case -ENOSYS:
>>> +             tegra_cpu_reset_handler_set(reset_address);
>>> +             /* pass-through */
>>
>> Rather than detecting -ENOSYS and falling back to the custom
>> tegra_cpu_reset_handler_set(), does it make sense to plug in
>> tegra_cpu_reset_handler_set as the firmware op when there is no secure
>> firmware detected? That way, this code wouldn't need the special case;
>> that would be isolated to firmware.c.
> 
> Mmmm I admit I just followed what Exynos did without thinking much
> about it. I don't see any reason why your suggestion wouldn't work,
> but on second thought tegra_cpu_reset_handler_set() is not a firmware
> operation - wouldn't it be unexpected (and maybe confusing) to have it
> called through call_firmware_op()?

I would see call_firmware_op() as an abstraction that performs certain
operations, which in some cases are performed by firmware. If the
operation doesn't actually need to call into firmware in some
situations, that seems fine to me. But you're right, others may object.
Perhaps get a ruling from whoever created firmware_ops and/or some main
ARM maintainers.

[PATCH v2 2/3] ARM: tegra: split setting of CPU reset handler

[Alexandre Courbot]

Not all Tegra devices need to set the CPU reset handler in the same way.
In particular, devices using a TrustZone secure monitor cannot set the
reset handler directly and need to do it through a firmware operation.

This patch separates the act of setting the reset handler from its
preparation, so the former can be implemented in a different way.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/mach-tegra/reset.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-tegra/reset.c b/arch/arm/mach-tegra/reset.c
index 1ac434e..6964117 100644
--- a/arch/arm/mach-tegra/reset.c
+++ b/arch/arm/mach-tegra/reset.c
@@ -33,26 +33,18 @@
 
 static bool is_enabled;
 
-static void __init tegra_cpu_reset_handler_enable(void)
+static void __init tegra_cpu_reset_handler_set(const u32 reset_address)
 {
-	void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
 	void __iomem *evp_cpu_reset =
 		IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE + 0x100);
 	void __iomem *sb_ctrl = IO_ADDRESS(TEGRA_SB_BASE);
 	u32 reg;
 
-	BUG_ON(is_enabled);
-	BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
-
-	memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
-			tegra_cpu_reset_handler_size);
-
 	/*
 	 * NOTE: This must be the one and only write to the EVP CPU reset
 	 *       vector in the entire system.
 	 */
-	writel(TEGRA_IRAM_RESET_BASE + tegra_cpu_reset_handler_offset,
-			evp_cpu_reset);
+	writel(reset_address, evp_cpu_reset);
 	wmb();
 	reg = readl(evp_cpu_reset);
 
@@ -66,6 +58,21 @@ static void __init tegra_cpu_reset_handler_enable(void)
 		writel(reg, sb_ctrl);
 		wmb();
 	}
+}
+
+static void __init tegra_cpu_reset_handler_enable(void)
+{
+	void __iomem *iram_base = IO_ADDRESS(TEGRA_IRAM_RESET_BASE);
+	const u32 reset_address = TEGRA_IRAM_RESET_BASE +
+						tegra_cpu_reset_handler_offset;
+
+	BUG_ON(is_enabled);
+	BUG_ON(tegra_cpu_reset_handler_size > TEGRA_IRAM_RESET_HANDLER_SIZE);
+
+	memcpy(iram_base, (void *)__tegra_cpu_reset_handler_start,
+			tegra_cpu_reset_handler_size);
+
+	tegra_cpu_reset_handler_set(reset_address);
 
 	is_enabled = true;
 }
-- 
1.8.3

Re: [PATCH v2 2/3] ARM: tegra: split setting of CPU reset handler

[Stephen Warren]

On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
> Not all Tegra devices need to set the CPU reset handler in the same way.

s/need/can/ ?

> In particular, devices using a TrustZone secure monitor cannot set the
> reset handler directly and need to do it through a firmware operation.
> 
> This patch separates the act of setting the reset handler from its
> preparation, so the former can be implemented in a different way.

Re: [PATCH v2 2/3] ARM: tegra: split setting of CPU reset handler

[Alexandre Courbot]

On Fri, Jun 14, 2013 at 4:21 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 06/13/2013 03:12 AM, Alexandre Courbot wrote:
>> Not all Tegra devices need to set the CPU reset handler in the same way.
>
> s/need/can/ ?

Fixed, thanks!
Alex.