lkml.org 
[lkml]   [2011]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCHv1 01/12] unicore32 core architecture: build infrastructure
Date


> -----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.
>
> > +# 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:
> > +KBUILD_CFLAGS += -municore32
> ^
Ok.

> > +
> > +#Default value
> > +head-y := arch/unicore32/kernel/head.o arch/unicore32/kernel/init_task.o
> Break longer lines in two like this:
> > +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.
> > +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?
>
> > +
> > +export TEXT_OFFSET GZFLAGS
> This variable is never used.
GZFLAGS is removed.
>
>
> > +
> > +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.
>
> > +
> > +CLEAN_FILES += $(ASM_GENERIC_HEADERS)
> > +
> > +# We use MRPROPER_FILES and CLEAN_FILES now
>
> Stale comment
Removed.
>
>
> > + 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



\
 
 \ /
  Last update: 2011-01-08 12:40    [W:0.099 / U:0.364 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site