From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59585) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XbtK9-0003p6-DL for qemu-devel@nongnu.org; Wed, 08 Oct 2014 11:38:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XbtK3-0007ak-UH for qemu-devel@nongnu.org; Wed, 08 Oct 2014 11:38:45 -0400 Received: from cantor2.suse.de ([195.135.220.15]:60454 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XbtK3-0007aQ-Kp for qemu-devel@nongnu.org; Wed, 08 Oct 2014 11:38:39 -0400 Message-ID: <54355A7C.8060007@suse.de> Date: Wed, 08 Oct 2014 17:38:36 +0200 From: =?ISO-8859-15?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1412777966-28286-1-git-send-email-chouteau@adacore.com> In-Reply-To: <1412777966-28286-1-git-send-email-chouteau@adacore.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH][SPARC] LEON3: Add emulation of AMBA plug&play List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, mark.cave-ayland@ilande.co.uk, Jiri Gaisler Hi, Am 08.10.2014 um 16:19 schrieb Fabien Chouteau: > From: Jiri Gaisler >=20 > 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. >=20 > Signed-off-by: Fabien Chouteau > --- > 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 >=20 > 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 +=3D sun4m.o leon3.o > +obj-$(CONFIG_GRLIB) +=3D 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 obtaini= ng a copy > + * of this software and associated documentation files (the "Software"= ), to deal > + * in the Software without restriction, including without limitation t= he rights > + * to use, copy, modify, merge, publish, distribute, sublicense, and/o= r 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 incl= uded in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXP= RESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABI= LITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT S= HALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES O= R OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARI= SING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALI= NGS 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 &=3D 0xfff; > + > + /* Unit registers */ > + switch (addr & 0xffc) { > + case 0x00: > + read_data =3D 0x0400f000; /* Memory controller */ > + break; > + case 0x04: > + read_data =3D 0x0000fff1; > + break; > + case 0x08: > + read_data =3D 0x0100c023; /* APBUART */ > + break; > + case 0x0C: > + read_data =3D 0x0010fff1; > + break; > + case 0x10: > + read_data =3D 0x0100d040; /* IRQMP */ > + break; > + case 0x14: > + read_data =3D 0x0020fff1; > + break; > + case 0x18: > + read_data =3D 0x01011006; /* GPTIMER */ > + break; > + case 0x1C: > + read_data =3D 0x0030fff1; > + break; > + > + default: > + read_data =3D 0; > + } > + if (size =3D=3D 1) { > + read_data >>=3D (24 - (addr & 3) * 8); > + read_data &=3D 0x0ff; > + } > + return read_data; > +} > + > +static void grlib_apbpnp_write(void *opaque, hwaddr addr, > + uint64_t value, unsigned size) > +{ > +} > + > +static const MemoryRegionOps grlib_apbpnp_ops =3D { > + .write =3D grlib_apbpnp_write, > + .read =3D grlib_apbpnp_read, > + .endianness =3D DEVICE_NATIVE_ENDIAN, > +}; > + > +static int grlib_apbpnp_init(SysBusDevice *dev) > +{ > + APBPNP *pnp =3D 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 =3D SYS_BUS_DEVICE_CLASS(klass); sdc? > + > + k->init =3D grlib_apbpnp_init; > +} > + > +static const TypeInfo grlib_apbpnp_info =3D { > + .name =3D TYPE_GRLIB_APB_PNP, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .instance_size =3D sizeof(APBPNP), > + .class_init =3D 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 &=3D 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 =3D { > + .write =3D grlib_ahbpnp_write, > + .read =3D grlib_ahbpnp_read, > + .endianness =3D 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 =3D 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 =3D SYS_BUS_DEVICE_CLASS(klass); > + > + k->init =3D grlib_ahbpnp_init; > +} > + > +static const TypeInfo grlib_ahbpnp_info =3D { > + .name =3D TYPE_GRLIB_AHB_PNP, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .instance_size =3D sizeof(AHBPNP), > + .class_init =3D 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 *ma= chine) > } > } > =20 > + /* 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); > =20 > 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; > } > =20 > +/* APB PNP */ > + > +static inline > +DeviceState *grlib_apbpnp_create(hwaddr base) > +{ > + DeviceState *dev; > + > + dev =3D 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 =3D 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg