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

[Julien Grall <julien.grall@linaro.org>]

Hi Suriyan,

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?

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

>   }
>
>   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(...)
{

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

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

>       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?

Regards,

-- 
Julien Grall