[PATCH 0/2] Command for entering mask rom USB download mode

[PATCH 0/2] Command for entering mask rom USB download mode

[Michal Suchanek]

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

-- 
2.36.1

[PATCH 1/2] cmd: boot: add brom cmd to reboot to brom dnl mode

[Michal Suchanek]

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(
-- 
2.36.1

[PATCH 2/2] cmd: boot: add brom cmd to reboot to FEL mode

[Michal Suchanek]

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",
-- 
2.36.1

Re: [PATCH 2/2] cmd: boot: add brom cmd to reboot to FEL mode

[Andre Przywara]

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",

Re: [PATCH 2/2] cmd: boot: add brom cmd to reboot to FEL mode

[Michal Suchánek]

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

Re: [PATCH 0/2] Command for entering mask rom USB download mode

[Andre Przywara]

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

Re: [PATCH 0/2] Command for entering mask rom USB download mode

[Michal Suchánek]

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

Re: [PATCH 0/2] Command for entering mask rom USB download mode

[Samuel Holland]

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

Re: [PATCH 0/2] Command for entering mask rom USB download mode

[Michal Suchánek]

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