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

[Achim Ehrlich <aehrlich@taskit.de>]

Dear Wolfgang Denk,

> 
> In message <4B7C14EC.7050209@taskit.de> you wrote:
>> Signed-off-by: Achim Ehrlich <aehrlich@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.
>

Sorry, I overlooked that feature and will change that.
 
> 
>> 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?
>

The file has not much content anyway. I can get rid of it completely.
 
> ...
>> +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.

Thanks for the hint, i will test it.

> 
>> +#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?
>

It switches on the pin initialization for the Ethernet MAC.
Actually, i just copied this part from the AT91SAM9260ek board code.
 
> 
>> 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).
>

Do you mean, I should change that for all AT91-Boards in u-boot?
How or who is going to test that?

 
> 
>> +#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.

I will fix these as well.

Thank you for your corrections.

Kind regards


Achim

-- 
product manager

email:aehrlich at taskit.de 
Tel.: ++49 30 611295-25
Fax: ++49 30 611295-11

--
taskit GmbH
Seelenbinderstr. 33 | D-12555 Berlin
web:http://www.taskit.de
Amtsgericht Charlottenburg: 93HRB39014
Managing director: Thorsten Raulfs
--