Patchwork [v3,2/2] buildroot: target: Add Blackfin architecture support.

login
register
mail settings
Submitter Sonic Zhang
Date March 22, 2013, 9:01 a.m.
Message ID <1363942902-6045-2-git-send-email-sonic.adi@gmail.com>
Download mbox | patch
Permalink /patch/229927/
State Changes Requested
Headers show

Comments

Sonic Zhang - March 22, 2013, 9:01 a.m.
From: Sonic Zhang <sonic.zhang@analog.com>

v3-changes:
- Put space around variable assignments.

v2-changes:
- Create arch makefile.

v1-changes:
- Create blackfin makefile.
- Add blackfin target ABI options.
- Add blackfin cpu options and shared library installation options.
- Add blackfin cpu revision options and mcpu link flags
- Add blackfin FLAT specific makefile flags.
- Add shared library installation options and makefile targets to
install shared libraries into rootfs image.
- Copy extra blackfin toolchain FDPIC shared libraries to target fs

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
---
 Makefile              |    2 +
 arch/Config.in.bfin   |  117 +++++++++++++++++++++++++++++++++++++++++++++++++
 arch/Makefile.in      |    5 ++
 arch/Makefile.in.bfin |   50 +++++++++++++++++++++
 4 files changed, 174 insertions(+), 0 deletions(-)
 create mode 100644 arch/Makefile.in
 create mode 100644 arch/Makefile.in.bfin
Thomas Petazzoni - March 22, 2013, 2:54 p.m.
Dear Sonic Zhang,

Please add a commit log explaining all what you're doing. Your changes
are quite architecture-specific and touch the core of Buildroot, but
there are also not comments and no commit log.

Also your changelog should be below the --- and not before, otherwise
it ends up in the commit log forever.

On Fri, 22 Mar 2013 17:01:42 +0800, Sonic Zhang wrote:

> diff --git a/Makefile b/Makefile
> index 7f0822f..c2f43a4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -329,6 +329,8 @@ ifneq ($(PACKAGE_OVERRIDE_FILE),)
>  -include $(PACKAGE_OVERRIDE_FILE)
>  endif
>  
> +include arch/Makefile.in

I must admit I am not entirely happy with having Blackfin-specific
Makefiles, but it's true that we already have some
architecture-specific crap in package/Makefile.in that we could more
cleanly spread in arch/Makefile.in.<arch>.

> diff --git a/arch/Config.in.bfin b/arch/Config.in.bfin
> index 0b137ae..0750b86 100644
> --- a/arch/Config.in.bfin
> +++ b/arch/Config.in.bfin
> @@ -7,10 +7,127 @@ config BR2_BFIN_FDPIC
>  config BR2_BFIN_FLAT
>  	bool "FLAT"
>  	select BR2_PREFER_STATIC_LIB
> +config BR2_BFIN_FLAT_SEP_DATA
> +	bool "FLAT (Separate data)"
> +	select BR2_PREFER_STATIC_LIB
> +config BR2_BFIN_SHARED_FLAT
> +	bool "Shared FLAT"
> +	select BR2_PREFER_STATIC_LIB
> +endchoice

I don't think we should have Blackfin-specific options. As I suggested
in the review of PATCH 1/2, we should probably have global
BR2_BINFMT_<foo> options.

> +config BR2_TARGET_CPU_REVISION
> +	string "Target CPU revision"

Help text needed.

> +config BR2_BFIN_INSTALL_ELF_SHARED
> +	depends on BR2_bfin && !BR2_BFIN_FDPIC

Confused: why in the FDPIC case you would not install the ELF shared
libraries?

> +	bool "Install ELF shared libraries"
> +	default y

Help text needed.

> +config BR2_BFIN_INSTALL_FLAT_SHARED
> +	depends on BR2_bfin
> +	bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
> +	default y

Help text needed.

> +
>  config BR2_ARCH
>  	default "bfin"
>  
>  config BR2_ENDIAN
>          default "LITTLE"
> +
> +config BR2_GCC_TARGET_CPU
> +	default bf606		if BR2_bf606
> +	default bf607		if BR2_bf607
> +	default bf608		if BR2_bf608
> +	default bf609		if BR2_bf609
> +	default bf512		if BR2_bf512
> +	default bf514		if BR2_bf514
> +	default bf516		if BR2_bf516
> +	default bf518		if BR2_bf518
> +	default bf522		if BR2_bf522
> +	default bf523		if BR2_bf523
> +	default bf524		if BR2_bf524
> +	default bf525		if BR2_bf525
> +	default bf526		if BR2_bf526
> +	default bf527		if BR2_bf527
> +	default bf531		if BR2_bf531
> +	default bf532		if BR2_bf532
> +	default bf533		if BR2_bf533
> +	default bf534		if BR2_bf534
> +	default bf536		if BR2_bf536
> +	default bf537		if BR2_bf537
> +	default bf538		if BR2_bf538
> +	default bf539		if BR2_bf539
> +	default bf542		if BR2_bf542
> +	default bf544		if BR2_bf544
> +	default bf547		if BR2_bf547
> +	default bf548		if BR2_bf548
> +	default bf549		if BR2_bf549
> +	default bf561		if BR2_bf561
> +
> +config BR2_TARGET_ABI_FLAT
> +	default n		if BR2_BFIN_FDPIC
> +	default y

To be refactored with the BR2_BINFMT_<foo> stuff.

> diff --git a/arch/Makefile.in b/arch/Makefile.in
> new file mode 100644
> index 0000000..d791118
> --- /dev/null
> +++ b/arch/Makefile.in
> @@ -0,0 +1,5 @@
> +# The architecture specific Makefile.in.$ARCH should be included only
> +# when the arch macro is selected.
> +ifeq ($(BR2_bfin),y)
> +include arch/Makefile.in.bfin
> +endif

Ok.

> diff --git a/arch/Makefile.in.bfin b/arch/Makefile.in.bfin
> new file mode 100644
> index 0000000..e16532a
> --- /dev/null
> +++ b/arch/Makefile.in.bfin
> @@ -0,0 +1,50 @@
> +TARGETS-y =

Isn't that dangerous if someone else is using TARGETS-y ?

> +TARGETS-$(BR2_BFIN_INSTALL_ELF_SHARED) += romfs.shared.libs.elf
> +TARGETS-$(BR2_BFIN_INSTALL_FLAT_SHARED) += romfs.shared.libs.flat

What's the relation with "romfs" ?

Also, we more or less name all our targets target-<something>, so it
would be good to follow this convention. Also remember to update
the TARGET_EXCEPTIONS variable in support/scripts/graph-depends when
you add more of such custom targets.

> +TARGETS += $(TARGETS-y)
> +
> +ifeq ($(BR2_BFIN_FDPIC),y)
> +USR_LIB_EXTERNAL_LIBS += libgfortran.so libgomp.so libmudflap.so libmudflapth.so libobjc.so
> +endif

This is totally unrelated to Blackfin, and therefore has no reason to
be here and to depend on BR2_BFIN_FDPIC.

> +CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-

No reason to be here. Please use $(TARGET_CC) and $(TARGET_READELF)
below.

> +romfs.shared.libs.elf:
> +	set -e; \
> +	t=`$(CROSS_COMPILE_SHARED_ELF)gcc $(CPUFLAGS) -print-file-name=libc.a`; \
> +	t=`dirname $$t`/../..; \
> +	for i in $$t/lib/*so*; do \
> +		i=`readlink -f "$$i"`; \
> +		soname=`$(CROSS_COMPILE_SHARED_ELF)readelf -d "$$i" | sed -n '/(SONAME)/s:.*[[]\(.*\)[]].*:\1:p'`; \
> +		$(INSTALL) -D $$i $(TARGET_DIR)/lib/$$soname; \
> +	done

Isn't this done already by the external toolchain logic? Some
documentation/comments on what you're doing here would be good. I'm
trying to understand if it's really Blackfin-specific, if it's specific
to the usage of external toolchains or would apply to the internal
toolchain backend as well, etc.

> +CROSS_COMPILE_SHARED_FLAT ?= bfin-uclinux-

TARGET_CC to be used below.

> +romfs.shared.libs.flat:
> +	set -e; \
> +	t=`$(CROSS_COMPILE_SHARED_FLAT)gcc $(CPUFLAGS) -mid-shared-library -print-file-name=libc`; \
> +	if [ -f $$t -a ! -h $$t ] ; then \
> +		$(INSTALL) -D $$t $(TARGET_DIR)/lib/lib1.so; \
> +	fi
> +
> +ifeq ($(BR2_TARGET_CPU_REVISION),)
> +TARGET_CPU = -mcpu=$(BR2_GCC_TARGET_CPU)
> +else
> +TARGET_CPU = -mcpu=$(BR2_GCC_TARGET_CPU)-$(BR2_TARGET_CPU_REVISION)
> +endif
> +TARGET_CFLAGS += $(call qstrip,$(TARGET_CPU))

This will not work for the external toolchain wrapper logic. The
external toolchain wrapper logic (in
toolchain/toolchain-external/ext-tool.mk) takes the value of
BR2_GCC_TARGET_CPU and encodes it as a -mcpu in the external toolchain
wrapper. We want this to be part of the external toolchain wrapper
instead of TARGET_CFLAGS so that people directly using the external
toolchain from host/usr/bin/ go through the wrapper, and end up having
the same mcpu/march/mtune options as the Buildroot build.

> +ifneq ($(BR2_USE_MMU), y)
> +TARGET_CFLAGS += -D__uClinux__
> +endif

Why do you have this one here in a Blackfin-specific area, and the
-D__NOMMU__ in PATCH 1/2 in an architecture-generic area?

> +ifeq ($(BR2_BFIN_FLAT_SEP_DATA),y)
> +TARGET_LDFLAGS += -msep-data
> +TARGET_CFLAGS += -msep-data
> +TARGET_CXXFLAGS += -msep-data
> +endif
> +
> +ifeq ($(BR2_BFIN_SHARED_FLAT), y)
> +TARGET_LDFLAGS += -mid-shared-library -mshared-library-id=0
> +TARGET_CFLAGS += -mid-shared-library -mshared-library-id=0
> +TARGET_CXXFLAGS += -mid-shared-library -mshared-library-id=0
> +endif

I think those options are used to select which multilib variant to use
in the toolchain. Therefore, they should be encoded into the external
toolchain wrapper and not just passed in TARGET_LDFLAGS, TARGET_CFLAGS
and TARGET_CXXFLAGS.

Best regards,

Thomas
Sonic Zhang - March 25, 2013, 11:33 a.m.
Hi Thomas,

On Fri, Mar 22, 2013 at 10:54 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Sonic Zhang,
>
> Please add a commit log explaining all what you're doing. Your changes
> are quite architecture-specific and touch the core of Buildroot, but
> there are also not comments and no commit log.
>
> Also your changelog should be below the --- and not before, otherwise
> it ends up in the commit log forever.
>
> On Fri, 22 Mar 2013 17:01:42 +0800, Sonic Zhang wrote:
>
>> diff --git a/Makefile b/Makefile
>> index 7f0822f..c2f43a4 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -329,6 +329,8 @@ ifneq ($(PACKAGE_OVERRIDE_FILE),)
>>  -include $(PACKAGE_OVERRIDE_FILE)
>>  endif
>>
>> +include arch/Makefile.in
>
> I must admit I am not entirely happy with having Blackfin-specific
> Makefiles, but it's true that we already have some
> architecture-specific crap in package/Makefile.in that we could more
> cleanly spread in arch/Makefile.in.<arch>.
>
>> diff --git a/arch/Config.in.bfin b/arch/Config.in.bfin
>> index 0b137ae..0750b86 100644
>> --- a/arch/Config.in.bfin
>> +++ b/arch/Config.in.bfin
>> @@ -7,10 +7,127 @@ config BR2_BFIN_FDPIC
>>  config BR2_BFIN_FLAT
>>       bool "FLAT"
>>       select BR2_PREFER_STATIC_LIB
>> +config BR2_BFIN_FLAT_SEP_DATA
>> +     bool "FLAT (Separate data)"
>> +     select BR2_PREFER_STATIC_LIB
>> +config BR2_BFIN_SHARED_FLAT
>> +     bool "Shared FLAT"
>> +     select BR2_PREFER_STATIC_LIB
>> +endchoice
>
> I don't think we should have Blackfin-specific options. As I suggested
> in the review of PATCH 1/2, we should probably have global
> BR2_BINFMT_<foo> options.
>

OK. I will move these macro to arch/Config.in

>> +config BR2_TARGET_CPU_REVISION
>> +     string "Target CPU revision"
>
> Help text needed.
>

OK.

>> +config BR2_BFIN_INSTALL_ELF_SHARED
>> +     depends on BR2_bfin && !BR2_BFIN_FDPIC
>
> Confused: why in the FDPIC case you would not install the ELF shared
> libraries?
>

Blackfin Linux kernel supports running both FDPIC and FLAT
applications concurrently if the binary format specific libraries
installed properly. This option allow developer to install FDPIC
libraries into a buildroot rootfs image built with binary format macro
other than BR2_BINFMT_FDPIC. It doesn't take effect if
BR2_BINMAT_FDPIC is configured.

>> +     bool "Install ELF shared libraries"
>> +     default y
>
> Help text needed.
>

OK.

>> +config BR2_BFIN_INSTALL_FLAT_SHARED
>> +     depends on BR2_bfin
>> +     bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
>> +     default y
>
> Help text needed.
>
>> +
>>  config BR2_ARCH
>>       default "bfin"
>>
>>  config BR2_ENDIAN
>>          default "LITTLE"
>> +
>> +config BR2_GCC_TARGET_CPU
>> +     default bf606           if BR2_bf606
>> +     default bf607           if BR2_bf607
>> +     default bf608           if BR2_bf608
>> +     default bf609           if BR2_bf609
>> +     default bf512           if BR2_bf512
>> +     default bf514           if BR2_bf514
>> +     default bf516           if BR2_bf516
>> +     default bf518           if BR2_bf518
>> +     default bf522           if BR2_bf522
>> +     default bf523           if BR2_bf523
>> +     default bf524           if BR2_bf524
>> +     default bf525           if BR2_bf525
>> +     default bf526           if BR2_bf526
>> +     default bf527           if BR2_bf527
>> +     default bf531           if BR2_bf531
>> +     default bf532           if BR2_bf532
>> +     default bf533           if BR2_bf533
>> +     default bf534           if BR2_bf534
>> +     default bf536           if BR2_bf536
>> +     default bf537           if BR2_bf537
>> +     default bf538           if BR2_bf538
>> +     default bf539           if BR2_bf539
>> +     default bf542           if BR2_bf542
>> +     default bf544           if BR2_bf544
>> +     default bf547           if BR2_bf547
>> +     default bf548           if BR2_bf548
>> +     default bf549           if BR2_bf549
>> +     default bf561           if BR2_bf561
>> +
>> +config BR2_TARGET_ABI_FLAT
>> +     default n               if BR2_BFIN_FDPIC
>> +     default y
>
> To be refactored with the BR2_BINFMT_<foo> stuff.
>

OK.

>> diff --git a/arch/Makefile.in b/arch/Makefile.in
>> new file mode 100644
>> index 0000000..d791118
>> --- /dev/null
>> +++ b/arch/Makefile.in
>> @@ -0,0 +1,5 @@
>> +# The architecture specific Makefile.in.$ARCH should be included only
>> +# when the arch macro is selected.
>> +ifeq ($(BR2_bfin),y)
>> +include arch/Makefile.in.bfin
>> +endif
>
> Ok.
>
>> diff --git a/arch/Makefile.in.bfin b/arch/Makefile.in.bfin
>> new file mode 100644
>> index 0000000..e16532a
>> --- /dev/null
>> +++ b/arch/Makefile.in.bfin
>> @@ -0,0 +1,50 @@
>> +TARGETS-y =
>
> Isn't that dangerous if someone else is using TARGETS-y ?

Yes, this line can be removed.

>
>> +TARGETS-$(BR2_BFIN_INSTALL_ELF_SHARED) += romfs.shared.libs.elf
>> +TARGETS-$(BR2_BFIN_INSTALL_FLAT_SHARED) += romfs.shared.libs.flat
>
> What's the relation with "romfs" ?
>
> Also, we more or less name all our targets target-<something>, so it
> would be good to follow this convention. Also remember to update
> the TARGET_EXCEPTIONS variable in support/scripts/graph-depends when
> you add more of such custom targets.
>

OK.

>> +TARGETS += $(TARGETS-y)
>> +
>> +ifeq ($(BR2_BFIN_FDPIC),y)
>> +USR_LIB_EXTERNAL_LIBS += libgfortran.so libgomp.so libmudflap.so libmudflapth.so libobjc.so
>> +endif
>
> This is totally unrelated to Blackfin, and therefore has no reason to
> be here and to depend on BR2_BFIN_FDPIC.
>

Where should I put these library names to ?

>> +CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-
>
> No reason to be here. Please use $(TARGET_CC) and $(TARGET_READELF)
> below.

There are 2 individual Blackfin toolchain binaries for FDPIC and FLAT
format. The prefix names are different. And as I explained ahead, the
target is to install libraries of a different binary format with
different toolchain prefix name in current $(TARGET_CC) and
$(TARGET_READELF). So, this macro can not be removed.

>> +romfs.shared.libs.elf:
>> +     set -e; \
>> +     t=`$(CROSS_COMPILE_SHARED_ELF)gcc $(CPUFLAGS) -print-file-name=libc.a`; \
>> +     t=`dirname $$t`/../..; \
>> +     for i in $$t/lib/*so*; do \
>> +             i=`readlink -f "$$i"`; \
>> +             soname=`$(CROSS_COMPILE_SHARED_ELF)readelf -d "$$i" | sed -n '/(SONAME)/s:.*[[]\(.*\)[]].*:\1:p'`; \
>> +             $(INSTALL) -D $$i $(TARGET_DIR)/lib/$$soname; \
>> +     done
>
> Isn't this done already by the external toolchain logic? Some
> documentation/comments on what you're doing here would be good. I'm
> trying to understand if it's really Blackfin-specific, if it's specific
> to the usage of external toolchains or would apply to the internal
> toolchain backend as well, etc.
>

No. Refer to my explanation ahead.

>> +CROSS_COMPILE_SHARED_FLAT ?= bfin-uclinux-
>
> TARGET_CC to be used below.
>

Ditto.

>> +romfs.shared.libs.flat:
>> +     set -e; \
>> +     t=`$(CROSS_COMPILE_SHARED_FLAT)gcc $(CPUFLAGS) -mid-shared-library -print-file-name=libc`; \
>> +     if [ -f $$t -a ! -h $$t ] ; then \
>> +             $(INSTALL) -D $$t $(TARGET_DIR)/lib/lib1.so; \
>> +     fi
>> +
>> +ifeq ($(BR2_TARGET_CPU_REVISION),)
>> +TARGET_CPU = -mcpu=$(BR2_GCC_TARGET_CPU)
>> +else
>> +TARGET_CPU = -mcpu=$(BR2_GCC_TARGET_CPU)-$(BR2_TARGET_CPU_REVISION)
>> +endif
>> +TARGET_CFLAGS += $(call qstrip,$(TARGET_CPU))
>
> This will not work for the external toolchain wrapper logic. The
> external toolchain wrapper logic (in
> toolchain/toolchain-external/ext-tool.mk) takes the value of
> BR2_GCC_TARGET_CPU and encodes it as a -mcpu in the external toolchain
> wrapper. We want this to be part of the external toolchain wrapper
> instead of TARGET_CFLAGS so that people directly using the external
> toolchain from host/usr/bin/ go through the wrapper, and end up having
> the same mcpu/march/mtune options as the Buildroot build.
>

OK


>> +ifneq ($(BR2_USE_MMU), y)
>> +TARGET_CFLAGS += -D__uClinux__
>> +endif
>
> Why do you have this one here in a Blackfin-specific area, and the
> -D__NOMMU__ in PATCH 1/2 in an architecture-generic area?

This is a blackfin specific hack for old applications. Can be removed
in patch for upstream.

>
>> +ifeq ($(BR2_BFIN_FLAT_SEP_DATA),y)
>> +TARGET_LDFLAGS += -msep-data
>> +TARGET_CFLAGS += -msep-data
>> +TARGET_CXXFLAGS += -msep-data
>> +endif
>> +
>> +ifeq ($(BR2_BFIN_SHARED_FLAT), y)
>> +TARGET_LDFLAGS += -mid-shared-library -mshared-library-id=0
>> +TARGET_CFLAGS += -mid-shared-library -mshared-library-id=0
>> +TARGET_CXXFLAGS += -mid-shared-library -mshared-library-id=0
>> +endif
>
> I think those options are used to select which multilib variant to use
> in the toolchain. Therefore, they should be encoded into the external
> toolchain wrapper and not just passed in TARGET_LDFLAGS, TARGET_CFLAGS
> and TARGET_CXXFLAGS.
>

OK.

Regards,

Sonic
Thomas De Schampheleire - March 25, 2013, 11:47 a.m.
Hi Sonic,

On Mon, Mar 25, 2013 at 12:33 PM, Sonic Zhang <sonic.adi@gmail.com> wrote:

> Hi Thomas,
>
> On Fri, Mar 22, 2013 at 10:54 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Dear Sonic Zhang,
> >
> > Please add a commit log explaining all what you're doing. Your changes
> > are quite architecture-specific and touch the core of Buildroot, but
> > there are also not comments and no commit log.
> >
> > Also your changelog should be below the --- and not before, otherwise
> > it ends up in the commit log forever.
> >
> > On Fri, 22 Mar 2013 17:01:42 +0800, Sonic Zhang wrote:
> >
> >> diff --git a/Makefile b/Makefile
> >> index 7f0822f..c2f43a4 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -329,6 +329,8 @@ ifneq ($(PACKAGE_OVERRIDE_FILE),)
> >>  -include $(PACKAGE_OVERRIDE_FILE)
> >>  endif
> >>
> >> +include arch/Makefile.in
> >
> > I must admit I am not entirely happy with having Blackfin-specific
> > Makefiles, but it's true that we already have some
> > architecture-specific crap in package/Makefile.in that we could more
> > cleanly spread in arch/Makefile.in.<arch>.
> >
> >> diff --git a/arch/Config.in.bfin b/arch/Config.in.bfin
> >> index 0b137ae..0750b86 100644
> >> --- a/arch/Config.in.bfin
> >> +++ b/arch/Config.in.bfin
> >> @@ -7,10 +7,127 @@ config BR2_BFIN_FDPIC
> >>  config BR2_BFIN_FLAT
> >>       bool "FLAT"
> >>       select BR2_PREFER_STATIC_LIB
> >> +config BR2_BFIN_FLAT_SEP_DATA
> >> +     bool "FLAT (Separate data)"
> >> +     select BR2_PREFER_STATIC_LIB
> >> +config BR2_BFIN_SHARED_FLAT
> >> +     bool "Shared FLAT"
> >> +     select BR2_PREFER_STATIC_LIB
> >> +endchoice
> >
> > I don't think we should have Blackfin-specific options. As I suggested
> > in the review of PATCH 1/2, we should probably have global
> > BR2_BINFMT_<foo> options.
> >
>
> OK. I will move these macro to arch/Config.in
>
> >> +config BR2_TARGET_CPU_REVISION
> >> +     string "Target CPU revision"
> >
> > Help text needed.
> >
>
> OK.
>
> >> +config BR2_BFIN_INSTALL_ELF_SHARED
> >> +     depends on BR2_bfin && !BR2_BFIN_FDPIC
> >
> > Confused: why in the FDPIC case you would not install the ELF shared
> > libraries?
> >
>
> Blackfin Linux kernel supports running both FDPIC and FLAT
> applications concurrently if the binary format specific libraries
> installed properly. This option allow developer to install FDPIC
> libraries into a buildroot rootfs image built with binary format macro
> other than BR2_BINFMT_FDPIC. It doesn't take effect if
> BR2_BINMAT_FDPIC is configured.
>
> >> +     bool "Install ELF shared libraries"
> >> +     default y
> >
> > Help text needed.
> >
>
> OK.
>
> >> +config BR2_BFIN_INSTALL_FLAT_SHARED
> >> +     depends on BR2_bfin
> >> +     bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
> >> +     default y
> >
> > Help text needed.
> >
> >> +
> >>  config BR2_ARCH
> >>       default "bfin"
> >>
> >>  config BR2_ENDIAN
> >>          default "LITTLE"
> >> +
> >> +config BR2_GCC_TARGET_CPU
> >> +     default bf606           if BR2_bf606
> >> +     default bf607           if BR2_bf607
> >> +     default bf608           if BR2_bf608
> >> +     default bf609           if BR2_bf609
> >> +     default bf512           if BR2_bf512
> >> +     default bf514           if BR2_bf514
> >> +     default bf516           if BR2_bf516
> >> +     default bf518           if BR2_bf518
> >> +     default bf522           if BR2_bf522
> >> +     default bf523           if BR2_bf523
> >> +     default bf524           if BR2_bf524
> >> +     default bf525           if BR2_bf525
> >> +     default bf526           if BR2_bf526
> >> +     default bf527           if BR2_bf527
> >> +     default bf531           if BR2_bf531
> >> +     default bf532           if BR2_bf532
> >> +     default bf533           if BR2_bf533
> >> +     default bf534           if BR2_bf534
> >> +     default bf536           if BR2_bf536
> >> +     default bf537           if BR2_bf537
> >> +     default bf538           if BR2_bf538
> >> +     default bf539           if BR2_bf539
> >> +     default bf542           if BR2_bf542
> >> +     default bf544           if BR2_bf544
> >> +     default bf547           if BR2_bf547
> >> +     default bf548           if BR2_bf548
> >> +     default bf549           if BR2_bf549
> >> +     default bf561           if BR2_bf561
> >> +
> >> +config BR2_TARGET_ABI_FLAT
> >> +     default n               if BR2_BFIN_FDPIC
> >> +     default y
> >
> > To be refactored with the BR2_BINFMT_<foo> stuff.
> >
>
> OK.
>
> >> diff --git a/arch/Makefile.in b/arch/Makefile.in
> >> new file mode 100644
> >> index 0000000..d791118
> >> --- /dev/null
> >> +++ b/arch/Makefile.in
> >> @@ -0,0 +1,5 @@
> >> +# The architecture specific Makefile.in.$ARCH should be included only
> >> +# when the arch macro is selected.
> >> +ifeq ($(BR2_bfin),y)
> >> +include arch/Makefile.in.bfin
> >> +endif
> >
> > Ok.
> >
> >> diff --git a/arch/Makefile.in.bfin b/arch/Makefile.in.bfin
> >> new file mode 100644
> >> index 0000000..e16532a
> >> --- /dev/null
> >> +++ b/arch/Makefile.in.bfin
> >> @@ -0,0 +1,50 @@
> >> +TARGETS-y =
> >
> > Isn't that dangerous if someone else is using TARGETS-y ?
>
> Yes, this line can be removed.
>
> >
> >> +TARGETS-$(BR2_BFIN_INSTALL_ELF_SHARED) += romfs.shared.libs.elf
> >> +TARGETS-$(BR2_BFIN_INSTALL_FLAT_SHARED) += romfs.shared.libs.flat
> >
> > What's the relation with "romfs" ?
> >
> > Also, we more or less name all our targets target-<something>, so it
> > would be good to follow this convention. Also remember to update
> > the TARGET_EXCEPTIONS variable in support/scripts/graph-depends when
> > you add more of such custom targets.
> >
>
> OK.
>
> >> +TARGETS += $(TARGETS-y)
> >> +
> >> +ifeq ($(BR2_BFIN_FDPIC),y)
> >> +USR_LIB_EXTERNAL_LIBS += libgfortran.so libgomp.so libmudflap.so
> libmudflapth.so libobjc.so
> >> +endif
> >
> > This is totally unrelated to Blackfin, and therefore has no reason to
> > be here and to depend on BR2_BFIN_FDPIC.
> >
>
> Where should I put these library names to ?
>
> >> +CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-
> >
> > No reason to be here. Please use $(TARGET_CC) and $(TARGET_READELF)
> > below.
>
> There are 2 individual Blackfin toolchain binaries for FDPIC and FLAT
> format. The prefix names are different. And as I explained ahead, the
> target is to install libraries of a different binary format with
> different toolchain prefix name in current $(TARGET_CC) and
> $(TARGET_READELF). So, this macro can not be removed.
>

Currently all compilations are using TARGET_CC and such, which expands to
$(TARGET_CROSS)gcc.
TARGET_CROSS in turn is derived as:

ifeq ($(BR2_TOOLCHAIN_BUILDROOT)$(BR2_TOOLCHAIN_CTNG),y)
TARGET_CROSS=$(HOST_DIR)/usr/bin/$(GNU_TARGET_NAME)-
else
TARGET_CROSS=$(HOST_DIR)/usr/bin/$(call
qstrip,$(BR2_TOOLCHAIN_EXTERNAL_PREFIX))-
endif


If you need to have different prefixes, isn't it possible to hook into one
of these variables, instead of creating a new variable
CROSS_COMPILE_SHARED_ELF/FLAT ?
Depending on the elf/flat choice, you would then for example set
BR2_TOOLCHAIN_EXTERNAL_PREFIX (possible from kconfig directly) (but this
would work only for external toolchains), or override TARGET_CROSS.
With such a change, you can reuse TARGET_CC and TARGET_READELF.

Best regards,
Thomas
Arnout Vandecappelle - March 26, 2013, 7:25 a.m.
On 22/03/13 10:01, Sonic Zhang wrote:
> From: Sonic Zhang <sonic.zhang@analog.com>
[snip]

  Hi Sonic,

  Looks like you again choose a difficult patch set to try to upstream...

  After reading all the comments in this thread, I propose to split up 
the patch set as follows in order to get some progress on it.

1. Add blackfin CPU choice.
This patch adds a Target CPU configuration option and uses it to select a 
-m option for gcc.

2. Introduce target CPU revision.
Adds the possibility (in the general infrastructure, not 
blackfin-specific) to have a free-form CPU revision string and append it 
to the target CPU. Only Blackfin actually uses this option.

3. Introduce BINFMT_FLAT.
Just introduce the symbol and add options in blackfin to select it.

4. Introduce package-specific BINFMT_FLAT options.
I.e. <PKG>_FLAT_STACKSIZE

5. Introduce blackfin-specific Makefile
Probably needs more work.

6. Introduce NOMMU symbol
Probably needs more work.



  Regards,
  Arnout
Thomas Petazzoni - March 26, 2013, 8:15 a.m.
Dear Arnout Vandecappelle,

On Tue, 26 Mar 2013 08:25:11 +0100, Arnout Vandecappelle wrote:

>   Looks like you again choose a difficult patch set to try to upstream...
> 
>   After reading all the comments in this thread, I propose to split up 
> the patch set as follows in order to get some progress on it.
> 
> 1. Add blackfin CPU choice.
> This patch adds a Target CPU configuration option and uses it to select a 
> -m option for gcc.
> 
> 2. Introduce target CPU revision.
> Adds the possibility (in the general infrastructure, not 
> blackfin-specific) to have a free-form CPU revision string and append it 
> to the target CPU. Only Blackfin actually uses this option.
> 
> 3. Introduce BINFMT_FLAT.
> Just introduce the symbol and add options in blackfin to select it.

I don't think we should introduce just BINFMT_FLAT, but also
BINFMT_FDPIC (we already have a FDPIC symbol) and BINFMT_ELF, for
completeness. So I would rephrase this item into "Introduce BINFMT_*
symbols and refactor the existing usage to use BINFMT_FDPIC/BINFMT_ELF".

> 4. Introduce package-specific BINFMT_FLAT options.
> I.e. <PKG>_FLAT_STACKSIZE
> 
> 5. Introduce blackfin-specific Makefile
> Probably needs more work.
> 
> 6. Introduce NOMMU symbol
> Probably needs more work.

I fully agree with Arnout's proposal here. Let's do it step by step, so
that we discuss one issue after the other, and gets things merged
progressively.

Sonic, would you mind reworking your patch set to split it as Arnout is
suggesting?

Best regards,

Thomas
Sonic Zhang - March 26, 2013, 8:16 a.m.
Hi Thomas,

On Mon, Mar 25, 2013 at 7:47 PM, Thomas De Schampheleire
<patrickdepinguin+buildroot@gmail.com> wrote:
> Hi Sonic,
>
> On Mon, Mar 25, 2013 at 12:33 PM, Sonic Zhang <sonic.adi@gmail.com> wrote:
>>
>> Hi Thomas,
>>
>> On Fri, Mar 22, 2013 at 10:54 PM, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > Dear Sonic Zhang,
>> >
>> > Please add a commit log explaining all what you're doing. Your changes
>> > are quite architecture-specific and touch the core of Buildroot, but
>> > there are also not comments and no commit log.
>> >
>> > Also your changelog should be below the --- and not before, otherwise
>> > it ends up in the commit log forever.
>> >
>> > On Fri, 22 Mar 2013 17:01:42 +0800, Sonic Zhang wrote:
>> >
>> >> diff --git a/Makefile b/Makefile
>> >> index 7f0822f..c2f43a4 100644
>> >> --- a/Makefile
>> >> +++ b/Makefile
>> >> @@ -329,6 +329,8 @@ ifneq ($(PACKAGE_OVERRIDE_FILE),)
>> >>  -include $(PACKAGE_OVERRIDE_FILE)
>> >>  endif
>> >>
>> >> +include arch/Makefile.in
>> >
>> > I must admit I am not entirely happy with having Blackfin-specific
>> > Makefiles, but it's true that we already have some
>> > architecture-specific crap in package/Makefile.in that we could more
>> > cleanly spread in arch/Makefile.in.<arch>.
>> >
>> >> diff --git a/arch/Config.in.bfin b/arch/Config.in.bfin
>> >> index 0b137ae..0750b86 100644
>> >> --- a/arch/Config.in.bfin
>> >> +++ b/arch/Config.in.bfin
>> >> @@ -7,10 +7,127 @@ config BR2_BFIN_FDPIC
>> >>  config BR2_BFIN_FLAT
>> >>       bool "FLAT"
>> >>       select BR2_PREFER_STATIC_LIB
>> >> +config BR2_BFIN_FLAT_SEP_DATA
>> >> +     bool "FLAT (Separate data)"
>> >> +     select BR2_PREFER_STATIC_LIB
>> >> +config BR2_BFIN_SHARED_FLAT
>> >> +     bool "Shared FLAT"
>> >> +     select BR2_PREFER_STATIC_LIB
>> >> +endchoice
>> >
>> > I don't think we should have Blackfin-specific options. As I suggested
>> > in the review of PATCH 1/2, we should probably have global
>> > BR2_BINFMT_<foo> options.
>> >
>>
>> OK. I will move these macro to arch/Config.in
>>
>> >> +config BR2_TARGET_CPU_REVISION
>> >> +     string "Target CPU revision"
>> >
>> > Help text needed.
>> >
>>
>> OK.
>>
>> >> +config BR2_BFIN_INSTALL_ELF_SHARED
>> >> +     depends on BR2_bfin && !BR2_BFIN_FDPIC
>> >
>> > Confused: why in the FDPIC case you would not install the ELF shared
>> > libraries?
>> >
>>
>> Blackfin Linux kernel supports running both FDPIC and FLAT
>> applications concurrently if the binary format specific libraries
>> installed properly. This option allow developer to install FDPIC
>> libraries into a buildroot rootfs image built with binary format macro
>> other than BR2_BINFMT_FDPIC. It doesn't take effect if
>> BR2_BINMAT_FDPIC is configured.
>>
>> >> +     bool "Install ELF shared libraries"
>> >> +     default y
>> >
>> > Help text needed.
>> >
>>
>> OK.
>>
>> >> +config BR2_BFIN_INSTALL_FLAT_SHARED
>> >> +     depends on BR2_bfin
>> >> +     bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
>> >> +     default y
>> >
>> > Help text needed.
>> >
>> >> +
>> >>  config BR2_ARCH
>> >>       default "bfin"
>> >>
>> >>  config BR2_ENDIAN
>> >>          default "LITTLE"
>> >> +
>> >> +config BR2_GCC_TARGET_CPU
>> >> +     default bf606           if BR2_bf606
>> >> +     default bf607           if BR2_bf607
>> >> +     default bf608           if BR2_bf608
>> >> +     default bf609           if BR2_bf609
>> >> +     default bf512           if BR2_bf512
>> >> +     default bf514           if BR2_bf514
>> >> +     default bf516           if BR2_bf516
>> >> +     default bf518           if BR2_bf518
>> >> +     default bf522           if BR2_bf522
>> >> +     default bf523           if BR2_bf523
>> >> +     default bf524           if BR2_bf524
>> >> +     default bf525           if BR2_bf525
>> >> +     default bf526           if BR2_bf526
>> >> +     default bf527           if BR2_bf527
>> >> +     default bf531           if BR2_bf531
>> >> +     default bf532           if BR2_bf532
>> >> +     default bf533           if BR2_bf533
>> >> +     default bf534           if BR2_bf534
>> >> +     default bf536           if BR2_bf536
>> >> +     default bf537           if BR2_bf537
>> >> +     default bf538           if BR2_bf538
>> >> +     default bf539           if BR2_bf539
>> >> +     default bf542           if BR2_bf542
>> >> +     default bf544           if BR2_bf544
>> >> +     default bf547           if BR2_bf547
>> >> +     default bf548           if BR2_bf548
>> >> +     default bf549           if BR2_bf549
>> >> +     default bf561           if BR2_bf561
>> >> +
>> >> +config BR2_TARGET_ABI_FLAT
>> >> +     default n               if BR2_BFIN_FDPIC
>> >> +     default y
>> >
>> > To be refactored with the BR2_BINFMT_<foo> stuff.
>> >
>>
>> OK.
>>
>> >> diff --git a/arch/Makefile.in b/arch/Makefile.in
>> >> new file mode 100644
>> >> index 0000000..d791118
>> >> --- /dev/null
>> >> +++ b/arch/Makefile.in
>> >> @@ -0,0 +1,5 @@
>> >> +# The architecture specific Makefile.in.$ARCH should be included only
>> >> +# when the arch macro is selected.
>> >> +ifeq ($(BR2_bfin),y)
>> >> +include arch/Makefile.in.bfin
>> >> +endif
>> >
>> > Ok.
>> >
>> >> diff --git a/arch/Makefile.in.bfin b/arch/Makefile.in.bfin
>> >> new file mode 100644
>> >> index 0000000..e16532a
>> >> --- /dev/null
>> >> +++ b/arch/Makefile.in.bfin
>> >> @@ -0,0 +1,50 @@
>> >> +TARGETS-y =
>> >
>> > Isn't that dangerous if someone else is using TARGETS-y ?
>>
>> Yes, this line can be removed.
>>
>> >
>> >> +TARGETS-$(BR2_BFIN_INSTALL_ELF_SHARED) += romfs.shared.libs.elf
>> >> +TARGETS-$(BR2_BFIN_INSTALL_FLAT_SHARED) += romfs.shared.libs.flat
>> >
>> > What's the relation with "romfs" ?
>> >
>> > Also, we more or less name all our targets target-<something>, so it
>> > would be good to follow this convention. Also remember to update
>> > the TARGET_EXCEPTIONS variable in support/scripts/graph-depends when
>> > you add more of such custom targets.
>> >
>>
>> OK.
>>
>> >> +TARGETS += $(TARGETS-y)
>> >> +
>> >> +ifeq ($(BR2_BFIN_FDPIC),y)
>> >> +USR_LIB_EXTERNAL_LIBS += libgfortran.so libgomp.so libmudflap.so
>> >> libmudflapth.so libobjc.so
>> >> +endif
>> >
>> > This is totally unrelated to Blackfin, and therefore has no reason to
>> > be here and to depend on BR2_BFIN_FDPIC.
>> >
>>
>> Where should I put these library names to ?
>>
>> >> +CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-
>> >
>> > No reason to be here. Please use $(TARGET_CC) and $(TARGET_READELF)
>> > below.
>>
>> There are 2 individual Blackfin toolchain binaries for FDPIC and FLAT
>> format. The prefix names are different. And as I explained ahead, the
>> target is to install libraries of a different binary format with
>> different toolchain prefix name in current $(TARGET_CC) and
>> $(TARGET_READELF). So, this macro can not be removed.
>
>
> Currently all compilations are using TARGET_CC and such, which expands to
> $(TARGET_CROSS)gcc.
> TARGET_CROSS in turn is derived as:
>
> ifeq ($(BR2_TOOLCHAIN_BUILDROOT)$(BR2_TOOLCHAIN_CTNG),y)
> TARGET_CROSS=$(HOST_DIR)/usr/bin/$(GNU_TARGET_NAME)-
> else
> TARGET_CROSS=$(HOST_DIR)/usr/bin/$(call
> qstrip,$(BR2_TOOLCHAIN_EXTERNAL_PREFIX))-
> endif
>
>
> If you need to have different prefixes, isn't it possible to hook into one
> of these variables, instead of creating a new variable
> CROSS_COMPILE_SHARED_ELF/FLAT ?
> Depending on the elf/flat choice, you would then for example set
> BR2_TOOLCHAIN_EXTERNAL_PREFIX (possible from kconfig directly) (but this
> would work only for external toolchains), or override TARGET_CROSS.
> With such a change, you can reuse TARGET_CC and TARGET_READELF.

I do set BR2_TOOLCHAIN_EXTERNAL_PREFIX to bfin-uclinux via kconfig to
build the complete packages in buildroot. Meanwhile, I would like to
install FDPIC libraries as well from the bfin-linux-uclibc toolchain
folder, which is different from the bfin-uclinux I set to
BR2_TOOLCHAIN_EXTERNAL_PREFIX.

Blackfin toolchain bfin-linux-uclibc and bfin-uclinux are located in
different path.

Regards,

Sonic
Thomas Petazzoni - March 26, 2013, 8:41 a.m.
Dear Sonic Zhang,

On Tue, 26 Mar 2013 16:16:07 +0800, Sonic Zhang wrote:

> > If you need to have different prefixes, isn't it possible to hook into one
> > of these variables, instead of creating a new variable
> > CROSS_COMPILE_SHARED_ELF/FLAT ?
> > Depending on the elf/flat choice, you would then for example set
> > BR2_TOOLCHAIN_EXTERNAL_PREFIX (possible from kconfig directly) (but this
> > would work only for external toolchains), or override TARGET_CROSS.
> > With such a change, you can reuse TARGET_CC and TARGET_READELF.
> 
> I do set BR2_TOOLCHAIN_EXTERNAL_PREFIX to bfin-uclinux via kconfig to
> build the complete packages in buildroot. Meanwhile, I would like to
> install FDPIC libraries as well from the bfin-linux-uclibc toolchain
> folder, which is different from the bfin-uclinux I set to
> BR2_TOOLCHAIN_EXTERNAL_PREFIX.
> 
> Blackfin toolchain bfin-linux-uclibc and bfin-uclinux are located in
> different path.

But the current external toolchain logic in
toolchain/toolchain-external/ext-tool.mk only extracts either the
bfin-uclinux sysroot *or* the bfin-linux-uclibc sysroot, so you in
practice don't have both at the same time, at least with the current
Buildroot. So presumably this would also need to be changed, right?

Thanks,

Thomas
Sonic Zhang - March 26, 2013, 9:36 a.m.
Ho Thomas,

On Tue, Mar 26, 2013 at 4:41 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Sonic Zhang,
>
> On Tue, 26 Mar 2013 16:16:07 +0800, Sonic Zhang wrote:
>
>> > If you need to have different prefixes, isn't it possible to hook into one
>> > of these variables, instead of creating a new variable
>> > CROSS_COMPILE_SHARED_ELF/FLAT ?
>> > Depending on the elf/flat choice, you would then for example set
>> > BR2_TOOLCHAIN_EXTERNAL_PREFIX (possible from kconfig directly) (but this
>> > would work only for external toolchains), or override TARGET_CROSS.
>> > With such a change, you can reuse TARGET_CC and TARGET_READELF.
>>
>> I do set BR2_TOOLCHAIN_EXTERNAL_PREFIX to bfin-uclinux via kconfig to
>> build the complete packages in buildroot. Meanwhile, I would like to
>> install FDPIC libraries as well from the bfin-linux-uclibc toolchain
>> folder, which is different from the bfin-uclinux I set to
>> BR2_TOOLCHAIN_EXTERNAL_PREFIX.
>>
>> Blackfin toolchain bfin-linux-uclibc and bfin-uclinux are located in
>> different path.
>
> But the current external toolchain logic in
> toolchain/toolchain-external/ext-tool.mk only extracts either the
> bfin-uclinux sysroot *or* the bfin-linux-uclibc sysroot, so you in
> practice don't have both at the same time, at least with the current
> Buildroot. So presumably this would also need to be changed, right?

Yes, that's why the installation target copy library files from the
original toolchain folder other than the sysroot.

Regards,

Sonic
Thomas Petazzoni - March 26, 2013, 10:08 a.m.
Dear Sonic Zhang,

On Tue, 26 Mar 2013 17:36:24 +0800, Sonic Zhang wrote:

> > But the current external toolchain logic in
> > toolchain/toolchain-external/ext-tool.mk only extracts either the
> > bfin-uclinux sysroot *or* the bfin-linux-uclibc sysroot, so you in
> > practice don't have both at the same time, at least with the current
> > Buildroot. So presumably this would also need to be changed, right?
> 
> Yes, that's why the installation target copy library files from the
> original toolchain folder other than the sysroot.

That doesn't work: when bfin-linux-uclibc is used, then the
bfin-uclinux toolchain is *NOT* installed. Your current code makes the
assumption that this toolchain is installed system-wide, but that's not
what we want in a Buildroot context.

But ok, it's not an impossible problem: we can tune the external
toolchain logic so that it installs both sysroot when the user wants
both the FDPIC and FLAT stuff.

Best regards,

Thomas
Sonic Zhang - March 28, 2013, 8:20 a.m.
Hi Thomas and Arnout,

On Tue, Mar 26, 2013 at 4:15 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Arnout Vandecappelle,
>
> On Tue, 26 Mar 2013 08:25:11 +0100, Arnout Vandecappelle wrote:
>
>>   Looks like you again choose a difficult patch set to try to upstream...
>>
>>   After reading all the comments in this thread, I propose to split up
>> the patch set as follows in order to get some progress on it.
>>
>> 1. Add blackfin CPU choice.
>> This patch adds a Target CPU configuration option and uses it to select a
>> -m option for gcc.
>>
>> 2. Introduce target CPU revision.
>> Adds the possibility (in the general infrastructure, not
>> blackfin-specific) to have a free-form CPU revision string and append it
>> to the target CPU. Only Blackfin actually uses this option.
>>
>> 3. Introduce BINFMT_FLAT.
>> Just introduce the symbol and add options in blackfin to select it.
>
> I don't think we should introduce just BINFMT_FLAT, but also
> BINFMT_FDPIC (we already have a FDPIC symbol) and BINFMT_ELF, for
> completeness. So I would rephrase this item into "Introduce BINFMT_*
> symbols and refactor the existing usage to use BINFMT_FDPIC/BINFMT_ELF".
>
>> 4. Introduce package-specific BINFMT_FLAT options.
>> I.e. <PKG>_FLAT_STACKSIZE
>>
>> 5. Introduce blackfin-specific Makefile
>> Probably needs more work.
>>
>> 6. Introduce NOMMU symbol
>> Probably needs more work.
>
> I fully agree with Arnout's proposal here. Let's do it step by step, so
> that we discuss one issue after the other, and gets things merged
> progressively.
>
> Sonic, would you mind reworking your patch set to split it as Arnout is
> suggesting?
>

Could you comment on the reworked patch set?

Regards,

Sonic
Thomas Petazzoni - March 28, 2013, 8:56 a.m.
Dear Sonic Zhang,

On Thu, 28 Mar 2013 16:20:14 +0800, Sonic Zhang wrote:

> > Sonic, would you mind reworking your patch set to split it as Arnout is
> > suggesting?
> 
> Could you comment on the reworked patch set?

Yes, sure. I've already looked at it. It looks a lot better, but I (of
course) still have some comments. Will try to send some review soon,
but I'll be on vacation until next Wednesday, with very limited time to
do any sort of coding/reviewing.

Thanks!

Thomas
Sonic Zhang - March 29, 2013, 9:50 a.m.
Hi Thomas,

On Thu, Mar 28, 2013 at 4:56 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Sonic Zhang,
>
> On Thu, 28 Mar 2013 16:20:14 +0800, Sonic Zhang wrote:
>
>> > Sonic, would you mind reworking your patch set to split it as Arnout is
>> > suggesting?
>>
>> Could you comment on the reworked patch set?
>
> Yes, sure. I've already looked at it. It looks a lot better, but I (of
> course) still have some comments. Will try to send some review soon,
> but I'll be on vacation until next Wednesday, with very limited time to
> do any sort of coding/reviewing.
>

I just sent out the v2 patch set to fix some typo error.
Please comment on the v2 after you are back.

Regards,

Sonic

Patch

diff --git a/Makefile b/Makefile
index 7f0822f..c2f43a4 100644
--- a/Makefile
+++ b/Makefile
@@ -329,6 +329,8 @@  ifneq ($(PACKAGE_OVERRIDE_FILE),)
 -include $(PACKAGE_OVERRIDE_FILE)
 endif
 
+include arch/Makefile.in
+
 include package/*/*.mk
 
 include boot/common.mk
