[U-Boot] [PATCH v5 2/2] timer: Add High Precision Event Timers (HPET) support

[Bin Meng]

Hi Simon,

On Tue, Apr 17, 2018 at 11:10 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 15 April 2018 at 23:06, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Simon,
>>
>> On Fri, Apr 13, 2018 at 12:42 AM, Simon Glass <sjg at chromium.org> wrote:
>>> Hi,
>>>
>>> On 8 April 2018 at 19:22, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> Hi Ivan,
>>>>
>>>> On Sat, Apr 7, 2018 at 3:18 AM, Ivan Gorinov <ivan.gorinov at intel.com> wrote:
>>>>> Add HPET driver as an alternative timer for x86 (default is TSC).
>>>>> HPET counter has constant frequency and does not need calibration.
>>>>> This change also makes TSC timer driver optional on x86.
>>>>> New HPET driver can also be selected as the early timer on x86.
>>>>>
>>>>> HPET can be selected as the tick timer in the Device Tree "chosen" node:
>>>>>
>>>>>     /include/ "hpet.dtsi"
>>>>>
>>>>> ...
>>>>>
>>>>>     chosen {
>>>>>         tick-timer = "/hpet";
>>>>>     };
>>>>>
>>>>> Signed-off-by: Ivan Gorinov <ivan.gorinov at intel.com>
>>>>> ---
>>>>>  arch/Kconfig               |   2 +-
>>>>>  arch/x86/Kconfig           |  21 ++++++
>>>>>  arch/x86/dts/hpet.dtsi     |   7 ++
>>>>>  drivers/timer/Kconfig      |   9 +++
>>>>>  drivers/timer/Makefile     |   1 +
>>>>>  drivers/timer/hpet_timer.c | 179 +++++++++++++++++++++++++++++++++++++++++++++
>>>>>  drivers/timer/tsc_timer.c  |   8 ++
>>>>>  7 files changed, 226 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 arch/x86/dts/hpet.dtsi
>>>>>  create mode 100644 drivers/timer/hpet_timer.c
>>>>>
>>>
>>> [..]
>>>>> diff --git a/drivers/timer/tsc_timer.c b/drivers/timer/tsc_timer.c
>>>>> index 9296de6..bd0e75c 100644
>>>>> --- a/drivers/timer/tsc_timer.c
>>>>> +++ b/drivers/timer/tsc_timer.c
>>>>> @@ -277,6 +277,8 @@ success:
>>>>>         return delta / 1000;
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_X86_EARLY_TIMER_TSC
>>>>
>>>> Why do we surround the following APIs with CONFIG_X86_EARLY_TIMER_TSC?
>>>> These APIs are generic U-Boot timer APIs. If we select
>>>> CONFIG_X86_EARLY_TIMER_HPET, these APIs are not available and will
>>>> cause build error.
>>>>
>>>> Simon, do you think we should fix such in the timer uclass driver?
>>>
>>> We should not have arch-specific code in the uclass, or in any generic driver.
>>
>> What I meant is these APIs like get_timer(). Can such APIs be moved to
>> timer uclass driver? To avoid conflicts with other timer drivers, in
>> this patch it was solved by using the config option
>> CONFIG_X86_EARLY_TIMER_TSC to hide this in the TSC driver, which does
>
> Well get_timer() just calls get_ticks() which uses DM, so what exactly
> are you proposing here?
>

The tsc_timer.c has the get_timer() implementation while lib/time.c
provides a default one that calls into DM. When I saw this new HPET
driver I got confused and thought we should move such from timer
driver to the uclass driver. My original comments regarding to
CONFIG_X86_EARLY_TIMER_TSC is to make sure we support mixed use of
timers, eg: HPET as the early timer and TSC as the normal timer. I
will have a look at the latest patch.

Regards,
Bin