[U-Boot] [PATCH] ARMV7: Add support For Logic OMAP35x/DM37x modules

[Wolfgang Denk]

Dear Peter Barada,

In message <1324067511-14142-1-git-send-email-peter.barada at logicpd.com> you wrote:
> This patch adds basic support for OMAP35x/DM37x SOM LV/Torpedo
> reference boards. It assumes U-boot is loaded to SDRAM with the
> help of another small bootloader (x-load) running from SRAM.
> 
> Signed-off-by: Peter Barada <peter.barada at logicpd.com>
> Cc: Tom Rini <tom.rini at gmail.com>
> Cc: Igor Grinberg <grinberg at compulab.co.il>
...
>  board/logicpd/omap3som/Makefile     |   42 +++
>  board/logicpd/omap3som/omap3logic.c |  523 +++++++++++++++++++++++++++++++++++
>  board/logicpd/omap3som/omap3logic.h |   35 +++
>  boards.cfg                          |    1 +
>  include/configs/omap3_logic.h       |  351 +++++++++++++++++++++++
>  5 files changed, 952 insertions(+), 0 deletions(-)
>  create mode 100644 board/logicpd/omap3som/Makefile
>  create mode 100644 board/logicpd/omap3som/omap3logic.c
>  create mode 100644 board/logicpd/omap3som/omap3logic.h
>  create mode 100644 include/configs/omap3_logic.h

Entry to MAINTAINERS missing.

> +/* two dimensional array of strucures containining board name and Linux
> + * machine IDs; row it selected based on CPU column is slected based
> + * on hsusb0_data5 pin having a pulldown resistor */

Incorrect multiline comment style.  Please fix globally.


> +#ifdef CONFIG_SMC911X
> +/* GPMC CS1 settings for Logic SOM LV/Torpedo LAN92xx Ethernet chip */
> +static const u32 gpmc_lan92xx_config[] = {
> +	0x00001000,
> +	0x00080801,
> +	0x00000000,
> +	0x08010801,
> +	0x00080a0a,
> +	0x03000280,
> +};

And what exactly is the meaning of these magic numbers?


> +#define CONFIG_HARD_I2C			1
> +#define CONFIG_SYS_I2C_SPEED		100000
> +#define CONFIG_SYS_I2C_SLAVE		1
> +#define CONFIG_SYS_I2C_BUS		0
> +#define CONFIG_SYS_I2C_BUS_SELECT	1
> +#define CONFIG_I2C_MULTI_BUS		1
> +#define CONFIG_DRIVER_OMAP34XX_I2C	1

Please do not define values for macros that only switch on a feature.

...
> +#define CONFIG_PREBOOT \
> +	"echo ======================NOTICE============================;"\
> +	"echo The u-boot environment is not set. - You are;"            \
> +	"echo required to set a valid display for your LCD panel.;"	\
> +	"echo Valid display options are:;"				\
> +	"echo \"  2 == LQ121S1DG31     TFT SVGA    (12.1)  Sharp\";"	\
> +	"echo \"  3 == LQ036Q1DA01     TFT QVGA    (3.6)   Sharp w/ASIC\";" \
> +	"echo \"  5 == LQ064D343       TFT VGA     (6.4)   Sharp\";"	\
> +	"echo \"  7 == LQ10D368        TFT VGA     (10.4)  Sharp\";"	\
> +	"echo \" 15 == LQ043T1DG01     TFT WQVGA   (4.3)   Sharp (DEFAULT)\";" \
> +	"echo \" vga[-16 OR -24]       LCD VGA     640x480\";"          \
> +	"echo \" svga[-16 OR -24]      LCD SVGA    800x600\";"          \
> +	"echo \" xga[-16 OR -24]       LCD XGA     1024x768\";"         \
> +	"echo \" 720p[-16 OR -24]      LCD 720P    1280x720\";"         \
> +	"echo \" sxga[-16 OR -24]      LCD SXGA    1280x1024\";"        \
> +	"echo \" uxga[-16 OR -24]      LCD UXGA    1600x1200\";"        \
> +	"echo MAKE SURE YOUR DISPLAY VARIABLE IS CORRECTLY ENTERED!;"	\
> +	"setenv display 15;"						\

Strange.  First you ask the user to set a valid display type, and then
you hard-set it to some predefined value.  What's the reationale
behind that?

> +	"setenv preboot;"						\
> +	"saveenv;"

Would it not be better to erase the preboot setting only after some
display type definition was entered and saved _by_the_user_ ?

> +/*-----------------------------------------------------------------------
> + * Physical Memory Map
> + */

Incorrect multiline comment style. 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
If all the Chinese simultaneously jumped into the Pacific  off  a  10
foot platform erected 10 feet off their coast, it would cause a tidal
wave that would destroy everything in this country west of Nebraska.