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

[Suriyan Ramasami <suriyan.r@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9774 bytes --]

On Tue, Feb 16, 2016 at 2:03 AM, Ian Campbell <ian.campbell@citrix.com>
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@citrix.com> 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@citrix.com>
> > > > 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.
>

[-- Attachment #1.2: Type: text/html, Size: 13448 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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