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

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