Patchwork [1/6,RFC] Emulation of GRLIB GPTimer as defined in GRLIB IP Core User's Manual.

login
register
mail settings
Submitter Fabien Chouteau
Date Dec. 6, 2010, 9:26 a.m.
Message ID <e514d821905ac015d3af52139769bb75e7bc2b1b.1291397462.git.chouteau@adacore.com>
Download mbox | patch
Permalink /patch/74323/
State New
Headers show

Comments

Fabien Chouteau - Dec. 6, 2010, 9:26 a.m.
Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 hw/grlib_gptimer.c |  448 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 448 insertions(+), 0 deletions(-)
Blue Swirl - Dec. 6, 2010, 5:12 p.m.
On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau <chouteau@adacore.com> wrote:
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  hw/grlib_gptimer.c |  448 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 448 insertions(+), 0 deletions(-)
>
> diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
> new file mode 100644
> index 0000000..41edbe4
> --- /dev/null
> +++ b/hw/grlib_gptimer.c
> @@ -0,0 +1,448 @@
> +/*
> + * QEMU GRLIB GPTimer Emulator
> + *
> + * Copyright (c) 2010 AdaCore
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "sysbus.h"
> +#include "qemu-timer.h"
> +
> +#include "grlib.h"
> +
> +/* #define DEBUG_TIMER */

The usual convention is
//#define DEBUG_TIMER
for easy editing.

However, very often the much more powerful tracepoints can replace
debug statements.

> +
> +#ifdef DEBUG_TIMER
> +#define DPRINTF(fmt, ...)                                       \
> +    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...)
> +#endif
> +
> +#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
> +#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
> +
> +#define GPTIMER_MAX_TIMERS 8
> +
> +/* GPTimer Config register fields */
> +#define GPTIMER_ENABLE      (1 << 0)
> +#define GPTIMER_RESTART     (1 << 1)
> +#define GPTIMER_LOAD        (1 << 2)
> +#define GPTIMER_INT_ENABLE  (1 << 3)
> +#define GPTIMER_INT_PENDING (1 << 4)
> +#define GPTIMER_CHAIN       (1 << 5) /* Not supported */
> +#define GPTIMER_DEBUG_HALT  (1 << 6) /* Not supported */
> +
> +/* Memory mapped register offsets */
> +#define SCALER_OFFSET         0x00
> +#define SCALER_RELOAD_OFFSET  0x04
> +#define CONFIG_OFFSET         0x08
> +#define COUNTER_OFFSET        0x00
> +#define COUNTER_RELOAD_OFFSET 0x04
> +#define TIMER_BASE            0x10
> +
> +typedef struct GPTimer     GPTimer;
> +typedef struct GPTimerUnit GPTimerUnit;
> +
> +struct GPTimer
> +{
> +    QEMUBH *bh;
> +    struct ptimer_state *ptimer;
> +
> +    qemu_irq     irq;
> +    int          id;
> +    GPTimerUnit *unit;
> +
> +    /* registers */
> +    uint32_t counter;
> +    uint32_t reload;
> +    uint32_t config;
> +};
> +
> +struct GPTimerUnit
> +{
> +    SysBusDevice  busdev;
> +
> +    uint32_t nr_timers;         /* Number of timers available */
> +    uint32_t freq_hz;           /* System frequency */
> +    uint32_t irq_line;          /* Base irq line */
> +
> +    GPTimer *timers;
> +
> +    /* registers */
> +    uint32_t scaler;
> +    uint32_t reload;
> +    uint32_t config;
> +};
> +
> +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
> +                                  uint32_t            nr_timers,
> +                                  uint32_t            freq,
> +                                  qemu_irq           *cpu_irqs,
> +                                  int                 base_irq)

This function belongs to leon3.c.

> +{
> +    DeviceState *dev;
> +    int i;
> +
> +    dev = qdev_create(NULL, "grlib,gptimer");
> +    qdev_prop_set_uint32(dev, "nr-timers", nr_timers);
> +    qdev_prop_set_uint32(dev, "frequency", freq);
> +    qdev_prop_set_uint32(dev, "irq-line", base_irq);

Base irq is not device property, but part of board configuration. Thus
leon3.c should just pass &cpu_irqs[base_irq] to this function.

> +
> +    if (qdev_init(dev)) {
> +        return NULL;
> +    }
> +
> +    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
> +
> +    for (i = 0; i < nr_timers; i++)
> +        sysbus_connect_irq(sysbus_from_qdev(dev), i, cpu_irqs[base_irq + i]);
> +
> +    return dev;
> +}
> +
> +static void grlib_gptimer_enable(GPTimer *timer)
> +{
> +    assert(timer != NULL);
> +
> +    DPRINTF("%s id:%d\n", __func__, timer->id);
> +
> +    ptimer_stop(timer->ptimer);
> +
> +    if (!(timer->config & GPTIMER_ENABLE)) {
> +        /* Timer disabled */
> +        DPRINTF("%s id:%d Timer disabled (config 0x%x)\n", __func__,
> +                timer->id, timer->config);
> +        return;
> +    }
> +
> +    /* ptimer is triggered when the counter reach 0 but GPTimer is triggered at
> +       underflow. Set count + 1 to simulate the GPTimer behavior. */
> +
> +    DPRINTF("%s id:%d set count 0x%x and run\n",
> +            __func__,
> +            timer->id,
> +            timer->counter + 1);
> +
> +    ptimer_set_count(timer->ptimer, timer->counter + 1);
> +    ptimer_run(timer->ptimer, 1);
> +}
> +
> +static void grlib_gptimer_restart(GPTimer *timer)
> +{
> +    assert(timer != NULL);
> +
> +    DPRINTF("%s id:%d reload val: 0x%x\n", __func__, timer->id, timer->reload);
> +
> +    timer->counter = timer->reload;
> +    grlib_gptimer_enable(timer);
> +}
> +
> +static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
> +{
> +    int i = 0;
> +    uint32_t value = 0;
> +
> +    assert(unit != NULL);
> +
> +
> +    if (scaler > 0) {
> +        value = unit->freq_hz / (scaler + 1);
> +    } else {
> +        value = unit->freq_hz;
> +    }
> +
> +    DPRINTF("%s scaler:%d freq:0x%x\n", __func__, scaler, value);
> +
> +    for (i = 0; i < unit->nr_timers; i++) {
> +        ptimer_set_freq(unit->timers[i].ptimer, value);
> +    }
> +}
> +
> +static void grlib_gptimer_hit(void *opaque)
> +{
> +    GPTimer *timer = opaque;
> +    assert(timer != NULL);
> +
> +    DPRINTF("%s id:%d\n", __func__, timer->id);
> +
> +    /* Timer expired */
> +
> +    if (timer->config & GPTIMER_INT_ENABLE) {
> +        /* Set the pending bit (only unset by write in the config register) */
> +        timer->config &= GPTIMER_INT_PENDING;
> +        qemu_set_irq(timer->irq, 1);
> +    }
> +
> +    if (timer->config & GPTIMER_RESTART) {
> +        grlib_gptimer_restart(timer);
> +    }
> +}
> +
> +static uint32_t grlib_gptimer_readl (void *opaque, target_phys_addr_t addr)

Extra space between function name and its arguments.

> +{
> +    GPTimerUnit *unit  = opaque;
> +    uint32_t     value = 0;
> +
> +    addr &= 0xff;
> +
> +    assert(unit != NULL);
> +
> +    /* Unit registers */
> +    switch (addr)
> +    {
> +        case SCALER_OFFSET:
> +            DPRINTF("%s scaler: 0x%x\n", __func__, unit->scaler);
> +            return unit->scaler;
> +
> +        case SCALER_RELOAD_OFFSET:
> +            DPRINTF("%s reload: 0x%x\n", __func__, unit->reload);
> +            return unit->reload;
> +
> +        case CONFIG_OFFSET:
> +            DPRINTF("%s unit config: 0x%x\n", __func__, unit->config);
> +            return unit->config;
> +
> +        default:
> +            break;
> +    }
> +
> +    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
> +    int                id         = (addr - TIMER_BASE) / TIMER_BASE;

Variables should be declared at the beginning of the function. But I'd
actually make two functions and use the first one for the general
registers above and second one for the timer registers below. Then the
opaque passed to the latter could be GPTimer instead of GPTimerUnit.

> +
> +    if (id >= 0 && id < unit->nr_timers) {
> +
> +        /* GPTimer registers */
> +        switch (timer_addr)
> +        {
> +            case COUNTER_OFFSET:
> +                value = ptimer_get_count (unit->timers[id].ptimer);
> +                DPRINTF("%s counter value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                return value;
> +
> +            case COUNTER_RELOAD_OFFSET:
> +                value = unit->timers[id].reload;
> +                DPRINTF("%s reload value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                return value;
> +
> +            case CONFIG_OFFSET:
> +                DPRINTF("%s config for timer %d: 0x%x\n",
> +                        __func__, id, unit->timers[id].config);
> +                return unit->timers[id].config;
> +
> +            default:
> +                break;
> +        }
> +
> +    }
> +
> +    DPRINTF("read unknown register 0x%04x\n", (int)addr);
> +    return 0;
> +}
> +
> +static void
> +grlib_gptimer_writel (void *opaque, target_phys_addr_t addr, uint32_t value)
> +{
> +    GPTimerUnit *unit = opaque;
> +
> +    addr &= 0xff;
> +
> +    assert(unit != NULL);
> +
> +    /* Unit registers */
> +    switch (addr)
> +    {
> +        case SCALER_OFFSET:
> +            value &= 0xFFFF; /* clean up the value */
> +            unit->scaler = value;
> +            return;
> +
> +        case SCALER_RELOAD_OFFSET:
> +            value &= 0xFFFF; /* clean up the value */
> +            unit->reload = value;
> +            grlib_gptimer_set_scaler(unit, value);
> +            return;
> +
> +        case CONFIG_OFFSET:
> +            /* Read Only (disable timer freeze not supported) */
> +            return;
> +
> +        default:
> +            break;
> +    }
> +
> +    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
> +    int                id         = (addr - TIMER_BASE) / TIMER_BASE;
> +
> +    if (id >= 0 && id < unit->nr_timers) {
> +
> +        /* GPTimer registers */
> +        switch (timer_addr)
> +        {
> +            case COUNTER_OFFSET:
> +                DPRINTF("%s counter value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                unit->timers[id].counter = value;
> +                grlib_gptimer_enable(&unit->timers[id]);
> +                return;
> +
> +            case COUNTER_RELOAD_OFFSET:
> +                DPRINTF("%s reload value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                unit->timers[id].reload = value;
> +                return;
> +
> +            case CONFIG_OFFSET:
> +                DPRINTF("%s config for timer %d: 0x%x\n", __func__, id, value);
> +
> +                unit->timers[id].config = value;
> +
> +                /* gptimer_restart calls gptimer_enable, so if "enable" and
> +                   "load" bits are present, we just have to call restart. */
> +
> +                if (value & GPTIMER_LOAD) {
> +                    grlib_gptimer_restart(&unit->timers[id]);
> +                } else if (value & GPTIMER_ENABLE) {
> +                    grlib_gptimer_enable(&unit->timers[id]);
> +                }
> +
> +                /* This fields must always be read as 0 */
> +                value &= ~(GPTIMER_LOAD & GPTIMER_DEBUG_HALT);
> +
> +                unit->timers[id].config = value;
> +                return;
> +
> +            default:
> +                break;
> +        }
> +
> +    }
> +
> +    DPRINTF("write unknown register 0x%04x\n", (int)addr);
> +}
> +
> +static CPUReadMemoryFunc *grlib_gptimer_read[] = {

'const'

> +    NULL, NULL, grlib_gptimer_readl,
> +};
> +
> +static CPUWriteMemoryFunc *grlib_gptimer_write[] = {
> +    NULL, NULL, grlib_gptimer_writel,
> +};
> +
> +static void grlib_gptimer_reset(void *opaque)
> +{
> +    /* int          i    = 0; */
> +    /* GPTimerUnit *unit = (GPTimerUnit *)opaque; */
> +    /* assert(unit != NULL); */
> +
> +    /* unit->scaler = 0; */
> +    /* unit->reload = 0; */
> +    /* unit->config = 0; */
> +
> +    /* unit->config  = unit->nr_timers; */
> +    /* unit->config |= unit->irq_line << 3; */
> +    /* unit->config |= 1 << 8;     /\* separate interrupt *\/ */
> +    /* unit->config |= 1 << 9;     /\* Disable timer freeze *\/ */
> +
> +
> +    /* for (i = 0; i < unit->nr_timers; i++) { */
> +    /*     GPTimer *timer = &unit->timers[i]; */
> +
> +    /*     timer->counter = 0; */
> +    /*     timer->reload = 0; */
> +    /*     timer->config = 0; */
> +    /*     ptimer_stop(timer->ptimer); */
> +    /*     ptimer_set_count(timer->ptimer, 0); */
> +    /*     ptimer_set_freq(timer->ptimer, unit->freq_hz); */
> +    /* } */

Why dead code?

> +
> +}
> +
> +static int grlib_gptimer_init(SysBusDevice *dev)
> +{
> +    GPTimerUnit  *unit = FROM_SYSBUS(typeof (*unit), dev);
> +    unsigned int  i;
> +    int           timer_regs;
> +
> +    assert(unit->nr_timers > 0);
> +    assert(unit->nr_timers <= GPTIMER_MAX_TIMERS);
> +    unit->timers = qemu_mallocz(sizeof unit->timers[0] * unit->nr_timers);
> +
> +    for (i = 0; i < unit->nr_timers; i++) {
> +        GPTimer *timer = &unit->timers[i];
> +
> +        timer->unit   = unit;
> +        timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
> +        timer->ptimer = ptimer_init(timer->bh);
> +        timer->id     = i;
> +
> +        /* One IRQ line for each timer */
> +        sysbus_init_irq(dev, &timer->irq);
> +
> +        ptimer_set_freq(timer->ptimer, unit->freq_hz);
> +    }
> +
> +    qemu_register_reset(grlib_gptimer_reset, unit);

This can be replaced by qdev.reset field.
Fabien Chouteau - Dec. 7, 2010, 9:55 a.m.
On 12/06/2010 06:12 PM, Blue Swirl wrote:
> On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau<chouteau@adacore.com>  wrote:
>>
>> Signed-off-by: Fabien Chouteau<chouteau@adacore.com>
>> ---
>>   hw/grlib_gptimer.c |  448 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 448 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
>> new file mode 100644
>> index 0000000..41edbe4
>> --- /dev/null
>> +++ b/hw/grlib_gptimer.c
>> @@ -0,0 +1,448 @@
>> +/*
>> + * QEMU GRLIB GPTimer Emulator
>> + *
>> + * Copyright (c) 2010 AdaCore
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "sysbus.h"
>> +#include "qemu-timer.h"
>> +
>> +#include "grlib.h"
>> +
>> +/* #define DEBUG_TIMER */
>
> The usual convention is
> //#define DEBUG_TIMER
> for easy editing.
>

Actually, it's easier for me with the /* */, but OK.

> However, very often the much more powerful tracepoints can replace
> debug statements.
>
>> +
>> +#ifdef DEBUG_TIMER
>> +#define DPRINTF(fmt, ...)                                       \
>> +    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...)
>> +#endif
>> +
>> +#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
>> +#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
>> +
>> +#define GPTIMER_MAX_TIMERS 8
>> +
>> +/* GPTimer Config register fields */
>> +#define GPTIMER_ENABLE      (1<<  0)
>> +#define GPTIMER_RESTART     (1<<  1)
>> +#define GPTIMER_LOAD        (1<<  2)
>> +#define GPTIMER_INT_ENABLE  (1<<  3)
>> +#define GPTIMER_INT_PENDING (1<<  4)
>> +#define GPTIMER_CHAIN       (1<<  5) /* Not supported */
>> +#define GPTIMER_DEBUG_HALT  (1<<  6) /* Not supported */
>> +
>> +/* Memory mapped register offsets */
>> +#define SCALER_OFFSET         0x00
>> +#define SCALER_RELOAD_OFFSET  0x04
>> +#define CONFIG_OFFSET         0x08
>> +#define COUNTER_OFFSET        0x00
>> +#define COUNTER_RELOAD_OFFSET 0x04
>> +#define TIMER_BASE            0x10
>> +
>> +typedef struct GPTimer     GPTimer;
>> +typedef struct GPTimerUnit GPTimerUnit;
>> +
>> +struct GPTimer
>> +{
>> +    QEMUBH *bh;
>> +    struct ptimer_state *ptimer;
>> +
>> +    qemu_irq     irq;
>> +    int          id;
>> +    GPTimerUnit *unit;
>> +
>> +    /* registers */
>> +    uint32_t counter;
>> +    uint32_t reload;
>> +    uint32_t config;
>> +};
>> +
>> +struct GPTimerUnit
>> +{
>> +    SysBusDevice  busdev;
>> +
>> +    uint32_t nr_timers;         /* Number of timers available */
>> +    uint32_t freq_hz;           /* System frequency */
>> +    uint32_t irq_line;          /* Base irq line */
>> +
>> +    GPTimer *timers;
>> +
>> +    /* registers */
>> +    uint32_t scaler;
>> +    uint32_t reload;
>> +    uint32_t config;
>> +};
>> +
>> +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
>> +                                  uint32_t            nr_timers,
>> +                                  uint32_t            freq,
>> +                                  qemu_irq           *cpu_irqs,
>> +                                  int                 base_irq)
>
> This function belongs to leon3.c.

I don't see why. GPTimer is a peripheral and you may want to use it in 
an other system.

>> +{
>> +    DeviceState *dev;
>> +    int i;
>> +_ir
>> +    dev = qdev_create(NULL, "grlib,gptimer");
>> +    qdev_prop_set_uint32(dev, "nr-timers", nr_timers);
>> +    qdev_prop_set_uint32(dev, "frequency", freq);
>> +    qdev_prop_set_uint32(dev, "irq-line", base_irq);
>
> Base irq is not device property, but part of board configuration. Thus
> leon3.c should just pass&cpu_irqs[base_irq] to this function.
>

I need this property to put the IRQ line in the configuration register. 
Is there a way to get this number from a qemu_irq structure?

>> +
>> +    if (qdev_init(dev)) {
>> +        return NULL;
>> +    }
>> +
>> +    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
>> +
>> +    for (i = 0; i<  nr_timers; i++)
>> +        sysbus_connect_irq(sysbus_from_qdev(dev), i, cpu_irqs[base_irq + i]);
>> +
>> +    return dev;
>> +}
>> +
>> +static void grlib_gptimer_enable(GPTimer *timer)
>> +{
>> +    assert(timer != NULL);
>> +
>> +    DPRINTF("%s id:%d\n", __func__, timer->id);
>> +
>> +    ptimer_stop(timer->ptimer);
>> +
>> +    if (!(timer->config&  GPTIMER_ENABLE)) {
>> +        /* Timer disabled */
>> +        DPRINTF("%s id:%d Timer disabled (config 0x%x)\n", __func__,
>> +                timer->id, timer->config);
>> +        return;
>> +    }
>> +
>> +    /* ptimer is triggered when the counter reach 0 but GPTimer is triggered at
>> +       underflow. Set count + 1 to simulate the GPTimer behavior. */
>> +
>> +    DPRINTF("%s id:%d set count 0x%x and run\n",
>> +            __func__,
>> +            timer->id,
>> +            timer->counter + 1);
>> +
>> +    ptimer_set_count(timer->ptimer, timer->counter + 1);
>> +    ptimer_run(timer->ptimer, 1);
>> +}
>> +
>> +static void grlib_gptimer_restart(GPTimer *timer)
>> +{
>> +    assert(timer != NULL);
>> +
>> +    DPRINTF("%s id:%d reload val: 0x%x\n", __func__, timer->id, timer->reload);
>> +
>> +    timer->counter = timer->reload;
>> +    grlib_gptimer_enable(timer);
>> +}
>> +
>> +static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
>> +{
>> +    int i = 0;
>> +    uint32_t value = 0;
>> +
>> +    assert(unit != NULL);
>> +
>> +
>> +    if (scaler>  0) {
>> +        value = unit->freq_hz / (scaler + 1);
>> +    } else {
>> +        value = unit->freq_hz;
>> +    }
>> +
>> +    DPRINTF("%s scaler:%d freq:0x%x\n", __func__, scaler, value);
>> +
>> +    for (i = 0; i<  unit->nr_timers; i++) {
>> +        ptimer_set_freq(unit->timers[i].ptimer, value);
>> +    }
>> +}
>> +
>> +static void grlib_gptimer_hit(void *opaque)
>> +{
>> +    GPTimer *timer = opaque;
>> +    assert(timer != NULL);
>> +
>> +    DPRINTF("%s id:%d\n", __func__, timer->id);
>> +
>> +    /* Timer expired */
>> +
>> +    if (timer->config&  GPTIMER_INT_ENABLE) {
>> +        /* Set the pending bit (only unset by write in the config register) */
>> +        timer->config&= GPTIMER_INT_PENDING;
>> +        qemu_set_irq(timer->irq, 1);
>> +    }
>> +
>> +    if (timer->config&  GPTIMER_RESTART) {
>> +        grlib_gptimer_restart(timer);
>> +    }
>> +}
>> +
>> +static uint32_t grlib_gptimer_readl (void *opaque, target_phys_addr_t addr)
>
> Extra space between function name and its arguments.

Fixed.

>
>> +{
>> +    GPTimerUnit *unit  = opaque;
>> +    uint32_t     value = 0;
>> +
>> +    addr&= 0xff;
>> +
>> +    assert(unit != NULL);
>> +
>> +    /* Unit registers */
>> +    switch (addr)
>> +    {
>> +        case SCALER_OFFSET:
>> +            DPRINTF("%s scaler: 0x%x\n", __func__, unit->scaler);
>> +            return unit->scaler;
>> +
>> +        case SCALER_RELOAD_OFFSET:
>> +            DPRINTF("%s reload: 0x%x\n", __func__, unit->reload);
>> +            return unit->reload;
>> +
>> +        case CONFIG_OFFSET:
>> +            DPRINTF("%s unit config: 0x%x\n", __func__, unit->config);
>> +            return unit->config;
>> +
>> +        default:
>> +            break;
>> +    }
>> +
>> +    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
>> +    int                id         = (addr - TIMER_BASE) / TIMER_BASE;
>
> Variables should be declared at the beginning of the function. But I'd
> actually make two functions and use the first one for the general
> registers above and second one for the timer registers below. Then the
> opaque passed to the latter could be GPTimer instead of GPTimerUnit.
>

If you want, I can put the variables at the beginning of the function, 
but I think that split the function will just obfuscate the code.

>> +
>> +    if (id>= 0&&  id<  unit->nr_timers) {
>> +
>> +        /* GPTimer registers */
>> +        switch (timer_addr)
>> +        {
>> +            case COUNTER_OFFSET:
>> +                value = ptimer_get_count (unit->timers[id].ptimer);
>> +                DPRINTF("%s counter value for timer %d: 0x%x\n",
>> +                        __func__, id, value);
>> +                return value;
>> +
>> +            case COUNTER_RELOAD_OFFSET:
>> +                value = unit->timers[id].reload;
>> +                DPRINTF("%s reload value for timer %d: 0x%x\n",
>> +                        __func__, id, value);
>> +                return value;
>> +
>> +            case CONFIG_OFFSET:
>> +                DPRINTF("%s config for timer %d: 0x%x\n",
>> +                        __func__, id, unit->timers[id].config);
>> +                return unit->timers[id].config;
>> +
>> +            default:
>> +                break;
>> +        }
>> +
>> +    }
>> +
>> +    DPRINTF("read unknown register 0x%04x\n", (int)addr);
>> +    return 0;
>> +}
>> +
>> +static void
>> +grlib_gptimer_writel (void *opaque, target_phys_addr_t addr, uint32_t value)
>> +{
>> +    GPTimerUnit *unit = opaque;
>> +
>> +    addr&= 0xff;
>> +
>> +    assert(unit != NULL);
>> +
>> +    /* Unit registers */
>> +    switch (addr)
>> +    {
>> +        case SCALER_OFFSET:
>> +            value&= 0xFFFF; /* clean up the value */
>> +            unit->scaler = value;
>> +            return;
>> +
>> +        case SCALER_RELOAD_OFFSET:
>> +            value&= 0xFFFF; /* clean up the value */
>> +            unit->reload = value;
>> +            grlib_gptimer_set_scaler(unit, value);
>> +            return;
>> +
>> +        case CONFIG_OFFSET:
>> +            /* Read Only (disable timer freeze not supported) */
>> +            return;
>> +
>> +        default:
>> +            break;
>> +    }
>> +
>> +    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
>> +    int                id         = (addr - TIMER_BASE) / TIMER_BASE;
>> +
>> +    if (id>= 0&&  id<  unit->nr_timers) {
>> +
>> +        /* GPTimer registers */
>> +        switch (timer_addr)
>> +        {
>> +            case COUNTER_OFFSET:
>> +                DPRINTF("%s counter value for timer %d: 0x%x\n",
>> +                        __func__, id, value);
>> +                unit->timers[id].counter = value;
>> +                grlib_gptimer_enable(&unit->timers[id]);
>> +                return;
>> +
>> +            case COUNTER_RELOAD_OFFSET:
>> +                DPRINTF("%s reload value for timer %d: 0x%x\n",
>> +                        __func__, id, value);
>> +                unit->timers[id].reload = value;
>> +                return;
>> +
>> +            case CONFIG_OFFSET:
>> +                DPRINTF("%s config for timer %d: 0x%x\n", __func__, id, value);
>> +
>> +                unit->timers[id].config = value;
>> +
>> +                /* gptimer_restart calls gptimer_enable, so if "enable" and
>> +                   "load" bits are present, we just have to call restart. */
>> +
>> +                if (value&  GPTIMER_LOAD) {
>> +                    grlib_gptimer_restart(&unit->timers[id]);
>> +                } else if (value&  GPTIMER_ENABLE) {
>> +                    grlib_gptimer_enable(&unit->timers[id]);
>> +                }
>> +
>> +                /* This fields must always be read as 0 */
>> +                value&= ~(GPTIMER_LOAD&  GPTIMER_DEBUG_HALT);
>> +
>> +                unit->timers[id].config = value;
>> +                return;
>> +
>> +            default:
>> +                break;
>> +        }
>> +
>> +    }
>> +
>> +    DPRINTF("write unknown register 0x%04x\n", (int)addr);
>> +}
>> +
>> +static CPUReadMemoryFunc *grlib_gptimer_read[] = {
>
> 'const'
>

Fixed.

>> +    NULL, NULL, grlib_gptimer_readl,
>> +};
>> +
>> +static CPUWriteMemoryFunc *grlib_gptimer_write[] = {
>> +    NULL, NULL, grlib_gptimer_writel,
>> +};
>> +
>> +static void grlib_gptimer_reset(void *opaque)
>> +{
>> +    /* int          i    = 0; */
>> +    /* GPTimerUnit *unit = (GPTimerUnit *)opaque; */
>> +    /* assert(unit != NULL); */
>> +
>> +    /* unit->scaler = 0; */
>> +    /* unit->reload = 0; */
>> +    /* unit->config = 0; */
>> +
>> +    /* unit->config  = unit->nr_timers; */
>> +    /* unit->config |= unit->irq_line<<  3; */
>> +    /* unit->config |= 1<<  8;     /\* separate interrupt *\/ */
>> +    /* unit->config |= 1<<  9;     /\* Disable timer freeze *\/ */
>> +
>> +
>> +    /* for (i = 0; i<  unit->nr_timers; i++) { */
>> +    /*     GPTimer *timer =&unit->timers[i]; */
>> +
>> +    /*     timer->counter = 0; */
>> +    /*     timer->reload = 0; */
>> +    /*     timer->config = 0; */
>> +    /*     ptimer_stop(timer->ptimer); */
>> +    /*     ptimer_set_count(timer->ptimer, 0); */
>> +    /*     ptimer_set_freq(timer->ptimer, unit->freq_hz); */
>> +    /* } */
>
> Why dead code?

No reason, this is just a mistake, the code should be un-commented.

>
>> +
>> +}
>> +
>> +static int grlib_gptimer_init(SysBusDevice *dev)
>> +{
>> +    GPTimerUnit  *unit = FROM_SYSBUS(typeof (*unit), dev);
>> +    unsigned int  i;
>> +    int           timer_regs;
>> +
>> +    assert(unit->nr_timers>  0);
>> +    assert(unit->nr_timers<= GPTIMER_MAX_TIMERS);
>> +    unit->timers = qemu_mallocz(sizeof unit->timers[0] * unit->nr_timers);
>> +
>> +    for (i = 0; i<  unit->nr_timers; i++) {
>> +        GPTimer *timer =&unit->timers[i];
>> +
>> +        timer->unit   = unit;
>> +        timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
>> +        timer->ptimer = ptimer_init(timer->bh);
>> +        timer->id     = i;
>> +
>> +        /* One IRQ line for each timer */
>> +        sysbus_init_irq(dev,&timer->irq);
>> +
>> +        ptimer_set_freq(timer->ptimer, unit->freq_hz);
>> +    }
>> +
>> +    qemu_register_reset(grlib_gptimer_reset, unit);
>
> This can be replaced by qdev.reset field.

All right, it's clearer indeed.
Edgar Iglesias - Dec. 8, 2010, 8:30 a.m.
On Tue, Dec 07, 2010 at 10:55:33AM +0100, Fabien Chouteau wrote:
> On 12/06/2010 06:12 PM, Blue Swirl wrote:
> > On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau<chouteau@adacore.com>  wrote:
> >>
> >> Signed-off-by: Fabien Chouteau<chouteau@adacore.com>
> >> ---
> >>   hw/grlib_gptimer.c |  448 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 files changed, 448 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
> >> new file mode 100644
> >> index 0000000..41edbe4
> >> --- /dev/null
> >> +++ b/hw/grlib_gptimer.c
> >> @@ -0,0 +1,448 @@
> >> +/*
> >> + * QEMU GRLIB GPTimer Emulator
> >> + *
> >> + * Copyright (c) 2010 AdaCore
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> >> + * of this software and associated documentation files (the "Software"), to deal
> >> + * in the Software without restriction, including without limitation the rights
> >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> >> + * copies of the Software, and to permit persons to whom the Software is
> >> + * furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice shall be included in
> >> + * all copies or substantial portions of the Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> >> + * THE SOFTWARE.
> >> + */
> >> +
> >> +#include "sysbus.h"
> >> +#include "qemu-timer.h"
> >> +
> >> +#include "grlib.h"
> >> +
> >> +/* #define DEBUG_TIMER */
> >
> > The usual convention is
> > //#define DEBUG_TIMER
> > for easy editing.
> >
> 
> Actually, it's easier for me with the /* */, but OK.
> 
> > However, very often the much more powerful tracepoints can replace
> > debug statements.
> >
> >> +
> >> +#ifdef DEBUG_TIMER
> >> +#define DPRINTF(fmt, ...)                                       \
> >> +    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
> >> +#else
> >> +#define DPRINTF(fmt, ...)
> >> +#endif
> >> +
> >> +#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
> >> +#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
> >> +
> >> +#define GPTIMER_MAX_TIMERS 8
> >> +
> >> +/* GPTimer Config register fields */
> >> +#define GPTIMER_ENABLE      (1<<  0)
> >> +#define GPTIMER_RESTART     (1<<  1)
> >> +#define GPTIMER_LOAD        (1<<  2)
> >> +#define GPTIMER_INT_ENABLE  (1<<  3)
> >> +#define GPTIMER_INT_PENDING (1<<  4)
> >> +#define GPTIMER_CHAIN       (1<<  5) /* Not supported */
> >> +#define GPTIMER_DEBUG_HALT  (1<<  6) /* Not supported */
> >> +
> >> +/* Memory mapped register offsets */
> >> +#define SCALER_OFFSET         0x00
> >> +#define SCALER_RELOAD_OFFSET  0x04
> >> +#define CONFIG_OFFSET         0x08
> >> +#define COUNTER_OFFSET        0x00
> >> +#define COUNTER_RELOAD_OFFSET 0x04
> >> +#define TIMER_BASE            0x10
> >> +
> >> +typedef struct GPTimer     GPTimer;
> >> +typedef struct GPTimerUnit GPTimerUnit;
> >> +
> >> +struct GPTimer
> >> +{
> >> +    QEMUBH *bh;
> >> +    struct ptimer_state *ptimer;
> >> +
> >> +    qemu_irq     irq;
> >> +    int          id;
> >> +    GPTimerUnit *unit;
> >> +
> >> +    /* registers */
> >> +    uint32_t counter;
> >> +    uint32_t reload;
> >> +    uint32_t config;
> >> +};
> >> +
> >> +struct GPTimerUnit
> >> +{
> >> +    SysBusDevice  busdev;
> >> +
> >> +    uint32_t nr_timers;         /* Number of timers available */
> >> +    uint32_t freq_hz;           /* System frequency */
> >> +    uint32_t irq_line;          /* Base irq line */
> >> +
> >> +    GPTimer *timers;
> >> +
> >> +    /* registers */
> >> +    uint32_t scaler;
> >> +    uint32_t reload;
> >> +    uint32_t config;
> >> +};
> >> +
> >> +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
> >> +                                  uint32_t            nr_timers,
> >> +                                  uint32_t            freq,
> >> +                                  qemu_irq           *cpu_irqs,
> >> +                                  int                 base_irq)
> >
> > This function belongs to leon3.c.
> 
> I don't see why. GPTimer is a peripheral and you may want to use it in 
> an other system.

This might depend a bit on taste, but I agree with Blue that we shouldn't
clutter the device models with this kind of instantiator helper calls.
IMO it's better to put them higher up (e.g board code or similar).

> 
> >> +{
> >> +    DeviceState *dev;
> >> +    int i;
> >> +_ir
> >> +    dev = qdev_create(NULL, "grlib,gptimer");
> >> +    qdev_prop_set_uint32(dev, "nr-timers", nr_timers);
> >> +    qdev_prop_set_uint32(dev, "frequency", freq);
> >> +    qdev_prop_set_uint32(dev, "irq-line", base_irq);
> >
> > Base irq is not device property, but part of board configuration. Thus
> > leon3.c should just pass&cpu_irqs[base_irq] to this function.
> >
> 
> I need this property to put the IRQ line in the configuration register. 
> Is there a way to get this number from a qemu_irq structure?

I don't think so. Also, I suspect that if you connect the device into
a larger interrupt structure (possibly with cascaded interrupt
controllers etc) the config value won't necessarily have much to do
with the particular qemu_irq object. So I think you need the
separate property. I might be missing something though.

Cheers
Fabien Chouteau - Dec. 8, 2010, 9:39 a.m.
On 12/08/2010 09:30 AM, Edgar E. Iglesias wrote:
> On Tue, Dec 07, 2010 at 10:55:33AM +0100, Fabien Chouteau wrote:
>> On 12/06/2010 06:12 PM, Blue Swirl wrote:
>>> On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau<chouteau@adacore.com>   wrote:
>>>> Signed-off-by: Fabien Chouteau<chouteau@adacore.com>
>>>> ---
>>>>    hw/grlib_gptimer.c |  448 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 files changed, 448 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
>>>> new file mode 100644
>>>> index 0000000..41edbe4
>>>> --- /dev/null
>>>> +++ b/hw/grlib_gptimer.c
>>>> @@ -0,0 +1,448 @@
>>>> +/*
>>>> + * QEMU GRLIB GPTimer Emulator
>>>> + *
>>>> + * Copyright (c) 2010 AdaCore
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>>>> + * of this software and associated documentation files (the "Software"), to deal
>>>> + * in the Software without restriction, including without limitation the rights
>>>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>>>> + * copies of the Software, and to permit persons to whom the Software is
>>>> + * furnished to do so, subject to the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice shall be included in
>>>> + * all copies or substantial portions of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>>>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>>>> + * THE SOFTWARE.
>>>> + */
>>>> +
>>>> +#include "sysbus.h"
>>>> +#include "qemu-timer.h"
>>>> +
>>>> +#include "grlib.h"
>>>> +
>>>> +/* #define DEBUG_TIMER */
>>> The usual convention is
>>> //#define DEBUG_TIMER
>>> for easy editing.
>>>
>> Actually, it's easier for me with the /* */, but OK.
>>
>>> However, very often the much more powerful tracepoints can replace
>>> debug statements.
>>>
>>>> +
>>>> +#ifdef DEBUG_TIMER
>>>> +#define DPRINTF(fmt, ...)                                       \
>>>> +    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
>>>> +#else
>>>> +#define DPRINTF(fmt, ...)
>>>> +#endif
>>>> +
>>>> +#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
>>>> +#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
>>>> +
>>>> +#define GPTIMER_MAX_TIMERS 8
>>>> +
>>>> +/* GPTimer Config register fields */
>>>> +#define GPTIMER_ENABLE      (1<<   0)
>>>> +#define GPTIMER_RESTART     (1<<   1)
>>>> +#define GPTIMER_LOAD        (1<<   2)
>>>> +#define GPTIMER_INT_ENABLE  (1<<   3)
>>>> +#define GPTIMER_INT_PENDING (1<<   4)
>>>> +#define GPTIMER_CHAIN       (1<<   5) /* Not supported */
>>>> +#define GPTIMER_DEBUG_HALT  (1<<   6) /* Not supported */
>>>> +
>>>> +/* Memory mapped register offsets */
>>>> +#define SCALER_OFFSET         0x00
>>>> +#define SCALER_RELOAD_OFFSET  0x04
>>>> +#define CONFIG_OFFSET         0x08
>>>> +#define COUNTER_OFFSET        0x00
>>>> +#define COUNTER_RELOAD_OFFSET 0x04
>>>> +#define TIMER_BASE            0x10
>>>> +
>>>> +typedef struct GPTimer     GPTimer;
>>>> +typedef struct GPTimerUnit GPTimerUnit;
>>>> +
>>>> +struct GPTimer
>>>> +{
>>>> +    QEMUBH *bh;
>>>> +    struct ptimer_state *ptimer;
>>>> +
>>>> +    qemu_irq     irq;
>>>> +    int          id;
>>>> +    GPTimerUnit *unit;
>>>> +
>>>> +    /* registers */
>>>> +    uint32_t counter;
>>>> +    uint32_t reload;
>>>> +    uint32_t config;
>>>> +};
>>>> +
>>>> +struct GPTimerUnit
>>>> +{
>>>> +    SysBusDevice  busdev;
>>>> +
>>>> +    uint32_t nr_timers;         /* Number of timers available */
>>>> +    uint32_t freq_hz;           /* System frequency */
>>>> +    uint32_t irq_line;          /* Base irq line */
>>>> +
>>>> +    GPTimer *timers;
>>>> +
>>>> +    /* registers */
>>>> +    uint32_t scaler;
>>>> +    uint32_t reload;
>>>> +    uint32_t config;
>>>> +};
>>>> +
>>>> +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
>>>> +                                  uint32_t            nr_timers,
>>>> +                                  uint32_t            freq,
>>>> +                                  qemu_irq           *cpu_irqs,
>>>> +                                  int                 base_irq)
>>> This function belongs to leon3.c.
>> I don't see why. GPTimer is a peripheral and you may want to use it in
>> an other system.
> This might depend a bit on taste, but I agree with Blue that we shouldn't
> clutter the device models with this kind of instantiator helper calls.
> IMO it's better to put them higher up (e.g board code or similar).

Do you mean like Xilinx devices where the instantiators are in-lined 
functions in hw/xilinx.h?
Edgar Iglesias - Dec. 8, 2010, 9:02 p.m.
On Wed, Dec 08, 2010 at 10:39:43AM +0100, Fabien Chouteau wrote:
> On 12/08/2010 09:30 AM, Edgar E. Iglesias wrote:
> > On Tue, Dec 07, 2010 at 10:55:33AM +0100, Fabien Chouteau wrote:
> >> On 12/06/2010 06:12 PM, Blue Swirl wrote:
> >>> On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau<chouteau@adacore.com>   wrote:
> >>>> +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
> >>>> +                                  uint32_t            nr_timers,
> >>>> +                                  uint32_t            freq,
> >>>> +                                  qemu_irq           *cpu_irqs,
> >>>> +                                  int                 base_irq)
> >>> This function belongs to leon3.c.
> >> I don't see why. GPTimer is a peripheral and you may want to use it in
> >> an other system.
> > This might depend a bit on taste, but I agree with Blue that we shouldn't
> > clutter the device models with this kind of instantiator helper calls.
> > IMO it's better to put them higher up (e.g board code or similar).
> 
> Do you mean like Xilinx devices where the instantiators are in-lined 
> functions in hw/xilinx.h?

Yes, that's one way. But if you only have a single board you can also just
put the function with the board code.

Cheers
Edgar Iglesias - Dec. 8, 2010, 10:51 p.m.
On Mon, Dec 06, 2010 at 10:26:02AM +0100, Fabien Chouteau wrote:
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  hw/grlib_gptimer.c |  448 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 448 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
> new file mode 100644
> index 0000000..41edbe4
> --- /dev/null
> +++ b/hw/grlib_gptimer.c
> @@ -0,0 +1,448 @@
> +/*
> + * QEMU GRLIB GPTimer Emulator
> + *
> + * Copyright (c) 2010 AdaCore
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "sysbus.h"
> +#include "qemu-timer.h"
> +
> +#include "grlib.h"
> +
> +/* #define DEBUG_TIMER */
> +
> +#ifdef DEBUG_TIMER
> +#define DPRINTF(fmt, ...)                                       \
> +    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...)
> +#endif
> +
> +#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
> +#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
> +
> +#define GPTIMER_MAX_TIMERS 8
> +
> +/* GPTimer Config register fields */
> +#define GPTIMER_ENABLE      (1 << 0)
> +#define GPTIMER_RESTART     (1 << 1)
> +#define GPTIMER_LOAD        (1 << 2)
> +#define GPTIMER_INT_ENABLE  (1 << 3)
> +#define GPTIMER_INT_PENDING (1 << 4)
> +#define GPTIMER_CHAIN       (1 << 5) /* Not supported */
> +#define GPTIMER_DEBUG_HALT  (1 << 6) /* Not supported */
> +
> +/* Memory mapped register offsets */
> +#define SCALER_OFFSET         0x00
> +#define SCALER_RELOAD_OFFSET  0x04
> +#define CONFIG_OFFSET         0x08
> +#define COUNTER_OFFSET        0x00
> +#define COUNTER_RELOAD_OFFSET 0x04
> +#define TIMER_BASE            0x10
> +
> +typedef struct GPTimer     GPTimer;
> +typedef struct GPTimerUnit GPTimerUnit;
> +
> +struct GPTimer
> +{
> +    QEMUBH *bh;
> +    struct ptimer_state *ptimer;
> +
> +    qemu_irq     irq;
> +    int          id;
> +    GPTimerUnit *unit;
> +
> +    /* registers */
> +    uint32_t counter;
> +    uint32_t reload;
> +    uint32_t config;
> +};
> +
> +struct GPTimerUnit
> +{
> +    SysBusDevice  busdev;
> +
> +    uint32_t nr_timers;         /* Number of timers available */
> +    uint32_t freq_hz;           /* System frequency */
> +    uint32_t irq_line;          /* Base irq line */
> +
> +    GPTimer *timers;
> +
> +    /* registers */
> +    uint32_t scaler;
> +    uint32_t reload;
> +    uint32_t config;
> +};
> +
> +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
> +                                  uint32_t            nr_timers,
> +                                  uint32_t            freq,
> +                                  qemu_irq           *cpu_irqs,
> +                                  int                 base_irq)
> +{
> +    DeviceState *dev;
> +    int i;
> +
> +    dev = qdev_create(NULL, "grlib,gptimer");
> +    qdev_prop_set_uint32(dev, "nr-timers", nr_timers);
> +    qdev_prop_set_uint32(dev, "frequency", freq);
> +    qdev_prop_set_uint32(dev, "irq-line", base_irq);
> +
> +    if (qdev_init(dev)) {
> +        return NULL;
> +    }
> +
> +    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
> +
> +    for (i = 0; i < nr_timers; i++)
> +        sysbus_connect_irq(sysbus_from_qdev(dev), i, cpu_irqs[base_irq + i]);
> +
> +    return dev;
> +}
> +
> +static void grlib_gptimer_enable(GPTimer *timer)
> +{
> +    assert(timer != NULL);
> +
> +    DPRINTF("%s id:%d\n", __func__, timer->id);
> +
> +    ptimer_stop(timer->ptimer);
> +
> +    if (!(timer->config & GPTIMER_ENABLE)) {
> +        /* Timer disabled */
> +        DPRINTF("%s id:%d Timer disabled (config 0x%x)\n", __func__,
> +                timer->id, timer->config);
> +        return;
> +    }
> +
> +    /* ptimer is triggered when the counter reach 0 but GPTimer is triggered at
> +       underflow. Set count + 1 to simulate the GPTimer behavior. */
> +
> +    DPRINTF("%s id:%d set count 0x%x and run\n",
> +            __func__,
> +            timer->id,
> +            timer->counter + 1);
> +
> +    ptimer_set_count(timer->ptimer, timer->counter + 1);
> +    ptimer_run(timer->ptimer, 1);
> +}
> +
> +static void grlib_gptimer_restart(GPTimer *timer)
> +{
> +    assert(timer != NULL);
> +
> +    DPRINTF("%s id:%d reload val: 0x%x\n", __func__, timer->id, timer->reload);
> +
> +    timer->counter = timer->reload;
> +    grlib_gptimer_enable(timer);
> +}
> +
> +static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
> +{
> +    int i = 0;
> +    uint32_t value = 0;
> +
> +    assert(unit != NULL);
> +
> +
> +    if (scaler > 0) {
> +        value = unit->freq_hz / (scaler + 1);
> +    } else {
> +        value = unit->freq_hz;
> +    }
> +
> +    DPRINTF("%s scaler:%d freq:0x%x\n", __func__, scaler, value);
> +
> +    for (i = 0; i < unit->nr_timers; i++) {
> +        ptimer_set_freq(unit->timers[i].ptimer, value);
> +    }
> +}
> +
> +static void grlib_gptimer_hit(void *opaque)
> +{
> +    GPTimer *timer = opaque;
> +    assert(timer != NULL);
> +
> +    DPRINTF("%s id:%d\n", __func__, timer->id);
> +
> +    /* Timer expired */
> +
> +    if (timer->config & GPTIMER_INT_ENABLE) {
> +        /* Set the pending bit (only unset by write in the config register) */
> +        timer->config &= GPTIMER_INT_PENDING;

Do you mean |= GPTIMER_INT_PENDING here?


> +        qemu_set_irq(timer->irq, 1);

Hmm, this interrupt logic doesn't seem right. You are never clearing
the timer->irq line?

Once you've got the INT_PENDING bit logic right (more on that later),
I suspect you simply want to connect the irq state to the interrupt
pending bit in the config reg. e.g:

qemu_set_irq(timer->irq, !!(timer->config & GPTIMER_INT_PENDING));

You'll need to update the timer->irq state after every piece of logic
that potentially changes the int-pending bit.


> +    }
> +
> +    if (timer->config & GPTIMER_RESTART) {
> +        grlib_gptimer_restart(timer);
> +    }
> +}
> +
> +static uint32_t grlib_gptimer_readl (void *opaque, target_phys_addr_t addr)
> +{
> +    GPTimerUnit *unit  = opaque;
> +    uint32_t     value = 0;
> +
> +    addr &= 0xff;
> +
> +    assert(unit != NULL);
> +
> +    /* Unit registers */
> +    switch (addr)
> +    {
> +        case SCALER_OFFSET:
> +            DPRINTF("%s scaler: 0x%x\n", __func__, unit->scaler);
> +            return unit->scaler;
> +
> +        case SCALER_RELOAD_OFFSET:
> +            DPRINTF("%s reload: 0x%x\n", __func__, unit->reload);
> +            return unit->reload;
> +
> +        case CONFIG_OFFSET:
> +            DPRINTF("%s unit config: 0x%x\n", __func__, unit->config);
> +            return unit->config;
> +
> +        default:
> +            break;
> +    }
> +
> +    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
> +    int                id         = (addr - TIMER_BASE) / TIMER_BASE;
> +
> +    if (id >= 0 && id < unit->nr_timers) {
> +
> +        /* GPTimer registers */
> +        switch (timer_addr)
> +        {
> +            case COUNTER_OFFSET:
> +                value = ptimer_get_count (unit->timers[id].ptimer);
> +                DPRINTF("%s counter value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                return value;
> +
> +            case COUNTER_RELOAD_OFFSET:
> +                value = unit->timers[id].reload;
> +                DPRINTF("%s reload value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                return value;
> +
> +            case CONFIG_OFFSET:
> +                DPRINTF("%s config for timer %d: 0x%x\n",
> +                        __func__, id, unit->timers[id].config);
> +                return unit->timers[id].config;
> +
> +            default:
> +                break;
> +        }
> +
> +    }
> +
> +    DPRINTF("read unknown register 0x%04x\n", (int)addr);
> +    return 0;
> +}
> +
> +static void
> +grlib_gptimer_writel (void *opaque, target_phys_addr_t addr, uint32_t value)
> +{
> +    GPTimerUnit *unit = opaque;
> +
> +    addr &= 0xff;
> +
> +    assert(unit != NULL);
> +
> +    /* Unit registers */
> +    switch (addr)
> +    {
> +        case SCALER_OFFSET:
> +            value &= 0xFFFF; /* clean up the value */
> +            unit->scaler = value;
> +            return;
> +
> +        case SCALER_RELOAD_OFFSET:
> +            value &= 0xFFFF; /* clean up the value */
> +            unit->reload = value;
> +            grlib_gptimer_set_scaler(unit, value);
> +            return;
> +
> +        case CONFIG_OFFSET:
> +            /* Read Only (disable timer freeze not supported) */
> +            return;
> +
> +        default:
> +            break;
> +    }
> +
> +    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
> +    int                id         = (addr - TIMER_BASE) / TIMER_BASE;
> +
> +    if (id >= 0 && id < unit->nr_timers) {
> +
> +        /* GPTimer registers */
> +        switch (timer_addr)
> +        {
> +            case COUNTER_OFFSET:
> +                DPRINTF("%s counter value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                unit->timers[id].counter = value;
> +                grlib_gptimer_enable(&unit->timers[id]);
> +                return;
> +
> +            case COUNTER_RELOAD_OFFSET:
> +                DPRINTF("%s reload value for timer %d: 0x%x\n",
> +                        __func__, id, value);
> +                unit->timers[id].reload = value;
> +                return;
> +
> +            case CONFIG_OFFSET:
> +                DPRINTF("%s config for timer %d: 0x%x\n", __func__, id, value);
> +
> +                unit->timers[id].config = value;

Writing a 1 to the interrupt-pending bit should clear it,
writing a zero should keep the IP bit untouched, e.g:

                   unit->timers[id].config &= GPTIMER_INT_PENDING;
                   unit->timers[id].config &= (~value) & GPTIMER_INT_PENDING;
                   unit->timers[id].config |= value & (~(GPTIMER_INT_PENDING | ... other magic bits...));


IIUC, writing to the config register and clearing the IP bit acks
the interrupt to the device. So it's a good time to update the
timer->irq state.


> +
> +                /* gptimer_restart calls gptimer_enable, so if "enable" and
> +                   "load" bits are present, we just have to call restart. */
> +
> +                if (value & GPTIMER_LOAD) {
> +                    grlib_gptimer_restart(&unit->timers[id]);
> +                } else if (value & GPTIMER_ENABLE) {
> +                    grlib_gptimer_enable(&unit->timers[id]);
> +                }
> +
> +                /* This fields must always be read as 0 */
> +                value &= ~(GPTIMER_LOAD & GPTIMER_DEBUG_HALT);
> +
> +                unit->timers[id].config = value;
> +                return;
> +
> +            default:
> +                break;
> +        }
> +
> +    }
> +
> +    DPRINTF("write unknown register 0x%04x\n", (int)addr);
> +}
> +
> +static CPUReadMemoryFunc *grlib_gptimer_read[] = {
> +    NULL, NULL, grlib_gptimer_readl,
> +};
> +
> +static CPUWriteMemoryFunc *grlib_gptimer_write[] = {
> +    NULL, NULL, grlib_gptimer_writel,
> +};
> +
> +static void grlib_gptimer_reset(void *opaque)
> +{
> +    /* int          i    = 0; */
> +    /* GPTimerUnit *unit = (GPTimerUnit *)opaque; */
> +    /* assert(unit != NULL); */
> +
> +    /* unit->scaler = 0; */
> +    /* unit->reload = 0; */
> +    /* unit->config = 0; */
> +
> +    /* unit->config  = unit->nr_timers; */
> +    /* unit->config |= unit->irq_line << 3; */
> +    /* unit->config |= 1 << 8;     /\* separate interrupt *\/ */
> +    /* unit->config |= 1 << 9;     /\* Disable timer freeze *\/ */
> +
> +
> +    /* for (i = 0; i < unit->nr_timers; i++) { */
> +    /*     GPTimer *timer = &unit->timers[i]; */
> +
> +    /*     timer->counter = 0; */
> +    /*     timer->reload = 0; */
> +    /*     timer->config = 0; */
> +    /*     ptimer_stop(timer->ptimer); */
> +    /*     ptimer_set_count(timer->ptimer, 0); */
> +    /*     ptimer_set_freq(timer->ptimer, unit->freq_hz); */
> +    /* } */
> +
> +}
> +
> +static int grlib_gptimer_init(SysBusDevice *dev)
> +{
> +    GPTimerUnit  *unit = FROM_SYSBUS(typeof (*unit), dev);
> +    unsigned int  i;
> +    int           timer_regs;
> +
> +    assert(unit->nr_timers > 0);
> +    assert(unit->nr_timers <= GPTIMER_MAX_TIMERS);
> +    unit->timers = qemu_mallocz(sizeof unit->timers[0] * unit->nr_timers);
> +
> +    for (i = 0; i < unit->nr_timers; i++) {
> +        GPTimer *timer = &unit->timers[i];
> +
> +        timer->unit   = unit;
> +        timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
> +        timer->ptimer = ptimer_init(timer->bh);
> +        timer->id     = i;
> +
> +        /* One IRQ line for each timer */
> +        sysbus_init_irq(dev, &timer->irq);
> +
> +        ptimer_set_freq(timer->ptimer, unit->freq_hz);
> +    }
> +
> +    qemu_register_reset(grlib_gptimer_reset, unit);
> +
> +    timer_regs = cpu_register_io_memory(grlib_gptimer_read,
> +                                        grlib_gptimer_write,
> +                                        unit);
> +    if (timer_regs < 0) {
> +        return -1;
> +    }
> +
> +    sysbus_init_mmio(dev, UNIT_REG_SIZE + GPTIMER_REG_SIZE * unit->nr_timers,
> +                     timer_regs);
> +    return 0;
> +}
> +
> +static SysBusDeviceInfo grlib_gptimer_info = {
> +    .init       = grlib_gptimer_init,
> +    .qdev.name  = "grlib,gptimer",
> +    .qdev.size  = sizeof(GPTimerUnit),
> +    .qdev.props = (Property[]) {
> +        {
> +            .name   = "frequency",
> +            .info   = &qdev_prop_uint32,
> +            .offset = offsetof(GPTimerUnit, freq_hz),
> +            .defval = (uint32_t[]) { 2 },
> +        },{
> +            .name   = "irq-line",
> +            .info   = &qdev_prop_uint32,
> +            .offset = offsetof(GPTimerUnit, irq_line),
> +            .defval = (uint32_t[]) { 8 },
> +        },{
> +            .name   = "nr-timers",
> +            .info   = &qdev_prop_uint32,
> +            .offset = offsetof(GPTimerUnit, nr_timers),
> +            .defval = (uint32_t[]) { 2 },
> +        },
> +        {/* end of list */}
> +    }
> +};
> +
> +static void grlib_gptimer_register(void)
> +{
> +    sysbus_register_withprop(&grlib_gptimer_info);
> +}
> +
> +device_init(grlib_gptimer_register)
> -- 
> 1.7.1
> 
>
Fabien Chouteau - Dec. 9, 2010, 10:04 a.m.
On 12/08/2010 11:51 PM, Edgar E. Iglesias wrote:
> On Mon, Dec 06, 2010 at 10:26:02AM +0100, Fabien Chouteau wrote:
>>
>> Signed-off-by: Fabien Chouteau<chouteau@adacore.com>
>> ---
>>   hw/grlib_gptimer.c |  448 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 448 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
>> new file mode 100644
>> index 0000000..41edbe4
>> --- /dev/null
>> +++ b/hw/grlib_gptimer.c
>> @@ -0,0 +1,448 @@
>> +/*
>> + * QEMU GRLIB GPTimer Emulator
>> + *
>> + * Copyright (c) 2010 AdaCore
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "sysbus.h"
>> +#include "qemu-timer.h"
>> +
>> +#include "grlib.h"
>> +
>> +/* #define DEBUG_TIMER */
>> +
>> +#ifdef DEBUG_TIMER
>> +#define DPRINTF(fmt, ...)                                       \
>> +    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...)
>> +#endif
>> +
>> +#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
>> +#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
>> +
>> +#define GPTIMER_MAX_TIMERS 8
>> +
>> +/* GPTimer Config register fields */
>> +#define GPTIMER_ENABLE      (1<<  0)
>> +#define GPTIMER_RESTART     (1<<  1)
>> +#define GPTIMER_LOAD        (1<<  2)
>> +#define GPTIMER_INT_ENABLE  (1<<  3)
>> +#define GPTIMER_INT_PENDING (1<<  4)
>> +#define GPTIMER_CHAIN       (1<<  5) /* Not supported */
>> +#define GPTIMER_DEBUG_HALT  (1<<  6) /* Not supported */
>> +
>> +/* Memory mapped register offsets */
>> +#define SCALER_OFFSET         0x00
>> +#define SCALER_RELOAD_OFFSET  0x04
>> +#define CONFIG_OFFSET         0x08
>> +#define COUNTER_OFFSET        0x00
>> +#define COUNTER_RELOAD_OFFSET 0x04
>> +#define TIMER_BASE            0x10
>> +
>> +typedef struct GPTimer     GPTimer;
>> +typedef struct GPTimerUnit GPTimerUnit;
>> +
>> +struct GPTimer
>> +{
>> +    QEMUBH *bh;
>> +    struct ptimer_state *ptimer;
>> +
>> +    qemu_irq     irq;
>> +    int          id;
>> +    GPTimerUnit *unit;
>> +
>> +    /* registers */
>> +    uint32_t counter;
>> +    uint32_t reload;
>> +    uint32_t config;
>> +};
>> +
>> +struct GPTimerUnit
>> +{
>> +    SysBusDevice  busdev;
>> +
>> +    uint32_t nr_timers;         /* Number of timers available */
>> +    uint32_t freq_hz;           /* System frequency */
>> +    uint32_t irq_line;          /* Base irq line */
>> +
>> +    GPTimer *timers;
>> +
>> +    /* registers */
>> +    uint32_t scaler;
>> +    uint32_t reload;
>> +    uint32_t config;
>> +};
>> +
>> +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
>> +                                  uint32_t            nr_timers,
>> +                                  uint32_t            freq,
>> +                                  qemu_irq           *cpu_irqs,
>> +                                  int                 base_irq)
>> +{
>> +    DeviceState *dev;
>> +    int i;
>> +
>> +    dev = qdev_create(NULL, "grlib,gptimer");
>> +    qdev_prop_set_uint32(dev, "nr-timers", nr_timers);
>> +    qdev_prop_set_uint32(dev, "frequency", freq);
>> +    qdev_prop_set_uint32(dev, "irq-line", base_irq);
>> +
>> +    if (qdev_init(dev)) {
>> +        return NULL;
>> +    }
>> +
>> +    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
>> +
>> +    for (i = 0; i<  nr_timers; i++)
>> +        sysbus_connect_irq(sysbus_from_qdev(dev), i, cpu_irqs[base_irq + i]);
>> +
>> +    return dev;
>> +}
>> +
>> +static void grlib_gptimer_enable(GPTimer *timer)
>> +{
>> +    assert(timer != NULL);
>> +
>> +    DPRINTF("%s id:%d\n", __func__, timer->id);
>> +
>> +    ptimer_stop(timer->ptimer);
>> +
>> +    if (!(timer->config&  GPTIMER_ENABLE)) {
>> +        /* Timer disabled */
>> +        DPRINTF("%s id:%d Timer disabled (config 0x%x)\n", __func__,
>> +                timer->id, timer->config);
>> +        return;
>> +    }
>> +
>> +    /* ptimer is triggered when the counter reach 0 but GPTimer is triggered at
>> +       underflow. Set count + 1 to simulate the GPTimer behavior. */
>> +
>> +    DPRINTF("%s id:%d set count 0x%x and run\n",
>> +            __func__,
>> +            timer->id,
>> +            timer->counter + 1);
>> +
>> +    ptimer_set_count(timer->ptimer, timer->counter + 1);
>> +    ptimer_run(timer->ptimer, 1);
>> +}
>> +
>> +static void grlib_gptimer_restart(GPTimer *timer)
>> +{
>> +    assert(timer != NULL);
>> +
>> +    DPRINTF("%s id:%d reload val: 0x%x\n", __func__, timer->id, timer->reload);
>> +
>> +    timer->counter = timer->reload;
>> +    grlib_gptimer_enable(timer);
>> +}
>> +
>> +static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
>> +{
>> +    int i = 0;
>> +    uint32_t value = 0;
>> +
>> +    assert(unit != NULL);
>> +
>> +
>> +    if (scaler>  0) {
>> +        value = unit->freq_hz / (scaler + 1);
>> +    } else {
>> +        value = unit->freq_hz;
>> +    }
>> +
>> +    DPRINTF("%s scaler:%d freq:0x%x\n", __func__, scaler, value);
>> +
>> +    for (i = 0; i<  unit->nr_timers; i++) {
>> +        ptimer_set_freq(unit->timers[i].ptimer, value);
>> +    }
>> +}
>> +
>> +static void grlib_gptimer_hit(void *opaque)
>> +{
>> +    GPTimer *timer = opaque;
>> +    assert(timer != NULL);
>> +
>> +    DPRINTF("%s id:%d\n", __func__, timer->id);
>> +
>> +    /* Timer expired */
>> +
>> +    if (timer->config&  GPTIMER_INT_ENABLE) {
>> +        /* Set the pending bit (only unset by write in the config register) */
>> +        timer->config&= GPTIMER_INT_PENDING;
>
> Do you mean |= GPTIMER_INT_PENDING here?

Yep, thanks.

>> +        qemu_set_irq(timer->irq, 1);
>
> Hmm, this interrupt logic doesn't seem right. You are never clearing
> the timer->irq line?
>
> Once you've got the INT_PENDING bit logic right (more on that later),
> I suspect you simply want to connect the irq state to the interrupt
> pending bit in the config reg. e.g:
>
> qemu_set_irq(timer->irq, !!(timer->config&  GPTIMER_INT_PENDING));
>
> You'll need to update the timer->irq state after every piece of logic
> that potentially changes the int-pending bit.
>

GrLib peripherals just throw interrupts and never expect any kind of
acknowledgment, that's why the interrupt pending bit has to be cleared by
software. IRQs are acknowledged by the CPU at the beginning of the trap
process.

The interrupt pending bit is only useful when all the timers use the 
same IRQ
line and you want to know which one expired. For instance, our software 
never
read or clear it at all.

> writing a zero should keep the IP bit untouched, e.g:
>
>                     unit->timers[id].config&= GPTIMER_INT_PENDING;
>                     unit->timers[id].config&= (~value)&  GPTIMER_INT_PENDING;
>                     unit->timers[id].config |= value&  (~(GPTIMER_INT_PENDING | ... other magic bits...));
>

Right, I've missed the "write 1 to clear" part, but I don't understand 
your code.
I prefer something clearer, like:

if (value & GPTIMER_INT_PENDING) {
     /* clear pending bit */
     value &= ~GPTIMER_INT_PENDING;
} else {
     /* keep pending bit */
     value |= unit->timers[id].config & GPTIMER_INT_PENDING;
}

Patch

diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
new file mode 100644
index 0000000..41edbe4
--- /dev/null
+++ b/hw/grlib_gptimer.c
@@ -0,0 +1,448 @@ 
+/*
+ * QEMU GRLIB GPTimer Emulator
+ *
+ * Copyright (c) 2010 AdaCore
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "sysbus.h"
+#include "qemu-timer.h"
+
+#include "grlib.h"
+
+/* #define DEBUG_TIMER */
+
+#ifdef DEBUG_TIMER
+#define DPRINTF(fmt, ...)                                       \
+    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...)
+#endif
+
+#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
+#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
+
+#define GPTIMER_MAX_TIMERS 8
+
+/* GPTimer Config register fields */
+#define GPTIMER_ENABLE      (1 << 0)
+#define GPTIMER_RESTART     (1 << 1)
+#define GPTIMER_LOAD        (1 << 2)
+#define GPTIMER_INT_ENABLE  (1 << 3)
+#define GPTIMER_INT_PENDING (1 << 4)
+#define GPTIMER_CHAIN       (1 << 5) /* Not supported */
+#define GPTIMER_DEBUG_HALT  (1 << 6) /* Not supported */
+
+/* Memory mapped register offsets */
+#define SCALER_OFFSET         0x00
+#define SCALER_RELOAD_OFFSET  0x04
+#define CONFIG_OFFSET         0x08
+#define COUNTER_OFFSET        0x00
+#define COUNTER_RELOAD_OFFSET 0x04
+#define TIMER_BASE            0x10
+
+typedef struct GPTimer     GPTimer;
+typedef struct GPTimerUnit GPTimerUnit;
+
+struct GPTimer
+{
+    QEMUBH *bh;
+    struct ptimer_state *ptimer;
+
+    qemu_irq     irq;
+    int          id;
+    GPTimerUnit *unit;
+
+    /* registers */
+    uint32_t counter;
+    uint32_t reload;
+    uint32_t config;
+};
+
+struct GPTimerUnit
+{
+    SysBusDevice  busdev;
+
+    uint32_t nr_timers;         /* Number of timers available */
+    uint32_t freq_hz;           /* System frequency */
+    uint32_t irq_line;          /* Base irq line */
+
+    GPTimer *timers;
+
+    /* registers */
+    uint32_t scaler;
+    uint32_t reload;
+    uint32_t config;
+};
+
+DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
+                                  uint32_t            nr_timers,
+                                  uint32_t            freq,
+                                  qemu_irq           *cpu_irqs,
+                                  int                 base_irq)
+{
+    DeviceState *dev;
+    int i;
+
+    dev = qdev_create(NULL, "grlib,gptimer");
+    qdev_prop_set_uint32(dev, "nr-timers", nr_timers);
+    qdev_prop_set_uint32(dev, "frequency", freq);
+    qdev_prop_set_uint32(dev, "irq-line", base_irq);
+
+    if (qdev_init(dev)) {
+        return NULL;
+    }
+
+    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
+
+    for (i = 0; i < nr_timers; i++)
+        sysbus_connect_irq(sysbus_from_qdev(dev), i, cpu_irqs[base_irq + i]);
+
+    return dev;
+}
+
+static void grlib_gptimer_enable(GPTimer *timer)
+{
+    assert(timer != NULL);
+
+    DPRINTF("%s id:%d\n", __func__, timer->id);
+
+    ptimer_stop(timer->ptimer);
+
+    if (!(timer->config & GPTIMER_ENABLE)) {
+        /* Timer disabled */
+        DPRINTF("%s id:%d Timer disabled (config 0x%x)\n", __func__,
+                timer->id, timer->config);
+        return;
+    }
+
+    /* ptimer is triggered when the counter reach 0 but GPTimer is triggered at
+       underflow. Set count + 1 to simulate the GPTimer behavior. */
+
+    DPRINTF("%s id:%d set count 0x%x and run\n",
+            __func__,
+            timer->id,
+            timer->counter + 1);
+
+    ptimer_set_count(timer->ptimer, timer->counter + 1);
+    ptimer_run(timer->ptimer, 1);
+}
+
+static void grlib_gptimer_restart(GPTimer *timer)
+{
+    assert(timer != NULL);
+
+    DPRINTF("%s id:%d reload val: 0x%x\n", __func__, timer->id, timer->reload);
+
+    timer->counter = timer->reload;
+    grlib_gptimer_enable(timer);
+}
+
+static void grlib_gptimer_set_scaler(GPTimerUnit *unit, uint32_t scaler)
+{
+    int i = 0;
+    uint32_t value = 0;
+
+    assert(unit != NULL);
+
+
+    if (scaler > 0) {
+        value = unit->freq_hz / (scaler + 1);
+    } else {
+        value = unit->freq_hz;
+    }
+
+    DPRINTF("%s scaler:%d freq:0x%x\n", __func__, scaler, value);
+
+    for (i = 0; i < unit->nr_timers; i++) {
+        ptimer_set_freq(unit->timers[i].ptimer, value);
+    }
+}
+
+static void grlib_gptimer_hit(void *opaque)
+{
+    GPTimer *timer = opaque;
+    assert(timer != NULL);
+
+    DPRINTF("%s id:%d\n", __func__, timer->id);
+
+    /* Timer expired */
+
+    if (timer->config & GPTIMER_INT_ENABLE) {
+        /* Set the pending bit (only unset by write in the config register) */
+        timer->config &= GPTIMER_INT_PENDING;
+        qemu_set_irq(timer->irq, 1);
+    }
+
+    if (timer->config & GPTIMER_RESTART) {
+        grlib_gptimer_restart(timer);
+    }
+}
+
+static uint32_t grlib_gptimer_readl (void *opaque, target_phys_addr_t addr)
+{
+    GPTimerUnit *unit  = opaque;
+    uint32_t     value = 0;
+
+    addr &= 0xff;
+
+    assert(unit != NULL);
+
+    /* Unit registers */
+    switch (addr)
+    {
+        case SCALER_OFFSET:
+            DPRINTF("%s scaler: 0x%x\n", __func__, unit->scaler);
+            return unit->scaler;
+
+        case SCALER_RELOAD_OFFSET:
+            DPRINTF("%s reload: 0x%x\n", __func__, unit->reload);
+            return unit->reload;
+
+        case CONFIG_OFFSET:
+            DPRINTF("%s unit config: 0x%x\n", __func__, unit->config);
+            return unit->config;
+
+        default:
+            break;
+    }
+
+    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
+    int                id         = (addr - TIMER_BASE) / TIMER_BASE;
+
+    if (id >= 0 && id < unit->nr_timers) {
+
+        /* GPTimer registers */
+        switch (timer_addr)
+        {
+            case COUNTER_OFFSET:
+                value = ptimer_get_count (unit->timers[id].ptimer);
+                DPRINTF("%s counter value for timer %d: 0x%x\n",
+                        __func__, id, value);
+                return value;
+
+            case COUNTER_RELOAD_OFFSET:
+                value = unit->timers[id].reload;
+                DPRINTF("%s reload value for timer %d: 0x%x\n",
+                        __func__, id, value);
+                return value;
+
+            case CONFIG_OFFSET:
+                DPRINTF("%s config for timer %d: 0x%x\n",
+                        __func__, id, unit->timers[id].config);
+                return unit->timers[id].config;
+
+            default:
+                break;
+        }
+
+    }
+
+    DPRINTF("read unknown register 0x%04x\n", (int)addr);
+    return 0;
+}
+
+static void
+grlib_gptimer_writel (void *opaque, target_phys_addr_t addr, uint32_t value)
+{
+    GPTimerUnit *unit = opaque;
+
+    addr &= 0xff;
+
+    assert(unit != NULL);
+
+    /* Unit registers */
+    switch (addr)
+    {
+        case SCALER_OFFSET:
+            value &= 0xFFFF; /* clean up the value */
+            unit->scaler = value;
+            return;
+
+        case SCALER_RELOAD_OFFSET:
+            value &= 0xFFFF; /* clean up the value */
+            unit->reload = value;
+            grlib_gptimer_set_scaler(unit, value);
+            return;
+
+        case CONFIG_OFFSET:
+            /* Read Only (disable timer freeze not supported) */
+            return;
+
+        default:
+            break;
+    }
+
+    target_phys_addr_t timer_addr = (addr % TIMER_BASE);
+    int                id         = (addr - TIMER_BASE) / TIMER_BASE;
+
+    if (id >= 0 && id < unit->nr_timers) {
+
+        /* GPTimer registers */
+        switch (timer_addr)
+        {
+            case COUNTER_OFFSET:
+                DPRINTF("%s counter value for timer %d: 0x%x\n",
+                        __func__, id, value);
+                unit->timers[id].counter = value;
+                grlib_gptimer_enable(&unit->timers[id]);
+                return;
+
+            case COUNTER_RELOAD_OFFSET:
+                DPRINTF("%s reload value for timer %d: 0x%x\n",
+                        __func__, id, value);
+                unit->timers[id].reload = value;
+                return;
+
+            case CONFIG_OFFSET:
+                DPRINTF("%s config for timer %d: 0x%x\n", __func__, id, value);
+
+                unit->timers[id].config = value;
+
+                /* gptimer_restart calls gptimer_enable, so if "enable" and
+                   "load" bits are present, we just have to call restart. */
+
+                if (value & GPTIMER_LOAD) {
+                    grlib_gptimer_restart(&unit->timers[id]);
+                } else if (value & GPTIMER_ENABLE) {
+                    grlib_gptimer_enable(&unit->timers[id]);
+                }
+
+                /* This fields must always be read as 0 */
+                value &= ~(GPTIMER_LOAD & GPTIMER_DEBUG_HALT);
+
+                unit->timers[id].config = value;
+                return;
+
+            default:
+                break;
+        }
+
+    }
+
+    DPRINTF("write unknown register 0x%04x\n", (int)addr);
+}
+
+static CPUReadMemoryFunc *grlib_gptimer_read[] = {
+    NULL, NULL, grlib_gptimer_readl,
+};
+
+static CPUWriteMemoryFunc *grlib_gptimer_write[] = {
+    NULL, NULL, grlib_gptimer_writel,
+};
+
+static void grlib_gptimer_reset(void *opaque)
+{
+    /* int          i    = 0; */
+    /* GPTimerUnit *unit = (GPTimerUnit *)opaque; */
+    /* assert(unit != NULL); */
+
+    /* unit->scaler = 0; */
+    /* unit->reload = 0; */
+    /* unit->config = 0; */
+
+    /* unit->config  = unit->nr_timers; */
+    /* unit->config |= unit->irq_line << 3; */
+    /* unit->config |= 1 << 8;     /\* separate interrupt *\/ */
+    /* unit->config |= 1 << 9;     /\* Disable timer freeze *\/ */
+
+
+    /* for (i = 0; i < unit->nr_timers; i++) { */
+    /*     GPTimer *timer = &unit->timers[i]; */
+
+    /*     timer->counter = 0; */
+    /*     timer->reload = 0; */
+    /*     timer->config = 0; */
+    /*     ptimer_stop(timer->ptimer); */
+    /*     ptimer_set_count(timer->ptimer, 0); */
+    /*     ptimer_set_freq(timer->ptimer, unit->freq_hz); */
+    /* } */
+
+}
+
+static int grlib_gptimer_init(SysBusDevice *dev)
+{
+    GPTimerUnit  *unit = FROM_SYSBUS(typeof (*unit), dev);
+    unsigned int  i;
+    int           timer_regs;
+
+    assert(unit->nr_timers > 0);
+    assert(unit->nr_timers <= GPTIMER_MAX_TIMERS);
+    unit->timers = qemu_mallocz(sizeof unit->timers[0] * unit->nr_timers);
+
+    for (i = 0; i < unit->nr_timers; i++) {
+        GPTimer *timer = &unit->timers[i];
+
+        timer->unit   = unit;
+        timer->bh     = qemu_bh_new(grlib_gptimer_hit, timer);
+        timer->ptimer = ptimer_init(timer->bh);
+        timer->id     = i;
+
+        /* One IRQ line for each timer */
+        sysbus_init_irq(dev, &timer->irq);
+
+        ptimer_set_freq(timer->ptimer, unit->freq_hz);
+    }
+
+    qemu_register_reset(grlib_gptimer_reset, unit);
+
+    timer_regs = cpu_register_io_memory(grlib_gptimer_read,
+                                        grlib_gptimer_write,
+                                        unit);
+    if (timer_regs < 0) {
+        return -1;
+    }
+
+    sysbus_init_mmio(dev, UNIT_REG_SIZE + GPTIMER_REG_SIZE * unit->nr_timers,
+                     timer_regs);
+    return 0;
+}
+
+static SysBusDeviceInfo grlib_gptimer_info = {
+    .init       = grlib_gptimer_init,
+    .qdev.name  = "grlib,gptimer",
+    .qdev.size  = sizeof(GPTimerUnit),
+    .qdev.props = (Property[]) {
+        {
+            .name   = "frequency",
+            .info   = &qdev_prop_uint32,
+            .offset = offsetof(GPTimerUnit, freq_hz),
+            .defval = (uint32_t[]) { 2 },
+        },{
+            .name   = "irq-line",
+            .info   = &qdev_prop_uint32,
+            .offset = offsetof(GPTimerUnit, irq_line),
+            .defval = (uint32_t[]) { 8 },
+        },{
+            .name   = "nr-timers",
+            .info   = &qdev_prop_uint32,
+            .offset = offsetof(GPTimerUnit, nr_timers),
+            .defval = (uint32_t[]) { 2 },
+        },
+        {/* end of list */}
+    }
+};
+
+static void grlib_gptimer_register(void)
+{
+    sysbus_register_withprop(&grlib_gptimer_info);
+}
+
+device_init(grlib_gptimer_register)