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

login
register
mail settings
Submitter Alexandre Courbot
Date June 13, 2013, 9:12 a.m.
Message ID <1371114745-24710-4-git-send-email-acourbot@nvidia.com>
Download mbox | patch
Permalink /patch/251020/
State Superseded, archived
Headers show

Comments

Alexandre Courbot - June 13, 2013, 9:12 a.m.
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@nvidia.com>
---
 arch/arm/mach-tegra/reset.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)
Stephen Warren - June 13, 2013, 7:23 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot - June 14, 2013, 8:54 a.m.
On Fri, Jun 14, 2013 at 4:23 AM, Stephen Warren <swarren@wwwdotorg.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - June 14, 2013, 3:30 p.m.
On 06/14/2013 02:54 AM, Alexandre Courbot wrote:
> On Fri, Jun 14, 2013 at 4:23 AM, Stephen Warren <swarren@wwwdotorg.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.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)