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

[Andre Przywara <andre.przywara@arm.com>]

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