[U-Boot,v6,1/2] x86: Add TSC-specific timer functions

Message ID 269698c303c3da454b34f149d4a3562fc57091f2.1523570597.git.ivan.gorinov@intel.com
State New
Delegated to: Bin Meng
Headers show
Series
  • timer: Add High Precision Event Timers (HPET) support
Related show

Commit Message

Ivan Gorinov April 12, 2018, 10:12 p.m.
Coreboot timestamp functions and Quark memory reference code use
get_tbclk() to get TSC frequency. This will not work if another
early timer is selected.

Add tsc_rate_mhz() function and use it in the code that specifically
needs to get TSC rate regardless of currently selected early timer.

Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
---
 arch/x86/cpu/coreboot/timestamp.c |  2 +-
 arch/x86/cpu/quark/mrc_util.c     | 13 ++++++-------
 arch/x86/include/asm/u-boot-x86.h |  2 +-
 drivers/timer/tsc_timer.c         | 33 ++++++++++++++++++++++++---------
 4 files changed, 32 insertions(+), 18 deletions(-)

Comments

Andy Shevchenko April 20, 2018, 12:25 p.m. | #1
On Thu, 2018-04-12 at 15:12 -0700, Ivan Gorinov wrote:
> Coreboot timestamp functions and Quark memory reference code use
> get_tbclk() to get TSC frequency. This will not work if another
> early timer is selected.
> 
> Add tsc_rate_mhz() function and use it in the code that specifically
> needs to get TSC rate regardless of currently selected early timer.


>  void delay_n(uint32_t ns)
>  {
>  	/* 1000 MHz clock has 1ns period --> no conversion required
> */
> -	uint64_t final_tsc = rdtsc();
> +	uint64_t start_tsc = rdtsc();
> +	uint64_t ticks;
>  
> -	final_tsc += ((get_tbclk_mhz() * ns) / 1000);
> -
> -	while (rdtsc() < final_tsc)
> -		;
> +	ticks = (tsc_rate_mhz() * ns) / 1000;

> +	while (rdtsc() - start_tsc < ticks);

I would rather preserve existing style.

>  }
 
>  /* Delay number of microseconds */
> -void delay_u(uint32_t ms)
> +void delay_u(uint32_t us)
>  {
>  	/* 64-bit math is not an option, just use loops */
> -	while (ms--)
> +	while (us--)
>  		delay_n(1000);
>  }

