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

[Suriyan Ramasami <suriyan.r@xxxxxxxxx>]

On Thu, Sep 18, 2014 at 11:30 AM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
> 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.

I shall remove this #define.

>> @@ -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."


>> +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.

Thanks
- Suriyan

> Ian.
>

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