[U-Boot] [PATCH 2/2] pcm051: Add support for Phytec phyCORE-AM335x

[Wolfgang Denk <wd@denx.de>]

Dear Lars,

In message <1357663926-15937-2-git-send-email-larsi@wh2.tu-dresden.de> you wrote:
...
>  arch/arm/include/asm/arch-am33xx/ddr_defs.h |   18 ++
>  board/phytec/pcm051/Makefile                |   46 ++++
>  board/phytec/pcm051/board.c                 |  271 +++++++++++++++++++++++
>  board/phytec/pcm051/board.h                 |   33 +++
>  board/phytec/pcm051/mux.c                   |  133 ++++++++++++
>  boards.cfg                                  |    1 +
>  include/configs/pcm051.h                    |  308 +++++++++++++++++++++++++++

Please add an entry to the MAINTAINERS file.

Also, please run your patch through checkpatch - it complains about a
number of style errors:

	WARNING: Whitespace before semicolon

> +/* DDR RAM defines */
> +#define DDR_CLK_MHZ		303

Is this really correct?  303 ??

> +static void rtc32k_enable(void)
> +{
> +	struct rtc_regs *rtc = (struct rtc_regs *)AM335X_RTC_BASE;
> +
> +	/*
> +	 * Unlock the RTC's registers.  For more details please see the
> +	 * RTC_SS section of the TRM.  In order to unlock we need to
> +	 * write these specific values (keys) in this order.
> +	 */
> +	writel(0x83e70b13, &rtc->kick0r);
> +	writel(0x95a4f1e0, &rtc->kick1r);

These magic numbers should probbly be moved to some header file ?

> +void s_init(void)
> +{
> +	/* WDT1 is already running when the bootloader gets control
> +	 * Disable it to avoid "random" resets
> +	 */

Incorrect multiline comment style.

> +	writel(0xAAAA, &wdtimer->wdtwspr);

More hard-wired magic values?

> +	while (readl(&wdtimer->wdtwwps) != 0x0)
> +		;

No timeout?

> +	writel(0x5555, &wdtimer->wdtwspr);

More hard-wired magic values?

> +	while (readl(&wdtimer->wdtwwps) != 0x0)
> +		;

No timeout?

> +	while ((readl(&uart_base->uartsyssts) &
> +		UART_CLK_RUNNING_MASK) != UART_CLK_RUNNING_MASK)
> +		;

Braces needed for multiline statement.

> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> +	return 0;
> +}
> +#endif

Do you plan to use this?  Otherwise please just omit such dead code.

> +#ifdef CONFIG_DRIVER_TI_CPSW
> +static void cpsw_control(int enabled)
> +{
> +	/* VTP can be added here */
> +
> +	return;
> +}

Ditto...

> +	if (!eth_getenv_enetaddr("ethaddr", mac_addr)) {
> +		debug("<ethaddr> not set. Reading from E-fuse\n");

This should be a printf().  Any such automatic changes are always
worth to be told explicitly.

> +			goto try_usbether;
...
> +#endif
> +try_usbether:

Did you ever try building without CONFIG_DRIVER_TI_CPSW set?  I bet
this causes a few compiler warnings. [Hint: You define a label, but
don't use it anywhere.]

> +#define CONFIG_ENV_SIZE			(128 << 10)	/* 128 KiB */

Does this really make sense?  I doubt you will ever need more than 10%
of this size - so you are just wasting a lot of time for checksumming
unused memory...

> +#define CONFIG_ENV_OVERWRITE		1

Please do not define values for logical variables like this one;
please fix globally.

> +#define CONFIG_ENV_IS_NOWHERE

Really?


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
Crash programs fail because they are based on the theory  that,  with
nine women pregnant, you can get a baby a month.  - Wernher von Braun