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 20:06:02 +0100
Message-ID: <1411067162.1920.31.camel@citrix.com>
References: <1410562915-16761-1-git-send-email-suriyan.r@gmail.com>
	<1411065029.1920.23.camel@citrix.com>
	<CANoR_OCm0d5un6uXpgoWpTA4dyW1CnazQvXOJdD3-Ypb2g-urg@mail.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: <CANoR_OCm0d5un6uXpgoWpTA4dyW1CnazQvXOJdD3-Ypb2g-urg@mail.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: Julien Grall <julien.grall@linaro.org>, Tim Deegan <tim@xen.org>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On Thu, 2014-09-18 at 11:44 -0700, Suriyan Ramasami wrote:
> >> @@ -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...)
> >
> 
> Julien had commented on this too. My response was:
> "We are anding the result of the readl, and hence as its outside of the
> readl (and not a parameter to it), I aligned it as such. Is that not
> right? Cause, if I align it under the ( of readl, it will appear as if
> it was a parameter to readl. Please let me know."

Hrm, ok. But it's still a spurious change as far as this commit goes, we
generally try and avoid such things (and this patch is already skirting
close to changing too much in one go)

> >> +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).
> >
> 
> Julien had commented on this too, and he too recommended I take a look
> at __invoke_psci_fn_smc in xen/arch/arm/psci.c
> 
> I had taken these into consideration and submitted a v3 of this patch.

Oh, I seem to have missed that, sorry.