On Mon, Nov 17, 2014 at 11:47:51AM -0800, Andrew Bresticker wrote:
> On Mon, Nov 17, 2014 at 7:13 AM, <naidu.tellapati@xxxxxxxxxx> wrote:
> > From: Naidu Tellapati <Naidu.Tellapati@xxxxxxxxxx>
> >
> > This commit adds support for ImgTec PowerDown Controller Watchdog Timer.
> >
> > Signed-off-by: Jude Abraham <Jude.Abraham@xxxxxxxxxx>
> > Signed-off-by: Naidu Tellapati <Naidu.Tellapati@xxxxxxxxxx>
>
> A couple comments below, but otherwise this is looking good.
>
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
>
> > @@ -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).
>
> > --- /dev/null
> > +++ b/drivers/watchdog/imgpdc_wdt.c
>
> > +#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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
