Re: [Qemu-devel] [patch v8 0/5] i.MX31 support

[Andreas Färber]

Hello Peter,

Am 06.06.2012 05:47, schrieb Peter Chubb:
> There are no major changes since last time, just rebased to current
> tip now that  QEMU 1.2 is open.
> 
> For those who have come into the story late, this is a series of
> patches to allow QEMU to emulate a Freescale i.MX31 on a Kyoto
> Microsystems evaluation board.  It's pretty bare-bones, but runs Linux
> and seL4 nicely.

Something is wrong with the patch submission, they are shown as
attachments and Thunderbird doesn't let me comment inline. Please use
git-send-email to submit.

Also the diffstat doesn't match the patch wrt file ordering, so it would
be advisable to use git-format-patch.

Our concept of topics in the subject seems to be troubling you, you have
added " support" since v7 (still readable, so I'm okay) whereas what we
usually use is a lower-case tag as described here plus an active
description of what it's doing (e.g., "foo: Add/Introduce bar"):
https://live.gnome.org/Git/CommitMessages

On patch 5:
Please use cpu_arm_init() in place of cpu_init() and prefer ARMCPU. This
is already in qemu.git and many more conversions are in the QOM CPUState
part 3 PULL and on qom-next.

Also note that arm_pic_init_cpu() and arm_load_kernel() are being
changed to take that ARMCPU, so this needs to be coordinated.

Please remove the semicolon after machine_init() and check the
indentation. After sysbus_create_varargs() for instance I spot one space
too few and below for imx_serial_create() there's a double space.

Please also make all your TypeInfos static const, probably same for
QEMUMachine.

The description sounds misleading: In qemu-system-arm all boards are ARM
architecture, and your wording may sound as if the board were from ARM
(Holdings plc) when it is from Kyoto Micro and uses a Freescale SoC.
Maybe also mention the exact board name from the commit message and
mention i.MX31?

s/I.MX31/i.MX31/ in the header?

On patch 4:
There's an empty line after type_init(), please remove.

On patch 3:
In IMXTimerGState you're saving DeviceState *ccm. That should probably
become a QOM link<> property, possibly after the initial submission.
CC'ing Paolo.

What should be considered as a second step is factoring out all the
devices individually added to the board into an i.MX31 SoC, which has
implications on how the devices are initialized (compare my recent
prep_pci and Anthony's i440fx patches). For a less sophisticated way
using functions check out exynos4210. The way it's done right now
there's no distinction of what is on the SoC and what is on the board,
so it needs to be done by you.

Missing is an entry to MAINTAINERS for this board and its exclusive
devices, naming who is to be cc'ed on patches.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg