Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg

[Peter Maydell]

On 16 September 2013 13:50,  <address@hidden> wrote:
> From: Alex Bennée <address@hidden>
>
> 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