From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=57469 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PPv0o-0006xR-KA for qemu-devel@nongnu.org; Tue, 07 Dec 2010 05:43:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PPv0m-0002L4-5s for qemu-devel@nongnu.org; Tue, 07 Dec 2010 05:43:10 -0500 Received: from mel.act-europe.fr ([194.98.77.210]:36421) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PPv0l-0002Kk-QF for qemu-devel@nongnu.org; Tue, 07 Dec 2010 05:43:08 -0500 Message-ID: <4CFE0FB9.5080000@adacore.com> Date: Tue, 07 Dec 2010 11:43:05 +0100 From: Fabien Chouteau MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/6] [RFC] Emulation of GRLIB IRQMP 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:25 PM, Blue Swirl wrote: > On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau wrote: >> >> Signed-off-by: Fabien Chouteau >> --- >> hw/grlib_irqmp.c | 416 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 416 insertions(+), 0 deletions(-) >> >> diff --git a/hw/grlib_irqmp.c b/hw/grlib_irqmp.c >> new file mode 100644 >> index 0000000..69e1553 >> --- /dev/null >> +++ b/hw/grlib_irqmp.c >> @@ -0,0 +1,416 @@ >> +/* >> + * QEMU GRLIB IRQMP Emulator >> + * >> + * (Multiprocessor and extended interrupt not supported) >> + * >> + * 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 "cpu.h" >> + >> +#include "grlib.h" >> + >> +/* #define DEBUG_IRQ */ >> + >> +#ifdef DEBUG_IRQ >> +#define DPRINTF(fmt, ...) \ >> + do { printf("IRQMP: " fmt , ## __VA_ARGS__); } while (0) >> +#else >> +#define DPRINTF(fmt, ...) >> +#endif >> + >> +#define IRQMP_MAX_CPU 16 >> +#define IRQMP_REG_SIZE 256 /* Size of memory mapped registers */ >> + >> +/* Memory mapped register offsets */ >> +#define LEVEL_OFFSET 0x00 >> +#define PENDING_OFFSET 0x04 >> +#define FORCE0_OFFSET 0x08 >> +#define CLEAR_OFFSET 0x0C >> +#define MP_STATUS_OFFSET 0x10 >> +#define BROADCAST_OFFSET 0x14 >> +#define MASK_OFFSET 0x40 >> +#define FORCE_OFFSET 0x80 >> +#define EXTENDED_OFFSET 0xC0 >> + >> +typedef struct IRQMP >> +{ >> + SysBusDevice busdev; >> + >> + CPUSPARCState *env; > > Devices should never access CPUState directly. Instead, board level > should create CPU irqs and these should then be passed here. > This case is special, Leon3 is a System-On-Chip and some of the components are very close to the processor. IRQMP is not really a peripheral nor a part of the CPU, it's both... >> +} IRQMP; >> + >> +typedef struct IRQMPState >> +{ >> + uint32_t level; >> + uint32_t pending; >> + uint32_t clear; >> + uint32_t broadcast; >> + >> + uint32_t mask[IRQMP_MAX_CPU]; >> + uint32_t force[IRQMP_MAX_CPU]; >> + uint32_t extended[IRQMP_MAX_CPU]; >> + >> + IRQMP *parent; >> +} IRQMPState; >> + >> +IRQMPState grlib_irqmp_state; > > Global state indicates poor design. Why separate IRQMP and IRQMPState? I have to access IRQMPState in grlib_irqmp_ack and grlib_irqmp_check_irqs, but I don't see how I can do it without a global variable. Again, I think that it's related to the special case of IRQMP. >> + >> +void grlib_irqmp_set_irq(void *opaque, int irq, int level); > > This should not be global. Again, creating qemu_irqs or moving some of > the code to board level should help. This one should be static indeed. >> + >> +DeviceState *grlib_irqmp_create(target_phys_addr_t base, >> + CPUState *env, >> + qemu_irq **cpu_irqs, >> + uint32_t nr_irqs) >> +{ >> + DeviceState *dev; >> + >> + assert(cpu_irqs != NULL); >> + >> + dev = qdev_create(NULL, "grlib,irqmp"); >> + qdev_prop_set_ptr(dev, "cpustate", env); >> + >> + if (qdev_init(dev)) { >> + return NULL; >> + } >> + >> + sysbus_mmio_map(sysbus_from_qdev(dev), 0, base); >> + >> + *cpu_irqs = qemu_allocate_irqs(grlib_irqmp_set_irq, >> +&grlib_irqmp_state, >> + nr_irqs); >> + >> + return dev; >> +} >> + >> +static void grlib_irqmp_check_irqs(CPUState *env) >> +{ >> + uint32_t pend = 0; >> + uint32_t level0 = 0; >> + uint32_t level1 = 0; >> + >> + assert(env != NULL); >> + >> + /* IRQ for CPU 0 (no SMP support) */ >> + pend = (grlib_irqmp_state.pending | grlib_irqmp_state.force[0]) >> +& grlib_irqmp_state.mask[0]; >> + >> + >> + level0 = pend& ~grlib_irqmp_state.level; >> + level1 = pend& grlib_irqmp_state.level; >> + >> + DPRINTF("pend:0x%04x force:0x%04x mask:0x%04x lvl1:0x%04x lvl0:0x%04x\n", >> + grlib_irqmp_state.pending, grlib_irqmp_state.force[0], >> + grlib_irqmp_state.mask[0], level1, level0); > > The above should stay here, but code below should to go to board level > (leon3.c). Then you need to separate device IRQ handling from CPU PIL > handling. If I want to use IRQMP for another machine I will have to duplicate the code. So I think it is the right place for this this code. >> + >> + /* Trigger level1 interrupt first and level0 if there is no level1 */ >> + if (level1 != 0) { >> + env->pil_in = level1; >> + } else { >> + env->pil_in = level0; >> + } >> + >> + if (env->pil_in&& (env->interrupt_index == 0 || >> + (env->interrupt_index& ~15) == TT_EXTINT)) { >> + unsigned int i; >> + >> + for (i = 15; i> 0; i--) { >> + if (env->pil_in& (1<< i)) { >> + int old_interrupt = env->interrupt_index; >> + >> + env->interrupt_index = TT_EXTINT | i; >> + if (old_interrupt != env->interrupt_index) { >> + DPRINTF("Set CPU IRQ %d\n", i); >> + cpu_interrupt(env, CPU_INTERRUPT_HARD); >> + } >> + break; >> + } >> + } >> + } else if (!env->pil_in&& (env->interrupt_index& ~15) == TT_EXTINT) { >> + DPRINTF("Reset CPU IRQ %d\n", env->interrupt_index& 15); >> + env->interrupt_index = 0; >> + cpu_reset_interrupt(env, CPU_INTERRUPT_HARD); >> + } >> +} >> + >> +void grlib_irqmp_ack(CPUSPARCState *env, int intno) >> +{ >> + assert(env != NULL); >> + >> + uint32_t mask; >> + >> + intno&= 15; >> + mask = 1<< intno; >> + >> + DPRINTF("grlib_irqmp_ack %d\n", intno); >> + >> + /* Clear registers */ >> + grlib_irqmp_state.pending&= ~mask; >> + grlib_irqmp_state.force[0]&= ~mask; /* Only CPU 0 (No SMP support) */ >> + >> + grlib_irqmp_check_irqs(env); >> +} >> + >> +void grlib_irqmp_set_irq(void *opaque, int irq, int level) >> +{ >> + IRQMPState *s = opaque; >> + int i = 0; >> + >> + assert(opaque != NULL); >> + assert(s->parent != NULL); >> + >> + if (level) { >> + DPRINTF("Raise CPU IRQ %d\n", irq); >> + >> + if (s->broadcast& 1<< irq) { >> + /* Broadcasted IRQ */ >> + for (i = 0; i< IRQMP_MAX_CPU; i++) { >> + s->force[i] |= 1<< irq; >> + } >> + } else { >> + s->pending |= 1<< irq; >> + } >> + grlib_irqmp_check_irqs(s->parent->env); >> + >> + } else { >> + >> + DPRINTF("Lower CPU IRQ %d\n", irq); >> + if (s->broadcast& 1<< irq) { >> + /* Broadcasted IRQ */ >> + for (i = 0; i< IRQMP_MAX_CPU; i++) { >> + s->force[i]&= ~(1<< irq); >> + } >> + } else { >> + s->pending&= ~(1<< irq); >> + } >> + grlib_irqmp_check_irqs(s->parent->env); >> + } >> +} >> + >> +static uint32_t grlib_irqmp_readl (void *opaque, target_phys_addr_t addr) >> +{ >> + IRQMP *irqmp = opaque; >> + >> + assert(irqmp != NULL); >> + >> + addr&= 0xff; >> + >> + /* global registers */ >> + switch (addr) >> + { >> + case LEVEL_OFFSET: >> + return grlib_irqmp_state.level; >> + >> + case PENDING_OFFSET: >> + return grlib_irqmp_state.pending; >> + >> + case FORCE0_OFFSET: >> + /* This register is an "alias" for the force register of CPU 0 */ >> + return grlib_irqmp_state.force[0]; >> + >> + case CLEAR_OFFSET: >> + case MP_STATUS_OFFSET: >> + /* Always read as 0 */ >> + return 0; >> + >> + case BROADCAST_OFFSET: >> + return grlib_irqmp_state.broadcast; >> + >> + default: >> + break; >> + } >> + >> + /* mask registers */ >> + if (addr>= MASK_OFFSET&& addr< FORCE_OFFSET) { >> + int cpu = (addr - MASK_OFFSET) / 4; >> + assert(cpu>= 0&& cpu< IRQMP_MAX_CPU); >> + >> + return grlib_irqmp_state.mask[cpu] ; >> + } >> + >> + /* force registers */ >> + if (addr>= FORCE_OFFSET&& addr< EXTENDED_OFFSET) { >> + int cpu = (addr - FORCE_OFFSET) / 4; >> + assert(cpu>= 0&& cpu< IRQMP_MAX_CPU); >> + >> + return grlib_irqmp_state.force[cpu]; >> + } >> + >> + /* extended (not supported) */ >> + if (addr>= EXTENDED_OFFSET&& addr< IRQMP_REG_SIZE) { >> + int cpu = (addr - EXTENDED_OFFSET) / 4; >> + assert(cpu>= 0&& cpu< IRQMP_MAX_CPU); >> + >> + return grlib_irqmp_state.extended[cpu]; >> + } >> + >> + DPRINTF("read unknown register 0x%04x\n", (int)addr); >> + return 0; >> +} >> + >> +static void >> +grlib_irqmp_writel (void *opaque, target_phys_addr_t addr, uint32_t value) >> +{ >> + IRQMP *irqmp = opaque; >> + >> + assert(irqmp != NULL); >> + >> + addr&= 0xff; >> + >> + /* global registers */ >> + switch (addr) >> + { >> + case LEVEL_OFFSET: >> + value&= 0xFFFF<< 1; /* clean up the value */ >> + grlib_irqmp_state.level = value; >> + return; >> + >> + case PENDING_OFFSET: >> + /* Read Only */ >> + return; >> + >> + case FORCE0_OFFSET: >> + /* This register is an "alias" for the force register of CPU 0 */ >> + >> + value&= 0xFFFE; /* clean up the value */ >> + grlib_irqmp_state.force[0] = value; >> + grlib_irqmp_check_irqs(irqmp->env); >> + return; >> + >> + case CLEAR_OFFSET: >> + value&= ~1; /* clean up the value */ >> + grlib_irqmp_state.pending&= ~value; >> + return; >> + >> + case MP_STATUS_OFFSET: >> + /* Read Only (no SMP support) */ >> + return; >> + >> + case BROADCAST_OFFSET: >> + value&= 0xFFFE; /* clean up the value */ >> + grlib_irqmp_state.broadcast = value; >> + return; >> + >> + default: >> + break; >> + } >> + >> + /* mask registers */ >> + if (addr>= MASK_OFFSET&& addr< FORCE_OFFSET) { >> + int cpu = (addr - MASK_OFFSET) / 4; >> + assert(cpu>= 0&& cpu< IRQMP_MAX_CPU); >> + >> + value&= ~1; /* clean up the value */ >> + grlib_irqmp_state.mask[cpu] = value; >> + grlib_irqmp_check_irqs(irqmp->env); >> + return; >> + } >> + >> + /* force registers */ >> + if (addr>= FORCE_OFFSET&& addr< EXTENDED_OFFSET) { >> + int cpu = (addr - FORCE_OFFSET) / 4; >> + assert(cpu>= 0&& cpu< IRQMP_MAX_CPU); >> + >> + uint32_t force = value& 0xFFFE; >> + uint32_t clear = (value>> 16)& 0xFFFE; >> + uint32_t old = grlib_irqmp_state.force[cpu]; >> + >> + grlib_irqmp_state.force[cpu] = (old | force)& ~clear; >> + grlib_irqmp_check_irqs(irqmp->env); >> + return; >> + } >> + >> + /* extended (not supported) */ >> + if (addr>= EXTENDED_OFFSET&& addr< IRQMP_REG_SIZE) { >> + int cpu = (addr - EXTENDED_OFFSET) / 4; >> + assert(cpu>= 0&& cpu< IRQMP_MAX_CPU); >> + >> + value&= 0xF; /* clean up the value */ >> + grlib_irqmp_state.extended[cpu] = value; >> + return; >> + } >> + >> + DPRINTF("write unknown register 0x%04x\n", (int)addr); > > Please use TARGET_FMT_plx, so the cast can be removed. OK, I didn't know about that. -- Fabien Chouteau