[U-Boot] [PATCH v2] wdt: Update uclass to make clear that the timeout is in ms

[Simon Glass]

Hi,

On 4 August 2017 at 16:37, Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
>
>> On 05 Aug 2017, at 00:30, Simon Glass <sjg at chromium.org> wrote:
>>
>> +Tom
>>
>> Hi Philipp,
>>
>> On 4 August 2017 at 16:21, Dr. Philipp Tomsich
>> <philipp.tomsich at theobroma-systems.com> wrote:
>>>
>>>> On 04 Aug 2017, at 23:48, Simon Glass <sjg at chromium.org> wrote:
>>>>
>>>> From: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
>>>>
>>>> Convert name to show explicitly that we are using milliseconds. For a
>>>> watchdog timer this is precise enough.
>>>>
>>>> No functional change intended.
>>>>
>>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Use milliseconds since microseconds seems too fine a control
>>>> - Update commit message to suit
>>>>
>>>> drivers/watchdog/wdt-uclass.c | 4 ++--
>>>> include/wdt.h                 | 8 ++++----
>>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
>>>> index bb9ae80866..8a30f024fd 100644
>>>> --- a/drivers/watchdog/wdt-uclass.c
>>>> +++ b/drivers/watchdog/wdt-uclass.c
>>>> @@ -13,14 +13,14 @@
>>>>
>>>> DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> -int wdt_start(struct udevice *dev, u64 timeout, ulong flags)
>>>> +int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
>>>> {
>>>>      const struct wdt_ops *ops = device_get_ops(dev);
>>>>
>>>>      if (!ops->start)
>>>>              return -ENOSYS;
>>>>
>>>> -     return ops->start(dev, timeout, flags);
>>>> +     return ops->start(dev, timeout_ms, flags);
>>>> }
>>>>
>>>> int wdt_stop(struct udevice *dev)
>>>> diff --git a/include/wdt.h b/include/wdt.h
>>>> index 0b5f05851a..9b90fbeeb3 100644
>>>> --- a/include/wdt.h
>>>> +++ b/include/wdt.h
>>>> @@ -21,12 +21,12 @@
>>>> * Start the timer
>>>> *
>>>> * @dev: WDT Device
>>>> - * @timeout: Number of ticks before timer expires
>>>> + * @timeout_ms: Number of ticks (milliseconds) before timer expires
>>>
>>> Should this not just be ‘number of milliseconds’ (as this would
>>> otherwise equate ticks and milliseconds, which don’t necessarily
>>> need to be the same)?
>>
>> I believe they are the same. We used to have a setting for it
>> (CONFIG_SYS_HZ I think) but now everything is in milliseconds. See for
>> example get_timer().
>
> I had thought of my remark more as a nitpick.
> Bit it seems there’s more to it, after all: at least lib/time.c still
> treats ’ticks’ and ‘milliseconds' as separate concepts:
>
> uint64_t __weak notrace get_ticks(void)
> {
>         unsigned long now = timer_read_counter();
>
>         /* increment tbu if tbl has rolled over */
>         if (now < gd->timebase_l)
>                 gd->timebase_h++;
>         gd->timebase_l = now;
>         return ((uint64_t)gd->timebase_h << 32) | gd->timebase_l;
> }
>
> /* Returns time in milliseconds */
> static uint64_t notrace tick_to_time(uint64_t tick)
> {
>         ulong div = get_tbclk();
>
>         tick *= CONFIG_SYS_HZ;
>         do_div(tick, div);
>         return tick;
> }
>

But see lib/Kconfig:

config SYS_HZ
        int
        default 1000
        help
          The frequency of the timer returned by get_timer().
          get_timer() must operate in milliseconds and this option must be
          set to 1000.


>>
>>>
>>>> * @flags: Driver specific flags. This might be used to specify
>>>> * which action needs to be executed when the timer expires
>>>> * @return: 0 if OK, -ve on error
>>>> */
>>>> -int wdt_start(struct udevice *dev, u64 timeout, ulong flags);
>>>> +int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags);
>>>>
>>>> /*
>>>> * Stop the timer, thus disabling the Watchdog. Use wdt_start to start it again.
>>>> @@ -67,12 +67,12 @@ struct wdt_ops {
>>>>       * Start the timer
>>>>       *
>>>>       * @dev: WDT Device
>>>> -      * @timeout: Number of ticks before the timer expires
>>>> +      * @timeout_ms: Number of ticks (milliseconds) before the timer expires
>>>
>>> See above.
>>>
>>>>       * @flags: Driver specific flags. This might be used to specify
>>>>       * which action needs to be executed when the timer expires
>>>>       * @return: 0 if OK, -ve on error
>>>>       */
>>>> -     int (*start)(struct udevice *dev, u64 timeout, ulong flags);
>>>> +     int (*start)(struct udevice *dev, u64 timeout_ms, ulong flags);
>>>>      /*
>>>>       * Stop the timer
>>>>       *
>>>> --
>>>> 2.14.0.rc1.383.gd1ce394fe2-goog
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> https://lists.denx.de/listinfo/u-boot

Regards,
Simon