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

[Andrew Bresticker <abrestic-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>]

On Mon, Nov 17, 2014 at 3:01 PM, Naidu Tellapati
<Naidu.Tellapati-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Andrew,
>
>>> > +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).
>
> Shall we go ahead with what James Hogan is suggesting here ?

Yup, sounds good to me.

>>>
>>> > --- /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.
>
> From the comments above both from Andrew & James Hogan, I understand that
> we don't need clock rate-change notifier callback for Watchdog driver.
> Please correct me if I am wrong.

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