[Xen-devel,V2,3/3] amd/seattle: Initial revision of AMD Seattle support

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

This patch add inital (minimal) platform support for AMD Seattle,
which mainly just define the matching ID, and specify system_off,
and sytem_reset mechanism.

Initially, the firmware only support a subset of PSCI-0.2 functions,
system-off and sytem-reset. The boot protocol is still using spin-table.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 xen/arch/arm/platforms/Makefile  |  1 +
 xen/arch/arm/platforms/seattle.c | 69 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 xen/arch/arm/platforms/seattle.c

Hi Suravee,

On 10/01/2014 08:51 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> This patch add inital (minimal) platform support for AMD Seattle,
> which mainly just define the matching ID, and specify system_off,
> and sytem_reset mechanism.
> 
> Initially, the firmware only support a subset of PSCI-0.2 functions,
> system-off and sytem-reset. The boot protocol is still using spin-table.

s/sytem-reset/system-reset/

I find "the boot protocol" not clear, I guess you are talking about CPU
bring up. Maybe something like "CPU management ..." will be better?

> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  xen/arch/arm/platforms/Makefile  |  1 +
>  xen/arch/arm/platforms/seattle.c | 69 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 xen/arch/arm/platforms/seattle.c
> 
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 680364f..03e7a14 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_ARM_32) += midway.o
>  obj-$(CONFIG_ARM_32) += omap5.o
>  obj-$(CONFIG_ARM_32) += sunxi.o
>  obj-$(CONFIG_ARM_64) += xgene-storm.o
> +obj-$(CONFIG_ARM_64) += seattle.o

NIT: Could we order the platform name alphabetically? i.e move
"seattle.o" just above "xgene-storm.o".

> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> new file mode 100644
> index 0000000..06d4e99
> --- /dev/null
> +++ b/xen/arch/arm/platforms/seattle.c
> @@ -0,0 +1,69 @@
> +/*
> + * xen/arch/arm/seattle.c
> + *
> + * AMD Seattle specific settings
> + *
> + * Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> + * Copyright (c) 2014 Advance Micro Devices Inc.
> + *
> + * 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 <asm/platform.h>

> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
> +#include <asm/gic.h>

I don't think those 4 includes are required here.

> +#include <asm/psci.h>
> +
> +static const char * const seattle_dt_compat[] __initconst =
> +{
> +    "amd,seattle",
> +    NULL
> +};
> +
> +/* Seattle firmware only implements PSCI handler for
> + * system off and system reset at this point.
> + * This is temporary until full PSCI-0.2 is supported.
> + * Then, these function will be removed.
> + */
> +static noinline void seattle_smc_psci(register_t func_id)
> +{
> +    asm volatile(
> +        "smc #0"
> +        : "+r" (func_id)
> +        :);
> +}

We already have multiple implementation of smc in different place. Can
we provide a common function rather than adding another one?


> +static noinline void seattle_system_reset(void)

noinline is not necessary here.

> +{
> +    seattle_smc_psci(PSCI_0_2_FN_SYSTEM_RESET);
> +}
> +
> +static noinline void seattle_system_off(void)

ditto

> +{
> +    seattle_smc_psci(PSCI_0_2_FN_SYSTEM_OFF);
> +}
> +
> +PLATFORM_START(seattle, "SEATTLE")
> +    .compatible = seattle_dt_compat,
> +    .reset      = seattle_system_reset,
> +    .poweroff   = seattle_system_off,
> +PLATFORM_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

Regards,

Please see comments inline below.

On 10/02/2014 05:41 AM, Julien Grall wrote:
> Hi Suravee,
>
> On 10/01/2014 08:51 PM, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>
>> This patch add inital (minimal) platform support for AMD Seattle,
>> which mainly just define the matching ID, and specify system_off,
>> and sytem_reset mechanism.
>>
>> Initially, the firmware only support a subset of PSCI-0.2 functions,
>> system-off and sytem-reset. The boot protocol is still using spin-table.
>
> s/sytem-reset/system-reset/

Ah... Thanks

>
> I find "the boot protocol" not clear, I guess you are talking about CPU
> bring up. Maybe something like "CPU management ..." will be better?
>

Reword to: "The mechanism for bring up auxiliary processors is still 
using spin-table."

>>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>>   xen/arch/arm/platforms/Makefile  |  1 +
>>   xen/arch/arm/platforms/seattle.c | 69 ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 70 insertions(+)
>>   create mode 100644 xen/arch/arm/platforms/seattle.c
>>
>> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
>> index 680364f..03e7a14 100644
>> --- a/xen/arch/arm/platforms/Makefile
>> +++ b/xen/arch/arm/platforms/Makefile
>> @@ -4,3 +4,4 @@ obj-$(CONFIG_ARM_32) += midway.o
>>   obj-$(CONFIG_ARM_32) += omap5.o
>>   obj-$(CONFIG_ARM_32) += sunxi.o
>>   obj-$(CONFIG_ARM_64) += xgene-storm.o
>> +obj-$(CONFIG_ARM_64) += seattle.o
>
> NIT: Could we order the platform name alphabetically? i.e move
> "seattle.o" just above "xgene-storm.o".

Done

>> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
>> new file mode 100644
>> index 0000000..06d4e99
>> --- /dev/null
>> +++ b/xen/arch/arm/platforms/seattle.c
>> @@ -0,0 +1,69 @@
>> +/*
>> + * xen/arch/arm/seattle.c
>> + *
>> + * AMD Seattle specific settings
>> + *
>> + * Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> + * Copyright (c) 2014 Advance Micro Devices Inc.
>> + *
>> + * 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 <asm/platform.h>
>
>> +#include <xen/mm.h>
>> +#include <xen/vmap.h>
>> +#include <asm/io.h>
>> +#include <asm/gic.h>
>
> I don't think those 4 includes are required here.

Done.

>> +#include <asm/psci.h>
>> +
>> +static const char * const seattle_dt_compat[] __initconst =
>> +{
>> +    "amd,seattle",
>> +    NULL
>> +};
>> +
>> +/* Seattle firmware only implements PSCI handler for
>> + * system off and system reset at this point.
>> + * This is temporary until full PSCI-0.2 is supported.
>> + * Then, these function will be removed.
>> + */
>> +static noinline void seattle_smc_psci(register_t func_id)
>> +{
>> +    asm volatile(
>> +        "smc #0"
>> +        : "+r" (func_id)
>> +        :);
>> +}
>
> We already have multiple implementation of smc in different place. Can
> we provide a common function rather than adding another one?

The only place I found this is in the arch/arm/psci.c, which is used 
mainly for the PSCI stuff. I can declare that one as non-static, and use 
it here in the seattle.c.

Suravee

On 10/02/2014 02:04 PM, Suravee Suthikulpanit wrote:
>>> +#include <asm/psci.h>
>>> +
>>> +static const char * const seattle_dt_compat[] __initconst =
>>> +{
>>> +    "amd,seattle",
>>> +    NULL
>>> +};
>>> +
>>> +/* Seattle firmware only implements PSCI handler for
>>> + * system off and system reset at this point.
>>> + * This is temporary until full PSCI-0.2 is supported.
>>> + * Then, these function will be removed.
>>> + */
>>> +static noinline void seattle_smc_psci(register_t func_id)
>>> +{
>>> +    asm volatile(
>>> +        "smc #0"
>>> +        : "+r" (func_id)
>>> +        :);
>>> +}
>>
>> We already have multiple implementation of smc in different place. Can
>> we provide a common function rather than adding another one?
>
> The only place I found this is in the arch/arm/psci.c, which is used
> mainly for the PSCI stuff. I can declare that one as non-static, and use
> it here in the seattle.c.
>
> Suravee

Julien,

Actually, think about this again, I would rather keep this independent 
from the other PSCI patch series, which might not make it into 4.5.  It 
is small enough and probably not too terrible to keep this.

Thanks,

Suravee

On Thu, 2014-10-02 at 15:37 -0500, Suravee Suthikulpanit wrote:
> >>> + * This is temporary until full PSCI-0.2 is supported.
> >>> + * Then, these function will be removed.

> Actually, think about this again, I would rather keep this independent 
> from the other PSCI patch series, which might not make it into 4.5.  It 
> is small enough and probably not too terrible to keep this.

Given the comment about the temporary nature it is IMHO fine to keep
this as is until proper PSCI-0.2 support lands.

Ian.

On 02/10/2014 21:37, Suravee Suthikulpanit wrote:
>
>
> On 10/02/2014 02:04 PM, Suravee Suthikulpanit wrote:
>>>> +#include <asm/psci.h>
>>>> +
>>>> +static const char * const seattle_dt_compat[] __initconst =
>>>> +{
>>>> +    "amd,seattle",
>>>> +    NULL
>>>> +};
>>>> +
>>>> +/* Seattle firmware only implements PSCI handler for
>>>> + * system off and system reset at this point.
>>>> + * This is temporary until full PSCI-0.2 is supported.
>>>> + * Then, these function will be removed.
>>>> + */
>>>> +static noinline void seattle_smc_psci(register_t func_id)
>>>> +{
>>>> +    asm volatile(
>>>> +        "smc #0"
>>>> +        : "+r" (func_id)
>>>> +        :);
>>>> +}
>>>
>>> We already have multiple implementation of smc in different place. Can
>>> we provide a common function rather than adding another one?
>>
>> The only place I found this is in the arch/arm/psci.c, which is used
>> mainly for the PSCI stuff. I can declare that one as non-static, and use
>> it here in the seattle.c.
>>
>> Suravee
>
> Julien,
>
> Actually, think about this again, I would rather keep this independent
> from the other PSCI patch series, which might not make it into 4.5.  It
> is small enough and probably not too terrible to keep this.

There is also one in xen/arch/arm/platforms/exynos.c (exynos_smc).

Moving this patch could be part of this series and I don't think it 
would be a showstopper for accepting it in 4.5.

Ian, any though?

Regards,