From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos
	secure firmware
Date: Thu, 18 Sep 2014 19:30:29 +0100
Message-ID: <1411065029.1920.23.camel@citrix.com>
References: <1410562915-16761-1-git-send-email-suriyan.r@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1410562915-16761-1-git-send-email-suriyan.r@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Suriyan Ramasami <suriyan.r@gmail.com>
Cc: keir@xen.org, julien.grall@linaro.org, tim@xen.org, xen-devel@lists.xen.org, jbeulich@suse.com, ian.jackson@citrix.com
List-Id: xen-devel@lists.xenproject.org

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.