[RFC PATCH] ARM: new architecture for Energy Micro's EFM32 Cortex-M3 SoCs

[Russell King - ARM Linux]

On Mon, Jan 16, 2012 at 05:29:33PM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 21, 2011 at 07:31:56PM +0200, Ohad Ben-Cohen wrote:
> > Hi Uwe,
> > 
> > 2011/12/21 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> > > If you're interested in the details I can publish my tree I think.
> > 
> > Definitely interested - it could be very nice if you can publish that tree.
> OK, it's online now at
> 
> 	http://git.pengutronix.de/?p=ukl/linux-2.6.git;a=shortlog;h=refs/heads/efm32
> 	git://git.pengutronix.de/git/ukl/linux.git efm32
> 
> Please note that this is work in progress and has some rough edges. ...
> but at least it boots on my machine.

Some of your patches seem weird, like this:

        for (i = 0; i < ARRAY_SIZE(cache_policies); i++) {
-               int len = strlen(cache_policies[i].policy);
-
-               if (memcmp(p, cache_policies[i].policy, len) == 0) {
+               if (strcmp(p, cache_policies[i].policy) == 0) {

Please compare the two, specifically how many bytes of the strings in
each case are compared.  I think you'll find it's not an equivalent
change, and you need to discard the patch.

This looks buggy:
+#if defined(CONFIG_CPU_V7M)
+       .macro  setmode, mode, reg
+       .endm
+#elif defined(CONFIG_THUMB2_KERNEL)
        .macro  setmode, mode, reg
        mov     \reg, #\mode
        msr     cpsr_c, \reg

as it does nothing about the interrupt mask.

The VFP stuff - adding 'clean' which is kernel state to the _user_
_exported_ VFP hardware structure is a bad idea.  So this needlessly
causes a variation in the kernels userspace API.  Please find somewhere
else to keep kernel internal state.  (As that patch comes from Catalin,
then that comment is directed to Catalin.)

In "mtd/maps: uclinux: fix sparse warnings and coding style":

-struct map_info uclinux_ram_map = {
+static struct map_info uclinux_ram_map = {
        .name = "RAM",
-       .phys = (unsigned long)&_ebss,
-       .size = 0,
+       .phys = (resource_size_t)&_ebss,
+       .bankwidth = 4,

This doesn't match the description - it's a functional change.  Basic rule
of kernel patch generation: functional changes must _never_ _ever_, not in
a million years, be mixed with such patches.

+       *virt = (__force void *)(map->virt + from);

This says "wrong" to me loudly - by the mere fact that you're using __force.
If you're having to do that, leave the sparse warning in place.  Sparse is
_not_ a tool which must produce zero warnings for kernel code.  Sparse is
an auditing tool, and as such there are valid reasons to ignore warnings.
This is one of them.

No driver should ever use __force within its code or definitions.  That
priviledge is reserved for stuff like arch code inside IO accessors where
it really knows that it's safe.

In "ARM: new architecture for ..." I expected to find a new arch/*
directory.  What you mean is "new platform" or "new machine" not
"new architecture".  Maybe "new sub-architecture".  Good to see you're
selecting NO_IOPORT here though.

config CPU_V7M
        bool "Support ARMv7-M processors"
-       depends on MACH_REALVIEW_EB && EXPERIMENTAL
+       depends on (MACH_REALVIEW_EB && EXPERIMENTAL) || ARCH_EFM32
        select THUMB2_KERNEL
        select ARM_THUMB
        select CPU_32v7M

Ok, so EFM32 may or may not have a V7M CPU.  Cool.  What does it have
when it doesn't have a V7M CPU?  (Please fix it, like all the other CPU
stuff in this file, so that the prompt appears for those which _may_
have it, but it's selected by those which must _always_ have it.)

Your code in arch/arm/mach-efm32/include/mach/system.h won't be called
as of this merge window - that's something you should consider fixing.

In your serial driver, you really need to deal with which modes you
can't set sanely: ask Alan Cox how to do this.  POSIX has a requirement
that termios modes which can't be set should be reported back.  (eg,
if you can only do one stop bit, don't blindly ignore the request for
two stop bits.)