From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=32953 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PQGV1-0000Ix-1r for qemu-devel@nongnu.org; Wed, 08 Dec 2010 04:39:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PQGUz-0002RS-I6 for qemu-devel@nongnu.org; Wed, 08 Dec 2010 04:39:46 -0500 Received: from mel.act-europe.fr ([194.98.77.210]:43755) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PQGUz-0002RE-9Y for qemu-devel@nongnu.org; Wed, 08 Dec 2010 04:39:45 -0500 Message-ID: <4CFF525F.2040105@adacore.com> Date: Wed, 08 Dec 2010 10:39:43 +0100 From: Fabien Chouteau MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 1/6] [RFC] Emulation of GRLIB GPTimer as defined in GRLIB IP Core User's Manual. References: <4CFE0495.6060906@adacore.com> <20101208083005.GA6845@edde.se.axis.com> In-Reply-To: <20101208083005.GA6845@edde.se.axis.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: Blue Swirl , qemu-devel@nongnu.org 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 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). Do you mean like Xilinx devices where the instantiators are in-lined functions in hw/xilinx.h? -- Fabien Chouteau