Re: [PATCH v2 1/2] watchdog: ImgTec PDC Watchdog Timer Driver

On Mon, Nov 17, 2014 at 11:47:51AM -0800, Andrew Bresticker wrote:

On Mon, Nov 17, 2014 at 7:13 AM,  [off-list ref] wrote:

quoted

From: Naidu Tellapati <redacted>

This commit adds support for ImgTec PowerDown Controller Watchdog Timer.

Signed-off-by: Jude Abraham <redacted>
Signed-off-by: Naidu Tellapati <redacted>

A couple comments below, but otherwise this is looking good.

quoted

--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig

quoted

@@ -1235,6 +1235,16 @@ config BCM_KONA_WDT_DEBUG

          If in doubt, say 'N'.

+config IMGPDC_WDT
+       tristate "Imagination Technologies PDC Watchdog Timer"
+       depends on HAS_IOMEM

We probably don't want to make this visible for everyone with
HAS_IOMEM, so perhaps MIPS || (HAS_IOMEM && COMPILE_TEST)?  Do we want
to make this available for META as well?

yeh, can I suggest something like this:
	depends on HAS_IOMEM
	depends on METAG || MIPS || COMPILE_TEST
More intuitive/scalable to have them separate IMO, since technically it
needs HAS_IOMEM regardless of platform (even if the listed platforms are
guaranteed to select HAS_IOMEM).

quoted

--- /dev/null
+++ b/drivers/watchdog/imgpdc_wdt.c

quoted

+#ifdef CONFIG_COMMON_CLK
+#define to_pdc_wdt(_nb) container_of(_nb, struct pdc_wdt_dev, clk_nb)
+
+static int img_pdc_wdt_clk_notify(struct notifier_block *nb,
+                                 unsigned long action, void *data)
+{
+       struct pdc_wdt_dev *wdt = to_pdc_wdt(nb);
+
+       switch (action) {
+       case POST_RATE_CHANGE:
+               if (wdt->clk_rate == data->new_rate)
+                       return NOTIFY_OK;
+
+               /* We use the clock rate to calculate the max timeout */
+               if (data->new_rate < 1 ||
+                   data->new_rate > PDC_MAX_WD_CLK_RATE) {
+                       dev_err(wdt->dev, "invalid clock rate\n");
+                       return NOTIFY_BAD;
+               }
+               wdt->clk_rate = data->new_rate;
+               if (order_base_2(wdt->clk_rate) == 0)
+                       wdt->min_delay = 0;
+               else
+                       wdt->min_delay = order_base_2(wdt->clk_rate) - 1;
+
+               wdt->wdt_dev.max_timeout =
+                               (1 << (PDC_WD_MAX_DELAY - wdt->min_delay));
+               ret = watchdog_init_timeout(&wdt->wdt_dev, timeout, wdt->dev);
+               if (ret < 0)
+                       wdt->wdt_dev.timeout = wdt->wdt_dev.max_timeout;
+
+               pdc_wdt_stop(&wdt->wdt_dev);
+               pdc_wdt_set_timeout(&wdt->wdt_dev, wdt->wdt_dev.timeout);
+               pdc_wdt_start(&wdt->wdt_dev);
+               break;
+       default:
+               break;
+       }
+       return NOTIFY_OK;
+}
+#endif

Do we really expect the watchdog clock to change out from under us?  I
think we can just get the rate in probe() and set the timeout bounds
based on that - no one outside the watchdog driver should be messing
with the watchdog clock, right?  I know the samsung driver does
something similar to this, but I believe that's because the watchdog
clock is derived from the CPU clock.

I agree (sorry if I implied wdt needed to be dynamic as well as reading
the [probably constant] rate before).

Note, dynamic changes did happen on TZ1090, but only just before suspend
(since the hardware would change the rate regardless of what software
did when the power actually went down, so it at least allowed drivers to
put their workarounds in place). I presume watchdog timers aren't meant
to be active during suspend though, in which case even for TZ1090 it
probably doesn't need to expect changes.

Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html