[U-Boot] [PATCH v2 3/3] watchdog: Convert Xilinx Axi watchdog driver to driver model

[Simon Glass]

Hi Shreenidhi,

On 14 July 2018 at 14:35, Shreenidhi Shedi <yesshedi at gmail.com> wrote:
> Xilinx Axi wdt driver conversion to driver model & Kconfig update
> for the same.
>
> Changes in V1:
>  - Xilinx Axi wdt driver conversion to DM initial version
>
> Changes in V2:
>  - Rectified print message
>  - Removed  stop instruction from probe
>
> Signed-off-by: Shreenidhi Shedi <yesshedi at gmail.com>
> ---
>
> Changes in v2: None
>
>  drivers/watchdog/Kconfig         |   8 +++
>  drivers/watchdog/xilinx_tb_wdt.c | 105 ++++++++++++++++++++++++-------
>  2 files changed, 89 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 148c6a0d68..351d2af8d9 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -103,4 +103,12 @@ config WDT_CDNS
>            Select this to enable Cadence watchdog timer, which can be found on some
>            Xilinx Microzed Platform.
>
> +config XILINX_TB_WATCHDOG
> +       bool "Xilinx Axi watchdog timer support"
> +       depends on WDT
> +       imply WATCHDOG
> +       help
> +          Select this to enable Xilinx Axi watchdog timer, which can be found on some
> +          Xilinx Microblaze Platform.

platforms?

> +
>  endmenu
> diff --git a/drivers/watchdog/xilinx_tb_wdt.c b/drivers/watchdog/xilinx_tb_wdt.c
> index 2274123e49..23ddbdfbee 100644
> --- a/drivers/watchdog/xilinx_tb_wdt.c
> +++ b/drivers/watchdog/xilinx_tb_wdt.c
> @@ -1,13 +1,17 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> + * Xilinx Axi platforms watchdog timer driver.
> + *
> + * Author(s):  Michal Šimek <michal.simek at xilinx.com>
> + *             Shreenidhi Shedi <yesshedi at gmail.com>
> + *
>   * Copyright (c) 2011-2013 Xilinx Inc.
>   */
>
>  #include <common.h>
> -#include <asm/io.h>
> -#include <asm/microblaze_intc.h>
> -#include <asm/processor.h>
> -#include <watchdog.h>
> +#include <dm.h>
> +#include <wdt.h>
> +#include <linux/io.h>
>
>  #define XWT_CSR0_WRS_MASK      0x00000008 /* Reset status Mask */
>  #define XWT_CSR0_WDS_MASK      0x00000004 /* Timer state Mask */
> @@ -20,49 +24,102 @@ struct watchdog_regs {
>         u32 tbr; /* 0x8 */
>  };
>
> -static struct watchdog_regs *watchdog_base =
> -                       (struct watchdog_regs *)CONFIG_WATCHDOG_BASEADDR;
> +struct xlnx_wdt_priv {
> +       bool enable_once;
> +       struct watchdog_regs *regs;
> +};
>
> -void hw_watchdog_reset(void)
> +static int xlnx_wdt_reset(struct udevice *dev)

You could pass just regs to this function? Same below.

>  {
>         u32 reg;
> +       struct xlnx_wdt_priv *priv = dev_get_priv(dev);
> +
> +       debug("%s\n", __func__);
>
>         /* Read the current contents of TCSR0 */
> -       reg = readl(&watchdog_base->twcsr0);
> +       reg = readl(&priv->regs->twcsr0);
>
>         /* Clear the watchdog WDS bit */
>         if (reg & (XWT_CSR0_EWDT1_MASK | XWT_CSRX_EWDT2_MASK))
> -               writel(reg | XWT_CSR0_WDS_MASK, &watchdog_base->twcsr0);
> +               writel(reg | XWT_CSR0_WDS_MASK, &priv->regs->twcsr0);
> +
> +       return 0;
>  }
>
> -void hw_watchdog_disable(void)
> +static int xlnx_wdt_stop(struct udevice *dev)
>  {
>         u32 reg;
> +       struct xlnx_wdt_priv *priv = dev_get_priv(dev);
> +
> +       if (priv->enable_once) {
> +               puts("Can't stop Xilinx watchdog.\n");

debug()

> +               return -1;

return -EBUSY

-1 is -EPERM which might not be correct.

> +       }
>
>         /* Read the current contents of TCSR0 */
> -       reg = readl(&watchdog_base->twcsr0);
> +       reg = readl(&priv->regs->twcsr0);
>
> -       writel(reg & ~XWT_CSR0_EWDT1_MASK, &watchdog_base->twcsr0);
> -       writel(~XWT_CSRX_EWDT2_MASK, &watchdog_base->twcsr1);
> +       writel(reg & ~XWT_CSR0_EWDT1_MASK, &priv->regs->twcsr0);
> +       writel(~XWT_CSRX_EWDT2_MASK, &priv->regs->twcsr1);
>
>         puts("Watchdog disabled!\n");

Drivers should not output to the console.

> +
> +       return 0;
>  }
>
> -static void hw_watchdog_isr(void *arg)
> +static int xlnx_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
>  {
> -       hw_watchdog_reset();
> +       struct xlnx_wdt_priv *priv = dev_get_priv(dev);
> +
> +       writel((XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK | XWT_CSR0_EWDT1_MASK),
> +              &priv->regs->twcsr0);
> +
> +       writel(XWT_CSRX_EWDT2_MASK, &priv->regs->twcsr1);
> +
> +       return 0;
>  }
>
> -void hw_watchdog_init(void)
> +static int xlnx_wdt_probe(struct udevice *dev)
>  {
> -       int ret;
> +       debug("%s: Probing wdt%u\n", __func__, dev->seq);
>
> -       writel((XWT_CSR0_WRS_MASK | XWT_CSR0_WDS_MASK | XWT_CSR0_EWDT1_MASK),
> -              &watchdog_base->twcsr0);
> -       writel(XWT_CSRX_EWDT2_MASK, &watchdog_base->twcsr1);
> +       return 0;
> +}
>
> -       ret = install_interrupt_handler(CONFIG_WATCHDOG_IRQ,
> -                                               hw_watchdog_isr, NULL);
> -       if (ret)
> -               puts("Watchdog IRQ registration failed.");
> +static int xlnx_wdt_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct xlnx_wdt_priv *priv = dev_get_priv(dev);
> +
> +       priv->regs = (struct watchdog_regs *)dev_read_addr(dev);
> +       if (IS_ERR(priv->regs))
> +               return PTR_ERR(priv->regs);
> +
> +       priv->enable_once = dev_read_u32_default(dev, "xlnx,wdt-enable-once",
> +                                                0);

Should that not be dev_read_bool()? Does it actually using an integer
to indicate a boolean? Is there a binding file for this?

> +
> +       debug("%s: wdt-enable-once %d\n", __func__, priv->enable_once);
> +
> +       return 0;
>  }
> +
> +static const struct wdt_ops xlnx_wdt_ops = {
> +       .start = xlnx_wdt_start,
> +       .reset = xlnx_wdt_reset,
> +       .stop = xlnx_wdt_stop,
> +};
> +
> +static const struct udevice_id xlnx_wdt_ids[] = {
> +       { .compatible = "xlnx,xps-timebase-wdt-1.00.a", },
> +       { .compatible = "xlnx,xps-timebase-wdt-1.01.a", },
> +       {},
> +};
> +
> +U_BOOT_DRIVER(xlnx_wdt) = {
> +       .name = "xlnx_wdt",
> +       .id = UCLASS_WDT,
> +       .of_match = xlnx_wdt_ids,
> +       .probe = xlnx_wdt_probe,
> +       .priv_auto_alloc_size = sizeof(struct xlnx_wdt_priv),
> +       .ofdata_to_platdata = xlnx_wdt_ofdata_to_platdata,
> +       .ops = &xlnx_wdt_ops,
> +};
> --
> 2.17.1
>

Regards,
Simon