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

[Peter Chubb]

>>>>> "Peter" == Peter Maydell <address@hidden> writes:

Peter> On 3 April 2012 02:55, Peter Chubb <address@hidden>
Peter> wrote:

>+        /*
>+         * 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;
+
Peter> This seems like the wrong level to do this. If over-eager timer
Peter> ticks are a problem we ought to be applying the limit globally
Peter> in the timer infrastructure layer somewhere, not via ad-hoc
Peter> bandaids in individual device models.

There appears to be some attempt in some code paths to limit the
interval between timer ticks to 250us in the timer code, but tracing
the relationship between ptimers, qemu-timers and the underlying posix
timers is difficult.  In particular it's easy to create a ptimer that
runs so fast that QEMU spends all its time there.  On real hardware
this isn't a problem --- interrupts end up being coalesced, 

I have a test image that reliably hangs without this hack and the
corresponding one in the timerp code.  It works on the real hardware.



>> +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; +    } + +}

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

I'm not sure how to make qdev properties work.  The timers need to be
able to get at the CCM-generated frequencies, which of course are
private to the CCM.

The same thing happens in the imx_serial emulation, to hook the right
serial device to the right host device.

Is there any documentation or appropriate examples anywhere?

--
Dr Peter Chubb                                  peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA