[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 <loeliger at gmail.com> wrote:
> On Sat, Apr 26, 2014 at 7:17 AM, Marc Zyngier <marc.zyngier at arm.com> wrote:
>
>> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
>> new file mode 100644
>> index 0000000..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.