This is a separate change.
Ivan Gorinov April 20, 2018, 6 p.m. | #2
On Fri, Apr 20, 2018 at 06:25:08AM -0600, Andy Shevchenko wrote:
> > Coreboot timestamp functions and Quark memory reference code use
> > get_tbclk() to get TSC frequency. This will not work if another
> > early timer is selected.
> > 
> > Add tsc_rate_mhz() function and use it in the code that specifically
> > needs to get TSC rate regardless of currently selected early timer.
> 
> 
> >  void delay_n(uint32_t ns)
> >  {
> >  	/* 1000 MHz clock has 1ns period --> no conversion required
> > */
> > -	uint64_t final_tsc = rdtsc();
> > +	uint64_t start_tsc = rdtsc();
> > +	uint64_t ticks;
> >  
> > -	final_tsc += ((get_tbclk_mhz() * ns) / 1000);
> > -
> > -	while (rdtsc() < final_tsc)
> > -		;
> > +	ticks = (tsc_rate_mhz() * ns) / 1000;
> 
> > +	while (rdtsc() - start_tsc < ticks);
> 
> I would rather preserve existing style.

OK. Existing style does not correctly handle overflow,
but for a 64-bit counter it's a very unlikely event.
Bin Meng April 23, 2018, 7:38 a.m. | #3
Hi Ivan,

On Fri, Apr 13, 2018 at 6:12 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> Coreboot timestamp functions and Quark memory reference code use
> get_tbclk() to get TSC frequency. This will not work if another
> early timer is selected.
>

Thanks for working on this. But get_tbclk() is one API provided by the
timer library. The get_tbclk_mhz() is something that is implemented by
the TSC timer driver, so can we get rid of the get_tbclk_mhz() and use
the get_tbclk() instead in coreboot/timestamp.c and quark/mrc_util.c?

> Add tsc_rate_mhz() function and use it in the code that specifically
> needs to get TSC rate regardless of currently selected early timer.
>
> Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> ---
>  arch/x86/cpu/coreboot/timestamp.c |  2 +-
>  arch/x86/cpu/quark/mrc_util.c     | 13 ++++++-------
>  arch/x86/include/asm/u-boot-x86.h |  2 +-
>  drivers/timer/tsc_timer.c         | 33 ++++++++++++++++++++++++---------
>  4 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/arch/x86/cpu/coreboot/timestamp.c b/arch/x86/cpu/coreboot/timestamp.c
> index b382795..05bb214 100644
> --- a/arch/x86/cpu/coreboot/timestamp.c
> +++ b/arch/x86/cpu/coreboot/timestamp.c
> @@ -78,7 +78,7 @@ int timestamp_add_to_bootstage(void)
>                 if (name) {
>                         bootstage_add_record(0, name, BOOTSTAGEF_ALLOC,
>                                              tse->entry_stamp /
> -                                                       get_tbclk_mhz());
> +                                                       tsc_rate_mhz());
>                 }
>         }
>
> diff --git a/arch/x86/cpu/quark/mrc_util.c b/arch/x86/cpu/quark/mrc_util.c
> index fac2d72..4794395 100644
> --- a/arch/x86/cpu/quark/mrc_util.c
> +++ b/arch/x86/cpu/quark/mrc_util.c
> @@ -57,19 +57,18 @@ void mrc_post_code(uint8_t major, uint8_t minor)
>  void delay_n(uint32_t ns)
>  {
>         /* 1000 MHz clock has 1ns period --> no conversion required */
> -       uint64_t final_tsc = rdtsc();
> +       uint64_t start_tsc = rdtsc();
> +       uint64_t ticks;
>
> -       final_tsc += ((get_tbclk_mhz() * ns) / 1000);
> -
> -       while (rdtsc() < final_tsc)
> -               ;
> +       ticks = (tsc_rate_mhz() * ns) / 1000;
> +       while (rdtsc() - start_tsc < ticks);
>  }
>
>  /* Delay number of microseconds */
> -void delay_u(uint32_t ms)
> +void delay_u(uint32_t us)
>  {
>         /* 64-bit math is not an option, just use loops */
> -       while (ms--)
> +       while (us--)
>                 delay_n(1000);
>  }
>
> diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
> index 187fe5f..de68120 100644
> --- a/arch/x86/include/asm/u-boot-x86.h
> +++ b/arch/x86/include/asm/u-boot-x86.h
> @@ -29,7 +29,7 @@ int cleanup_before_linux(void);
>  void timer_isr(void *);
>  typedef void (timer_fnc_t) (void);
>  int register_timer_isr (timer_fnc_t *isr_func);
> -unsigned long get_tbclk_mhz(void);
> +unsigned long tsc_rate_mhz(void);
>  void timer_set_base(uint64_t base);
>  int i8254_init(void);
>
> diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
> index 9296de6..98cbf12 100644
> --- a/drivers/timer/tsc_timer.c
> +++ b/drivers/timer/tsc_timer.c
> @@ -277,15 +277,12 @@ success:
>         return delta / 1000;
>  }
>
> -/* Get the speed of the TSC timer in MHz */
> -unsigned notrace long get_tbclk_mhz(void)
> -{
> -       return get_tbclk() / 1000000;
> -}
> -
>  static ulong get_ms_timer(void)
>  {
> -       return (get_ticks() * 1000) / get_tbclk();
> +       if (gd->arch.clock_rate == 0)
> +               return 0;
> +
> +       return (rdtsc() * 1000) / gd->arch.clock_rate;
>  }
>
>  ulong get_timer(ulong base)
> @@ -295,7 +292,10 @@ ulong get_timer(ulong base)
>
>  ulong notrace timer_get_us(void)
>  {
> -       return get_ticks() / get_tbclk_mhz();
> +       if (gd->arch.clock_rate == 0)
> +               return 0;
> +
> +       return (rdtsc() * 1000000) / gd->arch.clock_rate;
>  }
>
>  ulong timer_get_boot_us(void)
> @@ -308,7 +308,7 @@ void __udelay(unsigned long usec)
>         u64 now = get_ticks();
>         u64 stop;
>
> -       stop = now + usec * get_tbclk_mhz();
> +       stop = now + ((u64)usec * gd->arch.clock_rate) / 1000000;
>
>         while ((int64_t)(stop - get_ticks()) > 0)
>  #if defined(CONFIG_QEMU) && defined(CONFIG_SMP)
> @@ -355,6 +355,14 @@ static void tsc_timer_ensure_setup(void)
>         }
>  }
>
> +/* Get the speed of the TSC timer in MHz */
> +unsigned notrace long tsc_rate_mhz(void)
> +{
> +       tsc_timer_ensure_setup();
> +
> +       return gd->arch.clock_rate / 1000000;
> +}
> +
>  static int tsc_timer_probe(struct udevice *dev)
>  {
>         struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> @@ -365,6 +373,13 @@ static int tsc_timer_probe(struct udevice *dev)
>         return 0;
>  }
>
> +int timer_init(void)
> +{
> +       tsc_timer_ensure_setup();
> +
> +       return 0;
> +}
> +
>  unsigned long notrace timer_early_get_rate(void)
>  {
>         tsc_timer_ensure_setup();
> --

Regards,
Bin
Bin Meng April 23, 2018, 7:53 a.m. | #4
Hi Ivan,

On Mon, Apr 23, 2018 at 3:38 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Ivan,
>
> On Fri, Apr 13, 2018 at 6:12 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
>> Coreboot timestamp functions and Quark memory reference code use
>> get_tbclk() to get TSC frequency. This will not work if another
>> early timer is selected.
>>
>
> Thanks for working on this. But get_tbclk() is one API provided by the
> timer library. The get_tbclk_mhz() is something that is implemented by
> the TSC timer driver, so can we get rid of the get_tbclk_mhz() and use
> the get_tbclk() instead in coreboot/timestamp.c and quark/mrc_util.c?
>

Further request on TSC timer driver clean up, in order to make HPET
work with either TSC or HPET being the U-Boot (early) timer, we
should:

- Remove get_tbclk_mhz() in tsc_timer.c
- Remove get_timer() in tsc_timer.c
- Remove timer_get_us() in tsc_timer.c
- Remove timer_get_boot_us() in tsc_timer.c
- Move __udelay() implementation in tsc_timer.c to
arch/x86/cpu/qemu/qemu.c, as this is the QEMU specific support. For
other platforms, use the default one provided in lib/time.c

Regards,
Bin
Andy Shevchenko April 23, 2018, 8:22 a.m. | #5
On Fri, 2018-04-20 at 11:00 -0700, Ivan Gorinov wrote:
> On Fri, Apr 20, 2018 at 06:25:08AM -0600, Andy Shevchenko wrote:

> > > -	while (rdtsc() < final_tsc)
> > > -		;

> > > +	while (rdtsc() - start_tsc < ticks);
> > 
> > I would rather preserve existing style.
> 
> OK. Existing style does not correctly handle overflow,
> but for a 64-bit counter it's a very unlikely event.

You didn't get me. I'm just talking about style, not about
functionality.

See above for lines I left in this reply.
Ivan Gorinov April 23, 2018, 11:56 p.m. | #6
Hi Bin,

On Mon, Apr 23, 2018 at 01:38:05AM -0600, Bin Meng wrote:
> > Coreboot timestamp functions and Quark memory reference code use
> > get_tbclk() to get TSC frequency. This will not work if another
> > early timer is selected.
> 
> Thanks for working on this. But get_tbclk() is one API provided by the
> timer library. The get_tbclk_mhz() is something that is implemented by
> the TSC timer driver, so can we get rid of the get_tbclk_mhz() and use
> the get_tbclk() instead in coreboot/timestamp.c and quark/mrc_util.c?

The Coreboot timestamp code and Quark MRC specifically use rdtsc().
We can replace it with timer_early_get_count() or provide a function
to get the TSC frequency even when another early timer is selected.

> > Add tsc_rate_mhz() function and use it in the code that specifically
> > needs to get TSC rate regardless of currently selected early timer.
> >
> > Signed-off-by: Ivan Gorinov <ivan.gorinov@intel.com>
> > ---
> >  arch/x86/cpu/coreboot/timestamp.c |  2 +-
> >  arch/x86/cpu/quark/mrc_util.c     | 13 ++++++-------
> >  arch/x86/include/asm/u-boot-x86.h |  2 +-
> >  drivers/timer/tsc_timer.c         | 33 ++++++++++++++++++++++++---------
> >  4 files changed, 32 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/x86/cpu/coreboot/timestamp.c b/arch/x86/cpu/coreboot/timestamp.c
> > index b382795..05bb214 100644
> > --- a/arch/x86/cpu/coreboot/timestamp.c
> > +++ b/arch/x86/cpu/coreboot/timestamp.c
> > @@ -78,7 +78,7 @@ int timestamp_add_to_bootstage(void)
> >                 if (name) {
> >                         bootstage_add_record(0, name, BOOTSTAGEF_ALLOC,
> >                                              tse->entry_stamp /
> > -                                                       get_tbclk_mhz());
> > +                                                       tsc_rate_mhz());
> >                 }
> >         }
> >
> > diff --git a/arch/x86/cpu/quark/mrc_util.c b/arch/x86/cpu/quark/mrc_util.c
> > index fac2d72..4794395 100644
> > --- a/arch/x86/cpu/quark/mrc_util.c
> > +++ b/arch/x86/cpu/quark/mrc_util.c
> > @@ -57,19 +57,18 @@ void mrc_post_code(uint8_t major, uint8_t minor)
> >  void delay_n(uint32_t ns)
> >  {
> >         /* 1000 MHz clock has 1ns period --> no conversion required */
> > -       uint64_t final_tsc = rdtsc();
> > +       uint64_t start_tsc = rdtsc();
> > +       uint64_t ticks;
> >
> > -       final_tsc += ((get_tbclk_mhz() * ns) / 1000);
> > -
> > -       while (rdtsc() < final_tsc)
> > -               ;
> > +       ticks = (tsc_rate_mhz() * ns) / 1000;
> > +       while (rdtsc() - start_tsc < ticks);
> >  }
> >
> >  /* Delay number of microseconds */
> > -void delay_u(uint32_t ms)
> > +void delay_u(uint32_t us)
> >  {
> >         /* 64-bit math is not an option, just use loops */
> > -       while (ms--)
> > +       while (us--)
> >                 delay_n(1000);
> >  }
> >
> > diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
> > index 187fe5f..de68120 100644
> > --- a/arch/x86/include/asm/u-boot-x86.h
> > +++ b/arch/x86/include/asm/u-boot-x86.h
> > @@ -29,7 +29,7 @@ int cleanup_before_linux(void);
> >  void timer_isr(void *);
> >  typedef void (timer_fnc_t) (void);
> >  int register_timer_isr (timer_fnc_t *isr_func);
> > -unsigned long get_tbclk_mhz(void);
> > +unsigned long tsc_rate_mhz(void);
> >  void timer_set_base(uint64_t base);
> >  int i8254_init(void);
> >
> > diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
> > index 9296de6..98cbf12 100644
> > --- a/drivers/timer/tsc_timer.c
> > +++ b/drivers/timer/tsc_timer.c
> > @@ -277,15 +277,12 @@ success:
> >         return delta / 1000;
> >  }
> >
> > -/* Get the speed of the TSC timer in MHz */
> > -unsigned notrace long get_tbclk_mhz(void)
> > -{
> > -       return get_tbclk() / 1000000;
> > -}
> > -
> >  static ulong get_ms_timer(void)
> >  {
> > -       return (get_ticks() * 1000) / get_tbclk();
> > +       if (gd->arch.clock_rate == 0)
> > +               return 0;
> > +
> > +       return (rdtsc() * 1000) / gd->arch.clock_rate;
> >  }
> >
> >  ulong get_timer(ulong base)
> > @@ -295,7 +292,10 @@ ulong get_timer(ulong base)
> >
> >  ulong notrace timer_get_us(void)
> >  {
> > -       return get_ticks() / get_tbclk_mhz();
> > +       if (gd->arch.clock_rate == 0)
> > +               return 0;
> > +
> > +       return (rdtsc() * 1000000) / gd->arch.clock_rate;
> >  }
> >
> >  ulong timer_get_boot_us(void)
> > @@ -308,7 +308,7 @@ void __udelay(unsigned long usec)
> >         u64 now = get_ticks();
> >         u64 stop;
> >
> > -       stop = now + usec * get_tbclk_mhz();
> > +       stop = now + ((u64)usec * gd->arch.clock_rate) / 1000000;
> >
> >         while ((int64_t)(stop - get_ticks()) > 0)
> >  #if defined(CONFIG_QEMU) && defined(CONFIG_SMP)
> > @@ -355,6 +355,14 @@ static void tsc_timer_ensure_setup(void)
> >         }
> >  }
> >
> > +/* Get the speed of the TSC timer in MHz */
> > +unsigned notrace long tsc_rate_mhz(void)
> > +{
> > +       tsc_timer_ensure_setup();
> > +
> > +       return gd->arch.clock_rate / 1000000;
> > +}
> > +
> >  static int tsc_timer_probe(struct udevice *dev)
> >  {
> >         struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > @@ -365,6 +373,13 @@ static int tsc_timer_probe(struct udevice *dev)
> >         return 0;
> >  }
> >
> > +int timer_init(void)
> > +{
> > +       tsc_timer_ensure_setup();
> > +
> > +       return 0;
> > +}
> > +
> >  unsigned long notrace timer_early_get_rate(void)
> >  {
> >         tsc_timer_ensure_setup();
> > --
Bin Meng April 24, 2018, 8:41 a.m. | #7
Hi Ivan,

On Tue, Apr 24, 2018 at 7:56 AM, Ivan Gorinov <ivan.gorinov@intel.com> wrote:
> Hi Bin,
>
> On Mon, Apr 23, 2018 at 01:38:05AM -0600, Bin Meng wrote:
>> > Coreboot timestamp functions and Quark memory reference code use
>> > get_tbclk() to get TSC frequency. This will not work if another
>> > early timer is selected.
>>
>> Thanks for working on this. But get_tbclk() is one API provided by the
>> timer library. The get_tbclk_mhz() is something that is implemented by
>> the TSC timer driver, so can we get rid of the get_tbclk_mhz() and use
>> the get_tbclk() instead in coreboot/timestamp.c and quark/mrc_util.c?
>
> The Coreboot timestamp code and Quark MRC specifically use rdtsc().
> We can replace it with timer_early_get_count() or provide a function
> to get the TSC frequency even when another early timer is selected.
>

Good catch. Yes, we should fix coreboot timestamp code and Quark MRC
codes to not explicitly call rdtsc.

Another driver that explicitly calls rdtsc() is hw_watchdog_reset() in
watchdog/tangier_wdt.c driver. We need fix that too.

Regards,
Bin

Patch

diff --git a/arch/x86/cpu/coreboot/timestamp.c b/arch/x86/cpu/coreboot/timestamp.c
index b382795..05bb214 100644
--- a/arch/x86/cpu/coreboot/timestamp.c
+++ b/arch/x86/cpu/coreboot/timestamp.c
@@ -78,7 +78,7 @@  int timestamp_add_to_bootstage(void)
 		if (name) {
 			bootstage_add_record(0, name, BOOTSTAGEF_ALLOC,
 					     tse->entry_stamp /
-							get_tbclk_mhz());
+							tsc_rate_mhz());
 		}
 	}
 