diff --git a/arch/Config.in.bfin b/arch/Config.in.bfin
index 0b137ae..0750b86 100644
--- a/arch/Config.in.bfin
+++ b/arch/Config.in.bfin
@@ -7,10 +7,127 @@  config BR2_BFIN_FDPIC
 config BR2_BFIN_FLAT
 	bool "FLAT"
 	select BR2_PREFER_STATIC_LIB
+config BR2_BFIN_FLAT_SEP_DATA
+	bool "FLAT (Separate data)"
+	select BR2_PREFER_STATIC_LIB
+config BR2_BFIN_SHARED_FLAT
+	bool "Shared FLAT"
+	select BR2_PREFER_STATIC_LIB
+endchoice
+
+choice
+	prompt "Target CPU"
+	depends on BR2_bfin
+	default BR2_bf609
+	help
+	  Specify target CPU
+config BR2_bf606
+	bool "bf606"
+config BR2_bf607
+	bool "bf607"
+config BR2_bf608
+	bool "bf608"
+config BR2_bf609
+	bool "bf609"
+config BR2_bf512
+	bool "bf512"
+config BR2_bf514
+	bool "bf514"
+config BR2_bf516
+	bool "bf516"
+config BR2_bf518
+	bool "bf518"
+config BR2_bf522
+	bool "bf522"
+config BR2_bf523
+	bool "bf523"
+config BR2_bf524
+	bool "bf524"
+config BR2_bf525
+	bool "bf525"
+config BR2_bf526
+	bool "bf526"
+config BR2_bf527
+	bool "bf527"
+config BR2_bf531
+	bool "bf531"
+config BR2_bf532
+	bool "bf532"
+config BR2_bf533
+	bool "bf533"
+config BR2_bf534
+	bool "bf534"
+config BR2_bf536
+	bool "bf536"
+config BR2_bf537
+	bool "bf537"
+config BR2_bf538
+	bool "bf538"
+config BR2_bf539
+	bool "bf539"
+config BR2_bf542
+	bool "bf542"
+config BR2_bf544
+	bool "bf544"
+config BR2_bf547
+	bool "bf547"
+config BR2_bf548
+	bool "bf548"
+config BR2_bf549
+	bool "bf549"
+config BR2_bf561
+	bool "bf561"
 endchoice
 
