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

[Suriyan Ramasami <suriyan.r@xxxxxxxxx>]

On Tue, Feb 16, 2016 at 2:03 AM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:

On Sun, 2016-02-14 at 22:32 -0800, Suriyan Ramasami wrote:
>
>
> On Thu, Feb 11, 2016 at 1:40 AM, Ian Campbell <
> ian.campbell@xxxxxxxxxx> wrote:
> > On Wed, 2016-02-10 at 17:47 -0800, Suriyan Ramasami wrote:
> > >
> > >
> > > On Wed, Feb 10, 2016 at 2:03 AM, Ian Campbell <
> > ian.campbell@xxxxxxxxxx>
> > > wrote:
> > > > On Tue, 2016-02-09 at 10:20 -0800, Suriyan Ramasami wrote:
> > > > >Â 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?
> > > > > >
> > > > > >
> > > > > Unfortunately, at least looking at the boot up code for the
> > > > Exynos5422,
> > > > > the OS needs to be aware of it. This is what I see in the
> > linux
> > > > source
> > > > > code. If it boots up on an A7, then a special reset is needed
> > which
> > > > is
> > > > > not needed when booted up otherwise. I do not have much more
> > details
> > > > on
> > > > > that other than the Linux code.
> > > > > Without that reset sequence, I have also verified that the
> > powered on
> > > > CPU
> > > > > does not come up.
> > > >
> > > > Are we able to say that if we are booted on cluster 1 (always
> > the A7s)
> > > > then
> > > > we always need this magic reset? i.e. is true for all SoCs
> > which have
> > > > an A7
> > > > cluster and can boot from it? (it's tautologically true for
> > SocS which
> > > > either have no A7's or cannot boot from them).
> > > >
> > > I do not have the information to answer the question. I am
> > limited to
> > > what I know (albeit a little bit) wrt the hardkernel related
> > boards -
> > > Exynos 5410 (odroid-XU) and the Exynos 5422 (Odoird XU3/XU4).
> > With my
> > > limited knowledge, I am only aware of Exynos 5410 which is
> > capable of
> > > booting off of an A7 or an A15.
> > >
> > > >Â Maybe I'm looking for similarities between different exynos
> > variants
> > > > which
> > > > doesn't exist though. If we are going to talk about specific
> > SoCs in
> > > > the
> > > > comments then I would rather that the code was also explicit
> > rather
> > > > than
> > > > assuming cluster 1 will only be found on the 5800, that might
> > be as
> > > > simple
> > > > as mapping the compatible string to a max_cluster (default 0
> > for
> > > > unknown
> > > > SoC) and warning if pcluster > max_cluster.
> > > Can you please elaborate on the mapping that you talk about
> > above. I am
> > > lost here :-(
> >
> > What I mean is can we say:
> >Â Â Âexynos 1234 => Two clusters (max_cluster == 1)
> >Â Â Âexynos 5678 => One cluster (max_cluster == 0)
> >Â Â Âexynos ABCD => Two clusters (max_cluster == 1)
> >  ÂUnknown  Â=> Assume one cluster
> >
> > and can we also assume that cluster 0 always consists of A15s and
> > cluster 1
> > (if it exists) always consists of A7s?
> >
> > If so then we can say:
> >
> >Â Âmax_cluster = look_up_by_compat(compat)
> >Â Âpcluster = figure out from midr
> >Â Âpcpu = figure it out
> >
> >Â Âif (pcluster >= max_cluster)
> >Â Â Âerror
> >
> >Â Âdo bringup
> >
> >Â Âif (pluster == 1)
> >Â Â Âdo special handling for cluster 1 == a7
> >
> > The difference compared with what you have is that it adds a check
> > that we
> > expect a second cluster for the SoC before it goes poking at stuff.
> >
> > What I'm trying to avoid is coming across some other SoC variant
> > which has
> > 2 clusters but has something different to the A7s or which requires
> > some
> > different handling.
> >
> > If we were confident that all exynosXXXX SoCs always require the
> > same
> > special handling for cluster 1 then we wouldn't really need this,
> > but I
> > don't think we know that?
> >
> >
> I did some looking at the linux 4.4.y dts for exynos, and this is
> what I see:
>

[...]
> If we look at the ones that can potentially support hardware
> virtualization, limits us to the A7s and the A15s. That gives us:
> exynos3250: cpu0, cpu1 = A7 (1 cluster)
> exynos5250: cpu0, cpu1 = A15 (1 cluster)
> exynos5260: cpu0, cpu1 = A15. cpu2, cpu3, cpu4, cpu5 = A7 (2
> clusters)
> exynos5410: cpu0, cpu1, cpu2, cpu3 = A15 (1 cluster)
> exynos5420: cpu0, cpu1, cpu2, cpu3 = A15. cpu4, cpu5, cpu6, cpu7 = A7
> (2 clusters)
> exynos5422: cpu0, cpu1, cpu2, cpu3 = A7. cpu4, cpu5, cpu6, cpu7 = A15
> (2 clusters)
> exynos5440: cpu0, cpu1, cpu2, cpu3 = A15 (1 cluster)
>
> Of these the only ones which has A7 as the 1st cluster are:
> exynos3250: cpu0, cpu1 = A7
> exynos5420: cpu0, cpu1, cpu2, cpu3 = A15. cpu4, cpu5, cpu6, cpu7 = A7
> (2 clusters)

Did you mean 5422 for this second one i.e.
exynos5422: cpu0, cpu1, cpu2, cpu3 = A7. cpu4, cpu5, cpu6, cpu7 = A15 (2 clusters)
? (I'm assuming you did)

In such configurations does the second cluster (A15s in this case) need
the same magic dance as you were adding for the A7s in the other cases?

> Note that the exynos5800 has the same cpu config as the exysno5420.
>
> I was looking at the cpu bring up code, and notice that if the secondary cpu being brought up is an A7, then it invariably does the below:
> For the exynos3250: in platsmp.c
> void exynos_core_restart(u32 core_id)
> {
>Â Â Â Â Âu32 val;
>
>Â Â Â Â Âif (!of_machine_is_compatible("samsung,exynos3250"))
>Â Â Â Â Â Â Â Â Âreturn;
>
>Â Â Â Â Âwhile (!pmu_raw_readl(S5P_PMU_SPARE2))
>Â Â Â Â Â Â Â Â Âudelay(10);
>Â Â Â Â Âudelay(10);
>
>Â Â Â Â Âval = pmu_raw_readl(EXYNOS_ARM_CORE_STATUS(core_id));
>Â Â Â Â Âval |= S5P_CORE_WAKEUP_FROM_LOCAL_CFG;
>Â Â Â Â Âpmu_raw_writel(val, EXYNOS_ARM_CORE_STATUS(core_id));
>
>Â Â Â Â Âpmu_raw_writel(EXYNOS_CORE_PO_RESET(core_id), EXYNOS_SWRESET);
> }
>
> And for the exynos5800/exynos5422: mcpm-exynos.c
> static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
> {
>Â Â Â Â Âunsigned int cpunr = cpu + (cluster * EXYNOS5420_CPUS_PER_CLUSTER);
>
>Â Â Â Â Âpr_debug("%s: cpu %u cluster %u\n", __func__, cpu, cluster);
>Â Â Â Â Âif (cpu >= EXYNOS5420_CPUS_PER_CLUSTER ||
>Â Â Â Â Â Â Â Â Âcluster >= EXYNOS5420_NR_CLUSTERS)
>Â Â Â Â Â Â Â Â Âreturn -EINVAL;
>
>Â Â Â Â Âif (!exynos_cpu_power_state(cpunr)) {
>Â Â Â Â Â Â Â Â Âexynos_cpu_power_up(cpunr);
>
>Â Â Â Â Â Â Â Â Â/*
>Â Â Â Â Â Â Â Â Â * 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 (cluster &&
>Â Â Â Â Â Â Â Â Â Â Âcluster == 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.
>Â Â Â Â Â Â Â Â Â Â Â Â Â */
>Â Â Â Â Â Â Â Â Â Â Â Â Âwhile (!pmu_raw_readl(S5P_PMU_SPARE2))
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âudelay(10);
>
>Â Â Â Â Â Â Â Â Â Â Â Â Âpmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
>Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂEXYNOS_SWRESET);
>Â Â Â Â Â Â Â Â Â}
>Â Â Â Â Â}
>
>Â Â Â Â Âreturn 0;
> }
>
> This pretty much indicates that when bringing up the A7s, the special
> reset sequence is deemed essential.

If (and only if) they are in the second cluster?

>Â Probably, we could generalize that it might be true for future
> exynos's having A7s.

This whole thing has turned into a bit of a tarpit, sorry :-/

I think I'd be happiest with code which explicitly checked for SoC
configurations which we know about (and ideally can test) over code
which tries to predict what future SoC variants will do -- we can
always patch them in later.

Essentially I think that just means adding some machine_is_compatible
type checks to the code you already have rather than relying on the
overall compatibility list only have a couple of entries right now and
implicitly relying on the differences between them (the
presence/absence of a second cluster in this case).

I agree. Instead of generalizing (which this patch did), I will do a function call for the A7 powering up sequence, in case it's machine_is_compatible with exynos5800.

I am still moping over the cpupool suggestion :-)
Â

Ian.

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