| Message ID | 1319509652-5648-1-git-send-email-leiwen@marvell.com |
|---|---|
| State | Accepted |
| Commit | 5ff8b35412e895287d91172c9ac3b60520d41ddb |
| Headers | show |
Dear Lei Wen, In message <1319509652-5648-1-git-send-email-leiwen@marvell.com> you wrote: > For files like the drivers/serial/serial.c, it must include the > platform file, as the CONFIG_SYS_NS16550_COM1 must reference to > the definition in the platform definition files. > > Include the platform definition file in the config file, so that it > would decouple the dependence for the driver files. ... > --- a/include/configs/edminiv2.h > +++ b/include/configs/edminiv2.h > @@ -45,6 +45,7 @@ > #define CONFIG_88F5182 1 /* SOC Name */ > #define CONFIG_MACH_EDMINIV2 1 /* Machine type */ > > +#include <asm/arch/orion5x.h> I don't like this. Board config files MUST NOT do any such includes. Keep in mind that one day we might want to try something like Kconfig, so board config files should ONLY contain configuration information in the form of #define's and the like. NAK. Best regards, Wolfgang Denk
Hi Wolfgang, On Tue, Oct 25, 2011 at 3:43 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <1319509652-5648-1-git-send-email-leiwen@marvell.com> you wrote: >> For files like the drivers/serial/serial.c, it must include the >> platform file, as the CONFIG_SYS_NS16550_COM1 must reference to >> the definition in the platform definition files. >> >> Include the platform definition file in the config file, so that it >> would decouple the dependence for the driver files. > ... > >> --- a/include/configs/edminiv2.h >> +++ b/include/configs/edminiv2.h >> @@ -45,6 +45,7 @@ >> #define CONFIG_88F5182 1 /* SOC Name */ >> #define CONFIG_MACH_EDMINIV2 1 /* Machine type */ >> >> +#include <asm/arch/orion5x.h> > > I don't like this. > > > Board config files MUST NOT do any such includes. Keep in mind that > one day we might want to try something like Kconfig, so board config > files should ONLY contain configuration information in the form of > #define's and the like. > > NAK. I should admit I have some bit of confusing here... What I see there are lots of include in the config file, 742 include usage case in the config files...: $ grep -Rn "#include" include/configs | wc -l 742 Or should I directly hard coding the CONFIG_SYS_NS16550_COM1 insteading the macro ORION5X_UART0_BASE, since this macro is defined in arch/arm/include/asm/arch-orion5x/orion5x.h... Thanks, Lei
Dear Lei Wen, In message <CALZhoSQyCPnYCXgrYQCkvu6A0NYCrmtzveXcQsb3OSCMV_TKcA@mail.gmail.com> you wrote: > > >> +#include <asm/arch/orion5x.h> > > > > I don't like this. > > > > > > Board config files MUST NOT do any such includes. Keep in mind that > > one day we might want to try something like Kconfig, so board config > > files should ONLY contain configuration information in the form of > > #define's and the like. > > > > NAK. > > I should admit I have some bit of confusing here... > What I see there are lots of include in the config file, 742 include > usage case in the config files...: > $ grep -Rn "#include" include/configs | wc -l > 742 There are includes. Some of them are OK, some aren't and should be fixed. More than half of your number are "#include <config_cmd_default.h>" which are perfectly fine. For example, it's also perfectly fine if all boards that are similar or that provide a similar look & feel (for example, because they come from a single vendor) include a common configuration file. Such examples are #include "amcc-common.h", #include "mv-common.h", #include <configs/bfin_adi_common.h>, #include "manroland/common.h", etc. etc. etc. On the other hand, files from asm/arch/* should never define any config settings - if it cannot be avoided they may provide register definitions and the like, but any "#define CONFIG_*" has no place there. Best regards, Wolfgang Denk
Hi Wolfgang, On Wed, Oct 26, 2011 at 2:50 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Lei Wen, > > In message <CALZhoSQyCPnYCXgrYQCkvu6A0NYCrmtzveXcQsb3OSCMV_TKcA@mail.gmail.com> you wrote: >> >> >> +#include <asm/arch/orion5x.h> >> > >> > I don't like this. >> > >> > >> > Board config files MUST NOT do any such includes. Keep in mind that >> > one day we might want to try something like Kconfig, so board config >> > files should ONLY contain configuration information in the form of >> > #define's and the like. >> > >> > NAK. >> >> I should admit I have some bit of confusing here... >> What I see there are lots of include in the config file, 742 include >> usage case in the config files...: >> $ grep -Rn "#include" include/configs | wc -l >> 742 > > There are includes. Some of them are OK, some aren't and should be > fixed. > > More than half of your number are "#include <config_cmd_default.h>" > which are perfectly fine. > > > For example, it's also perfectly fine if all boards that are similar > or that provide a similar look & feel (for example, because they come > from a single vendor) include a common configuration file. Such > examples are #include "amcc-common.h", #include "mv-common.h", > #include <configs/bfin_adi_common.h>, #include "manroland/common.h", > etc. etc. etc. > > On the other hand, files from asm/arch/* should never define any > config settings - if it cannot be avoided they may provide register > definitions and the like, but any "#define CONFIG_*" has no place > there. > Thanks for your detailed explanation! I post another patch to address those issues you have mentioned. Thanks, Lei
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index 05bd45c..792b11d 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -28,8 +28,9 @@ #include <common.h> #include <netdev.h> #include <asm/cache.h> +#include <asm/io.h> #include <u-boot/md5.h> -#include <asm/arch/orion5x.h> +#include <asm/arch/cpu.h> #include <hush.h> #define BUFLEN 16 diff --git a/arch/arm/cpu/arm926ejs/orion5x/dram.c b/arch/arm/cpu/arm926ejs/orion5x/dram.c index 5cc31a9..c0f7ef1 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/dram.c +++ b/arch/arm/cpu/arm926ejs/orion5x/dram.c @@ -27,7 +27,7 @@ #include <common.h> #include <config.h> -#include <asm/arch/orion5x.h> +#include <asm/arch/cpu.h> DECLARE_GLOBAL_DATA_PTR; diff --git a/arch/arm/cpu/arm926ejs/orion5x/timer.c b/arch/arm/cpu/arm926ejs/orion5x/timer.c index 17df68f..e39ecc2 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/timer.c +++ b/arch/arm/cpu/arm926ejs/orion5x/timer.c @@ -25,7 +25,7 @@ */ #include <common.h> -#include <asm/arch/orion5x.h> +#include <asm/io.h> #define UBOOT_CNTR 0 /* counter to use for uboot timer */ diff --git a/arch/arm/include/asm/arch-orion5x/orion5x.h b/arch/arm/include/asm/arch-orion5x/orion5x.h index 9aeef88..18225b9 100644 --- a/arch/arm/include/asm/arch-orion5x/orion5x.h +++ b/arch/arm/include/asm/arch-orion5x/orion5x.h @@ -30,13 +30,7 @@ #ifndef _ASM_ARCH_ORION5X_H #define _ASM_ARCH_ORION5X_H -#ifndef __ASSEMBLY__ -#include <asm/types.h> -#include <asm/io.h> -#endif /* __ASSEMBLY__ */ - #if defined(CONFIG_FEROCEON) -#include <asm/arch/cpu.h> /* SOC specific definations */ #define ORION5X_REGISTER(x) (ORION5X_REGS_PHY_BASE + x) diff --git a/common/cmd_ide.c b/common/cmd_ide.c index da5189c..d909c54 100644 --- a/common/cmd_ide.c +++ b/common/cmd_ide.c @@ -46,12 +46,6 @@ #include <mpc5xxx.h> #endif -#ifdef CONFIG_ORION5X -#include <asm/arch/orion5x.h> -#elif defined CONFIG_KIRKWOOD -#include <asm/arch/kirkwood.h> -#endif - #include <ide.h> #include <ata.h> diff --git a/include/configs/edminiv2.h b/include/configs/edminiv2.h index f8affa8..88d32b2 100644 --- a/include/configs/edminiv2.h +++ b/include/configs/edminiv2.h @@ -45,6 +45,7 @@ #define CONFIG_88F5182 1 /* SOC Name */ #define CONFIG_MACH_EDMINIV2 1 /* Machine type */ +#include <asm/arch/orion5x.h> /* * CLKs configurations */
For files like the drivers/serial/serial.c, it must include the platform file, as the CONFIG_SYS_NS16550_COM1 must reference to the definition in the platform definition files. Include the platform definition file in the config file, so that it would decouple the dependence for the driver files. Signed-off-by: Lei Wen <leiwen@marvell.com> --- arch/arm/cpu/arm926ejs/orion5x/cpu.c | 3 ++- arch/arm/cpu/arm926ejs/orion5x/dram.c | 2 +- arch/arm/cpu/arm926ejs/orion5x/timer.c | 2 +- arch/arm/include/asm/arch-orion5x/orion5x.h | 6 ------ common/cmd_ide.c | 6 ------ include/configs/edminiv2.h | 1 + 6 files changed, 5 insertions(+), 15 deletions(-)