[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] hw/misc/arm_sp810: Create SP810 device
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] hw/misc/arm_sp810: Create SP810 device |
|
Date: |
Wed, 6 Aug 2014 09:03:40 +1000 |
On Tue, Aug 5, 2014 at 7:32 PM, 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 | 98
> +++++++++++++++++++++++++++++++++++++++++
> include/hw/misc/arm_sp810.h | 85 +++++++++++++++++++++++++++++++++++
> 4 files changed, 185 insertions(+)
> create mode 100644 hw/misc/arm_sp810.c
> create mode 100644 include/hw/misc/arm_sp810.h
>
> 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..21eb816
> --- /dev/null
> +++ b/hw/misc/arm_sp810.c
> @@ -0,0 +1,98 @@
> +/*
> + * 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/misc/arm_sp810.h"
> +
> +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 = opaque;
> +
> + switch (offset) {
> + case SCCTRL:
> + return s->sc_ctrl;
> + default:
> + qemu_log_mask(LOG_UNIMP,
> + "arm_sp810_read: Register not yet implemented: %s\n",
> + HWADDR_PRIx);
> + return 0;
> + }
> +}
> +
> +static void arm_sp810_write(void *opaque, hwaddr offset,
> + uint64_t val, unsigned size)
> +{
> + switch (offset) {
> + default:
> + qemu_log_mask(LOG_UNIMP,
> + "arm_sp810_write: Register not yet implemented: %s\n",
> + HWADDR_PRIx);
> + 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)
> +{
> + arm_sp810_state *s = ARM_SP810(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &arm_sp810_ops, s, "arm-sp810",
> + 0x1000);
> + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +}
> +
> +static Property arm_sp810_properties[] = {
> + DEFINE_PROP_UINT32("sc-ctrl", arm_sp810_state, sc_ctrl, 0x000009),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
So it looks to me like the SCCTRL contains multiple register fields
for multiple configurable options. Although i'm having trouble getting
my head around it as I could not easily find public docco for this
core (have a link handy?).
Anyways, the thinking is you should define multiple configurable
options as invididual QOM properties, rather than have board level
code init registers. This would mean that you
DEFINE_PROP_UINT8("timeren0-sel", ...)
DEFINE_PROP_UINT8("timeren1-sel", ...)
Have a look at hw/dma/pl330.c for an example of invidividual configs
getting packed into a single sw visible register.
However these look like software configurable options. Does the
hardware IP provide the option to configure these with a default or
must software (probably early stage firmware) configure these to match
clocking?
> +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)
> diff --git a/include/hw/misc/arm_sp810.h b/include/hw/misc/arm_sp810.h
> new file mode 100644
> index 0000000..33021d5
> --- /dev/null
> +++ b/include/hw/misc/arm_sp810.h
> @@ -0,0 +1,85 @@
> +/*
> + * 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.
> + *
> + */
> +
> +#ifndef HW_MISC_ARM_SP810H
> +#define HW_MISC_ARM_SP810H
> +
> +#include "hw/hw.h"
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ARM_SP810 "arm_sp810"
> +#define ARM_SP810(obj) OBJECT_CHECK(arm_sp810_state, (obj), TYPE_ARM_SP810)
> +
> +/* Register Offsets */
> +#define SCCTRL 0x000
> +#define SCSYSSTAT 0x004
> +#define SCIMCTRL 0x008
> +#define SCIMSTAT 0x00C
> +#define SCXTALCTRL 0x010
> +#define SCPLLCTRL 0x014
> +#define SCPLLFCTRL 0x018
> +#define SCPERCTRL0 0x01C
> +#define SCPERCTRL1 0x020
> +#define SCPEREN 0x024
> +#define SCPERDIS 0x028
> +#define SCPERCLKEN 0x02C
> +#define SCPERSTAT 0x030
> +#define SCSYSID0 0xEE0
> +#define SCSYSID1 0xEE4
> +#define SCSYSID2 0xEE8
> +#define SCSYSID3 0xEEC
> +#define SCITCR 0xF00
> +#define SCITIR0 0xF04
> +#define SCITIR1 0xF08
> +#define SCITOR 0xF0C
> +#define SCCNTCTRL 0xF10
> +#define SCCNTDATA 0xF14
> +#define SCCNTSTEP 0xF18
> +#define SCPERIPHID0 0xFE0
> +#define SCPERIPHID1 0xFE4
> +#define SCPERIPHID2 0xFE8
> +#define SCPERIPHID3 0xFEC
> +#define SCPCELLID0 0xFF0
> +#define SCPCELLID1 0xFF4
> +#define SCPCELLID2 0xFF8
> +#define SCPCELLID3 0xFFC
> +
> +/* System Control Register bits */
> +#define SCCTRL_TIMEREN0SEL (1 << 15)
> +#define SCCTRL_TIMEREN1SEL (1 << 17)
> +#define SCCTRL_TIMEREN2SEL (1 << 19)
> +#define SCCTRL_TIMEREN3SEL (1 << 21)
> +
> +static inline DeviceState *sp810_init(hwaddr base, uint32_t scctrl)
> +{
> + DeviceState *sp810;
> +
> + sp810 = qdev_create(NULL, "arm_sp810");
> + qdev_prop_set_uint32(sp810, "sc-ctrl", scctrl);
> + qdev_init_nofail(sp810);
> + sysbus_mmio_map(SYS_BUS_DEVICE(sp810), 0, base);
> + return sp810;
> +}
> +
_init helpers are progressively phasing out. Just inline this straight
into vexpress - its expected for boards code to do its own qdev_foo
sysbus_foo etc.
> +typedef struct {
> + SysBusDevice parent_obj;
> + MemoryRegion iomem;
> +
> + uint32_t sc_ctrl; /* SCCTRL */
Redundant comment.
Regards,
Peter
> +} arm_sp810_state;
> +
> +#endif
> --
> 1.8.3.2
>
>