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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2016-02-09 at 04:50 -0800, Suriyan Ramasami wrote:
> 
> 
> 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 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.

Does the OS need to be aware of all these combinations though? Is it not
sufficient to know how to bring up an A15 core and how to bring up an A7
core and then just do so based on the information in the DTS, without
needing to worry about which sort of core we happened to have booted on?

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

How much of this is down to physical h/w limitations and how much of it is
down to firmware or software limitations? Can you flip the to the other
cluster somehow?

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

If that's the case then I'm sorry but I'm rather wary of taking this patch
as it is, since the defaults are all to allow vCPUs to migrate around, so
people are just going to get a broken system out of the box.

Until actual support for heterogeneous cores is in place I'd much prefer to
only bring up one cluster, I don't mind if it is

    Always the A7s
    Always the A15s
    Always whatever the system has booted on
    Somehow configurable at boot time by the user.

Another option, which is a bit more work but not huge, would be to
automatically partition the system into 2 cpupools at boot, corresponding
to the two clusters of CPUs and ensure that dom0 is resident in only one
cpu pool, I think that would be the minimally workable arrangement for a
heterogeneous system.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel