[PATCH 4/6] arm64: topology: Implement basic CPU topology support

[Lorenzo Pieralisi]

On Mon, Dec 16, 2013 at 12:29:48PM +0000, Mark Brown wrote:
> On Mon, Dec 16, 2013 at 10:57:34AM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Dec 11, 2013 at 01:13:24PM +0000, Mark Brown wrote:
> 
> Replying again since I didn't notice half the content here - please
> delete unneeded context from your mails, it makes it much easier to 
> find the content you've added.
> 
> > > +const struct cpumask *cpu_coregroup_mask(int cpu);
> > > +int cluster_to_logical_mask(unsigned int socket_id, cpumask_t *cluster_mask);
> 
> > I think the function signature above needs changing when we move to DT
> > topology bindings, put it differently the look up won't be based on a
> > socket id anymore, I need some time to think about it.
> 
> Well, we need to consider the possibility of ACPI or whatever as well.

That's a fair point, I will have a look at v2.

[...]

> > > +       /*
> > > +        * Create cpu topology mapping, assume the cores are largely
> > > +        * independent since the DT bindings do not include the flags
> > > +        * for MT.
> > > +        */
> > > +       cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > > +       cpuid_topo->socket_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> 
> > This is what we are trying to prevent.  The way levels are mapped to
> > core and cluster id is just a recommendation. Better to follow DT bindings,
> > they are stricter and provide us with all the required bits of information.
> 
> Again, this is gone from the current version.  Like I said to Catalin it
> does feel like this is making more work for systems that have done the
> right thing with their MPIDRs which doesn't seem ideal (and note that
> all the DTs that you guys are publishing for your models lack any
> topology information at present).

This is an age-old question and the problem has always been that the
"right thing" is recommended, not enforced. I do not want to turn this into
bikeshedding, as long as cpu-map node takes priority if present, fine by me.

> > > +       for_each_online_cpu(cpu)
> > > +               if (socket_id == topology_physical_package_id(cpu)) {
> > > +                       cpumask_copy(cluster_mask, topology_core_cpumask(cpu));
> > > +                       return 0;
> > > +               }
> > > +
> > > +       return -EINVAL;
> > > +}
> 
> > As mentioned, I think this function will have to change. Masks can be
> > built using phandles to topology nodes. I know this is how cluster masks
> > are currently built in arm32 kernels, but this does not mean that's the
> > correct approach, given the laxity of the MPIDR specification.
> 
> So, this can actually be removed completely since there aren't any
> references to this function any more that said the socket_id assignment
> is still there...
> 
> This isn't being done using MPDIR, it's being done based on using the
> lowest level of cluster however we found it.  What we're doing is that
> while parsing the topology information source we use it to pick a
> physical package identifier and then later on we use that identifier as
> a handle.  The socket ID isn't really taken from a field in the MPDIR,
> it's the result of doing the mapping you mention.
> 
> I definitely don't think we should be using phandles directly unless we
> want to have a separate implementation for ACPI (or whatever else might
> come up) which would mean less encapsulation of the topology code.
> Having the parsing code assign socket IDs doesn't seem like a
> particularly bad way of doing things, we need IDs for the generic
> topology API functions to use anyway.

It makes sense, again I will have a look at v2 and comment on that.

Thanks,
Lorenzo