Re: [RFC PATCH v7 05/29] target/loongarch: Add constant timer support

[yangxiaojuan]

On 2022/3/29 上午3:46, Richard Henderson wrote:
> On 3/28/22 06:57, Xiaojuan Yang wrote:
> > +void cpu_loongarch_store_constant_timer_config(LoongArchCPU *cpu,
> > +                                               uint64_t value)
> > +{
> > +    CPULoongArchState *env = &cpu->env;
> > +    uint64_t now, next;
> > +
> > +    env->CSR_TCFG = value;
> > +    if (value & CONSTANT_TIMER_ENABLE) {
> > +        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +        next = now + (value & CONSTANT_TIMER_TICK_MASK) * TIMER_PERIOD;
> > +        timer_mod(&cpu->timer, next);
> > +    }
>
> If CONSTANT_TIMER_ENABLE is not set, you need to use timer_del() to 
> turn off any existing timer.
OK
> > +void loongarch_constant_timer_cb(void *opaque)
> > +{
> > +    LoongArchCPU *cpu  = opaque;
> > +    CPULoongArchState *env = &cpu->env;
> > +    uint64_t now, next;
> > +
> > +    if (FIELD_EX64(env->CSR_TCFG, CSR_TCFG, PERIODIC)) {
> > +        now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +        next = now + (env->CSR_TCFG & CONSTANT_TIMER_TICK_MASK) * 
> > TIMER_PERIOD;
> > +        timer_mod(&cpu->timer, next);
> > +    } else {
> > +        env->CSR_TCFG = FIELD_DP64(env->CSR_TCFG, CSR_TCFG, EN, 0);
> > +    }
> > +
> > +    env->CSR_ESTAT |= 1 << IRQ_TIMER;
> > +    cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
>
> I think this is wrong and you should be using loongarch_cpu_set_irq 
> (which is misplaced for you to be able to do so).
reuse loongarch_cpu_set_irq?  like this:
void loongarch_constant_timer_cb(void *opaque)
{
    ...
    if (FIELD_EX64(...)) {
    ...
    } else {
    ...
    }
    loongarch_cpu_set_irq(opaque, IRQ_IMER, 1);
}
> > @@ -297,4 +302,9 @@ enum {
> >   #define LOONGARCH_CPU_TYPE_NAME(model) model LOONGARCH_CPU_TYPE_SUFFIX
> >   #define CPU_RESOLVING_TYPE TYPE_LOONGARCH_CPU
> >   +void loongarch_constant_timer_cb(void *opaque);
> > +uint64_t cpu_loongarch_get_constant_timer_counter(LoongArchCPU *cpu);
> > +uint64_t cpu_loongarch_get_constant_timer_ticks(LoongArchCPU *cpu);
> > +void cpu_loongarch_store_constant_timer_config(LoongArchCPU *cpu,
> > +                                               uint64_t value);
>
> These can go in internals.h.
OK

Thanks.
Xiaojuan
> r~