From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50204) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VMK57-00015N-Df for qemu-devel@nongnu.org; Wed, 18 Sep 2013 11:54:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VMK52-0003Ox-El for qemu-devel@nongnu.org; Wed, 18 Sep 2013 11:54:21 -0400 Received: from cantor2.suse.de ([195.135.220.15]:51449 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VMK51-0003Nl-OX for qemu-devel@nongnu.org; Wed, 18 Sep 2013 11:54:16 -0400 Message-ID: <5239CCA1.8080703@suse.de> Date: Wed, 18 Sep 2013 17:54:09 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1379514703-21537-1-git-send-email-alex.bennee@linaro.org> <1379514703-21537-2-git-send-email-alex.bennee@linaro.org> In-Reply-To: <1379514703-21537-2-git-send-email-alex.bennee@linaro.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: alex.bennee@linaro.org Cc: peter.maydell@linaro.org, qemu-devel@nongnu.org, paul@codesourcery.com Am 18.09.2013 16:31, schrieb alex.bennee@linaro.org: > From: Alex Benn=C3=A9e >=20 > Commit 9b8c69243 broke the ability to boot the kernel as the value > returned by unassigned_mem_read returned non-zero and left the kernel > looping forever waiting for it to change (see integrator_led_set in > the kernel code). >=20 > Relying on a varying implementation detail is incorrect anyway so this > introduces a memory region to emulate the debug/led region on the > integrator board. It is currently a basic stub as I have no idea what t= he > behaviour of this region should be so for now it simply returns 0's as > the old unassigned_mem_read did. >=20 > Signed-off-by: Alex Benn=C3=A9e > --- > default-configs/arm-softmmu.mak | 1 + > hw/arm/integratorcp.c | 1 + > hw/misc/Makefile.objs | 1 + > hw/misc/arm_intdbg.c | 90 +++++++++++++++++++++++++++++++++= ++++++++ > 4 files changed, 93 insertions(+) > create mode 100644 hw/misc/arm_intdbg.c Looks okay in general, some minor nits below: > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-soft= mmu.mak > index ac0815d..a5718d1 100644 > --- a/default-configs/arm-softmmu.mak > +++ b/default-configs/arm-softmmu.mak > @@ -80,3 +80,4 @@ CONFIG_VERSATILE_PCI=3Dy > CONFIG_VERSATILE_I2C=3Dy > =20 > CONFIG_SDHCI=3Dy > +CONFIG_INTEGRATOR_DBG=3Dy > diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c > index 2ef93ed..46dc615 100644 > --- a/hw/arm/integratorcp.c > +++ b/hw/arm/integratorcp.c > @@ -508,6 +508,7 @@ static void integratorcp_init(QEMUMachineInitArgs *= args) > icp_control_init(0xcb000000); > sysbus_create_simple("pl050_keyboard", 0x18000000, pic[3]); > sysbus_create_simple("pl050_mouse", 0x19000000, pic[4]); > + sysbus_create_simple("integrator_dbg", 0x1a000000, 0); > sysbus_create_varargs("pl181", 0x1c000000, pic[23], pic[24], NULL)= ; > if (nd_table[0].used) > smc91c111_init(&nd_table[0], 0xc8000000, pic[27]); > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs > index 2578e29..be284f3 100644 > --- a/hw/misc/Makefile.objs > +++ b/hw/misc/Makefile.objs > @@ -10,6 +10,7 @@ obj-$(CONFIG_VMPORT) +=3D vmport.o > =20 > # ARM devices > common-obj-$(CONFIG_PL310) +=3D arm_l2x0.o > +common-obj-$(CONFIG_INTEGRATOR_DBG) +=3D arm_intdbg.o > =20 > # PKUnity SoC devices > common-obj-$(CONFIG_PUV3) +=3D puv3_pm.o > diff --git a/hw/misc/arm_intdbg.c b/hw/misc/arm_intdbg.c > new file mode 100644 > index 0000000..b505d09 > --- /dev/null > +++ b/hw/misc/arm_intdbg.c > @@ -0,0 +1,90 @@ > +/* > + * LED, Switch and Debug control registers for ARM Integrator Boards > + * > + * This currently is a stub for this functionality written with > + * reference to what the Linux kernel looks at. Previously we relied > + * on the behaviour of unassigned_mem_read() in the core. > + * > + * The real h/w is described at: > + * http://infocenter.arm.com/help/index.jsp?topic=3D/com.arm.doc.dui0= 159b/Babbfijf.html > + * > + * Written by Alex Benn=C3=A9e > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or = later. > + * See the COPYING file in the top-level directory. > + */ > + > +#include "hw/hw.h" > +#include "hw/sysbus.h" > +#include "exec/address-spaces.h" > + > +#define TYPE_ARM_INTDBG "integrator_dbg" If you move this constant into an include/hw/misc/arm_integratorcp_dbg.h then you can reuse it in hw/arm/integratorcp.c above. (Optional.) > +#define ARM_INTDBG(obj) \ > + OBJECT_CHECK(ARMIntDbgState, (obj), TYPE_ARM_INTDBG) > + > +typedef struct { > + SysBusDevice parent_obj; Please leave an empty line here to visually separate the parent field. > + MemoryRegion iomem; > + > + uint32_t alpha; > + uint32_t leds; > + uint32_t switches; > +} ARMIntDbgState; > + > +static uint64_t dbg_control_read(void *opaque, hwaddr offset, > + unsigned size) > +{ > + switch (offset >> 2) { > + case 0: /* ALPHA */ > + case 1: /* LEDS */ > + case 2: /* SWITCHES */ > + qemu_log_mask(LOG_UNIMP, "dbg_control_read: returning zero fro= m %x:%d\n", (int)offset, size); HWADDR_PRIx, %u Also suggest %s and __func__, cf. further below. > + return 0; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_read: Bad offset %= x\n", (int)offset); HWADDR_PRIx > + return 0; > + } > +} > + > +static void dbg_control_write(void *opaque, hwaddr offset, > + uint64_t value, unsigned size) > +{ > + switch (offset >> 2) { > + case 1: /* ALPHA */ > + case 2: /* LEDS */ > + case 3: /* SWITCHES */ > + /* Nothing interesting implemented yet. */ > + qemu_log_mask(LOG_UNIMP, "dbg_control_write: ignoring write of= %lx to %x:%d\n", value, (int)offset, size); > + break; > + default: > + qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_write: write of %l= x to bad offset %x\n", value, (int)offset); > + } > +} > + > +static const MemoryRegionOps dbg_control_ops =3D { > + .read =3D dbg_control_read, > + .write =3D dbg_control_write, > + .endianness =3D DEVICE_NATIVE_ENDIAN, > +}; > + > +static void dbg_control_init(Object *obj) > +{ > + SysBusDevice *sd =3D SYS_BUS_DEVICE(obj); > + ARMIntDbgState *s =3D ARM_INTDBG(obj); Please leave an empty line after the variable declaration block. > + memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "db= gleds", 0x1000000); "dbg-leds"? > + sysbus_init_mmio(sd, &s->iomem); > +} > + > +static const TypeInfo arm_intdbg_info =3D { > + .name =3D TYPE_ARM_INTDBG, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .instance_size =3D sizeof(ARMIntDbgState), > + .instance_init =3D dbg_control_init, > +}; > + > +static void arm_intdbg_register_types(void) > +{ > + type_register_static(&arm_intdbg_info); > +} > + > +type_init(arm_intdbg_register_types) The naming is a bit unfortunate and inconsistent. For one, QOM types should use dashes and at least the in-code constants/macros are requested to have a verbose, readable name. Could you expand INTDBG to INTEGRATOR_DBG or INTEGRATOR_DEBUG like you have done for the Makefile? Also please use arm_intdbg_ prefix (or whatever you choose) consistently throughout the file (the idea is to have "unique" names that allow to associate __func__ debug output with its origin). Having "integrator" or "integratorcp" in the file name would also clarify that it's not for "interrupt" or "internal". ;) Similar for ARMIntDbgState - "ARMIntegratorDbgState"? Cheers, Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg