[Qemu-devel] [PATCH v2 0/2] Add the generic ARM timer

[Qemu-devel] [PATCH v2 0/2] Add the generic ARM timer

[Alistair Francis]

These two patches and and connect the Generic ARM Timer. This includes
support for dropping insecure writes.

V2:
 - Fix couter/counter typo

Alistair Francis (2):
  arm_generic_timer: Add the ARM Generic Timer
  xlnx-zynqmp: Connect the ARM Generic Timer

 hw/arm/xlnx-zynqmp.c                 |  13 +++
 hw/timer/Makefile.objs               |   1 +
 hw/timer/arm_generic_timer.c         | 216 +++++++++++++++++++++++++++++++++++
 include/hw/arm/xlnx-zynqmp.h         |   2 +
 include/hw/timer/arm_generic_timer.h |  60 ++++++++++
 5 files changed, 292 insertions(+)
 create mode 100644 hw/timer/arm_generic_timer.c
 create mode 100644 include/hw/timer/arm_generic_timer.h

-- 
2.7.4

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

[Alistair Francis]

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 <alistair.francis@xilinx.com>
---
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

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
 
 obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
 obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
diff --git a/hw/timer/arm_generic_timer.c b/hw/timer/arm_generic_timer.c
new file mode 100644
index 0000000..7642206
--- /dev/null
+++ b/hw/timer/arm_generic_timer.c
@@ -0,0 +1,216 @@
+/*
+ * QEMU model of the ARM Generic Timer
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Written by Alistair Francis <alistair.francis@xilinx.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/timer/arm_generic_timer.h"
+#include "qemu/timer.h"
+#include "qemu/log.h"
+
+#ifndef ARM_GEN_TIMER_ERR_DEBUG
+#define ARM_GEN_TIMER_ERR_DEBUG 0
+#endif
+
+static void counter_control_postw(RegisterInfo *reg, uint64_t val64)
+{
+    ARMGenTimer *s = ARM_GEN_TIMER(reg->opaque);
+    bool new_status = extract32(s->regs[R_COUNTER_CONTROL_REGISTER],
+                                R_COUNTER_CONTROL_REGISTER_EN_SHIFT,
+                                R_COUNTER_CONTROL_REGISTER_EN_LENGTH);
+    uint64_t current_ticks;
+
+    current_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                             NANOSECONDS_PER_SECOND, 1000000);
+
+    if ((s->enabled && !new_status) ||
+        (!s->enabled && new_status)) {
+        /* The timer is being disabled or enabled */
+        s->tick_offset = current_ticks - s->tick_offset;
+    }
+
+    s->enabled = new_status;
+}
+
+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;
+}
+
+
+static RegisterAccessInfo arm_gen_timer_regs_info[] = {
+    {   .name = "COUNTER_CONTROL_REGISTER",
+        .addr = A_COUNTER_CONTROL_REGISTER,
+        .rsvd = 0xfffffffc,
+        .post_write = counter_control_postw,
+    },{ .name = "COUNTER_STATUS_REGISTER",
+        .addr = A_COUNTER_STATUS_REGISTER,
+        .rsvd = 0xfffffffd, .ro = 0x2,
+    },{ .name = "CURRENT_COUNTER_VALUE_LOWER_REGISTER",
+        .addr = A_CURRENT_COUNTER_VALUE_LOWER_REGISTER,
+        .post_read = counter_low_value_postr,
+    },{ .name = "CURRENT_COUNTER_VALUE_UPPER_REGISTER",
+        .addr = A_CURRENT_COUNTER_VALUE_UPPER_REGISTER,
+        .post_read = counter_high_value_postr,
+    },{ .name = "BASE_FREQUENCY_ID_REGISTER",
+        .addr = A_BASE_FREQUENCY_ID_REGISTER,
+    }
+};
+
+static void arm_gen_timer_reset(DeviceState *dev)
+{
+    ARMGenTimer *s = ARM_GEN_TIMER(dev);
+    unsigned int i;
+
+    for (i = 0; i < ARRAY_SIZE(s->regs_info); ++i) {
+        register_reset(&s->regs_info[i]);
+    }
+
+    s->tick_offset = 0;
+    s->enabled = false;
+}
+
+static MemTxResult arm_gen_timer_read(void *opaque,  hwaddr addr,
+                                      uint64_t *data, unsigned size,
+                                      MemTxAttrs attrs)
+{
+    /* Reads are always supported, just blindly pass them through */
+    *data = register_read_memory(opaque, addr, size);
+
+    return MEMTX_OK;
+}
+
+static MemTxResult arm_gen_timer_write(void *opaque, hwaddr addr,
+                                       uint64_t data, unsigned size,
+                                       MemTxAttrs attrs)
+{
+    /* Block insecure writes */
+    if (!attrs.secure) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Non secure writes to the system timestamp generator " \
+                      "are invalid\n");
+        return MEMTX_ERROR;
+    }
+
+    register_write_memory(opaque, addr, data, size);
+
+    return MEMTX_OK;
+}
+
+static const MemoryRegionOps arm_gen_timer_ops = {
+    .read_with_attrs = arm_gen_timer_read,
+    .write_with_attrs = arm_gen_timer_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+};
+
+static const VMStateDescription vmstate_arm_gen_timer = {
+    .name = TYPE_ARM_GEN_TIMER,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, ARMGenTimer, R_ARM_GEN_TIMER_MAX),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
+static void arm_gen_timer_init(Object *obj)
+{
+    ARMGenTimer *s = ARM_GEN_TIMER(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    RegisterInfoArray *reg_array;
+
+    memory_region_init_io(&s->iomem, obj, &arm_gen_timer_ops, s,
+                          TYPE_ARM_GEN_TIMER, R_ARM_GEN_TIMER_MAX * 4);
+    reg_array =
+        register_init_block32(DEVICE(obj), arm_gen_timer_regs_info,
+                              ARRAY_SIZE(arm_gen_timer_regs_info),
+                              s->regs_info, s->regs,
+                              &arm_gen_timer_ops,
+                              ARM_GEN_TIMER_ERR_DEBUG,
+                              R_ARM_GEN_TIMER_MAX * 4);
+    memory_region_add_subregion(&s->iomem,
+                                A_COUNTER_CONTROL_REGISTER,
+                                &reg_array->mem);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void arm_gen_timer_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = arm_gen_timer_reset;
+    dc->vmsd = &vmstate_arm_gen_timer;
+}
+
+static const TypeInfo arm_gen_timer_info = {
+    .name          = TYPE_ARM_GEN_TIMER,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ARMGenTimer),
+    .class_init    = arm_gen_timer_class_init,
+    .instance_init = arm_gen_timer_init,
+};
+
+static void arm_gen_timer_register_types(void)
+{
+    type_register_static(&arm_gen_timer_info);
+}
+
+type_init(arm_gen_timer_register_types)
diff --git a/include/hw/timer/arm_generic_timer.h b/include/hw/timer/arm_generic_timer.h
new file mode 100644
index 0000000..4cc0ac9
--- /dev/null
+++ b/include/hw/timer/arm_generic_timer.h
@@ -0,0 +1,60 @@
+/*
+ * QEMU model of the ARM Generic Timer
+ *
+ * Copyright (c) 2016 Xilinx Inc.
+ * Written by Alistair Francis <alistair.francis@xilinx.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef ARM_GEN_TIMER_H
+#define ARM_GEN_TIMER_H
+
+#include "hw/sysbus.h"
+#include "hw/register.h"
+
+#define TYPE_ARM_GEN_TIMER "arm.generic-timer"
+#define ARM_GEN_TIMER(obj) \
+            OBJECT_CHECK(ARMGenTimer, (obj), TYPE_ARM_GEN_TIMER)
+
+REG32(COUNTER_CONTROL_REGISTER, 0x00)
+    FIELD(COUNTER_CONTROL_REGISTER, EN, 1, 1)
+    FIELD(COUNTER_CONTROL_REGISTER, HDBG, 0, 1)
+REG32(COUNTER_STATUS_REGISTER, 0x04)
+    FIELD(COUNTER_STATUS_REGISTER, DBGH, 1, 1)
+REG32(CURRENT_COUNTER_VALUE_LOWER_REGISTER, 0x08)
+REG32(CURRENT_COUNTER_VALUE_UPPER_REGISTER, 0x0C)
+REG32(BASE_FREQUENCY_ID_REGISTER, 0x20)
+
+#define R_ARM_GEN_TIMER_MAX (R_BASE_FREQUENCY_ID_REGISTER + 1)
+
+typedef struct ARMGenTimer {
+    /* <private> */
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+
+    /* <public> */
+    bool enabled;
+    uint64_t tick_offset;
+
+    uint32_t regs[R_ARM_GEN_TIMER_MAX];
+    RegisterInfo regs_info[R_ARM_GEN_TIMER_MAX];
+} ARMGenTimer;
+
+#endif
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/2] xlnx-zynqmp: Connect the ARM Generic Timer

