[U-Boot,v6,5/8] x86: slimbootloader: Set TSC information for tsc_timer
diff mbox series

Message ID A1484485FD99714DB2AB2C5EF81E7AC2AA75839D@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 26, 2019, 7 a.m. UTC
Slim Bootloader already calibrated TSC and provides it to U-Boot.
Therefore, U-Boot does not have to re-calibrate TSC.
Configuring tsc_base and clock_rate makes x86 tsc_timer driver
bypass TSC calibration and use the provided TSC frequency.
- Get TSC frequency from performance info hob
- Set tsc_base and clock_rate for tsc_timer driver

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

Changes in v6:
  * Use EFI_GUID
  * Add more comments while setting tsc_base and clock_rate
  * Apply code-review comments

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

 arch/x86/cpu/slimbootloader/slimbootloader.c  | 37 +++++++++++++++++++
 .../asm/arch-slimbootloader/slimbootloader.h  | 28 ++++++++++++++
 2 files changed, 65 insertions(+)

Comments

Andy Shevchenko July 26, 2019, 10:36 a.m. UTC | #1
On Fri, Jul 26, 2019 at 10:00 AM Park, Aiden <aiden.park@intel.com> wrote:
>
> Slim Bootloader already calibrated TSC and provides it to U-Boot.
> Therefore, U-Boot does not have to re-calibrate TSC.
> Configuring tsc_base and clock_rate makes x86 tsc_timer driver
> bypass TSC calibration and use the provided TSC frequency.
> - Get TSC frequency from performance info hob
> - Set tsc_base and clock_rate for tsc_timer driver

I'm still not convinced to have this. As kernel followed by U-Boot we
thrust hardware more than something else.
One more layer in between is usually an additional point to be error-prone.

So, if something we may get directly from hardware, I consider better
to get it from there.

Of course, data structures can be left for sake of self-documentation.
Park, Aiden July 26, 2019, 12:50 p.m. UTC | #2
Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Friday, July 26, 2019 7:37 PM
> To: Park, Aiden <aiden.park@intel.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>; U-Boot Mailing List <u-
> boot@lists.denx.de>; Simon Glass <sjg@chromium.org>
> Subject: Re: [PATCH v6 5/8] x86: slimbootloader: Set TSC information for
> tsc_timer
> 
> On Fri, Jul 26, 2019 at 10:00 AM Park, Aiden <aiden.park@intel.com> wrote:
> >
> > Slim Bootloader already calibrated TSC and provides it to U-Boot.
> > Therefore, U-Boot does not have to re-calibrate TSC.
> > Configuring tsc_base and clock_rate makes x86 tsc_timer driver bypass
> > TSC calibration and use the provided TSC frequency.
> > - Get TSC frequency from performance info hob
> > - Set tsc_base and clock_rate for tsc_timer driver
> 
> I'm still not convinced to have this. As kernel followed by U-Boot we thrust
> hardware more than something else.
> One more layer in between is usually an additional point to be error-prone.
> 
> So, if something we may get directly from hardware, I consider better to get it
> from there.
> 
> Of course, data structures can be left for sake of self-documentation.
> 
Basically, Providing known information to a payload is Slim Bootloader
architecture concept. Slim Bootloader does initialize chipset and hardware
as much as it can, and provides useful information to a payload in HOBs
to minimize hardware re-initialization in a payload, make it generic
and make it light-weight.

TSC frequency is specific to CPU or chipset, and the way to get exact TSC
frequency also varies depending on CPU.
ex) checking cpuid, platform info MSR, perf MSR for LFM/HFM/Turbo Mode, or FSB.
All these consideration has already done in Slim Bootloader on each CPUs. 

I think that U-Boot tsc_timer also considers this case, and that's why
tsc_timer skips calibration in its probing time if tsc_base and clock_rate are
already configured.

Kernel uses many timers like TSC, APIC timer, 8254 timer, HPET and so on.
I also understand kernel trusts hardware and re-calibrate them because all
these timers may not be properly configured in boot firmware stage.
But, HOB information including TSC must be trusted between Slim Bootloader
and its payload(U-Boot) in Slim Bootloader architecture.

> --
> With Best Regards,
> Andy Shevchenko

Best Regards,
Aiden
Andy Shevchenko July 26, 2019, 9:06 p.m. UTC | #3
On Fri, Jul 26, 2019 at 3:50 PM Park, Aiden <aiden.park@intel.com> wrote:
> > On Fri, Jul 26, 2019 at 10:00 AM Park, Aiden <aiden.park@intel.com> wrote:
> > >
> > > Slim Bootloader already calibrated TSC and provides it to U-Boot.
> > > Therefore, U-Boot does not have to re-calibrate TSC.
> > > Configuring tsc_base and clock_rate makes x86 tsc_timer driver bypass
> > > TSC calibration and use the provided TSC frequency.
> > > - Get TSC frequency from performance info hob
> > > - Set tsc_base and clock_rate for tsc_timer driver
> >
> > I'm still not convinced to have this. As kernel followed by U-Boot we thrust
> > hardware more than something else.
> > One more layer in between is usually an additional point to be error-prone.
> >
> > So, if something we may get directly from hardware, I consider better to get it
> > from there.
> >
> > Of course, data structures can be left for sake of self-documentation.
> >
> Basically, Providing known information to a payload is Slim Bootloader
> architecture concept. Slim Bootloader does initialize chipset and hardware
> as much as it can, and provides useful information to a payload in HOBs
> to minimize hardware re-initialization in a payload, make it generic
> and make it light-weight.
>
> TSC frequency is specific to CPU or chipset, and the way to get exact TSC
> frequency also varies depending on CPU.
> ex) checking cpuid, platform info MSR, perf MSR for LFM/HFM/Turbo Mode, or FSB.
> All these consideration has already done in Slim Bootloader on each CPUs.
>
> I think that U-Boot tsc_timer also considers this case, and that's why
> tsc_timer skips calibration in its probing time if tsc_base and clock_rate are
> already configured.
>
> Kernel uses many timers like TSC, APIC timer, 8254 timer, HPET and so on.
> I also understand kernel trusts hardware and re-calibrate them because all
> these timers may not be properly configured in boot firmware stage.
> But, HOB information including TSC must be trusted between Slim Bootloader
> and its payload(U-Boot) in Slim Bootloader architecture.

