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

[Suriyan Ramasami <suriyan.r@xxxxxxxxx>]

On Fri, Sep 12, 2014 at 4:52 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Suriyan,
>
Hello Julien,

> On 12/09/14 16:01, Suriyan Ramasami wrote:
>>
>> +static int __init exynos5_smp_init(void)
>> +{
>> +    void __iomem *sysram;
>> +    u64 sysram_addr;
>> +    u64 size;
>> +    u64 sysram_offset;
>> +    int rc;
>> +
>> +    rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size,
>> &sysram_offset);
>
>
> The function name is odd. As you call the function only here, couldn't you
> inline it?
>
OK, I shall do that.

>> +    if ( rc )
>> +        return rc;
>> +
>> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset:
>> %016llx\n",
>> +            sysram_addr, size, sysram_offset);
>> +
>> +    sysram = ioremap_nocache(sysram_addr, size);
>>       if ( !sysram )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>> @@ -125,7 +158,7 @@ static int __init exynos5_smp_init(void)
>>
>>       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>>              __pa(init_secondary), init_secondary);
>> -    writel(__pa(init_secondary), sysram);
>> +    writel(__pa(init_secondary), sysram + sysram_offset);
>>
>>       iounmap(sysram);
>>
>> @@ -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;
>
>
> Why this change?
>
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_cpu_power_up(void __iomem *power, int cpu)
>> @@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem *power,
>> int cpu)
>>       return 0;
>>   }
>>
>> -static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
>> -    u64 size;
>> +static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size) {
>
>
> The Xen coding style is
>
> static int foo(...)
> {
>
Sorry, forgot the coding style in a momentary lapse of reason :-)

>>       struct dt_device_node *node;
>>       int rc;
>>       static const struct dt_device_match exynos_dt_pmu_matches[]
>> __initconst =
>> @@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64
>> *power_base_addr) {
>>           return -ENXIO;
>>       }
>>
>> -    rc = dt_device_get_address(node, 0, power_base_addr, &size);
>> +    rc = dt_device_get_address(node, 0, power_base_addr, size);
>>       if ( rc )
>>       {
>>           dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
>> @@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64
>> *power_base_addr) {
>>       }
>>
>>       dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
>> -            *power_base_addr, size);
>> +            *power_base_addr, *size);
>>
>>       return 0;
>>   }
>>
>> +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3)
>> +{
>> +    asm(
>> +        "dsb;"
>> +        "smc    #0;"
>> +    );
>> +}
>> +
>
>
> The compiler may decide to inline the function. This will end up to the
> command register not in register r0.
>
> Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be
> worth to introduce an SMC helper.
>
OK, will check that out.

>>   static int exynos5_cpu_up(int cpu)
>>   {
>>       u64 power_base_addr;
>> +    u64 size;
>>       void __iomem *power;
>>       int rc;
>>
>> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>>       if ( rc )
>>           return rc;
>>
>> -    power = ioremap_nocache(power_base_addr +
>> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>> +    power = ioremap_nocache(power_base_addr, size);
>>       if ( !power )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
>> @@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu)
>>
>>       iounmap(power);
>>
>> +    exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0);
>> +
>
>
> The call is not done unconditionally on Linux. It's only done when the
> secure firmware is present.
>
You are right again. I shall update the comment, and probably do the
call only if its under secure firmware.

>>       return cpu_up_send_sgi(cpu);
>>   }
>>
>>   static void exynos5_reset(void)
>>   {
>>       u64 power_base_addr;
>> +    u64 size;
>>       void __iomem *pmu;
>>       int rc;
>>
>> -    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
>> -
>> -    rc = exynos5_get_pmu_base_addr(&power_base_addr);
>> +    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
>>       if ( rc )
>>           return;
>>
>> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>> +    pmu = ioremap_nocache(power_base_addr, size);
>>       if ( !pmu )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map PMU\n");
>> @@ -264,6 +305,7 @@ static const struct dt_device_match
>> exynos5_blacklist_dev[] __initconst =
>>        * This is result to random freeze.
>>        */
>>       DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"),
>> +    DT_MATCH_COMPATIBLE("samsung,secure-firmware"),
>
>
> Can you add a comment explaining why we blacklist the secure firmware?
>
I shall add your comment in.
Thanks!
- Suriyan

> Regards,
>
> --
> Julien Grall

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