[U-Boot] [PATCH] Add support for the galaxy5200 board.

[Wolfgang Denk]

Dear "Eric Millbrandt",

In message <A88094362DE0AE49A118AB9B4EB3612403F9E020 at dekaexchange.deka.local> you wrote:
> 
> Add support for the DEKA R&D galaxy5200 board.
> The patch is based off of the mpc5xxx branch.

Please don't. Please follow the instructions and provide patches ONLY
against "master" resp "next".

> This e-mail and the information, including any attachments, it contains =
> are intended to be a confidential communication only to the person or =
> entity to whom it is addressed and may contain information that is =
> privileged. If the reader of this message is not the intended recipient, =
> you are hereby notified that any dissemination, distribution or copying =
> of this communication is strictly prohibited. If you have received this =
> communication in error, please immediately notify the sender and destroy =
> the original message.

I see. Well, in this case we are unfortunately not able to even look
at your patch.

> Content-Transfer-Encoding: base64
> Content-Description: V1-Add-support-for-the-galaxy5200.patch
> Content-Disposition: attachment;
> 	filename="V1-Add-support-for-the-galaxy5200.patch"
> 
> RnJvbSA5MDYwNWZkMWM0MzEyMjAxYjMwMWY2NjAyYTVlNjk5MTA1ZmJkNDNiIE1vbiBTZXAgMTcg
> MDA6MDA6MDAgMjAwMQpGcm9tOiBFcmljIE1pbGxicmFuZHQgPGVtaWxsYnJhbmR0QGRla2FyZXNl
> YXJjaC5jb20+CkRhdGU6IFdlZCwgMTIgQXVnIDIwMDkgMTI6NDM6NTkgLTA0MDAKU3ViamVjdDog
> W1BBVENIXSBBZGQgc3VwcG9ydCBmb3IgdGhlIERFS0EgUmVzZWFyY2ggYW5kIERldmVsb3BtZW50
> IGdhbGF4eTUyMDAgYm9hcmQuCgpTaWduZWQtb2ZmLWJ5OiBFcmljIE1pbGxicmFuZHQgPGVtaWxs
> YnJhbmR0QGRla2FyZXNlYXJjaC5jb20+Ci0tLQogTUFJTlRBSU5FUlMgICAgICAgICAgICAgICAg
... 

And I cannot read this either. It doesn't look like plain text to me.

Please make sure to follow the instructions in
http://www.denx.de/wiki/U-Boot/Patches


> --- a/Makefile
> +++ b/Makefile
> @@ -557,6 +557,16 @@ digsy_mtc_RAMBOOT_config:	unconfig
>  		}
>  	@$(MKCONFIG) -a digsy_mtc  ppc mpc5xxx digsy_mtc
>  
> +galaxy5200_config \
> +galaxy5200_LOWBOOT_config:	unconfig
> +	@mkdir -p $(obj)include $(obj)board/galaxy5200
> +	@ >$(obj)include/config.h
> +	@[ -z "$(findstring LOWBOOT_,$@)" ] || \
> +		{ echo "TEXT_BASE = 0xFE000000"	>$(obj)board/galaxy5200/config.tmp ; \
> +		  echo "... with LOWBOOT configuration" ; \
> +		}
> +	@$(MKCONFIG) -a galaxy5200 ppc mpc5xxx galaxy5200
> +

Please do not add such scripting to the Makefile; do this in your
board config file instead; for an example, please see 
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/65499

