Re: [U-Boot] [PATCH v4 10/10] ARM: HYP/non-sec/PSCI: emit DT nodes

[Marc Zyngier]

On Fri, May 02 2014 at  9:13:08 pm BST, Jon Loeliger  wrote:
> On Sat, Apr 26, 2014 at 7:17 AM, Marc Zyngier  wrote:
>
>> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
>> new file mode 100644
>> index 000..0b0d6a7
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/virt-dt.c
>
>> +
>> +static int fdt_psci(void *fdt)
>> +{
>> +#ifdef CONFIG_ARMV7_PSCI
>> +   int nodeoff;
>> +   int tmp;
>> +
>> +   nodeoff = fdt_path_offset(fdt, "/cpus");
>> +   if (nodeoff < 0) {
>> +   printf("couldn't find /cpus\n");
>> +   return nodeoff;
>> +   }
>> +
>> +   /* add 'enable-method = "psci"' to each cpu node */
>> +   for (tmp = fdt_first_subnode(fdt, nodeoff);
>> +tmp >= 0;
>> +tmp = fdt_next_subnode(fdt, tmp)) {
>> +   const struct fdt_property *prop;
>> +   int len;
>> +
>> +   prop = fdt_get_property(fdt, tmp, "device_type", &len);
>> +   if (!prop)
>> +   continue;
>> +   if (len < 4)
>> +   continue;
>> +   if (strcmp(prop->data, "cpu"))
>> +   continue;
>> +
>> +   fdt_setprop_string(fdt, tmp, "enable-method", "psci");
>> +   }
>> +
>> +   nodeoff = fdt_path_offset(fdt, "/psci");
>> +   if (nodeoff < 0) {
>> +   nodeoff = fdt_path_offset(fdt, "/");
>> +   if (nodeoff < 0)
>> +   return nodeoff;
>> +
>> +   nodeoff = fdt_add_subnode(fdt, nodeoff, "psci");
>> +   if (nodeoff < 0)
>> +   return nodeoff;
>> +   }
>> +
>> +   tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci");
>> +   if (tmp)
>> +   return tmp;
>> +   tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc");
>> +   if (tmp)
>> +   return tmp;
>> + tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend",
>> ARM_PSCI_FN_CPU_SUSPEND);
>> +   if (tmp)
>> +   return tmp;
>> +   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", ARM_PSCI_FN_CPU_OFF);
>> +   if (tmp)
>> +   return tmp;
>> +   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", ARM_PSCI_FN_CPU_ON);
>> +   if (tmp)
>> +   return tmp;
>> +   tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", ARM_PSCI_FN_MIGRATE);
>> +   if (tmp)
>> +   return tmp;
>> +#endif
>> +   return 0;
>> +}
>>

Hi Jon,

> So, I wonder if it would be better to be a bit more selective or
> cautious about adding these nodes and properties.  Specifically, if
> they are already present in the device tree itself, perhaps they
> should be honored and left alone?

Well, we have exactly two possibilities:
- PSCI is provided by the platform's firmware, and we'd better not touch
the DT at all.
- PSCI is provided U-Boot, and we *own* the PSCI related nodes.

I don't think there is an alternative to that, because either U-Boot
or the firmware will install its own secure vectors. The DT manipulation
code just reflect this situation.

> I understand that U-Boot gets to define what it implements, and that if
> the secure monitor code doesn't actually implement something, or for
> that matter *does* implement it, it makes sense for U-Boot to be able
> to state those facts in a device tree.  However, the DTS may also be
> stating what it has implemented or willing to honor on the Linux side
> as well.  So, yeah, there has to be agreement here.
>
> But who gets to make the final adjustment to the device tree?  U-boot
> with this code, or the DTS author who may have hand coded specific
> wishes and loaded a specific device tree?

We could have an environment variable named
"i_know_this_looks_silly_but_nevertheless_please_pretty_please_leave_my_cpu_nodes_alone"?

M.
-- 
Without deviation from the norm, progress is not possible.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4 10/10] ARM: HYP/non-sec/PSCI: emit DT nodes

[Jon Loeliger]

