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

[Ley Foon Tan <lftan.linux@gmail.com>]

On Thu, Nov 7, 2019 at 10:49 AM Marek Vasut <marex@denx.de> wrote:
>
> 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 ?
Yes, because spl_early_init() initialize DT stuff, so it needs to be
called before we get base address from DT.
>
> [...]
>
> > 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 ?
Okay, I can add a function for it.
>
> [...]
>
> > 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.
Okay, will change that.
>
> > +     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 .
Okay, will change it something like socfpga_get_base_addr().
>
> > +{
> > +     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 ?
Yes, as explained earlier,  spl_early_init() need to be called before
get base address from DT.

Regards
Ley Foon