On Fri, Dec 13, 2019 at 1:09 AM Philippe Mathieu-Daudé wrote: > On 12/2/19 10:09 PM, Niek Linnenbank wrote: > > The Allwinner H3 System on Chip has an System Control > > module that provides system wide generic controls and > > device information. This commit adds support for the > > Allwinner H3 System Control module. > > > > Signed-off-by: Niek Linnenbank > > --- > > hw/arm/allwinner-h3.c | 11 ++ > > hw/misc/Makefile.objs | 1 + > > hw/misc/allwinner-h3-syscon.c | 139 ++++++++++++++++++++++++++ > > include/hw/arm/allwinner-h3.h | 2 + > > include/hw/misc/allwinner-h3-syscon.h | 43 ++++++++ > > 5 files changed, 196 insertions(+) > > create mode 100644 hw/misc/allwinner-h3-syscon.c > > create mode 100644 include/hw/misc/allwinner-h3-syscon.h > > > > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c > > index afeb49c0ac..ebd8fde412 100644 > > --- a/hw/arm/allwinner-h3.c > > +++ b/hw/arm/allwinner-h3.c > > @@ -41,6 +41,9 @@ static void aw_h3_init(Object *obj) > > > > sysbus_init_child_obj(obj, "ccu", &s->ccu, sizeof(s->ccu), > > TYPE_AW_H3_CLK); > > + > > + sysbus_init_child_obj(obj, "syscon", &s->syscon, sizeof(s->syscon), > > + TYPE_AW_H3_SYSCON); > > } > > > > static void aw_h3_realize(DeviceState *dev, Error **errp) > > @@ -184,6 +187,14 @@ static void aw_h3_realize(DeviceState *dev, Error > **errp) > > } > > sysbus_mmio_map(SYS_BUS_DEVICE(&s->ccu), 0, AW_H3_CCU_BASE); > > > > + /* System Control */ > > + object_property_set_bool(OBJECT(&s->syscon), true, "realized", > &err); > > + if (err) { > > + error_propagate(errp, err); > > + return; > > + } > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->syscon), 0, AW_H3_SYSCON_BASE); > > + > > /* Universal Serial Bus */ > > sysbus_create_simple(TYPE_AW_H3_EHCI, AW_H3_EHCI0_BASE, > > s->irq[AW_H3_GIC_SPI_EHCI0]); > > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs > > index 200ed44ce1..b234aefba5 100644 > > --- a/hw/misc/Makefile.objs > > +++ b/hw/misc/Makefile.objs > > @@ -29,6 +29,7 @@ common-obj-$(CONFIG_MACIO) += macio/ > > common-obj-$(CONFIG_IVSHMEM_DEVICE) += ivshmem.o > > > > common-obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-clk.o > > +common-obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-syscon.o > > common-obj-$(CONFIG_REALVIEW) += arm_sysctl.o > > common-obj-$(CONFIG_NSERIES) += cbus.o > > common-obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o > > diff --git a/hw/misc/allwinner-h3-syscon.c > b/hw/misc/allwinner-h3-syscon.c > > new file mode 100644 > > index 0000000000..66bd518a05 > > --- /dev/null > > +++ b/hw/misc/allwinner-h3-syscon.c > > @@ -0,0 +1,139 @@ > > +/* > > + * Allwinner H3 System Control emulation > > + * > > + * Copyright (C) 2019 Niek Linnenbank > > + * > > + * 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 >. > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "hw/sysbus.h" > > +#include "migration/vmstate.h" > > +#include "qemu/log.h" > > +#include "qemu/module.h" > > +#include "hw/misc/allwinner-h3-syscon.h" > > + > > +/* SYSCON register offsets */ > > +#define REG_VER (0x24) /* Version */ > > +#define REG_EMAC_PHY_CLK (0x30) /* EMAC PHY Clock */ > > +#define REG_INDEX(offset) (offset / sizeof(uint32_t)) > > + > > +/* SYSCON register reset values */ > > +#define REG_VER_RST (0x0) > > +#define REG_EMAC_PHY_CLK_RST (0x58000) > > + > > +static uint64_t allwinner_h3_syscon_read(void *opaque, hwaddr offset, > > + unsigned size) > > +{ > > + const AwH3SysconState *s = (AwH3SysconState *)opaque; > > + const uint32_t idx = REG_INDEX(offset); > > + > > + if (idx >= AW_H3_SYSCON_REGS_NUM) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: bad read offset 0x%04x\n", > > + __func__, (uint32_t)offset); > > + return 0; > > + } > > + > > + return s->regs[idx]; > > +} > > + > > +static void allwinner_h3_syscon_write(void *opaque, hwaddr offset, > > + uint64_t val, unsigned size) > > +{ > > + AwH3SysconState *s = (AwH3SysconState *)opaque; > > + const uint32_t idx = REG_INDEX(offset); > > + > > + if (idx >= AW_H3_SYSCON_REGS_NUM) { > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: bad write offset 0x%04x\n", > > + __func__, (uint32_t)offset); > > + return; > > + } > > + > > + switch (offset) { > > + case REG_VER: /* Version */ > > + break; > > + default: > > + s->regs[idx] = (uint32_t) val; > > + break; > > + } > > +} > > + > > +static const MemoryRegionOps allwinner_h3_syscon_ops = { > > + .read = allwinner_h3_syscon_read, > > + .write = allwinner_h3_syscon_write, > > + .endianness = DEVICE_NATIVE_ENDIAN, > > + .valid = { > > + .min_access_size = 4, > > + .max_access_size = 4, > > Can you point me to the datasheet page that says this region is > restricted to 32-bit accesses? Maybe you want .valid -> .impl instead? > > Hehe well here I can only give the same answer as for the SD/MMC driver: the datasheet only provides the base address and register offsets, but nothing explicitely mentioned about alignment. I do see that also for this device the registers are 32-bit aligned. Does that mean I should change MemoryRegionOps to . impl instead? > > + .unaligned = false > > + } > > +}; > > + > > +static void allwinner_h3_syscon_reset(DeviceState *dev) > > +{ > > + AwH3SysconState *s = AW_H3_SYSCON(dev); > > + > > + /* Set default values for registers */ > > + s->regs[REG_INDEX(REG_VER)] = REG_VER_RST; > > + s->regs[REG_INDEX(REG_EMAC_PHY_CLK)] = REG_EMAC_PHY_CLK_RST; > > +} > > + > > +static void allwinner_h3_syscon_realize(DeviceState *dev, Error **errp) > > +{ > > +} > > + > > +static void allwinner_h3_syscon_init(Object *obj) > > +{ > > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > > + AwH3SysconState *s = AW_H3_SYSCON(obj); > > + > > + /* Memory mapping */ > > + memory_region_init_io(&s->iomem, OBJECT(s), > &allwinner_h3_syscon_ops, s, > > + TYPE_AW_H3_SYSCON, > AW_H3_SYSCON_REGS_MEM_SIZE); > > This definition isn't very helpful IMO, I'd use the value in place: '4 * > KiB'. > OK, I'll apply that too in the other drivers. > > > + sysbus_init_mmio(sbd, &s->iomem); > > +} > > + > > +static const VMStateDescription allwinner_h3_syscon_vmstate = { > > + .name = TYPE_AW_H3_SYSCON, > > Plain name. > > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .fields = (VMStateField[]) { > > + VMSTATE_UINT32_ARRAY(regs, AwH3SysconState, > AW_H3_SYSCON_REGS_NUM), > > + VMSTATE_END_OF_LIST() > > + } > > +}; > > + > > +static void allwinner_h3_syscon_class_init(ObjectClass *klass, void > *data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + > > + dc->reset = allwinner_h3_syscon_reset; > > + dc->realize = allwinner_h3_syscon_realize; > > + dc->vmsd = &allwinner_h3_syscon_vmstate; > > +} > > + > > +static const TypeInfo allwinner_h3_syscon_info = { > > + .name = TYPE_AW_H3_SYSCON, > > + .parent = TYPE_SYS_BUS_DEVICE, > > + .instance_init = allwinner_h3_syscon_init, > > + .instance_size = sizeof(AwH3SysconState), > > + .class_init = allwinner_h3_syscon_class_init, > > +}; > > + > > +static void allwinner_h3_syscon_register(void) > > +{ > > + type_register_static(&allwinner_h3_syscon_info); > > +} > > + > > +type_init(allwinner_h3_syscon_register) > > diff --git a/include/hw/arm/allwinner-h3.h > b/include/hw/arm/allwinner-h3.h > > index e596516c5c..2bc526b77b 100644 > > --- a/include/hw/arm/allwinner-h3.h > > +++ b/include/hw/arm/allwinner-h3.h > > @@ -27,6 +27,7 @@ > > #include "hw/timer/allwinner-a10-pit.h" > > #include "hw/intc/arm_gic.h" > > #include "hw/misc/allwinner-h3-clk.h" > > +#include "hw/misc/allwinner-h3-syscon.h" > > #include "target/arm/cpu.h" > > > > #define AW_H3_SRAM_A1_BASE (0x00000000) > > @@ -111,6 +112,7 @@ typedef struct AwH3State { > > qemu_irq irq[AW_H3_GIC_NUM_SPI]; > > AwA10PITState timer; > > AwH3ClockState ccu; > > + AwH3SysconState syscon; > > GICState gic; > > MemoryRegion sram_a1; > > MemoryRegion sram_a2; > > diff --git a/include/hw/misc/allwinner-h3-syscon.h > b/include/hw/misc/allwinner-h3-syscon.h > > new file mode 100644 > > index 0000000000..22a2f2a11b > > --- /dev/null > > +++ b/include/hw/misc/allwinner-h3-syscon.h > > @@ -0,0 +1,43 @@ > > +/* > > + * Allwinner H3 System Control emulation > > + * > > + * Copyright (C) 2019 Niek Linnenbank > > + * > > + * 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 >. > > + */ > > + > > +#ifndef HW_MISC_ALLWINNER_H3_SYSCON_H > > +#define HW_MISC_ALLWINNER_H3_SYSCON_H > > + > > +#include "hw/sysbus.h" > > + > > +#define AW_H3_SYSCON_REGS_MAX_ADDR (0x30) > > +#define AW_H3_SYSCON_REGS_NUM ((AW_H3_SYSCON_REGS_MAX_ADDR / \ > > + sizeof(uint32_t)) + 1) > > +#define AW_H3_SYSCON_REGS_MEM_SIZE (1024) > > "4.1. Memory Mapping" the System Control is 4KiB, isn't it? > Correct, I made a mistake there. Thanks, I'll change it and re-check the other files as well. > > > + > > +#define TYPE_AW_H3_SYSCON "allwinner-h3-syscon" > > +#define AW_H3_SYSCON(obj) OBJECT_CHECK(AwH3SysconState, (obj), \ > > + TYPE_AW_H3_SYSCON) > > + > > +typedef struct AwH3SysconState { > > + /*< private >*/ > > + SysBusDevice parent_obj; > > + /*< public >*/ > > + > > + MemoryRegion iomem; > > + uint32_t regs[AW_H3_SYSCON_REGS_NUM]; > > +} AwH3SysconState; > > + > > +#endif > > > > -- Niek Linnenbank