Hi, Am 08.10.2014 um 16:19 schrieb Fabien Chouteau: > From: Jiri Gaisler <j...@gaisler.se> > > AMBA plug&play is used by kernels to probe available devices (Timers, > UART, etc...). This is a static declaration of devices implemented in > QEMU. In the future, a more advanced version could compute those > information directly from the device tree.
Interesting. There's quite some magic numbers in the read functions; I wonder if you could read them via QOM if you actually give the devices a canonical path or search by type? You may want to peek at ACPI code. > > Signed-off-by: Fabien Chouteau <chout...@adacore.com> > --- > hw/sparc/Makefile.objs | 1 + > hw/sparc/grlib_ambapnp.c | 206 > ++++++++++++++++++++++++++++++++++++++++++++++ > hw/sparc/leon3.c | 6 ++ > include/hw/sparc/grlib.h | 36 ++++++++ > 4 files changed, 249 insertions(+) > create mode 100644 hw/sparc/grlib_ambapnp.c > > diff --git a/hw/sparc/Makefile.objs b/hw/sparc/Makefile.objs > index c987b5b..e763701 100644 > --- a/hw/sparc/Makefile.objs > +++ b/hw/sparc/Makefile.objs > @@ -1 +1,2 @@ > obj-y += sun4m.o leon3.o > +obj-$(CONFIG_GRLIB) += grlib_ambapnp.o > diff --git a/hw/sparc/grlib_ambapnp.c b/hw/sparc/grlib_ambapnp.c > new file mode 100644 > index 0000000..dfadd5c > --- /dev/null > +++ b/hw/sparc/grlib_ambapnp.c > @@ -0,0 +1,206 @@ > +/* > + * QEMU GRLIB AMBA Plug&Play Emulator > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > deal > + * in the Software without restriction, including without limitation the > rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > + * copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > + * THE SOFTWARE. > + */ > + > +#include "hw/sysbus.h" > + > +#define APBPNP_REG_SIZE 4096 /* Size of memory mapped registers */ > + > +#define TYPE_GRLIB_APB_PNP "grlib,apbpnp" If you move the two TYPE_* constants to grlib.h, you can reuse them. > +#define GRLIB_APB_PNP(obj) \ > + OBJECT_CHECK(APBPNP, (obj), TYPE_GRLIB_APB_PNP) > + > +typedef struct APBPNP { > + SysBusDevice parent_obj; > + MemoryRegion iomem; > +} APBPNP; > + > +static uint64_t grlib_apbpnp_read(void *opaque, hwaddr addr, > + unsigned size) Indentation is off by one for all read/write functions. > +{ > + uint64_t read_data; > + addr &= 0xfff; > + > + /* Unit registers */ > + switch (addr & 0xffc) { > + case 0x00: > + read_data = 0x0400f000; /* Memory controller */ > + break; > + case 0x04: > + read_data = 0x0000fff1; > + break; > + case 0x08: > + read_data = 0x0100c023; /* APBUART */ > + break; > + case 0x0C: > + read_data = 0x0010fff1; > + break; > + case 0x10: > + read_data = 0x0100d040; /* IRQMP */ > + break; > + case 0x14: > + read_data = 0x0020fff1; > + break; > + case 0x18: > + read_data = 0x01011006; /* GPTIMER */ > + break; > + case 0x1C: > + read_data = 0x0030fff1; > + break; > + > + default: > + read_data = 0; > + } > + if (size == 1) { > + read_data >>= (24 - (addr & 3) * 8); > + read_data &= 0x0ff; > + } > + return read_data; > +} > + > +static void grlib_apbpnp_write(void *opaque, hwaddr addr, > + uint64_t value, unsigned size) > +{ > +} > + > +static const MemoryRegionOps grlib_apbpnp_ops = { > + .write = grlib_apbpnp_write, > + .read = grlib_apbpnp_read, > + .endianness = DEVICE_NATIVE_ENDIAN, > +}; > + > +static int grlib_apbpnp_init(SysBusDevice *dev) > +{ > + APBPNP *pnp = GRLIB_APB_PNP(dev); > + > + memory_region_init_io(&pnp->iomem, OBJECT(pnp), &grlib_apbpnp_ops, pnp, > + "apbpnp", APBPNP_REG_SIZE); > + > + sysbus_init_mmio(dev, &pnp->iomem); APBPNP_REG_SIZE seems constant, so you could move both lines into an instance_init function. > + > + return 0; > +} > + > +static void grlib_apbpnp_class_init(ObjectClass *klass, void *data) s/klass/oc/g > +{ > + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); sdc? > + > + k->init = grlib_apbpnp_init; > +} > + > +static const TypeInfo grlib_apbpnp_info = { > + .name = TYPE_GRLIB_APB_PNP, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(APBPNP), > + .class_init = grlib_apbpnp_class_init, > +}; > + > +static void grlib_apbpnp_register_types(void) > +{ > + type_register_static(&grlib_apbpnp_info); > +} > + > +type_init(grlib_apbpnp_register_types) Please either split into two .c files here, ... > + > + > +/* AHB PNP */ > + > +#define AHBPNP_REG_SIZE (4096 - 8) /* Size of memory mapped registers */ > + > +#define TYPE_GRLIB_AHB_PNP "grlib,ahbpnp" > +#define GRLIB_AHB_PNP(obj) \ > + OBJECT_CHECK(AHBPNP, (obj), TYPE_GRLIB_AHB_PNP) > + > +typedef struct AHBPNP { > + SysBusDevice parent_obj; > + MemoryRegion iomem; > +} AHBPNP; > + > +static uint64_t grlib_ahbpnp_read(void *opaque, hwaddr addr, > + unsigned size) > +{ > + addr &= 0xffc; > + > + /* Unit registers */ > + switch (addr) { > + case 0: > + return 0x01003000; /* LEON3 */ > + case 0x800: > + return 0x0400f000; /* Memory controller */ > + case 0x810: > + return 0x0003e002; > + case 0x814: > + return 0x2000e002; > + case 0x818: > + return 0x4003c002; > + case 0x820: > + return 0x01006000; /* APB bridge @ 0x80000000 */ > + case 0x830: > + return 0x8000fff2; > + > + default: > + return 0; > + } > +} > + > +static void grlib_ahbpnp_write(void *opaque, hwaddr addr, > + uint64_t value, unsigned size) > +{ > +} > + > +static const MemoryRegionOps grlib_ahbpnp_ops = { > + .write = grlib_ahbpnp_write, > + .read = grlib_ahbpnp_read, > + .endianness = DEVICE_NATIVE_ENDIAN, > +}; > + > +static int grlib_ahbpnp_init(SysBusDevice *dev) "dev" is an unlucky naming in case DeviceState is ever needed. sbd? busdev? > +{ > + AHBPNP *pnp = GRLIB_AHB_PNP(dev); > + > + memory_region_init_io(&pnp->iomem, OBJECT(pnp), &grlib_ahbpnp_ops, pnp, > + "ahbpnp", AHBPNP_REG_SIZE); > + > + sysbus_init_mmio(dev, &pnp->iomem); Ditto for possibly moving to instance_init. > + > + return 0; > +} > + > +static void grlib_ahbpnp_class_init(ObjectClass *klass, void *data) > +{ > + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass); > + > + k->init = grlib_ahbpnp_init; > +} > + > +static const TypeInfo grlib_ahbpnp_info = { > + .name = TYPE_GRLIB_AHB_PNP, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(AHBPNP), > + .class_init = grlib_ahbpnp_class_init, > +}; > + > +static void grlib_ahbpnp_register_types(void) > +{ > + type_register_static(&grlib_ahbpnp_info); > +} > + > +type_init(grlib_ahbpnp_register_types) ... or if unavoidable use just one type_init and registration function. > diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c > index 751392e..40de78f 100644 > --- a/hw/sparc/leon3.c > +++ b/hw/sparc/leon3.c > @@ -207,6 +207,12 @@ static void leon3_generic_hw_init(MachineState *machine) > } > } > > + /* Allocate AHB PNP */ > + grlib_ahbpnp_create(0xFFFFF000); > + > + /* Allocate APB PNP */ > + grlib_apbpnp_create(0x800FF000); > + > /* Allocate timers */ > grlib_gptimer_create(0x80000300, 2, CPU_CLK, cpu_irqs, 6); > > diff --git a/include/hw/sparc/grlib.h b/include/hw/sparc/grlib.h > index 470ce72..a004427 100644 > --- a/include/hw/sparc/grlib.h > +++ b/include/hw/sparc/grlib.h > @@ -123,4 +123,40 @@ DeviceState *grlib_apbuart_create(hwaddr base, > return dev; > } > > +/* APB PNP */ > + > +static inline > +DeviceState *grlib_apbpnp_create(hwaddr base) > +{ > + DeviceState *dev; > + > + dev = qdev_create(NULL, "grlib,apbpnp"); > + > + if (qdev_init(dev)) { > + return NULL; > + } > + > + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); > + > + return dev; > +} > + > +/* AHB PNP */ > + > +static inline > +DeviceState *grlib_ahbpnp_create(hwaddr base) > +{ > + DeviceState *dev; > + > + dev = qdev_create(NULL, "grlib,ahbpnp"); > + > + if (qdev_init(dev)) { > + return NULL; > + } > + > + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base); > + > + return dev; > +} > + > #endif /* ! _GRLIB_H_ */ Are these functions really needed? Can't you just inline them? Also note that the return value is never actually checked. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg