Re: [Qemu-devel] [PATCH] Implement the global timer present in ARM MPCore chips.

[Peter Maydell]

On 6 July 2011 21:04, Christopher Harvey <address@hidden> wrote:
> [a patch for A9 global timer]

Cool, thanks.

> Reminder:
> There are probably bugs in this code, do not commit :P

(You can flag that by saying "[RFC]" in the subject rather than
"[PATCH]".)

The first thing to note is that hw/mpcore.c is shared between
the ARM11MPCore and the Cortex-A9MP, implementing the "private memory
region" peripherals. In fact the two cores don't have identical
sets of peripherals, and in particular the global timer is not
present in the 11MPCore. (The SCUs are different too, although
we have such a minimal implementation of the SCU it scarcely
matters.) You need to sort out some reasonably clean way of
distinguishing the two...


Below are some random comments noticed on a first-pass.

> +/* Global timer data */
> +static uint64_t global_timer_count = 0;
> +static uint64_t global_timer_this_inc = 0;
> +  /* Only for prescale and enable bits */
> +static uint32_t global_timer_control = 0;

Static globals look like the wrong thing to me.

scripts/checkpatch.pl will help you catch random nits like wrong
indentation and qemu's brace-placement shibboleths (though it is
not infallible by a long shot).

Avoid random unconnected whitespace changes like:
-    case 12: /* Interrupt status.  */
+    case 12: /* Interrupt status. */

Do implement the counter low/high read and write :-)

This constant looks a bit fishy, doesn't it need a ULL suffix?
+        s->compare &= 0xffffffff00000000;

+    if(s->control & 0xB)/*test for auto-inc bit, comp and timer enable bits*/

You could define some constants for the bits rather than
using a comment.

I don't know enough about qemu's timer infrastructure to
comment on your use of it.

-- PMM