[Qemu-devel] [PATCH 0/2] Add SP810 to Versatile Express boards

[Qemu-devel] [PATCH 0/2] Add SP810 to Versatile Express boards

[Fabian Aggeler]

Hi,

The Versatile Express emulation in QEMU currently does not
have a model of the SP810 used in real hardware. The registers
provided by this System Controller can be used to set the
frequency of the SP804 timers. On newer releases of the board
the SP804 is set to 32kHz by default and has to be increased
by writing to the SCCTRL. See: 
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/038696.html

Since QEMU sets the SP804 timers to 1MHz by default this should
be reflected in the SCCTRL register. These two patches add a
model of the SP804 to the vexpress-boards and sets the SCCTRL 
register so that software that queries this register behaves 
as expected.

Feedback is greatly appreciated!

Best,
Fabian

Fabian Aggeler (2):
  hw/misc/arm_sp810: Create SP810 device
  hw/arm/vexpress: add SP810 to the vexpress

 default-configs/arm-softmmu.mak |   1 +
 hw/arm/vexpress.c               |  11 ++-
 hw/misc/Makefile.objs           |   1 +
 hw/misc/arm_sp810.c             | 184 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 195 insertions(+), 2 deletions(-)
 create mode 100644 hw/misc/arm_sp810.c

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/2] hw/misc/arm_sp810: Create SP810 device

[Fabian Aggeler]

This adds a device model for the PrimeXsys System Controller (SP810)
which is present in the Versatile Express motherboards. It is
so far read-only but allows to set the SCCTRL register.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 default-configs/arm-softmmu.mak |   1 +
 hw/misc/Makefile.objs           |   1 +
 hw/misc/arm_sp810.c             | 184 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)
 create mode 100644 hw/misc/arm_sp810.c

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index f3513fa..67ba99a 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -55,6 +55,7 @@ CONFIG_PL181=y
 CONFIG_PL190=y
 CONFIG_PL310=y
 CONFIG_PL330=y
+CONFIG_SP810=y
 CONFIG_CADENCE=y
 CONFIG_XGMAC=y
 CONFIG_EXYNOS4=y
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 979e532..49d023b 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -13,6 +13,7 @@ common-obj-$(CONFIG_PL310) += arm_l2x0.o
 common-obj-$(CONFIG_INTEGRATOR_DEBUG) += arm_integrator_debug.o
 common-obj-$(CONFIG_A9SCU) += a9scu.o
 common-obj-$(CONFIG_ARM11SCU) += arm11scu.o
+common-obj-$(CONFIG_SP810) += arm_sp810.o
 
 # PKUnity SoC devices
 common-obj-$(CONFIG_PUV3) += puv3_pm.o
diff --git a/hw/misc/arm_sp810.c b/hw/misc/arm_sp810.c
new file mode 100644
index 0000000..82cbcf0
--- /dev/null
+++ b/hw/misc/arm_sp810.c
@@ -0,0 +1,184 @@
+/*
+ * ARM PrimeXsys System Controller (SP810)
+ *
+ * Copyright (C) 2014 Fabian Aggeler <aggelerf@ethz.ch>
+ *
+ * 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.
+ *
+ */
+
+#include "hw/hw.h"
+#include "hw/sysbus.h"
+
+#define LOCK_VALUE 0xa05f
+
+#define TYPE_ARM_SP810 "arm_sp810"
+#define ARM_SP810(obj) OBJECT_CHECK(arm_sp810_state, (obj), TYPE_ARM_SP810)
+
+typedef struct {
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+
+    uint32_t sc_ctrl; /* SCCTRL */
+} arm_sp810_state;
+
+#define TIMEREN0SEL (1 << 15)
+#define TIMEREN1SEL (1 << 17)
+#define TIMEREN2SEL (1 << 19)
+#define TIMEREN3SEL (1 << 21)
+
+static const VMStateDescription vmstate_arm_sysctl = {
+    .name = "arm_sp810",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(sc_ctrl, arm_sp810_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static uint64_t arm_sp810_read(void *opaque, hwaddr offset, unsigned size)
+{
+    arm_sp810_state *s = (arm_sp810_state *)opaque;
+
+    switch (offset) {
+    case 0x000: /* SCCTRL */
+        return s->sc_ctrl;
+    case 0x004: /* SCSYSSTAT */
+    case 0x008: /* SCIMCTRL */
+    case 0x00C: /* SCIMSTAT */
+    case 0x010: /* SCXTALCTRL */
+    case 0x014: /* SCPLLCTRL */
+    case 0x018: /* SCPLLFCTRL */
+    case 0x01C: /* SCPERCTRL0 */
+    case 0x020: /* SCPERCTRL1 */
+    case 0x024: /* SCPEREN */
+    case 0x028: /* SCPERDIS */
+    case 0x02C: /* SCPERCLKEN */
+    case 0x030: /* SCPERSTAT */
+    case 0xEE0: /* SCSYSID0 */
+    case 0xEE4: /* SCSYSID1 */
+    case 0xEE8: /* SCSYSID2 */
+    case 0xEEC: /* SCSYSID3 */
+    case 0xF00: /* SCITCR */
+    case 0xF04: /* SCITIR0 */
+    case 0xF08: /* SCITIR1 */
+    case 0xF0C: /* SCITOR */
+    case 0xF10: /* SCCNTCTRL */
+    case 0xF14: /* SCCNTDATA */
+    case 0xF18: /* SCCNTSTEP */
+    case 0xFE0: /* SCPERIPHID0 */
+    case 0xFE4: /* SCPERIPHID1 */
+    case 0xFE8: /* SCPERIPHID2 */
+    case 0xFEC: /* SCPERIPHID3 */
+    case 0xFF0: /* SCPCELLID0 */
+    case 0xFF4: /* SCPCELLID1 */
+    case 0xFF8: /* SCPCELLID2 */
+    case 0xFFC: /* SCPCELLID3 */
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                "arm_sp810_read: Register not yet implemented: "
+                "0x%x\n",
+                (int)offset);
+        return 0;
+    }
+}
+
+
+
+static void arm_sp810_write(void *opaque, hwaddr offset,
+                             uint64_t val, unsigned size)
+{
+    switch (offset) {
+    case 0x000: /* SCCTRL */
+    case 0x004: /* SCSYSSTAT */
+    case 0x008: /* SCIMCTRL */
+    case 0x00C: /* SCIMSTAT */
+    case 0x010: /* SCXTALCTRL */
+    case 0x014: /* SCPLLCTRL */
+    case 0x018: /* SCPLLFCTRL */
+    case 0x01C: /* SCPERCTRL0 */
+    case 0x020: /* SCPERCTRL1 */
+    case 0x024: /* SCPEREN */
+    case 0x028: /* SCPERDIS */
+    case 0x02C: /* SCPERCLKEN */
+    case 0x030: /* SCPERSTAT */
+    case 0xEE0: /* SCSYSID0 */
+    case 0xEE4: /* SCSYSID1 */
+    case 0xEE8: /* SCSYSID2 */
+    case 0xEEC: /* SCSYSID3 */
+    case 0xF00: /* SCITCR */
+    case 0xF04: /* SCITIR0 */
+    case 0xF08: /* SCITIR1 */
+    case 0xF0C: /* SCITOR */
+    case 0xF10: /* SCCNTCTRL */
+    case 0xF14: /* SCCNTDATA */
+    case 0xF18: /* SCCNTSTEP */
+    case 0xFE0: /* SCPERIPHID0 */
+    case 0xFE4: /* SCPERIPHID1 */
+    case 0xFE8: /* SCPERIPHID2 */
+    case 0xFEC: /* SCPERIPHID3 */
+    case 0xFF0: /* SCPCELLID0 */
+    case 0xFF4: /* SCPCELLID1 */
+    case 0xFF8: /* SCPCELLID2 */
+    case 0xFFC: /* SCPCELLID3 */
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                "arm_sp810_write: Register not yet implemented: 0x%x\n",
+                (int)offset);
+        return;
+    }
+}
+
+static const MemoryRegionOps arm_sp810_ops = {
+    .read = arm_sp810_read,
+    .write = arm_sp810_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void arm_sp810_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    arm_sp810_state *s = ARM_SP810(obj);
+
+    memory_region_init_io(&s->iomem, OBJECT(dev), &arm_sp810_ops, s,
+                          "arm-sp810", 0x1000);
+    sysbus_init_mmio(sd, &s->iomem);
+}
+
+static Property arm_sp810_properties[] = {
+    DEFINE_PROP_UINT32("sc-ctrl", arm_sp810_state, sc_ctrl, 0x000009),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void arm_sp810_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_arm_sysctl;
+    dc->props = arm_sp810_properties;
+}
+
+static const TypeInfo arm_sp810_info = {
+    .name          = TYPE_ARM_SP810,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(arm_sp810_state),
+    .instance_init = arm_sp810_init,
+    .class_init    = arm_sp810_class_init,
+};
+
+static void arm_sp810_register_types(void)
+{
+    type_register_static(&arm_sp810_info);
+}
+
+type_init(arm_sp810_register_types)
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 1/2] hw/misc/arm_sp810: Create SP810 device

[Peter Crosthwaite]

On Thu, Jul 17, 2014 at 12:42 AM, Fabian Aggeler <aggelerf@ethz.ch> wrote:
> This adds a device model for the PrimeXsys System Controller (SP810)
> which is present in the Versatile Express motherboards. It is
> so far read-only but allows to set the SCCTRL register.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/misc/Makefile.objs           |   1 +
>  hw/misc/arm_sp810.c             | 184 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
>  create mode 100644 hw/misc/arm_sp810.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index f3513fa..67ba99a 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -55,6 +55,7 @@ CONFIG_PL181=y
>  CONFIG_PL190=y
>  CONFIG_PL310=y
>  CONFIG_PL330=y
> +CONFIG_SP810=y
>  CONFIG_CADENCE=y
>  CONFIG_XGMAC=y
>  CONFIG_EXYNOS4=y
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 979e532..49d023b 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -13,6 +13,7 @@ common-obj-$(CONFIG_PL310) += arm_l2x0.o
>  common-obj-$(CONFIG_INTEGRATOR_DEBUG) += arm_integrator_debug.o
>  common-obj-$(CONFIG_A9SCU) += a9scu.o
>  common-obj-$(CONFIG_ARM11SCU) += arm11scu.o
> +common-obj-$(CONFIG_SP810) += arm_sp810.o
>
>  # PKUnity SoC devices
>  common-obj-$(CONFIG_PUV3) += puv3_pm.o
> diff --git a/hw/misc/arm_sp810.c b/hw/misc/arm_sp810.c
> new file mode 100644
> index 0000000..82cbcf0
> --- /dev/null
> +++ b/hw/misc/arm_sp810.c
> @@ -0,0 +1,184 @@
> +/*
> + * ARM PrimeXsys System Controller (SP810)
> + *
> + * Copyright (C) 2014 Fabian Aggeler <aggelerf@ethz.ch>
> + *
> + * 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.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +
> +#define LOCK_VALUE 0xa05f
> +
> +#define TYPE_ARM_SP810 "arm_sp810"
> +#define ARM_SP810(obj) OBJECT_CHECK(arm_sp810_state, (obj), TYPE_ARM_SP810)
> +
> +typedef struct {
> +    SysBusDevice parent_obj;
> +    MemoryRegion iomem;
> +
> +    uint32_t sc_ctrl; /* SCCTRL */
> +} arm_sp810_state;
> +
> +#define TIMEREN0SEL (1 << 15)
> +#define TIMEREN1SEL (1 << 17)
> +#define TIMEREN2SEL (1 << 19)
> +#define TIMEREN3SEL (1 << 21)
> +

Are these fields of a particular register? Is so they should have the
register name as a prefix.

