[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: |
Tue, 12 Aug 2014 17:49:12 +1000 |
On Thu, Aug 7, 2014 at 1:55 AM, Aggeler Fabian
<address@hidden> wrote:
>
> On 06 Aug 2014, at 01:03, Peter Crosthwaite <address@hidden> wrote:
>
>> 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?).
>
> Unfortunately not as it seems ARM marked it as obsolete. We found
> the document offline. I could not find it on the web either.
> See also
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/038722.html
>
> Indeed SCCTRL has many configuration bits, not just these 4 configuration
> options.
>
>>
>> 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.
>
> Okay, I will have a look at pl330 then.
>
>>
>> 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?
>
> These fields are indeed software configurable options. Apparently older
> versions
> of the V2M firmware configured these bits by default to REFCLK, which makes
> the
> SP804 run at 1MHz. On newer boards software has to set it itself to increase
> the timer
> to 1MHz, which is what Linux does (see link in my summary).
>
> I didn’t add functionality to switch the speed of the SP804 timer but if you
> could point
> me into the right direction I could look into it. Since the speed of the
> SP804 for vexpress
> boards in QEMU is fixed to 1MHz at the moment I set the bits of SCCTRL
> accordingly
> at initialisation.
>
> What do you suggest?
>
You approach of setting it to a constant is fine by me considering
there is no configurability. Actually implementing the clock frequency
changes can be follow up.
>>
>>> +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.
>
> I see, I put it there because of your previous comment. It seems I
> misunderstood
> you. Did you mean to only put qdev_create(NULL, "arm_sp810”); into the header?
>
> Which device is a good example as a reference for new devices in general?
>
Checking for recently merge series is a good idea. Allwinner and Digic
and their devs went in recently so they are reasonably fresh from
styling perspective.
Regards,
Peter
>>
>>> +typedef struct {
>>> + SysBusDevice parent_obj;
>>> + MemoryRegion iomem;
>>> +
>>> + uint32_t sc_ctrl; /* SCCTRL */
>>
>> Redundant comment.
>
> Will remove in the next version.
>
> Thanks,
> Fabian
>
>>
>> Regards,
>> Peter
>>
>>> +} arm_sp810_state;
>>> +
>>> +#endif
>>> --
>>> 1.8.3.2
>
>