From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46647 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PQFj7-0004NU-9f for qemu-devel@nongnu.org; Wed, 08 Dec 2010 03:50:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PQFj5-0001Bi-TM for qemu-devel@nongnu.org; Wed, 08 Dec 2010 03:50:17 -0500 Received: from mail-bw0-f45.google.com ([209.85.214.45]:40705) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PQFj5-0001BZ-HP for qemu-devel@nongnu.org; Wed, 08 Dec 2010 03:50:15 -0500 Received: by bwz16 with SMTP id 16so1044257bwz.4 for ; Wed, 08 Dec 2010 00:50:14 -0800 (PST) Date: Wed, 8 Dec 2010 09:30:05 +0100 From: "Edgar E. Iglesias" Subject: Re: [Qemu-devel] [PATCH 1/6] [RFC] Emulation of GRLIB GPTimer as defined in GRLIB IP Core User's Manual. Message-ID: <20101208083005.GA6845@edde.se.axis.com> References: <4CFE0495.6060906@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4CFE0495.6060906@adacore.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau Cc: Blue Swirl , qemu-devel@nongnu.org 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 wrote: > >> > >> Signed-off-by: Fabien Chouteau > >> --- > >> 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