[U-Boot] [PATCH v3 05/11] dm: timer: Support 64-bit counter
Simon Glass
sjg at chromium.org
Fri Nov 20 22:10:16 CET 2015
Hi Bin,
On 15 November 2015 at 19:19, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Sat, Nov 14, 2015 at 10:04 AM, Simon Glass <sjg at chromium.org> wrote:
>> 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(®s->snapl) & 0xffff;
>>> val |= (readl(®s->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?
>
> IMHO, this API should be created to be accept a 64-bit value in the
> first place. As you see the U-Boot time APIs in lib/time.c like
> get_ticks() has been using 64-bit value.
Right, but the point of this timer is for time delays. Making everyone
one of those 64-bit on a 32-bit machine does not seem like a win to
me.
>
>>
>> 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.
>
> 32-bit timer has to be converted to 64-bit value as get_ticks()
> requires 64-bit. As for 'unsigned long can be 32-bit on 32-bit
> machines', I think we should explicitly declare its width as this is
> hardware limitation (32-bit timer can only produce 32-bit value, even
> if it is on a 64-bit machine, where long on 64-bit means 64-bit, which
> is wrong for this 32-bit timer hardware).
This is a bit messy, but for now I think we should follow what
get_timer() does. It would be worth getting more input on the list I
think.
>
>>
>>> };
>>>
>>> /*
>>> 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)
>>> {
>>> --
>
> Regards,
> Bin
Regards,
Simon
More information about the U-Boot
mailing list