| Message ID | 1447402284-26598-6-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
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? 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
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. > > 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). > >> }; >> >> /* >> 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
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. > Missed this comment in my previous post. I cannot find one too. But this is quite natural as without a frequency the timer does not count :) I believe 'clock-frequency' might be dynamically calculated on some platforms like ARM SoCs, which is similar to the ns16550 serial clock source frequency (we've had such discussion before). [snip] Regards, Bin
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
Hi Simon, On 2015年11月21日 05:10, Simon Glass wrote: >>>> @@ -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. > I would agree with Bin that the API should have been created as 64 bits. I considered this option before, because most part of lib/time.c uses 64 bits. It was only that I didn't have much confidence to change so many global code (without buildman). I realized this mistake when I wrote the sandbox timer. It is wasteful to truncate the 64 bits count to 32 bits, and then reconstruct 32 bits to 64 bits. This is what Bin wanted to resolve. There is no overhead for 32 bits timer, as the patch only shift the 64 bits conversion from lib/time.c to driver. Yet for 64 bits timer, this is a good saving. Best regards, Thomas
Hi, On 20 November 2015 at 17:41, Thomas Chou <thomas@wytron.com.tw> wrote: > Hi Simon, > > > On 2015年11月21日 05:10, Simon Glass wrote: >>>>> >>>>> @@ -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. >> > > I would agree with Bin that the API should have been created as 64 bits. I > considered this option before, because most part of lib/time.c uses 64 bits. > It was only that I didn't have much confidence to change so many global code > (without buildman). I realized this mistake when I wrote the sandbox timer. > It is wasteful to truncate the 64 bits count to 32 bits, and then > reconstruct 32 bits to 64 bits. This is what Bin wanted to resolve. There is > no overhead for 32 bits timer, as the patch only shift the 64 bits > conversion from lib/time.c to driver. Yet for 64 bits timer, this is a good > saving. OK thank you both for your explanation. This makes sense to me now. Acked-by: Simon Glass <sjg@chromium.org> Regards, Simon
Applied to u-boot-dm, thanks!
Hi, On 22 November 2015 at 09:21, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 20 November 2015 at 17:41, Thomas Chou <thomas@wytron.com.tw> wrote: >> Hi Simon, >> >> >> On 2015年11月21日 05:10, Simon Glass wrote: >>>>>> >>>>>> @@ -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. >>> >> >> I would agree with Bin that the API should have been created as 64 bits. I >> considered this option before, because most part of lib/time.c uses 64 bits. >> It was only that I didn't have much confidence to change so many global code >> (without buildman). I realized this mistake when I wrote the sandbox timer. >> It is wasteful to truncate the 64 bits count to 32 bits, and then >> reconstruct 32 bits to 64 bits. This is what Bin wanted to resolve. There is >> no overhead for 32 bits timer, as the patch only shift the 64 bits >> conversion from lib/time.c to driver. Yet for 64 bits timer, this is a good >> saving. > > OK thank you both for your explanation. This makes sense to me now. > > Acked-by: Simon Glass <sjg@chromium.org> Unfortunately this causes build errors for a few avr32 boards - e.g. grasshopper. I think the problem is the gd declaration in the timer.h header file. I don't think that is a good idea. Can we move it to the C file? Regards, Simon
Hi Simon, On 2015年11月24日 18:09, Simon Glass wrote: > > Unfortunately this causes build errors for a few avr32 boards - e.g. > grasshopper. > > I think the problem is the gd declaration in the timer.h header file. > I don't think that is a good idea. Can we move it to the C file? If you meant, "warning: input is not relaxable", it is fixed after commit f300dccde433 ("i2c, avr32: fix compiler warning "input is not relaxable""). The u-boot-dm/testing branch does not have such warning. Best regards, Thomas
Hi Thomas, On 24 November 2015 at 07:01, Thomas Chou <thomas@wytron.com.tw> wrote: > Hi Simon, > > On 2015年11月24日 18:09, Simon Glass wrote: >> >> >> Unfortunately this causes build errors for a few avr32 boards - e.g. >> grasshopper. >> >> I think the problem is the gd declaration in the timer.h header file. >> I don't think that is a good idea. Can we move it to the C file? > > > If you meant, "warning: input is not relaxable", it is fixed after commit > f300dccde433 ("i2c, avr32: fix compiler warning "input is not relaxable""). > The u-boot-dm/testing branch does not have such warning. Please try u-boot-dm/testing. This seems to be a different problem: avr32: + grasshopper +lib/time.c:20: warning: register used for two global register variables We should not declare the global_data pointer in a header file. Bin, are you able to take a look? Perhaps the function that needs it should move to the C file? Regards, Simon
Hi, On 24 November 2015 at 11:23, Simon Glass <sjg@chromium.org> wrote: > Hi Thomas, > > On 24 November 2015 at 07:01, Thomas Chou <thomas@wytron.com.tw> wrote: >> Hi Simon, >> >> On 2015年11月24日 18:09, Simon Glass wrote: >>> >>> >>> Unfortunately this causes build errors for a few avr32 boards - e.g. >>> grasshopper. >>> >>> I think the problem is the gd declaration in the timer.h header file. >>> I don't think that is a good idea. Can we move it to the C file? >> >> >> If you meant, "warning: input is not relaxable", it is fixed after commit >> f300dccde433 ("i2c, avr32: fix compiler warning "input is not relaxable""). >> The u-boot-dm/testing branch does not have such warning. > > Please try u-boot-dm/testing. This seems to be a different problem: > > avr32: + grasshopper > +lib/time.c:20: warning: register used for two global register variables > > We should not declare the global_data pointer in a header file. > > Bin, are you able to take a look? Perhaps the function that needs it > should move to the C file? I've had a crack at this and will send an updated patch. Regards, Simon
Hi Simon, On 2015年11月25日 02:23, Simon Glass wrote: > > Please try u-boot-dm/testing. This seems to be a different problem: > > avr32: + grasshopper > +lib/time.c:20: warning: register used for two global register variables > I am using the avr32 toolchain from kernel.org, and couldn't see the problem. gcc version 4.2.4-atmel.1.1.3.avr32linux.1 Would you please offer the link to download the avr32 toolchain you used? Best regards, Thomas
On Wed, Nov 25, 2015 at 11:20 AM, Thomas Chou <thomas@wytron.com.tw> wrote: > Hi Simon, > > On 2015年11月25日 02:23, Simon Glass wrote: >> >> >> Please try u-boot-dm/testing. This seems to be a different problem: >> >> avr32: + grasshopper >> +lib/time.c:20: warning: register used for two global register variables >> > > I am using the avr32 toolchain from kernel.org, and couldn't see the > problem. > gcc version 4.2.4-atmel.1.1.3.avr32linux.1 For me, I cannot reproduce the build error too with buildman. I am using the same toolchain as Thomas. > > Would you please offer the link to download the avr32 toolchain you used? > Regards, Bin
Hi, On 24 November 2015 at 23:44, Bin Meng <bmeng.cn@gmail.com> wrote: > On Wed, Nov 25, 2015 at 11:20 AM, Thomas Chou <thomas@wytron.com.tw> wrote: >> Hi Simon, >> >> On 2015年11月25日 02:23, Simon Glass wrote: >>> >>> >>> Please try u-boot-dm/testing. This seems to be a different problem: >>> >>> avr32: + grasshopper >>> +lib/time.c:20: warning: register used for two global register variables >>> >> >> I am using the avr32 toolchain from kernel.org, and couldn't see the >> problem. >> gcc version 4.2.4-atmel.1.1.3.avr32linux.1 > > For me, I cannot reproduce the build error too with buildman. I am > using the same toolchain as Thomas. >> >> Would you please offer the link to download the avr32 toolchain you used? It is avr32-buildroot-linux-uclibc-gcc Target: avr32-buildroot-linux-uclibc Configured with: /home/sjg/c/buildroot/buildroot-2013.02/output/toolchain/gcc-4.2.2-avr32-2.1.5/configure --prefix=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr --build=x86_64-unknown-linux-gnu --host=x86_64-unknown-linux-gnu --target=avr32-buildroot-linux-uclibc --enable-languages=c --with-sysroot=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr/avr32-buildroot-linux-uclibc/sysroot --with-build-time-tools=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr/avr32-buildroot-linux-uclibc/bin --disable-__cxa_atexit --enable-target-optspace --disable-libquadmath --disable-libgomp --with-gnu-ld --disable-libssp --disable-multilib --disable-tls --enable-shared --with-gmp=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr --with-mpfr=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr --disable-nls --enable-threads --disable-largefile --disable-libmudflap Thread model: posix gcc version 4.2.2-atmel.1.0.8 So fairly old. I'm not sure if I built it or downloaded it - it was a while ago. So perhaps this is a toolchain problem. But still I don't think we want to declare the global_data pointer in a header file. Regards, Simon
On 25 November 2015 at 09:51, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 24 November 2015 at 23:44, Bin Meng <bmeng.cn@gmail.com> wrote: >> On Wed, Nov 25, 2015 at 11:20 AM, Thomas Chou <thomas@wytron.com.tw> wrote: >>> Hi Simon, >>> >>> On 2015年11月25日 02:23, Simon Glass wrote: >>>> >>>> >>>> Please try u-boot-dm/testing. This seems to be a different problem: >>>> >>>> avr32: + grasshopper >>>> +lib/time.c:20: warning: register used for two global register variables >>>> >>> >>> I am using the avr32 toolchain from kernel.org, and couldn't see the >>> problem. >>> gcc version 4.2.4-atmel.1.1.3.avr32linux.1 >> >> For me, I cannot reproduce the build error too with buildman. I am >> using the same toolchain as Thomas. >>> >>> Would you please offer the link to download the avr32 toolchain you used? > > It is avr32-buildroot-linux-uclibc-gcc > > Target: avr32-buildroot-linux-uclibc > Configured with: > /home/sjg/c/buildroot/buildroot-2013.02/output/toolchain/gcc-4.2.2-avr32-2.1.5/configure > --prefix=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr > --build=x86_64-unknown-linux-gnu --host=x86_64-unknown-linux-gnu > --target=avr32-buildroot-linux-uclibc --enable-languages=c > --with-sysroot=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr/avr32-buildroot-linux-uclibc/sysroot > --with-build-time-tools=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr/avr32-buildroot-linux-uclibc/bin > --disable-__cxa_atexit --enable-target-optspace --disable-libquadmath > --disable-libgomp --with-gnu-ld --disable-libssp --disable-multilib > --disable-tls --enable-shared > --with-gmp=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr > --with-mpfr=/home/sjg/c/buildroot/buildroot-2013.02/output/host/usr > --disable-nls --enable-threads --disable-largefile > --disable-libmudflap > Thread model: posix > gcc version 4.2.2-atmel.1.0.8 > > So fairly old. > > I'm not sure if I built it or downloaded it - it was a while ago. > > So perhaps this is a toolchain problem. But still I don't think we > want to declare the global_data pointer in a header file. Applied to u-boot-dm, replacing the earlier v2 patch.
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); }; /* 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) {
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(-)