Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2014-09-12 at 16:01 -0700, Suriyan Ramasami wrote:

Mostly looks good, couple of comments below. I'll also try and give it a
spin on arndale when I'm back in the office early next week.

> +/* This corresponds to CONFIG_NR_CPUS in linux config */
> +#define EXYNOS_CONFIG_NR_CPUS       0x08

This doesn't appear to be used, which is good because it would be wrong
to hardcode such things into Xen.
> @@ -135,7 +168,7 @@ static int __init exynos5_smp_init(void)
>  static int exynos_cpu_power_state(void __iomem *power, int cpu)
>  {
>      return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
> -                       S5P_CORE_LOCAL_PWR_EN;
> +           S5P_CORE_LOCAL_PWR_EN;

Please avoid spurious whitespace changes (especially since this one is
wrong...)

> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
> +{
> +    asm(
> +        "dsb;"
> +        "smc    #0;"
> +    );

I don't think this will work reliably in the face of compiler
optimisations. You need something like __invoke_psci_fn_smc. In fact it
would probably be best to refactor that into a common function for
calling into firmware (which looks like it might be a case of renaming
the existing fn and moving it somewhere more appropriate).

Ian.