Re: [Qemu-devel] [PATCH] Basic support for ARM A15 "architectured" (cp15) timers

[Daniel Forsgren]

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:address@hidden
> Sent: den 14 september 2012 19:26
> To: Daniel Forsgren
> Cc: address@hidden
> Subject: Re: [Qemu-devel] [PATCH] Basic support for ARM A15 "architectured"
> (cp15) timers
> 
> On Wed, Sep 12, 2012 at 11:49 AM, Daniel Forsgren
> <address@hidden> 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:
> >
> >     address@hidden:~# 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 <address@hidden>
> >
> > ---
> >
> > 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);
> >      }
> >
> >