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

[Blue Swirl]

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);
>      }
>
>