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

[Suriyan Ramasami <suriyan.r@xxxxxxxxx>]

On Tue, Feb 9, 2016 at 1:53 AM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:

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 believe for some SoCs its board based (5422 = pin controlled) and for others its purely SoC based (5420 = A7 boot, 5800 = A15 boot). For example, please look at this post: https://lkml.org/lkml/2015/12/11/107 which mentions something along those lines.

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 agree on the first two paragraphs.
For the third paragraph, the rebuttal is that the exynos5800 and exynos5422 based SoCs can have both clusters on at the same time. Hence, the third paragrapah comment will have to be tweaked further. Possibly reading:

The exynos5800 and exynos5422 can have both clusters on at the same time. The exynos5800 boots up with cpu0 on cluster0 (A15). The exynos5422 can boot up on either clusters as its pin controlled. In this case the DTS should properly reflect the cpu order. The exynos5410 can have only one cluster on at a time, and it boots up with pcluster == 0.

Any tweaks and comments on the above is appreciated.

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 am definitely OK with moving the comment to where you suggest.
Â

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.

Yes, it does get both A7 and A15 clusters up at the same time. I am sure that there needs to be patches to support this heterogenous core configuration, but if we keep the doms pinned to the same CPU type then I haven't faced any issues.

For example if I had a domU pinned to CPU 3 (A7) and CPU 4 (A15) then I faced hangs.
Â

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