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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel