[U-Boot,v5,5/8] x86: slimbootloader: Set TSC information for timer driver
diff mbox series

Message ID A1484485FD99714DB2AB2C5EF81E7AC2AA746862@ORSMSX116.amr.corp.intel.com
State Superseded
Delegated to: Bin Meng
Headers show
Series
  • x86: Add basic Slim Bootloader payload support
Related show

Commit Message

Park, Aiden July 17, 2019, 4:41 a.m. UTC
Slim Bootloader provides TSC clock information in its performance
info hob. For now, TSC clock information is only used for timer driver
from the performance info hob.
- Get TSC frequency from performance info hob
- Set tsc_base and clock_rate for timer driver

Signed-off-by: Aiden Park <aiden.park@intel.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---

Changes in v3:
  * Use HOB function from the common HOB library

 arch/x86/cpu/slimbootloader/slimbootloader.c  | 34 +++++++++++++++++++
 .../asm/arch-slimbootloader/slimbootloader.h  | 30 ++++++++++++++++
 2 files changed, 64 insertions(+)

--
2.20.1

Comments

Andy Shevchenko July 22, 2019, 3:41 p.m. UTC | #1
On Wed, Jul 17, 2019 at 7:41 AM Park, Aiden <aiden.park@intel.com> wrote:
>
> Slim Bootloader provides TSC clock information in its performance
> info hob. For now, TSC clock information is only used for timer driver
> from the performance info hob.
> - Get TSC frequency from performance info hob
> - Set tsc_base and clock_rate for timer driver

So why do we need this at all? We have a common TSC driver that works
for all x86.

> +#define LOADER_PERFORMANCE_INFO_GUID \
> +       { \
> +       0x868204be, 0x23d0, 0x4ff9, \
> +       { 0xac, 0x34, 0xb9, 0x95, 0xac, 0x04, 0xb1, 0xb9 } \
> +       }

Use EFI_GUID(), please.

> +struct performance_info {

Same, you better to make less generic names for data structs.

> +} __packed;

Same question. If it's aligned naturally by 32-bit boundaries, Why do
you need __packed?
Bin Meng July 23, 2019, 6:02 a.m. UTC | #2
On Mon, Jul 22, 2019 at 11:41 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jul 17, 2019 at 7:41 AM Park, Aiden <aiden.park@intel.com> wrote:
> >
> > Slim Bootloader provides TSC clock information in its performance
> > info hob. For now, TSC clock information is only used for timer driver
> > from the performance info hob.
> > - Get TSC frequency from performance info hob
> > - Set tsc_base and clock_rate for timer driver
>
> So why do we need this at all? We have a common TSC driver that works
> for all x86.
>

Ah, yes! Thanks for the review. The U-Boot native TSC driver should
support this out of the box.

Aiden, could you please add a TSC DT node in the slimbootloader.dts
file, and have a try?

If the native U-Boot TSC driver does not work on your platform, I
think you need send a patch to update the TSC driver directly.

> > +#define LOADER_PERFORMANCE_INFO_GUID \
> > +       { \
> > +       0x868204be, 0x23d0, 0x4ff9, \
> > +       { 0xac, 0x34, 0xb9, 0x95, 0xac, 0x04, 0xb1, 0xb9 } \
> > +       }
>
> Use EFI_GUID(), please.
>
> > +struct performance_info {
>
> Same, you better to make less generic names for data structs.
>
> > +} __packed;
>

Regards,
Bin
Park, Aiden July 24, 2019, 3:07 a.m. UTC | #3
Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Monday, July 22, 2019 8:42 AM
> To: Park, Aiden <aiden.park@intel.com>
> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Simon Glass
> <sjg@chromium.org>; Bin Meng <bmeng.cn@gmail.com>
> Subject: Re: [PATCH v5 5/8] x86: slimbootloader: Set TSC information for timer
> driver
> 
> On Wed, Jul 17, 2019 at 7:41 AM Park, Aiden <aiden.park@intel.com> wrote:
> >
> > Slim Bootloader provides TSC clock information in its performance info
> > hob. For now, TSC clock information is only used for timer driver from
> > the performance info hob.
> > - Get TSC frequency from performance info hob
> > - Set tsc_base and clock_rate for timer driver
> 
> So why do we need this at all? We have a common TSC driver that works for all
> x86.
> 
This series also uses TSC driver, but giving these values will bypass TSC calibration in the driver.
Slim Bootloader already has CPU specific TSC value and provides the TSC value
in its performance data HOB. Therefore, U-Boot TSC driver does not have to re-calibrate TSC.

