[U-Boot] [PATCH 4/7] PXA: Palm Tungsten|C Support

[Wolfgang Denk]

Dear Marek Vasut,

In message <1279811005-21858-4-git-send-email-marek.vasut at gmail.com> you wrote:
> This patch adds support for the Palm Tungsten|C PXA255 board. The support
> includes:
> - LCD
> - MMC
> - UART
> - NOR
> 
> Signed-off-by: Marek Vasut <marek.vasut at gmail.com>
> ---
>  board/palmtc/Makefile        |   54 +++++++++
>  board/palmtc/config.mk       |    3 +
>  board/palmtc/lowlevel_init.S |   40 +++++++
>  board/palmtc/palmtc.c        |   77 +++++++++++++
>  board/palmtc/u-boot.lds      |   56 ++++++++++
>  boards.cfg                   |    1 +
>  include/configs/palmtc.h     |  250 ++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 481 insertions(+), 0 deletions(-)
>  create mode 100644 board/palmtc/Makefile
>  create mode 100644 board/palmtc/config.mk
>  create mode 100644 board/palmtc/lowlevel_init.S
>  create mode 100644 board/palmtc/palmtc.c
>  create mode 100644 board/palmtc/u-boot.lds
>  create mode 100644 include/configs/palmtc.h

Entries to MAINTAINERS and MAKEALL missing.

> diff --git a/board/palmtc/Makefile b/board/palmtc/Makefile
> new file mode 100644
> index 0000000..92e5288
> --- /dev/null
> +++ b/board/palmtc/Makefile
> @@ -0,0 +1,54 @@
> +
> +#
> +# Copyright (C) 2009

2009?


> --- /dev/null
> +++ b/board/palmtc/config.mk
> @@ -0,0 +1,3 @@
> +#TEXT_BASE = 0xa1700000
> +TEXT_BASE = 0xa1000000
> +#TEXT_BASE = 0

Please remove dead code.


> +int board_init (void)
> +{
> +	/* memory and cpu-speed are setup before relocation */
> +	/* so we do _nothing_ here */

Incorrect multiline comment style.

> +	/* arch number of Lubbock-Board */
> +	gd->bd->bi_arch_number = MACH_TYPE_PALMTC;
> +
> +	/* Adress of boot parameters */
> +	gd->bd->bi_boot_params = 0xa0000100;
> +
> +	/* Set PWM for LCD */
> +	PWM_CTRL1 = 0x5f;
> +	PWM_PERVAL1 = 0x3ff;
> +	PWM_PWDUTY1 = 892;
> +
> +	return 0;
> +}
> +
> +int board_late_init(void)
> +{
> +#ifdef CONFIG_LCD
> +	setenv("stdout", "lcd");
> +	setenv("stderr", "lcd");
> +#else
> +	setenv("stdout", "serial");
> +	setenv("stderr", "serial");
> +#endif

It's a bad idea to force such settings on a user without leaving him
any other choice.  We had such discussions before. Please don't.

...
> +#define CONFIG_PXA250			1	/* Intel PXA255 CPU */
> +#define CONFIG_PALMTC			1	/* Palm Tungsten|C board */
> +
> +#undef	BOARD_LATE_INIT
> +#undef	CONFIG_SKIP_RELOCATE_UBOOT
> +#undef	CONFIG_USE_IRQ
> +#undef	CONFIG_SKIP_LOWLEVEL_INIT

Please do not undef what is not defined anyway.

> +/*
> + * Environment settings
> + */
> +#define	CONFIG_ENV_OVERWRITE
> +#define CONFIG_ENV_IS_IN_FLASH		1
> +#define CONFIG_ENV_ADDR			0x40000
> +#define CONFIG_ENV_SIZE			0x40000

Decide if you use TABs or SPACEs here (SPACEs recommended), but use
that consistently then.

> +#define	CONFIG_SYS_MALLOC_LEN		(CONFIG_ENV_SIZE + CONFIG_STACKSIZE)

That looks awfully wrong - the stack has nothing to do with the malloc
arena. If you want to use just the same size (which does not make much
sense to me either), then use aan independent variable for that.

> +#define	CONFIG_SYS_GBL_DATA_SIZE	512

Don't! Make as small as possible. You probably do NOT need that much,
or do you?

...
> +#define CONFIG_SYS_CPUSPEED		0x161		/* standard setting for 312MHz; L=16, N=1.5, A=0, SDCLK!=SystemBus */

Line too long. Please fix globally.


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
There's an old story about the person who wished his computer were as
easy to use as his telephone. That wish has come  true,  since  I  no
longer know how to use my telephone.