diff --git a/arch/x86/cpu/quark/mrc_util.c b/arch/x86/cpu/quark/mrc_util.c
index fac2d72..4794395 100644
--- a/arch/x86/cpu/quark/mrc_util.c
+++ b/arch/x86/cpu/quark/mrc_util.c
@@ -57,19 +57,18 @@  void mrc_post_code(uint8_t major, uint8_t minor)
 void delay_n(uint32_t ns)
 {
 	/* 1000 MHz clock has 1ns period --> no conversion required */
-	uint64_t final_tsc = rdtsc();
+	uint64_t start_tsc = rdtsc();
+	uint64_t ticks;
 
-	final_tsc += ((get_tbclk_mhz() * ns) / 1000);
-
-	while (rdtsc() < final_tsc)
-		;
+	ticks = (tsc_rate_mhz() * ns) / 1000;
+	while (rdtsc() - start_tsc < ticks);
 }
 
 /* Delay number of microseconds */
-void delay_u(uint32_t ms)
+void delay_u(uint32_t us)
 {
 	/* 64-bit math is not an option, just use loops */
-	while (ms--)
+	while (us--)
 		delay_n(1000);
 }
 
diff --git a/arch/x86/include/asm/u-boot-x86.h b/arch/x86/include/asm/u-boot-x86.h
index 187fe5f..de68120 100644
--- a/arch/x86/include/asm/u-boot-x86.h
+++ b/arch/x86/include/asm/u-boot-x86.h
@@ -29,7 +29,7 @@  int cleanup_before_linux(void);
 void timer_isr(void *);
 typedef void (timer_fnc_t) (void);
 int register_timer_isr (timer_fnc_t *isr_func);
