Date:	Sat, 8 Jan 2011 17:22:17 +0800
From:	"Guan Xuetao" <guanxuetao@...c.pku.edu.cn>
To:	"'Arnd Bergmann'" <arnd@...db.de>
Cc:	<linux-arch@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCHv1 01/12] unicore32 core architecture: build infrastructure


> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@...db.de]
> Sent: Friday, January 07, 2011 8:19 AM
> To: Guan Xuetao
> Cc: linux-arch@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCHv1 01/12] unicore32 core architecture: build infrastructure
> 
> On Saturday 25 December 2010, Guan Xuetao wrote:
> > From: Guan Xuetao <guanxuetao@...c.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.
Thanks for your review.

> 
> 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.

That's great, and I make the following changes:

commit 2319043024c3db0eedfadf63b37714e14325104d
Author: GuanXuetao <gxt@...c.pku.edu.cn>
Date:   Sat Jan 8 16:12:47 2011 +0800

    unicore32: move one-line asm-generic headers into arch/unicore32/include/generated/asm
    	however, 'make headers_install' can't work with two asm dirs

diff --git a/arch/unicore32/.gitignore b/arch/unicore32/.gitignore
index 815fad5..947e99c 100644
--- a/arch/unicore32/.gitignore
+++ b/arch/unicore32/.gitignore
@@ -1,57 +1,7 @@
 #
 # 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/div64.h
-include/asm/emergency-restart.h
-include/asm/errno.h
-include/asm/fb.h
-include/asm/fcntl.h
-include/asm/hardirq.h
-include/asm/hw_irq.h
-include/asm/ioctl.h
-include/asm/ioctls.h
-include/asm/ipcbuf.h
-include/asm/irq_regs.h
-include/asm/kdebug.h
-include/asm/kmap_types.h
-include/asm/local.h
-include/asm/mman.h
-include/asm/module.h
-include/asm/msgbuf.h
-include/asm/param.h
-include/asm/parport.h
-include/asm/percpu.h
-include/asm/poll.h
-include/asm/posix_types.h
-include/asm/resource.h
-include/asm/scatterlist.h
-include/asm/sections.h
-include/asm/sembuf.h
-include/asm/serial.h
-include/asm/shmbuf.h
-include/asm/shmparam.h
-include/asm/siginfo.h
-include/asm/signal.h
-include/asm/socket.h
-include/asm/sockios.h
-include/asm/statfs.h
-include/asm/termbits.h
-include/asm/termios.h
-include/asm/topology.h
-include/asm/types.h
-include/asm/ucontext.h
-include/asm/unaligned.h
-include/asm/user.h
-include/asm/vga.h
-include/asm/xor.h
+include/generated
 #
 # Generated ld script file
 #
diff --git a/arch/unicore32/Makefile b/arch/unicore32/Makefile
index 8718152..2e03fae 100644
--- a/arch/unicore32/Makefile
+++ b/arch/unicore32/Makefile
@@ -60,60 +60,33 @@ all:	$(KBUILD_IMAGE)
 
 boot := arch/unicore32/boot
 
-ASM_GENERIC_HEADERS	:= arch/unicore32/include/asm/atomic.h			\
-			   arch/unicore32/include/asm/auxvec.h			\
-			   arch/unicore32/include/asm/bitsperlong.h		\
-			   arch/unicore32/include/asm/bug.h			\
-			   arch/unicore32/include/asm/bugs.h			\
-			   arch/unicore32/include/asm/cputime.h			\
-			   arch/unicore32/include/asm/current.h			\
-			   arch/unicore32/include/asm/device.h			\
-			   arch/unicore32/include/asm/div64.h			\
-			   arch/unicore32/include/asm/emergency-restart.h	\
-			   arch/unicore32/include/asm/errno.h			\
-			   arch/unicore32/include/asm/fb.h			\
-			   arch/unicore32/include/asm/fcntl.h			\
-			   arch/unicore32/include/asm/hardirq.h			\
-			   arch/unicore32/include/asm/hw_irq.h			\
-			   arch/unicore32/include/asm/ioctl.h			\
-			   arch/unicore32/include/asm/ioctls.h			\
-			   arch/unicore32/include/asm/ipcbuf.h			\
-			   arch/unicore32/include/asm/irq_regs.h		\
-			   arch/unicore32/include/asm/kdebug.h			\
-			   arch/unicore32/include/asm/kmap_types.h		\
-			   arch/unicore32/include/asm/local.h			\
-			   arch/unicore32/include/asm/mman.h			\
-			   arch/unicore32/include/asm/module.h			\
-			   arch/unicore32/include/asm/msgbuf.h			\
-			   arch/unicore32/include/asm/param.h			\
-			   arch/unicore32/include/asm/parport.h			\
-			   arch/unicore32/include/asm/percpu.h			\
-			   arch/unicore32/include/asm/poll.h			\
-			   arch/unicore32/include/asm/posix_types.h		\
-			   arch/unicore32/include/asm/resource.h		\
-			   arch/unicore32/include/asm/scatterlist.h		\
-			   arch/unicore32/include/asm/sections.h		\
-			   arch/unicore32/include/asm/sembuf.h			\
-			   arch/unicore32/include/asm/serial.h			\
-			   arch/unicore32/include/asm/shmbuf.h			\
-			   arch/unicore32/include/asm/shmparam.h		\
-			   arch/unicore32/include/asm/siginfo.h			\
-			   arch/unicore32/include/asm/signal.h			\
-			   arch/unicore32/include/asm/socket.h			\
-			   arch/unicore32/include/asm/sockios.h			\
-			   arch/unicore32/include/asm/statfs.h			\
-			   arch/unicore32/include/asm/termbits.h		\
-			   arch/unicore32/include/asm/termios.h			\
-			   arch/unicore32/include/asm/topology.h		\
-			   arch/unicore32/include/asm/types.h			\
-			   arch/unicore32/include/asm/ucontext.h		\
-			   arch/unicore32/include/asm/unaligned.h		\
-			   arch/unicore32/include/asm/user.h			\
-			   arch/unicore32/include/asm/vga.h			\
-			   arch/unicore32/include/asm/xor.h
+ASM_GENERATED_DIR	:= $(srctree)/arch/unicore32/include/generated
+LINUXINCLUDE		+= -I$(ASM_GENERATED_DIR)
+
+ASM_GENERIC_HEADERS	:= atomic.h auxvec.h				\
+			   bitsperlong.h bug.h bugs.h			\
+			   cputime.h current.h				\
+			   device.h div64.h				\
+			   emergency-restart.h errno.h			\
+			   fb.h	fcntl.h					\
+			   hardirq.h hw_irq.h				\
+			   ioctl.h ioctls.h ipcbuf.h irq_regs.h		\
+			   kdebug.h kmap_types.h			\
+			   local.h					\
+			   mman.h module.h msgbuf.h			\
+			   param.h parport.h percpu.h poll.h posix_types.h \
+			   resource.h					\
+			   scatterlist.h sections.h sembuf.h serial.h	\
+			   shmbuf.h shmparam.h siginfo.h signal.h	\
+			   socket.h sockios.h statfs.h			\
+			   termbits.h termios.h topology.h types.h	\
+			   ucontext.h unaligned.h user.h		\
+			   vga.h					\
+			   xor.h
 
 %.h:
-	$(Q)echo '#include <asm-generic/$(notdir $@)>' > $@
+	$(Q)mkdir -p $(ASM_GENERATED_DIR)/asm
+	$(Q)echo '#include <asm-generic/$(notdir $@)>' > $(ASM_GENERATED_DIR)/asm/$@
 
 archprepare: $(ASM_GENERIC_HEADERS)
 
@@ -126,7 +99,7 @@ zImage Image uImage: vmlinux
 zinstall install: vmlinux
 	$(Q)$(MAKE) $(build)=$(boot) MACHINE=$(MACHINE) $@
 
-CLEAN_FILES += $(ASM_GENERIC_HEADERS)
+MRPROPER_DIRS	+= $(ASM_GENERATED_DIR)
 
 # We use MRPROPER_FILES and CLEAN_FILES now
 archclean:

However, 'make headers_install' can't work with two asm dirs.
The ugly alternative method is copying all files in arch/unicore32/include/asm
to arch/unicore32/include/generated/asm. Then using the latter for headers_install.

> 
> 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.
Ok.

> 
> > +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?
Removed.

> 
> > +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.
Ok.

> 
> > +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.
Removed now.

> 
> 
> 	Arnd
Thanks Arnd.

Guan Xuetao

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists