Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408088

Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver

Show all headers | View raw

On 05/27/2016 01:25 AM, Neil Armstrong wrote:
[ ... ]
>>> +    data->wdt_dev.max_hw_heartbeat_ms = GXBB_WDT_TCNT_SETUP_MASK;
>>> +    data->wdt_dev.min_hw_heartbeat_ms = 1;
>>
>> Does the device require a minimum time between heartbeats ?
>> Just asking, because you violate it yourself below.
>>
>> If you want to set the minimum timeout, that would be min_timeout.
>
> Ok, these values are not very clear actually.
>
Hmmm .. yes, reading the description again, it doesn't really describe well
what it is doing. Essentially, min_hw_heartbeat_ms is enforced by the watchdog
core, and should be used if a watchdog hardware can not tolerate short intervals
between heartbeats. min_timeout is the minimum timeout value configurable from
user space.

>>> +    data->wdt_dev.timeout = DEFAULT_TIMEOUT;
>>> +    watchdog_set_drvdata(&data->wdt_dev, data);
>>> +
>>> +    watchdog_init_timeout(&data->wdt_dev, DEFAULT_TIMEOUT, &pdev->dev);
>>> +
>>
>> DEFAULT_TIMEOUT is unnecessary here. Since the default timeout is already set,
>> it makes the call useless.
>>
>>> +    ret = watchdog_register_device(&data->wdt_dev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /* Setup with 1ms timebase */
>>> +    writel(((clk_get_rate(data->clk) / 1000) & GXBB_WDT_CTRL_DIV_MASK) |
>>> +        GXBB_WDT_CTRL_EE_RESET |
>>> +        GXBB_WDT_CTRL_CLK_EN |
>>> +        GXBB_WDT_CTRL_CLKDIV_EN,
>>> +        data->reg_base + GXBB_WDT_CTRL_REG);
>>> +    meson_gxbb_wdt_set_timeout(&data->wdt_dev, data->wdt_dev.timeout);
>>
>> set_timeout already calls the ping function. Also, the functions should be called
>> prior to watchdog registration to avoid race conditions.
>>
>>> +    meson_gxbb_wdt_ping(&data->wdt_dev);
>>
>> This is unusual. If the watchdog can be already running, it might make sense
>> to tell the core about it (set WDOG_HW_RUNNING in the status field), so it
>> can send heartbeats until user space opens the device.
>
> Yes, since meson_gxbb_wdt_set_timeout() already ping, this is useless.
>

Not only that - if the watchdog _is_ already running at boot time, you should
really set WDOG_HW_RUNNING to let the watchdog core know. You status function
would come handy there.

	if (meson_gxbb_wdt_status(data))	// note the changed parameter
		set_bit(WDOG_HW_RUNNING, &data->wdt_dev.status);

Thanks,
Guenter

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[RFC PATCH 0/3] watchdog: Add Amlogic Meson GXBB Watchdog Timer driver Neil Armstrong <narmstrong@baylibre.com> - 2016-05-26 10:00 +0200
  [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver Neil Armstrong <narmstrong@baylibre.com> - 2016-05-26 10:00 +0200
    Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver Carlo Caione <carlo@caione.org> - 2016-05-26 12:10 +0200
      Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver Neil Armstrong <narmstrong@baylibre.com> - 2016-05-27 10:30 +0200
    Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver Guenter Roeck <linux@roeck-us.net> - 2016-05-26 16:00 +0200
      Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver Neil Armstrong <narmstrong@baylibre.com> - 2016-05-27 10:30 +0200
        Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver Guenter Roeck <linux@roeck-us.net> - 2016-05-27 15:50 +0200
          Re: [RFC PATCH 1/3] watchdog: Add Meson GXBB Watchdog Driver Neil Armstrong <narmstrong@baylibre.com> - 2016-05-27 17:30 +0200
  [RFC PATCH 3/3] ARM64: dts: amlogic: meson-gxbb: Add watchdog node Neil Armstrong <narmstrong@baylibre.com> - 2016-05-26 10:00 +0200
  [RFC PATCH 2/3] dt-bindings: watchdog: Add Meson GXBB Watchdog bindings Neil Armstrong <narmstrong@baylibre.com> - 2016-05-26 10:00 +0200
    Re: [RFC PATCH 2/3] dt-bindings: watchdog: Add Meson GXBB Watchdog  bindings Carlo Caione <carlo@caione.org> - 2016-05-26 12:10 +0200

csiph-web