[{"id":1766134,"web_url":"http://patchwork.ozlabs.org/comment/1766134/","msgid":"","list_archive_url":null,"date":"2017-09-11T08:51:31","subject":"Re: [U-Boot] [PATCH] rockchip: add support for backing to bootrom\n\tdownload mode","submitter":{"id":64532,"url":"http://patchwork.ozlabs.org/api/people/64532/","name":"Kever Yang","email":"kever.yang@rock-chips.com"},"content":"Hi Andy,\n\n\nOn 09/11/2017 02:27 PM, Andy Yan wrote:\n> Rockchip bootrom will enter download mode if it returns from\n> spl/tpl with a none-zero value and couldn't find a valid image\n> in the backup partition.\n> This patch provide a method to instruct the system to back to\n> bootrom download mode by checking the BROM_DOWNLOAD_FLAG register.\n> As the bootrom download function relys on some modules such as\n> interrupts, so we need to back to bootrom as early as possbile\n> before the tpl/tps code override the interrupt settings.\n>\n> Signed-off-by: Andy Yan \n\nIt looks good to me,\nReviewed-by: Kever Yang \n\nThanks,\n- Kever\n> ---\n>\n> arch/arm/include/asm/arch-rockchip/bootrom.h | 2 +-\n> arch/arm/mach-rockchip/Kconfig | 13 +++++++\n> arch/arm/mach-rockchip/bootrom.c | 2 +-\n> arch/arm/mach-rockchip/save_boot_param.S | 57 +++++++++++++++++++++++-----\n> 4 files changed, 63 insertions(+), 11 deletions(-)\n>\n> diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h\n> index 92eb878..6ae3e94 100644\n> --- a/arch/arm/include/asm/arch-rockchip/bootrom.h\n> +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h\n> @@ -22,6 +22,6 @@ void back_to_bootrom(void);\n> /**\n> * Assembler component for the above (do not call this directly)\n> */\n> -void _back_to_bootrom_s(void);\n> +void _back_to_bootrom_s(int mode);\n> \n> #endif\n> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig\n> index d9b25d5..3ab0c30 100644\n> --- a/arch/arm/mach-rockchip/Kconfig\n> +++ b/arch/arm/mach-rockchip/Kconfig\n> @@ -113,6 +113,7 @@ config ROCKCHIP_RK3399\n> \tselect SPL_SERIAL_SUPPORT\n> \tselect SPL_DRIVERS_MISC_SUPPORT\n> \tselect ENABLE_ARM_SOC_BOOT0_HOOK\n> +\tselect ROCKCHIP_BROM_HELPER\n> \tselect DEBUG_UART_BOARD_INIT\n> \thelp\n> \t The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72\n> @@ -149,6 +150,18 @@ config TPL_ROCKCHIP_BACK_TO_BROM\n> SPL will return to the boot rom, which will then load the U-Boot\n> binary to keep going on.\n> \n> +config ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\thex \"Bootrom download mode flag register address\"\n> +\tdefault 0x200081c8 if ROCKCHIP_RK3036\n> +\tdefault 0xff730094 if ROCKCHIP_RK3288\n> +\tdefault 0xff738200 if ROCKCHIP_RK3368\n> +\tdefault 0xff320300 if ROCKCHIP_RK3399\n> +\tdefault 0x10300580 if ROCKCHIP_RV1108\n> +\tdefault 0x00\n> +\thelp\n> +\t The Soc will return to bootrom download mode if this register set\n> +\t to BOOTROM_DOWNLOAD_FLAG.\n> +\n> config ROCKCHIP_SPL_RESERVE_IRAM\n> \thex \"Size of IRAM reserved in SPL\"\n> \tdefault 0x4000\n> diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c\n> index 8380e4e..6f0d583 100644\n> --- a/arch/arm/mach-rockchip/bootrom.c\n> +++ b/arch/arm/mach-rockchip/bootrom.c\n> @@ -12,5 +12,5 @@ void back_to_bootrom(void)\n> #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)\n> \tputs(\"Returning to boot ROM...\\n\");\n> #endif\n> -\t_back_to_bootrom_s();\n> +\t_back_to_bootrom_s(0);\n> }\n> diff --git a/arch/arm/mach-rockchip/save_boot_param.S b/arch/arm/mach-rockchip/save_boot_param.S\n> index 50fce20..f1bed0b 100644\n> --- a/arch/arm/mach-rockchip/save_boot_param.S\n> +++ b/arch/arm/mach-rockchip/save_boot_param.S\n> @@ -7,11 +7,25 @@\n> \n> #include \n> \n> +#define BACK_TO_BROM_DOWNLOAD_FLAG 0xEF08A53C\n> +\n> #if defined(CONFIG_ARM64)\n> .globl\tSAVE_SP_ADDR\n> SAVE_SP_ADDR:\n> \t.quad 0\n> \n> +ENTRY(check_back_to_brom_dnl_flag)\n> +\tldr\tx8, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tldr\tx9, [x8]\n> +\tldr\tx0, =BACK_TO_BROM_DOWNLOAD_FLAG\n> +\tcmp\tx9, x0\n> +\tb.ne\tsave_boot_params_ret\n> +\tmov\tx9, xzr\n> +\tstr\tx9, [x8]\t/* clear flag */\n> +\tmov\tx0, #1 /* indicate the bootrom to enter download mode */\n> +\tb\t_back_to_bootrom_s\n> +ENDPROC(check_back_to_brom_dnl_flag)\n> +\n> ENTRY(save_boot_params)\n> \tsub\tsp, sp, #0x60\n> \tstp\tx29, x30, [sp, #0x50]\n> @@ -23,14 +37,22 @@ ENTRY(save_boot_params)\n> \tldr\tx8, =SAVE_SP_ADDR\n> \tmov\tx9, sp\n> \tstr\tx9, [x8]\n> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tb\tcheck_back_to_brom_dnl_flag\n> +#else\n> \tb\tsave_boot_params_ret /* back to my caller */\n> +#endif\n> ENDPROC(save_boot_params)\n> \n> +/*\n> + * x0: return value for bootrom, none-zero for bootrom download\n> + * mode and zero for normal boot mode\n> + */\n> .globl _back_to_bootrom_s\n> ENTRY(_back_to_bootrom_s)\n> -\tldr\tx0, =SAVE_SP_ADDR\n> -\tldr\tx0, [x0]\n> -\tmov\tsp, x0\n> +\tldr\tx1, =SAVE_SP_ADDR\n> +\tldr\tx1, [x1]\n> +\tmov\tsp, x1\n> \tldp\tx29, x30, [sp, #0x50]\n> \tldp\tx27, x28, [sp, #0x40]\n> \tldp\tx25, x26, [sp, #0x30]\n> @@ -38,7 +60,6 @@ ENTRY(_back_to_bootrom_s)\n> \tldp\tx21, x22, [sp, #0x10]\n> \tldp\tx19, x20, [sp]\n> \tadd\tsp, sp, #0x60\n> -\tmov\tx0, xzr\n> \tret\n> ENDPROC(_back_to_bootrom_s)\n> #else\n> @@ -46,6 +67,18 @@ ENDPROC(_back_to_bootrom_s)\n> SAVE_SP_ADDR:\n> \t.word 0\n> \n> +ENTRY(check_back_to_brom_dnl_flag)\n> +\tldr\tr0, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tldr\tr1, [r0]\n> +\tldr\tr2, =BACK_TO_BROM_DOWNLOAD_FLAG\n> +\tcmp\tr1, r2\n> +\tbne\tsave_boot_params_ret\n> +\tmov\tr3, #0\n> +\tstr\tr3, [r0] @clear flag\n> +\tmov\tr0, #1 @indicate the bootrom to enter download mode\n> +\tb\t_back_to_bootrom_s\n> +ENDPROC(check_back_to_brom_dnl_flag)\n> +\n> /*\n> * void save_boot_params\n> *\n> @@ -55,15 +88,21 @@ ENTRY(save_boot_params)\n> \tpush\t{r1-r12, lr}\n> \tldr\tr0, =SAVE_SP_ADDR\n> \tstr\tsp, [r0]\n> -\tb\tsave_boot_params_ret\t\t@ back to my caller\n> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tb\tcheck_back_to_brom_dnl_flag\n> +#else\n> +\tb save_boot_params_ret\n> +#endif\n> ENDPROC(save_boot_params)\n> \n> -\n> +/*\n> + * r0: return value for bootrom, none-zero for bootrom download\n> + * mode and zero for normal boot mode\n> + */\n> .globl _back_to_bootrom_s\n> ENTRY(_back_to_bootrom_s)\n> -\tldr\tr0, =SAVE_SP_ADDR\n> -\tldr\tsp, [r0]\n> -\tmov\tr0, #0\n> +\tldr\tr1, =SAVE_SP_ADDR\n> +\tldr\tsp, [r1]\n> \tpop\t{r1-r12, pc}\n> ENDPROC(_back_to_bootrom_s)\n> #endif","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xrM9p0kJ6z9s7g\n\tfor ;\n\tMon, 11 Sep 2017 18:51:56 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 9AC44C21DEB; Mon, 11 Sep 2017 08:51:49 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 73339C21D57;\n\tMon, 11 Sep 2017 08:51:46 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 9F8C8C21D57; Mon, 11 Sep 2017 08:51:44 +0000 (UTC)","from regular1.263xmail.com (regular1.263xmail.com [211.150.99.138])\n\tby lists.denx.de (Postfix) with ESMTPS id 487ABC21CB6\n\tfor ; Mon, 11 Sep 2017 08:51:43 +0000 (UTC)","from kever.yang?rock-chips.com (unknown [192.168.165.105])\n\tby regular1.263xmail.com (Postfix) with ESMTP id 9E10779BB;\n\tMon, 11 Sep 2017 16:51:37 +0800 (CST)","from [192.168.60.65] (localhost [127.0.0.1])\n\tby smtp.263.net (Postfix) with ESMTPA id D945F3FB;\n\tMon, 11 Sep 2017 16:51:34 +0800 (CST)","from [192.168.60.65] (unknown [103.29.142.67])\n\tby smtp.263.net (Postfix) whith ESMTP id 21185ZJEYG4;\n\tMon, 11 Sep 2017 16:51:37 +0800 (CST)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H2 autolearn=unavailable autolearn_force=no\n\tversion=3.4.0","X-263anti-spam":"KSV:0;","X-MAIL-GRAY":"0","X-MAIL-DELIVERY":"1","X-KSVirus-check":"0","X-ABS-CHECKED":"4","X-RL-SENDER":"kever.yang@rock-chips.com","X-FST-TO":"u-boot@lists.denx.de","X-SENDER-IP":"103.29.142.67","X-LOGIN-NAME":"kever.yang@rock-chips.com","X-UNIQUE-TAG":"<76bc869dae769bb0e205cf8df71ec6e4>","X-ATTACHMENT-NUM":"0","X-SENDER":"yk@rock-chips.com","X-DNS-TYPE":"0","To":"Andy Yan ,\n\tphilipp.tomsich@theobroma-systems.com, sjg@chromium.org","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>","From":"Kever Yang ","Message-ID":"","Date":"Mon, 11 Sep 2017 16:51:31 +0800","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tThunderbird/45.8.0","MIME-Version":"1.0","In-Reply-To":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>","Cc":"u-boot@lists.denx.de","Subject":"Re: [U-Boot] [PATCH] rockchip: add support for backing to bootrom\n\tdownload mode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767204,"web_url":"http://patchwork.ozlabs.org/comment/1767204/","msgid":"","list_archive_url":null,"date":"2017-09-12T15:47:45","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"> Rockchip bootrom will enter download mode if it returns from\n> spl/tpl with a none-zero value and couldn't find a valid image\n> in the backup partition.\n> This patch provide a method to instruct the system to back to\n> bootrom download mode by checking the BROM_DOWNLOAD_FLAG register.\n> As the bootrom download function relys on some modules such as\n> interrupts, so we need to back to bootrom as early as possbile\n> before the tpl/tps code override the interrupt settings.\n> \n> Signed-off-by: Andy Yan \n> Reviewed-by: Kever Yang \n> ---\n> \n> arch/arm/include/asm/arch-rockchip/bootrom.h | 2 +-\n> arch/arm/mach-rockchip/Kconfig | 13 +++++++\n> arch/arm/mach-rockchip/bootrom.c | 2 +-\n> arch/arm/mach-rockchip/save_boot_param.S | 57 +++++++++++++++++++++++-----\n> 4 files changed, 63 insertions(+), 11 deletions(-)\n> \n\nAcked-by: Philipp Tomsich ","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xs8rP0dvsz9s7f\n\tfor ;\n\tWed, 13 Sep 2017 02:09:41 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid D5502C222F6; Tue, 12 Sep 2017 16:05:54 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id DCCDCC2227A;\n\tTue, 12 Sep 2017 16:04:49 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 8D160C222E4; Tue, 12 Sep 2017 16:03:44 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 01BA0C2224A\n\tfor ; Tue, 12 Sep 2017 16:03:42 +0000 (UTC)","from [86.59.122.178] (port=60150 helo=vpn-10-11-0-14.lan)\n\tby mail.theobroma-systems.com with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1drnPd-00083Q-Hp; Tue, 12 Sep 2017 17:47:45 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=none autolearn=unavailable\n\tautolearn_force=no version=3.4.0","MIME-Version":"1.0","From":"Philipp Tomsich ","To":"Andy Yan ","In-Reply-To":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>","Message-Id":"","Date":"Tue, 12 Sep 2017 17:47:45 +0200","Cc":"u-boot@lists.denx.de, Andy Yan ","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767297,"web_url":"http://patchwork.ozlabs.org/comment/1767297/","msgid":"","list_archive_url":null,"date":"2017-09-12T19:30:13","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"On Mon, 11 Sep 2017, Andy Yan wrote:\n\n> Rockchip bootrom will enter download mode if it returns from\n> spl/tpl with a none-zero value and couldn't find a valid image\n> in the backup partition.\n> This patch provide a method to instruct the system to back to\n> bootrom download mode by checking the BROM_DOWNLOAD_FLAG register.\n> As the bootrom download function relys on some modules such as\n> interrupts, so we need to back to bootrom as early as possbile\n> before the tpl/tps code override the interrupt settings.\n\nI was not aware that the TPL/SPL overrides interrupt settings. What \nexactly does this comment refer to?\n\n>\n> Signed-off-by: Andy Yan \n> Reviewed-by: Kever Yang \n> Acked-by: Philipp Tomsich \n> ---\n>\n> arch/arm/include/asm/arch-rockchip/bootrom.h | 2 +-\n> arch/arm/mach-rockchip/Kconfig | 13 +++++++\n> arch/arm/mach-rockchip/bootrom.c | 2 +-\n> arch/arm/mach-rockchip/save_boot_param.S | 57 +++++++++++++++++++++++-----\n> 4 files changed, 63 insertions(+), 11 deletions(-)\n>\n> diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h\n> index 92eb878..6ae3e94 100644\n> --- a/arch/arm/include/asm/arch-rockchip/bootrom.h\n> +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h\n> @@ -22,6 +22,6 @@ void back_to_bootrom(void);\n> /**\n> * Assembler component for the above (do not call this directly)\n> */\n> -void _back_to_bootrom_s(void);\n> +void _back_to_bootrom_s(int mode);\n\nPlease add documentation for externally visible functions.\n\n>\n> #endif\n> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig\n> index d9b25d5..3ab0c30 100644\n> --- a/arch/arm/mach-rockchip/Kconfig\n> +++ b/arch/arm/mach-rockchip/Kconfig\n> @@ -113,6 +113,7 @@ config ROCKCHIP_RK3399\n> \tselect SPL_SERIAL_SUPPORT\n> \tselect SPL_DRIVERS_MISC_SUPPORT\n> \tselect ENABLE_ARM_SOC_BOOT0_HOOK\n> +\tselect ROCKCHIP_BROM_HELPER\n> \tselect DEBUG_UART_BOARD_INIT\n> \thelp\n> \t The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72\n> @@ -149,6 +150,18 @@ config TPL_ROCKCHIP_BACK_TO_BROM\n> SPL will return to the boot rom, which will then load the U-Boot\n> binary to keep going on.\n>\n> +config ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\thex \"Bootrom download mode flag register address\"\n> +\tdefault 0x200081c8 if ROCKCHIP_RK3036\n> +\tdefault 0xff730094 if ROCKCHIP_RK3288\n> +\tdefault 0xff738200 if ROCKCHIP_RK3368\n> +\tdefault 0xff320300 if ROCKCHIP_RK3399\n> +\tdefault 0x10300580 if ROCKCHIP_RV1108\n> +\tdefault 0x00\n\nIf this is not user-configurable (i.e. if it is a per-chip constant), we \nshould define this in a header file. I would suggest to do the \ndetection/mapping either in include/asm/arch-rockchip/bootrom.h or in a \ncpu-specific header that gets included from there.\n\n> +\thelp\n> +\t The Soc will return to bootrom download mode if this register set\n> +\t to BOOTROM_DOWNLOAD_FLAG.\n\nDocumenting here what BOOTROM_DOWNLOAD_FLAG is or where it can be found \nwould be very helpful to anyone coming across this in the future.\n\n> +\n> config ROCKCHIP_SPL_RESERVE_IRAM\n> \thex \"Size of IRAM reserved in SPL\"\n> \tdefault 0x4000\n> diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c\n> index 8380e4e..6f0d583 100644\n> --- a/arch/arm/mach-rockchip/bootrom.c\n> +++ b/arch/arm/mach-rockchip/bootrom.c\n> @@ -12,5 +12,5 @@ void back_to_bootrom(void)\n> #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)\n> \tputs(\"Returning to boot ROM...\\n\");\n> #endif\n> -\t_back_to_bootrom_s();\n> +\t_back_to_bootrom_s(0);\n> }\n> diff --git a/arch/arm/mach-rockchip/save_boot_param.S b/arch/arm/mach-rockchip/save_boot_param.S\n> index 50fce20..f1bed0b 100644\n> --- a/arch/arm/mach-rockchip/save_boot_param.S\n> +++ b/arch/arm/mach-rockchip/save_boot_param.S\n> @@ -7,11 +7,25 @@\n>\n> #include \n>\n> +#define BACK_TO_BROM_DOWNLOAD_FLAG 0xEF08A53C\n\nThis looks like it should be defined in bootrom.h\n\n> +\n> #if defined(CONFIG_ARM64)\n> .globl\tSAVE_SP_ADDR\n> SAVE_SP_ADDR:\n> \t.quad 0\n>\n> +ENTRY(check_back_to_brom_dnl_flag)\n> +\tldr\tx8, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tldr\tx9, [x8]\n> +\tldr\tx0, =BACK_TO_BROM_DOWNLOAD_FLAG\n> +\tcmp\tx9, x0\n> +\tb.ne\tsave_boot_params_ret\n> +\tmov\tx9, xzr\n> +\tstr\tx9, [x8]\t/* clear flag */\n> +\tmov\tx0, #1 /* indicate the bootrom to enter download mode */\n> +\tb\t_back_to_bootrom_s\n\nHow does this ever get entered? If the download flag is already set prior \nto this code being executed, then the BROM would certainly not even come \nhere?\n\nIf you just always save the boot_params and check the download flag later \nfrom C code, then you could have this implemented in C. This will remove \nthe need to write two separate assembly functions (for AArch64 and \nAArch32) and generally be more readable. Please revise.\n\n> +ENDPROC(check_back_to_brom_dnl_flag)\n> +\n> ENTRY(save_boot_params)\n> \tsub\tsp, sp, #0x60\n> \tstp\tx29, x30, [sp, #0x50]\n> @@ -23,14 +37,22 @@ ENTRY(save_boot_params)\n> \tldr\tx8, =SAVE_SP_ADDR\n> \tmov\tx9, sp\n> \tstr\tx9, [x8]\n> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tb\tcheck_back_to_brom_dnl_flag\n> +#else\n> \tb\tsave_boot_params_ret /* back to my caller */\n> +#endif\n\nPlease avoid the #if #else #endif here. Could you simply call into a \nfunction that handles this correctly for both cases?\n\nHowever, this should fold back onto just the save_boot_params case anyway, \nif you can implement the checking function in C (as suggested above).\n\n> ENDPROC(save_boot_params)\n>\n> +/*\n> + * x0: return value for bootrom, none-zero for bootrom download\n\ntypo: non-zero\n\n> + * mode and zero for normal boot mode\n> + */\n> .globl _back_to_bootrom_s\n> ENTRY(_back_to_bootrom_s)\n> -\tldr\tx0, =SAVE_SP_ADDR\n> -\tldr\tx0, [x0]\n> -\tmov\tsp, x0\n> +\tldr\tx1, =SAVE_SP_ADDR\n> +\tldr\tx1, [x1]\n> +\tmov\tsp, x1\n> \tldp\tx29, x30, [sp, #0x50]\n> \tldp\tx27, x28, [sp, #0x40]\n> \tldp\tx25, x26, [sp, #0x30]\n> @@ -38,7 +60,6 @@ ENTRY(_back_to_bootrom_s)\n> \tldp\tx21, x22, [sp, #0x10]\n> \tldp\tx19, x20, [sp]\n> \tadd\tsp, sp, #0x60\n> -\tmov\tx0, xzr\n> \tret\n> ENDPROC(_back_to_bootrom_s)\n> #else\n> @@ -46,6 +67,18 @@ ENDPROC(_back_to_bootrom_s)\n> SAVE_SP_ADDR:\n> \t.word 0\n>\n> +ENTRY(check_back_to_brom_dnl_flag)\n> +\tldr\tr0, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tldr\tr1, [r0]\n> +\tldr\tr2, =BACK_TO_BROM_DOWNLOAD_FLAG\n> +\tcmp\tr1, r2\n> +\tbne\tsave_boot_params_ret\n> +\tmov\tr3, #0\n> +\tstr\tr3, [r0] @clear flag\n> +\tmov\tr0, #1 @indicate the bootrom to enter download mode\n> +\tb\t_back_to_bootrom_s\n> +ENDPROC(check_back_to_brom_dnl_flag)\n\nSee above: should be possible to do in C.\n\n> +\n> /*\n> * void save_boot_params\n> *\n> @@ -55,15 +88,21 @@ ENTRY(save_boot_params)\n> \tpush\t{r1-r12, lr}\n> \tldr\tr0, =SAVE_SP_ADDR\n> \tstr\tsp, [r0]\n> -\tb\tsave_boot_params_ret\t\t@ back to my caller\n> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n> +\tb\tcheck_back_to_brom_dnl_flag\n> +#else\n> +\tb save_boot_params_ret\n> +#endif\n\nSee above.\n\n> ENDPROC(save_boot_params)\n>\n> -\n> +/*\n> + * r0: return value for bootrom, none-zero for bootrom download\n> + * mode and zero for normal boot mode\n> + */\n> .globl _back_to_bootrom_s\n> ENTRY(_back_to_bootrom_s)\n> -\tldr\tr0, =SAVE_SP_ADDR\n> -\tldr\tsp, [r0]\n> -\tmov\tr0, #0\n> +\tldr\tr1, =SAVE_SP_ADDR\n> +\tldr\tsp, [r1]\n> \tpop\t{r1-r12, pc}\n> ENDPROC(_back_to_bootrom_s)\n> #endif\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsFJ35vWDz9s9Y\n\tfor ;\n\tWed, 13 Sep 2017 05:30:27 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 6E111C22120; Tue, 12 Sep 2017 19:30:24 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 6B1F7C21EEB;\n\tTue, 12 Sep 2017 19:30:21 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 5D5BCC21EEB; Tue, 12 Sep 2017 19:30:20 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 15612C21EC2\n\tfor ; Tue, 12 Sep 2017 19:30:20 +0000 (UTC)","from [86.59.122.178] (port=59792 helo=android.lan)\n\tby mail.theobroma-systems.com with esmtps\n\t(TLS1.2:RSA_AES_128_CBC_SHA1:128)\n\t(Exim 4.80) (envelope-from )\n\tid 1drqsz-00077y-Iy; Tue, 12 Sep 2017 21:30:17 +0200","from [10.11.0.14] (helo=vpn-10-11-0-14.lan)\n\tby android.lan with esmtp (Exim 4.84_2)\n\t(envelope-from )\n\tid 1drqsz-00080E-5O; Tue, 12 Sep 2017 21:30:17 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.0 required=5.0 tests=none autolearn=unavailable\n\tautolearn_force=no version=3.4.0","Date":"Tue, 12 Sep 2017 21:30:13 +0200 (CEST)","From":"Philipp Tomsich ","X-X-Sender":"ptomsich@vpn-10-11-0-14.lan","To":"Andy Yan ","In-Reply-To":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>","Message-ID":"","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>","User-Agent":"Alpine 2.21 (OSX 202 2017-01-01)","MIME-Version":"1.0","Cc":"u-boot@lists.denx.de","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767448,"web_url":"http://patchwork.ozlabs.org/comment/1767448/","msgid":"","list_archive_url":null,"date":"2017-09-13T01:23:05","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":65124,"url":"http://patchwork.ozlabs.org/api/people/65124/","name":"Andy Yan","email":"andy.yan@rock-chips.com"},"content":"Hi Philipp:\n\n\nOn 2017年09月13日 03:30, Philipp Tomsich wrote:\n>\n>\n> On Mon, 11 Sep 2017, Andy Yan wrote:\n>\n>> Rockchip bootrom will enter download mode if it returns from\n>> spl/tpl with a none-zero value and couldn't find a valid image\n>> in the backup partition.\n>> This patch provide a method to instruct the system to back to\n>> bootrom download mode by checking the BROM_DOWNLOAD_FLAG register.\n>> As the bootrom download function relys on some modules such as\n>> interrupts, so we need to back to bootrom as early as possbile\n>> before the tpl/tps code override the interrupt settings.\n>\n> I was not aware that the TPL/SPL overrides interrupt settings. What \n> exactly does this comment refer to?\n\n For armv7, the VBAR and V in SCTLR(which should be 1 for rk3288 and \nzero for other arm32 platforms)\n are override in arch/cpu/armv7/start.S\n For armv8, I also find the VBAR related settings, but I didn't test it.\n I am not sure is there any other settings in the TPL/SPL startup \ncode that will override the default setting in\nbootrom(maybe mmu, cache and other feathers added to the startup code in \nthe future, this is out of control),\nbut the VBAR and V are indeed override on arm32 platforms. So I think \nback to bootrom download mode if the\nflag is set before anything changed is a efficient way.\n>\n>>\n>> Signed-off-by: Andy Yan \n>> Reviewed-by: Kever Yang \n>> Acked-by: Philipp Tomsich \n>> ---\n>>\n>> arch/arm/include/asm/arch-rockchip/bootrom.h | 2 +-\n>> arch/arm/mach-rockchip/Kconfig | 13 +++++++\n>> arch/arm/mach-rockchip/bootrom.c | 2 +-\n>> arch/arm/mach-rockchip/save_boot_param.S | 57 \n>> +++++++++++++++++++++++-----\n>> 4 files changed, 63 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h \n>> b/arch/arm/include/asm/arch-rockchip/bootrom.h\n>> index 92eb878..6ae3e94 100644\n>> --- a/arch/arm/include/asm/arch-rockchip/bootrom.h\n>> +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h\n>> @@ -22,6 +22,6 @@ void back_to_bootrom(void);\n>> /**\n>> * Assembler component for the above (do not call this directly)\n>> */\n>> -void _back_to_bootrom_s(void);\n>> +void _back_to_bootrom_s(int mode);\n>\n> Please add documentation for externally visible functions.\n Okay.\n>\n>>\n>> #endif\n>> diff --git a/arch/arm/mach-rockchip/Kconfig \n>> b/arch/arm/mach-rockchip/Kconfig\n>> index d9b25d5..3ab0c30 100644\n>> --- a/arch/arm/mach-rockchip/Kconfig\n>> +++ b/arch/arm/mach-rockchip/Kconfig\n>> @@ -113,6 +113,7 @@ config ROCKCHIP_RK3399\n>> select SPL_SERIAL_SUPPORT\n>> select SPL_DRIVERS_MISC_SUPPORT\n>> select ENABLE_ARM_SOC_BOOT0_HOOK\n>> + select ROCKCHIP_BROM_HELPER\n>> select DEBUG_UART_BOARD_INIT\n>> help\n>> The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72\n>> @@ -149,6 +150,18 @@ config TPL_ROCKCHIP_BACK_TO_BROM\n>> SPL will return to the boot rom, which will then load the \n>> U-Boot\n>> binary to keep going on.\n>>\n>> +config ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>> + hex \"Bootrom download mode flag register address\"\n>> + default 0x200081c8 if ROCKCHIP_RK3036\n>> + default 0xff730094 if ROCKCHIP_RK3288\n>> + default 0xff738200 if ROCKCHIP_RK3368\n>> + default 0xff320300 if ROCKCHIP_RK3399\n>> + default 0x10300580 if ROCKCHIP_RV1108\n>> + default 0x00\n>\n> If this is not user-configurable (i.e. if it is a per-chip constant), \n> we should define this in a header file. I would suggest to do the \n> detection/mapping either in include/asm/arch-rockchip/bootrom.h or in \n> a cpu-specific header that gets included from there.\n\n Actually this is user-configuarable, we just chose a register that \nnot be used by the system to pass the boot mode flag.\nAnd we also have boards that don't want to enable this function, so they \ncan set the register address to 0. then we will ship\nthe mode check .\n>\n>> + help\n>> + The Soc will return to bootrom download mode if this register set\n>> + to BOOTROM_DOWNLOAD_FLAG.\n>\n> Documenting here what BOOTROM_DOWNLOAD_FLAG is or where it can be \n> found would be very helpful to anyone coming across this in the future.\n\n okay\n>\n>> +\n>> config ROCKCHIP_SPL_RESERVE_IRAM\n>> hex \"Size of IRAM reserved in SPL\"\n>> default 0x4000\n>> diff --git a/arch/arm/mach-rockchip/bootrom.c \n>> b/arch/arm/mach-rockchip/bootrom.c\n>> index 8380e4e..6f0d583 100644\n>> --- a/arch/arm/mach-rockchip/bootrom.c\n>> +++ b/arch/arm/mach-rockchip/bootrom.c\n>> @@ -12,5 +12,5 @@ void back_to_bootrom(void)\n>> #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)\n>> puts(\"Returning to boot ROM...\\n\");\n>> #endif\n>> - _back_to_bootrom_s();\n>> + _back_to_bootrom_s(0);\n>> }\n>> diff --git a/arch/arm/mach-rockchip/save_boot_param.S \n>> b/arch/arm/mach-rockchip/save_boot_param.S\n>> index 50fce20..f1bed0b 100644\n>> --- a/arch/arm/mach-rockchip/save_boot_param.S\n>> +++ b/arch/arm/mach-rockchip/save_boot_param.S\n>> @@ -7,11 +7,25 @@\n>>\n>> #include \n>>\n>> +#define BACK_TO_BROM_DOWNLOAD_FLAG 0xEF08A53C\n>\n> This looks like it should be defined in bootrom.h\n\n It has been move to boot-mode.h in new version.\n>\n>> +\n>> #if defined(CONFIG_ARM64)\n>> .globl SAVE_SP_ADDR\n>> SAVE_SP_ADDR:\n>> .quad 0\n>>\n>> +ENTRY(check_back_to_brom_dnl_flag)\n>> + ldr x8, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>> + ldr x9, [x8]\n>> + ldr x0, =BACK_TO_BROM_DOWNLOAD_FLAG\n>> + cmp x9, x0\n>> + b.ne save_boot_params_ret\n>> + mov x9, xzr\n>> + str x9, [x8] /* clear flag */\n>> + mov x0, #1 /* indicate the bootrom to enter download \n>> mode */\n>> + b _back_to_bootrom_s\n>\n> How does this ever get entered? If the download flag is already set \n> prior to this code being executed, then the BROM would certainly not \n> even come here?\n\n BROM would not check the boot mode register, it only enter bootrom \ndownload mode if we return a non-zere value for it or it can't find a \nvalid image from all the storage\ndevice.\n>\n> If you just always save the boot_params and check the download flag \n> later from C code, then you could have this implemented in C. This \n> will remove the need to write two separate assembly functions (for \n> AArch64 and AArch32) and generally be more readable. Please revise.\n\n We can't predict how many settings the TPL/SPL startup code changed \nnow and future\nwill affect the bootrom download function, So back to bootrom download \nmode before\nanything been changed is a simple way.\n>\n>> +ENDPROC(check_back_to_brom_dnl_flag)\n>> +\n>> ENTRY(save_boot_params)\n>> sub sp, sp, #0x60\n>> stp x29, x30, [sp, #0x50]\n>> @@ -23,14 +37,22 @@ ENTRY(save_boot_params)\n>> ldr x8, =SAVE_SP_ADDR\n>> mov x9, sp\n>> str x9, [x8]\n>> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>> + b check_back_to_brom_dnl_flag\n>> +#else\n>> b save_boot_params_ret /* back to my caller */\n>> +#endif\n>\n> Please avoid the #if #else #endif here. Could you simply call into a \n> function that handles this correctly for both cases?\n\n I have to skip the check if the REG address is zero.\n>\n> However, this should fold back onto just the save_boot_params case \n> anyway, if you can implement the checking function in C (as suggested \n> above).\n\n Please see my explanation above.\n>\n>> ENDPROC(save_boot_params)\n>>\n>> +/*\n>> + * x0: return value for bootrom, none-zero for bootrom download\n>\n> typo: non-zero\n>\n>> + * mode and zero for normal boot mode\n>> + */\n>> .globl _back_to_bootrom_s\n>> ENTRY(_back_to_bootrom_s)\n>> - ldr x0, =SAVE_SP_ADDR\n>> - ldr x0, [x0]\n>> - mov sp, x0\n>> + ldr x1, =SAVE_SP_ADDR\n>> + ldr x1, [x1]\n>> + mov sp, x1\n>> ldp x29, x30, [sp, #0x50]\n>> ldp x27, x28, [sp, #0x40]\n>> ldp x25, x26, [sp, #0x30]\n>> @@ -38,7 +60,6 @@ ENTRY(_back_to_bootrom_s)\n>> ldp x21, x22, [sp, #0x10]\n>> ldp x19, x20, [sp]\n>> add sp, sp, #0x60\n>> - mov x0, xzr\n>> ret\n>> ENDPROC(_back_to_bootrom_s)\n>> #else\n>> @@ -46,6 +67,18 @@ ENDPROC(_back_to_bootrom_s)\n>> SAVE_SP_ADDR:\n>> .word 0\n>>\n>> +ENTRY(check_back_to_brom_dnl_flag)\n>> + ldr r0, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>> + ldr r1, [r0]\n>> + ldr r2, =BACK_TO_BROM_DOWNLOAD_FLAG\n>> + cmp r1, r2\n>> + bne save_boot_params_ret\n>> + mov r3, #0\n>> + str r3, [r0] @clear flag\n>> + mov r0, #1 @indicate the bootrom to enter download mode\n>> + b _back_to_bootrom_s\n>> +ENDPROC(check_back_to_brom_dnl_flag)\n>\n> See above: should be possible to do in C.\n>\n>> +\n>> /*\n>> * void save_boot_params\n>> *\n>> @@ -55,15 +88,21 @@ ENTRY(save_boot_params)\n>> push {r1-r12, lr}\n>> ldr r0, =SAVE_SP_ADDR\n>> str sp, [r0]\n>> - b save_boot_params_ret @ back to my caller\n>> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>> + b check_back_to_brom_dnl_flag\n>> +#else\n>> + b save_boot_params_ret\n>> +#endif\n>\n> See above.\n>\n>> ENDPROC(save_boot_params)\n>>\n>> -\n>> +/*\n>> + * r0: return value for bootrom, none-zero for bootrom download\n>> + * mode and zero for normal boot mode\n>> + */\n>> .globl _back_to_bootrom_s\n>> ENTRY(_back_to_bootrom_s)\n>> - ldr r0, =SAVE_SP_ADDR\n>> - ldr sp, [r0]\n>> - mov r0, #0\n>> + ldr r1, =SAVE_SP_ADDR\n>> + ldr sp, [r1]\n>> pop {r1-r12, pc}\n>> ENDPROC(_back_to_bootrom_s)\n>> #endif\n>>\n>\n>\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsP7M4S9cz9t4V\n\tfor ;\n\tWed, 13 Sep 2017 11:23:27 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 5D3A9C22005; Wed, 13 Sep 2017 01:23:24 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id E89E6C21EF7;\n\tWed, 13 Sep 2017 01:23:20 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid A1C1EC21EF7; Wed, 13 Sep 2017 01:23:19 +0000 (UTC)","from regular1.263xmail.com (regular1.263xmail.com [211.150.99.139])\n\tby lists.denx.de (Postfix) with ESMTPS id C6F97C21DA5\n\tfor ; Wed, 13 Sep 2017 01:23:17 +0000 (UTC)","from andy.yan?rock-chips.com (unknown [192.168.167.204])\n\tby regular1.263xmail.com (Postfix) with ESMTP id B8CCB5676\n\tfor ; Wed, 13 Sep 2017 09:23:13 +0800 (CST)","from [172.16.12.133] (localhost [127.0.0.1])\n\tby smtp.263.net (Postfix) with ESMTPA id 128AF3BE;\n\tWed, 13 Sep 2017 09:23:06 +0800 (CST)","from [172.16.12.133] (unknown [58.22.7.114])\n\tby smtp.263.net (Postfix) whith ESMTP id 15435WS1CU1;\n\tWed, 13 Sep 2017 09:23:09 +0800 (CST)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.6 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_SORBS_WEB autolearn=no autolearn_force=no version=3.4.0","X-263anti-spam":"KSV:0;BIG:0;","X-MAIL-GRAY":"0","X-MAIL-DELIVERY":"1","X-KSVirus-check":"0","X-ADDR-CHECKED4":"1","X-ABS-CHECKED":"1","X-SKE-CHECKED":"1","X-ANTISPAM-LEVEL":"2","X-RL-SENDER":"andy.yan@rock-chips.com","X-FST-TO":"u-boot@lists.denx.de","X-SENDER-IP":"58.22.7.114","X-LOGIN-NAME":"andy.yan@rock-chips.com","X-UNIQUE-TAG":"<5229edf8f9721ba62acb690c0f544c11>","X-ATTACHMENT-NUM":"0","X-SENDER":"yxj@rock-chips.com","X-DNS-TYPE":"0","To":"Philipp Tomsich ","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>\n\t","From":"Andy Yan ","Message-ID":"","Date":"Wed, 13 Sep 2017 09:23:05 +0800","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","Cc":"u-boot@lists.denx.de","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767658,"web_url":"http://patchwork.ozlabs.org/comment/1767658/","msgid":"","list_archive_url":null,"date":"2017-09-13T08:01:59","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"> On 13 Sep 2017, at 03:23, Andy Yan wrote:\n> \n> Hi Philipp:\n> \n> \n> On 2017年09月13日 03:30, Philipp Tomsich wrote:\n>> \n>> \n>> On Mon, 11 Sep 2017, Andy Yan wrote:\n>> \n>>> Rockchip bootrom will enter download mode if it returns from\n>>> spl/tpl with a none-zero value and couldn't find a valid image\n>>> in the backup partition.\n>>> This patch provide a method to instruct the system to back to\n>>> bootrom download mode by checking the BROM_DOWNLOAD_FLAG register.\n>>> As the bootrom download function relys on some modules such as\n>>> interrupts, so we need to back to bootrom as early as possbile\n>>> before the tpl/tps code override the interrupt settings.\n>> \n>> I was not aware that the TPL/SPL overrides interrupt settings. What exactly does this comment refer to?\n> \n> For armv7, the VBAR and V in SCTLR(which should be 1 for rk3288 and zero for other arm32 platforms)\n> are override in arch/cpu/armv7/start.S\n> For armv8, I also find the VBAR related settings, but I didn't test it.\n> I am not sure is there any other settings in the TPL/SPL startup code that will override the default setting in\n> bootrom(maybe mmu, cache and other feathers added to the startup code in the future, this is out of control),\n> but the VBAR and V are indeed override on arm32 platforms. So I think back to bootrom download mode if the\n> flag is set before anything changed is a efficient way.\n\nShould we save these flags as part of the \"save_boot_params + back_to_bootrom_s” processing?\n\n>> \n>>> \n>>> Signed-off-by: Andy Yan \n>>> Reviewed-by: Kever Yang \n>>> Acked-by: Philipp Tomsich \n>>> ---\n>>> \n>>> arch/arm/include/asm/arch-rockchip/bootrom.h | 2 +-\n>>> arch/arm/mach-rockchip/Kconfig | 13 +++++++\n>>> arch/arm/mach-rockchip/bootrom.c | 2 +-\n>>> arch/arm/mach-rockchip/save_boot_param.S | 57 +++++++++++++++++++++++-----\n>>> 4 files changed, 63 insertions(+), 11 deletions(-)\n>>> \n>>> diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h\n>>> index 92eb878..6ae3e94 100644\n>>> --- a/arch/arm/include/asm/arch-rockchip/bootrom.h\n>>> +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h\n>>> @@ -22,6 +22,6 @@ void back_to_bootrom(void);\n>>> /**\n>>> * Assembler component for the above (do not call this directly)\n>>> */\n>>> -void _back_to_bootrom_s(void);\n>>> +void _back_to_bootrom_s(int mode);\n>> \n>> Please add documentation for externally visible functions.\n> Okay.\n>> \n>>> \n>>> #endif\n>>> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig\n>>> index d9b25d5..3ab0c30 100644\n>>> --- a/arch/arm/mach-rockchip/Kconfig\n>>> +++ b/arch/arm/mach-rockchip/Kconfig\n>>> @@ -113,6 +113,7 @@ config ROCKCHIP_RK3399\n>>> select SPL_SERIAL_SUPPORT\n>>> select SPL_DRIVERS_MISC_SUPPORT\n>>> select ENABLE_ARM_SOC_BOOT0_HOOK\n>>> + select ROCKCHIP_BROM_HELPER\n>>> select DEBUG_UART_BOARD_INIT\n>>> help\n>>> The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72\n>>> @@ -149,6 +150,18 @@ config TPL_ROCKCHIP_BACK_TO_BROM\n>>> SPL will return to the boot rom, which will then load the U-Boot\n>>> binary to keep going on.\n>>> \n>>> +config ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>> + hex \"Bootrom download mode flag register address\"\n>>> + default 0x200081c8 if ROCKCHIP_RK3036\n>>> + default 0xff730094 if ROCKCHIP_RK3288\n>>> + default 0xff738200 if ROCKCHIP_RK3368\n>>> + default 0xff320300 if ROCKCHIP_RK3399\n>>> + default 0x10300580 if ROCKCHIP_RV1108\n>>> + default 0x00\n>> \n>> If this is not user-configurable (i.e. if it is a per-chip constant), we should define this in a header file. I would suggest to do the detection/mapping either in include/asm/arch-rockchip/bootrom.h or in a cpu-specific header that gets included from there.\n> \n> Actually this is user-configuarable, we just chose a register that not be used by the system to pass the boot mode flag.\n> And we also have boards that don't want to enable this function, so they can set the register address to 0. then we will ship\n> the mode check .\n\nUnderstood.\n\n>> \n>>> + help\n>>> + The Soc will return to bootrom download mode if this register set\n>>> + to BOOTROM_DOWNLOAD_FLAG.\n>> \n>> Documenting here what BOOTROM_DOWNLOAD_FLAG is or where it can be found would be very helpful to anyone coming across this in the future.\n> \n> okay\n>> \n>>> +\n>>> config ROCKCHIP_SPL_RESERVE_IRAM\n>>> hex \"Size of IRAM reserved in SPL\"\n>>> default 0x4000\n>>> diff --git a/arch/arm/mach-rockchip/bootrom.c b/arch/arm/mach-rockchip/bootrom.c\n>>> index 8380e4e..6f0d583 100644\n>>> --- a/arch/arm/mach-rockchip/bootrom.c\n>>> +++ b/arch/arm/mach-rockchip/bootrom.c\n>>> @@ -12,5 +12,5 @@ void back_to_bootrom(void)\n>>> #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)\n>>> puts(\"Returning to boot ROM...\\n\");\n>>> #endif\n>>> - _back_to_bootrom_s();\n>>> + _back_to_bootrom_s(0);\n>>> }\n>>> diff --git a/arch/arm/mach-rockchip/save_boot_param.S b/arch/arm/mach-rockchip/save_boot_param.S\n>>> index 50fce20..f1bed0b 100644\n>>> --- a/arch/arm/mach-rockchip/save_boot_param.S\n>>> +++ b/arch/arm/mach-rockchip/save_boot_param.S\n>>> @@ -7,11 +7,25 @@\n>>> \n>>> #include \n>>> \n>>> +#define BACK_TO_BROM_DOWNLOAD_FLAG 0xEF08A53C\n>> \n>> This looks like it should be defined in bootrom.h\n> \n> It has been move to boot-mode.h in new version.\n>> \n>>> +\n>>> #if defined(CONFIG_ARM64)\n>>> .globl SAVE_SP_ADDR\n>>> SAVE_SP_ADDR:\n>>> .quad 0\n>>> \n>>> +ENTRY(check_back_to_brom_dnl_flag)\n>>> + ldr x8, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>> + ldr x9, [x8]\n>>> + ldr x0, =BACK_TO_BROM_DOWNLOAD_FLAG\n>>> + cmp x9, x0\n>>> + b.ne save_boot_params_ret\n>>> + mov x9, xzr\n>>> + str x9, [x8] /* clear flag */\n>>> + mov x0, #1 /* indicate the bootrom to enter download mode */\n>>> + b _back_to_bootrom_s\n>> \n>> How does this ever get entered? If the download flag is already set prior to this code being executed, then the BROM would certainly not even come here?\n> \n> BROM would not check the boot mode register, it only enter bootrom download mode if we return a non-zere value for it or it can't find a valid image from all the storage\n> device.\n\nWe should document this somewhere. A comment in this code might be great to let everyone know why we are checking this here.\n\n>> \n>> If you just always save the boot_params and check the download flag later from C code, then you could have this implemented in C. This will remove the need to write two separate assembly functions (for AArch64 and AArch32) and generally be more readable. Please revise.\n> \n> We can't predict how many settings the TPL/SPL startup code changed now and future\n> will affect the bootrom download function, So back to bootrom download mode before\n> anything been changed is a simple way.\n\nOk. I’d still like to have this in C.\nThe only requirement for this will be having a valid stack-pointer, so we should be able to do this early (before the various initialisation runs).\nI think board_init_f() would be a suitable place.\n\n>> \n>>> +ENDPROC(check_back_to_brom_dnl_flag)\n>>> +\n>>> ENTRY(save_boot_params)\n>>> sub sp, sp, #0x60\n>>> stp x29, x30, [sp, #0x50]\n>>> @@ -23,14 +37,22 @@ ENTRY(save_boot_params)\n>>> ldr x8, =SAVE_SP_ADDR\n>>> mov x9, sp\n>>> str x9, [x8]\n>>> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>> + b check_back_to_brom_dnl_flag\n>>> +#else\n>>> b save_boot_params_ret /* back to my caller */\n>>> +#endif\n>> \n>> Please avoid the #if #else #endif here. Could you simply call into a function that handles this correctly for both cases?\n> \n> I have to skip the check if the REG address is zero.\n\nGood idea. Having a check for zero would match exactly how you described the handling of the Kconfig variable.\nNote that you should document the special meaning of zero both in a comment and in the Kconfig help text.\n\n>> \n>> However, this should fold back onto just the save_boot_params case anyway, if you can implement the checking function in C (as suggested above).\n> \n> Please see my explanation above.\n>> \n>>> ENDPROC(save_boot_params)\n>>> \n>>> +/*\n>>> + * x0: return value for bootrom, none-zero for bootrom download\n>> \n>> typo: non-zero\n>> \n>>> + * mode and zero for normal boot mode\n>>> + */\n>>> .globl _back_to_bootrom_s\n>>> ENTRY(_back_to_bootrom_s)\n>>> - ldr x0, =SAVE_SP_ADDR\n>>> - ldr x0, [x0]\n>>> - mov sp, x0\n>>> + ldr x1, =SAVE_SP_ADDR\n>>> + ldr x1, [x1]\n>>> + mov sp, x1\n>>> ldp x29, x30, [sp, #0x50]\n>>> ldp x27, x28, [sp, #0x40]\n>>> ldp x25, x26, [sp, #0x30]\n>>> @@ -38,7 +60,6 @@ ENTRY(_back_to_bootrom_s)\n>>> ldp x21, x22, [sp, #0x10]\n>>> ldp x19, x20, [sp]\n>>> add sp, sp, #0x60\n>>> - mov x0, xzr\n>>> ret\n>>> ENDPROC(_back_to_bootrom_s)\n>>> #else\n>>> @@ -46,6 +67,18 @@ ENDPROC(_back_to_bootrom_s)\n>>> SAVE_SP_ADDR:\n>>> .word 0\n>>> \n>>> +ENTRY(check_back_to_brom_dnl_flag)\n>>> + ldr r0, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>> + ldr r1, [r0]\n>>> + ldr r2, =BACK_TO_BROM_DOWNLOAD_FLAG\n>>> + cmp r1, r2\n>>> + bne save_boot_params_ret\n>>> + mov r3, #0\n>>> + str r3, [r0] @clear flag\n>>> + mov r0, #1 @indicate the bootrom to enter download mode\n>>> + b _back_to_bootrom_s\n>>> +ENDPROC(check_back_to_brom_dnl_flag)\n>> \n>> See above: should be possible to do in C.\n>> \n>>> +\n>>> /*\n>>> * void save_boot_params\n>>> *\n>>> @@ -55,15 +88,21 @@ ENTRY(save_boot_params)\n>>> push {r1-r12, lr}\n>>> ldr r0, =SAVE_SP_ADDR\n>>> str sp, [r0]\n>>> - b save_boot_params_ret @ back to my caller\n>>> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>> + b check_back_to_brom_dnl_flag\n>>> +#else\n>>> + b save_boot_params_ret\n>>> +#endif\n>> \n>> See above.\n>> \n>>> ENDPROC(save_boot_params)\n>>> \n>>> -\n>>> +/*\n>>> + * r0: return value for bootrom, none-zero for bootrom download\n>>> + * mode and zero for normal boot mode\n>>> + */\n>>> .globl _back_to_bootrom_s\n>>> ENTRY(_back_to_bootrom_s)\n>>> - ldr r0, =SAVE_SP_ADDR\n>>> - ldr sp, [r0]\n>>> - mov r0, #0\n>>> + ldr r1, =SAVE_SP_ADDR\n>>> + ldr sp, [r1]\n>>> pop {r1-r12, pc}\n>>> ENDPROC(_back_to_bootrom_s)\n>>> #endif","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsYzR1KMlz9sPm\n\tfor ;\n\tWed, 13 Sep 2017 18:02:11 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid E40E5C22542; Wed, 13 Sep 2017 08:02:09 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 3A723C21EE5;\n\tWed, 13 Sep 2017 08:02:05 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 76CA0C21EE5; Wed, 13 Sep 2017 08:02:04 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 05AB3C21EE2\n\tfor ; Wed, 13 Sep 2017 08:02:04 +0000 (UTC)","from 89-104-28-141.customer.bnet.at ([89.104.28.141]:52412\n\thelo=[192.168.2.129]) by mail.theobroma-systems.com with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1ds2cT-00062I-11; Wed, 13 Sep 2017 10:02:01 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"*","X-Spam-Status":"No, score=1.0 required=5.0 tests=HK_NAME_DR autolearn=no\n\tautolearn_force=no version=3.4.0","From":"\"Dr. Philipp Tomsich\" ","Message-Id":"","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","Date":"Wed, 13 Sep 2017 10:01:59 +0200","In-Reply-To":"","To":"Andy Yan ","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>\n\t\n\t","X-Mailer":"Apple Mail (2.3273)","X-Content-Filtered-By":"Mailman/MimeDel 2.1.18","Cc":"u-boot@lists.denx.de","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767680,"web_url":"http://patchwork.ozlabs.org/comment/1767680/","msgid":"","list_archive_url":null,"date":"2017-09-13T08:36:57","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":65124,"url":"http://patchwork.ozlabs.org/api/people/65124/","name":"Andy Yan","email":"andy.yan@rock-chips.com"},"content":"Hi Philipp:\n\n\nOn 2017年09月13日 16:01, Dr. Philipp Tomsich wrote:\n>\n>> On 13 Sep 2017, at 03:23, Andy Yan > > wrote:\n>>\n>> Hi Philipp:\n>>\n>>\n>> On 2017年09月13日 03:30, Philipp Tomsich wrote:\n>>>\n>>>\n>>> On Mon, 11 Sep 2017, Andy Yan wrote:\n>>>\n>>>> Rockchip bootrom will enter download mode if it returns from\n>>>> spl/tpl with a none-zero value and couldn't find a valid image\n>>>> in the backup partition.\n>>>> This patch provide a method to instruct the system to back to\n>>>> bootrom download mode by checking the BROM_DOWNLOAD_FLAG register.\n>>>> As the bootrom download function relys on some modules such as\n>>>> interrupts, so we need to back to bootrom as early as possbile\n>>>> before the tpl/tps code override the interrupt settings.\n>>>\n>>> I was not aware that the TPL/SPL overrides interrupt settings. What \n>>> exactly does this comment refer to?\n>>\n>> For armv7, the VBAR and V in SCTLR(which should be 1 for rk3288 \n>> and zero for other arm32 platforms)\n>> are override in arch/cpu/armv7/start.S\n>> For armv8, I also find the VBAR related settings, but I didn't \n>> test it.\n>> I am not sure is there any other settings in the TPL/SPL startup \n>> code that will override the default setting in\n>> bootrom(maybe mmu, cache and other feathers added to the startup code \n>> in the future, this is out of control),\n>> but the VBAR and V are indeed override on arm32 platforms. So I think \n>> back to bootrom download mode if the\n>> flag is set before anything changed is a efficient way.\n>\n> Should we save these flags as part of the \"save_boot_params + \n> back_to_bootrom_s” processing?\n>\n>>>\n>>>>\n>>>> Signed-off-by: Andy Yan >>> >\n>>>> Reviewed-by: Kever Yang >>> >\n>>>> Acked-by: Philipp Tomsich >>> >\n>>>> ---\n>>>>\n>>>> arch/arm/include/asm/arch-rockchip/bootrom.h | 2 +-\n>>>> arch/arm/mach-rockchip/Kconfig | 13 +++++++\n>>>> arch/arm/mach-rockchip/bootrom.c | 2 +-\n>>>> arch/arm/mach-rockchip/save_boot_param.S | 57 \n>>>> +++++++++++++++++++++++-----\n>>>> 4 files changed, 63 insertions(+), 11 deletions(-)\n>>>>\n>>>> diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h \n>>>> b/arch/arm/include/asm/arch-rockchip/bootrom.h\n>>>> index 92eb878..6ae3e94 100644\n>>>> --- a/arch/arm/include/asm/arch-rockchip/bootrom.h\n>>>> +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h\n>>>> @@ -22,6 +22,6 @@ void back_to_bootrom(void);\n>>>> /**\n>>>> * Assembler component for the above (do not call this directly)\n>>>> */\n>>>> -void _back_to_bootrom_s(void);\n>>>> +void _back_to_bootrom_s(int mode);\n>>>\n>>> Please add documentation for externally visible functions.\n>> Okay.\n>>>\n>>>>\n>>>> #endif\n>>>> diff --git a/arch/arm/mach-rockchip/Kconfig \n>>>> b/arch/arm/mach-rockchip/Kconfig\n>>>> index d9b25d5..3ab0c30 100644\n>>>> --- a/arch/arm/mach-rockchip/Kconfig\n>>>> +++ b/arch/arm/mach-rockchip/Kconfig\n>>>> @@ -113,6 +113,7 @@ config ROCKCHIP_RK3399\n>>>> select SPL_SERIAL_SUPPORT\n>>>> select SPL_DRIVERS_MISC_SUPPORT\n>>>> select ENABLE_ARM_SOC_BOOT0_HOOK\n>>>> + select ROCKCHIP_BROM_HELPER\n>>>> select DEBUG_UART_BOARD_INIT\n>>>> help\n>>>> The Rockchip RK3399 is a ARM-based SoC with a dual-core Cortex-A72\n>>>> @@ -149,6 +150,18 @@ config TPL_ROCKCHIP_BACK_TO_BROM\n>>>> SPL will return to the boot rom, which will then load the \n>>>> U-Boot\n>>>> binary to keep going on.\n>>>>\n>>>> +config ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>>> + hex \"Bootrom download mode flag register address\"\n>>>> + default 0x200081c8 if ROCKCHIP_RK3036\n>>>> + default 0xff730094 if ROCKCHIP_RK3288\n>>>> + default 0xff738200 if ROCKCHIP_RK3368\n>>>> + default 0xff320300 if ROCKCHIP_RK3399\n>>>> + default 0x10300580 if ROCKCHIP_RV1108\n>>>> + default 0x00\n>>>\n>>> If this is not user-configurable (i.e. if it is a per-chip \n>>> constant), we should define this in a header file. I would suggest \n>>> to do the detection/mapping either in \n>>> include/asm/arch-rockchip/bootrom.h or in a cpu-specific header that \n>>> gets included from there.\n>>\n>> Actually this is user-configuarable, we just chose a register that \n>> not be used by the system to pass the boot mode flag.\n>> And we also have boards that don't want to enable this function, so \n>> they can set the register address to 0. then we will ship\n>> the mode check .\n>\n> Understood.\n>\n>>>\n>>>> + help\n>>>> + The Soc will return to bootrom download mode if this \n>>>> register set\n>>>> + to BOOTROM_DOWNLOAD_FLAG.\n>>>\n>>> Documenting here what BOOTROM_DOWNLOAD_FLAG is or where it can be \n>>> found would be very helpful to anyone coming across this in the future.\n>>\n>> okay\n>>>\n>>>> +\n>>>> config ROCKCHIP_SPL_RESERVE_IRAM\n>>>> hex \"Size of IRAM reserved in SPL\"\n>>>> default 0x4000\n>>>> diff --git a/arch/arm/mach-rockchip/bootrom.c \n>>>> b/arch/arm/mach-rockchip/bootrom.c\n>>>> index 8380e4e..6f0d583 100644\n>>>> --- a/arch/arm/mach-rockchip/bootrom.c\n>>>> +++ b/arch/arm/mach-rockchip/bootrom.c\n>>>> @@ -12,5 +12,5 @@ void back_to_bootrom(void)\n>>>> #if CONFIG_IS_ENABLED(LIBCOMMON_SUPPORT)\n>>>> puts(\"Returning to boot ROM...\\n\");\n>>>> #endif\n>>>> - _back_to_bootrom_s();\n>>>> + _back_to_bootrom_s(0);\n>>>> }\n>>>> diff --git a/arch/arm/mach-rockchip/save_boot_param.S \n>>>> b/arch/arm/mach-rockchip/save_boot_param.S\n>>>> index 50fce20..f1bed0b 100644\n>>>> --- a/arch/arm/mach-rockchip/save_boot_param.S\n>>>> +++ b/arch/arm/mach-rockchip/save_boot_param.S\n>>>> @@ -7,11 +7,25 @@\n>>>>\n>>>> #include \n>>>>\n>>>> +#define BACK_TO_BROM_DOWNLOAD_FLAG 0xEF08A53C\n>>>\n>>> This looks like it should be defined in bootrom.h\n>>\n>> It has been move to boot-mode.h in new version.\n>>>\n>>>> +\n>>>> #if defined(CONFIG_ARM64)\n>>>> .globl SAVE_SP_ADDR\n>>>> SAVE_SP_ADDR:\n>>>> .quad 0\n>>>>\n>>>> +ENTRY(check_back_to_brom_dnl_flag)\n>>>> + ldr x8, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>>> + ldr x9, [x8]\n>>>> + ldr x0, =BACK_TO_BROM_DOWNLOAD_FLAG\n>>>> + cmp x9, x0\n>>>> + b.ne save_boot_params_ret\n>>>> + mov x9, xzr\n>>>> + str x9, [x8] /* clear flag */\n>>>> + mov x0, #1 /* indicate the bootrom to enter \n>>>> download mode */\n>>>> + b _back_to_bootrom_s\n>>>\n>>> How does this ever get entered? If the download flag is already set \n>>> prior to this code being executed, then the BROM would certainly not \n>>> even come here?\n>>\n>> BROM would not check the boot mode register, it only enter bootrom \n>> download mode if we return a non-zere value for it or it can't find a \n>> valid image from all the storage\n>> device.\n>\n> We should document this somewhere. A comment in this code might be \n> great to let everyone know why we are checking this here.\n>\n>>>\n>>> If you just always save the boot_params and check the download flag \n>>> later from C code, then you could have this implemented in C. This \n>>> will remove the need to write two separate assembly functions (for \n>>> AArch64 and AArch32) and generally be more readable. Please revise.\n>>\n>> We can't predict how many settings the TPL/SPL startup code \n>> changed now and future\n>> will affect the bootrom download function, So back to bootrom \n>> download mode before\n>> anything been changed is a simple way.\n>\n> Ok. I’d still like to have this in C.\n> The only requirement for this will be having a valid stack-pointer, so \n> we should be able to do this early (before the various initialisation \n> runs).\n> I think board_init_f() would be a suitable place.\n\n\n When I hack this function first time, I indeed wrote a c \nimplemented code in board_init_f on kylin-rk3036, but the usb failed \nconnect to my PC\nwhen back to bootrom, after a long time checking whit the bootrom code \nauthor, we found the interrupts related configurations are changed.\nThen I try to save and restore the VBAR, then the bootrom download \nfunction works. But when I tested this function on rk3288 based board, \nit failed\nagain, and I found rk3288 bootrom require a different interrupt \nconfiguration with rk3036 after a long time dig, the interrupts vector \nbase should be\n 0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR \nshould be set to 1, but the SPL startup code set it to zero.\n Then I have the idea back to bootrom download mode as early as \npossible when the download flag is set, Because no matter how many \nparameters\nI saved and restored today, no one can make sure that other parameters \nwill not changed by start.S in the future(maybe more properties changed \nabout\nthe interrupt, maybe the change of mmu /caches, ), because we always \nhave the chance to modify the startup code by the desire of new feature \nor the need\nto workaround something for a new soc, what's more the start.S it's now \nhave many #if / #else configuration, this still have risk to change the \ndefault configurations\nwhich will be used in bootrom download mode. If we rely on a save and \nrestore policy, this function may work well today, but may failed some \ndays later just because\nsome one changed another configuration in start.S.\n>\n>>>\n>>>> +ENDPROC(check_back_to_brom_dnl_flag)\n>>>> +\n>>>> ENTRY(save_boot_params)\n>>>> sub sp, sp, #0x60\n>>>> stp x29, x30, [sp, #0x50]\n>>>> @@ -23,14 +37,22 @@ ENTRY(save_boot_params)\n>>>> ldr x8, =SAVE_SP_ADDR\n>>>> mov x9, sp\n>>>> str x9, [x8]\n>>>> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>>> + b check_back_to_brom_dnl_flag\n>>>> +#else\n>>>> b save_boot_params_ret /* back to my caller */\n>>>> +#endif\n>>>\n>>> Please avoid the #if #else #endif here. Could you simply call into a \n>>> function that handles this correctly for both cases?\n>>\n>> I have to skip the check if the REG address is zero.\n>\n> Good idea. Having a check for zero would match exactly how you \n> described the handling of the Kconfig variable.\n> Note that you should document the special meaning of zero both in a \n> comment and in the Kconfig help text.\n>\n>>>\n>>> However, this should fold back onto just the save_boot_params case \n>>> anyway, if you can implement the checking function in C (as \n>>> suggested above).\n>>\n>> Please see my explanation above.\n>>>\n>>>> ENDPROC(save_boot_params)\n>>>>\n>>>> +/*\n>>>> + * x0: return value for bootrom, none-zero for bootrom download\n>>>\n>>> typo: non-zero\n>>>\n>>>> + * mode and zero for normal boot mode\n>>>> + */\n>>>> .globl _back_to_bootrom_s\n>>>> ENTRY(_back_to_bootrom_s)\n>>>> - ldr x0, =SAVE_SP_ADDR\n>>>> - ldr x0, [x0]\n>>>> - mov sp, x0\n>>>> + ldr x1, =SAVE_SP_ADDR\n>>>> + ldr x1, [x1]\n>>>> + mov sp, x1\n>>>> ldp x29, x30, [sp, #0x50]\n>>>> ldp x27, x28, [sp, #0x40]\n>>>> ldp x25, x26, [sp, #0x30]\n>>>> @@ -38,7 +60,6 @@ ENTRY(_back_to_bootrom_s)\n>>>> ldp x21, x22, [sp, #0x10]\n>>>> ldp x19, x20, [sp]\n>>>> add sp, sp, #0x60\n>>>> - mov x0, xzr\n>>>> ret\n>>>> ENDPROC(_back_to_bootrom_s)\n>>>> #else\n>>>> @@ -46,6 +67,18 @@ ENDPROC(_back_to_bootrom_s)\n>>>> SAVE_SP_ADDR:\n>>>> .word 0\n>>>>\n>>>> +ENTRY(check_back_to_brom_dnl_flag)\n>>>> + ldr r0, =CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>>> + ldr r1, [r0]\n>>>> + ldr r2, =BACK_TO_BROM_DOWNLOAD_FLAG\n>>>> + cmp r1, r2\n>>>> + bne save_boot_params_ret\n>>>> + mov r3, #0\n>>>> + str r3, [r0] @clear flag\n>>>> + mov r0, #1 @indicate the bootrom to enter download \n>>>> mode\n>>>> + b _back_to_bootrom_s\n>>>> +ENDPROC(check_back_to_brom_dnl_flag)\n>>>\n>>> See above: should be possible to do in C.\n>>>\n>>>> +\n>>>> /*\n>>>> * void save_boot_params\n>>>> *\n>>>> @@ -55,15 +88,21 @@ ENTRY(save_boot_params)\n>>>> push {r1-r12, lr}\n>>>> ldr r0, =SAVE_SP_ADDR\n>>>> str sp, [r0]\n>>>> - b save_boot_params_ret @ back to my caller\n>>>> +#if CONFIG_ROCKCHIP_BACK_TO_BROM_DOWNLOAD_REG\n>>>> + b check_back_to_brom_dnl_flag\n>>>> +#else\n>>>> + b save_boot_params_ret\n>>>> +#endif\n>>>\n>>> See above.\n>>>\n>>>> ENDPROC(save_boot_params)\n>>>>\n>>>> -\n>>>> +/*\n>>>> + * r0: return value for bootrom, none-zero for bootrom download\n>>>> + * mode and zero for normal boot mode\n>>>> + */\n>>>> .globl _back_to_bootrom_s\n>>>> ENTRY(_back_to_bootrom_s)\n>>>> - ldr r0, =SAVE_SP_ADDR\n>>>> - ldr sp, [r0]\n>>>> - mov r0, #0\n>>>> + ldr r1, =SAVE_SP_ADDR\n>>>> + ldr sp, [r1]\n>>>> pop {r1-r12, pc}\n>>>> ENDPROC(_back_to_bootrom_s)\n>>>> #endif\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsZlx3RVhz9sNr\n\tfor ;\n\tWed, 13 Sep 2017 18:37:17 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 226BCC225AB; Wed, 13 Sep 2017 08:37:15 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id DF5A0C2204B;\n\tWed, 13 Sep 2017 08:37:10 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 1FD71C2204B; Wed, 13 Sep 2017 08:37:09 +0000 (UTC)","from regular1.263xmail.com (regular1.263xmail.com [211.150.99.131])\n\tby lists.denx.de (Postfix) with ESMTPS id 3393DC21DF3\n\tfor ; Wed, 13 Sep 2017 08:37:04 +0000 (UTC)","from andy.yan?rock-chips.com (unknown [192.168.167.193])\n\tby regular1.263xmail.com (Postfix) with ESMTP id D215862C6\n\tfor ; Wed, 13 Sep 2017 16:36:58 +0800 (CST)","from [172.16.12.133] (localhost [127.0.0.1])\n\tby smtp.263.net (Postfix) with ESMTPA id 6107D39E;\n\tWed, 13 Sep 2017 16:36:59 +0800 (CST)","from [172.16.12.133] (unknown [58.22.7.114])\n\tby smtp.263.net (Postfix) whith ESMTP id 4763N2MWRV;\n\tWed, 13 Sep 2017 16:37:00 +0800 (CST)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.6 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H2,RCVD_IN_SORBS_WEB autolearn=no autolearn_force=no\n\tversion=3.4.0","X-263anti-spam":"KSV:0;","X-MAIL-GRAY":"0","X-MAIL-DELIVERY":"1","X-KSVirus-check":"0","X-ABS-CHECKED":"4","X-RL-SENDER":"andy.yan@rock-chips.com","X-FST-TO":"kever.yang@rock-chips.com","X-SENDER-IP":"58.22.7.114","X-LOGIN-NAME":"andy.yan@rock-chips.com","X-UNIQUE-TAG":"<73225c04b5e8350874a3c35f394bbbf2>","X-ATTACHMENT-NUM":"0","X-SENDER":"yxj@rock-chips.com","X-DNS-TYPE":"0","To":"\"Dr. Philipp Tomsich\" ","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>\n\t\n\t\n\t","From":"Andy Yan ","Message-ID":"","Date":"Wed, 13 Sep 2017 16:36:57 +0800","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","X-Content-Filtered-By":"Mailman/MimeDel 2.1.18","Cc":"u-boot@lists.denx.de","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767684,"web_url":"http://patchwork.ozlabs.org/comment/1767684/","msgid":"<8CBB4CDC-285D-4C01-906B-447FACC5EDF5@theobroma-systems.com>","list_archive_url":null,"date":"2017-09-13T08:50:00","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"> On 13 Sep 2017, at 10:36, Andy Yan wrote:\n> \n>>>> \n>>>> If you just always save the boot_params and check the download flag later from C code, then you could have this implemented in C. This will remove the need to write two separate assembly functions (for AArch64 and AArch32) and generally be more readable. Please revise.\n>>> \n>>> We can't predict how many settings the TPL/SPL startup code changed now and future\n>>> will affect the bootrom download function, So back to bootrom download mode before\n>>> anything been changed is a simple way.\n>> \n>> Ok. I’d still like to have this in C.\n>> The only requirement for this will be having a valid stack-pointer, so we should be able to do this early (before the various initialisation runs).\n>> I think board_init_f() would be a suitable place.\n> \n> \n> When I hack this function first time, I indeed wrote a c implemented code in board_init_f on kylin-rk3036, but the usb failed connect to my PC\n> when back to bootrom, after a long time checking whit the bootrom code author, we found the interrupts related configurations are changed.\n> Then I try to save and restore the VBAR, then the bootrom download function works. But when I tested this function on rk3288 based board, it failed\n> again, and I found rk3288 bootrom require a different interrupt configuration with rk3036 after a long time dig, the interrupts vector base should be\n> 0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR should be set to 1, but the SPL startup code set it to zero.\n> Then I have the idea back to bootrom download mode as early as possible when the download flag is set, Because no matter how many parameters \n> I saved and restored today, no one can make sure that other parameters will not changed by start.S in the future(maybe more properties changed about\n> the interrupt, maybe the change of mmu /caches, ), because we always have the chance to modify the startup code by the desire of new feature or the need\n> to workaround something for a new soc, what's more the start.S it's now have many #if / #else configuration, this still have risk to change the default configurations\n> which will be used in bootrom download mode. If we rely on a save and restore policy, this function may work well today, but may failed some days later just because \n> some one changed another configuration in start.S. \n\nWhen looking at what happens (on armv7) between the save_boot_params\nand the call to _main (which in turn just sets up SP and calls into board_init_f),\nthere isn’t much happening: it really is only cpu_init_cp15 and cpu_init_crit\nthat will be of concern.\nI really wonder whether we could have a sufficient C runtime (i.e. SP valid)\navailable before those run.\n\nDoes your BROM always call us with the SP valid (I know that that’s the case\non the RK3399 and RK3368), so that we could run on the BROM’s stack here?","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsb2y3x2Yz9s81\n\tfor ;\n\tWed, 13 Sep 2017 18:50:18 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 4F12BC22597; Wed, 13 Sep 2017 08:50:08 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 2CCCCC2204B;\n\tWed, 13 Sep 2017 08:50:06 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 693DBC2204B; Wed, 13 Sep 2017 08:50:04 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 31C1DC21ED8\n\tfor ; Wed, 13 Sep 2017 08:50:04 +0000 (UTC)","from 89-104-28-141.customer.bnet.at ([89.104.28.141]:54450\n\thelo=[192.168.2.129]) by mail.theobroma-systems.com with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1ds3Mw-0007We-1h; Wed, 13 Sep 2017 10:50:02 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"*","X-Spam-Status":"No, score=1.0 required=5.0 tests=HK_NAME_DR autolearn=no\n\tautolearn_force=no version=3.4.0","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","From":"\"Dr. Philipp Tomsich\" ","In-Reply-To":"","Date":"Wed, 13 Sep 2017 10:50:00 +0200","Message-Id":"<8CBB4CDC-285D-4C01-906B-447FACC5EDF5@theobroma-systems.com>","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>\n\t\n\t\n\t\n\t","To":"Andy Yan ","X-Mailer":"Apple Mail (2.3273)","Cc":"u-boot@lists.denx.de","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767692,"web_url":"http://patchwork.ozlabs.org/comment/1767692/","msgid":"<84c68bf6-f2bb-2268-7d7c-cd4d7d98e5bb@rock-chips.com>","list_archive_url":null,"date":"2017-09-13T09:06:53","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":65124,"url":"http://patchwork.ozlabs.org/api/people/65124/","name":"Andy Yan","email":"andy.yan@rock-chips.com"},"content":"Hi Philipp:\n\n\nOn 2017年09月13日 16:50, Dr. Philipp Tomsich wrote:\n>> On 13 Sep 2017, at 10:36, Andy Yan wrote:\n>>\n>>>>> If you just always save the boot_params and check the download flag later from C code, then you could have this implemented in C. This will remove the need to write two separate assembly functions (for AArch64 and AArch32) and generally be more readable. Please revise.\n>>>> We can't predict how many settings the TPL/SPL startup code changed now and future\n>>>> will affect the bootrom download function, So back to bootrom download mode before\n>>>> anything been changed is a simple way.\n>>> Ok. I’d still like to have this in C.\n>>> The only requirement for this will be having a valid stack-pointer, so we should be able to do this early (before the various initialisation runs).\n>>> I think board_init_f() would be a suitable place.\n>>\n>> When I hack this function first time, I indeed wrote a c implemented code in board_init_f on kylin-rk3036, but the usb failed connect to my PC\n>> when back to bootrom, after a long time checking whit the bootrom code author, we found the interrupts related configurations are changed.\n>> Then I try to save and restore the VBAR, then the bootrom download function works. But when I tested this function on rk3288 based board, it failed\n>> again, and I found rk3288 bootrom require a different interrupt configuration with rk3036 after a long time dig, the interrupts vector base should be\n>> 0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR should be set to 1, but the SPL startup code set it to zero.\n>> Then I have the idea back to bootrom download mode as early as possible when the download flag is set, Because no matter how many parameters\n>> I saved and restored today, no one can make sure that other parameters will not changed by start.S in the future(maybe more properties changed about\n>> the interrupt, maybe the change of mmu /caches, ), because we always have the chance to modify the startup code by the desire of new feature or the need\n>> to workaround something for a new soc, what's more the start.S it's now have many #if / #else configuration, this still have risk to change the default configurations\n>> which will be used in bootrom download mode. If we rely on a save and restore policy, this function may work well today, but may failed some days later just because\n>> some one changed another configuration in start.S.\n> When looking at what happens (on armv7) between the save_boot_params\n> and the call to _main (which in turn just sets up SP and calls into board_init_f),\n> there isn’t much happening: it really is only cpu_init_cp15 and cpu_init_crit\n> that will be of concern.\n\n The bellow codes changes the V bit and VBAR address, that are what \ncause problems on rockchip armv7 based boards.\n\n As for armv7, I haven't check what will cause problem.\n\n As bootrom is a very critical environment and very hard to debug, So \nI hope to keep it as clean as possible when we jump back to it.\n\n/*\n * Setup vector:\n * (OMAP4 spl TEXT_BASE is not 32 byte aligned.\n * Continue to use ROM code vector only in OMAP4 spl)\n */\n#if !(defined(CONFIG_OMAP44XX) && defined(CONFIG_SPL_BUILD))\n /* Set V=0 in CP15 SCTLR register - for VBAR to point to vector */\n mrc p15, 0, r0, c1, c0, 0 @ Read CP15 SCTLR Register\n bic r0, #CR_V @ V = 0\n mcr p15, 0, r0, c1, c0, 0 @ Write CP15 SCTLR Register\n\n /* Set vector address in CP15 VBAR register */\n ldr r0, =_start\n mcr p15, 0, r0, c12, c0, 0 @Set VBAR\n#endif\n\n> I really wonder whether we could have a sufficient C runtime (i.e. SP valid)\n> available before those run.\n>\n> Does your BROM always call us with the SP valid (I know that that’s the case\n> on the RK3399 and RK3368), so that we could run on the BROM’s stack here?\n>\n>\n>\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsbQQ6DRGz9sMN\n\tfor ;\n\tWed, 13 Sep 2017 19:07:09 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 2DEBFC225E0; Wed, 13 Sep 2017 09:07:04 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 48D28C21ED8;\n\tWed, 13 Sep 2017 09:07:02 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 33271C21ED8; Wed, 13 Sep 2017 09:07:00 +0000 (UTC)","from regular1.263xmail.com (regular1.263xmail.com [211.150.99.138])\n\tby lists.denx.de (Postfix) with ESMTPS id 0FA45C21D19\n\tfor ; Wed, 13 Sep 2017 09:06:59 +0000 (UTC)","from andy.yan?rock-chips.com (unknown [192.168.167.140])\n\tby regular1.263xmail.com (Postfix) with ESMTP id 3D7517A2C\n\tfor ; Wed, 13 Sep 2017 17:06:53 +0800 (CST)","from [172.16.12.133] (localhost [127.0.0.1])\n\tby smtp.263.net (Postfix) with ESMTPA id 6E2B7323;\n\tWed, 13 Sep 2017 17:06:54 +0800 (CST)","from [172.16.12.133] (unknown [58.22.7.114])\n\tby smtp.263.net (Postfix) whith ESMTP id 927671G7X5;\n\tWed, 13 Sep 2017 17:06:54 +0800 (CST)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=0.6 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H2,RCVD_IN_SORBS_WEB,T_FRT_BELOW2 autolearn=no\n\tautolearn_force=no version=3.4.0","X-263anti-spam":"KSV:0;","X-MAIL-GRAY":"0","X-MAIL-DELIVERY":"1","X-KSVirus-check":"0","X-ABS-CHECKED":"4","X-RL-SENDER":"andy.yan@rock-chips.com","X-FST-TO":"kever.yang@rock-chips.com","X-SENDER-IP":"58.22.7.114","X-LOGIN-NAME":"andy.yan@rock-chips.com","X-UNIQUE-TAG":"","X-ATTACHMENT-NUM":"0","X-SENDER":"yxj@rock-chips.com","X-DNS-TYPE":"0","To":"\"Dr. Philipp Tomsich\" ","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>\n\t\n\t\n\t\n\t\n\t<8CBB4CDC-285D-4C01-906B-447FACC5EDF5@theobroma-systems.com>","From":"Andy Yan ","Message-ID":"<84c68bf6-f2bb-2268-7d7c-cd4d7d98e5bb@rock-chips.com>","Date":"Wed, 13 Sep 2017 17:06:53 +0800","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<8CBB4CDC-285D-4C01-906B-447FACC5EDF5@theobroma-systems.com>","Content-Language":"en-US","Cc":"u-boot@lists.denx.de","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1767701,"web_url":"http://patchwork.ozlabs.org/comment/1767701/","msgid":"<39694314-23DE-4EAA-B91D-3F2A8EE91324@theobroma-systems.com>","list_archive_url":null,"date":"2017-09-13T09:17:54","subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","submitter":{"id":53488,"url":"http://patchwork.ozlabs.org/api/people/53488/","name":"Philipp Tomsich","email":"philipp.tomsich@theobroma-systems.com"},"content":"> On 13 Sep 2017, at 10:50, Dr. Philipp Tomsich wrote:\n> \n> \n>> On 13 Sep 2017, at 10:36, Andy Yan wrote:\n>> \n>>>>> \n>>>>> If you just always save the boot_params and check the download flag later from C code, then you could have this implemented in C. This will remove the need to write two separate assembly functions (for AArch64 and AArch32) and generally be more readable. Please revise.\n>>>> \n>>>> We can't predict how many settings the TPL/SPL startup code changed now and future\n>>>> will affect the bootrom download function, So back to bootrom download mode before\n>>>> anything been changed is a simple way.\n>>> \n>>> Ok. I’d still like to have this in C.\n>>> The only requirement for this will be having a valid stack-pointer, so we should be able to do this early (before the various initialisation runs).\n>>> I think board_init_f() would be a suitable place.\n>> \n>> \n>> When I hack this function first time, I indeed wrote a c implemented code in board_init_f on kylin-rk3036, but the usb failed connect to my PC\n>> when back to bootrom, after a long time checking whit the bootrom code author, we found the interrupts related configurations are changed.\n>> Then I try to save and restore the VBAR, then the bootrom download function works. But when I tested this function on rk3288 based board, it failed\n>> again, and I found rk3288 bootrom require a different interrupt configuration with rk3036 after a long time dig, the interrupts vector base should be\n>> 0xffff0000(other arm32 based boards are 0x0000), so the V bit of SCTLR should be set to 1, but the SPL startup code set it to zero.\n>> Then I have the idea back to bootrom download mode as early as possible when the download flag is set, Because no matter how many parameters \n>> I saved and restored today, no one can make sure that other parameters will not changed by start.S in the future(maybe more properties changed about\n>> the interrupt, maybe the change of mmu /caches, ), because we always have the chance to modify the startup code by the desire of new feature or the need\n>> to workaround something for a new soc, what's more the start.S it's now have many #if / #else configuration, this still have risk to change the default configurations\n>> which will be used in bootrom download mode. If we rely on a save and restore policy, this function may work well today, but may failed some days later just because \n>> some one changed another configuration in start.S. \n> \n> When looking at what happens (on armv7) between the save_boot_params\n> and the call to _main (which in turn just sets up SP and calls into board_init_f),\n> there isn’t much happening: it really is only cpu_init_cp15 and cpu_init_crit\n> that will be of concern.\n> I really wonder whether we could have a sufficient C runtime (i.e. SP valid)\n> available before those run.\n> \n> Does your BROM always call us with the SP valid (I know that that’s the case\n> on the RK3399 and RK3368), so that we could run on the BROM’s stack here?\n\nJust to explain my thinking here…\nIf we had a valid SP, we could mark our devices via Kconfig accordingly\nby introducing a new config (e.g. $(SPL_TPL_)BROM_PERFORMS_C_CALL\nfor this) and then do something along these lines:\n\nin start.S:\n#if CONFIG_IS_ENABLED(BROM_PERFORMS_C_CALL)\n\tb brom_handoff_f\n\t.globl brom_handoff_f_ret\nbrom_handoff_f_ret:\n#endif\n\nand have a C file implementing the following (if setjmp/longjmp is not\nyet implemented for AArch32 and AArch64, this will be a quick exercise):\n\n/* The following is functionally identical to mach-rockchip/save_boot_param.S */\njmp_buf* brom_ctx = NULL;\n\nenum {\n\tBROM_SETJMP_CONTINUE = 0\n\tBROM_RETURN_NORMAL = 1,\n\tBROM_RETURN_DNL = 2,\n}\n\nint brom_handoff_t(void)\n{\n\tjmp_buf saved_brom_ctx;\n\tint res ;\n\n\tres = setjmp(saved_brom_ctx);\n\tswitch (res) {\n\tcase BROM_SETJMP_CONTINUE:\n\t\tbrom_ctx = &saved_brom_ctx;\n\t\tres = brom_handoff_f_ret();\t\t/* needs a better name */\n\t\tbreak;\n\n\tcase BROM_RETURN_NORMAL:\n\t\tres = 0;\n\t\tbreak;\n\n\tcase BROM_RETURN_DNL:\n\t\tres = MAGIC_VALUE_TO_PASS_BACK;\n\t\tbreak;\n\t}\n\n\treturn res;\n}\n\nvoid back_to_bootrom(void)\n{\n\tlongjmp(*brom_ctx, BROM_RETURN_NORMAL);\n}","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsbg41YQMz9sNr\n\tfor ;\n\tWed, 13 Sep 2017 19:18:08 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid E7D53C21F16; Wed, 13 Sep 2017 09:18:04 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id C6130C21ED8;\n\tWed, 13 Sep 2017 09:18:01 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid 69C2FC21ED8; Wed, 13 Sep 2017 09:17:59 +0000 (UTC)","from mail.theobroma-systems.com (vegas.theobroma-systems.com\n\t[144.76.126.164])\n\tby lists.denx.de (Postfix) with ESMTPS id 2AFDDC21D19\n\tfor ; Wed, 13 Sep 2017 09:17:59 +0000 (UTC)","from 89-104-28-141.customer.bnet.at ([89.104.28.141]:55638\n\thelo=[192.168.2.129]) by mail.theobroma-systems.com with esmtpsa\n\t(TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80)\n\t(envelope-from )\n\tid 1ds3nv-0008Qd-DB; Wed, 13 Sep 2017 11:17:55 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"*","X-Spam-Status":"No, score=1.0 required=5.0 tests=HK_NAME_DR autolearn=no\n\tautolearn_force=no version=3.4.0","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","From":"\"Dr. Philipp Tomsich\" ","In-Reply-To":"<8CBB4CDC-285D-4C01-906B-447FACC5EDF5@theobroma-systems.com>","Date":"Wed, 13 Sep 2017 11:17:54 +0200","Message-Id":"<39694314-23DE-4EAA-B91D-3F2A8EE91324@theobroma-systems.com>","References":"<1505111271-17489-1-git-send-email-andy.yan@rock-chips.com>\n\t\n\t\n\t\n\t\n\t<8CBB4CDC-285D-4C01-906B-447FACC5EDF5@theobroma-systems.com>","To":"Andy Yan ","X-Mailer":"Apple Mail (2.3273)","Cc":"U-Boot Mailing List , Klaus Goger\n\t, =?utf-8?q?Christoph_M=C3=BCllner?=\n\t","Subject":"Re: [U-Boot] rockchip: add support for backing to bootrom download\n\tmode","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}}]