[U-Boot] [PATCH] arm, at91: add axm extensions

[Heiko Schocher <hs@denx.de>]

Hello Andreas,

Am 18.08.2015 um 11:10 schrieb Andreas Bie?mann:
> Hi Heiko,
>
> sorry for the late reply!
>
> This patch does not cleanly apply ... and some comments below follow.

I have rebased version against current head, so I can easy
repost this (and the taurus patch). I work in your comments
ASAP and post them, thanks!

Done .. testing them on the boards ...

> On 06/15/2015 02:21 PM, Heiko Schocher wrote:
>> add extensions for the axm board:
>> - power on LED on power up
>> - press both recovery buttons on power up to enter
>>    recovery mode
>> - detect 64 MiB and 128 MiB ramsize
>> - PHY rest at reboot because of ATMEL bug
>> - use siemens update concept
>> - add axm default environment
>> - set CONFIG_SPL_MAX_SIZE to 15k
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>
>>   board/siemens/taurus/taurus.c | 232 +++++++++++++++++++++++++++++++++++++++---
>>   include/configs/taurus.h      |  68 ++++++++++++-
>>   2 files changed, 284 insertions(+), 16 deletions(-)
>>
>> diff --git a/board/siemens/taurus/taurus.c b/board/siemens/taurus/taurus.c
>> index 013dac2..554a0c2 100644
>> --- a/board/siemens/taurus/taurus.c
>> +++ b/board/siemens/taurus/taurus.c
>> @@ -84,11 +84,33 @@ void at91_spl_board_init(void)
>>   	taurus_nand_hw_init();
>>   	at91_spi0_hw_init(TAURUS_SPI_MASK);
>>
>> +#if defined(CONFIG_BOARD_AXM)
>> +		/* Configure LED PINs */
>
> indention?

removed.

>
>> +	at91_set_gpio_output(AT91_PIN_PA6, 0);
>> +	at91_set_gpio_output(AT91_PIN_PA8, 0);
>> +	at91_set_gpio_output(AT91_PIN_PA9, 0);
>> +	at91_set_gpio_output(AT91_PIN_PA10, 0);
>> +	at91_set_gpio_output(AT91_PIN_PA11, 0);
>> +	at91_set_gpio_output(AT91_PIN_PA12, 0);
>> +#endif
>> +
>> +#if defined(CONFIG_BOARD_AXM)
>>   	/* Configure recovery button PINs */
>
> I would have placed the comment above the if-def

done.

>
>> +	at91_set_gpio_input(AT91_PIN_PA26, 1);
>> +	at91_set_gpio_input(AT91_PIN_PA27, 1);
>> +#endif
>> +#if defined(CONFIG_BOARD_TAURUS)
>
> I prefer the #elif statement here, isn't AXM and TAURUS mutually exclusive?

Yes, fixed.

>
>>   	at91_set_gpio_input(AT91_PIN_PA31, 1);
>> +#endif
>>
>> -	/* check if button is pressed */
>> +	/* check if both  button is pressed */
>
> check for recovery mode?

fixed.

