Hi Bin,
On 15 November 2015 at 19:19, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sat, Nov 14, 2015 at 10:04 AM, Simon Glass sjg@chromium.org wrote:
Hi Bin,
On 13 November 2015 at 01:11, Bin Meng bmeng.cn@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@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