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

[Stefan Roese <sr@denx.de>]

On 11/22/22 15:18, Jit Loon Lim wrote:
> From: Siew Chin Lim <elly.siew.chin.lim@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@intel.com>
> Signed-off-by: Jit Loon Lim <jit.loon.lim@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?

>   	/* 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?

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

-- 
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@denx.de