[U-Boot] [PATCH] AT91: Added support for taskit Stamp9G20 and PortuxG20

[Wolfgang Denk]

Dear Achim Ehrlich,

In message <4B7C14EC.7050209 at taskit.de> you wrote:
> Signed-off-by: Achim Ehrlich <aehrlich at taskit.de>
> ---
...
> --- a/Makefile
> +++ b/Makefile
> @@ -2906,6 +2906,15 @@ TNY_A9260_config	:	unconfig
>  	@echo "#define CONFIG_$(@:_config=) 1" >$(obj)include/config.h
>  	@$(MKCONFIG) -a tny_a9260 arm arm926ejs tny_a9260 calao at91
>  
> +portuxG20_config \
> +stamp9G20_config	:	unconfig
> +	@mkdir -p $(obj)include
> +	@if [ "$(findstring portux,$@)" ] ; then \
> +		echo "#define CONFIG_PORTUXG20 1" >$(obj)include/config.h; \
> +		$(XECHO) "... PortuxG20";\
> +	fi;

NAK. We don't accept such scripting in the Makefile any more. Please
use the available mkconfig features instead.


> diff --git a/board/taskit/stamp9G20/partition.c b/board/taskit/stamp9G20/partition.c
> new file mode 100644
> index 0000000..2629c67
> --- /dev/null
> +++ b/board/taskit/stamp9G20/partition.c

So we are adding yet another copy of this file. We already have 12 of
these. Mostly identical.

Isn't there a more intelligent way to handle this?

...
> +int dram_init(void)
> +{
> +	gd->bd->bi_dram[0].start = PHYS_SDRAM;
> +	gd->bd->bi_dram[0].size = PHYS_SDRAM_SIZE;
> +	return 0;
> +}

Please consider using get_ram_size() for memory auto-sizing and
testing.

> +#ifdef CONFIG_RESET_PHY_R
> +void reset_phy(void)
> +{
> +#ifdef CONFIG_MACB


CONFIG_MACB seems to be an undocumented variable. What is it supposed
to do?


> diff --git a/include/configs/stamp9G20.h b/include/configs/stamp9G20.h
> new file mode 100644
> index 0000000..5008554
> --- /dev/null
> +++ b/include/configs/stamp9G20.h
...
> +/* ARM asynchronous clock */
> +#define AT91_MAIN_CLOCK		18432000	/* 18.432 MHz crystal */

This should be changed into a CONFIG_SYS_ (globally).


> +#define AT91_CPU_NAME		"AT91SAM9G20"

Ditto.

> +#define CONFIG_ATMEL_USART	1
> +#undef CONFIG_USART0
> +#undef CONFIG_USART1
> +#undef CONFIG_USART2
> +#define CONFIG_USART3		1	/* USART 3 is DBGU */

Do not undefine what is not defined anyway.

> +/* LED */
> +
> +#undef CONFIG_AT91_LED

Ditto.

> +/*
> + * Command line configuration.
> + */
> +#include <config_cmd_default.h>
> +#undef CONFIG_CMD_BDI
> +#undef CONFIG_CMD_FPGA
> +#undef CONFIG_CMD_IMI
> +#undef CONFIG_CMD_IMLS
> +#undef CONFIG_CMD_SOURCE

Is there any specific reason for disabling the "bdi", "imi", "imls"
and "source" commands? They are pretty useful, and you don't seem to
be especially short on resources...

...
> +/* USB */
> +#define CONFIG_USB_ATMEL
> +#define CONFIG_USB_OHCI_NEW		1
> +#define CONFIG_DOS_PARTITION		1
> +#define CONFIG_SYS_USB_OHCI_CPU_INIT		1
> +#define CONFIG_SYS_USB_OHCI_REGS_BASE		0x00500000	/* AT91SAM9260_UHP_BASE */

Line too long. Please fix globally.

...
> +	"basicargs=console=ttyS0,115200 mem=64M\0"			\
> +	""
> +
> +
> +

Excessive white space. Drop 3 of the empty lines.

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
"Do we define evil as the absence of goodness? It seems only  logical
that shit happens--we discover this by the process of elimination."
                                                        -- Larry Wall