* [Qemu-devel] [PATCH v2 0/2] Add the generic ARM timer @ 2016-11-08 0:58 Alistair Francis 2016-11-08 0:58 ` [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer Alistair Francis 2016-11-08 0:58 ` [Qemu-devel] [PATCH v2 2/2] xlnx-zynqmp: Connect " Alistair Francis 0 siblings, 2 replies; 6+ messages in thread From: Alistair Francis @ 2016-11-08 0:58 UTC (permalink / raw) To: qemu-devel, qemu-arm, peter.maydell, fred.konrad Cc: alistair.francis, alistair23 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer 2016-11-08 0:58 [Qemu-devel] [PATCH v2 0/2] Add the generic ARM timer Alistair Francis @ 2016-11-08 0:58 ` Alistair Francis 2016-12-13 13:11 ` Peter Maydell 2016-11-08 0:58 ` [Qemu-devel] [PATCH v2 2/2] xlnx-zynqmp: Connect " Alistair Francis 1 sibling, 1 reply; 6+ messages in thread From: Alistair Francis @ 2016-11-08 0:58 UTC (permalink / raw) To: qemu-devel, qemu-arm, peter.maydell, fred.konrad Cc: alistair.francis, alistair23 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, + ®_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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer 2016-11-08 0:58 ` [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer Alistair Francis @ 2016-12-13 13:11 ` Peter Maydell 2016-12-16 21:03 ` Alistair Francis 0 siblings, 1 reply; 6+ messages in thread From: Peter Maydell @ 2016-12-13 13:11 UTC (permalink / raw) To: Alistair Francis Cc: QEMU Developers, qemu-arm, KONRAD Frédéric, Alistair Francis 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer 2016-12-13 13:11 ` Peter Maydell @ 2016-12-16 21:03 ` Alistair Francis 2016-12-20 22:43 ` Alistair Francis 0 siblings, 1 reply; 6+ messages in thread From: Alistair Francis @ 2016-12-16 21:03 UTC (permalink / raw) To: Peter Maydell Cc: Alistair Francis, QEMU Developers, qemu-arm, KONRAD Frédéric 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer 2016-12-16 21:03 ` Alistair Francis @ 2016-12-20 22:43 ` Alistair Francis 0 siblings, 0 replies; 6+ messages in thread From: Alistair Francis @ 2016-12-20 22:43 UTC (permalink / raw) To: Alistair Francis Cc: Peter Maydell, QEMU Developers, qemu-arm, KONRAD Frédéric 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] xlnx-zynqmp: Connect the ARM Generic Timer 2016-11-08 0:58 [Qemu-devel] [PATCH v2 0/2] Add the generic ARM timer Alistair Francis 2016-11-08 0:58 ` [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer Alistair Francis @ 2016-11-08 0:58 ` Alistair Francis 1 sibling, 0 replies; 6+ messages in thread From: Alistair Francis @ 2016-11-08 0:58 UTC (permalink / raw) To: qemu-devel, qemu-arm, peter.maydell, fred.konrad Cc: alistair.francis, alistair23 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-20 22:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-08 0:58 [Qemu-devel] [PATCH v2 0/2] Add the generic ARM timer Alistair Francis 2016-11-08 0:58 ` [Qemu-devel] [PATCH v2 1/2] arm_generic_timer: Add the ARM Generic Timer Alistair Francis 2016-12-13 13:11 ` Peter Maydell 2016-12-16 21:03 ` Alistair Francis 2016-12-20 22:43 ` Alistair Francis 2016-11-08 0:58 ` [Qemu-devel] [PATCH v2 2/2] xlnx-zynqmp: Connect " Alistair Francis
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.