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

[sundeep subbaraya]

Hi Alistair,

On Mon, Apr 24, 2017 at 11:14 PM, Alistair Francis <address@hidden> wrote:
>>>> +
>>>> +    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?

Sorry I forgot about replying to this in earlier mail.
There are two independent timer blocks accessing same base address.
Based on offset passed in read/write functions we figure out
which block has to be handled.
0x0 to 0x14 -> timer1
0x18 to 0x2C -> timer2
Here R_MAX is 0x18 hence addr >= R_MAX is valid and refers to timer2.
Although I missed the bounds checking 0 < addr < 0x2C. I will add that
check in read and
write functions.

Thanks,
Sundeep
>
>>
>>>
>>>> +
>>>> +    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
>>>>
>>>>