qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 5/7] nios2: Add periodic timer emulation


From: Marek Vasut
Subject: Re: [Qemu-devel] [PATCH 5/7] nios2: Add periodic timer emulation
Date: Tue, 16 Aug 2016 18:48:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.2.0

On 08/15/2016 01:39 PM, Dmitry Osipenko wrote:

[...]

>>> Also, ptimer now supports "on the fly mode switch":
>>>
>>> https://github.com/qemu/qemu/commit/869e92b5c392eb6b2c7b398b878c435442b8e9dd
>>>
>>> ptimer_run(t->ptimer, !(value & CONTROL_CONT)) could be used here and 
>>> "manual"
>>> re-run dropped from the timer_hit() handler.
>>
>> So who will re-run the timer if it's configured in the continuous mode
>> if it's not re-run in the timer_hit() ?
>>
> 
> Ptimer will, timer_hit() will only have to assert IRQ and set ptimer count to
> the limit tvalue if timer is in the oneshot mode.
> 
> Something like:
> 
> static void timer_hit(void *opaque)
> {
>     AlteraTimer *t = opaque;
>     const uint64_t tvalue = (t->regs[R_PERIODH] << 16) | t->regs[R_PERIODL];
> 
>     t->regs[R_STATUS] |= STATUS_TO;
> 
>     if (!(t->regs[R_CONTROL] & CONTROL_CONT)) {
>         t->regs[R_STATUS] &= ~STATUS_RUN;

There should probably be } else { statement here , no ?

>       ptimer_set_count(t->ptimer, tvalue);
>     }
> 
>     qemu_set_irq(t->irq, timer_irq_state(t));
> }

Thanks for the draft.

[...]

>>>>>> +static void timer_hit(void *opaque)
>>>>>> +{
>>>>>> +    AlteraTimer *t = opaque;
>>>>>> +    const uint64_t tvalue = (t->regs[R_PERIODH] << 16) | 
>>>>>> t->regs[R_PERIODL];
>>>
>>> ptimer_get_limit() could be used here.
>>
>> You probably want to use the value in the register instead of the value
>> in the ptimer state in case someone rewrote those registers, no ?
>>
> 
> In case someone rewrote the registers, the ptimer limit would be also updated.
> So this could be changed to:
> 
> uint64_t tvalue = ptimer_get_limit(t->ptimer) - 1;
> 
> Actually, R_PERIODL and R_PERIODH regs aren't really needed and could be 
> removed
> from the AlteraTimer state, since ptimer stores the same limit value + 1. The
> timer_read/write() would need to be changed accordingly.

Ha, so we're getting to something like the following code (based on your
example + the else bit) ?

static void timer_hit(void *opaque)
{
    AlteraTimer *t = opaque;
    const uint64_t tvalue = ptimer_get_limit(t->ptimer) - 1;

    t->regs[R_STATUS] |= STATUS_TO;

    if (!(t->regs[R_CONTROL] & CONTROL_CONT)) {
        t->regs[R_STATUS] &= ~STATUS_RUN;
    } else {
        ptimer_set_count(t->ptimer, tvalue);
    }

    qemu_set_irq(t->irq, timer_irq_state(t));
}

[...]

>> For the timer, that reset would look like this?
>>
>> 197 static void altera_timer_reset(DeviceState *dev)
>> 198 {
>> 199     AlteraTimer *t = ALTERA_TIMER(dev);
>> 200
>> 201     ptimer_stop(t->ptimer);
>> 202     memset(t->regs, 0, ARRAY_SIZE(t->regs));
>> 203 }
>>
> 
> Yes, and move ptimer_set_limit(t->ptimer, 0xffffffff, 1) here from the
> altera_timer_realize().

Got it, thanks.

>> +static Property altera_timer_properties[] = {
>> +    DEFINE_PROP_UINT32("clock-frequency", AlteraTimer, freq_hz, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
> 
> Why not to have some sane "clock-frequency" by default?

Well what is sane clock frequency for hardware which can have arbitrary
frequency configured in ?

>> For the IIC, I wonder what that'd look like -- probably
>> qemu_set_irq(parent, 0); ?
>>
> 
> No, IRQ's would be "deasserted" by resetting CPUClass state, of which QEMU 
> takes
> care itself. No action needed by the IIC.

I see, thanks :)

btw any chance someone can look at the other patches too ?


-- 
Best regards,
Marek Vasut



reply via email to

[Prev in Thread] Current Thread [Next in Thread]