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<chout...@adacore.com> > > wrote: > >> > >> Signed-off-by: Fabien Chouteau<chout...@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