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

Andreas Bießmann andreas.devel at googlemail.com
Tue Aug 18 11:10:44 CEST 2015


Hi Heiko,

sorry for the late reply!

This patch does not cleanly apply ... and some comments below follow.

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 at 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?

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

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

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

check for recovery mode?

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

>  	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) {

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

> +
> +	/* 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?

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

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

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

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

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

> +
> +	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"); ?

> +
> +	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[]);

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

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



More information about the U-Boot mailing list