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

[Peter Chubb]

>>>>> "Andreas" == Andreas Färber <address@hidden> writes:

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

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

This is a known issue with some thunderbird versions --- Mozilla/Mime
support  is a bit weird when it comes to emails with a
Content-Disposition: inline with a filename.  

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

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

This series was broken in a couple of ways.  Please ignore it! (my
laptop got restored from a backup and in moving forward again I think
some changes that were in v7 got lost, including this one).

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

I dunno what happened there.  I put that in, then the result wouldn;t
compile, so I reverted it.

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

OK.

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

OK.  Dunno why checkpatch didn't catch this.

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

OK.

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

OK.

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

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

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

I'm not sure what this means.

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

Let's get the first round in first.

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

OK.

--
Dr Peter Chubb                                  peter.chubb AT nicta.com.au
http://www.ssrg.nicta.com.au          Software Systems Research Group/NICTA