From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57528) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VW5YZ-0000LT-N1 for qemu-devel@nongnu.org; Tue, 15 Oct 2013 10:25:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VW5YU-0007f0-NM for qemu-devel@nongnu.org; Tue, 15 Oct 2013 10:25:07 -0400 Received: from static.88-198-71-155.clients.your-server.de ([88.198.71.155]:48435 helo=socrates.bennee.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VW5YU-0007as-Gv for qemu-devel@nongnu.org; Tue, 15 Oct 2013 10:25:02 -0400 References: <1379335841-13391-1-git-send-email-alex.bennee@linaro.org> <1379335841-13391-2-git-send-email-alex.bennee@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Tue, 15 Oct 2013 15:25:01 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 1/1] integrator: fix Linux boot failure by emulating dbg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers peter.maydell@linaro.org writes: > On 16 September 2013 13:50, wrote: >> From: Alex Bennée >> >> 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. >> + /* 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. -- Alex Bennée