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

[Bin Meng]

Hi Ivan,

On Tue, Apr 24, 2018 at 4:41 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Ivan,
>
> On Tue, Apr 24, 2018 at 7:56 AM, Ivan Gorinov <ivan.gorinov at 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.
>

Further checking the coreboot timestamp codes, I think we may have to
leave the coreboot timestamp codes as it is now.

We have the codes blow:
void timestamp_add_now(enum timestamp_id id)
{
    timestamp_add(id, rdtsc());
}

We cannot replace rdtsc() with timer_early_get_count(), because this
timestamp_add_now() is called both before and after DM initialization.
If the HPET is selected as the early timer and TSC is selected as the
normal timer, the timestamp numbers are meaningless to compare against
each other.

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

Simon, another issue is the bootstage support. So far the
timer_get_boot_us() is not implemented by DM timer APIs.
timer_get_boot_us() is implemented per timer driver if
CONFIG_SYS_TIMER_COUNTER is not defined. Note CONFIG_SYS_TIMER_COUNTER
is non-DM stuff. That means the bootstage support is bounded by a
specific timer driver, instead of a generic library. To me this
overall timer support is somehow fragmentary.

Regards,
Bin