-unsigned long get_tbclk_mhz(void);
+unsigned long tsc_rate_mhz(void);
 void timer_set_base(uint64_t base);
 int i8254_init(void);
 
diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
index 9296de6..98cbf12 100644
--- a/drivers/timer/tsc_timer.c
+++ b/drivers/timer/tsc_timer.c
@@ -277,15 +277,12 @@  success:
 	return delta / 1000;
 }
 
-/* Get the speed of the TSC timer in MHz */
-unsigned notrace long get_tbclk_mhz(void)
-{
-	return get_tbclk() / 1000000;
-}
-
 static ulong get_ms_timer(void)
 {
-	return (get_ticks() * 1000) / get_tbclk();
+	if (gd->arch.clock_rate == 0)
+		return 0;
+
+	return (rdtsc() * 1000) / gd->arch.clock_rate;
 }
 
 ulong get_timer(ulong base)
@@ -295,7 +292,10 @@  ulong get_timer(ulong base)
 
 ulong notrace timer_get_us(void)
 {
-	return get_ticks() / get_tbclk_mhz();
+	if (gd->arch.clock_rate == 0)
+		return 0;
+
+	return (rdtsc() * 1000000) / gd->arch.clock_rate;
 }
 
 ulong timer_get_boot_us(void)
@@ -308,7 +308,7 @@  void __udelay(unsigned long usec)
 	u64 now = get_ticks();
 	u64 stop;
 
-	stop = now + usec * get_tbclk_mhz();
+	stop = now + ((u64)usec * gd->arch.clock_rate) / 1000000;
 
 	while ((int64_t)(stop - get_ticks()) > 0)
 #if defined(CONFIG_QEMU) && defined(CONFIG_SMP)
@@ -355,6 +355,14 @@  static void tsc_timer_ensure_setup(void)
 	}
 }
 
+/* Get the speed of the TSC timer in MHz */
+unsigned notrace long tsc_rate_mhz(void)
+{
+	tsc_timer_ensure_setup();
+
+	return gd->arch.clock_rate / 1000000;
+}
+
 static int tsc_timer_probe(struct udevice *dev)
 {
 	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
@@ -365,6 +373,13 @@  static int tsc_timer_probe(struct udevice *dev)
 	return 0;
 }
 
+int timer_init(void)
+{
+       tsc_timer_ensure_setup();
+
+       return 0;
+}
+
 unsigned long notrace timer_early_get_rate(void)
 {
 	tsc_timer_ensure_setup();