Re: [Qemu-devel] [PATCH 1/6] [RFC] Emulation of GRLIB GPTimer as defined in GRLIB IP Core User's Manual.

[Edgar E. Iglesias]

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<address@hidden>  wrote:
> >>
> >> Signed-off-by: Fabien Chouteau<address@hidden>
> >> ---
> >>   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