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

Hi Shreenidhi,

On 14 July 2018 at 14:35, Shreenidhi Shedi yesshedi@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@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@xilinx.com

• •         Shreenidhi Shedi <yesshedi@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