[U-Boot] [PATCH] Makefile: Include vendor common library in include search path

[Simon Glass]

Hi,

On 6 November 2015 at 10:23, Nishanth Menon <nm at ti.com> wrote:
> On 11:10-20151106, Nishanth Menon wrote:
>> On 11/05/2015 10:50 PM, Masahiro Yamada wrote:
>> > 2015-11-06 12:30 GMT+09:00 Nishanth Menon <nm at ti.com>:
>> >> On Thu, Nov 5, 2015 at 6:15 PM, Simon Glass <sjg at chromium.org> wrote:
>> >>> Hi,
>> >>>
>> >>> On 5 November 2015 at 00:32, Nishanth Menon <nm at ti.com> wrote:
>> >>>> On 11/05/2015 01:28 AM, Nishanth Menon wrote:
>> >>>>> When the vendor common libraries exists, then board should be able to
>> >>>>> reference headers located there, rather than having to do weird logic
>> >>>>> such as '#include "../common/xyz.h"'.
>> >>>>>
>> >>>>> Signed-off-by: Nishanth Menon <nm at ti.com>
>> >>>>> ---
>> >>>>>
>> >>>>>  Makefile                | 1 +
>> >>>>>  board/ti/am57xx/board.c | 2 +-
>> >>>>>  2 files changed, 2 insertions(+), 1 deletion(-)
>> >>>>
>> >>>> Arrgh.. Apologies on the diffstat messup, but anyways, the patch does
>> >>>> apply, and will wait to repost in case of further comments.
>> >>>>
>> >>>>>
>> >>>>> diff --git a/Makefile b/Makefile
>> >>>>> index 3c21f8ddf9e9..75d5ea802dfd 100644
>> >>>>> --- a/Makefile
>> >>>>> +++ b/Makefile
>> >>>>> @@ -620,6 +620,7 @@ c_flags := $(KBUILD_CFLAGS) $(cpp_flags)
>> >>>>>  # U-Boot objects....order is important (i.e. start must be first)
>> >>>>>
>> >>>>>  HAVE_VENDOR_COMMON_LIB = $(if $(wildcard $(srctree)/board/$(VENDOR)/common/Makefile),y,n)
>> >>>>> +UBOOTINCLUDE += $(if $(HAVE_VENDOR_COMMON_LIB:y=1), -I$(srctree)/board/$(VENDOR)/common)
>> >>>>>
>> >>>>>  libs-y += lib/
>> >>>>>  libs-$(HAVE_VENDOR_COMMON_LIB) += board/$(VENDOR)/common/
>> >>>>>
>> >>>>
>> >>>
>> >>> I suppose this is OK. But it might be hard to figure out where a
>> >>> header file is coming from. I wonder if we could make it support:
>> >>>
>> >>> #include <board-common/...>
>> >>>
>> >>> and it would find the file?
>> >>>
>> >>
>> >> Hmmm... Are folks ok if I create a softlink?
>> >
>> >
>> > Or,  you can maybe add the following into board/ti/am57xx/Makefile.
>> >
>> > subdir-ccflags-y := -I$(src)/../common
>> >
>> > then, you can use #include <xyz.h> from your board/ti/am57xx/board.c
>>
>> I could, OR i could even do ../common/xyz.h (which is what current
>> board files do)
>>
>> >
>> >
>> >
>> > But, I personally wouldn't do this in this case
>> > for the reason Simon mentioned.
>> >
>>
>> I agree as well. but the files are built without explicit board level
>> Makefile rules, but the root Makefile(which makes this happen) does
>> not ensure the headers are made available - that sounds contradictory.
>>
>> >
>> > How many vender-common do we have
>> > and is it worth supporting  #include <board-common/...>?
>>
>> Surprisingly larger set that what I expected:
>> $ git branch -v|grep "^*"
>> * master c3c016cf7536 sf: Add SPI NOR protection mechanism
>> $ find board/ -iname "common"|wc -l
>> 19
>> $ git grep "#include \"../common" board/|wc -l
>> 141
>>
> Illustration Purposes only: example of how a
> #include <board-common/xyz.h" could be done - is this what folks want me
> to do?
> diff --git a/Kconfig b/Kconfig
> index 821b464db207..6e825be0545e 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -224,6 +224,8 @@ endmenu             # Boot images
>
>  source "common/Kconfig"
>
> +source "board/Kconfig"
> +
>  source "dts/Kconfig"
>
>  source "net/Kconfig"
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 0d756cbc55a2..62c9b90eac79 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -356,6 +356,7 @@ config TARGET_AM335X_EVM
>         select DM
>         select DM_SERIAL
>         select DM_GPIO
> +       select CREATE_BOARD_SYMLINK
>
>  config TARGET_AM335X_SL50
>         bool "Support am335x_sl50"
> diff --git a/scripts/Makefile.autoconf b/scripts/Makefile.autoconf
> index d668982cc1dd..3f9b969c2271 100644
> --- a/scripts/Makefile.autoconf
> +++ b/scripts/Makefile.autoconf
> @@ -125,6 +125,23 @@ else
>         ln -fsn $$dest arch/$(ARCH)/include/asm/arch
>  endif
>  endif
> +ifdef CONFIG_CREATE_BOARD_SYMLINK
> +ifneq ($(KBUILD_SRC),)
> +       if [ -d $(KBUILD_SRC)/board/$(VENDOR)/common/include ]; then \
> +               dest=board/$(VENDOR)/common/include;                    \
> +       else                                                            \
> +               dest=board/$(VENDOR)/common;                            \
> +       fi;                                                             \
> +       ln -fsn $(KBUILD_SRC)/$$dest include/board-common
> +else
> +       if [ -d board/$(VENDOR)/common/include ]; then \
> +               dest=board/$(VENDOR)/common/include;                    \
> +       else                                                            \
> +               dest=board/$(VENDOR)/common;                            \
> +       fi;                                                             \
> +       ln -fsn ../$$dest include/board-common
> +endif
> +endif
>
>  PHONY += FORCE
>  FORCE:
> --

It looks reasonable to me. Tom and Masahiro may have opinions on this too.

Regards,
Simon