+config BR2_TARGET_CPU_REVISION
+	string "Target CPU revision"
+
+config BR2_BFIN_INSTALL_ELF_SHARED
+	depends on BR2_bfin && !BR2_BFIN_FDPIC
+	bool "Install ELF shared libraries"
+	default y
+
+config BR2_BFIN_INSTALL_FLAT_SHARED
+	depends on BR2_bfin
+	bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
+	default y
+
 config BR2_ARCH
 	default "bfin"
 
 config BR2_ENDIAN
         default "LITTLE"
+
+config BR2_GCC_TARGET_CPU
+	default bf606		if BR2_bf606
+	default bf607		if BR2_bf607
+	default bf608		if BR2_bf608
+	default bf609		if BR2_bf609
+	default bf512		if BR2_bf512
+	default bf514		if BR2_bf514
+	default bf516		if BR2_bf516
+	default bf518		if BR2_bf518
+	default bf522		if BR2_bf522
+	default bf523		if BR2_bf523
+	default bf524		if BR2_bf524
+	default bf525		if BR2_bf525
+	default bf526		if BR2_bf526
+	default bf527		if BR2_bf527
+	default bf531		if BR2_bf531
+	default bf532		if BR2_bf532
+	default bf533		if BR2_bf533
+	default bf534		if BR2_bf534
+	default bf536		if BR2_bf536
+	default bf537		if BR2_bf537
+	default bf538		if BR2_bf538
+	default bf539		if BR2_bf539
+	default bf542		if BR2_bf542
+	default bf544		if BR2_bf544
+	default bf547		if BR2_bf547
+	default bf548		if BR2_bf548
+	default bf549		if BR2_bf549
+	default bf561		if BR2_bf561
+
+config BR2_TARGET_ABI_FLAT
+	default n		if BR2_BFIN_FDPIC
+	default y
diff --git a/arch/Makefile.in b/arch/Makefile.in
new file mode 100644
index 0000000..d791118
--- /dev/null
+++ b/arch/Makefile.in
@@ -0,0 +1,5 @@ 
+# The architecture specific Makefile.in.$ARCH should be included only
+# when the arch macro is selected.
+ifeq ($(BR2_bfin),y)
+include arch/Makefile.in.bfin
+endif
diff --git a/arch/Makefile.in.bfin b/arch/Makefile.in.bfin
new file mode 100644
index 0000000..e16532a
--- /dev/null
+++ b/arch/Makefile.in.bfin
@@ -0,0 +1,50 @@ 
+TARGETS-y =
+TARGETS-$(BR2_BFIN_INSTALL_ELF_SHARED) += romfs.shared.libs.elf
+TARGETS-$(BR2_BFIN_INSTALL_FLAT_SHARED) += romfs.shared.libs.flat
+TARGETS += $(TARGETS-y)
+
+ifeq ($(BR2_BFIN_FDPIC),y)
+USR_LIB_EXTERNAL_LIBS += libgfortran.so libgomp.so libmudflap.so libmudflapth.so libobjc.so
+endif
+
+CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-
+romfs.shared.libs.elf:
+	set -e; \
+	t=`$(CROSS_COMPILE_SHARED_ELF)gcc $(CPUFLAGS) -print-file-name=libc.a`; \
+	t=`dirname $$t`/../..; \
+	for i in $$t/lib/*so*; do \
+		i=`readlink -f "$$i"`; \
+		soname=`$(CROSS_COMPILE_SHARED_ELF)readelf -d "$$i" | sed -n '/(SONAME)/s:.*[[]\(.*\)[]].*:\1:p'`; \
+		$(INSTALL) -D $$i $(TARGET_DIR)/lib/$$soname; \
+	done
+
+CROSS_COMPILE_SHARED_FLAT ?= bfin-uclinux-
+romfs.shared.libs.flat:
+	set -e; \
+	t=`$(CROSS_COMPILE_SHARED_FLAT)gcc $(CPUFLAGS) -mid-shared-library -print-file-name=libc`; \
+	if [ -f $$t -a ! -h $$t ] ; then \
+		$(INSTALL) -D $$t $(TARGET_DIR)/lib/lib1.so; \
+	fi
+
+ifeq ($(BR2_TARGET_CPU_REVISION),)
+TARGET_CPU = -mcpu=$(BR2_GCC_TARGET_CPU)
+else
+TARGET_CPU = -mcpu=$(BR2_GCC_TARGET_CPU)-$(BR2_TARGET_CPU_REVISION)
+endif
+TARGET_CFLAGS += $(call qstrip,$(TARGET_CPU))
+
+ifneq ($(BR2_USE_MMU), y)
+TARGET_CFLAGS += -D__uClinux__
+endif
+
+ifeq ($(BR2_BFIN_FLAT_SEP_DATA),y)
+TARGET_LDFLAGS += -msep-data
+TARGET_CFLAGS += -msep-data
+TARGET_CXXFLAGS += -msep-data
+endif
+
+ifeq ($(BR2_BFIN_SHARED_FLAT), y)
+TARGET_LDFLAGS += -mid-shared-library -mshared-library-id=0
+TARGET_CFLAGS += -mid-shared-library -mshared-library-id=0
+TARGET_CXXFLAGS += -mid-shared-library -mshared-library-id=0
+endif