| Message ID | 1382096703-31938-2-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 18 October 2013 12:45, <alex.bennee@linaro.org> wrote: > +typedef struct { > + SysBusDevice parent_obj; > + > + MemoryRegion iomem; > + > + uint32_t alpha; > + uint32_t leds; > + uint32_t switches; These three fields are never used, or did I miss something? > +} IntegratorDebugState; Looks good otherwise. thanks -- PMM
peter.maydell@linaro.org writes: > On 18 October 2013 12:45, <alex.bennee@linaro.org> wrote: >> +typedef struct { >> + SysBusDevice parent_obj; >> + >> + MemoryRegion iomem; >> + >> + uint32_t alpha; >> + uint32_t leds; >> + uint32_t switches; > > These three fields are never used, or did I miss something? Nope, I obviously had thoughts of tracking the actual LED/Switch state and never added it in. > >> +} IntegratorDebugState; > > Looks good otherwise. > > thanks > -- PMM
On 18 October 2013 12:45, <alex.bennee@linaro.org> wrote: > From: Alex Bennée <alex@bennee.com> > > Commit 9b8c69243 (since reverted) 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). > > Relying on a varying implementation detail is incorrect anyway so this > introduces a basic stub of a memory region for the debug/LED section > on the integrator board. > > Signed-off-by: Alex Bennée <alex@bennee.com> > --- > hw/arm/Makefile.objs | 3 +- > hw/arm/integrator_debug.c | 103 ++++++++++++++++++++++++++++++++++++++ > hw/arm/integratorcp.c | 2 + > include/hw/arm/integrator_debug.h | 18 +++++++ > 4 files changed, 125 insertions(+), 1 deletion(-) > create mode 100644 hw/arm/integrator_debug.c > create mode 100644 include/hw/arm/integrator_debug.h > > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs > index 3671b42..ffdf7ee 100644 > --- a/hw/arm/Makefile.objs > +++ b/hw/arm/Makefile.objs > @@ -1,5 +1,6 @@ > obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o > -obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o > +obj-y += integratorcp.o integrator_debug.o > +obj-y += kzm.o mainstone.o musicpal.o nseries.o > obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o > obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o > > diff --git a/hw/arm/integrator_debug.c b/hw/arm/integrator_debug.c > new file mode 100644 > index 0000000..c9dd7f0 > --- /dev/null > +++ b/hw/arm/integrator_debug.c Oh, and this belongs in hw/misc/, since it's a standalone device model. thanks -- PMM
peter.maydell@linaro.org writes: > On 18 October 2013 12:45, <alex.bennee@linaro.org> wrote: >> From: Alex Bennée <alex@bennee.com> >> >> Commit 9b8c69243 (since reverted) 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). >> >> Relying on a varying implementation detail is incorrect anyway so this >> introduces a basic stub of a memory region for the debug/LED section >> on the integrator board. >> >> Signed-off-by: Alex Bennée <alex@bennee.com> >> --- >> hw/arm/Makefile.objs | 3 +- >> hw/arm/integrator_debug.c | 103 ++++++++++++++++++++++++++++++++++++++ <snip> > > Oh, and this belongs in hw/misc/, since it's a standalone > device model. <snip> Ahh I was pondering this. Surely as it's only associated with ARM (and specifically integrator) it gets grouped with that? I was following up on Andreas' comments and deciding between: hw/misc/arm_integrator_debug.c with include/hw/misc/arm_integrator_debug.c and what I went with. The existing devices are a mix of inline ones in integratorcp.c and integrator_pit which is in hw/timer/arm_timer.c. Perhaps there should be an addendum to HACKING.txt or a doc/src_layout.txt?
On 18 October 2013 14:19, Alex Bennée <alex.bennee@linaro.org> wrote: > > peter.maydell@linaro.org writes: >> Oh, and this belongs in hw/misc/, since it's a standalone >> device model. > Ahh I was pondering this. Surely as it's only associated with ARM (and > specifically integrator) it gets grouped with that? No, the only things in hw/arm are: * board models * random bits of code like the boot code which aren't devices * a few legacy "still in the same file as the board model" devices; ideally we'll split these out at some point Anything that's a freestanding qdev device model goes elsewhere. -- PMM
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs index 3671b42..ffdf7ee 100644 --- a/hw/arm/Makefile.objs +++ b/hw/arm/Makefile.objs @@ -1,5 +1,6 @@ obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o -obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o +obj-y += integratorcp.o integrator_debug.o +obj-y += kzm.o mainstone.o musicpal.o nseries.o obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o obj-y += tosa.o versatilepb.o vexpress.o xilinx_zynq.o z2.o diff --git a/hw/arm/integrator_debug.c b/hw/arm/integrator_debug.c new file mode 100644 index 0000000..c9dd7f0 --- /dev/null +++ b/hw/arm/integrator_debug.c @@ -0,0 +1,103 @@ +/* + * LED, Switch and Debug control registers for ARM Integrator Boards + * + * This is currently a stub for this functionality but at least + * ensures something other than unassigned_mem_read() handles access + * to this area. + * + * The real h/w is described at: + * http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html + * + * Copyright (c) 2013 Alex Bennée <alex@bennee.com> + * + * 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" +#include "hw/arm/integrator_debug.h" + +#define INTEGRATOR_DEBUG(obj) \ + OBJECT_CHECK(IntegratorDebugState, (obj), TYPE_INTEGRATOR_DEBUG) + +typedef struct { + SysBusDevice parent_obj; + + MemoryRegion iomem; + + uint32_t alpha; + uint32_t leds; + uint32_t switches; +} IntegratorDebugState; + +static uint64_t intdbg_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, + "%s: returning zero from %" HWADDR_PRIx ":%u\n", + __func__, offset, size); + return 0; + default: + qemu_log_mask(LOG_GUEST_ERROR, + "%s: Bad offset %" HWADDR_PRIx, + __func__, offset); + return 0; + } +} + +static void intdbg_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, + "%s: ignoring write of %" PRIu64 + " to %" HWADDR_PRIx ":%u\n", + __func__, value, offset, size); + break; + default: + qemu_log_mask(LOG_GUEST_ERROR, + "%s: write of %" PRIu64 + " to bad offset %" HWADDR_PRIx "\n", + __func__, value, offset); + } +} + +static const MemoryRegionOps intdbg_control_ops = { + .read = intdbg_control_read, + .write = intdbg_control_write, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static void intdbg_control_init(Object *obj) +{ + SysBusDevice *sd = SYS_BUS_DEVICE(obj); + IntegratorDebugState *s = INTEGRATOR_DEBUG(obj); + + memory_region_init_io(&s->iomem, NULL, &intdbg_control_ops, + NULL, "dbg-leds", 0x1000000); + sysbus_init_mmio(sd, &s->iomem); +} + +static const TypeInfo intdbg_info = { + .name = TYPE_INTEGRATOR_DEBUG, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(IntegratorDebugState), + .instance_init = intdbg_control_init, +}; + +static void intdbg_register_types(void) +{ + type_register_static(&intdbg_info); +} + +type_init(intdbg_register_types) diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c index 2ef93ed..0b2ac2c 100644 --- a/hw/arm/integratorcp.c +++ b/hw/arm/integratorcp.c @@ -11,6 +11,7 @@ #include "hw/devices.h" #include "hw/boards.h" #include "hw/arm/arm.h" +#include "hw/arm/integrator_debug.h" #include "net/net.h" #include "exec/address-spaces.h" #include "sysemu/sysemu.h" @@ -508,6 +509,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(TYPE_INTEGRATOR_DEBUG, 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/include/hw/arm/integrator_debug.h b/include/hw/arm/integrator_debug.h new file mode 100644 index 0000000..37789b6 --- /dev/null +++ b/include/hw/arm/integrator_debug.h @@ -0,0 +1,18 @@ +/* + * ARM Integrator Board Debug, switch and LED section + * + * Browse the data sheet: + * + * http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html + * + * Copyright (c) 2013 Alex Bennée <alex@bennee.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ +#ifndef QEMU_INTEGRATOR_DEBUG_H +#define QEMU_INTEGRATOR_DEBUG_H + +#define TYPE_INTEGRATOR_DEBUG "integrator_debug" + +#endif