On Tue, Apr 25, 2017 at 3:36 AM, sundeep subbaraya
<sundeep.l...@gmail.com> wrote:
> Hi Alistair,
>
> On Mon, Apr 24, 2017 at 11:14 PM, Alistair Francis <alistai...@gmail.com>
> 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.
Ok, when you send the next version can you explain this in comments
that way it's clear what you are trying to do.
Thanks,
Alistair
>
> 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
>>>>>
>>>>>
