| Message ID | C6ECFE15BF5B3D4B8DE8EEC71CCB8A29212CC26B@sestoex09.enea.se |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 12, 2012 at 11:49 AM, Daniel Forsgren <daniel.forsgren@enea.com> wrote: > This patch adds basic support for the "architected" timers (i.e. cp15) > found in A15. It's enough to allow Linux to boot, using arch_timer for > the tick. However - it is not a complete model of the timer block at > large, it is not that well structured, and it is currently tested with > qemu-linaro-1.1.50-2012.07 (not latest and greatest). It's simply a > prototype. > > However, if anyone wants to play with the architectured (cp15) timers > instead of sp804, then please feel free to try it out. It has been > tested with linux-linaro-3.6-rc2-2012.08, and you can easily verify > the existence of these timers under /proc/interrupts: > > root@linaro-developer:~# cat /proc/interrupts > cat /proc/interrupts > CPU0 > 29: 7424 GIC arch_timer > 30: 0 GIC arch_timer > > Please note that this also requires some minor fixes that are not part > of qemu-linaro-1.1.50-2012.07: > > http://patches.linaro.org/9833/ > > Signed-off-by: Daniel Forsgren <daniel.forsgren@enea.com> > > --- > > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c qemu-linaro-1.1.50-2012.07-modified/hw/a15mpcore.c > --- qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c 2012-07-05 16:48:28.000000000 +0200 > +++ qemu-linaro-1.1.50-2012.07-modified/hw/a15mpcore.c 2012-09-12 11:24:25.844237405 +0200 > @@ -28,6 +28,7 @@ typedef struct A15MPPrivState { > uint32_t num_cpu; > uint32_t num_irq; > MemoryRegion container; > + DeviceState *archtimer; > DeviceState *gic; > } A15MPPrivState; > > @@ -40,7 +41,8 @@ static void a15mp_priv_set_irq(void *opa > static int a15mp_priv_init(SysBusDevice *dev) > { > A15MPPrivState *s = FROM_SYSBUS(A15MPPrivState, dev); > - SysBusDevice *busdev; > + SysBusDevice *busdev, *timerbusdev; > + int i; > > if (kvm_irqchip_in_kernel()) { > s->gic = qdev_create(NULL, "kvm-arm_gic"); > @@ -60,6 +62,11 @@ static int a15mp_priv_init(SysBusDevice > /* Pass through inbound GPIO lines to the GIC */ > qdev_init_gpio_in(&s->busdev.qdev, a15mp_priv_set_irq, s->num_irq - 32); > > + s->archtimer = qdev_create(NULL, "arm_archtimer"); > + // qdev_prop_set_uint32(s->archtimer, "num-cpu", s->num_cpu); Please don't introduce dead code. > + qdev_init_nofail(s->archtimer); > + timerbusdev = sysbus_from_qdev(s->archtimer); > + > /* Memory map (addresses are offsets from PERIPHBASE): > * 0x0000-0x0fff -- reserved > * 0x1000-0x1fff -- GIC Distributor > @@ -75,6 +82,16 @@ static int a15mp_priv_init(SysBusDevice > sysbus_mmio_get_region(busdev, 1)); > > sysbus_init_mmio(dev, &s->container); > + > + > + for (i = 0; i < s->num_cpu; i++) { > + int ppibase = (s->num_irq - 32) + i * 32; > + sysbus_connect_irq(timerbusdev, i * 2, > + qdev_get_gpio_in(s->gic, ppibase + 29)); > + sysbus_connect_irq(timerbusdev, i * 2 + 1, > + qdev_get_gpio_in(s->gic, ppibase + 30)); > + } > + > return 0; > } > > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs qemu-linaro-1.1.50-2012.07-modified/hw/arm/Makefile.objs > --- qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs 2012-07-05 16:48:28.000000000 +0200 > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm/Makefile.objs 2012-09-12 11:28:39.121053287 +0200 > @@ -1,4 +1,4 @@ > -obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o > +obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o arm_archtimer.o > obj-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o pl190.o > obj-y += versatile_pci.o > obj-y += versatile_i2c.o > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c qemu-linaro-1.1.50-2012.07-modified/hw/arm_archtimer.c > --- qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c 1970-01-01 01:00:00.000000000 +0100 > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm_archtimer.c 2012-09-12 13:21:44.676267111 +0200 > @@ -0,0 +1,232 @@ > +/* > + * "Architectured" timer for A15 > + * > + * Copyright (c) 2012 Enea Software AB > + * Written by Daniel Forsgren > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include "sysbus.h" > +#include "qemu-timer.h" > + > +/* Primitive (and imcomplete) support for A15 "architectured" timers incomplete > + > + - Only PL1 timer supported. > + > + - Only minimal subset of fuctionality requred by Linux. functionality required > + > + - Only tested with single-core. > + > +*/ > + > +/* control register bit assignment */ > +#define CTL_ENABLE 0x01 > +#define CTL_MASK 0x02 > +#define CTL_INT 0x04 > + > +/* state of per-core timers */ > +typedef struct { > + ARMCPU *cpu; /* who are we serving */ > + QEMUTimer *pl1_timer; > + QEMUTimer *pl2_timer; > + qemu_irq pl1_irq; > + qemu_irq pl2_irq; > + uint32_t cntkctl; /* timer pl1 control register */ > + uint32_t cntp_ctl; /* pl1 physical timer control register */ > + uint64_t cntvoff; /* virtual offset register */ > +} archtimers; ArchTimers, please see CODING_STYLE. > + > +#define MAX_CPUS 4 > + > +typedef struct { > + SysBusDevice busdev; > + archtimers archtimers[MAX_CPUS]; > +} arm_archtimer_state; ARMArchTimerState > + > + > +static int a15_cntfrq_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) > +{ > + /* Let's assume that we're running at 1GHz for now. It's not > + correct, but it simplifies translation between cycles <-> ns */ > + *value = 1000000000UL; > + return 0; > +} > + > +static int a15_cntkctl_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) > +{ > + archtimers *at = (archtimers *)(ri->opaque); If ri->opaque is void pointer, the cast is useless in C. Also other similar cases. > + *value = at->cntkctl; > + return 0; > +} > + > +static int a15_cntpct_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) > +{ > + /* Let's assume that the physical count register is driven by > + vm_clock for now. As we have specified that that we're running > + at 1GHz, no translation from ns should be needed. */ > + *value = qemu_get_clock_ns(vm_clock); > + return 0; > +} > + > +static int a15_cntvct_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) > +{ > + archtimers *at = (archtimers *)(ri->opaque); > + > + /* Virtual count should subtract the virtual offfset from physical > + count? */ > + *value = qemu_get_clock_ns(vm_clock) - at->cntvoff; > + return 0; > +} > + > +static int a15_cntp_tval_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) > +{ > + archtimers *at = (archtimers *)(ri->opaque); > + *value = qemu_timer_expire_time_ns(at->pl1_timer); > + return 0; > +} > + > +static int a15_cntp_tval_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) > +{ > + archtimers *at = (archtimers *)(ri->opaque); > + > + /* I assume that setting a new value means that we should clear > + any previous? */ > + qemu_set_irq(at->pl1_irq, 0); > + at->cntp_ctl &= ~CTL_INT; > + > + qemu_mod_timer_ns(at->pl1_timer, qemu_get_clock_ns(vm_clock) + value); > + > + return 0; > +} > + > +static int a15_cntp_ctl_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) > +{ > + archtimers *at = (archtimers *)(ri->opaque); > + > + *value = at->cntp_ctl; > + > + return 0; > +} > + > +static int a15_cntp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri, > + uint64_t value) > +{ > + archtimers *at = (archtimers *)(ri->opaque); > + > + /* Copy "enable" and "mask" bits from the new value. Preserve the rest. */ > + at->cntp_ctl = (at->cntp_ctl & ~(CTL_ENABLE | CTL_MASK)) | (value & (CTL_ENABLE | CTL_MASK)); > + > + /* If no interrupt is asserted, or interrupt is masked, then lower the irq. */ > + if (!(at->cntp_ctl & CTL_INT) || (at->cntp_ctl & CTL_MASK)) > + qemu_set_irq(at->pl1_irq, 0); Missing braces, please read CODING_STYLE. Please fix also other cases below. > + > + /* If interrupt is asserted and not masked, then raise the irq. */ > + if ((at->cntp_ctl & CTL_INT) && !(at->cntp_ctl & CTL_MASK)) > + qemu_set_irq(at->pl1_irq, 1); > + > + return 0; > +} > + > +static const ARMCPRegInfo archtimer_cp_reginfo[] = { > + > + { .name = "CNTFRQ", .cp = 15, .crn = 14, .crm = 0, .opc1 = 0, .opc2 = 0, > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntfrq_read, }, > + > + { .name = "CNTKCTL", .cp = 15, .crn = 14, .crm = 1, .opc1 = 0, .opc2 = 0, > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntkctl_read, }, > + > + { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0, > + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_tval_read, > + .writefn = a15_cntp_tval_write, }, > + > + { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1, > + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_ctl_read, > + .writefn = a15_cntp_ctl_write, }, > + > + { .name = "CNTPCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, .opc1 = 0, .opc2 = 0, > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntpct_read, }, > + > + { .name = "CNTVCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, .opc1 = 1, .opc2 = 0, > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntvct_read, }, > + > + REGINFO_SENTINEL > +}; > + > +static void pl1_timer_cb(void *opaque) > +{ > + archtimers *at = (archtimers *)opaque; Here the cast is definitely useless, please remove. > + > + /* Set the interrupt bit in control register */ > + at->cntp_ctl |= CTL_INT; > + > + /* If not masked, then raise the irq */ > + if (!(at->cntp_ctl & CTL_MASK)) > + qemu_set_irq(at->pl1_irq, 1); > +} > + > +static int arm_archtimer_init(SysBusDevice *dev) > +{ > + arm_archtimer_state *s = FROM_SYSBUS(arm_archtimer_state, dev); > + CPUArchState *cpu; > + int i; > + > + for (cpu = first_cpu, i = 0; cpu; cpu = cpu->next_cpu, i++) { > + archtimers *at = &s->archtimers[i]; > + at->pl1_timer = qemu_new_timer_ns(vm_clock, &pl1_timer_cb, at); > + sysbus_init_irq(dev, &(at->pl1_irq)); > + sysbus_init_irq(dev, &(at->pl2_irq)); > + s->archtimers[i].cpu = arm_env_get_cpu(cpu); > + define_arm_cp_regs_with_opaque(s->archtimers[i].cpu, archtimer_cp_reginfo, (void *)at); > + } > + > + return 0; > +} > + > +static void arm_archtimer_reset(DeviceState *dev) > +{ > + arm_archtimer_state *s = > + FROM_SYSBUS(arm_archtimer_state, sysbus_from_qdev(dev)); > + int i; > + > + for (i = 0; i < MAX_CPUS; i++) { > + if (s->archtimers[i].pl1_timer) > + qemu_del_timer(s->archtimers[i].pl1_timer); > + if (s->archtimers[i].pl2_timer) > + qemu_del_timer(s->archtimers[i].pl2_timer); > + } > +} > + > +static void arm_archtimer_class_init(ObjectClass *class, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(class); > + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); > + > + sbc->init = arm_archtimer_init; > + dc->reset = arm_archtimer_reset; > +} > + > +static TypeInfo arm_archtimer_info = { > + .name = "arm_archtimer", > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(arm_archtimer_state), > + .class_init = arm_archtimer_class_init, > +}; > + > +static void arm_archtimer_register_types(void) > +{ > + type_register_static(&arm_archtimer_info); > +} > + > +type_init(arm_archtimer_register_types) > diff -Nupr qemu-linaro-1.1.50-2012.07/target-arm/helper.c qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c > --- qemu-linaro-1.1.50-2012.07/target-arm/helper.c 2012-07-05 16:48:28.000000000 +0200 > +++ qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c 2012-09-12 13:15:45.544424842 +0200 > @@ -1012,9 +1012,11 @@ void register_cp_regs_for_features(ARMCP > if (arm_feature(env, ARM_FEATURE_THUMB2EE)) { > define_arm_cp_regs(cpu, t2ee_cp_reginfo); > } > + /* Why would this be needed? > if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { > define_arm_cp_regs(cpu, generic_timer_cp_reginfo); > } > + */ > if (arm_feature(env, ARM_FEATURE_VAPA)) { > define_arm_cp_regs(cpu, vapa_cp_reginfo); > } > >
On 14 September 2012 18:26, Blue Swirl <blauwirbel@gmail.com> wrote: > On Wed, Sep 12, 2012 at 11:49 AM, Daniel Forsgren >> +++ qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c 2012-09-12 13:15:45.544424842 +0200 >> @@ -1012,9 +1012,11 @@ void register_cp_regs_for_features(ARMCP >> if (arm_feature(env, ARM_FEATURE_THUMB2EE)) { >> define_arm_cp_regs(cpu, t2ee_cp_reginfo); >> } >> + /* > > Why would this be needed? > >> if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { >> define_arm_cp_regs(cpu, generic_timer_cp_reginfo); >> } >> + */ At the moment helper.c defines a set of registers which RAZ/WI the entire crn=14 space where the generic timer lives (I forget why, probably because Linux probes for it); this bit of code is installing those dummy registers. The correct way to add the feature this patch is adding is to replace that dummy definition with the proper one, not merely to comment out this bit of code which installs the dummies. How exactly we should model this kind of device which is really an integral part of the CPU core (ie it is a set of cp15 registers, not memory mapped IO) but still has interrupt lines that connect into the GIC) is an interesting question. "This is a sysbus device" is even more dubious than for most of our sysbus devices :-) -- PMM
Thanks for the feedback! I should probably point out (as I wrote in my initial mail) that this is just a prototype - a quick n dirty hack to get Linux up and running with the arch timers. It is very true that I'm not following the QEMU coding standard (I must admit that haven't even read it). The background is that I wanted to run QEMU and the A15 CoreTile side by side with as similar configuration as possible. And the missing A15 timers was kind of stopping me, so I had to work around that. (For that reason, I tried to keep most of my additions in a single file and not to clutter the entire source tree). At the same time I saw that someone asked for these timers on the mailing list some month ago. So I thought that I could as well share my results. That said, I'm very grateful that you still took the time to actually review the code, and I will try to improve it. I have fixed some minor issues that prevented me to run multicore so far. (My eventual goal is to run as close as possible to the real 2xA15+3xA7 CoreTile that I try to mimic). However, being a QEMU newbie I have a couple of questions related to the right way of implementing this: 1) What is considered to be part of the "core" and what is considered to be a device external to the core? To me, it looks like co-processor functionality in general is considered to be part of the core (implemented in target-arm/helper.c or similar), whereas timer devices in general are kept in hw/arm_* (c.f. arm_timer.c and arm_mptimer.c). But in this case I have a timer that is implemented as a coprocessor - where should that go? Or should it be split in two places? 2) Where should a device like this save its own internal state? Some other devices seems to save its state as an extension of the SysBusDevice structure, but coprocessor state in general rather seems to be part of CPUARMState or similar. What is the right way in this particular case? br, /D > -----Original Message----- > From: Blue Swirl [mailto:blauwirbel@gmail.com] > Sent: den 14 september 2012 19:26 > To: Daniel Forsgren > Cc: qemu-devel@nongnu.org > Subject: Re: [Qemu-devel] [PATCH] Basic support for ARM A15 "architectured" > (cp15) timers > > On Wed, Sep 12, 2012 at 11:49 AM, Daniel Forsgren > <daniel.forsgren@enea.com> wrote: > > This patch adds basic support for the "architected" timers (i.e. cp15) > > found in A15. It's enough to allow Linux to boot, using arch_timer for > > the tick. However - it is not a complete model of the timer block at > > large, it is not that well structured, and it is currently tested with > > qemu-linaro-1.1.50-2012.07 (not latest and greatest). It's simply a > > prototype. > > > > However, if anyone wants to play with the architectured (cp15) timers > > instead of sp804, then please feel free to try it out. It has been > > tested with linux-linaro-3.6-rc2-2012.08, and you can easily verify > > the existence of these timers under /proc/interrupts: > > > > root@linaro-developer:~# cat /proc/interrupts > > cat /proc/interrupts > > CPU0 > > 29: 7424 GIC arch_timer > > 30: 0 GIC arch_timer > > > > Please note that this also requires some minor fixes that are not part > > of qemu-linaro-1.1.50-2012.07: > > > > http://patches.linaro.org/9833/ > > > > Signed-off-by: Daniel Forsgren <daniel.forsgren@enea.com> > > > > --- > > > > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c qemu-linaro-1.1.50- > 2012.07-modified/hw/a15mpcore.c > > --- qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c 2012-07-05 > 16:48:28.000000000 +0200 > > +++ qemu-linaro-1.1.50-2012.07-modified/hw/a15mpcore.c 2012-09-12 > > +++ 11:24:25.844237405 +0200 > > @@ -28,6 +28,7 @@ typedef struct A15MPPrivState { > > uint32_t num_cpu; > > uint32_t num_irq; > > MemoryRegion container; > > + DeviceState *archtimer; > > DeviceState *gic; > > } A15MPPrivState; > > > > @@ -40,7 +41,8 @@ static void a15mp_priv_set_irq(void *opa static int > > a15mp_priv_init(SysBusDevice *dev) { > > A15MPPrivState *s = FROM_SYSBUS(A15MPPrivState, dev); > > - SysBusDevice *busdev; > > + SysBusDevice *busdev, *timerbusdev; > > + int i; > > > > if (kvm_irqchip_in_kernel()) { > > s->gic = qdev_create(NULL, "kvm-arm_gic"); @@ -60,6 +62,11 @@ > > static int a15mp_priv_init(SysBusDevice > > /* Pass through inbound GPIO lines to the GIC */ > > qdev_init_gpio_in(&s->busdev.qdev, a15mp_priv_set_irq, s->num_irq > > - 32); > > > > + s->archtimer = qdev_create(NULL, "arm_archtimer"); > > + // qdev_prop_set_uint32(s->archtimer, "num-cpu", s->num_cpu); > > Please don't introduce dead code. > > > + qdev_init_nofail(s->archtimer); > > + timerbusdev = sysbus_from_qdev(s->archtimer); > > + > > /* Memory map (addresses are offsets from PERIPHBASE): > > * 0x0000-0x0fff -- reserved > > * 0x1000-0x1fff -- GIC Distributor @@ -75,6 +82,16 @@ static > > int a15mp_priv_init(SysBusDevice > > sysbus_mmio_get_region(busdev, 1)); > > > > sysbus_init_mmio(dev, &s->container); > > + > > + > > + for (i = 0; i < s->num_cpu; i++) { > > + int ppibase = (s->num_irq - 32) + i * 32; > > + sysbus_connect_irq(timerbusdev, i * 2, > > + qdev_get_gpio_in(s->gic, ppibase + 29)); > > + sysbus_connect_irq(timerbusdev, i * 2 + 1, > > + qdev_get_gpio_in(s->gic, ppibase + 30)); > > + } > > + > > return 0; > > } > > > > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs qemu-linaro- > 1.1.50-2012.07-modified/hw/arm/Makefile.objs > > --- qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs 2012-07-05 > 16:48:28.000000000 +0200 > > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm/Makefile.objs 2012-09- > 12 11:28:39.121053287 +0200 > > @@ -1,4 +1,4 @@ > > -obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o > > +obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o > > +arm_archtimer.o > > obj-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o > > pl190.o obj-y += versatile_pci.o obj-y += versatile_i2c.o diff -Nupr > > qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c qemu-linaro-1.1.50- > 2012.07-modified/hw/arm_archtimer.c > > --- qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c 1970-01-01 > 01:00:00.000000000 +0100 > > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm_archtimer.c 2012-09- > 12 13:21:44.676267111 +0200 > > @@ -0,0 +1,232 @@ > > +/* > > + * "Architectured" timer for A15 > > + * > > + * Copyright (c) 2012 Enea Software AB > > + * Written by Daniel Forsgren > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; either version > > + * 2 of the License, or (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > +along > > + * with this program; if not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#include "sysbus.h" > > +#include "qemu-timer.h" > > + > > +/* Primitive (and imcomplete) support for A15 "architectured" timers > > incomplete > > > + > > + - Only PL1 timer supported. > > + > > + - Only minimal subset of fuctionality requred by Linux. > > functionality required > > > + > > + - Only tested with single-core. > > + > > +*/ > > + > > +/* control register bit assignment */ #define CTL_ENABLE 0x01 > > +#define CTL_MASK 0x02 > > +#define CTL_INT 0x04 > > + > > +/* state of per-core timers */ > > +typedef struct { > > + ARMCPU *cpu; /* who are we serving */ > > + QEMUTimer *pl1_timer; > > + QEMUTimer *pl2_timer; > > + qemu_irq pl1_irq; > > + qemu_irq pl2_irq; > > + uint32_t cntkctl; /* timer pl1 control register */ > > + uint32_t cntp_ctl; /* pl1 physical timer control register */ > > + uint64_t cntvoff; /* virtual offset register */ } archtimers; > > ArchTimers, please see CODING_STYLE. > > > + > > +#define MAX_CPUS 4 > > + > > +typedef struct { > > + SysBusDevice busdev; > > + archtimers archtimers[MAX_CPUS]; > > +} arm_archtimer_state; > > ARMArchTimerState > > > + > > + > > +static int a15_cntfrq_read(CPUARMState *env, const ARMCPRegInfo *ri, > > +uint64_t *value) { > > + /* Let's assume that we're running at 1GHz for now. It's not > > + correct, but it simplifies translation between cycles <-> ns */ > > + *value = 1000000000UL; > > + return 0; > > +} > > + > > +static int a15_cntkctl_read(CPUARMState *env, const ARMCPRegInfo *ri, > > +uint64_t *value) { > > + archtimers *at = (archtimers *)(ri->opaque); > > If ri->opaque is void pointer, the cast is useless in C. Also other similar cases. > > > + *value = at->cntkctl; > > + return 0; > > +} > > + > > +static int a15_cntpct_read(CPUARMState *env, const ARMCPRegInfo *ri, > > +uint64_t *value) { > > + /* Let's assume that the physical count register is driven by > > + vm_clock for now. As we have specified that that we're running > > + at 1GHz, no translation from ns should be needed. */ > > + *value = qemu_get_clock_ns(vm_clock); > > + return 0; > > +} > > + > > +static int a15_cntvct_read(CPUARMState *env, const ARMCPRegInfo *ri, > > +uint64_t *value) { > > + archtimers *at = (archtimers *)(ri->opaque); > > + > > + /* Virtual count should subtract the virtual offfset from physical > > + count? */ > > + *value = qemu_get_clock_ns(vm_clock) - at->cntvoff; > > + return 0; > > +} > > + > > +static int a15_cntp_tval_read(CPUARMState *env, const ARMCPRegInfo > > +*ri, uint64_t *value) { > > + archtimers *at = (archtimers *)(ri->opaque); > > + *value = qemu_timer_expire_time_ns(at->pl1_timer); > > + return 0; > > +} > > + > > +static int a15_cntp_tval_write(CPUARMState *env, const ARMCPRegInfo > > +*ri, uint64_t value) { > > + archtimers *at = (archtimers *)(ri->opaque); > > + > > + /* I assume that setting a new value means that we should clear > > + any previous? */ > > + qemu_set_irq(at->pl1_irq, 0); > > + at->cntp_ctl &= ~CTL_INT; > > + > > + qemu_mod_timer_ns(at->pl1_timer, qemu_get_clock_ns(vm_clock) + > > + value); > > + > > + return 0; > > +} > > + > > +static int a15_cntp_ctl_read(CPUARMState *env, const ARMCPRegInfo > > +*ri, uint64_t *value) { > > + archtimers *at = (archtimers *)(ri->opaque); > > + > > + *value = at->cntp_ctl; > > + > > + return 0; > > +} > > + > > +static int a15_cntp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri, > > + uint64_t value) { > > + archtimers *at = (archtimers *)(ri->opaque); > > + > > + /* Copy "enable" and "mask" bits from the new value. Preserve the rest. > */ > > + at->cntp_ctl = (at->cntp_ctl & ~(CTL_ENABLE | CTL_MASK)) | (value > > + & (CTL_ENABLE | CTL_MASK)); > > + > > + /* If no interrupt is asserted, or interrupt is masked, then lower the irq. */ > > + if (!(at->cntp_ctl & CTL_INT) || (at->cntp_ctl & CTL_MASK)) > > + qemu_set_irq(at->pl1_irq, 0); > > Missing braces, please read CODING_STYLE. Please fix also other cases below. > > > + > > + /* If interrupt is asserted and not masked, then raise the irq. */ > > + if ((at->cntp_ctl & CTL_INT) && !(at->cntp_ctl & CTL_MASK)) > > + qemu_set_irq(at->pl1_irq, 1); > > + > > + return 0; > > +} > > + > > +static const ARMCPRegInfo archtimer_cp_reginfo[] = { > > + > > + { .name = "CNTFRQ", .cp = 15, .crn = 14, .crm = 0, .opc1 = 0, .opc2 = 0, > > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntfrq_read, }, > > + > > + { .name = "CNTKCTL", .cp = 15, .crn = 14, .crm = 1, .opc1 = 0, .opc2 = 0, > > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntkctl_read, > > + }, > > + > > + { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0, > > + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_tval_read, > > + .writefn = a15_cntp_tval_write, }, > > + > > + { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1, > > + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_ctl_read, > > + .writefn = a15_cntp_ctl_write, }, > > + > > + { .name = "CNTPCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, > .opc1 = 0, .opc2 = 0, > > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntpct_read, }, > > + > > + { .name = "CNTVCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, > .opc1 = 1, .opc2 = 0, > > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntvct_read, }, > > + > > + REGINFO_SENTINEL > > +}; > > + > > +static void pl1_timer_cb(void *opaque) { > > + archtimers *at = (archtimers *)opaque; > > Here the cast is definitely useless, please remove. > > > + > > + /* Set the interrupt bit in control register */ > > + at->cntp_ctl |= CTL_INT; > > + > > + /* If not masked, then raise the irq */ > > + if (!(at->cntp_ctl & CTL_MASK)) > > + qemu_set_irq(at->pl1_irq, 1); } > > + > > +static int arm_archtimer_init(SysBusDevice *dev) { > > + arm_archtimer_state *s = FROM_SYSBUS(arm_archtimer_state, dev); > > + CPUArchState *cpu; > > + int i; > > + > > + for (cpu = first_cpu, i = 0; cpu; cpu = cpu->next_cpu, i++) { > > + archtimers *at = &s->archtimers[i]; > > + at->pl1_timer = qemu_new_timer_ns(vm_clock, &pl1_timer_cb, at); > > + sysbus_init_irq(dev, &(at->pl1_irq)); > > + sysbus_init_irq(dev, &(at->pl2_irq)); > > + s->archtimers[i].cpu = arm_env_get_cpu(cpu); > > + define_arm_cp_regs_with_opaque(s->archtimers[i].cpu, > archtimer_cp_reginfo, (void *)at); > > + } > > + > > + return 0; > > +} > > + > > +static void arm_archtimer_reset(DeviceState *dev) { > > + arm_archtimer_state *s = > > + FROM_SYSBUS(arm_archtimer_state, sysbus_from_qdev(dev)); > > + int i; > > + > > + for (i = 0; i < MAX_CPUS; i++) { > > + if (s->archtimers[i].pl1_timer) > > + qemu_del_timer(s->archtimers[i].pl1_timer); > > + if (s->archtimers[i].pl2_timer) > > + qemu_del_timer(s->archtimers[i].pl2_timer); > > + } > > +} > > + > > +static void arm_archtimer_class_init(ObjectClass *class, void *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(class); > > + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); > > + > > + sbc->init = arm_archtimer_init; > > + dc->reset = arm_archtimer_reset; > > +} > > + > > +static TypeInfo arm_archtimer_info = { > > + .name = "arm_archtimer", > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_size = sizeof(arm_archtimer_state), > > + .class_init = arm_archtimer_class_init, > > +}; > > + > > +static void arm_archtimer_register_types(void) > > +{ > > + type_register_static(&arm_archtimer_info); > > +} > > + > > +type_init(arm_archtimer_register_types) > > diff -Nupr qemu-linaro-1.1.50-2012.07/target-arm/helper.c qemu-linaro- > 1.1.50-2012.07-modified/target-arm/helper.c > > --- qemu-linaro-1.1.50-2012.07/target-arm/helper.c 2012-07-05 > 16:48:28.000000000 +0200 > > +++ qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c 2012-09-12 > 13:15:45.544424842 +0200 > > @@ -1012,9 +1012,11 @@ void register_cp_regs_for_features(ARMCP > > if (arm_feature(env, ARM_FEATURE_THUMB2EE)) { > > define_arm_cp_regs(cpu, t2ee_cp_reginfo); > > } > > + /* > > Why would this be needed? > > > if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { > > define_arm_cp_regs(cpu, generic_timer_cp_reginfo); > > } > > + */ > > if (arm_feature(env, ARM_FEATURE_VAPA)) { > > define_arm_cp_regs(cpu, vapa_cp_reginfo); > > } > > > >
On Sat, Sep 15, 2012 at 8:57 AM, Daniel Forsgren <daniel.forsgren@enea.com> wrote: > Thanks for the feedback! > > I should probably point out (as I wrote in my initial mail) that this is just a prototype - a quick n dirty hack to get Linux up and running with the arch timers. It is very true that I'm not following the QEMU coding standard (I must admit that haven't even read it). > > The background is that I wanted to run QEMU and the A15 CoreTile side by side with as similar configuration as possible. And the missing A15 timers was kind of stopping me, so I had to work around that. (For that reason, I tried to keep most of my additions in a single file and not to clutter the entire source tree). At the same time I saw that someone asked for these timers on the mailing list some month ago. So I thought that I could as well share my results. > > That said, I'm very grateful that you still took the time to actually review the code, and I will try to improve it. I have fixed some minor issues that prevented me to run multicore so far. (My eventual goal is to run as close as possible to the real 2xA15+3xA7 CoreTile that I try to mimic). > > However, being a QEMU newbie I have a couple of questions related to the right way of implementing this: > > 1) What is considered to be part of the "core" and what is considered to be a device external to the core? To me, it looks like co-processor functionality in general is considered to be part of the core (implemented in target-arm/helper.c or similar), whereas timer devices in general are kept in hw/arm_* (c.f. arm_timer.c and arm_mptimer.c). But in this case I have a timer that is implemented as a coprocessor - where should that go? Or should it be split in two places? SoC devices attached to the CPU is a bit grey area. In this case, I think coprocessor should be part of the CPU. Peter? > > 2) Where should a device like this save its own internal state? Some other devices seems to save its state as an extension of the SysBusDevice structure, but coprocessor state in general rather seems to be part of CPUARMState or similar. What is the right way in this particular case? Currently the divisive line seems to be that devices which are only accessible via MMIO or generic IO instructions should be external to CPU. But it could be possible to introduce generic methods to register other classes, for example for the ARM coprocessors, x86 model specific registers, PPC SPRs and Sparc ASIs. The memory API should support adding more address spaces. Maybe this could be a nice approach. But I'd vote for CPUARMState for now. > > br, > > /D > >> -----Original Message----- >> From: Blue Swirl [mailto:blauwirbel@gmail.com] >> Sent: den 14 september 2012 19:26 >> To: Daniel Forsgren >> Cc: qemu-devel@nongnu.org >> Subject: Re: [Qemu-devel] [PATCH] Basic support for ARM A15 "architectured" >> (cp15) timers >> >> On Wed, Sep 12, 2012 at 11:49 AM, Daniel Forsgren >> <daniel.forsgren@enea.com> wrote: >> > This patch adds basic support for the "architected" timers (i.e. cp15) >> > found in A15. It's enough to allow Linux to boot, using arch_timer for >> > the tick. However - it is not a complete model of the timer block at >> > large, it is not that well structured, and it is currently tested with >> > qemu-linaro-1.1.50-2012.07 (not latest and greatest). It's simply a >> > prototype. >> > >> > However, if anyone wants to play with the architectured (cp15) timers >> > instead of sp804, then please feel free to try it out. It has been >> > tested with linux-linaro-3.6-rc2-2012.08, and you can easily verify >> > the existence of these timers under /proc/interrupts: >> > >> > root@linaro-developer:~# cat /proc/interrupts >> > cat /proc/interrupts >> > CPU0 >> > 29: 7424 GIC arch_timer >> > 30: 0 GIC arch_timer >> > >> > Please note that this also requires some minor fixes that are not part >> > of qemu-linaro-1.1.50-2012.07: >> > >> > http://patches.linaro.org/9833/ >> > >> > Signed-off-by: Daniel Forsgren <daniel.forsgren@enea.com> >> > >> > --- >> > >> > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c qemu-linaro-1.1.50- >> 2012.07-modified/hw/a15mpcore.c >> > --- qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c 2012-07-05 >> 16:48:28.000000000 +0200 >> > +++ qemu-linaro-1.1.50-2012.07-modified/hw/a15mpcore.c 2012-09-12 >> > +++ 11:24:25.844237405 +0200 >> > @@ -28,6 +28,7 @@ typedef struct A15MPPrivState { >> > uint32_t num_cpu; >> > uint32_t num_irq; >> > MemoryRegion container; >> > + DeviceState *archtimer; >> > DeviceState *gic; >> > } A15MPPrivState; >> > >> > @@ -40,7 +41,8 @@ static void a15mp_priv_set_irq(void *opa static int >> > a15mp_priv_init(SysBusDevice *dev) { >> > A15MPPrivState *s = FROM_SYSBUS(A15MPPrivState, dev); >> > - SysBusDevice *busdev; >> > + SysBusDevice *busdev, *timerbusdev; >> > + int i; >> > >> > if (kvm_irqchip_in_kernel()) { >> > s->gic = qdev_create(NULL, "kvm-arm_gic"); @@ -60,6 +62,11 @@ >> > static int a15mp_priv_init(SysBusDevice >> > /* Pass through inbound GPIO lines to the GIC */ >> > qdev_init_gpio_in(&s->busdev.qdev, a15mp_priv_set_irq, s->num_irq >> > - 32); >> > >> > + s->archtimer = qdev_create(NULL, "arm_archtimer"); >> > + // qdev_prop_set_uint32(s->archtimer, "num-cpu", s->num_cpu); >> >> Please don't introduce dead code. >> >> > + qdev_init_nofail(s->archtimer); >> > + timerbusdev = sysbus_from_qdev(s->archtimer); >> > + >> > /* Memory map (addresses are offsets from PERIPHBASE): >> > * 0x0000-0x0fff -- reserved >> > * 0x1000-0x1fff -- GIC Distributor @@ -75,6 +82,16 @@ static >> > int a15mp_priv_init(SysBusDevice >> > sysbus_mmio_get_region(busdev, 1)); >> > >> > sysbus_init_mmio(dev, &s->container); >> > + >> > + >> > + for (i = 0; i < s->num_cpu; i++) { >> > + int ppibase = (s->num_irq - 32) + i * 32; >> > + sysbus_connect_irq(timerbusdev, i * 2, >> > + qdev_get_gpio_in(s->gic, ppibase + 29)); >> > + sysbus_connect_irq(timerbusdev, i * 2 + 1, >> > + qdev_get_gpio_in(s->gic, ppibase + 30)); >> > + } >> > + >> > return 0; >> > } >> > >> > diff -Nupr qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs qemu-linaro- >> 1.1.50-2012.07-modified/hw/arm/Makefile.objs >> > --- qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs 2012-07-05 >> 16:48:28.000000000 +0200 >> > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm/Makefile.objs 2012-09- >> 12 11:28:39.121053287 +0200 >> > @@ -1,4 +1,4 @@ >> > -obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o >> > +obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o >> > +arm_archtimer.o >> > obj-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o >> > pl190.o obj-y += versatile_pci.o obj-y += versatile_i2c.o diff -Nupr >> > qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c qemu-linaro-1.1.50- >> 2012.07-modified/hw/arm_archtimer.c >> > --- qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c 1970-01-01 >> 01:00:00.000000000 +0100 >> > +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm_archtimer.c 2012-09- >> 12 13:21:44.676267111 +0200 >> > @@ -0,0 +1,232 @@ >> > +/* >> > + * "Architectured" timer for A15 >> > + * >> > + * Copyright (c) 2012 Enea Software AB >> > + * Written by Daniel Forsgren >> > + * >> > + * This program is free software; you can redistribute it and/or >> > + * modify it under the terms of the GNU General Public License >> > + * as published by the Free Software Foundation; either version >> > + * 2 of the License, or (at your option) any later version. >> > + * >> > + * This program is distributed in the hope that it will be useful, >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > + * GNU General Public License for more details. >> > + * >> > + * You should have received a copy of the GNU General Public License >> > +along >> > + * with this program; if not, see <http://www.gnu.org/licenses/>. >> > + */ >> > + >> > +#include "sysbus.h" >> > +#include "qemu-timer.h" >> > + >> > +/* Primitive (and imcomplete) support for A15 "architectured" timers >> >> incomplete >> >> > + >> > + - Only PL1 timer supported. >> > + >> > + - Only minimal subset of fuctionality requred by Linux. >> >> functionality required >> >> > + >> > + - Only tested with single-core. >> > + >> > +*/ >> > + >> > +/* control register bit assignment */ #define CTL_ENABLE 0x01 >> > +#define CTL_MASK 0x02 >> > +#define CTL_INT 0x04 >> > + >> > +/* state of per-core timers */ >> > +typedef struct { >> > + ARMCPU *cpu; /* who are we serving */ >> > + QEMUTimer *pl1_timer; >> > + QEMUTimer *pl2_timer; >> > + qemu_irq pl1_irq; >> > + qemu_irq pl2_irq; >> > + uint32_t cntkctl; /* timer pl1 control register */ >> > + uint32_t cntp_ctl; /* pl1 physical timer control register */ >> > + uint64_t cntvoff; /* virtual offset register */ } archtimers; >> >> ArchTimers, please see CODING_STYLE. >> >> > + >> > +#define MAX_CPUS 4 >> > + >> > +typedef struct { >> > + SysBusDevice busdev; >> > + archtimers archtimers[MAX_CPUS]; >> > +} arm_archtimer_state; >> >> ARMArchTimerState >> >> > + >> > + >> > +static int a15_cntfrq_read(CPUARMState *env, const ARMCPRegInfo *ri, >> > +uint64_t *value) { >> > + /* Let's assume that we're running at 1GHz for now. It's not >> > + correct, but it simplifies translation between cycles <-> ns */ >> > + *value = 1000000000UL; >> > + return 0; >> > +} >> > + >> > +static int a15_cntkctl_read(CPUARMState *env, const ARMCPRegInfo *ri, >> > +uint64_t *value) { >> > + archtimers *at = (archtimers *)(ri->opaque); >> >> If ri->opaque is void pointer, the cast is useless in C. Also other similar cases. >> >> > + *value = at->cntkctl; >> > + return 0; >> > +} >> > + >> > +static int a15_cntpct_read(CPUARMState *env, const ARMCPRegInfo *ri, >> > +uint64_t *value) { >> > + /* Let's assume that the physical count register is driven by >> > + vm_clock for now. As we have specified that that we're running >> > + at 1GHz, no translation from ns should be needed. */ >> > + *value = qemu_get_clock_ns(vm_clock); >> > + return 0; >> > +} >> > + >> > +static int a15_cntvct_read(CPUARMState *env, const ARMCPRegInfo *ri, >> > +uint64_t *value) { >> > + archtimers *at = (archtimers *)(ri->opaque); >> > + >> > + /* Virtual count should subtract the virtual offfset from physical >> > + count? */ >> > + *value = qemu_get_clock_ns(vm_clock) - at->cntvoff; >> > + return 0; >> > +} >> > + >> > +static int a15_cntp_tval_read(CPUARMState *env, const ARMCPRegInfo >> > +*ri, uint64_t *value) { >> > + archtimers *at = (archtimers *)(ri->opaque); >> > + *value = qemu_timer_expire_time_ns(at->pl1_timer); >> > + return 0; >> > +} >> > + >> > +static int a15_cntp_tval_write(CPUARMState *env, const ARMCPRegInfo >> > +*ri, uint64_t value) { >> > + archtimers *at = (archtimers *)(ri->opaque); >> > + >> > + /* I assume that setting a new value means that we should clear >> > + any previous? */ >> > + qemu_set_irq(at->pl1_irq, 0); >> > + at->cntp_ctl &= ~CTL_INT; >> > + >> > + qemu_mod_timer_ns(at->pl1_timer, qemu_get_clock_ns(vm_clock) + >> > + value); >> > + >> > + return 0; >> > +} >> > + >> > +static int a15_cntp_ctl_read(CPUARMState *env, const ARMCPRegInfo >> > +*ri, uint64_t *value) { >> > + archtimers *at = (archtimers *)(ri->opaque); >> > + >> > + *value = at->cntp_ctl; >> > + >> > + return 0; >> > +} >> > + >> > +static int a15_cntp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri, >> > + uint64_t value) { >> > + archtimers *at = (archtimers *)(ri->opaque); >> > + >> > + /* Copy "enable" and "mask" bits from the new value. Preserve the rest. >> */ >> > + at->cntp_ctl = (at->cntp_ctl & ~(CTL_ENABLE | CTL_MASK)) | (value >> > + & (CTL_ENABLE | CTL_MASK)); >> > + >> > + /* If no interrupt is asserted, or interrupt is masked, then lower the irq. */ >> > + if (!(at->cntp_ctl & CTL_INT) || (at->cntp_ctl & CTL_MASK)) >> > + qemu_set_irq(at->pl1_irq, 0); >> >> Missing braces, please read CODING_STYLE. Please fix also other cases below. >> >> > + >> > + /* If interrupt is asserted and not masked, then raise the irq. */ >> > + if ((at->cntp_ctl & CTL_INT) && !(at->cntp_ctl & CTL_MASK)) >> > + qemu_set_irq(at->pl1_irq, 1); >> > + >> > + return 0; >> > +} >> > + >> > +static const ARMCPRegInfo archtimer_cp_reginfo[] = { >> > + >> > + { .name = "CNTFRQ", .cp = 15, .crn = 14, .crm = 0, .opc1 = 0, .opc2 = 0, >> > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntfrq_read, }, >> > + >> > + { .name = "CNTKCTL", .cp = 15, .crn = 14, .crm = 1, .opc1 = 0, .opc2 = 0, >> > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntkctl_read, >> > + }, >> > + >> > + { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0, >> > + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_tval_read, >> > + .writefn = a15_cntp_tval_write, }, >> > + >> > + { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1, >> > + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_ctl_read, >> > + .writefn = a15_cntp_ctl_write, }, >> > + >> > + { .name = "CNTPCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, >> .opc1 = 0, .opc2 = 0, >> > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntpct_read, }, >> > + >> > + { .name = "CNTVCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, >> .opc1 = 1, .opc2 = 0, >> > + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntvct_read, }, >> > + >> > + REGINFO_SENTINEL >> > +}; >> > + >> > +static void pl1_timer_cb(void *opaque) { >> > + archtimers *at = (archtimers *)opaque; >> >> Here the cast is definitely useless, please remove. >> >> > + >> > + /* Set the interrupt bit in control register */ >> > + at->cntp_ctl |= CTL_INT; >> > + >> > + /* If not masked, then raise the irq */ >> > + if (!(at->cntp_ctl & CTL_MASK)) >> > + qemu_set_irq(at->pl1_irq, 1); } >> > + >> > +static int arm_archtimer_init(SysBusDevice *dev) { >> > + arm_archtimer_state *s = FROM_SYSBUS(arm_archtimer_state, dev); >> > + CPUArchState *cpu; >> > + int i; >> > + >> > + for (cpu = first_cpu, i = 0; cpu; cpu = cpu->next_cpu, i++) { >> > + archtimers *at = &s->archtimers[i]; >> > + at->pl1_timer = qemu_new_timer_ns(vm_clock, &pl1_timer_cb, at); >> > + sysbus_init_irq(dev, &(at->pl1_irq)); >> > + sysbus_init_irq(dev, &(at->pl2_irq)); >> > + s->archtimers[i].cpu = arm_env_get_cpu(cpu); >> > + define_arm_cp_regs_with_opaque(s->archtimers[i].cpu, >> archtimer_cp_reginfo, (void *)at); >> > + } >> > + >> > + return 0; >> > +} >> > + >> > +static void arm_archtimer_reset(DeviceState *dev) { >> > + arm_archtimer_state *s = >> > + FROM_SYSBUS(arm_archtimer_state, sysbus_from_qdev(dev)); >> > + int i; >> > + >> > + for (i = 0; i < MAX_CPUS; i++) { >> > + if (s->archtimers[i].pl1_timer) >> > + qemu_del_timer(s->archtimers[i].pl1_timer); >> > + if (s->archtimers[i].pl2_timer) >> > + qemu_del_timer(s->archtimers[i].pl2_timer); >> > + } >> > +} >> > + >> > +static void arm_archtimer_class_init(ObjectClass *class, void *data) >> > +{ >> > + DeviceClass *dc = DEVICE_CLASS(class); >> > + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); >> > + >> > + sbc->init = arm_archtimer_init; >> > + dc->reset = arm_archtimer_reset; >> > +} >> > + >> > +static TypeInfo arm_archtimer_info = { >> > + .name = "arm_archtimer", >> > + .parent = TYPE_SYS_BUS_DEVICE, >> > + .instance_size = sizeof(arm_archtimer_state), >> > + .class_init = arm_archtimer_class_init, >> > +}; >> > + >> > +static void arm_archtimer_register_types(void) >> > +{ >> > + type_register_static(&arm_archtimer_info); >> > +} >> > + >> > +type_init(arm_archtimer_register_types) >> > diff -Nupr qemu-linaro-1.1.50-2012.07/target-arm/helper.c qemu-linaro- >> 1.1.50-2012.07-modified/target-arm/helper.c >> > --- qemu-linaro-1.1.50-2012.07/target-arm/helper.c 2012-07-05 >> 16:48:28.000000000 +0200 >> > +++ qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c 2012-09-12 >> 13:15:45.544424842 +0200 >> > @@ -1012,9 +1012,11 @@ void register_cp_regs_for_features(ARMCP >> > if (arm_feature(env, ARM_FEATURE_THUMB2EE)) { >> > define_arm_cp_regs(cpu, t2ee_cp_reginfo); >> > } >> > + /* >> >> Why would this be needed? >> >> > if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { >> > define_arm_cp_regs(cpu, generic_timer_cp_reginfo); >> > } >> > + */ >> > if (arm_feature(env, ARM_FEATURE_VAPA)) { >> > define_arm_cp_regs(cpu, vapa_cp_reginfo); >> > } >> > >> >
On 15 September 2012 17:41, Blue Swirl <blauwirbel@gmail.com> wrote: > On Sat, Sep 15, 2012 at 8:57 AM, Daniel Forsgren > <daniel.forsgren@enea.com> wrote: >> 1) What is considered to be part of the "core" and what is >> considered to be a device external to the core? To me, it looks >> like co-processor functionality in general is considered to be >> part of the core (implemented in target-arm/helper.c or similar), >> whereas timer devices in general are kept in hw/arm_* (c.f. >> arm_timer.c and arm_mptimer.c). But in this case I have a timer >> that is implemented as a coprocessor - where should that go? Or >> should it be split in two places? > > SoC devices attached to the CPU is a bit grey area. In this case, I > think coprocessor should be part of the CPU. Peter? Yes, I think I would agree; as you say it's an odd case, but I would go for putting it in the CPU model. (For KVM the architectural timers are entirely handled in the kernel so it's not possible to have an in-kernel CPU model and out-of-kernel timers. I think that kind of weights towards calling this part of the CPU.) -- PMM
diff -Nupr qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c qemu-linaro-1.1.50-2012.07-modified/hw/a15mpcore.c --- qemu-linaro-1.1.50-2012.07/hw/a15mpcore.c 2012-07-05 16:48:28.000000000 +0200 +++ qemu-linaro-1.1.50-2012.07-modified/hw/a15mpcore.c 2012-09-12 11:24:25.844237405 +0200 @@ -28,6 +28,7 @@ typedef struct A15MPPrivState { uint32_t num_cpu; uint32_t num_irq; MemoryRegion container; + DeviceState *archtimer; DeviceState *gic; } A15MPPrivState; @@ -40,7 +41,8 @@ static void a15mp_priv_set_irq(void *opa static int a15mp_priv_init(SysBusDevice *dev) { A15MPPrivState *s = FROM_SYSBUS(A15MPPrivState, dev); - SysBusDevice *busdev; + SysBusDevice *busdev, *timerbusdev; + int i; if (kvm_irqchip_in_kernel()) { s->gic = qdev_create(NULL, "kvm-arm_gic"); @@ -60,6 +62,11 @@ static int a15mp_priv_init(SysBusDevice /* Pass through inbound GPIO lines to the GIC */ qdev_init_gpio_in(&s->busdev.qdev, a15mp_priv_set_irq, s->num_irq - 32); + s->archtimer = qdev_create(NULL, "arm_archtimer"); + // qdev_prop_set_uint32(s->archtimer, "num-cpu", s->num_cpu); + qdev_init_nofail(s->archtimer); + timerbusdev = sysbus_from_qdev(s->archtimer); + /* Memory map (addresses are offsets from PERIPHBASE): * 0x0000-0x0fff -- reserved * 0x1000-0x1fff -- GIC Distributor @@ -75,6 +82,16 @@ static int a15mp_priv_init(SysBusDevice sysbus_mmio_get_region(busdev, 1)); sysbus_init_mmio(dev, &s->container); + + + for (i = 0; i < s->num_cpu; i++) { + int ppibase = (s->num_irq - 32) + i * 32; + sysbus_connect_irq(timerbusdev, i * 2, + qdev_get_gpio_in(s->gic, ppibase + 29)); + sysbus_connect_irq(timerbusdev, i * 2 + 1, + qdev_get_gpio_in(s->gic, ppibase + 30)); + } + return 0; } diff -Nupr qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs qemu-linaro-1.1.50-2012.07-modified/hw/arm/Makefile.objs --- qemu-linaro-1.1.50-2012.07/hw/arm/Makefile.objs 2012-07-05 16:48:28.000000000 +0200 +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm/Makefile.objs 2012-09-12 11:28:39.121053287 +0200 @@ -1,4 +1,4 @@ -obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o +obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o arm_archtimer.o obj-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o pl190.o obj-y += versatile_pci.o obj-y += versatile_i2c.o diff -Nupr qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c qemu-linaro-1.1.50-2012.07-modified/hw/arm_archtimer.c --- qemu-linaro-1.1.50-2012.07/hw/arm_archtimer.c 1970-01-01 01:00:00.000000000 +0100 +++ qemu-linaro-1.1.50-2012.07-modified/hw/arm_archtimer.c 2012-09-12 13:21:44.676267111 +0200 @@ -0,0 +1,232 @@ +/* + * "Architectured" timer for A15 + * + * Copyright (c) 2012 Enea Software AB + * Written by Daniel Forsgren + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version + * 2 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#include "sysbus.h" +#include "qemu-timer.h" + +/* Primitive (and imcomplete) support for A15 "architectured" timers + + - Only PL1 timer supported. + + - Only minimal subset of fuctionality requred by Linux. + + - Only tested with single-core. + +*/ + +/* control register bit assignment */ +#define CTL_ENABLE 0x01 +#define CTL_MASK 0x02 +#define CTL_INT 0x04 + +/* state of per-core timers */ +typedef struct { + ARMCPU *cpu; /* who are we serving */ + QEMUTimer *pl1_timer; + QEMUTimer *pl2_timer; + qemu_irq pl1_irq; + qemu_irq pl2_irq; + uint32_t cntkctl; /* timer pl1 control register */ + uint32_t cntp_ctl; /* pl1 physical timer control register */ + uint64_t cntvoff; /* virtual offset register */ +} archtimers; + +#define MAX_CPUS 4 + +typedef struct { + SysBusDevice busdev; + archtimers archtimers[MAX_CPUS]; +} arm_archtimer_state; + + +static int a15_cntfrq_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) +{ + /* Let's assume that we're running at 1GHz for now. It's not + correct, but it simplifies translation between cycles <-> ns */ + *value = 1000000000UL; + return 0; +} + +static int a15_cntkctl_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) +{ + archtimers *at = (archtimers *)(ri->opaque); + *value = at->cntkctl; + return 0; +} + +static int a15_cntpct_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) +{ + /* Let's assume that the physical count register is driven by + vm_clock for now. As we have specified that that we're running + at 1GHz, no translation from ns should be needed. */ + *value = qemu_get_clock_ns(vm_clock); + return 0; +} + +static int a15_cntvct_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) +{ + archtimers *at = (archtimers *)(ri->opaque); + + /* Virtual count should subtract the virtual offfset from physical + count? */ + *value = qemu_get_clock_ns(vm_clock) - at->cntvoff; + return 0; +} + +static int a15_cntp_tval_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) +{ + archtimers *at = (archtimers *)(ri->opaque); + *value = qemu_timer_expire_time_ns(at->pl1_timer); + return 0; +} + +static int a15_cntp_tval_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) +{ + archtimers *at = (archtimers *)(ri->opaque); + + /* I assume that setting a new value means that we should clear + any previous? */ + qemu_set_irq(at->pl1_irq, 0); + at->cntp_ctl &= ~CTL_INT; + + qemu_mod_timer_ns(at->pl1_timer, qemu_get_clock_ns(vm_clock) + value); + + return 0; +} + +static int a15_cntp_ctl_read(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t *value) +{ + archtimers *at = (archtimers *)(ri->opaque); + + *value = at->cntp_ctl; + + return 0; +} + +static int a15_cntp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri, + uint64_t value) +{ + archtimers *at = (archtimers *)(ri->opaque); + + /* Copy "enable" and "mask" bits from the new value. Preserve the rest. */ + at->cntp_ctl = (at->cntp_ctl & ~(CTL_ENABLE | CTL_MASK)) | (value & (CTL_ENABLE | CTL_MASK)); + + /* If no interrupt is asserted, or interrupt is masked, then lower the irq. */ + if (!(at->cntp_ctl & CTL_INT) || (at->cntp_ctl & CTL_MASK)) + qemu_set_irq(at->pl1_irq, 0); + + /* If interrupt is asserted and not masked, then raise the irq. */ + if ((at->cntp_ctl & CTL_INT) && !(at->cntp_ctl & CTL_MASK)) + qemu_set_irq(at->pl1_irq, 1); + + return 0; +} + +static const ARMCPRegInfo archtimer_cp_reginfo[] = { + + { .name = "CNTFRQ", .cp = 15, .crn = 14, .crm = 0, .opc1 = 0, .opc2 = 0, + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntfrq_read, }, + + { .name = "CNTKCTL", .cp = 15, .crn = 14, .crm = 1, .opc1 = 0, .opc2 = 0, + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntkctl_read, }, + + { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0, + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_tval_read, + .writefn = a15_cntp_tval_write, }, + + { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1, + .access = PL1_RW, .resetvalue = 0, .readfn = a15_cntp_ctl_read, + .writefn = a15_cntp_ctl_write, }, + + { .name = "CNTPCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, .opc1 = 0, .opc2 = 0, + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntpct_read, }, + + { .name = "CNTVCT", .type = ARM_CP_64BIT, .cp = 15, .crn = 0, .crm = 14, .opc1 = 1, .opc2 = 0, + .access = PL1_R, .resetvalue = 0, .readfn = a15_cntvct_read, }, + + REGINFO_SENTINEL +}; + +static void pl1_timer_cb(void *opaque) +{ + archtimers *at = (archtimers *)opaque; + + /* Set the interrupt bit in control register */ + at->cntp_ctl |= CTL_INT; + + /* If not masked, then raise the irq */ + if (!(at->cntp_ctl & CTL_MASK)) + qemu_set_irq(at->pl1_irq, 1); +} + +static int arm_archtimer_init(SysBusDevice *dev) +{ + arm_archtimer_state *s = FROM_SYSBUS(arm_archtimer_state, dev); + CPUArchState *cpu; + int i; + + for (cpu = first_cpu, i = 0; cpu; cpu = cpu->next_cpu, i++) { + archtimers *at = &s->archtimers[i]; + at->pl1_timer = qemu_new_timer_ns(vm_clock, &pl1_timer_cb, at); + sysbus_init_irq(dev, &(at->pl1_irq)); + sysbus_init_irq(dev, &(at->pl2_irq)); + s->archtimers[i].cpu = arm_env_get_cpu(cpu); + define_arm_cp_regs_with_opaque(s->archtimers[i].cpu, archtimer_cp_reginfo, (void *)at); + } + + return 0; +} + +static void arm_archtimer_reset(DeviceState *dev) +{ + arm_archtimer_state *s = + FROM_SYSBUS(arm_archtimer_state, sysbus_from_qdev(dev)); + int i; + + for (i = 0; i < MAX_CPUS; i++) { + if (s->archtimers[i].pl1_timer) + qemu_del_timer(s->archtimers[i].pl1_timer); + if (s->archtimers[i].pl2_timer) + qemu_del_timer(s->archtimers[i].pl2_timer); + } +} + +static void arm_archtimer_class_init(ObjectClass *class, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(class); + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(class); + + sbc->init = arm_archtimer_init; + dc->reset = arm_archtimer_reset; +} + +static TypeInfo arm_archtimer_info = { + .name = "arm_archtimer", + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(arm_archtimer_state), + .class_init = arm_archtimer_class_init, +}; + +static void arm_archtimer_register_types(void) +{ + type_register_static(&arm_archtimer_info); +} + +type_init(arm_archtimer_register_types) diff -Nupr qemu-linaro-1.1.50-2012.07/target-arm/helper.c qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c --- qemu-linaro-1.1.50-2012.07/target-arm/helper.c 2012-07-05 16:48:28.000000000 +0200 +++ qemu-linaro-1.1.50-2012.07-modified/target-arm/helper.c 2012-09-12 13:15:45.544424842 +0200 @@ -1012,9 +1012,11 @@ void register_cp_regs_for_features(ARMCP if (arm_feature(env, ARM_FEATURE_THUMB2EE)) { define_arm_cp_regs(cpu, t2ee_cp_reginfo); } + /* if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { define_arm_cp_regs(cpu, generic_timer_cp_reginfo); } + */ if (arm_feature(env, ARM_FEATURE_VAPA)) { define_arm_cp_regs(cpu, vapa_cp_reginfo); }
This patch adds basic support for the "architected" timers (i.e. cp15) found in A15. It's enough to allow Linux to boot, using arch_timer for the tick. However - it is not a complete model of the timer block at large, it is not that well structured, and it is currently tested with qemu-linaro-1.1.50-2012.07 (not latest and greatest). It's simply a prototype. However, if anyone wants to play with the architectured (cp15) timers instead of sp804, then please feel free to try it out. It has been tested with linux-linaro-3.6-rc2-2012.08, and you can easily verify the existence of these timers under /proc/interrupts: root@linaro-developer:~# cat /proc/interrupts cat /proc/interrupts CPU0 29: 7424 GIC arch_timer 30: 0 GIC arch_timer Please note that this also requires some minor fixes that are not part of qemu-linaro-1.1.50-2012.07: http://patches.linaro.org/9833/ Signed-off-by: Daniel Forsgren <daniel.forsgren@enea.com> ---