>
>> +#if defined(CONFIG_BOARD_AXM)
>> +	if ((at91_get_gpio_value(AT91_PIN_PA26) == 0) &&
>> +	    (at91_get_gpio_value(AT91_PIN_PA27) == 0)) {
>> +#endif
>> +#if defined(CONFIG_BOARD_TAURUS)
>
> #elif?

yep, fixed.

>
>>   	if (at91_get_gpio_value(AT91_PIN_PA31) == 0) {
>> +#endif
>
> WARNING: suspect code indent for conditional statements (8, 8)
> #59: FILE: board/siemens/taurus/taurus.c:108:
> +	if ((at91_get_gpio_value(AT91_PIN_PA26) == 0) &&
> [...]
>   	if (at91_get_gpio_value(AT91_PIN_PA31) == 0) {
>

fixed.

>>   		struct spi_flash *flash;
>>
>>   		debug("Recovery button pressed\n");
>> @@ -108,35 +130,72 @@ void at91_spl_board_init(void)
>>   	}
>>   }
>>
>> -void mem_init(void)
>> +#define SDRAM_BASE_CONF	(AT91_SDRAMC_NR_13 | AT91_SDRAMC_CAS_3 \
>> +			 |AT91_SDRAMC_NB_4 | AT91_SDRAMC_DBW_32 \
>> +			 | AT91_SDRAMC_TWR_VAL(3) | AT91_SDRAMC_TRC_VAL(9) \
>> +			 | AT91_SDRAMC_TRP_VAL(3) | AT91_SDRAMC_TRCD_VAL(3) \
>> +			 | AT91_SDRAMC_TRAS_VAL(6) | AT91_SDRAMC_TXSR_VAL(10))
>> +
>> +void sdramc_configure(unsigned int mask)
>>   {
>>   	struct at91_matrix *ma = (struct at91_matrix *)ATMEL_BASE_MATRIX;
>>   	struct sdramc_reg setting;
>>
>>   	at91_sdram_hw_init();
>> -	setting.cr = (AT91_SDRAMC_NC_9 |
>> -		      AT91_SDRAMC_NR_13 |
>> -		      AT91_SDRAMC_CAS_3 |
>> -		      AT91_SDRAMC_NB_4 |
>> -		      AT91_SDRAMC_DBW_32 |
>> -		      AT91_SDRAMC_TWR_VAL(3) |
>> -		      AT91_SDRAMC_TRC_VAL(9) |
>> -		      AT91_SDRAMC_TRP_VAL(3) |
>> -		      AT91_SDRAMC_TRCD_VAL(3) |
>> -		      AT91_SDRAMC_TRAS_VAL(6) |
>> -		      AT91_SDRAMC_TXSR_VAL(10));
>> +	setting.cr = SDRAM_BASE_CONF | mask;
>>   	setting.mdr = AT91_SDRAMC_MD_SDRAM;
>>   	setting.tr = (CONFIG_SYS_MASTER_CLOCK * 7) / 1000000;
>>
>> -
>>   	writel(readl(&ma->ebicsa) | AT91_MATRIX_CS1A_SDRAMC |
>>   		AT91_MATRIX_VDDIOMSEL_3_3V | AT91_MATRIX_EBI_IOSR_SEL,
>>   		&ma->ebicsa);
>> +
>>   	sdramc_initialize(ATMEL_BASE_CS1, &setting);
>>   }
>> +
>> +void mem_init(void)
>> +{
>> +	unsigned int ram_size = 0;
>> +
>> +	/* Configure SDRAM for 128MB */
>> +	sdramc_configure(AT91_SDRAMC_NC_10);
>
> I wonder why it was NC_9 before but still 128MiB?

Good question, can;t remember ...

>
>> +
>> +	/* Do memtest for 128MB */
>> +	ram_size = get_ram_size((void *)CONFIG_SYS_SDRAM_BASE,
>> +				CONFIG_SYS_SDRAM_SIZE);
>> +
>> +	/*
>> +	 * If 32MB or 16MB should be supported check also for
>> +	 * expected mirroring at A16 and A17
>> +	 * To find mirror addresses depends how the collumns are connected
>> +	 * at RAM (internaly or externaly)
>> +	 * If the collumns are not in inverted order the mirror size effect
>> +	 * behaves like normal SRAM with A0,A1,A2,etc. connected incremantal
>> +	 */
>> +
>> +	/* Mirrors at A15 on ATMEL G20 SDRAM Controller with 64MB*/
>> +	if (ram_size == 0x800) {
>> +		printf("\n\r 64MB");
>> +		sdramc_configure(AT91_SDRAMC_NC_9);
>> +	} else {
>> +		/* Size already initialized */
>> +		printf("\n\r 128MB");
>> +	}
>> +}
>>   #endif
>>
>>   #ifdef CONFIG_MACB
>> +static void siemens_phy_reset(void)
>> +{
>> +	/*
>> +	 * we need to reset PHY for 200us
>> +	 * because of bug in ATMEL G20 CPU (undefined initial state of GPIO)
>> +	 */
>> +	if ((readl(AT91_ASM_RSTC_SR) & AT91_RSTC_RSTTYP) ==
>> +	    AT91_RSTC_RSTTYP_GENERAL)
>> +		at91_set_gpio_value(AT91_PIN_PA25, 0); /* reset eth switch */
>> +}
>> +
>>   static void taurus_macb_hw_init(void)
>>   {
>>   	/* Enable EMAC clock */
>> @@ -160,6 +219,8 @@ static void taurus_macb_hw_init(void)
>>   	at91_set_pio_pullup(AT91_PIO_PORTA, 26, 0);
>>   	at91_set_pio_pullup(AT91_PIO_PORTA, 28, 0);
>>
>> +	siemens_phy_reset();
>> +
>>   	at91_phy_reset();
>>
>>   	at91_set_gpio_input(AT91_PIN_PA25, 1);   /* ERST tri-state */
>> @@ -244,3 +305,146 @@ int board_eth_init(bd_t *bis)
>>   #endif
>>   	return rc;
>>   }
>> +
>> +#if !defined(CONFIG_SPL_BUILD)
>> +#if defined(CONFIG_BOARD_AXM)
>> +/*
>> + * Booting the Fallback Image.
>> + *
>> + *  The function is used to provide and
>> + *  boot the image with the fallback
>> + *  parameters, incase if the faulty image
>> + *  is upgraded over the base firmware.
>> + *
>> + */
>> +void upgrade_failure_fallback(void)
>> +{
>> +	unsigned long upgrade_available = 0;
>> +	unsigned long boot_retry = 0;
>> +	char boot_buf[10];
>> +	char upgrade_buf[3];
>> +	char *partitionset_active = NULL;
>> +	char  alpha[2] = {'A', 'B'};
>> +	char *rootfs = NULL;
>> +	char *rootfs_fallback = NULL;
>> +	unsigned long kernel_off = 0;
>> +	unsigned long kernel_off_fallback = 0;
>> +	unsigned long kernel_size = 0;
>> +	unsigned long kernel_size_fallback = 0;
>> +	char temp_buf[100];
>> +	char temp_kernsze[100];
>> +	char temp_kernoff[100];
>> +	char *rootfs_buf = NULL;
>> +	char *rootfs_fall_buf = NULL;
>
> Why have some buffers pre-allocated and some malloc'ed?

Hmm... I must look deeper into this.

>> +	char store_buff[5];
>> +	char store_buff_new[5];
>> +	char kern_off[100];
>> +	char kern_off_buff[100];
>> +	char kern_size[100];
>> +	char kern_size_buf[100];
>> +
>> +	partitionset_active = getenv("partitionset_active");
>> +	sprintf(store_buff, "%c", alpha[0]);
>> +	sprintf(store_buff_new, "%c", alpha[1]);
>> +
>> +	if (partitionset_active == store_buff)
>
> This is a pointer comparision ... it will always be false

good catch, reworked.

>
>> +		setenv("partitionset_active", store_buff_new);
>> +	else
>> +		setenv("partitionset_active", store_buff);
>
> How about:
>
> ---8<---
> partitionset_active = getenv("partitionset_active");
>
> if (partitionset_active) {
> 	if (partitionset_active[0] == 'A')
> 		setenv("partitionset_active", "B");
> 	else
> 		setenv("partitionset_active", "A");
> } // error handling
> --->8---

Yep.

>> +
>> +	rootfs = getenv("rootfs");
>> +	rootfs_fallback = getenv("rootfs_fallback");
>> +	rootfs_buf = malloc(strlen(rootfs) + 2);
>> +	rootfs_fall_buf = malloc(strlen(rootfs_fallback) + 2);
>> +
>> +	sprintf(rootfs_buf, "%s", rootfs);
>> +	sprintf(rootfs_fall_buf, "%s", rootfs_fallback);
>
> I wonder why you sprintf() the string into another buffer ...
>> +
>> +	strcpy(temp_buf, rootfs_buf);
>
> is temp_buf big enough?
>
>> +	strcpy(rootfs_buf, rootfs_fall_buf);
>> +	strcpy(rootfs_fall_buf, temp_buf);
>
> ... just to switch the content here ...
>
>> +
>> +	setenv("rootfs", rootfs_buf);
>> +	setenv("rootfs_fallback", rootfs_fall_buf);
>
> ... and set it again.
>
> Doesn't this work:
>
> ---8<---
> char *rootfs = NULL;
> char *rootfs_fallback = NULL;
>
> rootfs = getenv("rootfs");
> rootfs_fallback = getenv("rootfs_fallback");
>
> setenv("rootfs", rootfs_fallback);
> setenv("rootfs_fallback", rootfs);
> --->8---

I try this.

>> +
>> +	kernel_off = simple_strtoul(getenv("kernel_Off"), NULL, 16);
>> +	kernel_size = simple_strtoul(getenv("kernel_size"), NULL, 16);
>> +	kernel_off_fallback = simple_strtoul(getenv("kernel_Off_fallback"),
>> +					     NULL, 16);
>> +	kernel_size_fallback = simple_strtoul(getenv("kernel_size_fallback"),
>> +						     NULL, 16);
>> +
>> +	sprintf(kern_size_buf, "%lx", kernel_size_fallback);
>
> Shouldn't we print hex numbers with 0x prefixed?
>
>> +	sprintf(kern_size, "%lx", kernel_size);
>> +	strcpy(temp_kernsze, kern_size);
>
> I think one single temp_XX buffer should be enough here, what do you think?

Yep.

>> +	strcpy(kern_size, kern_size_buf);
>> +	strcpy(kern_size_buf, temp_kernsze);
>> +	setenv("kernel_size", kern_size);
>> +	setenv("kernel_size_fallback", kern_size_buf);
>
> I'm completely baffled here ... Is the kern_{size|Off} and the
> kern_{size|Off}_fallback switched here or not?

It should be, reworking ...

>> +
>> +	sprintf(kern_off_buff, "%lx", kernel_off_fallback);
>> +	sprintf(kern_off, "%lx", kernel_off);
>> +
>> +	strcpy(temp_kernoff, kern_off);
>> +	strcpy(kern_off, kern_off_buff);
>> +	strcpy(kern_off_buff, temp_kernoff);
>> +	setenv("kernel_Off", kern_off);
>> +	setenv("kernel_Off_fallback", kern_off_buff);
>> +
>> +	setenv("bootargs", '\0');
>> +
>> +	sprintf(upgrade_buf, "%lx", upgrade_available);
>> +	setenv("upgrade_available", upgrade_buf);
>
> setenv("upgrade_available", "0"); ?
>
>> +
>> +	sprintf(boot_buf, "%lx", boot_retry);
>> +	setenv("boot_retries", boot_buf);
>
> setenv("boot_retries", "0"); ?

fixed.

>> +
>> +	saveenv();
>> +
>> +	free(rootfs_buf);
>> +	free(rootfs_fall_buf);
>> +}
>> +
>> +extern int do_bootm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>
> please mention at least this warning
>
> WARNING: externs should be avoided in .c files
> #408: FILE: board/siemens/taurus/taurus.c:408:
> +extern int do_bootm(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[]);

Hups... reworking ... I think, I remove/rework the hole
upgrade mess ... sorry for this!

>> +
>> +static int do_upgrade_available(cmd_tbl_t *cmdtp, int flag, int argc,
>> +			char * const argv[])
>> +{
>> +	unsigned long upgrade_available = 0;
>> +	unsigned long boot_retry = 0;
>> +	char boot_buf[10];
>> +
>> +	upgrade_available = simple_strtoul(getenv("upgrade_available"), NULL,
>> +					   10);
>> +	if (upgrade_available) {
>> +		boot_retry = simple_strtoul(getenv("boot_retries"), NULL, 10);
>> +		boot_retry++;
>> +		sprintf(boot_buf, "%lx", boot_retry);
>> +		setenv("boot_retries", boot_buf);
>> +		saveenv();
>> +
>> +		/*
>> +		 * Here the boot_retries count is checked,and if the
>> +		 * count becomes greater than two,fallback function
>> +		 * is called to execute.When the fallback function
>> +		 * returns,bootm command is executed with null
>> +		 * parameters.In the fourth boot,the image with
>> +		 * fallback parameters is loaded onto the RAM.
>> +		 */
>> +
>> +		if (boot_retry > 2) {
>> +			upgrade_failure_fallback();
>> +			do_bootm(NULL, 0, 0, NULL);
>> +			return -1;
>> +		}
>> +	}
>> +	return 0;
>> +}
>> +
>> +U_BOOT_CMD(
>> +	upgrade_available,	1,	1,	do_upgrade_available,
>> +	"check Siemens update",
>> +	"no parameters"
>> +);
>> +#endif
>> +#endif
>> diff --git a/include/configs/taurus.h b/include/configs/taurus.h
>> index 2cf4558..6d18f2f 100644
>> --- a/include/configs/taurus.h
>> +++ b/include/configs/taurus.h
>> @@ -167,12 +167,75 @@
>>   #define CONFIG_ENV_OFFSET_REDUND	0x180000
>>   #define CONFIG_ENV_SIZE		0x20000		/* 1 sector = 128 kB */
>>   #define CONFIG_BOOTCOMMAND	"nand read 0x22000000 0x200000 0x300000; bootm"
>> -#define CONFIG_BOOTARGS							\
>> +
>> +#if defined(CONFIG_BOARD_TAURUS)
>> +#define	CONFIG_BOOTARGS_TAURUS						\
>>   	"console=ttyS0,115200 earlyprintk "				\
>>   	"mtdparts=atmel_nand:256k(bootstrap)ro,512k(uboot)ro,"		\
>>   	"256k(env),256k(env_redundant),256k(spare),"			\
>>   	"512k(dtb),6M(kernel)ro,-(rootfs) "				\
>>   	"root=/dev/mtdblock7 rw rootfstype=jffs2"
>> +#endif
>> +
>> +#if defined(CONFIG_BOARD_AXM)
>> +#define CONFIG_BOOTARGS_AXM						\
>> +	"\0"	\
>> +	"addip=setenv bootargs ${bootargs} ip=${ipaddr}:${serverip}:"	\
>> +	"${gatewayip}:${netmask}:${hostname}:${netdev}::off\0"		\
>> +	"addtest=setenv bootargs ${bootargs} loglevel=4 test\0"		\
>> +	"baudrate=115200\0"						\
>> +	"boot_file=setenv bootfile /${project_dir}/kernel/uImage\0"	\
>> +	"boot_retries=0\0"						\
>> +	"bootcmd=run flash_self\0"					\
>> +	"bootdelay=3\0"							\
>> +	"ethact=macb0\0"						\
>> +	"flash_nfs=run nand_kernel;run nfsargs;run addip;upgrade_available;"\
>> +	"bootm ${kernel_ram};reset\0"					\
>> +	"flash_self=run nand_kernel;run setbootargs;upgrade_available;" \
>> +	"bootm ${kernel_ram};reset\0"					\
>> +	"flash_self_test=run nand_kernel;run setbootargs addtest; "	\
>> +	"upgrade_available;bootm ${kernel_ram};reset\0"			\
>> +	"hostname=systemone\0"						\
>> +	"kernel_Off=0x00200000\0"					\
>> +	"kernel_Off_fallback=0x03800000\0"				\
>> +	"kernel_ram=0x21500000\0"					\
>> +	"kernel_size=0x00400000\0"					\
>> +	"kernel_size_fallback=0x00400000\0"				\
>> +	"loads_echo=1\0"						\
>> +	"nand_kernel=nand read.e ${kernel_ram} ${kernel_Off} "		\
>> +		"${kernel_size}\0"					\
>> +	"net_nfs=run boot_file;tftp ${kernel_ram} ${bootfile};"		\
>> +	"run nfsargs;run addip;upgrade_available;bootm "		\
>> +		"${kernel_ram};reset\0"					\
>> +	"netdev=eth0\0"							\
>> +	"nfsargs=run root_path;setenv bootargs ${bootargs} "		\
>> +	"root=/dev/nfs rw nfsroot=${serverip}:${rootpath} "		\
>> +	"at91sam9_wdt.wdt_timeout=16\0"					\
>> +	"partitionset_active=A\0"					\
>> +	"preboot=echo;echo Type 'run flash_self' to use kernel and root "\
>> +	"filesystem on memory;echo Type 'run flash_nfs' to use kernel "	\
>> +	"from memory and root filesystem over NFS;echo Type 'run net_nfs' "\
>> +	"to get Kernel over TFTP and mount root filesystem over NFS;echo\0"\
>> +	"project_dir=systemone\0"					\
>> +	"root_path=setenv rootpath /home/projects/${project_dir}/rootfs\0"\
>> +	"rootfs=/dev/mtdblock5\0"					\
>> +	"rootfs_fallback=/dev/mtdblock7\0"				\
>> +	"setbootargs=setenv bootargs ${bootargs} console=ttyMTD,mtdoops "\
>
>> +		"root=${rootfs} rootfstype=jffs2 panic=7 "		\
>> +		"at91sam9_wdt.wdt_timeout=16\0"				\
>> +	"stderr=serial\0"						\
>> +	"stdin=serial\0"						\
>> +	"stdout=serial\0"						\
>> +	"upgrade_available=0\0"
>> +#endif
>> +
>> +#if defined(CONFIG_BOARD_TAURUS)
>> +#define CONFIG_BOOTARGS		CONFIG_BOOTARGS_TAURUS
>> +#endif
>> +
>> +#if defined(CONFIG_BOARD_AXM)
>> +#define CONFIG_BOOTARGS		CONFIG_BOOTARGS_AXM
>> +#endif
>>
>>   #define CONFIG_SYS_PROMPT		"U-Boot> "
>>   #define CONFIG_SYS_CBSIZE		256
>> @@ -192,7 +255,7 @@
>>   /* Defines for SPL */
>>   #define CONFIG_SPL_FRAMEWORK
>>   #define CONFIG_SPL_TEXT_BASE		0x0
>> -#define CONFIG_SPL_MAX_SIZE		(14 * 1024)
>> +#define CONFIG_SPL_MAX_SIZE		(15 * 1024)
>>   #define CONFIG_SPL_STACK		(16 * 1024)
>
> Which processor is this? 9g20 has two 16k SRAM, is the stack placed
> proberly?

Its a 9g20 .. good hint ... I try to use 0x304000 for stack.

bye,
Heiko
>
>>   #define CONFIG_SYS_SPL_MALLOC_START     (CONFIG_SYS_TEXT_BASE - \
>>   					CONFIG_SYS_MALLOC_LEN)
>> @@ -242,4 +305,5 @@
>>   #define CONFIG_SYS_MCKR			0x1300
>>   #define CONFIG_SYS_MCKR_CSS		(0x02 | CONFIG_SYS_MCKR)
>>   #define CONFIG_SYS_AT91_PLLB		0x10193F05
>> +
>>   #endif
>>
>
> best regards
>
> Andreas
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany