[v1,15/15] board: ns3: start sp805 watchdog service

Start sp805 watchdog service.

Parse wdt timeout from env and dts, give precedence to env
timeout if defined. Set default timeout to 60s if both env
and dts doesn't specifiy timeout.

Stop the WDT in board late init and start the
WDT service before giving control to Linux.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com>
Signed-off-by: Bharat Kumar Reddy Gooty <bharat.gooty at broadcom.com>
Signed-off-by: Pramod Kumar <pramod.kumar at broadcom.com>
---
 board/broadcom/bcmns3/ns3.c | 56 +++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

Hi Rayagonda,

On Sun, 17 May 2020 at 02:21, Rayagonda Kokatanur
<rayagonda.kokatanur at broadcom.com> wrote:
>
> Start sp805 watchdog service.
>
> Parse wdt timeout from env and dts, give precedence to env
> timeout if defined. Set default timeout to 60s if both env
> and dts doesn't specifiy timeout.
>
> Stop the WDT in board late init and start the
> WDT service before giving control to Linux.
>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur at broadcom.com>
> Signed-off-by: Bharat Kumar Reddy Gooty <bharat.gooty at broadcom.com>
> Signed-off-by: Pramod Kumar <pramod.kumar at broadcom.com>
> ---
>  board/broadcom/bcmns3/ns3.c | 56 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/board/broadcom/bcmns3/ns3.c b/board/broadcom/bcmns3/ns3.c
> index 51cab1aad3..37a3cf9d14 100644
> --- a/board/broadcom/bcmns3/ns3.c
> +++ b/board/broadcom/bcmns3/ns3.c
> @@ -11,9 +11,12 @@
>  #include <asm/gic.h>
>  #include <asm/gic-v3.h>
>  #include <asm/system.h>
> +#include <dm/device.h>
> +#include <dm/uclass.h>

You can include <dm.h> and get both of those

>  #include <dt-bindings/memory/bcm-ns3-mc.h>
>  #include <fdtdec.h>

Hoping you don't need that?

>  #include <fdt_support.h>
> +#include <wdt.h>
>
>  #define BANK_OFFSET(bank)      ((u64)BCM_NS3_DDR_INFO_BASE + 8 + ((bank) * 16))

Maybe you should change the BCM_NS3... to ULL? Then you shouldn't need
that cast.

>
> @@ -134,6 +137,14 @@ int board_init(void)
>
>  int board_late_init(void)
>  {
> +#if CONFIG_IS_ENABLED(WDT)

Is it possible to use if() instead of #if?

> +       /*
> +        * Default WDT service is started with 60 sec time out.
> +        * Disable it and start before giving control to Linux.
> +        */
> +       wdt_stop(gd->watchdog_dev);
> +#endif
> +
>         return 0;
>  }
>
> @@ -185,12 +196,57 @@ void reset_cpu(ulong level)
>  }
>
>  #ifdef CONFIG_OF_BOARD_SETUP
> +#if CONFIG_IS_ENABLED(WDT)
> +
> +#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS
> +#define CONFIG_WATCHDOG_TIMEOUT_MSECS  (60 * 1000)
> +#endif
> +#define DEF_TIMEOUT_SEC        (CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000)
> +
> +static int start_wdt(void)
> +{
> +       u32 timeout = DEF_TIMEOUT_SEC;
> +       struct udevice *udev;
> +       int rc = 0;
> +       u32 wdt_enable;
> +
> +       wdt_enable = env_get_ulong("wdt_enable", 16, 0);
> +       printf("wdt_enable :%u\n", wdt_enable);
> +       if (!wdt_enable)
> +               return rc;
> +
> +       rc = uclass_get_device(UCLASS_WDT, 0, &udev);
> +       if (rc) {
> +               printf("Failed to get wdt rc:%d\n", rc);

I suggest return rc here, so you don't need the 'else' below, and the
extra indent/

> +       } else {
> +               timeout = env_get_ulong("wdt_timeout_sec", 10, 0);
> +               if (!timeout) {
> +                       if (CONFIG_IS_ENABLED(OF_CONTROL))
> +                               timeout = dev_read_u32_default(gd->watchdog_dev,
> +                                                              "timeout-sec",
> +                                                              DEF_TIMEOUT_SEC);
> +               }
> +               wdt_start(udev, timeout * 1000, 0);
> +               printf("Started wdt (%ds timeout)\n", timeout);
> +       }
> +
> +       return rc;
> +}
> +#else

Not sure why you need this:

> +static int start_wdt(void)
> +{
> +       return 0;
> +}
> +#endif /* CONFIG_WDT */
> +
>  int ft_board_setup(void *fdt, bd_t *bd)
>  {
>         gic_lpi_tables_init(BCM_NS3_GIC_LPI_BASE, MAX_GIC_REDISTRIBUTORS);
>
>         mem_info_parse_fixup(fdt);
>
> +       start_wdt();

Perhaps you could drop the #idefs above and do something like this here:

if (CONFIG_IS_ENABLED(WDT))
   start_wdt();

> +
>         return 0;
>  }
>  #endif /* CONFIG_OF_BOARD_SETUP */
> --
> 2.17.1
>

Regards,
Simon