From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53557 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PPuHG-00043D-Ar for qemu-devel@nongnu.org; Tue, 07 Dec 2010 04:56:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PPuGx-0005Ni-Pt for qemu-devel@nongnu.org; Tue, 07 Dec 2010 04:56:06 -0500 Received: from mel.act-europe.fr ([194.98.77.210]:59036) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PPuGx-0005Kv-9a for qemu-devel@nongnu.org; Tue, 07 Dec 2010 04:55:47 -0500 Message-ID: <4CFE0495.6060906@adacore.com> Date: Tue, 07 Dec 2010 10:55:33 +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: In-Reply-To: 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: Blue Swirl Cc: qemu-devel@nongnu.org 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. >> +{ >> + 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. -- Fabien Chouteau