Re: [PATCH v1 1/1] driver: watchdog: get timeout and clock from dt file

[Stefan Roese]

On 31.03.20 08:08, Rayagonda Kokatanur wrote:

From: Bharat Kumar Reddy Gooty <bharat.go...@broadcom.com>

Get the watchdog default timeout value and clock from the DTS file

The default timeout is already read from the DT. Please see below...

Signed-off-by: Bharat Kumar Reddy Gooty <bharat.go...@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokata...@broadcom.com>
---
  drivers/watchdog/sp805_wdt.c | 36 ++++++++++++++++++++++++++++++++++--
  1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/sp805_wdt.c b/drivers/watchdog/sp805_wdt.c
index ca3ccbe76c..e8f54d6804 100644
--- a/drivers/watchdog/sp805_wdt.c
+++ b/drivers/watchdog/sp805_wdt.c
@@ -34,8 +34,31 @@ DECLARE_GLOBAL_DATA_PTR;

struct sp805_wdt_priv {

        void __iomem *reg;
+       u32 timeout_msec;
+       u32 clk_mhz;
  };

+static u32 msec_to_ticks(struct udevice *dev)

+{
+       u32 timeout_msec;
+       u32 msec;
+       struct sp805_wdt_priv *priv = dev_get_priv(dev);
+
+       timeout_msec = env_get_ulong("wdt_timeout_msec", 10, 0);
+       if (timeout_msec) {
+               dev_dbg(dev, "Overriding timeout :%u\n", timeout_msec);
+               msec = timeout_msec;
+       } else {
+               msec = priv->timeout_msec;
+       }

Why is this overriding via environment needed? BTW: You should mention
such a change in the commit text as well.

+
+       timeout_msec = (msec / 2) * (priv->clk_mhz / 1000);
+
+       dev_dbg(dev, "ticks :%u\n", timeout_msec);
+
+       return timeout_msec;
+}
+
  static int sp805_wdt_reset(struct udevice *dev)
  {
        struct sp805_wdt_priv *priv = dev_get_priv(dev);
@@ -63,8 +86,11 @@ static int sp805_wdt_start(struct udevice *dev, u64 timeout, 
ulong flags)
         * set 120s, the gd->bus_clk is less than 1145MHz, the load_value will
         * not overflow.
         */
-       load_value = (gd->bus_clk) /
-               (2 * 1000 * SYS_FSL_WDT_CLK_DIV) * load_time;
+       if (gd->bus_clk)
+               load_value = (gd->bus_clk) /
+                       (2 * 1000 * SYS_FSL_WDT_CLK_DIV) * load_time;
+       else /* platform provide clk */
+               load_value = msec_to_ticks(dev);

Nitpicking: Please use paranthesis on multi-line statements.

        writel(UNLOCK, priv->reg + WDTLOCK);
        writel(load_value, priv->reg + WDTLOAD);
@@ -110,6 +136,12 @@ static int sp805_wdt_ofdata_to_platdata(struct udevice 
*dev)
        if (IS_ERR(priv->reg))
                return PTR_ERR(priv->reg);

+ if (dev_read_u32(dev, "timeout-msec", &priv->timeout_msec))

+               return -ENODATA;

This does not seem to be an official DT binding. At least I was unable
to find it in the latest Linux tree.

You are aware that the WDT core code already reads the WDT timeout from
the DT using the official "timeout-sec" property? Do you need a higher
graned timeout value (ms vs s)?

Thanks,
Stefan