On Tue, May 02, 2017 at 07:57:22PM -0700, Aaron Larson wrote: > > Previous QEMU open-pic implemented the 4 open-pic timers including all > timer registers, but the timers did not "count" or generate any > interrupts. The patch makes the timers both count and generate > interrupts. The timer clock frequency is fixed at 100MHZ. > > Signed-off-by: Aaron Larson <alar...@ddci.com>
Looks sound in concept AFAICT not knowing the openpic hardware.
> ---
> hw/intc/openpic.c | 135
> ++++++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 117 insertions(+), 18 deletions(-)
>
> diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> index 4349e45..e0556f1 100644
> --- a/hw/intc/openpic.c
> +++ b/hw/intc/openpic.c
> @@ -45,6 +45,7 @@
> #include "qemu/bitops.h"
> #include "qapi/qmp/qerror.h"
> #include "qemu/log.h"
> +#include "qemu/timer.h"
>
> //#define DEBUG_OPENPIC
>
> @@ -54,8 +55,10 @@ static const int debug_openpic = 1;
> static const int debug_openpic = 0;
> #endif
>
> +static int get_current_cpu(void);
> #define DPRINTF(fmt, ...) do { \
> if (debug_openpic) { \
> + printf("Core%d: ", get_current_cpu()); \
> printf(fmt , ## __VA_ARGS__); \
> } \
> } while (0)
> @@ -246,9 +249,25 @@ typedef struct IRQSource {
> #define IDR_EP 0x80000000 /* external pin */
> #define IDR_CI 0x40000000 /* critical interrupt */
>
> +/* Conversion between openpic clock ticks and nanosecs. Ideally this clock
> + frequency would follow the openpic spec, for now hard code to 100mz.
> + A 100mhz clock, divided by 8, or 25mhz
> + 25,000,000 ticks/sec, 25,000/ms, 25/us, 1 tick/40ns
> +*/
> +#define CONV_FACTOR 40LL
> +static inline uint64_t ns_to_ticks(uint64_t ns) { return ns /
> CONV_FACTOR; }
> +static inline uint64_t ticks_to_ns(uint64_t tick) { return tick *
> CONV_FACTOR; }
This is a little hard to follow. Where does the divide by 8 come
from? Also 100MHz / 8 is 12.5 MHz, not 25MHz..
I'd prefer logic that comes from an explicit clock frequency
value, even if that's a constant 100000000 for now.
> typedef struct OpenPICTimer {
> uint32_t tccr; /* Global timer current count register */
> uint32_t tbcr; /* Global timer base count register */
> + int n_IRQ;
> + bool qemu_timer_active; /* Is the qemu_timer is
> running? */
> + struct QEMUTimer *qemu_timer; /* May be NULL if not created. */
> + struct OpenPICState *opp; /* Device timer is part of. */
> + /* The QEMU_CLOCK_VIRTUAL time (in ns) corresponding to the last
> + current_count written or read, only defined if qemu_timer_active. */
> + uint64_t originTime;
qemu doesn't generally use camelCase for structure fields. I'd
consider an exception if the name 'originTime' appears exactly like
that in the documentation, otherwise not.
> } OpenPICTimer;
>
> typedef struct OpenPICMSI {
> @@ -795,37 +814,102 @@ static uint64_t openpic_gbl_read(void *opaque, hwaddr
> addr, unsigned len)
> return retval;
> }
>
> +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool
> enabled);
> +
> +static void qemu_timer_cb(void *opaque)
> +{
> + OpenPICTimer *tmr = opaque;
> + OpenPICState *opp = tmr->opp;
> + uint32_t n_IRQ = tmr->n_IRQ;
> + uint32_t val = tmr->tbcr & ~TBCR_CI;
> + uint32_t tog = ((tmr->tccr & TCCR_TOG) ^ TCCR_TOG); /* invert toggle. */
> +
> + DPRINTF("%s n_IRQ=%d\n", __func__, n_IRQ);
> + /* Reload current count from base count and setup timer. */
> + tmr->tccr = val | tog;
> + openpic_tmr_set_tmr(tmr, val, /*enabled=*/true);
> + /* Raise the interrupt. */
> + opp->src[n_IRQ].destmask = read_IRQreg_idr(opp, n_IRQ);
> + openpic_set_irq(opp, n_IRQ, 1);
> + openpic_set_irq(opp, n_IRQ, 0);
> +}
> +
> +/* If enabled is true, arranges for an interrupt to be raised val clocks into
> + the future, if enabled is false cancels the timer. */
> +static void openpic_tmr_set_tmr(OpenPICTimer *tmr, uint32_t val, bool
> enabled)
> +{
> + /* If timer doesn't exist, create it. */
> + if (tmr->qemu_timer == NULL) {
> + tmr->qemu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &qemu_timer_cb,
> tmr);
> + DPRINTF("Created timer for n_IRQ %d\n", tmr->n_IRQ);
Is there a reason to lazily create the timer, rather than always
creating it at init time and just activating it when the timer is set?
> + }
> + uint64_t ns = ticks_to_ns(val & ~TCCR_TOG);
> + /* A count of zero causes a timer to be set to expire immediately. This
> + effectively stops the simulation so we don't honor that configuration.
> + On real hardware, this would generate an interrupt on every clock
> cycle
> + if the interrupt was unmasked. */
Could you also jam up if the count is non-zero but a too-small value
to make forward progress? It's probably worth doing an error_report()
in this case too, so the user has some idea what's wrong.
> + if ((ns == 0) || !enabled) {
> + tmr->qemu_timer_active = false;
> + tmr->tccr = tmr->tccr & TCCR_TOG;
> + timer_del(tmr->qemu_timer); /* set timer to never expire. */
> + } else {
> + tmr->qemu_timer_active = true;
> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + tmr->originTime = now;
> + timer_mod(tmr->qemu_timer, now + ns); /* set timer expiration. */
> + }
> +}
> +
> +/* Returns the currrent tccr value, i.e., timer value (in clocks) with
> + appropriate TOG. */
> +static uint64_t openpic_tmr_get_timer(OpenPICTimer *tmr)
> +{
> + uint64_t retval;
> + if (!tmr->qemu_timer_active) {
> + retval = tmr->tccr;
> + } else {
> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + uint64_t used = now - tmr->originTime; /* nsecs */
> + uint32_t used_ticks = (uint32_t)ns_to_ticks(used);
> + uint32_t count = (tmr->tccr & ~TCCR_TOG) - used_ticks;
> + retval = (uint32_t)((tmr->tccr & TCCR_TOG) | (count & ~TCCR_TOG));
> + }
> + return retval;
> +}
> +
> static void openpic_tmr_write(void *opaque, hwaddr addr, uint64_t val,
> - unsigned len)
> + unsigned len)
> {
> OpenPICState *opp = opaque;
> int idx;
>
> - addr += 0x10f0;
> -
> DPRINTF("%s: addr %#" HWADDR_PRIx " <= %08" PRIx64 "\n",
> - __func__, addr, val);
> + __func__, (addr + 0x10f0), val);
> if (addr & 0xF) {
> return;
> }
>
> - if (addr == 0x10f0) {
> + if (addr == 0) {
I don't really understand how this change fits in with the rest - it
appears to be changing existing unrelated behaviour.
> /* TFRR */
> opp->tfrr = val;
> return;
> }
> -
> + addr -= 0x10; /* correct for TFRR */
> idx = (addr >> 6) & 0x3;
> - addr = addr & 0x30;
>
> switch (addr & 0x30) {
> case 0x00: /* TCCR */
> break;
> case 0x10: /* TBCR */
> - if ((opp->timers[idx].tccr & TCCR_TOG) != 0 &&
> - (val & TBCR_CI) == 0 &&
> - (opp->timers[idx].tbcr & TBCR_CI) != 0) {
> - opp->timers[idx].tccr &= ~TCCR_TOG;
> + /* Did the enable status change? */
> + if ((opp->timers[idx].tbcr & TBCR_CI) != (val & TBCR_CI)) {
> + /* Did "Count Inhibit" transition from 1 to 0? */
> + if ((val & TBCR_CI) == 0) {
> + opp->timers[idx].tccr = val & ~TCCR_TOG;
> + }
> + openpic_tmr_set_tmr(&opp->timers[idx],
> + (val & ~TBCR_CI),
> + /*enabled=*/((val & TBCR_CI) == 0));
> }
> opp->timers[idx].tbcr = val;
> break;
> @@ -844,27 +928,28 @@ static uint64_t openpic_tmr_read(void *opaque, hwaddr
> addr, unsigned len)
> uint32_t retval = -1;
> int idx;
>
> - DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr);
> + DPRINTF("%s: addr %#" HWADDR_PRIx "\n", __func__, addr + 0x10f0);
> if (addr & 0xF) {
> goto out;
> }
> - idx = (addr >> 6) & 0x3;
> - if (addr == 0x0) {
> + if (addr == 0) {
> /* TFRR */
> retval = opp->tfrr;
> goto out;
> }
> + addr -= 0x10; /* correct for TFRR */
> + idx = (addr >> 6) & 0x3;
> switch (addr & 0x30) {
> case 0x00: /* TCCR */
> - retval = opp->timers[idx].tccr;
> + retval = openpic_tmr_get_timer(&opp->timers[idx]);
> break;
> case 0x10: /* TBCR */
> retval = opp->timers[idx].tbcr;
> break;
> - case 0x20: /* TIPV */
> + case 0x20: /* TVPR */
> retval = read_IRQreg_ivpr(opp, opp->irq_tim0 + idx);
> break;
> - case 0x30: /* TIDE (TIDR) */
> + case 0x30: /* TDR */
> retval = read_IRQreg_idr(opp, opp->irq_tim0 + idx);
> break;
> }
> @@ -1138,7 +1223,10 @@ static uint32_t openpic_iack(OpenPICState *opp,
> IRQDest *dst, int cpu)
> IRQ_resetbit(&dst->raised, irq);
> }
>
> - if ((irq >= opp->irq_ipi0) && (irq < (opp->irq_ipi0 +
> OPENPIC_MAX_IPI))) {
> + /* Timers and IPIs support multicast. */
> + if (((irq >= opp->irq_ipi0) && (irq < (opp->irq_ipi0 +
> OPENPIC_MAX_IPI))) ||
> + ((irq >= opp->irq_tim0) && (irq < (opp->irq_tim0 +
> OPENPIC_MAX_TMR)))) {
> + DPRINTF("irq is IPI or TMR\n");
> src->destmask &= ~(1 << cpu);
> if (src->destmask && !src->level) {
> /* trigger on CPUs that didn't know about it yet */
> @@ -1343,6 +1431,10 @@ static void openpic_reset(DeviceState *d)
> for (i = 0; i < OPENPIC_MAX_TMR; i++) {
> opp->timers[i].tccr = 0;
> opp->timers[i].tbcr = TBCR_CI;
> + if (opp->timers[i].qemu_timer_active) {
> + timer_del(opp->timers[i].qemu_timer); /* Inhibit timer */
> + opp->timers[i].qemu_timer_active = false;
> + }
> }
> /* Go out of RESET state */
> opp->gcr = 0;
> @@ -1393,6 +1485,13 @@ static void fsl_common_init(OpenPICState *opp)
> opp->src[i].type = IRQ_TYPE_FSLSPECIAL;
> opp->src[i].level = false;
> }
> +
> + for (i = 0; i < OPENPIC_MAX_TMR; i++) {
> + opp->timers[i].n_IRQ = opp->irq_tim0 + i;
> + opp->timers[i].qemu_timer_active = false;
> + opp->timers[i].qemu_timer = NULL;
> + opp->timers[i].opp = opp;
> + }
> }
>
> static void map_list(OpenPICState *opp, const MemReg *list, int *count)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
