[U-Boot] [PATCH v1 6/6] watchdog: Convert Xilinx Axi watchdog driver to driver model

[Michal Simek]

please fill commit message.

On 29.6.2018 23:51, Shreenidhi Shedi wrote:
> Signed-off-by: Shreenidhi Shedi <yesshedi at gmail.com>
> ---
> 
> Changes in v1: None
> 
>  drivers/watchdog/Kconfig         |   8 +++
>  drivers/watchdog/xilinx_tb_wdt.c | 107 ++++++++++++++++++++++++-------
>  2 files changed, 91 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.
> +
>  endmenu
> diff --git a/drivers/watchdog/xilinx_tb_wdt.c b/drivers/watchdog/xilinx_tb_wdt.c
> index 2274123e49..7f20c2ce2f 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,104 @@ 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)
>  {
>  	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 Xilinux Axi watchdog.\n");

Just Xilinx here.

> +		return -1;
> +	}
>  
>  	/* 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");
> +
> +	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);
> +	xlnx_wdt_stop(dev);

Based on what I see on running microblaze system. Watchdog is started
right after bitstream is loaded. It is weird that you want to stop
watchdog which is not started yet.
Also in case you can disable it because it was enabled in previous code
you are no covering time between probe done and watchdog enabling again.

That's why I think you should remove this line.

>  
> -	ret = install_interrupt_handler(CONFIG_WATCHDOG_IRQ,
> -						hw_watchdog_isr, NULL);
> -	if (ret)
> -		puts("Watchdog IRQ registration failed.");
> +	return 0;
>  }
> +
> +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);
> +
> +	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,
> +};
> 

Other then this the patch is fine and I have tested it on microblaze system.

Thanks,
Michal