> +static const VMStateDescription vmstate_arm_sysctl = {
> +    .name = "arm_sp810",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(sc_ctrl, arm_sp810_state),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static uint64_t arm_sp810_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    arm_sp810_state *s = (arm_sp810_state *)opaque;

Drop the cast and just rely on the implicit.

> +
> +    switch (offset) {
> +    case 0x000: /* SCCTRL */

Define macros for register offset rather than switch on literal
addresses + comments.

> +        return s->sc_ctrl;
> +    case 0x004: /* SCSYSSTAT */
> +    case 0x008: /* SCIMCTRL */
> +    case 0x00C: /* SCIMSTAT */
> +    case 0x010: /* SCXTALCTRL */
> +    case 0x014: /* SCPLLCTRL */
> +    case 0x018: /* SCPLLFCTRL */
> +    case 0x01C: /* SCPERCTRL0 */
> +    case 0x020: /* SCPERCTRL1 */
> +    case 0x024: /* SCPEREN */
> +    case 0x028: /* SCPERDIS */
> +    case 0x02C: /* SCPERCLKEN */
> +    case 0x030: /* SCPERSTAT */
> +    case 0xEE0: /* SCSYSID0 */
> +    case 0xEE4: /* SCSYSID1 */
> +    case 0xEE8: /* SCSYSID2 */
> +    case 0xEEC: /* SCSYSID3 */
> +    case 0xF00: /* SCITCR */
> +    case 0xF04: /* SCITIR0 */
> +    case 0xF08: /* SCITIR1 */
> +    case 0xF0C: /* SCITOR */
> +    case 0xF10: /* SCCNTCTRL */
> +    case 0xF14: /* SCCNTDATA */
> +    case 0xF18: /* SCCNTSTEP */
> +    case 0xFE0: /* SCPERIPHID0 */
> +    case 0xFE4: /* SCPERIPHID1 */
> +    case 0xFE8: /* SCPERIPHID2 */
> +    case 0xFEC: /* SCPERIPHID3 */
> +    case 0xFF0: /* SCPCELLID0 */
> +    case 0xFF4: /* SCPCELLID1 */
> +    case 0xFF8: /* SCPCELLID2 */
> +    case 0xFFC: /* SCPCELLID3 */

I would just let the default do it's thing for the moment and not
worry about these. As a general rule I prefer to just index an array
for register writes and limit the switch statement to only regs with
side effects. So most of these would disappear under that scheme. You
could #define them all (as mentioned above for SCCTRL) if you are
looking for completeness.

> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                "arm_sp810_read: Register not yet implemented: "
> +                "0x%x\n",
> +                (int)offset);

HWADDR_PRIx will avoid the cast.

> +        return 0;
> +    }
> +}
> +
> +
> +

Extra blanks not needed.

> +static void arm_sp810_write(void *opaque, hwaddr offset,
> +                             uint64_t val, unsigned size)
> +{
> +    switch (offset) {
> +    case 0x000: /* SCCTRL */
> +    case 0x004: /* SCSYSSTAT */
> +    case 0x008: /* SCIMCTRL */
> +    case 0x00C: /* SCIMSTAT */
> +    case 0x010: /* SCXTALCTRL */
> +    case 0x014: /* SCPLLCTRL */
> +    case 0x018: /* SCPLLFCTRL */
> +    case 0x01C: /* SCPERCTRL0 */
> +    case 0x020: /* SCPERCTRL1 */
> +    case 0x024: /* SCPEREN */
> +    case 0x028: /* SCPERDIS */
> +    case 0x02C: /* SCPERCLKEN */
> +    case 0x030: /* SCPERSTAT */
> +    case 0xEE0: /* SCSYSID0 */
> +    case 0xEE4: /* SCSYSID1 */
> +    case 0xEE8: /* SCSYSID2 */
> +    case 0xEEC: /* SCSYSID3 */
> +    case 0xF00: /* SCITCR */
> +    case 0xF04: /* SCITIR0 */
> +    case 0xF08: /* SCITIR1 */
> +    case 0xF0C: /* SCITOR */
> +    case 0xF10: /* SCCNTCTRL */
> +    case 0xF14: /* SCCNTDATA */
> +    case 0xF18: /* SCCNTSTEP */
> +    case 0xFE0: /* SCPERIPHID0 */
> +    case 0xFE4: /* SCPERIPHID1 */
> +    case 0xFE8: /* SCPERIPHID2 */
> +    case 0xFEC: /* SCPERIPHID3 */
> +    case 0xFF0: /* SCPCELLID0 */
> +    case 0xFF4: /* SCPCELLID1 */
> +    case 0xFF8: /* SCPCELLID2 */
> +    case 0xFFC: /* SCPCELLID3 */
> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                "arm_sp810_write: Register not yet implemented: 0x%x\n",
> +                (int)offset);
> +        return;
> +    }
> +}
> +
> +static const MemoryRegionOps arm_sp810_ops = {
> +    .read = arm_sp810_read,
> +    .write = arm_sp810_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void arm_sp810_init(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
> +    arm_sp810_state *s = ARM_SP810(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(dev), &arm_sp810_ops, s,

obj instead of OBJECT(dev).

> +                          "arm-sp810", 0x1000);
> +    sysbus_init_mmio(sd, &s->iomem);

For single use QOM casts I tend to just do them inline:

sysbus_init_mmio(SYSBUS_DEVICE(obj), &s->iomem);

Regards,
Peter

> +}
> +
> +static Property arm_sp810_properties[] = {
> +    DEFINE_PROP_UINT32("sc-ctrl", arm_sp810_state, sc_ctrl, 0x000009),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void arm_sp810_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_arm_sysctl;
> +    dc->props = arm_sp810_properties;
> +}
> +
> +static const TypeInfo arm_sp810_info = {
> +    .name          = TYPE_ARM_SP810,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(arm_sp810_state),
> +    .instance_init = arm_sp810_init,
> +    .class_init    = arm_sp810_class_init,
> +};
> +
> +static void arm_sp810_register_types(void)
> +{
> +    type_register_static(&arm_sp810_info);
> +}
> +
> +type_init(arm_sp810_register_types)
> --
> 1.8.3.2
>
>

[Qemu-devel] [PATCH 2/2] hw/arm/vexpress: add SP810 to the vexpress

[Fabian Aggeler]

The SP810, which is present in the Versatile Express motherboards,
allows to set the timing reference to either REFCLK or TIMCLK.
QEMU currently sets the SP804 timer to 1MHz by default. To reflect
this, we set the TimerEn0Sel, TimerEn1Sel, TimerEn2Sel, and
TimerEn3Sel of the system control register (SCCTRL) to TIMCLK (1).

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
---
 hw/arm/vexpress.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index a88732c..b96c3fd 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -512,7 +512,7 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
 static void vexpress_common_init(VEDBoardInfo *daughterboard,
                                  MachineState *machine)
 {
-    DeviceState *dev, *sysctl, *pl041;
+    DeviceState *dev, *sysctl, *pl041, *sp810;
     qemu_irq pic[64];
     uint32_t sys_id;
     DriveInfo *dinfo;
@@ -575,7 +575,14 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
     qdev_init_nofail(sysctl);
     sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, map[VE_SYSREGS]);
 
-    /* VE_SP810: not modelled */
+    /* VE_SP810 */
+    sp810 = qdev_create(NULL, "arm_sp810");
+    /* SP804 is already running at 1MHz (TIMCLK) so SCCTRL TimerEnXSel=1 */
+    qdev_prop_set_uint32(sp810, "sc-ctrl", (1 << 15) | (1 << 17)
+                                          | (1 << 19) | (1 << 21));
+    qdev_init_nofail(sp810);
+    sysbus_mmio_map(SYS_BUS_DEVICE(sp810), 0, map[VE_SP810]);
+
     /* VE_SERIALPCI: not modelled */
 
     pl041 = qdev_create(NULL, "pl041");
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 2/2] hw/arm/vexpress: add SP810 to the vexpress

