Patchwork [v2,1/1] integrator: fix Linux boot failure by emulating dbg

login
register
mail settings
Submitter alex.bennee@linaro.org
Date Sept. 16, 2013, 12:50 p.m.
Message ID <1379335841-13391-2-git-send-email-alex.bennee@linaro.org>
Download mbox | patch
Permalink /patch/275213/
State New
Headers show

Comments

alex.bennee@linaro.org - Sept. 16, 2013, 12:50 p.m.
From: Alex Bennée <alex@bennee.com>

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).

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 the
behaviour of this region should be so for now it simply returns 0's as
the old unassigned_mem_read did.

Signed-off-by: Alex Bennée <alex@bennee.com>
---
 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
Peter Maydell - Oct. 15, 2013, 1:21 p.m.
On 16 September 2013 13:50,  <alex.bennee@linaro.org> wrote:
> From: Alex Bennée <alex@bennee.com>
>
> 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).

This generally looks OK, but I have a few nits.

> 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 the
> behaviour of this region should be so for now it simply returns 0's as
> the old unassigned_mem_read did.

It looks like you need to update the commit message -- you
looked at the board docs, so we do know the correct behaviour.

> +++ 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.

This sounds like the code was written based on the kernel, not
on the board docs, which is wrong, so I think it could use
rephrasing. There's no need to mention previous behaviour either
IMHO -- people who care can look at the commit history.

> + *
> + * The real h/w is described at:
> + *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
> + *
> + * Written by Alex Bennée

Assuming you wrote this with your Linaro hat on, the comment
should have a
 * Copyright (c) 2013 Linaro Limited

in it above your 'Written by' line.

> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */

> +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 %lx to bad offset %x\n", value, (int)offset);

This looks like an overlength line : checkpatch.pl should
warn if so. (I think there are others too.)

thanks
-- PMM
alex.bennee@linaro.org - Oct. 15, 2013, 2:25 p.m.
peter.maydell@linaro.org writes:

> On 16 September 2013 13:50,  <alex.bennee@linaro.org> wrote:
>> From: Alex Bennée <alex@bennee.com>
>>
>> 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).
>
> This generally looks OK, but I have a few nits.

OK

>> 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 the
>> behaviour of this region should be so for now it simply returns 0's as
>> the old unassigned_mem_read did.
>
> It looks like you need to update the commit message -- you
> looked at the board docs, so we do know the correct behaviour.

True, I shall update.

>> +++ 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.
>
> This sounds like the code was written based on the kernel, not
> on the board docs, which is wrong, so I think it could use
> rephrasing. There's no need to mention previous behaviour either
> IMHO -- people who care can look at the commit history.

Sure. I've got a bunch of other review comments to address so I can
clean this up. Unfortunately I've been so busy with other stuff I
haven't had a chance to go through them yet.

As it happens it looks like the integrator image is now running again on
the current HEAD which has made this a little less urgent. However it
does raise an interesting question as to why!

>> + *
>> + * The real h/w is described at:
>> + *  http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0159b/Babbfijf.html
>> + *
>> + * Written by Alex Bennée
>
> Assuming you wrote this with your Linaro hat on, the comment
> should have a
>  * Copyright (c) 2013 Linaro Limited
>
> in it above your 'Written by' line.

Yeah, I was holding off that until I was officially on the clock
although as noticed I have been taking advantage of the email facilities
as a handy place to have the mailing list go through.

<snip>
>> +        /* 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 %lx to bad offset %x\n", value, (int)offset);
>
> This looks like an overlength line : checkpatch.pl should
> warn if so. (I think there are others too.)

OK thanks.
Peter Maydell - Oct. 15, 2013, 2:30 p.m.
On 15 October 2013 15:25, Alex Bennée <alex.bennee@linaro.org> wrote:
> peter.maydell@linaro.org writes:
>> This sounds like the code was written based on the kernel, not
>> on the board docs, which is wrong, so I think it could use
>> rephrasing. There's no need to mention previous behaviour either
>> IMHO -- people who care can look at the commit history.
>
> Sure. I've got a bunch of other review comments to address so I can
> clean this up. Unfortunately I've been so busy with other stuff I
> haven't had a chance to go through them yet.
>
> As it happens it looks like the integrator image is now running again on
> the current HEAD which has made this a little less urgent. However it
> does raise an interesting question as to why!

Commits 3bb28b72 and 68a7439a1 reverted the changes to the
behaviour of reads from unassigned regions (since they were
causing widespread brokenness).

>>> + * Written by Alex Bennée
>>
>> Assuming you wrote this with your Linaro hat on, the comment
>> should have a
>>  * Copyright (c) 2013 Linaro Limited
>>
>> in it above your 'Written by' line.
>
> Yeah, I was holding off that until I was officially on the clock
> although as noticed I have been taking advantage of the email facilities
> as a handy place to have the mailing list go through.

Well, it needs *some* copyright line, at least, for whichever
entity has the rights to the code and ability to (re)license it...

thanks
-- PMM

Patch

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ac0815d..a5718d1 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -80,3 +80,4 @@  CONFIG_VERSATILE_PCI=y
 CONFIG_VERSATILE_I2C=y
 
 CONFIG_SDHCI=y
+CONFIG_INTEGRATOR_DBG=y
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) += vmport.o
 
 # ARM devices
 common-obj-$(CONFIG_PL310) += arm_l2x0.o
+common-obj-$(CONFIG_INTEGRATOR_DBG) += arm_intdbg.o
 
 # PKUnity SoC devices
 common-obj-$(CONFIG_PUV3) += 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=/com.arm.doc.dui0159b/Babbfijf.html
+ *
+ * Written by Alex Bennée
+ *
+ * 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"
+#define ARM_INTDBG(obj) \
+    OBJECT_CHECK(ARMIntDbgState, (obj), TYPE_ARM_INTDBG)
+
+typedef struct {
+    SysBusDevice parent_obj;
+    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 from %x:%d\n", (int)offset, size);
+        return 0;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "dbg_control_read: Bad offset %x\n", (int)offset);
+        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 %lx to bad offset %x\n", value, (int)offset);
+    }
+}
+
+static const MemoryRegionOps dbg_control_ops = {
+    .read = dbg_control_read,
+    .write = dbg_control_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void dbg_control_init(Object *obj)
+{
+    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
+    ARMIntDbgState *s = ARM_INTDBG(obj);
+    memory_region_init_io(&s->iomem, NULL, &dbg_control_ops, NULL, "dbgleds", 0x1000000);
+    sysbus_init_mmio(sd, &s->iomem);
+}
+
+static const TypeInfo arm_intdbg_info = {
+    .name          = TYPE_ARM_INTDBG,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(ARMIntDbgState),
+    .instance_init = dbg_control_init,
+};
+
+static void arm_intdbg_register_types(void)
+{
+    type_register_static(&arm_intdbg_info);
+}
+
+type_init(arm_intdbg_register_types)