[PATCH v2 1/3] PSCI: Use DT Function ID values only for old versions of spec

[Ashwin Chaugule]

Hi Rob,

On 27 March 2014 12:41, Rob Herring <rob.herring at linaro.org> wrote:
> On Thu, Mar 27, 2014 at 10:38 AM, Ashwin Chaugule
> <ashwin.chaugule at linaro.org> wrote:
>> PSCI v0.2+ spec mandates specific values of Function IDs
>> for ARM32 and ARM64. Use DT bindings of Function IDs only
>> when using older versions. Use standard values otherwise.
>
> The subject line is a bit out of date.

Yea. This patch has now become more of a PSCIv0.2 implementation.

>> +       int err = 0;
>> +       int ver = 0;
>
> Initialization not needed.

Ok.

>>
>> -       np = of_find_matching_node(NULL, psci_of_match);
>> -       if (!np)
>> -               return;
>> +       pr_info("probing for conduit method from DT.\n");
>> +
>> +       if (of_property_read_string(np, "method", &method)) {
>> +               pr_warn("missing \"method\" property\n");
>> +               err = -ENXIO;
>> +               goto out_put_node;
>> +       }
>> +
>> +       if (!strcmp("hvc", method)) {
>> +               invoke_psci_fn = __invoke_psci_fn_hvc;
>> +       } else if (!strcmp("smc", method)) {
>> +               invoke_psci_fn = __invoke_psci_fn_smc;
>> +       } else {
>> +               pr_warn("invalid \"method\" property: %s\n", method);
>> +               err = -EINVAL;
>> +               goto out_put_node;
>> +       }
>
> This should all be common setup.

I'll roll this up into a helper.

>> +
>> +       ver = psci_get_version();
>> +
>> +       pr_info("PSCIv%d.%d detected in firmware.\n",
>> +                       (ver & PSCI_VER_MAJOR_MASK) >> PSCI_VER_MAJOR_SHIFT,
>> +                       (ver & PSCI_VER_MINOR_MASK));
>
> If it doesn't report 0.2, then we should probably bail (possibly
> falling back to 0.1).

Hm. I'd prefer bailing out here. That would hopefully force the user
to check the PSCI implementation.
Theres always the option of trying out "arm, psci" later.


>> +       ver = psci_get_version();
>
> This may not be safe to do on 0.1 as the call didn't exist.

Ok, I will take it out from this function.

>
>> +
>> +       pr_info("PSCIv%d.%d detected in firmware.\n",
>> +                       (ver & PSCI_VER_MAJOR_MASK) >> PSCI_VER_MAJOR_SHIFT,
>> +                       (ver & PSCI_VER_MINOR_MASK));
>> +
>>         if (!of_property_read_u32(np, "cpu_suspend", &id)) {
>>                 psci_function_id[PSCI_FN_CPU_SUSPEND] = id;
>>                 psci_ops.cpu_suspend = psci_cpu_suspend;
>> @@ -204,7 +302,38 @@ void __init psci_init(void)
>>                 psci_ops.migrate = psci_migrate;
>>         }
>>
>> +       if (!of_property_read_u32(np, "affinity_info", &id)) {
>> +               psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_ID_AFFINITY_INFO;
>> +               psci_ops.affinity_info = psci_affinity_info;
>> +       }
>> +
>> +       if (!of_property_read_u32(np, "migrate_info_type", &id)) {
>> +               psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE]
>> +                       = PSCI_ID_MIGRATE_INFO_TYPE;
>> +               psci_ops.migrate_info_type = psci_migrate_info_type;
>> +       }
>
> These 2 functions don't exist for 0.1.

Will remove.

>> +static const struct of_device_id psci_of_match[] __initconst = {
>> +       { .compatible = "arm,psci", .data = psci_of_init},
>> +       { .compatible = "arm,psci-0.2", .data = psci_static_init},
>
> "of" and "static" don't really describe the difference here. Use
> something like psci_init_0_1 and psci_init_0_2.

Ok.

> Same comments apply to this file.
>> index 0000000..b271e9a
>> --- /dev/null
>> +++ b/include/uapi/linux/psci.h
>> @@ -0,0 +1,45 @@
>> +/*
>
> Linaro copyright?
>

Will add.

>> +#ifdef CONFIG_ARM64
>
> You don't need ifdefs here. The 32-bit calls are valid on arm64 as
> that is what 32-bit guests will use. Just do something like this:
>
> #define PSCI_ID_VERSION                                0x84000000
> #define PSCI_ID_CPU_SUSPEND_32                    0x84000001
> #define PSCI_ID_CPU_SUSPEND_64                    0xc4000001
>

Ok, thats better.

Cheers,
Ashwin