Re: [Xen-devel] [PATCH v2 1/1] XEN/ARM: Add Odroid-XU3/XU4 support

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Mon, 2016-02-08 at 21:48 -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.
> 
> This change has been tested on the Odroid-XU/XU3/XU4.
> 
> Signed-off-by: Suriyan Ramasami <suriyan.r@xxxxxxxxx>

Thanks, this now looks good to me, with one question about a comment which
I could resolve upon commit if we agree a wording.

> ---
> Changes between versions as follows:
> 
> v2:
> Try to use common code as much as possible
> 
> v1:
> Initial code submission
> ---
> Âxen/arch/arm/platforms/exynos5.c | 61
> ++++++++++++++++++++++++++++++++++++++--
> Â1 file changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/exynos5.c
> b/xen/arch/arm/platforms/exynos5.c
> index bf4964d..12aea31 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -34,9 +34,18 @@ static bool_t secure_firmware;
> Â#define EXYNOS_ARM_CORE_CONFIG(_nr) (EXYNOS_ARM_CORE0_CONFIG + (0x80 *
> (_nr)))
> Â#define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
> Â#define S5P_CORE_LOCAL_PWR_ENÂÂÂÂÂÂÂ0x3
> +#define S5P_PMU_SPARE2ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ0x908
> Â
> Â#define SMC_CMD_CPU1BOOTÂÂÂÂÂÂÂÂÂÂÂÂ(-4)
> Â
> +#define EXYNOS5800_CPUS_PER_CLUSTER 4
> +
> +#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;
> @@ -166,14 +175,23 @@ static void exynos_cpu_power_up(void __iomem
> *power, int cpu)
> Âstatic int exynos5_cpu_power_up(void __iomem *power, int cpu)
> Â{
> ÂÂÂÂÂunsigned int timeout;
> +ÂÂÂÂunsigned int mpidr, pcpu, pcluster, cpunr;
> +
> +ÂÂÂÂmpidr = cpu_logical_map(cpu);
> +ÂÂÂÂpcpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +ÂÂÂÂpcluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> Â
> -ÂÂÂÂif ( !exynos_cpu_power_state(power, cpu) )
> +ÂÂÂÂ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, cpu);
> +ÂÂÂÂÂÂÂÂexynos_cpu_power_up(power, cpunr);
> ÂÂÂÂÂÂÂÂÂtimeout = 10;
> Â
> ÂÂÂÂÂÂÂÂÂ/* wait max 10 ms until cpu is on */
> -ÂÂÂÂÂÂÂÂwhile ( exynos_cpu_power_state(power, cpu) !=
> S5P_CORE_LOCAL_PWR_EN )
> +ÂÂÂÂÂÂÂÂwhile ( exynos_cpu_power_state(power, cpunr) !=
> S5P_CORE_LOCAL_PWR_EN )
> ÂÂÂÂÂÂÂÂÂ{
> ÂÂÂÂÂÂÂÂÂÂÂÂÂif ( timeout-- == 0 )
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> @@ -186,6 +204,42 @@ static int exynos5_cpu_power_up(void __iomem *power,
> int cpu)
> ÂÂÂÂÂÂÂÂÂÂÂÂÂdprintk(XENLOG_ERR, "CPU%d power enable failed\n", cpu);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ETIMEDOUT;
> ÂÂÂÂÂÂÂÂÂ}
> +
> +ÂÂÂÂÂÂÂÂ/*
> +ÂÂÂÂÂÂÂÂÂ* 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.
> +ÂÂÂÂÂÂÂÂÂ* Note that the below condition is true for Odroid XU3/XU4, and
> +ÂÂÂÂÂÂÂÂÂ* false for the XU and the Exynos5800 based boards.

I think the SoC is more relevant/useful in this context than specific
boards. So I think what it is trying to say here is that for systems
matchingÂsamsung,exynos5410 pcluster will always be zero, where as for ones
matchingÂsamsung,exynos5800 it can be non-zero, and for non-zero clusters
we need to do some extra bringup.

I think the comment should therefore focus on the SoC (maybe giving some
examples of systems, but such a list cannot be exhaustive and could be
omitted IMHO). How about:

    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.

    Note that only exynos5800 based SoCs have a pcluster==1 of little cores,
    forÂexynos5410 there is only pcluster==0.

?

I would also consider perhaps moving the comment up to where pcluster is
calculated, and in particular near the:

    cpunr = pcpu + (pcluster * EXYNOS5800_CPUS_PER_CLUSTER);

Since this also relies on pcluster==0 for !5800 systems.

What do you think? I could adjust the comment wording and placement on
commit if you are happy.

I have one other question -- does this patch result in a Xen system which
consists of both A7 and A15 pcpus being live at the same time? Does that
actually work given the subtle differences in things like the cache line
length? I would expect there to need to be other patches to support a
heterogeneous core configuration.

Hopefully I've just misunderstood and the effect of this patch is to switch
to just running the A15 processors even after booting on the A7s.

Ian.


> +ÂÂÂÂÂÂÂÂÂ*/
> +ÂÂÂÂÂÂÂÂ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.
> +ÂÂÂÂÂÂÂÂÂÂÂÂ*/
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂ/* 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;
> Â}
> @@ -298,6 +352,7 @@ static const char * const exynos5250_dt_compat[]
> __initconst =
> Âstatic const char * const exynos5_dt_compat[] __initconst =
> Â{
> ÂÂÂÂÂ"samsung,exynos5410",
> +ÂÂÂÂ"samsung,exynos5800",
> ÂÂÂÂÂNULL
> Â};
> Â
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel