Re: [Qemu-arm] [Qemu-devel] [Qemu-devel RFC v2 1/4] msf2: Add Smartfusion2 System timer

[Alistair Francis]

>>> +
>>> +    isr = !!(st->regs[R_RIS] & TIMER_RIS_ACK);
>>> +    ier = !!(st->regs[R_CTRL] & TIMER_CTRL_INTR);
>>> +
>>> +    qemu_set_irq(st->irq, (ier && isr));
>>> +}
>>> +
>>> +static uint64_t
>>> +timer_read(void *opaque, hwaddr addr, unsigned int size)
>>> +{
>>> +    struct timerblock *t = opaque;
>>> +    struct msf2_timer *st;
>>> +    uint32_t r = 0;
>>> +    unsigned int timer;
>>> +    int isr;
>>> +    int ier;
>>> +
>>> +    addr >>= 2;
>>> +    timer = timer_from_addr(addr);
>>> +    st = &t->timers[timer];
>>> +
>>> +    if (timer) {
>>> +        addr -= 6;
>>> +    }
>>
>> Isn't this timer logic just checking if (addr >> 2) == R_MAX and if it
>> is set (addr >> 2) back to zero? This seems an overly complex way to
>> check that.
> I did not get you clearly. Do you want me to write like this:
> unsigned int timer = 0;
>
> addr >>= 2;
> if (addr >= R_MAX) {
>     timer = 1;
>     addr =  addr - R_MAX;
> }

Yeah, I think this is clearer then what you had earlier.

Although why do you have to subtract R_MAX, shouldn't it just be an
error if accessing values larger then R_MAX?

>
>>
>>> +
>>> +    switch (addr) {
>>> +    case R_VAL:
>>> +        r = ptimer_get_count(st->ptimer);
>>> +        D(qemu_log("msf2_timer t=%d read counter=%x\n", timer, r));
>>> +        break;
>>> +
>>> +    case R_MIS:
>>> +        isr = !!(st->regs[R_RIS] & TIMER_RIS_ACK);
>>> +        ier = !!(st->regs[R_CTRL] & TIMER_CTRL_INTR);
>>> +        r = ier && isr;
>>> +        break;
>>> +
>>> +    default:
>>> +        if (addr < ARRAY_SIZE(st->regs)) {
>>> +            r = st->regs[addr];
>>> +        }
>>> +        break;
>>> +    }
>>> +    D(fprintf(stderr, "%s timer=%d %x=%x\n", __func__, timer, addr * 4, 
>>> r));
>>> +    return r;
>>> +}
>>> +
>>> +static void timer_update(struct msf2_timer *st)
>>> +{
>>> +    uint64_t count;
>>> +
>>> +    D(fprintf(stderr, "%s timer=%d\n", __func__, st->nr));
>>> +
>>> +    if (!(st->regs[R_CTRL] & TIMER_CTRL_ENBL)) {
>>> +        ptimer_stop(st->ptimer);
>>> +        return;
>>> +    }
>>> +
>>> +    count = st->regs[R_LOADVAL];
>>> +    ptimer_set_limit(st->ptimer, count, 1);
>>> +    ptimer_run(st->ptimer, 1);
>>> +}
>>
>> The update function should be above the read/write functions.
>>
> Ok I will change
>
>>> +
>>> +static void
>>> +timer_write(void *opaque, hwaddr addr,
>>> +            uint64_t val64, unsigned int size)
>>> +{
>>> +    struct timerblock *t = opaque;
>>> +    struct msf2_timer *st;
>>> +    unsigned int timer;
>>> +    uint32_t value = val64;
>>> +
>>> +    addr >>= 2;
>>> +    timer = timer_from_addr(addr);
>>> +    st = &t->timers[timer];
>>> +    D(fprintf(stderr, "%s addr=%x val=%x (timer=%d)\n",
>>> +             __func__, addr * 4, value, timer));
>>> +
>>> +    if (timer) {
>>> +        addr -= 6;
>>> +    }
>>
>> Same comment from the read function.
>>
>>> +
>>> +    switch (addr) {
>>> +    case R_CTRL:
>>> +        st->regs[R_CTRL] = value;
>>> +        timer_update(st);
>>> +        break;
>>> +
>>> +    case R_RIS:
>>> +        if (value & TIMER_RIS_ACK) {
>>> +            st->regs[R_RIS] &= ~TIMER_RIS_ACK;
>>> +        }
>>> +        break;
>>> +
>>> +    case R_LOADVAL:
>>> +        st->regs[R_LOADVAL] = value;
>>> +        if (st->regs[R_CTRL] & TIMER_CTRL_ENBL) {
>>> +            timer_update(st);
>>> +        }
>>> +        break;
>>> +
>>> +    case R_BGLOADVAL:
>>> +        st->regs[R_BGLOADVAL] = value;
>>> +        st->regs[R_LOADVAL] = value;
>>> +        break;
>>> +
>>> +    case R_VAL:
>>> +    case R_MIS:
>>> +        break;
>>> +
>>> +    default:
>>> +        if (addr < ARRAY_SIZE(st->regs)) {
>>> +            st->regs[addr] = value;
>>> +        }
>>> +        break;
>>> +    }
>>> +    timer_update_irq(st);
>>> +}
>>> +
>>> +static const MemoryRegionOps timer_ops = {
>>> +    .read = timer_read,
>>> +    .write = timer_write,
>>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>>> +    .valid = {
>>> +        .min_access_size = 4,
>>> +        .max_access_size = 4
>>> +    }
>>> +};
>>> +
>>> +static void timer_hit(void *opaque)
>>> +{
>>> +    struct msf2_timer *st = opaque;
>>> +    D(fprintf(stderr, "%s %d\n", __func__, st->nr));
>>> +    st->regs[R_RIS] |= TIMER_RIS_ACK;
>>> +
>>> +    if (!(st->regs[R_CTRL] & TIMER_CTRL_ONESHOT)) {
>>> +        timer_update(st);
>>> +    }
>>> +    timer_update_irq(st);
>>> +}
>>> +
>>> +static void msf2_timer_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    struct timerblock *t = MSF2_TIMER(dev);
>>> +    unsigned int i;
>>> +
>>> +    /* Init all the ptimers.  */
>>> +    t->timers = g_malloc0((sizeof t->timers[0]) * NUM_TIMERS);
>>> +    for (i = 0; i < NUM_TIMERS; i++) {
>>> +        struct msf2_timer *st = &t->timers[i];
>>> +
>>> +        st->parent = t;
>>> +        st->nr = i;
>>> +        st->bh = qemu_bh_new(timer_hit, st);
>>> +        st->ptimer = ptimer_init(st->bh, PTIMER_POLICY_DEFAULT);
>>> +        ptimer_set_freq(st->ptimer, t->freq_hz);
>>> +        sysbus_init_irq(SYS_BUS_DEVICE(dev), &st->irq);
>>> +    }
>>> +
>>> +    memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, "msf2-timer",
>>> +                          R_MAX * 4 * NUM_TIMERS);
>>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &t->mmio);
>>
>> This should be in the devices init() function.
>
> I referred Xilinx soft IP models for writing these models and used
> same boilerplate code.
> I am not clear about realize and init functions yet. Can you please
> give a brief about them.

Basically the simple explanation is that init is called when the
object is created and realize is called when the object is realized.

Generally for devices it will go something like this:
 1. init
 2. Set properties
 3. Connect things
 4. realize
 5. Map to memory

> Don't we need to use realize function for new models?

AFAIK we still put things like: sysbus_init_irq(),
memory_region_init_io() and sysbus_init_mmio() in the init function.

I don't think we are at a stage yet to not use init functions.

Thanks,

Alistair

>
> Thanks,
> Sundeep
>>
>> Thanks,
>>
>> Alistair
>>
>>> +}
>>> +
>>> +static Property msf2_timer_properties[] = {
>>> +    DEFINE_PROP_UINT32("clock-frequency", struct timerblock, freq_hz,
>>> +                                                                83 * 
>>> 1000000),
>>> +    DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void msf2_timer_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> +    dc->realize = msf2_timer_realize;
>>> +    dc->props = msf2_timer_properties;
>>> +}
>>> +
>>> +static const TypeInfo msf2_timer_info = {
>>> +    .name          = TYPE_MSF2_TIMER,
>>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>>> +    .instance_size = sizeof(struct timerblock),
>>> +    .class_init    = msf2_timer_class_init,
>>> +};
>>> +
>>> +static void msf2_timer_register_types(void)
>>> +{
>>> +    type_register_static(&msf2_timer_info);
>>> +}
>>> +
>>> +type_init(msf2_timer_register_types)
>>> --
>>> 2.5.0
>>>
>>>