> +void init_ide_reset (void)
> +{
> +	debug ("init_ide_reset\n");
> +
> +	/* Configure TIMER_5 as GPIO output for ATA reset */
> +	volatile struct mpc5xxx_gpt *gpt = (struct mpc5xxx_gpt *)MPC5XXX_GPT;

Please do not put declarations in the middle of the code, only at the
start of a block.

> +void ide_set_reset (int idereset)
> +{
> +	debug ("ide_reset(%d)\n", idereset);
> +
> +	/* Configure TIMER_5 as GPIO output for ATA reset */
> +	volatile struct mpc5xxx_gpt *gpt = (struct mpc5xxx_gpt *)MPC5XXX_GPT;

Ditto.

> +	if (idereset) {
> +		gpt[5].emsr = MPC5XXX_GPT_GPIO_OUT0 | MPC5XXX_GPT_TMS_GPIO;
> +
> +		/* Make a delay. MPC5200 spec says 25 usec min */
> +		udelay(500000);

Why do you exceed the needed delay by a factor of 20,000 ?

> +	} else {
> +		gpt[5].emsr = MPC5XXX_GPT_GPIO_OUT1 | MPC5XXX_GPT_TMS_GPIO;
> +	}

Don't we need a delay here, too?

> +#define	CONFIG_EXTRA_ENV_SETTINGS					\
> +	"filesize=0\0"							\
> +	"baudrate=115200\0"						\
> +	"stdin=serial\0"						\
> +	"stdout=serial\0"						\
> +	"stderr=serial\0"						\

Are you sure you need these?

> +	"ethaddr=unconfigured\0"					\
> +	"hostname=unconfigured\0"					\

PLease delete these. It is counterproductive to put incorrect values
there. Rather leave these undefined.

> +#define CONFIG_BOOTCOMMAND		"boot"

What are you trying to do here? "boot" means: "run CONFIG_BOOTCOMMAND",
which then will "run CONFIG_BOOTCOMMAND", which then will ... ?


> +#define CONFIG_SYS_FLASH_BASE		0xfe000000
> +/* The flash size is autoconfigured, but cpu/mpc5xxx/cpu_init.c needs this 
> +   variable defined */

Incorrect multiline comment style. Please fix globally.

> +/*---------------------------------------------------------------------------
> + Miscellaneous configurable options
> +---------------------------------------------------------------------------*/
> +#define CONFIG_SYS_LONGHELP	/* undef to save memory */
> +#define CONFIG_SYS_PROMPT "uboot> " /* Monitor Command Prompt */
> +
> +#define CONFIG_CMDLINE_EDITING 1 /* add command line history */
> +
> +#define CONFIG_SYS_CACHELINE_SIZE 32 /* For MPC5xxx CPUs */
> +#if defined(CONFIG_CMD_KGDB)
> +#define CONFIG_SYS_CACHELINE_SHIFT 5 /* log base 2 of the above value */
> +#endif
> +
> +#if defined(CONFIG_CMD_KGDB)
> +#define CONFIG_SYS_CBSIZE 1024 /* Console I/O Buffer Size */
> +#else
> +#define CONFIG_SYS_CBSIZE 512 /* Console I/O Buffer Size */
> +#endif
> +#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + sizeof(CONFIG_SYS_PROMPT) + 16)
> +							/* Print Buffer Size */
> +#define CONFIG_SYS_MAXARGS 32 /* max number of command args */
> +#define CONFIG_SYS_BARGSIZE CONFIG_SYS_CBSIZE /* Boot Argument Buffer Size */
> +
> +#define CONFIG_SYS_MEMTEST_START 0x00100000 /* memtest works on */
> +#define CONFIG_SYS_MEMTEST_END 0x00f00000 /* 1 ... 15 MB in DRAM */
> +
> +#define CONFIG_SYS_LOAD_ADDR 0x400000 /* default load address */
> +#define CONFIG_SYS_HZ 1000 /* decrementer freq: 1 ms ticks */

How about some vertical alignment of the comments?

> +#define CONFIG_DISPLAY_BOARDINFO 1
> +
> +#define CONFIG_SYS_HUSH_PARSER 1
> +#define CONFIG_SYS_PROMPT_HUSH_PS2   "uboot> "

I consider it a really bad idea to have PS1 and PS2 the same. Please
fix.

> +#ifdef CONFIG_BOOTP_MASK
> +#undef CONFIG_BOOTP_MASK
> +#endif

Don't do this. Either it's not needed, or it hushes up bugs.


There are also some minor coding style issues (trailing white space).


Please clean up and resubmit (following the rules as mentioned above).

Thanks.

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
You can love it, change it, or leave it.    There is NO other option.
But do not complain - it is your own choice...                  -- wd