[U-Boot] [PATCH 2/2] TT-01: add basic board support for HALE TT-01

Wolfgang Denk
2011-09-22T14:08:33Z

Dear Helmut Raiger,

In message <1316693575-20726-3-git-send-email-helmut.raiger@hale.at> you wrote:
> This adds basic board support for TT-01 based on
> the bluetechnix i.MX31 SOM. Currently only NOR-Flash
> boot is supported.
>
> Signed-off-by: Helmut Raiger <helmut.raiger@hale.at>
> ---
> Makefile | 4 +
> board/hale/tt01/Makefile | 53 +++++++++
> board/hale/tt01/config.mk | 1 +
> board/hale/tt01/tt01.c | 98 ++++++++++++++++
> include/configs/tt01.h | 283 +++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 439 insertions(+), 0 deletions(-)
> create mode 100644 board/hale/tt01/Makefile
> create mode 100644 board/hale/tt01/config.mk
> create mode 100644 board/hale/tt01/tt01.c
> create mode 100644 include/configs/tt01.h

Entry to MAINTAINERS missing.

> diff --git a/Makefile b/Makefile
> index e9ba6a4..aaed9e6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -908,6 +908,10 @@ mx31pdk_nand_config : unconfig
> fi
> @$(MKCONFIG) -n $@ -a mx31pdk arm arm1136 mx31pdk freescale mx31
>
> +tt01_config : unconfig
> + @mkdir -p $(obj)include
> + @$(MKCONFIG) -n $@ -a tt01 arm arm1136 tt01 hale mx31

We don't add boards to Makefile any more. Please add instead to
boards.cfg

...
> + /* start CSPI3 clock (3 = always on except if PLL off) */
> + writel(readl(CCM_CGR0) | (3 << 16), CCM_CGR0);

Please use setbits() instead.

> +int board_early_init_f(void)
> +{
> + /* CS4: FPGA incl. network controller */
> + __REG(CSCR_U(4)) = 0x0000dcf6;
> + __REG(CSCR_L(4)) = 0x444A4541;
> + __REG(CSCR_A(4)) = 0x44443302;

We don't allow such accesses any more. Please use proper I/O
accessors instead.

And please introduce some symbolic constants for the magic numbers,
and add comments whatthese are.

> +int board_init(void)
> +{
> + /* board id for linux, not in mach-types.h yet */
> + gd->bd->bi_arch_number = 3726; /* MACH_TYPE_TT01 = 3726 */

NAK. Please use standard approach for handling the mach_id.

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

Incorrect multiline comment style. Please fix globally.

...
> +#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_FLASH_BASE
> +#define CONFIG_SYS_MONITOR_LEN (1024 * 1024)

Are you really, really sure about this? I doubt it...

> +#define CONFIG_ENV_IS_IN_FLASH 1

Please do not define values for macros that select features only.
Please fix globally.

> +#include <config_cmd_default.h>
> +
> +/* this is currently not supported, mxc_nand.c is too incomplete for it */
> +#undef CONFIG_CMD_MTDPARTS

Please do not undef what is not defined in the first place.

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 go slow, be gentle. It's no one-way street -- you know how you
feel and that's all. It's how the girl feels too. Don't press. If the
girl feels anything for you at all, you'll know.
-- Kirk, "Charlie X", stardate 1535.8