On Sat, Apr 26, 2014 at 7:17 AM, Marc Zyngier  wrote:

> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
> new file mode 100644
> index 000..0b0d6a7
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/virt-dt.c

> +
> +static int fdt_psci(void *fdt)
> +{
> +#ifdef CONFIG_ARMV7_PSCI
> +   int nodeoff;
> +   int tmp;
> +
> +   nodeoff = fdt_path_offset(fdt, "/cpus");
> +   if (nodeoff < 0) {
> +   printf("couldn't find /cpus\n");
> +   return nodeoff;
> +   }
> +
> +   /* add 'enable-method = "psci"' to each cpu node */
> +   for (tmp = fdt_first_subnode(fdt, nodeoff);
> +tmp >= 0;
> +tmp = fdt_next_subnode(fdt, tmp)) {
> +   const struct fdt_property *prop;
> +   int len;
> +
> +   prop = fdt_get_property(fdt, tmp, "device_type", &len);
> +   if (!prop)
> +   continue;
> +   if (len < 4)
> +   continue;
> +   if (strcmp(prop->data, "cpu"))
> +   continue;
> +
> +   fdt_setprop_string(fdt, tmp, "enable-method", "psci");
> +   }
> +
> +   nodeoff = fdt_path_offset(fdt, "/psci");
> +   if (nodeoff < 0) {
> +   nodeoff = fdt_path_offset(fdt, "/");
> +   if (nodeoff < 0)
> +   return nodeoff;
> +
> +   nodeoff = fdt_add_subnode(fdt, nodeoff, "psci");
> +   if (nodeoff < 0)
> +   return nodeoff;
> +   }
> +
> +   tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci");
> +   if (tmp)
> +   return tmp;
> +   tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc");
> +   if (tmp)
> +   return tmp;
> +   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend", 
> ARM_PSCI_FN_CPU_SUSPEND);
> +   if (tmp)
> +   return tmp;
> +   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", ARM_PSCI_FN_CPU_OFF);
> +   if (tmp)
> +   return tmp;
> +   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", ARM_PSCI_FN_CPU_ON);
> +   if (tmp)
> +   return tmp;
> +   tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", ARM_PSCI_FN_MIGRATE);
> +   if (tmp)
> +   return tmp;
> +#endif
> +   return 0;
> +}
>

So, I wonder if it would be better to be a bit more selective or cautious
about adding these nodes and properties.  Specifically, if they are already
present in the device tree itself, perhaps they should be honored and left
alone?

I understand that U-Boot gets to define what it implements, and that if
the secure monitor code doesn't actually implement something, or for
that matter *does* implement it, it makes sense for U-Boot to be able
to state those facts in a device tree.  However, the DTS may also be
stating what it has implemented or willing to honor on the Linux side
as well.  So, yeah, there has to be agreement here.

But who gets to make the final adjustment to the device tree?  U-boot
with this code, or the DTS author who may have hand coded specific
wishes and loaded a specific device tree?

HTH,
jdl
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v4 10/10] ARM: HYP/non-sec/PSCI: emit DT nodes

[Marc Zyngier]

Generate the PSCI node in the device tree.

Also add a reserve section for the "secure" code that lives in
in normal RAM, so that the kernel knows it'd better not trip on
it.

Signed-off-by: Marc Zyngier 
---
 arch/arm/cpu/armv7/Makefile  |   1 +
 arch/arm/cpu/armv7/virt-dt.c | 100 +++
 arch/arm/include/asm/armv7.h |   1 +
 arch/arm/lib/bootm-fdt.c |  12 +-
 4 files changed, 112 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/virt-dt.c

diff --git a/arch/arm/cpu/armv7/Makefile b/arch/arm/cpu/armv7/Makefile
index de1cc1a..44329cd 100644
--- a/arch/arm/cpu/armv7/Makefile
+++ b/arch/arm/cpu/armv7/Makefile
@@ -21,6 +21,7 @@ endif
 ifneq ($(CONFIG_ARMV7_NONSEC)$(CONFIG_ARMV7_VIRT),)
 obj-y  += nonsec_virt.o
 obj-y  += virt-v7.o
