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

[Simon Glass]

Hi Bin,

On 25 April 2018 at 21:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.

Yes I agree. I'm open to ideas and patches :-)

Regards,
Simon