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

Wolfgang Denk
2011-12-17T20:44:19Z

Dear Peter Barada,

In message <1324067511-14142-1-git-send-email-peter.barada@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@logicpd.com>
> Cc: Tom Rini <tom.rini@gmail.com>
> Cc: Igor Grinberg <grinberg@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.