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

[Tom Rini <trini@konsulko.com>]

On Fri, Nov 06, 2015 at 03:58:17PM -0800, Simon Glass wrote:
> Hi,
> 
> On 6 November 2015 at 10:23, Nishanth Menon <nm@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@ti.com>:
> >> >> On Thu, Nov 5, 2015 at 6:15 PM, Simon Glass <sjg@chromium.org> wrote:
> >> >>> Hi,
> >> >>>
> >> >>> On 5 November 2015 at 00:32, Nishanth Menon <nm@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@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.

Conceptually fine with me.  Do we really need a config option for it
tho?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151107/aa952467/attachment.sig>