On 6 July 2011 21:04, Christopher Harvey <char...@matrox.com> 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
