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