[U-Boot] [PATCH 1/1] Adding Configuration option for TQM5200S Board on Goetting
Wolfgang Denk
wd at denx.de
Mon Jul 27 00:02:43 CEST 2009
Dear Axel Beierlein,
In message <1248471861-8085-1-git-send-email-beierlein at goetting.de> you wrote:
> Signed-off-by: Axel Beierlein <beierlein at goetting.de>
> ---
> Makefile | 7 +++++++
> board/tqc/tqm5200/tqm5200.c | 9 ++++++++-
> include/configs/TQM5200.h | 33 ++++++++++++++++++++++++++-------
> 3 files changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 2a06440..7f197dd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -764,6 +764,7 @@ Total5200_Rev2_lowboot_config: unconfig
> }
> @$(MKCONFIG) -a Total5200 ppc mpc5xxx total5200
>
> +HG43630_config \
> cam5200_config \
> cam5200_niosflash_config \
> fo300_config \
Please use a lower case board name, and keep list sorted.
> @@ -808,6 +809,12 @@ TQM5200_STK100_config: unconfig
> @[ -z "$(findstring HIGHBOOT,$@)" ] || \
> { echo "TEXT_BASE = 0xFFF00000" >$(obj)board/tqm5200/config.tmp ; \
> }
> + @[ -z "$(findstring HG43630,$@)" ] || \
> + { echo "#define CONFIG_HG43630" >>$(obj)include/config.h ; \
> + echo "#define CONFIG_TQM5200S" >>$(obj)include/config.h ; \
> + echo "#define CONFIG_TQM5200_B" >>$(obj)include/config.h ; \
> + $(XECHO) "... TQM5200S on Goetting HG43630 Board" ; \
> + }
> @$(MKCONFIG) -n $@ -a TQM5200 ppc mpc5xxx tqm5200 tqc
Please don't add board specific configuration to the top level
Makefile; do thi sin your board config file instead.
> diff --git a/board/tqc/tqm5200/tqm5200.c b/board/tqc/tqm5200/tqm5200.c
> index faa2e02..0d69e86 100644
> --- a/board/tqc/tqm5200/tqm5200.c
> +++ b/board/tqc/tqm5200/tqm5200.c
> @@ -255,6 +255,10 @@ int checkboard (void)
> # error "UNKNOWN"
> #endif
>
> +#ifdef CONFIG_HG43630
> +#define CARRIER_NAME "HG43630"
> +#endif
Please add as "#elif" in the "#if" construct above. And move the block
out of checkboard(), please.
> {
> if (line_number == 1) {
> strcpy (info, " Board: TQM5200 (TQ-Components GmbH)");
> -#if defined (CONFIG_STK52XX) || defined (CONFIG_TB5200) || defined(CONFIG_FO300)
> +#if defined (CONFIG_STK52XX) || defined (CONFIG_TB5200) || defined(CONFIG_FO300) || defined(CONFIG_HG43630)
Line too long.
> #if defined (CONFIG_STK52XX)
> strcpy (info, " on a STK52xx carrier board");
> @@ -657,6 +661,9 @@ void video_get_info_str (int line_number, char *info)
> #if defined (CONFIG_FO300)
> strcpy (info, " on a FO300 carrier board");
> #endif
> +#if defined (CONFIG_HG43630)
> + strcpy (info, " on a HG43630 carrier board");
> +#endif
It's time to get rid of this "#if" here and use something like
strcpy (info, " on a " CARRIER_NAME " carrier board");
instead, as it has been done in checkboard() long ago.
> diff --git a/include/configs/TQM5200.h b/include/configs/TQM5200.h
> index a4336a7..0f5ff77 100644
> --- a/include/configs/TQM5200.h
> +++ b/include/configs/TQM5200.h
> @@ -71,7 +71,7 @@
> /* switch is open */
> #endif /* CONFIG_FO300 */
>
> -#ifdef CONFIG_STK52XX
> +#if defined(CONFIG_STK52XX) && !defined(CONFIG_HG43630)
Is CONFIG_STK52XX really defined for CONFIG_HG43630? where?
> #define CONFIG_PS2KBD /* AT-PS/2 Keyboard */
> #define CONFIG_PS2MULT /* .. on PS/2 Multiplexer */
> #define CONFIG_PS2SERIAL 6 /* .. on PSC6 */
> @@ -85,6 +85,7 @@
> * 0x50000000 - 0x50ffffff - PCI IO Space
> */
> #ifdef CONFIG_STK52XX
> +#ifndef CONFIG_HG43630
> #define CONFIG_PCI 1
> #define CONFIG_PCI_PNP 1
> /* #define CONFIG_PCI_SCAN_SHOW 1 */
> @@ -96,7 +97,7 @@
> #define CONFIG_PCI_IO_BUS 0x50000000
> #define CONFIG_PCI_IO_PHYS CONFIG_PCI_IO_BUS
> #define CONFIG_PCI_IO_SIZE 0x01000000
> -
> +#endif/* ifndef(CONFIG_HG43630)*/
Add blank line here. But - isn;t it sufficient to only not define
CONFIG_PCI, while leaving the rest unchanged?
> -#if defined(CONFIG_STK52XX) || defined(CONFIG_FO300)
> +#if defined(CONFIG_STK52XX) || defined(CONFIG_FO300) && !defined(CONFIG_HG43630)
Are you sure this logic is correct, and necessary?
> @@ -196,7 +197,7 @@
> #define CONFIG_PCIAUTO_SKIP_HOST_BRIDGE 1
> #endif
>
> -#if defined(CONFIG_MINIFAP) || defined(CONFIG_STK52XX) || defined(CONFIG_FO300)
> +#if defined(CONFIG_MINIFAP) || defined(CONFIG_STK52XX) || defined(CONFIG_FO300)
Please do not add trailing white space.
> #ifdef CONFIG_STK52XX
> # if defined(CONFIG_TQM5200_B)
> # if defined(CONFIG_SYS_LOWBOOT)
> +# if defined(CONFIG_HG43630)
> +# define MTDPARTS_DEFAULT "mtdparts=TQM5200-0:512k(u-boot)," \
> + "512k(env)," \
> + "2m(kernel)," \
> + "16m(ramfs1)," \
> + "5m(ramfs2)," \
> + "8m(jffs2)" \
> +
> +# else
> # define MTDPARTS_DEFAULT "mtdparts=TQM5200-0:1m(firmware)," \
> "256k(dtb)," \
> "2304k(kernel)," \
> @@ -422,6 +438,7 @@
> "2m(initrd)," \
> "8m(misc)," \
> "16m(big-fs)"
> +# endif /* CONFIG_HG43630 */
> # else /* highboot */
> # define MTDPARTS_DEFAULT "mtdparts=TQM5200-0:2560k(kernel)," \
This #ifdef block is becoming an unreadable mess. Please split up into
separate definitions for each board plus a default config, something
like
#define MTDPARTS_DEFAULT ...
#define MTDPARTS_HG43630 ...
#define MTDPARTS_CAM5200 ...
#define MTDPARTS_FO300 ...
#define MTDPARTS_STK5200 MTDPARTS_DEFAULT
and then use reference the needed version based on the CARRIER_NAME
setting.
> #undef CONFIG_IDE_8xx_DIRECT /* Direct IDE not supported */
> #undef CONFIG_IDE_LED /* LED for ide not supported */
> -
Don't delete this blank line.
> #define CONFIG_IDE_RESET /* reset for ide supported */
> #define CONFIG_IDE_PREINIT
>
> +#ifndef CONFIG_HG43630
> #define CONFIG_SYS_IDE_MAXBUS 1 /* max. 1 IDE bus */
> #define CONFIG_SYS_IDE_MAXDEVICE 2 /* max. 2 drives per IDE bus */
> -
> +#else
> +#define CONFIG_SYS_IDE_MAXBUS 0
> +#define CONFIG_SYS_IDE_MAXDEVICE 0
> +#endif /* CONFIG_HG43630 */
> #define CONFIG_SYS_ATA_IDE0_OFFSET 0x0000
Is it not sufficient to not enable IDE support?
> #define CONFIG_SYS_ATA_BASE_ADDR MPC5XXX_ATA
> @@ -725,7 +745,6 @@
>
> /* Support ATAPI devices */
> #define CONFIG_ATAPI 1
> -
> /*-----------------------------------------------------------------------
Don't delete this blank line.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
KLB is an acronym for `Known Lazy Bastard', aka non-FAQ reader, aka
person who would rather make someone take their time to explain
something basic than look it up in a FAQ.
-- Tom Christiansen in <6aq547$mnr$2 at csnews.cs.colorado.edu>
More information about the U-Boot
mailing list