+obj-y  += virt-dt.o
 endif
 
 ifneq ($(CONFIG_ARMV7_PSCI),)
diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
new file mode 100644
index 000..0b0d6a7
--- /dev/null
+++ b/arch/arm/cpu/armv7/virt-dt.c
@@ -0,0 +1,100 @@
+/*
+ * Copyright (C) 2013 - ARM Ltd
+ * Author: Marc Zyngier 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static int fdt_psci(void *fdt)
+{
+#ifdef CONFIG_ARMV7_PSCI
+   int nodeoff;
+   int tmp;
+
+   nodeoff = fdt_path_offset(fdt, "/cpus");
+   if (nodeoff < 0) {
+   printf("couldn't find /cpus\n");
+   return nodeoff;
+   }
+
+   /* add 'enable-method = "psci"' to each cpu node */
+   for (tmp = fdt_first_subnode(fdt, nodeoff);
+tmp >= 0;
+tmp = fdt_next_subnode(fdt, tmp)) {
+   const struct fdt_property *prop;
+   int len;
+
+   prop = fdt_get_property(fdt, tmp, "device_type", &len);
+   if (!prop)
+   continue;
+   if (len < 4)
+   continue;
+   if (strcmp(prop->data, "cpu"))
+   continue;
+
+   fdt_setprop_string(fdt, tmp, "enable-method", "psci");
+   }
+
+   nodeoff = fdt_path_offset(fdt, "/psci");
+   if (nodeoff < 0) {
+   nodeoff = fdt_path_offset(fdt, "/");
+   if (nodeoff < 0)
+   return nodeoff;
+
+   nodeoff = fdt_add_subnode(fdt, nodeoff, "psci");
+   if (nodeoff < 0)
+   return nodeoff;
+   }
+
+   tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci");
+   if (tmp)
+   return tmp;
+   tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc");
+   if (tmp)
+   return tmp;
+   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend", 
ARM_PSCI_FN_CPU_SUSPEND);
+   if (tmp)
+   return tmp;
+   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", ARM_PSCI_FN_CPU_OFF);
+   if (tmp)
+   return tmp;
+   tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", ARM_PSCI_FN_CPU_ON);
+   if (tmp)
+   return tmp;
+   tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", ARM_PSCI_FN_MIGRATE);
+   if (tmp)
+   return tmp;
+#endif
+   return 0;
+}
+
+int armv7_update_dt(void *fdt)
+{
+#ifndef CONFIG_ARMV7_SECURE_BASE
+   /* secure code lives in RAM, keep it alive */
+   fdt_add_mem_rsv(fdt, (unsigned long)__secure_start,
+   __secure_end - __secure_start);
+#endif
+
+   return fdt_psci(fdt);
+}
diff --git a/arch/arm/include/asm/armv7.h b/arch/arm/include/asm/armv7.h
index 11476dd..323f282 100644
--- a/arch/arm/include/asm/armv7.h
+++ b/arch/arm/include/asm/armv7.h
@@ -79,6 +79,7 @@ void v7_outer_cache_inval_range(u32 start, u32 end);
 #if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
 
 int armv7_init_nonsec(void);
+int armv7_update_dt(void *fdt);
 
 /* defined in assembly file */
 unsigned int _nonsec_init(void);
diff --git a/arch/arm/lib/bootm-fdt.c b/arch/arm/lib/bootm-fdt.c
index 8394e15..d4f1578 100644
--- a/arch/arm/lib/bootm-fdt.c
+++ b/arch/arm/lib/bootm-fdt.c
@@ -17,13 +17,14 @@
 
 #include 
 #include 
+#include 
 
 DECLARE_GLOBAL_DATA_PTR;
 
 int arch_fixup_fdt(void *blob)
 {
bd_t *bd = gd->bd;
-   int bank;
+   int bank, ret;
u64 start[CONFIG_NR_DRAM_BANKS];
u64 size[CONFIG_NR_DRAM_BANKS];
 
@@ -32,5 +33,12 @@ int arch_fixup