[Alex Bennée]

Fabian Aggeler writes:

> The SP810, which is present in the Versatile Express motherboards,
> allows to set the timing reference to either REFCLK or TIMCLK.
> QEMU currently sets the SP804 timer to 1MHz by default. To reflect
> this, we set the TimerEn0Sel, TimerEn1Sel, TimerEn2Sel, and
> TimerEn3Sel of the system control register (SCCTRL) to TIMCLK (1).
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  hw/arm/vexpress.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index a88732c..b96c3fd 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -512,7 +512,7 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
>  static void vexpress_common_init(VEDBoardInfo *daughterboard,
>                                   MachineState *machine)
>  {
> -    DeviceState *dev, *sysctl, *pl041;
> +    DeviceState *dev, *sysctl, *pl041, *sp810;
>      qemu_irq pic[64];
>      uint32_t sys_id;
>      DriveInfo *dinfo;
> @@ -575,7 +575,14 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
>      qdev_init_nofail(sysctl);
>      sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, map[VE_SYSREGS]);
>  
> -    /* VE_SP810: not modelled */
> +    /* VE_SP810 */
> +    sp810 = qdev_create(NULL, "arm_sp810");
> +    /* SP804 is already running at 1MHz (TIMCLK) so SCCTRL TimerEnXSel=1 */
> +    qdev_prop_set_uint32(sp810, "sc-ctrl", (1 << 15) | (1 << 17)
> +                                          | (1 << 19) | (1 << 21));
<snip>

Could the #defines in the first patch be moved into a header and used
here rather than manually  setting these bits?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 2/2] hw/arm/vexpress: add SP810 to the vexpress

[Peter Crosthwaite]

