Re: [PATCHv1 01/12] unicore32 core architecture: build infrastructure

[Arnd Bergmann <arnd@arndb.de>]

On Saturday 25 December 2010, Guan Xuetao wrote:
> From: Guan Xuetao <guanxuetao@mprc.pku.edu.cn>
> 
> This patch implements build infrastructure.

Sorry for the late reply, I was planning to do the new review earlier, but didn't
get to it before the Christmas break.

Overall, I can see that there has been a lot of good progress in the code since
the original versions that I looked at, very nice!

> diff --git a/arch/unicore32/.gitignore b/arch/unicore32/.gitignore
> new file mode 100644
> index 0000000..f0fc866
> --- /dev/null
> +++ b/arch/unicore32/.gitignore
> @@ -0,0 +1,70 @@
> +#
> +# Generated include files
> +#
> +include/asm/atomic.h
> +include/asm/auxvec.h
> +include/asm/bitsperlong.h
> +include/asm/bug.h
> +include/asm/bugs.h
> +include/asm/cputime.h
> +include/asm/current.h
> +include/asm/device.h
> +include/asm/emergency-restart.h
> +include/asm/errno.h
> +include/asm/fb.h
> +include/asm/fcntl.h
> +include/asm/hardirq.h
> ...

Maybe it would be better to put these files into a separate directory, like
arch/unicore32/include/generated/asm, to make it easier to separate them
from the other files and avoid listing them all in .gitignore besides the
other places.

It's certainly good to see so many of these files.

> +config GENERIC_GPIO
> +	def_bool y
> +
> +config GENERIC_CLOCKEVENTS
> +	bool
> +
> +config NO_IOPORT
> +	bool
> +
> +config GENERIC_HARDIRQS
> +	bool
> +	default y

You are somewhat inconsistent with "def_bool", "default y" and "select FOO",
which all have the same effect. I would recommend using def_bool y where
possible.

> +config REALMODE
> +	bool

I don't see this being used anywhere. Is it just a leftover from an earlier
version, or did I miss something?

> +choice
> +	prompt "PKUnity system type"
> +	default ARCH_PUV3
> +
> +	config ARCH_PUV3
> +	bool "PKUnity v3 (SuperK)"
> +	select CPU_UCV2
> +	select GENERIC_CLOCKEVENTS
> +	select HAVE_CLK
> +	select ARCH_REQUIRE_GPIOLIB
> +	select ARCH_HAS_CPUFREQ
> +
> +endchoice

As long as there is only one option, you can drop the entire "choice"
statement.

> +config DEBUG_USER
> +	bool "Verbose user fault messages"
> +	help
> +	  When a user program crashes due to an exception, the kernel can
> +	  print a brief message explaining what the problem was. This is
> +	  sometimes helpful for debugging but serves no purpose on a
> +	  production system. Most people should say N here.
> +
> +	  In addition, you need to pass user_debug=N on the kernel command
> +	  line to enable this feature.  N consists of the sum of:
> +
> +	      1 - undefined instruction events
> +	      2 - system calls
> +	      4 - invalid data aborts
> +	      8 - SIGSEGV faults
> +	     16 - SIGBUS faults

We already have four architectures doing this using the "exception-trace"
sysctl and the show_unhandled_signals variable. Please follow what those
architectures are doing, or remove the option and all code depending on
it.


	Arnd