Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

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

Hello Suriyan,

My email address is @linaro.org not @linaro.com. I nearly miss this 
patch because of this.

Depending if Ian apply his patch (see the conversation on 
"Odroid-XU..."), I would update the title and the message.

On 11/09/14 10:25, Suriyan Ramasami wrote:
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index bc9ae15..334650c 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -28,6 +28,9 @@
>   #include <asm/platform.h>
>   #include <asm/io.h>
>
> +/* This corresponds to CONFIG_NR_CPUS in linux config */
> +#define EXYNOS_CONFIG_NR_CPUS       0x08
> +
>   #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>   #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>   #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>       u64 mct_base_addr;
>       u64 size;
>
> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
> -
>       node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct");
>       if ( !node )
>       {
> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>       dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>               mct_base_addr, size);
>
> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE, PAGE_HYPERVISOR_NOCACHE);
> +    if ( size <= EXYNOS5_MCT_G_TCON )

Honestly, I don't think this check is very useful. The device tree 
should always contains valid size.

> +    {
> +        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
> +                EXYNOS5_MCT_G_TCON);
> +        return -EINVAL;
> +    }
> +
> +    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>       if ( !mct )
>       {
>           dprintk(XENLOG_ERR, "Unable to map MCT\n");
> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain *d)
>       return 0;
>   }
>
> -static int __init exynos5_smp_init(void)
> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
> +                                            u64 *sysram_offset)
>   {

This function contains nearly twice the same code. Except the compatible 
string and the offset which differs, everything is the same.

I would do smth like:

node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
if ( !node )
{
    compatible = "samsung,exynos4210-sysram";
    *sysram_offset = 0;
}
else
{
    compatible "samsung,exynos4210-sysram-ns";
    *sysram_offset = 0x1c;
}

node = dt_find_compatible_node(NULL, NULL, compatible);
if ( !node )
....

[..]

> +static int __init exynos5_smp_init(void)
> +{
> +    void __iomem *sysram;
> +    u64 sysram_addr;
> +    u64 sysram_offset;
> +    int rc;
> +
> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
> +    if ( rc )
> +        return rc;
>
> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
> +            sysram_addr, sysram_offset);
> +
> +    if ( sysram_offset >= PAGE_SIZE )
> +    {
> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
> +        return -EINVAL;
> +    }

As the previous check for the MCT. I don't think it's useful. Also why 
don't do get the size from the device tree?

> +
> +    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>       if ( !sysram )
>       {
>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
> @@ -125,7 +176,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 + 0x1c);
> +    writel(__pa(init_secondary), sysram + sysram_offset);
>
>       iounmap(sysram);
>
> @@ -134,14 +185,14 @@ 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;
> +    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
> +                       EXYNOS_ARM_CORE_STATUS(cpu)) & S5P_CORE_LOCAL_PWR_EN;

Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro 
EXYNOS_CORE_CONFIGURATION. I would do the same here.

[..]

> -    power = ioremap_nocache(power_base_addr +
> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
> +    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
> +                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
> +    {
> +        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu range.\n");
> +        return -EINVAL;
> +    }
> +

Same remark as before for the check.

[..]

> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
> +    if ( size <= EXYNOS5_SWRESET )
> +    {
> +        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
> +                EXYNOS5_SWRESET);
> +        return;
> +    }
> +

Same here too.

Regards,

-- 
Julien Grall