[PATCH] ARM: make it easier to check the CPU part number correctly

[Russell King - ARM Linux]

On Thu, Jun 26, 2014 at 05:26:18PM +0100, Mark Rutland wrote:
> Hi Russell,
> 
> On Tue, Jun 24, 2014 at 07:39:08PM +0100, Russell King - ARM Linux wrote:
> > Ensure that platform maintainers check the CPU part number in the right
> > manner: the CPU part number is meaningless without also checking the
> > CPU implement(e|o)r (choose your preferred spelling!)  Provide an
> > interface which returns both the implementer and part number together,
> > and update the definitions to include the implementer.
> > 
> > Mark the old function as being deprecated... indeed, using the old
> > function with the definitions will now always evaluate as false, so
> > people must update their un-merged code to the new function.  While
> > this could be avoided by adding new definitions, we'd also have to
> > create new names for them which would be awkward.
> > 
> > Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
> > ---
> >  arch/arm/include/asm/cputype.h         | 37 +++++++++++++++--------
> >  arch/arm/include/asm/smp_scu.h         |  2 +-
> >  arch/arm/kernel/perf_event_cpu.c       | 55 +++++++++++++++++-----------------
> >  arch/arm/kvm/guest.c                   |  8 +----
> >  arch/arm/mach-exynos/mcpm-exynos.c     |  2 +-
> >  arch/arm/mach-exynos/platsmp.c         |  4 +--
> >  arch/arm/mach-vexpress/tc2_pm.c        |  2 +-
> >  arch/arm/mm/cache-l2x0.c               |  2 +-
> >  arch/arm64/include/asm/cputype.h       |  5 ++++
> >  drivers/clocksource/arm_global_timer.c |  2 +-
> >  10 files changed, 64 insertions(+), 55 deletions(-)
> 
> It looks like you missed drivers/cpuidle/cpuidle-big_little.c, which has
> a homebrew comparison in bl_idle_driver_init.
> 
> I'm slightly confused by the diffstat -- it shows changes to
> arch/arm64/include/asm/cputype.h, for which I can't see the
> corresponding diff. Did you mean to drop that?

Yes, I don't think arm_global_timer.c is used by ARM64, so I dropped that
change.

Yes, it looks like I missed cpuidle-big_little.c, but now that you point
it out, I have to ask whether this is really coded in an easy to understand
way:

                /* read cpu id part number */
                if ((cpuid & 0xFFF0) == cpu_id)
                        cpumask_set_cpu(cpu, cpumask);

It is /very/ easy to read that as (cpuid & 0xfff0) == cpuid dropping the
additional underscore.  It may make sense to rename cpu_id to be
desired_id.  I'd also suggest that this test becomes:

		if (((cpuid ^ desired_id) & 0xff00fff0) == 0)
                        cpumask_set_cpu(cpu, cpumask);

or:

		if ((cpuid & 0xff00fff0) == (desired_id & 0xff00fff0))
                        cpumask_set_cpu(cpu, cpumask);

as that's what we're really wanting - to check that certain bits match
between the two IDs and not care about the rest.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.