Re: [PATCH v2 3/6] arm: parse PSCI node from the host device-tree

[Julien Grall <julien.grall@linaro.org>]

On 12/02/2013 01:44 PM, Andre Przywara wrote:
> On 12/02/2013 02:28 PM, Julien Grall wrote:
>>
>>
>> On 12/02/2013 11:08 AM, Andre Przywara wrote:
>>> The availability of a PSCI handler is advertised in the DTB.
>>> Find and parse the node (described in the Linux device-tree binding)
>>> and save the function number for bringing up a CPU for later usage.
>>> We do some sanity checks, especially we deny using HVC as a calling
>>> method, as it does not make much sense currently under Xen.
>>>
>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>>> ---
>>>   xen/arch/arm/Makefile      |  1 +
>>>   xen/arch/arm/psci.c        | 64
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>   xen/arch/arm/smpboot.c     |  7 +++++
>>>   xen/include/asm-arm/psci.h |  6 +++++
>>>   4 files changed, 78 insertions(+)
>>>   create mode 100644 xen/arch/arm/psci.c
>>>
>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>>> index 11cf663..d70f6d5 100644
>>> --- a/xen/arch/arm/Makefile
>>> +++ b/xen/arch/arm/Makefile
>>> @@ -5,6 +5,7 @@ subdir-y += platforms
>>>   obj-$(EARLY_PRINTK) += early_printk.o
>>>   obj-y += cpu.o
>>>   obj-y += domain.o
>>> +obj-y += psci.o
>>>   obj-y += vpsci.o
>>>   obj-y += domctl.o
>>>   obj-y += sysctl.o
>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>>> new file mode 100644
>>> index 0000000..9ff06cd
>>> --- /dev/null
>>> +++ b/xen/arch/arm/psci.c
>>> @@ -0,0 +1,64 @@
>>> +/*
>>> + * xen/arch/arm/psci.c
>>> + *
>>> + * PSCI host support
>>> + *
>>> + * Andre Przywara <andre.przywara@linaro.org>
>>> + * Copyright (c) 2013 Linaro Limited.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +
>>> +#include <xen/types.h>
>>> +#include <xen/mm.h>
>>> +#include <xen/smp.h>
>>> +#include <asm/psci.h>
>>> +
>>> +int psci_available;
>>
>> I would use bool_t here.
>
> Yes.
>
>>
>>> +
>>> +static uint32_t psci_cpu_on_nr;
>>
>> What about handling shutdown/reboot?
>
> Given the pressure on the release, I'd hold any extra wishes back for
> now and just enable PSCI for SMP bringup.

It's a bit annoying to not being able to reboot the platform via 
"reboot" command in dom0.

Even if you don't implement reboot for this patch series, how about 
creating an array to store the different value ie (on, off,...)?

>>> +    int ret;
>>> +    const char *prop_str;
>>> +
>>> +    psci = dt_find_compatible_node(NULL, NULL, "arm,psci");
>>> +    if ( !psci )
>>> +        return -ENODEV;
>>> +
>>> +    ret = dt_property_read_string(psci, "method", &prop_str);
>>> +    if ( ret )
>>> +    {
>>> +        printk("/psci node does not provide a method (%d)\n", ret);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Since Xen runs in HYP all of the time, it does not make sense to
>>> +     * let it call into HYP for PSCI handling, since the handler won't
>>> +     * just be there. So bail out with an error if "smc" is not used.
>>> +     */
>>> +    if ( strcmp(prop_str, "smc") )
>>
>> As we only handle "hvc" method, why not checking if we use the right
>> method and bail out in all other case? It will help for the future, if
>> PSCI guys decide to implement another method.
>
> We don't handle hvc, but only smc. Remember the inverse logic of strcmp.

Oh right, I was thinking about guest, no host :)


-- 
Julien Grall