| Message ID | 20221122141837.24055-1-jit.loon.lim@intel.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/2] drivers: watchdog: Enhance watchdog support in SPL for Stratix 10 and Agilex | expand |
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
-----Original Message----- From: Stefan Roese <sr@denx.de> Sent: Monday, 5 December, 2022 8:28 PM To: Lim, Jit Loon <jit.loon.lim@intel.com>; u-boot@lists.denx.de Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R <vigneshr@ti.com>; Vasut, Marek <marex@denx.de>; Simon <simon.k.r.goldschmidt@gmail.com>; Chee, Tien Fong <tien.fong.chee@intel.com>; Hea, Kok Kiang <kok.kiang.hea@intel.com>; Lim, Elly Siew Chin <elly.siew.chin.lim@intel.com>; Kho, Sin Hui <sin.hui.kho@intel.com>; Lokanathan, Raaj <raaj.lokanathan@intel.com>; Maniyam, Dinesh <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>; Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Chong, Teik Heng <teik.heng.chong@intel.com>; Zamri, Muhammad Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Tang, Sieu Mun <sieu.mun.tang@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@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? 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. 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
Hi Lim, Jit Loon, On 12/5/22 14:27, Lim, Jit Loon wrote: > -----Original Message----- > From: Stefan Roese <sr@denx.de> > Sent: Monday, 5 December, 2022 8:28 PM > To: Lim, Jit Loon <jit.loon.lim@intel.com>; u-boot@lists.denx.de > Cc: Jagan Teki <jagan@amarulasolutions.com>; Vignesh R <vigneshr@ti.com>; Vasut, Marek <marex@denx.de>; Simon <simon.k.r.goldschmidt@gmail.com>; Chee, Tien Fong <tien.fong.chee@intel.com>; Hea, Kok Kiang <kok.kiang.hea@intel.com>; Lim, Elly Siew Chin <elly.siew.chin.lim@intel.com>; Kho, Sin Hui <sin.hui.kho@intel.com>; Lokanathan, Raaj <raaj.lokanathan@intel.com>; Maniyam, Dinesh <dinesh.maniyam@intel.com>; Ng, Boon Khai <boon.khai.ng@intel.com>; Yuslaimi, Alif Zakuan <alif.zakuan.yuslaimi@intel.com>; Chong, Teik Heng <teik.heng.chong@intel.com>; Zamri, Muhammad Hazim Izzat <muhammad.hazim.izzat.zamri@intel.com>; Tang, Sieu Mun <sieu.mun.tang@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@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? > > 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
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 - /* 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(); + 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