Re: [PATCH 0/6] MN10300: Add the MN10300 architecture to Linux kernel [try #4]

David Howells
2007-11-08T19:11:23Z

Sam Ravnborg <sam@ravnborg.org> wrote:

>
> arch/mn10300/Makefile:
>
> 1) Use KBUILD_CFLAGS & KBUILD_AFLAGS & KBUILD_CPPFLAGS - they
> have replaced the former xFLAGS.

Done. What about ASFLAGS and LDFLAGS?

> 2) Consider using unit-y as replacement for UNIT.
> This enable you to do:
> unit-$(CONFIG_MN10300_UNIT_ASB2303) += asb2303
>
> 3) Same for PROCESSOR => processor-y

That makes sense if I'm not calculating UNIT or PROCESSOR anyway. However,
see below.

> 4) Drop the symlinks - they are evil...
> Use a structure like:
> include/asm-mn10300/asb2303/proc/*.h

No. I think all the arch headers should really appear to be #included under
asm/. That means that the structure would have to be:

include/asm-mn10300/asb2303/asm/proc/*.h

That then puts all these header files several levels further down, which isn't
that good.

> Then you adjust -I include/asm-mn10300/asb2303
> and no symlinks needed.
> And if you change processor kbuild will notice
> and recompile everything.

That's the only plus, but it's a smaller plus than not interpolating several
levels of almost empty directory into the paths for the proc- and unit-specific
headers.

> No-one else does it this way but mn10300 could show how to do it.

I could. Or I could do what everyone else does.

Actually, what you perhaps ought to do for a start is to move the individual
include/asm-$ARCH dirs to arch/$ARCH/asm and then you can avoid that symlink
too.

> 5)
> +zImage: vmlinux
> + $(Q)$(MAKE) $(build)=$(boot) $(KBUILD_IMAGE)
> +
> +all: zImage
> +
> +Image: vmlinux
> + $(Q)$(MAKE) $(build)=arch/mn10300/boot $@
>
> This could be done as:
> +Image zImage: vmlinux
> + $(Q)$(MAKE) $(build)=$(boot) $(boot)/$@

Done.

> And then modify boot/Makefile accordingly

I don't see why it needs modification. $(KBUILD_IMAGE) == $(boot)/zImage for
the zImage target.

> 6) archdep is long gone - delete the following

Done.

> 7) Please set KBUILD_DEFCONFIG := asb2303_defconfig
> This enable build monkeys(1) to do "make ARCH=mn10300 defconfig"

Done.

> arch/mn10300/boot/Makefile:
>
> 1) You can safely remove the copyright of Linus..
> Same goes for other places where his copyright are kept but filecontent
> is new.

The copyright assignment on some of these files was made by MEI. I'm not sure
I can change them.

> 2) The Image target is missing (as defined in arch/mn10300/Makefile
> Can you just kill it?

Yes. Done.

> arch/mn10300/boot/compressed/Makefile:
> 1) Is -tarditional really needed. We try to kill it all over (slowly)

Apparently not. Consider it gone.

> 2) Please name the linker script *.lds

Done.

> arch/mn10300/kernel/Makefile:
> +# This next line shouldn't be necessary, but it is.
> +CPPFLAGS += $(CPPFLAGS_$(@F))
> +CPPFLAGS_asm-offsets.s = -D__ASM_OFFSETS_H__
>
> Please explain why...

I don't remember why. It doesn't seem to be necessary now, however, so it's
now gone.

> arch/mn10300/oprofile/Makefile:
> +profdrvr-y := op_model_null.o
> +
> +oprofile-y := $(DRIVER_OBJS) $(profdrvr-y)
>
> The variable profdrvr seems useless.

Gone.

> arch/mn10300/unit-asb2303/Makefile:
>
> Please drop this wrong comment:
> +# Note! Dependencies are done automagically by 'make dep', which also
> +# removes any old dependencies. DON'T put your own dependencies here
> +# unless it's something special (ie not a .c file).
> +#
> +# Note 2! The CFLAGS definitions are now in the main makefile...
> +
>
> Seen in other places where they are equally wrong/outdated.

Gone.

David