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

On Tue, Mar 31, 2020 at 12:44 PM Stefan Roese sr@denx.de wrote:

On 31.03.20 08:08, Rayagonda Kokatanur wrote:

...

From: Bharat Kumar Reddy Gooty bharat.gooty@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...

Thank you, will fix this.

...

Signed-off-by: Bharat Kumar Reddy Gooty bharat.gooty@broadcom.com Signed-off-by: Rayagonda Kokatanur rayagonda.kokatanur@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.

This code will be removed as default timeout is already read in include/wdt.h

...

• 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.

Thank you fix this.

...

  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)?

Thank you for pointing this, will update my dt file.

Thanks, Stefan