[PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
Add basic support for booting secondary processors on Tegra devices
using the Trusted Foundations secure monitor.
Signed-off-by: Alexandre Courbot
---
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 @@ stSTMicroelectronics
steST-Ericsson
stericsson ST-Ericsson
ti Texas Instruments
+tl Trusted Logic
toshibaToshiba Corporation
viaVIA Technologies, Inc.
wlfWolfson 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 000..ea8
Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
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
> ---
> 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-teg
Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
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
> ---
> 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.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
On Fri, Jun 14, 2013 at 4:19 AM, Stephen Warren 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
>> ---
>> 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.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin 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_extensionsec\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 ca
Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
On 06/14/2013 02:27 AM, Alexandre Courbot wrote:
> On Fri, Jun 14, 2013 at 4:19 AM, Stephen Warren 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?
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
On 06/14/2013 02:43 AM, Alexandre Courbot wrote:
> On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin 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.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v2 1/3] ARM: tegra: basic support for Trusted Foundations
On Fri, Jun 14, 2013 at 05:43:03PM +0900, Alexandre Courbot wrote:
> On Thu, Jun 13, 2013 at 11:35 PM, Dave Martin 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
