Hi Bin,
On Fri, 11 Oct 2019 at 23:18, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Sat, Oct 12, 2019 at 11:38 AM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Fri, 11 Oct 2019 at 07:19, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Fri, Oct 11, 2019 at 1:06 AM Simon Glass sjg@chromium.org wrote:
Hi Bin,
On Sat, 5 Oct 2019 at 08:36, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Wed, Sep 25, 2019 at 10:58 PM Simon Glass sjg@chromium.org wrote:
Most of the timer-calibration methods are not needed on recent Intel CPUs and just increase code size. Add an option to use the known-good way to get the clock frequency in TPL. Size reduction is about 700 bytes.
Signed-off-by: Simon Glass sjg@chromium.org
drivers/timer/Kconfig | 29 +++++++++++++++++++---------- drivers/timer/tsc_timer.c | 7 +++++-- 2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index 5f4bc6edb67..90bc8ec7c53 100644 --- a/drivers/timer/Kconfig +++ b/drivers/timer/Kconfig @@ -117,16 +117,6 @@ config RENESAS_OSTM_TIMER Enables support for the Renesas OSTM Timer driver. This timer is present on Renesas RZ/A1 R7S72100 SoCs.
-config X86_TSC_TIMER_EARLY_FREQ
int "x86 TSC timer frequency in MHz when used as the early timer"depends on X86_TSC_TIMERdefault 1000helpSets the estimated CPU frequency in MHz when TSC is used as theearly timer and the frequency can neither be calibrated via somehardware ways, nor got from device tree at the time when devicetree is not available yet.config OMAP_TIMER bool "Omap timer support" depends on TIMER @@ -174,6 +164,25 @@ config X86_TSC_TIMER help Select this to enable Time-Stamp Counter (TSC) timer for x86.
+config X86_TSC_TIMER_EARLY_FREQ
int "x86 TSC timer frequency in MHz when used as the early timer"depends on X86_TSC_TIMERdefault 1000helpSets the estimated CPU frequency in MHz when TSC is used as theearly timer and the frequency can neither be calibrated via somehardware ways, nor got from device tree at the time when devicetree is not available yet.+config TPL_X86_TSC_TIMER_NATIVE
bool "x86 TSC timer uses native calibration"depends on TPL && X86_TSC_TIMERhelpSelects native timer calibration for TPL and don't include the othermethods in the code. This helps to reduce code size in TPL and workson fairly modern Intel chips. Code-size reductions is about 700bytes.config MTK_TIMER bool "MediaTek timer support" depends on TIMER diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c index 919caba8a14..9630036bc7f 100644 --- a/drivers/timer/tsc_timer.c +++ b/drivers/timer/tsc_timer.c @@ -49,8 +49,7 @@ static unsigned long native_calibrate_tsc(void) return 0;
crystal_freq = tsc_info.ecx / 1000;
if (!crystal_freq) {
if (!CONFIG_IS_ENABLED(X86_TSC_TIMER_NATIVE) && !crystal_freq) { switch (gd->arch.x86_model) { case INTEL_FAM6_SKYLAKE_MOBILE: case INTEL_FAM6_SKYLAKE_DESKTOP:@@ -405,6 +404,10 @@ static void tsc_timer_ensure_setup(bool early) if (fast_calibrate) goto done;
/* Reduce code size by dropping other methods */if (CONFIG_IS_ENABLED(X86_TSC_TIMER_NATIVE))panic("no timer");I don't get it. How could this reduce the code size? I don't see any #ifdefs around the other methods we want to drop?
The compiler sees that CONFIG_IS_ENABLED(..) is 1, and leaves out the code that follows it.
Why?
if (1) panic("no timer");
then compiler does not generate any codes of the following?
fast_calibrate = cpu_mhz_from_cpuid();
I don't understand.
The panic() function is marked as noreturn, so the compiler assume it doesn't return. You can try this if you like. It reduces the size by 700 bytes which on a 22KB image is a lot.
OK, compiler is smart to generate less codes :)
But the way you added the CONFIG_IS_ENABLED(..) logic check here is obscure if one does not dig into that deep ..
Besides, I think adding some random Kconfig options to exclude some specific parts in one C file is a bad idea. It's unclear to me why we should exclude one part versus another part. I'm OK to exclude the whole C file for TPL/SPL though, but not part of it for size limitation purpose.
My understanding is the most of the code in this function is a fallback in case an earlier method doesn't work. But on modern CPUs
Yes, correct.
the first method always works, so this is a waste of time?
It's not a wast of time, but a bloat of the code size. As you said, these are fallbacks, and methods are prioritized based on the age of the processors, so that native method is tried first, followed by cpuid, MSR, and finally PIT.
You also mentioned that "on modern CPUs the first method always works", so today the first method is native_calibrate_tsc(), but say 3 years later, this might not be true, and chances are that we may add another method before native_calibrate_tsc() for whatever mechanism is used on the latest processors, and the insertion of the TPL Kconfig option (TPL_X86_TSC_TIMER_NATIVE) check today is not future proof.
That's right, it is not. Perhaps we need to have separate timer drivers for different generations? But if not, I am loath to have 700 bytes of dead code in TPL, which I why I added the option.
If we later need to adjust it, we can do so, but this cuts off the worst of the bloat.
Regards, Simon