[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] hw/misc/arm_sp810: Create SP810 device
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH 1/2] hw/misc/arm_sp810: Create SP810 device |
|
Date: |
Thu, 17 Jul 2014 09:21:44 +1000 |
On Thu, Jul 17, 2014 at 12:42 AM, Fabian Aggeler <address@hidden> 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 <address@hidden>
> ---
> 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 <address@hidden>
> + *
> + * 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
>
>