[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic T
From: |
Alistair Francis |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer |
Date: |
Fri, 16 Dec 2016 13:03:35 -0800 |
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.
>
>> 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