On Thu, Jul 17, 2014 at 1:05 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Fabian Aggeler writes:
>
>> The SP810, which is present in the Versatile Express motherboards,
>> allows to set the timing reference to either REFCLK or TIMCLK.
>> QEMU currently sets the SP804 timer to 1MHz by default. To reflect
>> this, we set the TimerEn0Sel, TimerEn1Sel, TimerEn2Sel, and
>> TimerEn3Sel of the system control register (SCCTRL) to TIMCLK (1).
>>
>> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>> ---
>>  hw/arm/vexpress.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
>> index a88732c..b96c3fd 100644
>> --- a/hw/arm/vexpress.c
>> +++ b/hw/arm/vexpress.c
>> @@ -512,7 +512,7 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
>>  static void vexpress_common_init(VEDBoardInfo *daughterboard,
>>                                   MachineState *machine)
>>  {
>> -    DeviceState *dev, *sysctl, *pl041;
>> +    DeviceState *dev, *sysctl, *pl041, *sp810;
>>      qemu_irq pic[64];
>>      uint32_t sys_id;
>>      DriveInfo *dinfo;
>> @@ -575,7 +575,14 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
>>      qdev_init_nofail(sysctl);
>>      sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, map[VE_SYSREGS]);
>>
>> -    /* VE_SP810: not modelled */
>> +    /* VE_SP810 */
>> +    sp810 = qdev_create(NULL, "arm_sp810");

Move the the type definition macro to header as well.

Regards,
Peter

>> +    /* SP804 is already running at 1MHz (TIMCLK) so SCCTRL TimerEnXSel=1 */
>> +    qdev_prop_set_uint32(sp810, "sc-ctrl", (1 << 15) | (1 << 17)
>> +                                          | (1 << 19) | (1 << 21));
> <snip>
>
> Could the #defines in the first patch be moved into a header and used
> here rather than manually  setting these bits?
>
> --
> Alex Bennée
>

Re: [Qemu-devel] [PATCH 2/2] hw/arm/vexpress: add SP810 to the vexpress

[Aggeler Fabian]

On 17 Jul 2014, at 01:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Thu, Jul 17, 2014 at 1:05 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> 
>> Fabian Aggeler writes:
>> 
>>> The SP810, which is present in the Versatile Express motherboards,
>>> allows to set the timing reference to either REFCLK or TIMCLK.
>>> QEMU currently sets the SP804 timer to 1MHz by default. To reflect
>>> this, we set the TimerEn0Sel, TimerEn1Sel, TimerEn2Sel, and
>>> TimerEn3Sel of the system control register (SCCTRL) to TIMCLK (1).
>>> 
>>> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>>> ---
>>> hw/arm/vexpress.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
>>> index a88732c..b96c3fd 100644
>>> --- a/hw/arm/vexpress.c
>>> +++ b/hw/arm/vexpress.c
>>> @@ -512,7 +512,7 @@ static pflash_t *ve_pflash_cfi01_register(hwaddr base, const char *name,
>>> static void vexpress_common_init(VEDBoardInfo *daughterboard,
>>>                                  MachineState *machine)
>>> {
>>> -    DeviceState *dev, *sysctl, *pl041;
>>> +    DeviceState *dev, *sysctl, *pl041, *sp810;
>>>     qemu_irq pic[64];
>>>     uint32_t sys_id;
>>>     DriveInfo *dinfo;
>>> @@ -575,7 +575,14 @@ static void vexpress_common_init(VEDBoardInfo *daughterboard,
>>>     qdev_init_nofail(sysctl);
>>>     sysbus_mmio_map(SYS_BUS_DEVICE(sysctl), 0, map[VE_SYSREGS]);
>>> 
>>> -    /* VE_SP810: not modelled */
>>> +    /* VE_SP810 */
>>> +    sp810 = qdev_create(NULL, "arm_sp810");
> 
> Move the the type definition macro to header as well.
> 
> Regards,
> Peter

Thanks for your comments. I addressed them in v2 which I am going to send shortly.

Best,
Fabian

> 
>>> +    /* SP804 is already running at 1MHz (TIMCLK) so SCCTRL TimerEnXSel=1 */
>>> +    qdev_prop_set_uint32(sp810, "sc-ctrl", (1 << 15) | (1 << 17)
>>> +                                          | (1 << 19) | (1 << 21));
>> <snip>
>> 
>> Could the #defines in the first patch be moved into a header and used
>> here rather than manually  setting these bits?
>> 
>> --
>> Alex Bennée