From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 18 Aug 2015 14:09:50 +0200 Subject: [U-Boot] [PATCH] arm, at91: add axm extensions In-Reply-To: <55D2F694.3040307@gmail.com> References: <1434370913-8328-1-git-send-email-hs@denx.de> <55D2F694.3040307@gmail.com> Message-ID: <55D3208E.10400@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 >> --- >> >> 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