-----Original Message-----
From: Sam Ravnborg [mailto:sam@ravnborg.org]
Sent: Saturday, January 08, 2011 3:21 PM
To: Guan Xuetao
Cc: 'Paul Mundt'; linux-arch@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCHv1 01/12] unicore32 core architecture: build infrastructure

Hi Guan.

A few nits.

quoted

+# Explicitly specifiy 32-bit UniCore ISA:
+KBUILD_CFLAGS	+=$(call cc-option,-municore32)

If gcc does not support -municore32 then I assume it
is a wrong gcc version used.
So just add it unconditionally.

The cc-option macro is used to add options to gcc iff gcc
supports this option. So each time you use cc-option we
actually run gcc to determine if the opton is supported.

Corrected.

Also as a general rule add a space after the equal sign:

quoted

+KBUILD_CFLAGS	+= -municore32

                          ^

Ok.

quoted

+
+#Default value
+head-y		:= arch/unicore32/kernel/head.o arch/unicore32/kernel/init_task.o

Break longer lines in two like this:

quoted

+head-y		:= arch/unicore32/kernel/head.o
+head-y		+= arch/unicore32/kernel/init_task.o

Note: I see lots of Makefile where longer lines are continued on
the next line using a '\'. But like in regular C code to use an incremental
assignment looks just better / is more clear.

Ok.

quoted

+textofs-y	:= 0x00408000
+
+# The byte offset of the kernel image in RAM from the start of RAM.
+TEXT_OFFSET := $(textofs-y)

If you are going to have different TEXT_OFFSET's then I suggest to move
this to KConfig as an "hex "Text offset" config option.
You can set default values dependign on BSP etc.

There is no different TEXT_OFFSET.

Also defiing stuff here just to export it for use in boot/
has always looked like a strange concept - but many archs do so today.
You do not export TEXT_OFFSET but I guess this is a bug?

I need TEXT_OFFSET for kernel/ and boot/, so export it.
What's the better method?

quoted

+
+export	TEXT_OFFSET GZFLAGS

This variable is never used.

GZFLAGS is removed.

quoted

+
+core-y				+= arch/unicore32/kernel/ arch/unicore32/mm/
+core-$(CONFIG_UNICORE_FPU_F64)	+= arch/unicore32/uc-f64/
+
+drivers-$(CONFIG_ARCH_PUV3)	+= drivers/staging/puv3/
+
+libs-y				+= arch/unicore32/lib/
+# include libc.a in libs-y for string functions, like memcpy and so on.
+libs-y				+= $(shell $(CC) $(KBUILD_CFLAGS) -print-file-name=libc.a)
+libs-y				+= $(shell $(CC) $(KBUILD_CFLAGS) -print-file-name=libgcc.a)
+

The other three archs that uses libgcc use:

    $(shell $(CC) $(KBUILD_CFLAGS) -print-libgcc-file-name)

So when I read the above I am confused why it looks different than the others.
For libc I guess you do nto have that option and what you do is fine there.

It's the same with -print-libgcc-file-name and -print-file-name=libgcc.a.
And we need libc.a for string like functions.

quoted

+
+CLEAN_FILES += $(ASM_GENERIC_HEADERS)
+
+# We use MRPROPER_FILES and CLEAN_FILES now

Stale comment

Removed.

quoted

+  echo  '                  Install using (your) ~/bin/$(INSTALLKERNEL) or'
+  echo  '                  (distribution) /sbin/$(INSTALLKERNEL) or'
+  echo  '                  install to $$(INSTALL_PATH) and run lilo'

I do not think the three lines above is correct for unicore32.
Looks like they are left from a copy from x86.

Removed.

	Sam

Thanks Sam.

Guan Xuetao