[U-Boot] [PATCH v3 05/11] dm: timer: Support 64-bit counter

[Simon Glass]

Hi Bin,

On 13 November 2015 at 01:11, Bin Meng <bmeng.cn at gmail.com> wrote:
> There are timers with a 64-bit counter value but current timer
> uclass driver assumes a 32-bit one. Modify timer_get_count()
> to ask timer driver to always return a 64-bit counter value,
> and provide an inline helper function timer_conv_64() to handle
> the 32-bit/64-bit conversion automatically.
>
> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>
> ---
>
> Changes in v3:
> - Update commit message to reflect the v2 changes (ie: there is
>   no "counter-64bit" property)
>
> Changes in v2:
> - Do not use "counter-64bit" property, instead create an inline
>   function for 32-bit timer driver to construct a 64-bit timer value.
>
>  drivers/timer/altera_timer.c  |  4 ++--
>  drivers/timer/sandbox_timer.c |  2 +-
>  drivers/timer/timer-uclass.c  |  8 +++-----
>  include/timer.h               | 23 ++++++++++++++++++++---
>  lib/time.c                    |  9 ++++++---
>  5 files changed, 32 insertions(+), 14 deletions(-)

Is there a binding file for this timer somewhere? If both altera and
sandbox share this property, should we add a generic binding? It
doesn't look like Linux has one though.

>
> diff --git a/drivers/timer/altera_timer.c b/drivers/timer/altera_timer.c
> index 6fe24f2..a7ed3cc 100644
> --- a/drivers/timer/altera_timer.c
> +++ b/drivers/timer/altera_timer.c
> @@ -34,7 +34,7 @@ struct altera_timer_platdata {
>         struct altera_timer_regs *regs;
>  };
>
> -static int altera_timer_get_count(struct udevice *dev, unsigned long *count)
> +static int altera_timer_get_count(struct udevice *dev, u64 *count)
>  {
>         struct altera_timer_platdata *plat = dev->platdata;
>         struct altera_timer_regs *const regs = plat->regs;
> @@ -46,7 +46,7 @@ static int altera_timer_get_count(struct udevice *dev, unsigned long *count)
>         /* Read timer value */
>         val = readl(&regs->snapl) & 0xffff;
>         val |= (readl(&regs->snaph) & 0xffff) << 16;
> -       *count = ~val;
> +       *count = timer_conv_64(~val);
>
>         return 0;
>  }
> diff --git a/drivers/timer/sandbox_timer.c b/drivers/timer/sandbox_timer.c
> index 4b20af2..00a9944 100644
> --- a/drivers/timer/sandbox_timer.c
> +++ b/drivers/timer/sandbox_timer.c
> @@ -18,7 +18,7 @@ void sandbox_timer_add_offset(unsigned long offset)
>         sandbox_timer_offset += offset;
>  }
>
> -static int sandbox_timer_get_count(struct udevice *dev, unsigned long *count)
> +static int sandbox_timer_get_count(struct udevice *dev, u64 *count)
>  {
>         *count = os_get_nsec() / 1000 + sandbox_timer_offset * 1000;
>
> diff --git a/drivers/timer/timer-uclass.c b/drivers/timer/timer-uclass.c
> index 0218591..1ef0012 100644
> --- a/drivers/timer/timer-uclass.c
> +++ b/drivers/timer/timer-uclass.c
> @@ -9,18 +9,16 @@
>  #include <errno.h>
>  #include <timer.h>
>
> -DECLARE_GLOBAL_DATA_PTR;
> -
>  /*
>   * Implement a timer uclass to work with lib/time.c. The timer is usually
> - * a 32 bits free-running up counter. The get_rate() method is used to get
> + * a 32/64 bits free-running up counter. The get_rate() method is used to get
>   * the input clock frequency of the timer. The get_count() method is used
> - * to get the current 32 bits count value. If the hardware is counting down,
> + * to get the current 64 bits count value. If the hardware is counting down,
>   * the value should be inversed inside the method. There may be no real
>   * tick, and no timer interrupt.
>   */
>
> -int timer_get_count(struct udevice *dev, unsigned long *count)
> +int timer_get_count(struct udevice *dev, u64 *count)
>  {
>         const struct timer_ops *ops = device_get_ops(dev);
>
> diff --git a/include/timer.h b/include/timer.h
> index ed5c685..4eed504 100644
> --- a/include/timer.h
> +++ b/include/timer.h
> @@ -7,6 +7,23 @@
>  #ifndef _TIMER_H_
>  #define _TIMER_H_
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * timer_conv_64 - convert 32-bit counter value to 64-bit
> + *
> + * @count: 32-bit counter value
> + * @return: 64-bit counter value
> + */
> +static inline u64 timer_conv_64(u32 count)
> +{
> +       /* increment tbh if tbl has rolled over */
> +       if (count < gd->timebase_l)
> +               gd->timebase_h++;
> +       gd->timebase_l = count;
> +       return ((u64)gd->timebase_h << 32) | gd->timebase_l;
> +}
> +
>  /*
>   * Get the current timer count
>   *
> @@ -14,7 +31,7 @@
>   * @count: pointer that returns the current timer count
>   * @return: 0 if OK, -ve on error
>   */
> -int timer_get_count(struct udevice *dev, unsigned long *count);
> +int timer_get_count(struct udevice *dev, u64 *count);
>
>  /*
>   * Get the timer input clock frequency
> @@ -35,10 +52,10 @@ struct timer_ops {
>          * Get the current timer count
>          *
>          * @dev: The timer device
> -        * @count: pointer that returns the current timer count
> +        * @count: pointer that returns the current 64-bit timer count
>          * @return: 0 if OK, -ve on error
>          */
> -       int (*get_count)(struct udevice *dev, unsigned long *count);
> +       int (*get_count)(struct udevice *dev, u64 *count);

Why do we need to change the API to 64-bit?

My only concern is that we are pretty happy with the 32-bit timer and
I'm not sure it should change. At present unsigned long can be 32-bit
on 32-bit machines.

>  };
>
>  /*
> diff --git a/lib/time.c b/lib/time.c
> index b001745..f37a662 100644
> --- a/lib/time.c
> +++ b/lib/time.c
> @@ -69,9 +69,9 @@ ulong notrace get_tbclk(void)
>         return timer_get_rate(gd->timer);
>  }
>
> -unsigned long notrace timer_read_counter(void)
> +uint64_t notrace get_ticks(void)
>  {
> -       unsigned long count;
> +       u64 count;
>         int ret;
>
>         ret = dm_timer_init();
> @@ -84,7 +84,8 @@ unsigned long notrace timer_read_counter(void)
>
>         return count;
>  }
> -#endif /* CONFIG_TIMER */
> +
> +#else /* !CONFIG_TIMER */
>
>  uint64_t __weak notrace get_ticks(void)
>  {
> @@ -97,6 +98,8 @@ uint64_t __weak notrace get_ticks(void)
>         return ((uint64_t)gd->timebase_h << 32) | gd->timebase_l;
>  }
>
> +#endif /* CONFIG_TIMER */
> +
>  /* Returns time in milliseconds */
>  static uint64_t notrace tick_to_time(uint64_t tick)
>  {
> --
> 1.8.2.1
>

Regards,
Simon