Hi Simon,
On 19.7.2018 03:31, Simon Glass wrote:
> Hi Shreenidhi,
>
> On 14 July 2018 at 14:35, Shreenidhi Shedi <yessh...@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 <yessh...@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?
will fix.
>
>> +
>> 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.si...@xilinx.com>
>> + * Shreenidhi Shedi <yessh...@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.
I don't think so. This function is the part of struct wdt_ops{}
which is
82 /*
83 * Reset the timer, typically restoring the counter to
84 * the value configured by start()
85 *
86 * @dev: WDT Device
87 * @return: 0 if OK, -ve on error
88 */
89 int (*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 Xilinx watchdog.\n");
>
> debug()
will fix
>
>> + return -1;
>
> return -EBUSY
>
> -1 is -EPERM which might not be correct.
will fix
>
>> + }
>>
>> /* 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.
debug is fine here.
>
>> +
>> + 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?
Unfortunately no. It is pretty old binding and it wasn't defined as
bool. Here is binding doc.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt
>
>> +
>> + 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
>>
Thanks,
Michal
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
