[LinuxBIOS] [PATCH] [v3] create map files, clean up

[Stefan Reinauer]

see patch

[Uwe Hermann]

On Sun, Jul 22, 2007 at 05:47:41PM +0200, Stefan Reinauer wrote:
> move post_code() to a seperate file
> statically link linuxbios.initram for now (depends on printk, post_code
> and die)
> greatly simplify all makefiles by creating a global Rules.make
> use $(dir $@) in Makefiles instead of absolute paths for mkdir..
> clean up Makefiles by calling components' code locally instead of in the
> Makefile. (Remember: one day, no code per mainboard)
> unconditionally create .xcompile in case it changed
> add NM to xcompile
> create $(obj)/linuxbios.map with all symbols of all stages
> +# These are possibly not permanent
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o

INITRAM_OBJ := $(obj)/lib/console.o $(obj)/lib/vtxprintf.o \
           $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o

(is a bit shorter)
> +# These are possibly not permanent
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o

See above (there are multiple occurenceѕ of this).
> Index: device/Makefile
>
> --- device/Makefile	(revision 462)
> +++ device/Makefile	(working copy)
> @@ -20,7 +20,7 @@
>  ##
>
> $(obj)/device/%.o: $(src)/device/%.c
> $(Q)mkdir -p $(obj)/device
> $(Q)mkdir -p $(dir $@)
> $(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(CC) $(INITCFLAGS) -c $< -o $@

This is for a later patch, but can we have a generic rule for this
kind of thing? So that you only have to say 'FOO_OBJ += foo.o'
in the Makefiles?
> +#
> +# Miscellaneous superio specific build targets!
> +#
>
> +$(obj)/superio/%.o: $(src)/superio/%.c $(obj)/statictree.h
> $(Q)mkdir -p $(dir $@)
> $(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(CC) $(INITCFLAGS) -c $< -o $@
>
> +#
> +# Miscellaneous northbridge specific build targets!
> +#
>
> +$(obj)/northbridge/%.o: $(src)/northbridge/%.c $(obj)/statictree.h
> $(Q)mkdir -p $(dir $@)
> $(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(CC) $(INITCFLAGS) -c $< -o $@
>
> +#
> +# Miscellaneous southbridge specific build targets!
> +#
>
> +$(obj)/southbridge/%.o: $(src)/southbridge/%.c $(obj)/statictree.h
> $(Q)mkdir -p $(dir $@)
> $(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(CC) $(INITCFLAGS) -c $< -o $@

All of the above are almost identical. Can they be merged easily?
> Index: Makefile
>
> --- Makefile	(revision 462)
> +++ Makefile	(working copy)
> @@ -26,7 +26,6 @@
>  SUBLEVEL = 0
>
> have_dotconfig := $(wildcard .config)
> -have_dotxcompile := $(wildcard .xcompile)
>
> src := $(shell pwd)
>  obj := $(shell pwd)/build
> @@ -60,6 +59,9 @@
>  KERNELVERSION = $(VERSION).$(PATCHLEVEL).$(SUBLEVEL)
>  export KERNELVERSION
> include $(shell $(src)/util/xcompile/xcompile > $(src)/.xcompile || \
> { echo "complete\\ toolchain" && rm -f $(src)/.xcompile && exit 1; }; echo $(src)/.xcompile)

No indentation needed, I think.
> Index: util/dtc/Makefile
>
> --- util/dtc/Makefile	(revision 462)
> +++ util/dtc/Makefile	(working copy)
> @@ -13,7 +13,7 @@
>  #all: $(TARGETS)
>
> $(obj)/util/dtc/dtc: $(patsubst %,$(obj)/util/dtc/%,$(DTC_OBJS))
> $(Q)mkdir -p $(obj)/util/dtc/
> $(Q)mkdir -p $(dir $@)

Yep, this is definately a good thing.
> Index: arch/x86/Makefile
>
> --- arch/x86/Makefile	(revision 462)
> +++ arch/x86/Makefile	(working copy)

[...]
> @@ -136,8 +146,11 @@
>    $(Q)printf "  TEST    $(subst $(shell pwd)/,,$(@))\n"
>    $(Q)test `wc -c < $(obj)/stage0.init` -gt 16128 && \
>    	printf "Error. Bootblock got too big.\n" || true
> $(Q)printf "  NM      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(NM) $(obj)/stage0.o |sort -u > $(obj)/stage0.init.map

^
                            missing space

I'd rename Rules.make to Makefile.rules, but other than that:

Acked-by: Uwe Hermann

Uwe.

[Stefan Reinauer]

* Uwe Hermann  [070723 11:54]:
> > +# These are possibly not permanent
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o
>
> INITRAM_OBJ := $(obj)/lib/console.o $(obj)/lib/vtxprintf.o \
>           $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
>
> (is a bit shorter)

INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o $(obj)/lib/vtxprintf.o \
           $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o

would be the way to go I think.
> > $(obj)/device/%.o: $(src)/device/%.c
> > $(Q)mkdir -p $(obj)/device
> > $(Q)mkdir -p $(dir $@)
> > $(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
> > $(Q)$(CC) $(INITCFLAGS) -c $< -o $@
>
> This is for a later patch, but can we have a generic rule for this
> kind of thing? So that you only have to say 'FOO_OBJ += foo.o'
> in the Makefiles?

Yes, that should be the goal I think. I did that for the mainboard/ code
already, but there's still a lot of potential.
> > +$(obj)/southbridge/%.o: $(src)/southbridge/%.c $(obj)/statictree.h
> > $(Q)mkdir -p $(dir $@)
> > $(Q)printf "  CC      $(subst $(shell pwd)/,,$(@))\n"
> > $(Q)$(CC) $(INITCFLAGS) -c $< -o $@
>
> All of the above are almost identical. Can they be merged easily?

good question. They have a different paths and not all files have the 
dependency to statictree.h, otherwise we could hit them with a global
rule. Maybe we can somehow...?

I will play some more to get this right, but I did not want to pile this
stuff up forever, as Marc was waiting for the post_code part while I got
into messing with the Makefiles..
> > printf "Error. Bootblock got too big.\n" || true
> > $(Q)printf "  NM      $(subst $(shell pwd)/,,$(@))\n"
> > $(Q)$(NM) $(obj)/stage0.o |sort -u > $(obj)/stage0.init.map
>                                ^
>                         missing space
>
> I'd rename Rules.make to Makefile.rules, but other than that:
>
> Acked-by: Uwe Hermann

Thanks...

[Stefan Reinauer]

* Uwe Hermann  [070723 11:54]:
> I'd rename Rules.make to Makefile.rules, but other than that:

I left it for now. Googling for Makefile.rules gives 74k hits, Googling
for Rules.make gives 553k hits, and Linux calls that file Rules.make,
too.

We can change it of course, if we feel the need for it.. Is there a
reason to do so?

Stefan

[Uwe Hermann]

On Mon, Jul 23, 2007 at 06:51:40PM +0200, Stefan Reinauer wrote:
> Uwe Hermann  [070723 11:54]:
> > I'd rename Rules.make to Makefile.rules, but other than that:
>
> I left it for now. Googling for Makefile.rules gives 74k hits, Googling
> for Rules.make gives 553k hits, and Linux calls that file Rules.make,
> too.
>
> We can change it of course, if we feel the need for it.. Is there a
> reason to do so?

Nope, Makefile.rules looked like the obvious choice to me, but
Rules.make is fine, too.

Uwe.

[Stefan Reinauer]

* Uwe Hermann  [070723 11:54]:
> Acked-by: Uwe Hermann

r463

[Juergen Beisert]

Hi Stefan,

On Sunday 22 July 2007 17:47, Stefan Reinauer wrote:
> see patch

Arrg. We are in make not in shell...
> +# These are possibly not permanent
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o

This is a way to go and also it shows every reader what you *intent* to do:

INITRAM_OBJ := $(obj)/lib/console.o
INITRAM_OBJ += $(obj)/lib/vtxprintf.o
INITRAM_OBJ += $(obj)/lib/uart8250.o
INITRAM_OBJ += $(obj)/arch/x86/post_code.o

Or choose the way Uwe mentioned. But do not use the variable itself to append 
something to it.

Juergen

[Stefan Reinauer]

* Juergen Beisert  [070723 12:35]:
> Hi Stefan,
>
> On Sunday 22 July 2007 17:47, Stefan Reinauer wrote:
> > see patch
>
> Arrg. We are in make not in shell...
> > +# These are possibly not permanent
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o
>
> This is a way to go and also it shows every reader what you *intent* to do:
>
> INITRAM_OBJ := $(obj)/lib/console.o

^^^ +=
> INITRAM_OBJ += $(obj)/lib/vtxprintf.o
> INITRAM_OBJ += $(obj)/lib/uart8250.o
> INITRAM_OBJ += $(obj)/arch/x86/post_code.o
>
> Or choose the way Uwe mentioned. But do not use the variable itself to append 
> something to it.

Ok, will change this. Though I don't think it makes a big difference
internally. Both ways just append a string to another one..

[Juergen Beisert]

Hi Stefan,

On Monday 23 July 2007 12:47, Stefan Reinauer wrote:
> Juergen Beisert  [070723 12:35]:
> > On Sunday 22 July 2007 17:47, Stefan Reinauer wrote:
> > > see patch
> >
> > Arrg. We are in make not in shell...
> > > +# These are possibly not permanent
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o
> >
> > This is a way to go and also it shows every reader what you *intent* to
> > do:
> >
> > INITRAM_OBJ := $(obj)/lib/console.o
>           ^^^ +=

NACK! The first one define variable's flavour (refer make manual)

"=" variable expansion when used (slow in large build environments), ":=" 
variable expansion immediatly (mostly faster, but with (sometimes ugly) side 
effects!)

Juergen

[Stefan Reinauer]

* Juergen Beisert  [070723 13:21]:
> Hi Stefan,
>
> On Monday 23 July 2007 12:47, Stefan Reinauer wrote:
> > Juergen Beisert  [070723 12:35]:
> > > On Sunday 22 July 2007 17:47, Stefan Reinauer wrote:
> > > > see patch
> > >
> > > Arrg. We are in make not in shell...
> > > > +# These are possibly not permanent
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o
> > >
> > > This is a way to go and also it shows every reader what you *intent* to
> > > do:
> > >
> > > INITRAM_OBJ := $(obj)/lib/console.o
> >           ^^^ +=
>
> NACK! The first one define variable's flavour (refer make manual)

Sure, but your suggestion would delete the existing objects in the list.

Stefan

[Juergen Beisert]

On Monday 23 July 2007 12:47, Stefan Reinauer wrote:
> Juergen Beisert  [070723 12:35]:
> > Hi Stefan,
> >
> > On Sunday 22 July 2007 17:47, Stefan Reinauer wrote:
> > > see patch
> >
> > Arrg. We are in make not in shell...
> > > +# These are possibly not permanent
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o
> >
> > This is a way to go and also it shows every reader what you *intent* to
> > do:
> >
> > INITRAM_OBJ := $(obj)/lib/console.o
>           ^^^ +=

Ah, I see. Its defined at another position. Than it is "+=". Yes.

Juergen

[Stefan Reinauer]

* Juergen Beisert  [070723 13:40]:
> On Monday 23 July 2007 12:47, Stefan Reinauer wrote:
> > Juergen Beisert  [070723 12:35]:
> > > Hi Stefan,
> > >
> > > On Sunday 22 July 2007 17:47, Stefan Reinauer wrote:
> > > > see patch
> > >
> > > Arrg. We are in make not in shell...
> > > > +# These are possibly not permanent
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/console.o
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/vtxprintf.o
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/lib/uart8250.o
> > > > +INITRAM_OBJ := $(INITRAM_OBJ) $(obj)/arch/x86/post_code.o
> > >
> > > This is a way to go and also it shows every reader what you *intent* to
> > > do:
> > >
> > > INITRAM_OBJ := $(obj)/lib/console.o
> >           ^^^ +=
>
> Ah, I see. Its defined at another position. Than it is "+=". Yes.

Thanks for the hint, I will fix this tonight and check it in.