Okay, I didn't remember if this is described in documentation.
If it's not, please, add a section and put there somelike you
expleined to me here.
Park, Aiden July 28, 2019, 12:48 p.m. UTC | #4
Hi Andy,

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Saturday, July 27, 2019 6:06 AM
> To: Park, Aiden <aiden.park@intel.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>; U-Boot Mailing List <u-
> boot@lists.denx.de>; Simon Glass <sjg@chromium.org>
> Subject: Re: [PATCH v6 5/8] x86: slimbootloader: Set TSC information for
> tsc_timer
> 
> On Fri, Jul 26, 2019 at 3:50 PM Park, Aiden <aiden.park@intel.com> wrote:
> > > On Fri, Jul 26, 2019 at 10:00 AM Park, Aiden <aiden.park@intel.com> wrote:
> > > >
> > > > Slim Bootloader already calibrated TSC and provides it to U-Boot.
> > > > Therefore, U-Boot does not have to re-calibrate TSC.
> > > > Configuring tsc_base and clock_rate makes x86 tsc_timer driver
> > > > bypass TSC calibration and use the provided TSC frequency.
> > > > - Get TSC frequency from performance info hob
> > > > - Set tsc_base and clock_rate for tsc_timer driver
> > >
> > > I'm still not convinced to have this. As kernel followed by U-Boot
> > > we thrust hardware more than something else.
> > > One more layer in between is usually an additional point to be error-prone.
> > >
> > > So, if something we may get directly from hardware, I consider
> > > better to get it from there.
> > >
> > > Of course, data structures can be left for sake of self-documentation.
> > >
> > Basically, Providing known information to a payload is Slim Bootloader
> > architecture concept. Slim Bootloader does initialize chipset and
> > hardware as much as it can, and provides useful information to a
> > payload in HOBs to minimize hardware re-initialization in a payload,
> > make it generic and make it light-weight.
> >
> > TSC frequency is specific to CPU or chipset, and the way to get exact
> > TSC frequency also varies depending on CPU.
> > ex) checking cpuid, platform info MSR, perf MSR for LFM/HFM/Turbo Mode,
> or FSB.
> > All these consideration has already done in Slim Bootloader on each CPUs.
> >
> > I think that U-Boot tsc_timer also considers this case, and that's why
> > tsc_timer skips calibration in its probing time if tsc_base and
> > clock_rate are already configured.
> >
> > Kernel uses many timers like TSC, APIC timer, 8254 timer, HPET and so on.
> > I also understand kernel trusts hardware and re-calibrate them because
> > all these timers may not be properly configured in boot firmware stage.
> > But, HOB information including TSC must be trusted between Slim
> > Bootloader and its payload(U-Boot) in Slim Bootloader architecture.
> 
> Okay, I didn't remember if this is described in documentation.
> If it's not, please, add a section and put there somelike you expleined to me here.
> 
Thanks for your understanding. Let me add more description in documentation.

> --
> 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..e6b174ca88 100644
--- a/arch/x86/cpu/slimbootloader/slimbootloader.c
+++ b/arch/x86/cpu/slimbootloader/slimbootloader.c
@@ -4,9 +4,46 @@ 
  */
 
 #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.
+ *
+ * Slim Bootloader already calibrated TSC and provides it to U-Boot.
+ * Therefore, U-Boot does not have to re-calibrate TSC.
+ * Configuring tsc_base and clock_rate here makes x86 tsc_timer driver
+ * bypass TSC calibration and use the provided TSC frequency.
+ */
+static void tsc_init(void)
+{
+	struct sbl_performance_info *data;
+	const efi_guid_t guid = SBL_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 = 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 174a341d32..05dd1b2b44 100644
--- a/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h
+++ b/arch/x86/include/asm/arch-slimbootloader/slimbootloader.h
@@ -23,6 +23,13 @@ 
 	EFI_GUID(0x6c6872fe, 0x56a9, 0x4403, \
 		0xbb, 0x98, 0x95, 0x8d, 0x62, 0xde, 0x87, 0xf1)
 
+/**
+ * A GUID to get boot performance info hob which is provided by Slim Bootloader
+ */
+#define SBL_PERFORMANCE_INFO_GUID \
+	EFI_GUID(0x868204be, 0x23d0, 0x4ff9, \
+		0xac, 0x34, 0xb9, 0x95, 0xac, 0x04, 0xb1, 0xb9)
+
 /**
  * A single entry of memory map information
  *
@@ -84,4 +91,25 @@  struct sbl_serial_port_info {
 	u32     rsvd1;
 };
 
+/**
+ * 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 sbl_performance_info {
+	u8      rev;
+	u8      rsvd[3];
+	u16     count;
+	u16     flags;
+	u32     frequency;
+	u64     timestamp[0];
+};
+
 #endif /* __SLIMBOOTLOADER_ARCH_H__ */