[Qemu-devel] [PATCH v3 0/1] integrator: fix Linux boot failure

[Qemu-devel] [PATCH v3 0/1] integrator: fix Linux boot failure

[alex.bennee]

>From Alex Bennée <alex.bennee@linaro.org> # This line is ignored.
Hi,

I finally got a chance to follow up on the review comments from Peter
and Andreas.

Changes for v3:

* Moved into hw/arm/integrator_debug.c
* Expanded QOM symbol to INTEGRATOR_DEBUG, moved to header
* Use __func__, HWADDR_PRIx and PRIu64 for debug output
* Add copyright statements
* Fixed line lengths

Cheers,

Alex.

[Qemu-devel] [PATCH v3] integrator: fix Linux boot failure by emulating dbg region

[alex.bennee]

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
@@ -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
-- 
1.8.4

Re: [Qemu-devel] [PATCH v3] integrator: fix Linux boot failure by emulating dbg region

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v3] integrator: fix Linux boot failure by emulating dbg region

[Alex Bennée]

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


-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3] integrator: fix Linux boot failure by emulating dbg region

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v3] integrator: fix Linux boot failure by emulating dbg region

[Alex Bennée]

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?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v3] integrator: fix Linux boot failure by emulating dbg region

[Peter Maydell]

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