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

[Peter Maydell]

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.)

Is this code trying to implement that Generic Timer, or something
else with a similar name?

> 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.

>
>  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.

thanks
-- PMM