On 16 September 2013 13:50, <alex.ben...@linaro.org> wrote: > From: Alex Bennée <a...@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