[PATCH 0/2] Command for entering mask rom USB download mode
Hello,
many ARM SoCs have a mask rom feature that provides interface for downloading firmware over USB.
Downstream rockchip u-boot has 'brom' or 'rbrom' command for this purpose, and downstream sunxi u-boot provides 'efex' command. p-boot has code for entering FEL on A64 SoC.
With this patch I am able to activate the USB downloader on a rk3399 but the rkflashtool fails to communicate with the device. On a H2+ I can get into the FEL mode and get flash parameters. YMMV
I don't have any great idea how to structure this so that the command does not need platform-specific code. Is there an example of a command that has platform-specific implementations?
Thanks
Michal
Andy Yan (1): cmd: boot: add brom cmd to reboot to brom dnl mode
Michal Suchanek (1): cmd: boot: add brom cmd to reboot to FEL mode
.../arm/include/asm/arch-rockchip/boot_mode.h | 1 + arch/arm/include/asm/arch-sunxi/cpu.h | 11 ++++++ arch/arm/mach-sunxi/Kconfig | 18 ++++++++++ arch/arm/mach-sunxi/board.c | 24 +++++++++++++ cmd/boot.c | 35 +++++++++++++++++++ 5 files changed, 89 insertions(+)
From: Andy Yan andy.yan@rock-chips.com
Change-Id: I797491ebe25af1013732aeee87e61e3ba4bc1689 Signed-off-by: Andy Yan andy.yan@rock-chips.com Signed-off-by: Michal Suchanek msuchanek@suse.de --- .../arm/include/asm/arch-rockchip/boot_mode.h | 1 + cmd/boot.c | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+)
diff --git a/arch/arm/include/asm/arch-rockchip/boot_mode.h b/arch/arm/include/asm/arch-rockchip/boot_mode.h index 6b2a610cf4..bcdf4420cf 100644 --- a/arch/arm/include/asm/arch-rockchip/boot_mode.h +++ b/arch/arm/include/asm/arch-rockchip/boot_mode.h @@ -19,6 +19,7 @@ #define BOOT_BROM_DOWNLOAD 0xEF08A53C
#ifndef __ASSEMBLY__ +void set_back_to_bootrom_dnl_flag(void); int setup_boot_mode(void); #endif
diff --git a/cmd/boot.c b/cmd/boot.c index be67a5980d..d48c0bf1b3 100644 --- a/cmd/boot.c +++ b/cmd/boot.c @@ -43,16 +43,36 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) printf ("## Application terminated, rc = 0x%lX\n", rc); return rcode; } +#endif + +#if defined(CONFIG_ROCKCHIP_BOOT_MODE_REG) && CONFIG_ROCKCHIP_BOOT_MODE_REG +#include <asm/arch-rockchip/boot_mode.h> +static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) +{ + set_back_to_bootrom_dnl_flag(); + do_reset(NULL, 0, 0, NULL); + + return 0; +} +#endif
/* -------------------------------------------------------------------- */
+#ifdef CONFIG_CMD_GO U_BOOT_CMD( go, CONFIG_SYS_MAXARGS, 1, do_go, "start application at address 'addr'", "addr [arg ...]\n - start application at address 'addr'\n" " passing 'arg' as arguments" ); +#endif
+#if defined(CONFIG_ROCKCHIP_BOOT_MODE_REG) && CONFIG_ROCKCHIP_BOOT_MODE_REG +U_BOOT_CMD( + rbrom, 1, 0, do_reboot_brom, + "Perform RESET of the CPU and enter boot rom", + "" +); #endif
U_BOOT_CMD(
p-boot uses RTC GPR 1 value 0xb0010fe1 to flag FEL boot on A64
Default to the same.
Signed-off-by: Michal Suchanek msuchanek@suse.de --- arch/arm/include/asm/arch-sunxi/cpu.h | 11 +++++++++++ arch/arm/mach-sunxi/Kconfig | 18 ++++++++++++++++++ arch/arm/mach-sunxi/board.c | 24 ++++++++++++++++++++++++ cmd/boot.c | 17 ++++++++++++++++- 4 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/arch-sunxi/cpu.h b/arch/arm/include/asm/arch-sunxi/cpu.h index b08f202374..36e7697b1c 100644 --- a/arch/arm/include/asm/arch-sunxi/cpu.h +++ b/arch/arm/include/asm/arch-sunxi/cpu.h @@ -20,4 +20,15 @@ #define SOCID_H5 0x1718 #define SOCID_R40 0x1701
+#if defined(CONFIG_SUNXI_RTC_FEL_ENTRY_GPR) && (CONFIG_SUNXI_RTC_FEL_ENTRY_GPR >= 0) +#ifdef CONFIG_MACH_SUN8I_H3 +#define SUNXI_FEL_ENTRY_ADDRESS 0xffff0020 +#define SUNXI_RTC_GPR_OFFSET 0x100 +#define SUNXI_FEL_REG (SUNXI_RTC_BASE + SUNXI_RTC_GPR_OFFSET + CONFIG_SUNXI_RTC_FEL_ENTRY_GPR * 4) +#endif +#endif +#ifndef __ASSEMBLY__ +void set_rtc_fel_flag(void); +#endif + #endif /* _SUNXI_CPU_H */ diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index e712a89534..10645fc644 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -1048,6 +1048,24 @@ source "board/sunxi/Kconfig"
endif
+if MACH_SUN8I_H3 || MACH_SUN50I +config SUNXI_RTC_FEL_ENTRY_GPR + int "Use a RTC GPR to enter FEL" + range -1 7 if MACH_SUN8I_H3 + range -1 3 if MACH_SUN50I + default 1 + help + Add rbrom command to set a RTC general purpose register before reboot. + Check the GPR value in SPL and jump to FEL if set. + Value -1 disables the feature. + +config SUNXI_RTC_FEL_ENTRY_VALUE + hex "Value to set in the RTC GPR" + depends on SUNXI_RTC_FEL_ENTRY_GPR >= 0 + range 0x1 0xffffffff + default 0xb0010fe1 +endif + config CHIP_DIP_SCAN bool "Enable DIPs detection for CHIP board" select SUPPORT_EXTENSION_SCAN diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c index 8f7c894286..91866a3be6 100644 --- a/arch/arm/mach-sunxi/board.c +++ b/arch/arm/mach-sunxi/board.c @@ -297,7 +297,30 @@ uint32_t sunxi_get_boot_device(void) return -1; /* Never reached */ }
+void set_rtc_fel_flag(void) +{ +#ifdef SUNXI_FEL_REG + volatile long *check_reg = (void *)SUNXI_FEL_REG; + + *check_reg = CONFIG_SUNXI_RTC_FEL_ENTRY_VALUE; +#endif +} + #ifdef CONFIG_SPL_BUILD + +void check_rtc_fel_flag(void) +{ +#ifdef SUNXI_FEL_REG + volatile long *check_reg = (void *)SUNXI_FEL_REG; + void (*entry)(void) = (void*)SUNXI_FEL_ENTRY_ADDRESS; + + if (*check_reg == CONFIG_SUNXI_RTC_FEL_ENTRY_VALUE) { + *check_reg = 0; + return entry(); + } +#endif +} + uint32_t sunxi_get_spl_size(void) { struct boot_file_head *egon_head = (void *)SPL_ADDR; @@ -432,6 +455,7 @@ u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device)
void board_init_f(ulong dummy) { + check_rtc_fel_flag(); sunxi_sram_init();
#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I_H3 diff --git a/cmd/boot.c b/cmd/boot.c index d48c0bf1b3..c870e6a217 100644 --- a/cmd/boot.c +++ b/cmd/boot.c @@ -46,6 +46,7 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) #endif
#if defined(CONFIG_ROCKCHIP_BOOT_MODE_REG) && CONFIG_ROCKCHIP_BOOT_MODE_REG +#define RBROM #include <asm/arch-rockchip/boot_mode.h> static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) { @@ -56,6 +57,20 @@ static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * cons } #endif
+#ifdef CONFIG_ARCH_SUNXI +#include <asm/arch-sunxi/cpu.h> +#ifdef SUNXI_FEL_REG +#define RBROM +static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) +{ + set_rtc_fel_flag(); + do_reset(NULL, 0, 0, NULL); + + return 0; +} +#endif +#endif + /* -------------------------------------------------------------------- */
#ifdef CONFIG_CMD_GO @@ -67,7 +82,7 @@ U_BOOT_CMD( ); #endif
-#if defined(CONFIG_ROCKCHIP_BOOT_MODE_REG) && CONFIG_ROCKCHIP_BOOT_MODE_REG +#ifdef RBROM U_BOOT_CMD( rbrom, 1, 0, do_reboot_brom, "Perform RESET of the CPU and enter boot rom",
On Sun, 3 Jul 2022 21:20:22 +0200 Michal Suchanek msuchanek@suse.de wrote:
Hi Michal,
p-boot uses RTC GPR 1 value 0xb0010fe1 to flag FEL boot on A64
Default to the same.
Please don't add any more #ifdef's to U-Boot, especially no nested ones, there are already far too many.
Signed-off-by: Michal Suchanek msuchanek@suse.de
arch/arm/include/asm/arch-sunxi/cpu.h | 11 +++++++++++ arch/arm/mach-sunxi/Kconfig | 18 ++++++++++++++++++ arch/arm/mach-sunxi/board.c | 24 ++++++++++++++++++++++++ cmd/boot.c | 17 ++++++++++++++++- 4 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/arch-sunxi/cpu.h b/arch/arm/include/asm/arch-sunxi/cpu.h index b08f202374..36e7697b1c 100644 --- a/arch/arm/include/asm/arch-sunxi/cpu.h +++ b/arch/arm/include/asm/arch-sunxi/cpu.h @@ -20,4 +20,15 @@ #define SOCID_H5 0x1718 #define SOCID_R40 0x1701
+#if defined(CONFIG_SUNXI_RTC_FEL_ENTRY_GPR) && (CONFIG_SUNXI_RTC_FEL_ENTRY_GPR >= 0) +#ifdef CONFIG_MACH_SUN8I_H3 +#define SUNXI_FEL_ENTRY_ADDRESS 0xffff0020
That is actually SUNXI_BROM_BASE + 0x20, regardless of the SoC. Only that the SUNXI_BROM_BASE is wrong for newer SoC, but anyway ...
+#define SUNXI_RTC_GPR_OFFSET 0x100 +#define SUNXI_FEL_REG (SUNXI_RTC_BASE + SUNXI_RTC_GPR_OFFSET + CONFIG_SUNXI_RTC_FEL_ENTRY_GPR * 4) +#endif +#endif
In general there is no need to protect #defines, unless you actually depend on another symbol. More importantly, as there is only one user, in a platform specific command, those defines are better held in the implementation. cpu*.h is actually legacy code, and I have patches to cut it down significantly, eventually possibly removing it altogether.
+#ifndef __ASSEMBLY__ +void set_rtc_fel_flag(void); +#endif
#endif /* _SUNXI_CPU_H */ diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index e712a89534..10645fc644 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -1048,6 +1048,24 @@ source "board/sunxi/Kconfig"
endif
+if MACH_SUN8I_H3 || MACH_SUN50I
I don't think if's in Kconfig are customary. Just use "depends on" inside.
+config SUNXI_RTC_FEL_ENTRY_GPR
- int "Use a RTC GPR to enter FEL"
- range -1 7 if MACH_SUN8I_H3
- range -1 3 if MACH_SUN50I
The current code does not work easily with 64-bit SoCs, so we can add this later. But also IIRC there are actually eight GPRs in the A64 RTC, despite what the manual says, as someone found out by experimentation.
- default 1
- help
Add rbrom command to set a RTC general purpose register before reboot.Check the GPR value in SPL and jump to FEL if set.Value -1 disables the feature.+config SUNXI_RTC_FEL_ENTRY_VALUE
- hex "Value to set in the RTC GPR"
- depends on SUNXI_RTC_FEL_ENTRY_GPR >= 0
- range 0x1 0xffffffff
- default 0xb0010fe1
+endif
config CHIP_DIP_SCAN bool "Enable DIPs detection for CHIP board" select SUPPORT_EXTENSION_SCAN diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c index 8f7c894286..91866a3be6 100644 --- a/arch/arm/mach-sunxi/board.c +++ b/arch/arm/mach-sunxi/board.c @@ -297,7 +297,30 @@ uint32_t sunxi_get_boot_device(void) return -1; /* Never reached */ }
+void set_rtc_fel_flag(void) +{ +#ifdef SUNXI_FEL_REG
- volatile long *check_reg = (void *)SUNXI_FEL_REG;
- *check_reg = CONFIG_SUNXI_RTC_FEL_ENTRY_VALUE;
Please don't let the compiler write to an MMIO device. We have writel for that purpose.
+#endif +}
#ifdef CONFIG_SPL_BUILD
+void check_rtc_fel_flag(void) +{ +#ifdef SUNXI_FEL_REG
- volatile long *check_reg = (void *)SUNXI_FEL_REG;
- void (*entry)(void) = (void*)SUNXI_FEL_ENTRY_ADDRESS;
- if (*check_reg == CONFIG_SUNXI_RTC_FEL_ENTRY_VALUE) {
Same here, readl() please.
*check_reg = 0;return entry();- }
+#endif +}
uint32_t sunxi_get_spl_size(void) { struct boot_file_head *egon_head = (void *)SPL_ADDR; @@ -432,6 +455,7 @@ u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device)
void board_init_f(ulong dummy) {
- check_rtc_fel_flag(); sunxi_sram_init();
#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I_H3 diff --git a/cmd/boot.c b/cmd/boot.c index d48c0bf1b3..c870e6a217 100644 --- a/cmd/boot.c +++ b/cmd/boot.c @@ -46,6 +46,7 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) #endif
#if defined(CONFIG_ROCKCHIP_BOOT_MODE_REG) && CONFIG_ROCKCHIP_BOOT_MODE_REG +#define RBROM #include <asm/arch-rockchip/boot_mode.h> static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) { @@ -56,6 +57,20 @@ static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * cons } #endif
+#ifdef CONFIG_ARCH_SUNXI +#include <asm/arch-sunxi/cpu.h> +#ifdef SUNXI_FEL_REG +#define RBROM +static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) +{
- set_rtc_fel_flag();
- do_reset(NULL, 0, 0, NULL);
- return 0;
+} +#endif +#endif
Please no nested #ifdefs. You can lose one of them by just putting the code in an extra file, and making this depend on ARCH_.. in Kconfig.
So I would suggest you move your #define's from cpu.h into this new file. Also the definition of set_rtc_fel_flag(), that saves you the need for the prototype there as well.
And since this is U-Boot proper code, we can avoid most of those SoC specific defines anyway, by looking up the address of the RTC in the DT. Can you try: uclass_find_device_by_name(UCLASS_CLK, "clk_sun6i_rtc", &dev), then maybe devfdt_get_addr(dev)? I think that should give you the RTC address in a better way.
Cheers, Andre
/* -------------------------------------------------------------------- */
#ifdef CONFIG_CMD_GO @@ -67,7 +82,7 @@ U_BOOT_CMD( ); #endif
-#if defined(CONFIG_ROCKCHIP_BOOT_MODE_REG) && CONFIG_ROCKCHIP_BOOT_MODE_REG +#ifdef RBROM U_BOOT_CMD( rbrom, 1, 0, do_reboot_brom, "Perform RESET of the CPU and enter boot rom",
Hello,
On Sun, Jul 03, 2022 at 11:23:26PM +0100, Andre Przywara wrote:
On Sun, 3 Jul 2022 21:20:22 +0200 Michal Suchanek msuchanek@suse.de wrote:
Hi Michal,
p-boot uses RTC GPR 1 value 0xb0010fe1 to flag FEL boot on A64
Default to the same.
Please don't add any more #ifdef's to U-Boot, especially no nested ones, there are already far too many.
I'm sure that the ifdefs can be reduced if the code is cleaned up.
Signed-off-by: Michal Suchanek msuchanek@suse.de
arch/arm/include/asm/arch-sunxi/cpu.h | 11 +++++++++++ arch/arm/mach-sunxi/Kconfig | 18 ++++++++++++++++++ arch/arm/mach-sunxi/board.c | 24 ++++++++++++++++++++++++ cmd/boot.c | 17 ++++++++++++++++- 4 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/arch-sunxi/cpu.h b/arch/arm/include/asm/arch-sunxi/cpu.h index b08f202374..36e7697b1c 100644 --- a/arch/arm/include/asm/arch-sunxi/cpu.h +++ b/arch/arm/include/asm/arch-sunxi/cpu.h @@ -20,4 +20,15 @@ #define SOCID_H5 0x1718 #define SOCID_R40 0x1701
+#if defined(CONFIG_SUNXI_RTC_FEL_ENTRY_GPR) && (CONFIG_SUNXI_RTC_FEL_ENTRY_GPR >= 0) +#ifdef CONFIG_MACH_SUN8I_H3 +#define SUNXI_FEL_ENTRY_ADDRESS 0xffff0020
That is actually SUNXI_BROM_BASE + 0x20, regardless of the SoC. Only that the SUNXI_BROM_BASE is wrong for newer SoC, but anyway ...
Which means that it needs to be defined, anyway.
+#define SUNXI_RTC_GPR_OFFSET 0x100 +#define SUNXI_FEL_REG (SUNXI_RTC_BASE + SUNXI_RTC_GPR_OFFSET + CONFIG_SUNXI_RTC_FEL_ENTRY_GPR * 4) +#endif +#endif
In general there is no need to protect #defines, unless you actually depend on another symbol. More importantly, as there is only one user,
I don't want to define SUNXI_FEL_REG when CONFIG_SUNXI_RTC_FEL_ENTRY_GPR is -1, and I want to detect the feature availability which is currently done by detecting SUNXI_FEL_REG.
in a platform specific command, those defines are better held in the implementation. cpu*.h is actually legacy code, and I have patches to cut it down significantly, eventually possibly removing it altogether.
The implementation has multiple parts, and SUNXI_FEL_REG is used by all of them. Also SUNXI_RTC_BASE is defined in cpu.h so anything that depends on it logically belongs there.
+#ifndef __ASSEMBLY__ +void set_rtc_fel_flag(void); +#endif
#endif /* _SUNXI_CPU_H */ diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index e712a89534..10645fc644 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -1048,6 +1048,24 @@ source "board/sunxi/Kconfig"
endif
+if MACH_SUN8I_H3 || MACH_SUN50I
I don't think if's in Kconfig are customary. Just use "depends on" inside.
Sure, then there is no need for matching endif then, easier to maintain.
+config SUNXI_RTC_FEL_ENTRY_GPR
- int "Use a RTC GPR to enter FEL"
- range -1 7 if MACH_SUN8I_H3
- range -1 3 if MACH_SUN50I
The current code does not work easily with 64-bit SoCs, so we can add this later.
IS the problem that you need to run FEL in 32bit but this code runs in 64bit?
But also IIRC there are actually eight GPRs in the A64 RTC, despite what the manual says, as someone found out by experimentation.
- default 1
- help
Add rbrom command to set a RTC general purpose register before reboot.Check the GPR value in SPL and jump to FEL if set.Value -1 disables the feature.+config SUNXI_RTC_FEL_ENTRY_VALUE
- hex "Value to set in the RTC GPR"
- depends on SUNXI_RTC_FEL_ENTRY_GPR >= 0
- range 0x1 0xffffffff
- default 0xb0010fe1
+endif
config CHIP_DIP_SCAN bool "Enable DIPs detection for CHIP board" select SUPPORT_EXTENSION_SCAN diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c index 8f7c894286..91866a3be6 100644 --- a/arch/arm/mach-sunxi/board.c +++ b/arch/arm/mach-sunxi/board.c @@ -297,7 +297,30 @@ uint32_t sunxi_get_boot_device(void) return -1; /* Never reached */ }
+void set_rtc_fel_flag(void) +{ +#ifdef SUNXI_FEL_REG
- volatile long *check_reg = (void *)SUNXI_FEL_REG;
- *check_reg = CONFIG_SUNXI_RTC_FEL_ENTRY_VALUE;
Please don't let the compiler write to an MMIO device. We have writel for that purpose.
Right, it's an interesting question if writing only part of the register would work but this is not the code where it should be tested.
+#endif +}
#ifdef CONFIG_SPL_BUILD
+void check_rtc_fel_flag(void) +{ +#ifdef SUNXI_FEL_REG
- volatile long *check_reg = (void *)SUNXI_FEL_REG;
- void (*entry)(void) = (void*)SUNXI_FEL_ENTRY_ADDRESS;
- if (*check_reg == CONFIG_SUNXI_RTC_FEL_ENTRY_VALUE) {
Same here, readl() please.
*check_reg = 0;return entry();- }
+#endif +}
uint32_t sunxi_get_spl_size(void) { struct boot_file_head *egon_head = (void *)SPL_ADDR; @@ -432,6 +455,7 @@ u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device)
void board_init_f(ulong dummy) {
- check_rtc_fel_flag(); sunxi_sram_init();
#if defined CONFIG_MACH_SUN6I || defined CONFIG_MACH_SUN8I_H3 diff --git a/cmd/boot.c b/cmd/boot.c index d48c0bf1b3..c870e6a217 100644 --- a/cmd/boot.c +++ b/cmd/boot.c @@ -46,6 +46,7 @@ static int do_go(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) #endif
#if defined(CONFIG_ROCKCHIP_BOOT_MODE_REG) && CONFIG_ROCKCHIP_BOOT_MODE_REG +#define RBROM #include <asm/arch-rockchip/boot_mode.h> static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) { @@ -56,6 +57,20 @@ static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * cons } #endif
+#ifdef CONFIG_ARCH_SUNXI +#include <asm/arch-sunxi/cpu.h> +#ifdef SUNXI_FEL_REG +#define RBROM +static int do_reboot_brom(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]) +{
- set_rtc_fel_flag();
- do_reset(NULL, 0, 0, NULL);
- return 0;
+} +#endif +#endif
Please no nested #ifdefs. You can lose one of them by just putting the code in an extra file, and making this depend on ARCH_.. in Kconfig.
Yes, some of the defines can be moved into kconfig.
So I would suggest you move your #define's from cpu.h into this new file. Also the definition of set_rtc_fel_flag(), that saves you the need for the prototype there as well.
I don't expect this to ever work out this way across multiple platforms.
Sure, you can put the set code here and the test code into platform code but that's not great for maintainability, and breeds ifdefs.
On the other hand, if this function is renamed to the same symbola across platforms you can share the command code here.
And since this is U-Boot proper code, we can avoid most of those SoC specific defines anyway, by looking up the address of the RTC in the DT. Can you try: uclass_find_device_by_name(UCLASS_CLK, "clk_sun6i_rtc", &dev), then maybe devfdt_get_addr(dev)? I think that should give you the RTC address in a better way.
Interestingly u-boot has rtc-sun6i but not rtc-sunxi so this will likely not work on sun4i and sun7i. Also the test is supposed to happen early in SPL, and there you may not have the device tree. Doubly so if it is required to happen in 32bit assembly on 64bit.
Thanks
Michal
On Sun, 3 Jul 2022 21:20:20 +0200 Michal Suchanek msuchanek@suse.de wrote:
Hi,
many ARM SoCs have a mask rom feature that provides interface for downloading firmware over USB.
Downstream rockchip u-boot has 'brom' or 'rbrom' command for this purpose, and downstream sunxi u-boot provides 'efex' command. p-boot has code for entering FEL on A64 SoC.
Thanks for bringing this up. We have discussed several options before, including some true FEL reset commands, which do not depend on the SPL doing the branch. But I guess we can just start with this one and expand it from there.
With this patch I am able to activate the USB downloader on a rk3399 but the rkflashtool fails to communicate with the device. On a H2+ I can get into the FEL mode and get flash parameters. YMMV
I don't have any great idea how to structure this so that the command does not need platform-specific code. Is there an example of a command that has platform-specific implementations?
I don't think that's a problem: there are already platform specific commands, you just put them in a separate file and mark them as "depends on ARCH_SUNXI" (or whatever) in cmd/Kconfig and be done. No need to boil the ocean here in trying to be generic. That's why I wouldn't put them in cmd/boot.c, especially since you don't seem to share any code?
For more details see the reply to the actual (sunxi) patch.
Cheers, Andre
Thanks
Michal
Andy Yan (1): cmd: boot: add brom cmd to reboot to brom dnl mode
Michal Suchanek (1): cmd: boot: add brom cmd to reboot to FEL mode
.../arm/include/asm/arch-rockchip/boot_mode.h | 1 + arch/arm/include/asm/arch-sunxi/cpu.h | 11 ++++++ arch/arm/mach-sunxi/Kconfig | 18 ++++++++++ arch/arm/mach-sunxi/board.c | 24 +++++++++++++ cmd/boot.c | 35 +++++++++++++++++++ 5 files changed, 89 insertions(+)
On Sun, Jul 03, 2022 at 11:22:51PM +0100, Andre Przywara wrote:
On Sun, 3 Jul 2022 21:20:20 +0200 Michal Suchanek msuchanek@suse.de wrote:
Hi,
many ARM SoCs have a mask rom feature that provides interface for downloading firmware over USB.
Downstream rockchip u-boot has 'brom' or 'rbrom' command for this purpose, and downstream sunxi u-boot provides 'efex' command. p-boot has code for entering FEL on A64 SoC.
Thanks for bringing this up. We have discussed several options before, including some true FEL reset commands, which do not depend on the SPL doing the branch. But I guess we can just start with this one and expand it from there.
With this patch I am able to activate the USB downloader on a rk3399 but the rkflashtool fails to communicate with the device. On a H2+ I can get into the FEL mode and get flash parameters. YMMV
I don't have any great idea how to structure this so that the command does not need platform-specific code. Is there an example of a command that has platform-specific implementations?
I don't think that's a problem: there are already platform specific commands, you just put them in a separate file and mark them as "depends on ARCH_SUNXI" (or whatever) in cmd/Kconfig and be done. No need to boil the ocean here in trying to be generic. That's why I wouldn't put them in cmd/boot.c, especially since you don't seem to share any code?
The reset implementation also does not share any actual code between platforms, has different implementation on each platform, and is cross-platform command that provides the same functionality on each paltform.
Because the functionality can be provided on multiple platfroms the goal is to provide cross-platform user interface for it.
Thanks
Michal
Hi Michal,
On 7/3/22 2:20 PM, Michal Suchanek wrote:
Hello,
many ARM SoCs have a mask rom feature that provides interface for downloading firmware over USB.
Downstream rockchip u-boot has 'brom' or 'rbrom' command for this purpose, and downstream sunxi u-boot provides 'efex' command. p-boot has code for entering FEL on A64 SoC.
With this patch I am able to activate the USB downloader on a rk3399 but the rkflashtool fails to communicate with the device. On a H2+ I can get into the FEL mode and get flash parameters. YMMV
I don't have any great idea how to structure this so that the command does not need platform-specific code. Is there an example of a command that has platform-specific implementations?
Generally you would do this by having the driver interact with some DM uclass. Platforms may or may not provide a driver implementing that uclass; the details are hidden behind the uclass's interface. All the command has to do is search for a DM device by uclass, and if one is found, call some operation on it.
In this case, UCLASS_REBOOT_MODE already exists. It sounds like what we need is the inverse of dm_reboot_mode_update() -- a function that takes a string and programs the device with the corresponding mode value.
To be generic, you could call this function from do_reset() if some positional argument is provided, allowing usage like "reset bootloader" or "reset fel".
Rockchip platforms already have syscon-reboot-mode hooked up in their devicetrees. The driver for this should be trivial.
Allwinner platforms would need to add a nvmem-reboot-mode node, and a bit more driver infrastructure. U-Boot does not have NVMEM uclass, so you would need to add one or reuse UCLASS_MISC.
Regards, Samuel
On Sun, Jul 03, 2022 at 06:26:22PM -0500, Samuel Holland wrote:
Hi Michal,
On 7/3/22 2:20 PM, Michal Suchanek wrote:
Hello,
many ARM SoCs have a mask rom feature that provides interface for downloading firmware over USB.
Downstream rockchip u-boot has 'brom' or 'rbrom' command for this purpose, and downstream sunxi u-boot provides 'efex' command. p-boot has code for entering FEL on A64 SoC.
With this patch I am able to activate the USB downloader on a rk3399 but the rkflashtool fails to communicate with the device. On a H2+ I can get into the FEL mode and get flash parameters. YMMV
I don't have any great idea how to structure this so that the command does not need platform-specific code. Is there an example of a command that has platform-specific implementations?
Generally you would do this by having the driver interact with some DM uclass. Platforms may or may not provide a driver implementing that uclass; the details are hidden behind the uclass's interface. All the command has to do is search for a DM device by uclass, and if one is found, call some operation on it.
In this case, UCLASS_REBOOT_MODE already exists. It sounds like what we need is the inverse of dm_reboot_mode_update() -- a function that takes a string and programs the device with the corresponding mode value.
To be generic, you could call this function from do_reset() if some positional argument is provided, allowing usage like "reset bootloader" or "reset fel".
Rockchip platforms already have syscon-reboot-mode hooked up in their devicetrees. The driver for this should be trivial.
Allwinner platforms would need to add a nvmem-reboot-mode node, and a bit more driver infrastructure. U-Boot does not have NVMEM uclass, so you would need to add one or reuse UCLASS_MISC.
Not sure I understand the DM enough but as firt step I can at least hook it to the do_reset function.
Thanks
Michal
participants (4)
-
Andre Przywara -
Michal Suchanek -
Michal Suchánek
-
Samuel Holland