[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 at 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 at 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