[U-Boot,4/6] x86: tsc: Try hardware calibration first

Message ID 1533893978-12838-4-git-send-email-bmeng.cn@gmail.com
State Accepted
Commit 165db7c426f1946376c3643fd635afd6b167e3ee
Delegated to: Bin Meng
Headers show
Series
  • [U-Boot,1/6] x86: coreboot: Add generic coreboot payload support
Related show

Commit Message

Bin Meng Aug. 10, 2018, 9:39 a.m.
At present if TSC frequency is provided in the device tree, it takes
precedence over hardware calibration result. This swaps the order to
try hardware calibration first and uses device tree as last resort.

This can be helpful when a generic dts (eg: coreboot/efi payload) is
supposed to work on as many hardware as possible, including emulators
like QEMU where TSC hardware calibration sometimes fails.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/timer/tsc_timer.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

Comments

Christian Gmeiner Aug. 14, 2018, 6:54 a.m. | #1
Am Fr., 10. Aug. 2018 um 11:40 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>:
>
> At present if TSC frequency is provided in the device tree, it takes
> precedence over hardware calibration result. This swaps the order to
> try hardware calibration first and uses device tree as last resort.
>
> This can be helpful when a generic dts (eg: coreboot/efi payload) is
> supposed to work on as many hardware as possible, including emulators
> like QEMU where TSC hardware calibration sometimes fails.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/timer/tsc_timer.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
> index 747f190..6473de2 100644
> --- a/drivers/timer/tsc_timer.c
> +++ b/drivers/timer/tsc_timer.c
> @@ -341,16 +341,12 @@ static int tsc_timer_get_count(struct udevice *dev, u64 *count)
>         return 0;
>  }
>
> -static void tsc_timer_ensure_setup(void)
> +static void tsc_timer_ensure_setup(bool stop)
>  {
>         if (gd->arch.tsc_base)
>                 return;
>         gd->arch.tsc_base = rdtsc();
>
> -       /*
> -        * If there is no clock frequency specified in the device tree,
> -        * calibrate it by ourselves.
> -        */
>         if (!gd->arch.clock_rate) {
>                 unsigned long fast_calibrate;
>
> @@ -366,7 +362,10 @@ static void tsc_timer_ensure_setup(void)
>                 if (fast_calibrate)
>                         goto done;
>
> -               panic("TSC frequency is ZERO");
> +               if (stop)
> +                       panic("TSC frequency is ZERO");
> +               else
> +                       return;
>
>  done:
>                 gd->arch.clock_rate = fast_calibrate * 1000000;
> @@ -377,11 +376,17 @@ static int tsc_timer_probe(struct udevice *dev)
>  {
>         struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>
> -       if (!uc_priv->clock_rate) {
> -               tsc_timer_ensure_setup();
> -               uc_priv->clock_rate = gd->arch.clock_rate;
> +       /* Try hardware calibration first */
> +       tsc_timer_ensure_setup(false);
> +       if (!gd->arch.clock_rate) {
> +               /*
> +                * Use the clock frequency specified in the
> +                * device tree as last resort
> +                */
> +               if (!uc_priv->clock_rate)

Where gets uc_priv->clock_rate set to something? DM should set zero-out
the whole uc_priv thing when bind/probe - or?


> +                       panic("TSC frequency is ZERO");
>         } else {
> -               gd->arch.tsc_base = rdtsc();
> +               uc_priv->clock_rate = gd->arch.clock_rate;
>         }
>
>         return 0;
> @@ -394,7 +399,7 @@ unsigned long notrace timer_early_get_rate(void)
>          * clock rate can only be calibrated via some hardware ways. Specifying
>          * it in the device tree won't work for the early timer.
>          */
> -       tsc_timer_ensure_setup();
> +       tsc_timer_ensure_setup(true);
>
>         return gd->arch.clock_rate;
>  }
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Bin Meng Aug. 14, 2018, 7:07 a.m. | #2
Hi Christian,

On Tue, Aug 14, 2018 at 2:54 PM, Christian Gmeiner
<christian.gmeiner@gmail.com> wrote:
> Am Fr., 10. Aug. 2018 um 11:40 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>:
>>
>> At present if TSC frequency is provided in the device tree, it takes
>> precedence over hardware calibration result. This swaps the order to
>> try hardware calibration first and uses device tree as last resort.
>>
>> This can be helpful when a generic dts (eg: coreboot/efi payload) is
>> supposed to work on as many hardware as possible, including emulators
>> like QEMU where TSC hardware calibration sometimes fails.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  drivers/timer/tsc_timer.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
>> index 747f190..6473de2 100644
>> --- a/drivers/timer/tsc_timer.c
>> +++ b/drivers/timer/tsc_timer.c
>> @@ -341,16 +341,12 @@ static int tsc_timer_get_count(struct udevice *dev, u64 *count)
>>         return 0;
>>  }
>>
>> -static void tsc_timer_ensure_setup(void)
>> +static void tsc_timer_ensure_setup(bool stop)
>>  {
>>         if (gd->arch.tsc_base)
>>                 return;
>>         gd->arch.tsc_base = rdtsc();
>>
>> -       /*
>> -        * If there is no clock frequency specified in the device tree,
>> -        * calibrate it by ourselves.
>> -        */
>>         if (!gd->arch.clock_rate) {
>>                 unsigned long fast_calibrate;
>>
>> @@ -366,7 +362,10 @@ static void tsc_timer_ensure_setup(void)
>>                 if (fast_calibrate)
>>                         goto done;
>>
>> -               panic("TSC frequency is ZERO");
>> +               if (stop)
>> +                       panic("TSC frequency is ZERO");
>> +               else
>> +                       return;
>>
>>  done:
>>                 gd->arch.clock_rate = fast_calibrate * 1000000;
>> @@ -377,11 +376,17 @@ static int tsc_timer_probe(struct udevice *dev)
>>  {
>>         struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>>
>> -       if (!uc_priv->clock_rate) {
>> -               tsc_timer_ensure_setup();
>> -               uc_priv->clock_rate = gd->arch.clock_rate;
>> +       /* Try hardware calibration first */
>> +       tsc_timer_ensure_setup(false);
>> +       if (!gd->arch.clock_rate) {
>> +               /*
>> +                * Use the clock frequency specified in the
>> +                * device tree as last resort
>> +                */
>> +               if (!uc_priv->clock_rate)
>
> Where gets uc_priv->clock_rate set to something? DM should set zero-out
> the whole uc_priv thing when bind/probe - or?
>

uc_priv->clock_rate is set in timer_pre_probe()

>
>> +                       panic("TSC frequency is ZERO");
>>         } else {
>> -               gd->arch.tsc_base = rdtsc();
>> +               uc_priv->clock_rate = gd->arch.clock_rate;
>>         }
>>
>>         return 0;
>> @@ -394,7 +399,7 @@ unsigned long notrace timer_early_get_rate(void)
>>          * clock rate can only be calibrated via some hardware ways. Specifying
>>          * it in the device tree won't work for the early timer.
>>          */
>> -       tsc_timer_ensure_setup();
>> +       tsc_timer_ensure_setup(true);
>>
>>         return gd->arch.clock_rate;
>>  }
>> --

Regards,
Bin
Christian Gmeiner Aug. 14, 2018, 8:35 a.m. | #3
Am Di., 14. Aug. 2018 um 09:07 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>:
>
> Hi Christian,
>
> On Tue, Aug 14, 2018 at 2:54 PM, Christian Gmeiner
> <christian.gmeiner@gmail.com> wrote:
> > Am Fr., 10. Aug. 2018 um 11:40 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>:
> >>
> >> At present if TSC frequency is provided in the device tree, it takes
> >> precedence over hardware calibration result. This swaps the order to
> >> try hardware calibration first and uses device tree as last resort.
> >>
> >> This can be helpful when a generic dts (eg: coreboot/efi payload) is
> >> supposed to work on as many hardware as possible, including emulators
> >> like QEMU where TSC hardware calibration sometimes fails.
> >>
> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >> ---
> >>
> >>  drivers/timer/tsc_timer.c | 27 ++++++++++++++++-----------
> >>  1 file changed, 16 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
> >> index 747f190..6473de2 100644
> >> --- a/drivers/timer/tsc_timer.c
> >> +++ b/drivers/timer/tsc_timer.c
> >> @@ -341,16 +341,12 @@ static int tsc_timer_get_count(struct udevice *dev, u64 *count)
> >>         return 0;
> >>  }
> >>
> >> -static void tsc_timer_ensure_setup(void)
> >> +static void tsc_timer_ensure_setup(bool stop)
> >>  {
> >>         if (gd->arch.tsc_base)
> >>                 return;
> >>         gd->arch.tsc_base = rdtsc();
> >>
> >> -       /*
> >> -        * If there is no clock frequency specified in the device tree,
> >> -        * calibrate it by ourselves.
> >> -        */
> >>         if (!gd->arch.clock_rate) {
> >>                 unsigned long fast_calibrate;
> >>
> >> @@ -366,7 +362,10 @@ static void tsc_timer_ensure_setup(void)
> >>                 if (fast_calibrate)
> >>                         goto done;
> >>
> >> -               panic("TSC frequency is ZERO");
> >> +               if (stop)
> >> +                       panic("TSC frequency is ZERO");
> >> +               else
> >> +                       return;
> >>
> >>  done:
> >>                 gd->arch.clock_rate = fast_calibrate * 1000000;
> >> @@ -377,11 +376,17 @@ static int tsc_timer_probe(struct udevice *dev)
> >>  {
> >>         struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> >>
> >> -       if (!uc_priv->clock_rate) {
> >> -               tsc_timer_ensure_setup();
> >> -               uc_priv->clock_rate = gd->arch.clock_rate;
> >> +       /* Try hardware calibration first */
> >> +       tsc_timer_ensure_setup(false);
> >> +       if (!gd->arch.clock_rate) {
> >> +               /*
> >> +                * Use the clock frequency specified in the
> >> +                * device tree as last resort
> >> +                */
> >> +               if (!uc_priv->clock_rate)
> >
> > Where gets uc_priv->clock_rate set to something? DM should set zero-out
> > the whole uc_priv thing when bind/probe - or?
> >
>
> uc_priv->clock_rate is set in timer_pre_probe()
>

Ah.. yes - with that new knowledge:

Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
Bin Meng Aug. 20, 2018, 5:57 a.m. | #4
On Tue, Aug 14, 2018 at 4:35 PM, Christian Gmeiner
<christian.gmeiner@gmail.com> wrote:
> Am Di., 14. Aug. 2018 um 09:07 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>:
>>
>> Hi Christian,
>>
>> On Tue, Aug 14, 2018 at 2:54 PM, Christian Gmeiner
>> <christian.gmeiner@gmail.com> wrote:
>> > Am Fr., 10. Aug. 2018 um 11:40 Uhr schrieb Bin Meng <bmeng.cn@gmail.com>:
>> >>
>> >> At present if TSC frequency is provided in the device tree, it takes
>> >> precedence over hardware calibration result. This swaps the order to
>> >> try hardware calibration first and uses device tree as last resort.
>> >>
>> >> This can be helpful when a generic dts (eg: coreboot/efi payload) is
>> >> supposed to work on as many hardware as possible, including emulators
>> >> like QEMU where TSC hardware calibration sometimes fails.
>> >>
>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> >> ---
>> >>
>> >>  drivers/timer/tsc_timer.c | 27 ++++++++++++++++-----------
>> >>  1 file changed, 16 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
>> >> index 747f190..6473de2 100644
>> >> --- a/drivers/timer/tsc_timer.c
>> >> +++ b/drivers/timer/tsc_timer.c
>> >> @@ -341,16 +341,12 @@ static int tsc_timer_get_count(struct udevice *dev, u64 *count)
>> >>         return 0;
>> >>  }
>> >>
>> >> -static void tsc_timer_ensure_setup(void)
>> >> +static void tsc_timer_ensure_setup(bool stop)
>> >>  {
>> >>         if (gd->arch.tsc_base)
>> >>                 return;
>> >>         gd->arch.tsc_base = rdtsc();
>> >>
>> >> -       /*
>> >> -        * If there is no clock frequency specified in the device tree,
>> >> -        * calibrate it by ourselves.
>> >> -        */
>> >>         if (!gd->arch.clock_rate) {
>> >>                 unsigned long fast_calibrate;
>> >>
>> >> @@ -366,7 +362,10 @@ static void tsc_timer_ensure_setup(void)
>> >>                 if (fast_calibrate)
>> >>                         goto done;
>> >>
>> >> -               panic("TSC frequency is ZERO");
>> >> +               if (stop)
>> >> +                       panic("TSC frequency is ZERO");
>> >> +               else
>> >> +                       return;
>> >>
>> >>  done:
>> >>                 gd->arch.clock_rate = fast_calibrate * 1000000;
>> >> @@ -377,11 +376,17 @@ static int tsc_timer_probe(struct udevice *dev)
>> >>  {
>> >>         struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> >>
>> >> -       if (!uc_priv->clock_rate) {
>> >> -               tsc_timer_ensure_setup();
>> >> -               uc_priv->clock_rate = gd->arch.clock_rate;
>> >> +       /* Try hardware calibration first */
>> >> +       tsc_timer_ensure_setup(false);
>> >> +       if (!gd->arch.clock_rate) {
>> >> +               /*
>> >> +                * Use the clock frequency specified in the
>> >> +                * device tree as last resort
>> >> +                */
>> >> +               if (!uc_priv->clock_rate)
>> >
>> > Where gets uc_priv->clock_rate set to something? DM should set zero-out
>> > the whole uc_priv thing when bind/probe - or?
>> >
>>
>> uc_priv->clock_rate is set in timer_pre_probe()
>>
>
> Ah.. yes - with that new knowledge:
>
> Reviewed-by: Christian Gmeiner <christian.gmeiner@gmail.com>
>

applied to u-boot-x86, thanks!

Patch

diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
index 747f190..6473de2 100644
--- a/drivers/timer/tsc_timer.c
+++ b/drivers/timer/tsc_timer.c
@@ -341,16 +341,12 @@  static int tsc_timer_get_count(struct udevice *dev, u64 *count)
 	return 0;
 }
 
-static void tsc_timer_ensure_setup(void)
+static void tsc_timer_ensure_setup(bool stop)
 {
 	if (gd->arch.tsc_base)
 		return;
 	gd->arch.tsc_base = rdtsc();
 
-	/*
-	 * If there is no clock frequency specified in the device tree,
-	 * calibrate it by ourselves.
-	 */
 	if (!gd->arch.clock_rate) {
 		unsigned long fast_calibrate;
 
@@ -366,7 +362,10 @@  static void tsc_timer_ensure_setup(void)
 		if (fast_calibrate)
 			goto done;
 
-		panic("TSC frequency is ZERO");
+		if (stop)
+			panic("TSC frequency is ZERO");
+		else
+			return;
 
 done:
 		gd->arch.clock_rate = fast_calibrate * 1000000;
@@ -377,11 +376,17 @@  static int tsc_timer_probe(struct udevice *dev)
 {
 	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
 
-	if (!uc_priv->clock_rate) {
-		tsc_timer_ensure_setup();
-		uc_priv->clock_rate = gd->arch.clock_rate;
+	/* Try hardware calibration first */
+	tsc_timer_ensure_setup(false);
+	if (!gd->arch.clock_rate) {
+		/*
+		 * Use the clock frequency specified in the
+		 * device tree as last resort
+		 */
+		if (!uc_priv->clock_rate)
+			panic("TSC frequency is ZERO");
 	} else {
-		gd->arch.tsc_base = rdtsc();
+		uc_priv->clock_rate = gd->arch.clock_rate;
 	}
 
 	return 0;
@@ -394,7 +399,7 @@  unsigned long notrace timer_early_get_rate(void)
 	 * clock rate can only be calibrated via some hardware ways. Specifying
 	 * it in the device tree won't work for the early timer.
 	 */
-	tsc_timer_ensure_setup();
+	tsc_timer_ensure_setup(true);
 
 	return gd->arch.clock_rate;
 }