Patchwork [U-Boot] ARM: orion5x: reduce dependence of including platform file

login
register
mail settings
Submitter Lei Wen
Date Oct. 25, 2011, 2:27 a.m.
Message ID <1319509652-5648-1-git-send-email-leiwen@marvell.com>
Download mbox | patch
Permalink /patch/121471/
State Accepted
Commit 5ff8b35412e895287d91172c9ac3b60520d41ddb
Headers show

Comments

Lei Wen - Oct. 25, 2011, 2:27 a.m.
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(-)
Wolfgang Denk - Oct. 25, 2011, 7:43 a.m.
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
Lei Wen - Oct. 25, 2011, 8:58 a.m.
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
Wolfgang Denk - Oct. 25, 2011, 6:50 p.m.
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
Lei Wen - Oct. 26, 2011, 2:23 a.m.
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

Patch

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
  */