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

[Simon Glass]

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