qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Tim


From: Alistair Francis
Subject: Re: [Qemu-arm] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer
Date: Tue, 20 Dec 2016 14:43:35 -0800

On Fri, Dec 16, 2016 at 1:03 PM, Alistair Francis
<address@hidden> wrote:
> On Tue, Dec 13, 2016 at 5:11 AM, Peter Maydell <address@hidden> wrote:
>> On 8 November 2016 at 00:58, Alistair Francis
>> <address@hidden> wrote:
>>> Add the ARM generic timer. This allows the guest to poll the timer for
>>> values and also supports secure writes only.
>>>
>>> Signed-off-by: Alistair Francis <address@hidden>
>>> ---
>>> V2:
>>>  - Fix couter/counter typo
>>>
>>>  hw/timer/Makefile.objs               |   1 +
>>>  hw/timer/arm_generic_timer.c         | 216 
>>> +++++++++++++++++++++++++++++++++++
>>>  include/hw/timer/arm_generic_timer.h |  60 ++++++++++
>>>  3 files changed, 277 insertions(+)
>>>  create mode 100644 hw/timer/arm_generic_timer.c
>>>  create mode 100644 include/hw/timer/arm_generic_timer.h
>>
>> I can't quite seem to make this line up with the spec in the v8
>> ARM ARM (chapter 11 "System Level Implementation of the Generic
>> Timer"). The generic timer documented there looks quite like this,
>> but has extra ID registers at 0xFD0..0xFFC which this code doesn't
>> implement, and also has the "CNTReadBase" memory map (which
>> exposes just the count value and ID registers) as well as the
>> "CNTControlBase" map that looks like what you have here.
>> (The register names here differ from the conventions in the
>> ARM ARM too.)
>
> Of course they don't line up :(
>
> I wrote it based off the Xilinx documentation, I (wrongly) assumed
> that we would follow the normal convention but apparently not.
>
>>
>> Is this code trying to implement that Generic Timer, or something
>> else with a similar name?
>
> I'm not sure now which one it is.
>
> I will dig into this and hopefully can respin this based on what is in
> the ARM ARM.

It turns out it is pretty close.

I have fixed up the names to be more like the ARM ARM. I have added
some comments about the implementation specific registers and I have
added the read base registers as well.

Sending out a V3.

Thanks,

Alistair

>
>>
>>> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
>>> index 7ba8c23..f88c468 100644
>>> --- a/hw/timer/Makefile.objs
>>> +++ b/hw/timer/Makefile.objs
>>> @@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
>>>  common-obj-$(CONFIG_IMX) += imx_gpt.o
>>>  common-obj-$(CONFIG_LM32) += lm32_timer.o
>>>  common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
>>> +common-obj-$(CONFIG_XLNX_ZYNQMP) += arm_generic_timer.o
>>
>> If this is really generic I think it ought to have its own
>> CONFIG_foo rather than using the XLNX_ZYNQMP symbol.
>
> Yes, Fred pointed that out as well. I will fix that up in the next version.
>
>>
>>>
>>>  obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
>>>  obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
>>
>>> +static uint64_t counter_low_value_postr(RegisterInfo *reg, uint64_t val64)
>>> +{
>>> +    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
>>> +    uint64_t current_ticks, total_ticks;
>>> +    uint32_t low_ticks;
>>> +
>>> +    if (s->enabled) {
>>> +        current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>>> +                                 NANOSECONDS_PER_SECOND, 1000000);
>>> +        total_ticks = current_ticks - s->tick_offset;
>>> +        low_ticks = (uint32_t) total_ticks;
>>> +    } else {
>>> +        /* Timer is disabled, return the time when it was disabled */
>>> +        low_ticks = (uint32_t) s->tick_offset;
>>> +    }
>>> +
>>> +    return low_ticks;
>>> +}
>>> +
>>> +static uint64_t counter_high_value_postr(RegisterInfo *reg, uint64_t val64)
>>> +{
>>> +    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
>>> +    uint64_t current_ticks, total_ticks;
>>> +    uint32_t high_ticks;
>>> +
>>> +    if (s->enabled) {
>>> +        current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>>> +                                 NANOSECONDS_PER_SECOND, 1000000);
>>> +        total_ticks = current_ticks - s->tick_offset;
>>> +        high_ticks = (uint32_t) (total_ticks >> 32);
>>> +    } else {
>>> +        /* Timer is disabled, return the time when it was disabled */
>>> +        high_ticks = (uint32_t) (s->tick_offset >> 32);
>>> +    }
>>> +
>>> +    return high_ticks;
>>
>> These two functions are very similar and would benefit from
>> being refactored to call a utility function that just returned
>> the 64-bit count.
>
> Good point.
>
> Thanks,
>
> Alistair
>
>>
>> thanks
>> -- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]