From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Thu, 7 Nov 2019 09:33:11 +0100
Subject: [U-Boot] [PATCH v5 2/4] arm: socfpga: Convert reset manager
 from struct to defines
In-Reply-To: <CAFiDJ5-2u4-_1a6HbP-vfZmLz=m0dtD99F5W2QswXx8PeS28-Q@mail.gmail.com>
References: <1573092659-24385-1-git-send-email-ley.foon.tan@intel.com>
 <1573092659-24385-3-git-send-email-ley.foon.tan@intel.com>
 <5fa629f0-cf26-0b60-6f0f-594148e607b8@denx.de>
 <CAFiDJ5-2u4-_1a6HbP-vfZmLz=m0dtD99F5W2QswXx8PeS28-Q@mail.gmail.com>
Message-ID: <cd0962be-8e65-4e94-ecc0-e3da373fc197@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 11/7/19 9:06 AM, Ley Foon Tan wrote:
> On Thu, Nov 7, 2019 at 10:49 AM Marek Vasut 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.

That really should be documented in the commit message though.

[...]

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

If it's only used to retrieve the reset manager base, then it should say
so in the function name.

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

[...]

>>> 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.

OK, please just add it into the commit message.