Re: [PATCH v1 1/1] XEN/ARM: Add Odroid-XU3/XU4 support

[Ian Campbell <ian.campbell@citrix.com>]

On Sun, 2016-02-07 at 09:44 -0800, Suriyan Ramasami wrote:
> The Odroid-XU3/XU4 from hardkernel is an Exynos 5422 based board.
> Code from mcpm-exynos.c and mcpm-platsmp.c from the Linux kernel
> has been used to get all the 8 cores from the 2 clusters powered
> on.
> The Linux DTS for these odroid uses "samsung,exynos5800" as
> the machine compatible string. Hence, the same is used herein.
> 
> Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
> ---
>  xen/arch/arm/platforms/exynos5.c | 94
> +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/platforms/exynos5.c
> b/xen/arch/arm/platforms/exynos5.c
> index bf4964d..60ceb2b 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -37,6 +37,15 @@ static bool_t secure_firmware;
>  
>  #define SMC_CMD_CPU1BOOT            (-4)
>  
> +#define EXYNOS5800_CPUS_PER_CLUSTER 4
> +#define EXYNOS5800_NR_CLUSTERS      2
> +
> +#define EXYNOS5420_KFC_CORE_RESET0  BIT(8)
> +#define EXYNOS5420_KFC_ETM_RESET0   BIT(20)
> +
> +#define EXYNOS5420_KFC_CORE_RESET(_nr) \
> +        ((EXYNOS5420_KFC_CORE_RESET0 | EXYNOS5420_KFC_ETM_RESET0) <<
> (_nr))
> +
>  static int exynos5_init_time(void)
>  {
>      uint32_t reg;
> @@ -163,6 +172,84 @@ static void exynos_cpu_power_up(void __iomem *power,
> int cpu)
>                   power + EXYNOS_ARM_CORE_CONFIG(cpu));
>  }
>  
> +static void cpu_to_pcpu(int cpu, int *pcpu, int *pcluster)
> +{
> +   int mpidr;
> +
> +   mpidr = cpu_logical_map(cpu);
> +   *pcpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +   *pcluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +}
> +
> +static int exynos5800_cpu_power_up(void __iomem *power, int cpu)
> +{
> +    int timeout;
> +    int pcpu, pcluster;
> +    unsigned int cpunr;
> +
> +    cpu_to_pcpu(cpu, &pcpu, &pcluster);
> +    cpunr = pcpu + (pcluster * EXYNOS5800_CPUS_PER_CLUSTER);
> +    dprintk(XENLOG_DEBUG, "cpu: %d pcpu: %d, cluster: %d cpunr: %d\n",
> +            cpu, pcpu, pcluster, cpunr);
> +
> +
> +    if ( !exynos_cpu_power_state(power, cpunr) )
> +    {
> +        exynos_cpu_power_up(power, cpunr);
> +        timeout = 10;
> +
> +        /* wait max 10 ms until cpu is on */
> +        while ( exynos_cpu_power_state(power, cpunr) !=
> S5P_CORE_LOCAL_PWR_EN )
> +        {
> +            if ( timeout-- == 0 )
> +                break;
> +
> +            mdelay(1);
> +        }
> +
> +        if ( timeout == 0 )
> +        {
> +            dprintk(XENLOG_ERR, "CPU%d power enable failed\n", cpu);
> +            return -ETIMEDOUT;
> +        }

Up to here this is the same as exynos5_cpu_power_up.

I think either of these two approaches would be acceptable to reduce the
duplication either:

move the above into a common helper called from both here and
exynos5_cpu_power_up

or

move the remainder of this function into a exynos5800_cluster_reset (or
would it be better await_reset?) function and call that from exynos5_cpu_up
after the call to exynos5_cpu_power_up when the compat string matches.

I think I have a light preference for the second option.


> +
> +        /*
> +         * This assumes the cluster number of the big cores(Cortex A15)
> +         * is 0 and the Little cores(Cortex A7) is 1.
> +         * When the system was booted from the Little core,
> +         * they should be reset during power up cpu.
> +         */
> +        if (pcluster &&
> +            pcluster == MPIDR_AFFINITY_LEVEL(cpu_logical_map(0), 1)) {
> +            /*
> +             * Before we reset the Little cores, we should wait
> +             * the SPARE2 register is set to 1 because the init
> +             * codes of the iROM will set the register after
> +             * initialization.

If you have a reference (e.g. a URL) for this that would be useful to
include (but don't worry if there isn't one).

> +            */
> +
> +            /* wait max 10ms for the spare register to be set to 1 */
> +            timeout = 10;
> +            while ( !__raw_readl(power + S5P_PMU_SPARE2) )
> +            {
> +                if ( timeout-- == 0 )
> +                    break;
> +
> +                mdelay(1);
> +            }
> +
> +            if ( timeout == 0 )
> +            {
> +                dprintk(XENLOG_ERR, "CPU%d SPARE2 register wait
> failed\n", cpu);
> +                return -ETIMEDOUT;
> +            }
> +            __raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu), power +
> EXYNOS5_SWRESET);
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int exynos5_cpu_power_up(void __iomem *power, int cpu)
>  {
>      unsigned int timeout;
> @@ -240,7 +327,10 @@ static int exynos5_cpu_up(int cpu)
>          return -EFAULT;
>      }
>  
> -    rc = exynos5_cpu_power_up(power, cpu);
> +    if ( dt_machine_is_compatible("samsung,exynos5800") )
> +        rc = exynos5800_cpu_power_up(power, cpu);
> +    else
> +        rc = exynos5_cpu_power_up(power, cpu);
>      if ( rc )
>      {
>          iounmap(power);
> @@ -298,6 +388,7 @@ static const char * const exynos5250_dt_compat[]
> __initconst =
>  static const char * const exynos5_dt_compat[] __initconst =
>  {
>      "samsung,exynos5410",
> +    "samsung,exynos5800",
>      NULL
>  };
>  
> @@ -318,6 +409,7 @@ PLATFORM_START(exynos5, "SAMSUNG EXYNOS5")
>      .cpu_up = exynos5_cpu_up,
>      .reset = exynos5_reset,
>      .blacklist_dev = exynos5_blacklist_dev,
> +
>  PLATFORM_END
>  
>  /*
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel