[PATCH 1/2] drivers: watchdog: Enhance watchdog support in SPL for Stratix 10 and Agilex

[Stefan Roese]

Hi Lim, Jit Loon,

On 12/5/22 14:27, Lim, Jit Loon wrote:
> -----Original Message-----
> From: Stefan Roese <sr at denx.de>
> Sent: Monday, 5 December, 2022 8:28 PM
> To: Lim, Jit Loon <jit.loon.lim at intel.com>; u-boot at lists.denx.de
> Cc: Jagan Teki <jagan at amarulasolutions.com>; Vignesh R <vigneshr at ti.com>; Vasut, Marek <marex at denx.de>; Simon <simon.k.r.goldschmidt at gmail.com>; Chee, Tien Fong <tien.fong.chee at intel.com>; Hea, Kok Kiang <kok.kiang.hea at intel.com>; Lim, Elly Siew Chin <elly.siew.chin.lim at intel.com>; Kho, Sin Hui <sin.hui.kho at intel.com>; Lokanathan, Raaj <raaj.lokanathan at intel.com>; Maniyam, Dinesh <dinesh.maniyam at intel.com>; Ng, Boon Khai <boon.khai.ng at intel.com>; Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi at intel.com>; Chong, Teik Heng <teik.heng.chong at intel.com>; Zamri, Muhammad Hazim Izzat <muhammad.hazim.izzat.zamri at intel.com>; Tang, Sieu Mun <sieu.mun.tang at intel.com>
> Subject: Re: [PATCH 1/2] drivers: watchdog: Enhance watchdog support in SPL for Stratix 10 and Agilex
> 
> On 11/22/22 15:18, Jit Loon Lim wrote:
>> From: Siew Chin Lim <elly.siew.chin.lim at intel.com>
>>
>> Change watchdog default timeout to 10 seconds and enable watchdog
>> before initializing other component (example: DDR). Thus, watchdog
>> need to be fully executed in onchip ram.
> 
> Please find some comments below.
> 
>> Signed-off-by: Siew Chin Lim <elly.siew.chin.lim at intel.com>
>> Signed-off-by: Jit Loon Lim <jit.loon.lim at intel.com>
>> ---
>>    arch/arm/mach-socfpga/spl_agilex.c | 17 +++++++++--------
>>    arch/arm/mach-socfpga/spl_s10.c    | 14 +++++++-------
>>    drivers/watchdog/Kconfig           |  2 +-
>>    3 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/mach-socfpga/spl_agilex.c
>> b/arch/arm/mach-socfpga/spl_agilex.c
>> index ee5a9dc1e2..c279f97cea 100644
>> --- a/arch/arm/mach-socfpga/spl_agilex.c
>> +++ b/arch/arm/mach-socfpga/spl_agilex.c
>> @@ -20,7 +20,7 @@
>>    #include <asm/arch/misc.h>
>>    #include <asm/arch/reset_manager.h>
>>    #include <asm/arch/system_manager.h> -#include <watchdog.h>
>> +#include <wdt.h>
>>    #include <dm/uclass.h>
>>    
>>    DECLARE_GLOBAL_DATA_PTR;
>> @@ -40,13 +40,6 @@ void board_init_f(ulong dummy)
>>    	writel(SYSMGR_WDDBG_PAUSE_ALL_CPU,
>>    	       socfpga_get_sysmgr_addr() + SYSMGR_SOC64_WDDBG);
>>    
>> -#ifdef CONFIG_HW_WATCHDOG
>> -	/* Enable watchdog before initializing the HW */
>> -	socfpga_per_reset(SOCFPGA_RESET(L4WD0), 1);
>> -	socfpga_per_reset(SOCFPGA_RESET(L4WD0), 0);
>> -	hw_watchdog_init();
>> -#endif
>> -
> 
> The patch also removes the CONFIG_HW_WATCHDOG stuff, which is probably unused right now. Could you please also mention this in the commit message?
> 
> Sure.
> 
>>    	/* ensure all processors are not released prior Linux boot */
>>    	writeq(0, CPU_RELEASE_ADDR);
>>    
>> @@ -60,6 +53,14 @@ void board_init_f(ulong dummy)
>>    		hang();
>>    	}
>>    
>> +	/*
>> +	 * Enable watchdog as early as possible before initializing other
>> +	 * component. Watchdog need to be enabled after clock driver because
>> +	 * it will retrieve the clock frequency from clock driver.
>> +	 */
>> +	if (CONFIG_IS_ENABLED(WDT))
>> +		initr_watchdog();
>> +
> 
> initr_watchdog() will get called from spl/common/spl/spl.c:
> board_init_r(). Is this too late? How much time passes between these two code path's?
> 
> We do not have the actual measurement now and shall update once we get it measured.

Do you have some updates on this by now?

Thanks,
Stefan

> Thanks,
> Stefan
> 
>>    	preloader_console_init();
>>    	print_reset_info();
>>    	cm_print_clock_quick_summary();
>> diff --git a/arch/arm/mach-socfpga/spl_s10.c
>> b/arch/arm/mach-socfpga/spl_s10.c index c20e87cdbe..4044dc335e 100644
>> --- a/arch/arm/mach-socfpga/spl_s10.c
>> +++ b/arch/arm/mach-socfpga/spl_s10.c
>> @@ -21,7 +21,7 @@
>>    #include <asm/arch/misc.h>
>>    #include <asm/arch/reset_manager.h>
>>    #include <asm/arch/system_manager.h> -#include <watchdog.h>
>> +#include <wdt.h>
>>    #include <dm/uclass.h>
>>    
>>    DECLARE_GLOBAL_DATA_PTR;
>> @@ -41,12 +41,12 @@ void board_init_f(ulong dummy)
>>    	writel(SYSMGR_WDDBG_PAUSE_ALL_CPU,
>>    	       socfpga_get_sysmgr_addr() + SYSMGR_SOC64_WDDBG);
>>    
>> -#ifdef CONFIG_HW_WATCHDOG
>> -	/* Enable watchdog before initializing the HW */
>> -	socfpga_per_reset(SOCFPGA_RESET(L4WD0), 1);
>> -	socfpga_per_reset(SOCFPGA_RESET(L4WD0), 0);
>> -	hw_watchdog_init();
>> -#endif
>> +	/*
>> +	 * Enable watchdog as early as possible before initializing other
>> +	 * component.
>> +	 */
>> +	if (CONFIG_IS_ENABLED(WDT))
>> +		initr_watchdog();
>>    
>>    	/* ensure all processors are not released prior Linux boot */
>>    	writeq(0, CPU_RELEASE_ADDR);
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>> 50e6a1efba..a6c242ac9d 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -27,7 +27,7 @@ config WATCHDOG_TIMEOUT_MSECS
>>    	int "Watchdog timeout in msec"
>>    	default 128000 if ARCH_MX31 || ARCH_MX5 || ARCH_MX6
>>    	default 128000 if ARCH_MX7 || ARCH_VF610
>> -	default 30000 if ARCH_SOCFPGA
>> +	default 10000 if ARCH_SOCFPGA
>>    	default 16000 if ARCH_SUNXI
>>    	default 60000
>>    	help
> 
> Viele Grüße,
> Stefan Roese
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de