[Alistair Francis]

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 hw/arm/xlnx-zynqmp.c         | 13 +++++++++++++
 include/hw/arm/xlnx-zynqmp.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 0d86ba3..43c68c5 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -38,6 +38,8 @@
 #define SATA_ADDR           0xFD0C0000
 #define SATA_NUM_PORTS      2
 
+#define ARM_GEN_TIMER_ADDR  0xFF260000
+
 #define DP_ADDR             0xfd4a0000
 #define DP_IRQ              113
 
@@ -172,6 +174,10 @@ static void xlnx_zynqmp_init(Object *obj)
         qdev_set_parent_bus(DEVICE(&s->spi[i]), sysbus_get_default());
     }
 
+    object_initialize(&s->arm_gen_timer, sizeof(s->arm_gen_timer),
+                      TYPE_ARM_GEN_TIMER);
+    qdev_set_parent_bus(DEVICE(&s->arm_gen_timer), sysbus_get_default());
+
     object_initialize(&s->dp, sizeof(s->dp), TYPE_XLNX_DP);
     qdev_set_parent_bus(DEVICE(&s->dp), sysbus_get_default());
 
@@ -405,6 +411,13 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
         g_free(bus_name);
     }
 
+    object_property_set_bool(OBJECT(&s->arm_gen_timer), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(&s->arm_gen_timer), 0, ARM_GEN_TIMER_ADDR);
+
     object_property_set_bool(OBJECT(&s->dp), true, "realized", &err);
     if (err) {
         error_propagate(errp, err);
diff --git a/include/hw/arm/xlnx-zynqmp.h b/include/hw/arm/xlnx-zynqmp.h
index c2931bf..8deabb4 100644
--- a/include/hw/arm/xlnx-zynqmp.h
+++ b/include/hw/arm/xlnx-zynqmp.h
@@ -26,6 +26,7 @@
 #include "hw/ide/ahci.h"
 #include "hw/sd/sdhci.h"
 #include "hw/ssi/xilinx_spips.h"
+#include "hw/timer/arm_generic_timer.h"
 #include "hw/dma/xlnx_dpdma.h"
 #include "hw/display/xlnx_dp.h"
 
@@ -83,6 +84,7 @@ typedef struct XlnxZynqMPState {
     SysbusAHCIState sata;
     SDHCIState sdhci[XLNX_ZYNQMP_NUM_SDHCI];
     XilinxSPIPS spi[XLNX_ZYNQMP_NUM_SPIS];
+    ARMGenTimer arm_gen_timer;
     XlnxDPState dp;
     XlnxDPDMAState dpdma;
 
-- 
2.7.4

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

[Peter Maydell]

On 8 November 2016 at 00:58, Alistair Francis
<alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
> ---
> 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

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

[Alistair Francis]

On Tue, Dec 13, 2016 at 5:11 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 November 2016 at 00:58, Alistair Francis
> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>> ---
>> 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

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

[Alistair Francis]

On Fri, Dec 16, 2016 at 1:03 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Tue, Dec 13, 2016 at 5:11 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 8 November 2016 at 00:58, Alistair Francis
>> <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com>
>>> ---
>>> 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