Re: [Qemu-devel] [patch V5 3/5] FreeSCALE i.MX31 support: Timers

[Peter Maydell]

On 3 April 2012 02:55, Peter Chubb <address@hidden> wrote:
> +    case 2: /* LR - set ticks */
> +        s->lr = value;
> +        /*
> +         * Artificially limit tick rate to something
> +         * achievable under QEMU.  Otherwise, QEMU spends all
> +         * its time generating timer interrupts, and there
> +         * is no forward progress.
> +         * About ten microseconds is the fastest that really works.
> +         */
> +        if ((value * 1000000)/s->freq < 10) {
> +            value = 10*1000000/s->freq;
> +        }
> +        ptimer_set_limit(s->timer, value, !!(s->cr & CR_IOVW));
> +        break;

This seems like the wrong level to do this. If over-eager timer
ticks are a problem we ought to be applying the limit globally
in the timer infrastructure layer somewhere, not via ad-hoc
bandaids in individual device models.

> +void imx_timer_create(const char * const name,
> +                              const target_phys_addr_t addr,
> +                              qemu_irq irq,
> +                              DeviceState *ccm)
> +{
> +    IMXTimerGState *gp;
> +    IMXTimerPState *pp;
> +    DeviceState *dev;
> +
> +    dev = sysbus_create_simple(name, addr, irq);
> +    if (strcmp(name, "imx_timerp") == 0) {
> +        pp = container_of(dev, IMXTimerPState, busdev.qdev);
> +        pp->ccm = ccm;
> +    } else {
> +        gp = container_of(dev, IMXTimerGState, busdev.qdev);
> +        gp->ccm = ccm;
> +    }
> +
> +}

This is wrong in two ways:
(a) strcmp() on the device name is pretty ugly
(b) this kind of helper creation method mustn't go reaching
into the implementation of the device like this. If you need
to hand something to the device it needs to be a qdev property.

-- PMM