[U-Boot] [PATCH v5 2/4] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 11/7/19 3:10 AM, Ley Foon Tan wrote:
> Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> to defines.
> 
> Change to get reset manager base address from DT node instead of using
> #define.

It seems the patch also moves spl_early_init() around ?

[...]

> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> index 6ad037e325..a5b6931350 100644
> --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> @@ -6,6 +6,8 @@
>  #ifndef _RESET_MANAGER_H_
>  #define _RESET_MANAGER_H_
>  
> +extern phys_addr_t socfpga_rstmgr_base;

Would it make sense to implement a getter function which would return
the reset manager base address , instead of using the extern ?

[...]

> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-socfpga/misc.c
> index 49dadd4c3d..901c432f82 100644
> --- a/arch/arm/mach-socfpga/misc.c
> +++ b/arch/arm/mach-socfpga/misc.c
> @@ -22,6 +22,8 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +phys_addr_t socfpga_rstmgr_base __section(".data");
> +
>  #ifdef CONFIG_SYS_L2_PL310
>  static const struct pl310_regs *const pl310 =
>  	(struct pl310_regs *)CONFIG_SYS_PL310_BASE;
> @@ -145,6 +147,8 @@ void socfpga_fpga_add(void *fpga_desc)
>  
>  int arch_cpu_init(void)
>  {
> +	socfpga_get_manager_addr();
> +
>  #ifdef CONFIG_HW_WATCHDOG
>  	/*
>  	 * In case the watchdog is enabled, make sure to (re-)configure it
> @@ -202,3 +206,31 @@ U_BOOT_CMD(bridge, 3, 1, do_bridge,
>  );
>  
>  #endif
> +
> +static phys_addr_t socfpga_get_base_addr(const char *compat)
> +{
> +	const void *blob = gd->fdt_blob;
> +	struct fdt_resource r;
> +	int node;
> +	int ret;
> +
> +	node = fdt_node_offset_by_compatible(blob, -1, compat);
> +	if (node < 0)
> +		return 0;

0 is a valid address, so you want to discern 0 from errors here. I think
if you change the prototype of the function to e.g.
static int socfpga_get_rstmgr_base_addr(const char *compat, phys_addr_t
*base) {} , it should work. Then you can return error values as well as
the base address.

> +	if (!fdtdec_get_is_enabled(blob, node))
> +		return 0;
> +
> +	ret = fdt_get_resource(blob, node, "reg", 0, &r);
> +	if (ret)
> +		return 0;
> +
> +	return (phys_addr_t)r.start;
> +}
> +
> +void socfpga_get_manager_addr(void)

You should rename this function, a lot of blocks on the Gen5 are called
<something>-manager .

> +{
> +	socfpga_rstmgr_base = socfpga_get_base_addr("altr,rst-mgr");
> +	if (!socfpga_rstmgr_base)
> +		hang();
> +}
[...]

> diff --git a/arch/arm/mach-socfpga/spl_a10.c b/arch/arm/mach-socfpga/spl_a10.c
> index b820cb0673..a0d80fd47e 100644
> --- a/arch/arm/mach-socfpga/spl_a10.c
> +++ b/arch/arm/mach-socfpga/spl_a10.c
> @@ -106,6 +106,11 @@ void spl_board_init(void)
>  
>  void board_init_f(ulong dummy)
>  {
> +	if (spl_early_init())
> +		hang();
> +
> +	socfpga_get_manager_addr();
> +
>  	dcache_disable();
>  
>  	socfpga_init_security_policies();
> @@ -116,8 +121,6 @@ void board_init_f(ulong dummy)
>  	socfpga_per_reset_all();
>  	socfpga_watchdog_disable();
>  
> -	spl_early_init();
> -
>  	/* Configure the clock based on handoff */
>  	cm_basic_init(gd->fdt_blob);
>  
> diff --git a/arch/arm/mach-socfpga/spl_gen5.c b/arch/arm/mach-socfpga/spl_gen5.c
> index 47e63709ad..c646331081 100644
> --- a/arch/arm/mach-socfpga/spl_gen5.c
> +++ b/arch/arm/mach-socfpga/spl_gen5.c
> @@ -67,8 +67,14 @@ void board_init_f(ulong dummy)
>  	int ret;
>  	struct udevice *dev;
>  
> +	ret = spl_early_init();
> +	if (ret)
> +		hang();
> +
> +	socfpga_get_manager_addr();
> +
>  	/*
> -	 * First C code to run. Clear fake OCRAM ECC first as SBE
> +	 * Clear fake OCRAM ECC first as SBE
>  	 * and DBE might triggered during power on
>  	 */
>  	reg = readl(&sysmgr_regs->eccgrp_ocram);
> @@ -128,12 +134,6 @@ void board_init_f(ulong dummy)
>  	debug_uart_init();
>  #endif
>  
> -	ret = spl_early_init();
> -	if (ret) {
> -		debug("spl_early_init() failed: %d\n", ret);
> -		hang();
> -	}
> -
>  	ret = uclass_get_device(UCLASS_RESET, 0, &dev);
>  	if (ret)
>  		debug("Reset init failed: %d\n", ret);
> diff --git a/arch/arm/mach-socfpga/spl_s10.c b/arch/arm/mach-socfpga/spl_s10.c
> index ec65e1ce64..9a97a84e1e 100644
> --- a/arch/arm/mach-socfpga/spl_s10.c
> +++ b/arch/arm/mach-socfpga/spl_s10.c
> @@ -14,6 +14,7 @@
>  #include <asm/arch/clock_manager.h>
>  #include <asm/arch/firewall_s10.h>
>  #include <asm/arch/mailbox_s10.h>
> +#include <asm/arch/misc.h>
>  #include <asm/arch/reset_manager.h>
>  #include <asm/arch/system_manager.h>
>  #include <watchdog.h>
> @@ -120,6 +121,12 @@ void board_init_f(ulong dummy)
>  	const struct cm_config *cm_default_cfg = cm_get_default_config();
>  	int ret;
>  
> +	ret = spl_early_init();
> +	if (ret)
> +		hang();
> +
> +	socfpga_get_manager_addr();
> +
>  #ifdef CONFIG_HW_WATCHDOG
>  	/* Ensure watchdog is paused when debugging is happening */
>  	writel(SYSMGR_WDDBG_PAUSE_ALL_CPU, &sysmgr_regs->wddbg);
> @@ -145,11 +152,6 @@ void board_init_f(ulong dummy)
>  	socfpga_per_reset(SOCFPGA_RESET(UART0), 0);
>  	debug_uart_init();
>  #endif
> -	ret = spl_early_init();
> -	if (ret) {
> -		debug("spl_early_init() failed: %d\n", ret);
> -		hang();
> -	}
>  
>  	preloader_console_init();
>  	cm_print_clock_quick_summary();

Are these three hunks above really supposed to be in this patch ?