> > +#define LOADER_PERFORMANCE_INFO_GUID \
> > +       { \
> > +       0x868204be, 0x23d0, 0x4ff9, \
> > +       { 0xac, 0x34, 0xb9, 0x95, 0xac, 0x04, 0xb1, 0xb9 } \
> > +       }
> 
> Use EFI_GUID(), please.
> 
Let me use EFI_GUID.

> > +struct performance_info {
> 
> Same, you better to make less generic names for data structs.
>
Let me use particular one. Thanks.

> > +} __packed;
> 
> Same question. If it's aligned naturally by 32-bit boundaries, Why do you need
> __packed?
>
Let me remote __packed. Thanks.
 
> --
> With Best Regards,
> Andy Shevchenko

Best Regards,
Aiden

Patch
diff mbox series

diff --git a/arch/x86/cpu/slimbootloader/slimbootloader.c b/arch/x86/cpu/slimbootloader/slimbootloader.c
index 9f3a61ec61..2153d2ac66 100644
--- a/arch/x86/cpu/slimbootloader/slimbootloader.c
+++ b/arch/x86/cpu/slimbootloader/slimbootloader.c
@@ -4,9 +4,43 @@ 
  */

 #include <common.h>
+#include <asm/arch/slimbootloader.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/**
+ * This sets tsc_base and clock_rate for early_timer and tsc_timer.
+ * The performance info guid hob has all performance timestamp data, but
+ * the only tsc frequency info is used for the timer driver for now.
+ */
+static void tsc_init(void)
+{
+       struct performance_info *data = NULL;
+       const struct efi_guid guid = LOADER_PERFORMANCE_INFO_GUID;
+
+       if (!gd->arch.hob_list)
+               panic("hob list not found!");
+
+       gd->arch.tsc_base = rdtsc();
+       debug("tsc_base=0x%llx\n", gd->arch.tsc_base);
+
+       data = (struct performance_info *)
+               hob_get_guid_hob_data(gd->arch.hob_list, NULL, &guid);
+
+       if (!data) {
+               debug("performance info hob not found\n");
+               return;
+       }
+
+       /* frequency is in KHz, so to Hz */
+       gd->arch.clock_rate = data->frequency * 1000;
+       debug("freq=0x%lx\n", gd->arch.clock_rate);
+}

 int arch_cpu_init(void)
 {
+       tsc_init();
+
        return x86_cpu_init_f();
 }

diff --git a/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h b/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h
index 5a0994544a..e0edbdddf5 100644
--- a/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h
+++ b/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h
@@ -27,6 +27,15 @@ 
        { 0xbb, 0x98, 0x95, 0x8d, 0x62, 0xde, 0x87, 0xf1 } \
        }

+/**
+ * A GUID to get boot performance info hob which is provided by Slim Bootloader
+ */
+#define LOADER_PERFORMANCE_INFO_GUID \
+       { \
+       0x868204be, 0x23d0, 0x4ff9, \
+       { 0xac, 0x34, 0xb9, 0x95, 0xac, 0x04, 0xb1, 0xb9 } \
+       }
+
 /**
  * A single entry of memory map information
  *
@@ -71,6 +80,27 @@  struct serial_port_info {
        u32     rsvd1;
 } __packed;

+/**
+ * This includes timestamp data which has been collected in Slim Bootloader
+ * stages from the reset vector. In addition, this has TSC frequency in KHz to
+ * calculate each timestamp.
+ *
+ * @rev   : revision of performance_info structure. currently 1.
+ * @rsvd  : padding for alignment
+ * @count : the number of collected timestamp data
+ * @flags : only used in Slim Bootloader
+ * @frequency: tsc frequency in KHz
+ * @timestamp: the array of timestamp data which has 64-bit tsc value
+ */
+struct performance_info {
+       u8      rev;
+       u8      rsvd[3];
+       u16     count;
+       u16     flags;
+       u32     frequency;
+       u64     timestamp[0];
+} __packed;
+
 /**
  * This includes all memory map entries which is sorted based on physical start
  * address, from low to high, and carved out reserved, acpi nvs, acpi reclaim